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

hdoupe · 2018-01-25T20:49:51Z

Resolves #813

[EDIT] Also resolves #812

hdoupe · 2018-01-26T21:56:59Z

webapp/apps/dynamic/models.py

+            return self.tax_result
+        else:
+            return rename_keys(self.tax_result, PRE_TC_0130_RES_MAP)
+


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

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.

@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.

hdoupe · 2018-01-26T22:04:49Z

#812 was caused because I added a decorator for the tax_result variable. In doing so, I added a field _tax_result that pointed to a column in the taxbrain_taxsaveinputs table, tax_result. However, in the migrations file, you can see that the field tax_result was removed from the table and then, added back later, effectively deleting all previous information.

This PR uses a method to do the table formatting instead of a decorator. I'd be interested in hearing feedback on this approach and ideas for other approaches.

codekansas

See noted changes

codekansas · 2018-01-31T09:36:25Z

webapp/apps/dynamic/models.py

+        """
+        outputurl = OutputUrl.objects.get(unique_inputs__pk=self.pk)
+        taxcalc_vers = outputurl.taxcalc_vers
+        taxcalc_vers = taxcalc_vers.split('.')


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.

codekansas · 2018-01-31T09:45:34Z

webapp/apps/dynamic/models.py

+            return self.tax_result
+        else:
+            return rename_keys(self.tax_result, PRE_TC_0130_RES_MAP)
+


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.

codekansas · 2018-01-31T09:47:16Z

webapp/apps/taxbrain/models.py

-
-    @property
-    def tax_result(self):
+    def get_tax_result(self):


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

It is computationally light, or

It is cached (i.e. the result is only computed once)

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.

codekansas · 2018-01-31T09:47:52Z

webapp/apps/taxbrain/models.py

        """
        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('.')


See earlier comment about using distutils for version parsing instead

codekansas · 2018-01-31T09:51:30Z

webapp/apps/taxbrain/views.py

@@ -1014,7 +1014,7 @@ def csv_output(request, pk):
    filename = "taxbrain_outputs_" + suffix + ".csv"
    response['Content-Disposition'] = 'attachment; filename="' + filename + '"'


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).

codekansas · 2018-02-01T10:10:51Z

webapp/apps/dynamic/models.py

+        taxcalc_vers = outputurl.taxcalc_vers
+        # only the older (pre 0.13.0) taxcalc versions are null
+        if taxcalc_vers:
+            taxcalc_vers = LooseVersion(taxcalc_vers)


I feel like this should break something because LooseVersion isn't imported... Maybe I'm missing something

Nope, thanks for catching that. I added the import statement and a test for testing this method on the dynamic behavioral side.

codekansas · 2018-02-01T10:12:10Z

webapp/apps/dynamic/models.py

+
+        # older PB versions stored commit reference too
+        # e.g. taxcalc_vers = "0.9.0.d79abf"
+        if taxcalc_vers >= LooseVersion("0.13.0"):


You could put this in a constant somewhere, like "taxcalc_min_compat_version" or something... Might be a bit clearer where this number comes from, so that people looking at it in the future are less confused.

Done, thanks.

codekansas · 2018-02-01T10:12:43Z

webapp/apps/taxbrain/models.py

-        self._tax_result = result
+        # older PB versions stored commit reference too
+        # e.g. taxcalc_vers = "0.9.0.d79abf"
+        if taxcalc_vers >= LooseVersion("0.13.0"):


See comment above... Might also be a good idea to make a utils.py file for this commit anyway?

Ok, I moved this method to the bottom of taxbrain/helpers.py. The utility and helper functions need to be refactored and re-organzied. See issue #816

codekansas · 2018-02-03T04:59:55Z

webapp/apps/taxbrain/tests/test_helpers.py

+    assert (res["table_label_0"]["bin_0"] == [0, 1, 2] and
+            res["table_label_0"]["bin_1"] == [2, 1, 0] and
+            res["table_label_1"]["bin_0"] == [0, 1, 2] and
+            res["table_label_1"]["bin_1"] == [2, 1, 0])


Maybe this could be changed into four assert statements so it's clearer where something breaks

codekansas · 2018-02-03T05:27:37Z

webapp/apps/taxbrain/views.py

