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

Fix dynamic simulation button logic for file upload #670

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Sep 27, 2017

Closes #669

@brittainhard
Copy link
Contributor

LGTM, I wonder how this got broken in the first place?

@brittainhard brittainhard merged commit 9e4c477 into ospc-org:master Oct 2, 2017
@@ -70,7 +70,6 @@ def dropq_task(year, user_mods, first_budget_year, beh_params, tax_data):
user_mods[reform_year].pop(key)
user_reform = {"policy": user_mods}
print('user_reform', user_reform, user_mods)
reform_style = [True if x else False for x in user_reform]
if beh_params:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reform_style can only be [True] since it is a dictionary with just one key policy. Further, we now pass back assumptions data with empty dictionaries even if no assumptions are set. So, this method no longer detected whether the user specified assumptions or not.

reform_file_contents = reform_file_contents.replace(" "," ")
assump_file_contents = model.json_text.assumption_text
assump_file_contents = model.json_text.raw_assumption_text
assump_file_contents = assump_file_contents.replace(" "," ")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #641, all reforms have attribute assumption_text as an assumption dictionary but the values are empty if they are not specified via an assumptions file or the dynamic behavior interface. So, raw_assumption_text was used instead to distinguish file-upload reforms with an assumptions file from other reforms.

@hdoupe
Copy link
Collaborator Author

hdoupe commented Oct 2, 2017

@brittainhard I had trouble pinning down the exact cause. But I found another bug once I dug further into the code.

The results.html file checked two different conditions before it handled the dynamic case. It set up the link to dynamic simulations: allow_dyn_links and assump_file_contents which is True if assump_file_contents is a string of length greater than one. Work in #641 and later PR's changed these attributes and they no longer indicated whether the user specified the assumptions or not.

It turned out that allow_dyn_links was always true and a link was created to a dynamic simulation even in cases where an assumptions file was uploaded and the "Dynamic Simulation" button was not displayed. Also, using the absence of assumption file contents in the java script no longer worked (still not sure why). So, I checked that condition in views.py and stored the value in allow_dyn_links. That works ok.

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.

2 participants