Beginning steps for refactoring high level submission logic (retry of #894)

[hdoupe]

See #894:

The high level parameter submission logic is housed primarily in submit_reform. This is a complex function and does not have any unit tests. There are a fairly high number of paths that data can take through this function: File or GUI? Full calculation or quick calculation? Errors or no errors? If errors, were they detected in PolicyBrain or the upstream project? If errors, what type of errors and has the user seen them? On top of that, this function produces about 20 pieces of data.

The aim for this first PR is to:

1. Move submit_reform into its own module. This makes the views.py module more pure in the sense that its functions map directly to web pages (i.e. a TaxBrain inputs page, an outputs page, an edit parameters page, etc.). This also modularizes the submission logic which will make it easier to apply to other apps in the future.
2. Add tests to pin down the behavior of submit_reform so that it can be changed safely in the future.

I am starting #894 with a clean slate given the extensive changes to code style and the compute.py architecture.

[hdoupe]

There are no changes in the data submission logic in pr #923. The only substantive change is making dropq_compute an argument in submission functions process_reform and submit_reform. This is essential so that the test suite can check that the correct data is submitted to the backend.

[hdoupe]

The most recent commit does not change the test logic but does remove a hack that allowed PolicyBrain to use unit tests and pytest at the same time. My "hack" was broken in pytest release 3.7.1 (most likely by the patch for issue pytest-dev/pytest#3747).

[martinholmer]

@hdoupe said:

There are no changes in the data submission logic in pr #923. The only substantive change is making dropq_compute an argument in submission functions process_reform and submit_reform.

Why is there any mention of dropq in PolicyBrain? It was only relevant to TaxBrain (not any other model), but that terminology is long obsolete even for TaxBrain/Tax-Calculator.

[hdoupe]

@martinholmer Good question. @lucassz was also curious about this when he first started. We are phasing out the use of "dropq" throughout PolicyBrain. In PR #922, "dropq" was removed from many of the Compute methods. We plan to make further changes in #921 to remove "dropq" from variable names and URL extensions.

[hdoupe]

@lucassz can you review?

[martinholmer]

@hdoupe said:

@martinholmer Good question. @lucassz was also curious about this when he first started. We are phasing out the use of "dropq" throughout PolicyBrain. In PR #922, "dropq" was removed from many of the Compute methods. We plan to make further changes in #921 to remove "dropq" from variable names and URL extensions.

Sounds like a sensible plan. I know from experience that reworking code this large and complex is a difficult and time-consuming task.

[lucassz]

Looks good to me, this is a very good start. I'll think of whether there's any other ways we can further improve the structure of this.

[hdoupe]

Thanks for taking a look @lucassz. One way to break submit_reform down is:

• in what context is it called?

  • file input
  • is the user resubmitting after errors have been shown to them
  • gui input
  • is the user resubmitting after errors have been shown to them

• is the input OK?

  • has warnings, errors
  • the user hasn't seen them yet (don't submit)
  • the user has seen them and has resolved all errors
    • still has warnings? (submit anyway)
    • no more warnings? (submit anyway)
  • the user has seen them and has failed to resolve the errors (don't submit)
  • has only warnings and the user has seen them (submit)
  • no errors (submit)

• quick calc or full calc?

[hdoupe]

There's a lot of shared code and data between all of the paths through that I outline above. This leads me to think that this should be some type of class.

I'm inclined to merge this as is to make the work in #921 smoother by reducing clutter in the TaxBrain views module. We can then begin tinkering with different ways to approach this problem while we are wrapping up #921.

Does this seem sensible?

[lucassz]

Yes, I agree that it will be easier to do more in-depth refactoring (such as packaging it into a class) once this is merged into master and constitutes the base to start from.