Skip to content
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

Generate new form data for each page load #829

Merged
merged 8 commits into from
Mar 1, 2018

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Mar 1, 2018

This PR refactors the TaxBrain form logic so that the page form data is re-generated each time a page is loaded. In order to save user input data as a JSON object instead of columns (see #822), we need to dynamically add fields to the ModelForm subclass. We must do this because the ModelForm class expects there to be a mapping from the database fields to the form fields. But, what we have is a mapping from form fields to items in a JSON object which is stored in the database more or less as a blob of data. Thus, the form fields were dynamically created from the keys in the blob of data. This was done by directly modifying the fields attribute of an instance of (subclass of?) django.ModelForms.

The previous logic created all of the form data once--on start up--in an inner class Meta. Then, if the user changed the start year, all of the data was updated according to the Tax-Calculator current_law_policy.json document. After implementing PR #822, we were updating form data in self.fields (thinking of self is in a TaxBrainForms instance) and in the self._meta.widgets object which I think was created from the Meta class. We were essentially updating two different objects with the same data. This had some strange side effects that I missed while developing and testing locally via REPL type methods (i.e. change some code, print some output, load a page or two, change some more code, etc.).

This method of development involved saving and reloading the modules pretty regularly. Once I got the desired behavior, I merged my changes, ran some tests on the test app, pushed to production, ran a few quick tests immediately after, and was quite surprised when I checked this morning and saw that all of the CPI flags were off. After much head scratching over the fact that the production app had a bug while the test app and my local instance did not, I realized that the test app and my local instance had the same bug. After much more head scratching, I realized that everything worked just fine on the test app immediately after I deployed and locally immediately after I made a change (i.e. triggered Django to save and reload all of the modules and classes). I came to the conclusion that the same data was being updated and manipulated, and it was not being reloaded as I assumed. Apparently, this is how inner classes like Meta work.

The changes in this PR do two things:

  1. Reload the form fields and widgets each time a new form is created. This will probably increase the time required to load the page, but it fixes our CPI parameter bug. I think the load-time issue can be improved in the future.
  2. Only operate on self.widgets and self.fields. I think that one of the problems was operating on the data that is either stored in different places or is the same but is accessed differently. That is accessing self.fields and self._meta.widgets separately.

@hdoupe hdoupe merged commit 96d113d into ospc-org:master Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant