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.

Sign up for GitHub

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

Resolve more backwards compatibility issues related to old taxcalc tables #814

Merged
merged 18 commits into from
Feb 7, 2018
Merged

Resolve more backwards compatibility issues related to old taxcalc tables #814

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5b4a687
Fix integer casting error
Jan 25, 2018
28047b8
Use method instead of decorator for tax_result field
Jan 26, 2018
69116c7
Need to use get_tax_result for dynamic results too
Jan 26, 2018
7fcc713
Remove migrations that remove tax_result field
Jan 26, 2018
2b4d02f
Account for old runs where taxcalc_vers is not stored
Jan 29, 2018
2da32f1
Use distutils.version.LooseVersion for version comparison
Jan 31, 2018
6fce841
Add error message if error message is not available
Jan 31, 2018
fc6b876
Fix imports and add dynamic behavoiral tests
Feb 1, 2018
21b283b
Inherit test methods from TaxBrainTableResults class
Feb 1, 2018
9a1ae6d
Create dynamic.test_models module and clean up imports
Feb 1, 2018
ae28a6e
Access result templates as fixtures
Feb 1, 2018
caf6986
Use result template fixtures
Feb 1, 2018
0d58dfb
Move get_tax_results to taxbain/helpers.py
Feb 1, 2018
78d700e
Move store backwards incompatible version in constants.py
Feb 1, 2018
d243f19
Reorder value lists for diff tables made prior to tc 0.13.0
Feb 1, 2018
1f6bed8
Add docstring for reorder_lists
Feb 1, 2018
ad4fb24
Update test data
Feb 2, 2018
1e90ae4
Separate assertion statements
Feb 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions webapp/apps/dynamic/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,25 @@ class DynamicBehaviorSaveInputs(models.Model):
micro_sim = models.ForeignKey(OutputUrl, blank=True, null=True,
on_delete=models.SET_NULL)