+            if error_msgs:
+                error_msg = error_msgs[0]
+            else:
+                error_msg = "Error: stack trace for this error is unavailable"
            val_err_idx = error_msg.rfind("Error")
            error = ErrorMessageTaxCalculator()
            error_contents = error_msg[val_err_idx:].replace(" ","&nbsp;")


This chunk of code seems pretty suspect... Ideally I guess there would be some easily-viewable logs, and you wouldn't need to store error messages in a database. Could to comment on the logic behind this? I.e. if the error messages aren't designed to be read by users easily, what's the purpose of storing them in the database / showing them to the user instead of logging them on the server side and presenting a more generic error message?

This error message is supposed to be a stack trace that is returned by the policy model (e.g. Tax-Calculator). When you hit previous runs that did not complete, you see the saved error message:

The stack trace is logged on the server side, too.

So, saving the error message in the database is more for the user than for us to introspect. The logs are a better source of info for that.

hdoupe · 2018-02-05T15:31:47Z

@codekansas do you have any more suggestions before #814 is merged?

codekansas · 2018-02-07T13:21:22Z

LGTM

Fix integer casting error

5b4a687

hdoupe mentioned this pull request Jan 26, 2018

Accidentally deleted tax_result column #812

Closed

Henry Doupe added 3 commits January 26, 2018 16:52

Use method instead of decorator for tax_result field

28047b8

Need to use get_tax_result for dynamic results too

69116c7

Remove migrations that remove tax_result field

7fcc713

hdoupe commented Jan 26, 2018

View reviewed changes

Account for old runs where taxcalc_vers is not stored

2b4d02f

codekansas suggested changes Jan 31, 2018

View reviewed changes

Henry Doupe added 2 commits January 31, 2018 13:23

Use distutils.version.LooseVersion for version comparison

2da32f1

Add error message if error message is not available

6fce841

codekansas reviewed Feb 1, 2018

View reviewed changes

Henry Doupe added 10 commits February 1, 2018 10:17

Fix imports and add dynamic behavoiral tests

fc6b876

Inherit test methods from TaxBrainTableResults class

21b283b

Create dynamic.test_models module and clean up imports

9a1ae6d

Access result templates as fixtures

ae28a6e

Use result template fixtures

caf6986

Move get_tax_results to taxbain/helpers.py

0d58dfb

Move store backwards incompatible version in constants.py

78d700e

Reorder value lists for diff tables made prior to tc 0.13.0

d243f19

Add docstring for reorder_lists

1f6bed8

Update test data

ad4fb24

hdoupe changed the title ~~Fix integer casting error~~ Resolve more backwards compatibility issues related to old taxcalc tables Feb 2, 2018

codekansas reviewed Feb 3, 2018

View reviewed changes

Separate assertion statements

1e90ae4

hdoupe mentioned this pull request Feb 6, 2018

Update to Django 1.9 and store parameters as JSON object #822

Merged

hdoupe merged commit 41642fe into ospc-org:master Feb 7, 2018

hdoupe mentioned this pull request Mar 13, 2018

Updates to TC tables in Tax-Calculator PR 1917 #846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hdoupe commented Jan 25, 2018 •

edited

hdoupe Jan 26, 2018

codekansas Jan 31, 2018

hdoupe Jan 31, 2018

hdoupe commented Jan 26, 2018

codekansas left a comment

codekansas Jan 31, 2018

codekansas Jan 31, 2018

codekansas Jan 31, 2018

hdoupe Jan 31, 2018

codekansas Jan 31, 2018

codekansas Jan 31, 2018

codekansas Feb 1, 2018

hdoupe Feb 1, 2018

codekansas Feb 1, 2018

hdoupe Feb 1, 2018

codekansas Feb 1, 2018

hdoupe Feb 1, 2018

codekansas Feb 3, 2018

codekansas Feb 3, 2018

hdoupe Feb 3, 2018

hdoupe commented Feb 5, 2018

codekansas commented Feb 7, 2018

		@@ -1014,7 +1014,7 @@ def csv_output(request, pk):
		filename = "taxbrain_outputs_" + suffix + ".csv"
		response['Content-Disposition'] = 'attachment; filename="' + filename + '"'

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

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

Conversation

hdoupe commented Jan 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdoupe commented Jan 26, 2018

codekansas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdoupe commented Feb 5, 2018

codekansas commented Feb 7, 2018

hdoupe commented Jan 25, 2018 •

edited