-
-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Populate the form fields values. #1170
Conversation
@mjauvin is there any chance you could add a unit test that could cover all of the issues we've encountered with this change? (just interacting with the Form widget & some models directly) |
@LukeTowers I'd like to, but I'm not sure I know how to do this. |
@mjauvin are you able to start just by listing out all the problems we've had (starting with the first issue that caused us to want to apply it in multiple locations) describing the original issue, what should have happened, what did happen, and what we did to fix the issue? I should be able to turn those into some unit tests. |
Initial PR #927 fixed #950: PR #1099 Fixed #1036: |
@LukeTowers just realized this PR probably breaks #950 ... |
I'd really like to know why the form fields AND the model fields are set when |
@LukeTowers @bennothommo @jaxwilko this is the most promissing fix yet. If there is no objections from you guys in calling |
@mjauvin I have no objections if it works, just need to test it in a variety of contexts to make sure it doesn't break something :) |
I'll review this and see if I can build out some tests, I don't want to have to come back to this again. |
@LukeTowers I am still getting a case where having the I surrender, strongly suggest we avoid calling this as done in #1126 |
Closing this as it does not resolve the issues. |
Replaces #1126