def get_tax_result(self):
"""
If taxcalc version is greater than or equal to 0.13.0, return table
If taxcalc version is less than 0.13.0, then rename keys to new names
and then return table
"""
outputurl = OutputUrl.objects.get(unique_inputs__pk=self.pk)
taxcalc_vers = outputurl.taxcalc_vers
taxcalc_vers = taxcalc_vers.split('.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can simplify to

taxcalc_vers = outputurl.taxcalc_vers.split('.')

But probably the best solution would be to use some versioning comparison standard library, i.e. distutils.version.StrictVersion or distutils.version.LooseVersion so that you don't have to mess around with this version comparison stuff.

# older PB versions stored commit reference too
# e.g. taxcalc_vers = "0.9.0.d79abf"
if len(taxcalc_vers) >=3:
taxcalc_vers = taxcalc_vers[:3]
taxcalc_vers = tuple(map(int, taxcalc_vers))
if taxcalc_vers >= (0, 13, 0):
return self.tax_result
else:
return rename_keys(self.tax_result, PRE_TC_0130_RES_MAP)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codekansas I would prefer to not copy this method from the taxbrain models code. Do you have any ideas for a cleaner solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll probably want to put this in a utils.py file in the parent directory. Hmm... If you do this, you'll probably also want to include

from __future__ import absolute_import

in the top of the file, so that importing is consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codekansas thanks for the advice. That makes sense.

Another solution would be to create an abstract model class with this method. Then, the TaxSaveInputs and DynamicSaveInputs classes could inherit this class. It is likely that we will have to add a get_fields method in a separate PR that resolves #811. This would be like the base class approach described here. An example that is similar to our situation is described here.

class Meta:
permissions = (
("view_inputs", "Allowed to view Taxbrain."),
Expand Down
2 changes: 1 addition & 1 deletion webapp/apps/dynamic/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ def behavior_results(request, pk):
model = url.unique_inputs
if model.tax_result:

output = model.tax_result
output = model.get_tax_result()
first_year = model.first_year
created_on = model.creation_date
if 'fiscal_tots' in output:
Expand Down
9 changes: 0 additions & 9 deletions webapp/apps/taxbrain/migrations/0056_auto_20171120_2123.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ class Migration(migrations.Migration):
]

operations = [
migrations.RemoveField(
model_name='taxsaveinputs',
name='tax_result',
),
migrations.AddField(
model_name='taxsaveinputs',
name='CTC_new_for_all',
Expand Down Expand Up @@ -177,11 +173,6 @@ class Migration(migrations.Migration):
name='PT_wages_active_income',
field=models.CharField(default=b'False', max_length=50, null=True, blank=True),
),
migrations.AddField(
model_name='taxsaveinputs',
name='_tax_result',
field=jsonfield.fields.JSONField(default=None, null=True, db_column=b'tax_result', blank=True),
),
migrations.AddField(
model_name='taxsaveinputs',
name='cpi_offset',
Expand Down
21 changes: 10 additions & 11 deletions webapp/apps/taxbrain/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ class TaxSaveInputs(models.Model):
"""

# Result
_tax_result = JSONField(default=None, blank=True, null=True, db_column='tax_result')
tax_result = JSONField(default=None, blank=True, null=True)

# JSON input text
json_text = models.ForeignKey(JSONReformTaxCalculator, null=True, default=None, blank=True)
Expand All @@ -768,25 +768,24 @@ class TaxSaveInputs(models.Model):
# Creation DateTime
creation_date = models.DateTimeField(default=datetime.datetime(2015, 1, 1))


@property
def tax_result(self):
def get_tax_result(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good idea - I think the best "logic" for deciding if something should be a property is if:

  1. It is computationally light, or
  2. It is cached (i.e. the result is only computed once)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, that makes sense. Thanks for the info. I also couldn't get the property approach to work without deleting the column in the database.

"""
If taxcalc version is greater than or equal to 0.13.0, return table
If taxcalc version is less than 0.13.0, then rename keys to new names
and then return table
"""
outputurl = OutputUrl.objects.get(unique_inputs__pk=self.pk)
taxcalc_vers = outputurl.taxcalc_vers
taxcalc_vers = tuple(map(int, taxcalc_vers.split('.')))
taxcalc_vers = taxcalc_vers.split('.')
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier comment about using distutils for version parsing instead

# older PB versions stored commit reference too
# e.g. taxcalc_vers = "0.9.0.d79abf"
if len(taxcalc_vers) >=3:
taxcalc_vers = taxcalc_vers[:3]
taxcalc_vers = tuple(map(int, taxcalc_vers))
if taxcalc_vers >= (0, 13, 0):
return self._tax_result
return self.tax_result
else:
return rename_keys(self._tax_result, PRE_TC_0130_RES_MAP)

@tax_result.setter
def tax_result(self, result):
self._tax_result = result
return rename_keys(self.tax_result, PRE_TC_0130_RES_MAP)

class Meta:
permissions = (
Expand Down
6 changes: 3 additions & 3 deletions webapp/apps/taxbrain/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ def test_tc_lt_0130(self):
new_labels = json.loads(js.read())

unique_url = get_taxbrain_model(self.test_coverage_fields,
taxcalc_vers="0.10.2",
taxcalc_vers="0.10.2.abc",
webapp_vers="1.1.1")

model = unique_url.unique_inputs
model.tax_result = old_labels
model.creation_date = datetime.now()
model.save()

np.testing.assert_equal(model.tax_result, new_labels)
np.testing.assert_equal(model.get_tax_result(), new_labels)


def test_tc_gt_0130(self):
Expand All @@ -74,4 +74,4 @@ def test_tc_gt_0130(self):
model.creation_date = datetime.now()
model.save()

np.testing.assert_equal(model.tax_result, new_labels)
np.testing.assert_equal(model.get_tax_result(), new_labels)
4 changes: 2 additions & 2 deletions webapp/apps/taxbrain/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,7 @@ def add_summary_column(table):


def get_result_context(model, request, url):
output = model.tax_result
output = model.get_tax_result()
first_year = model.first_year
quick_calc = model.quick_calc
created_on = model.creation_date
Expand Down Expand Up @@ -1014,7 +1014,7 @@ def csv_output(request, pk):
filename = "taxbrain_outputs_" + suffix + ".csv"
response['Content-Disposition'] = 'attachment; filename="' + filename + '"'
Copy link
Contributor

Choose a reason for hiding this comment

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

It is probably better to use the following:

filename = 'taxbrain_outputs_{}.csv'.format(suffix)
response['Content-Disposition'] = 'attachment; filename="{}"'.format(filename)

Also: For debugging purposes, it is a good idea to log things (the response filename, for example).


results = url.unique_inputs.tax_result
results = url.unique_inputs.get_tax_result()
first_year = url.unique_inputs.first_year
csv_results = format_csv(results, pk, first_year)
writer = csv.writer(response)
Expand Down