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

Refactor calculate-your-child-maintenance to new style flow #2569

Merged

Conversation

Projects
None yet
2 participants
@floehopper
Copy link
Contributor

commented Jun 3, 2016

Description

Similar to #2561, this moves the flow towards the style described in the refactoring documentation. I've concentrated on moving logic out of the flow and into the calculator and presentational concerns into the templates. The idea is to change all the flows to use this approach so we can significantly simplify the DSL.

There are still one calculate block and a bunch of precalculate blocks towards the end of the flow which are horribly tangled up with both calculation and presentational concerns. I couldn't see an easy way to untangle them while retaining the same rounding, number formatting, etc. My suspicion is that there is some unintentional behaviour in here, but at this stage I want to avoid getting sucked into needing any fact-checking for this PR.

I haven't really done any significant work on the calculator itself e.g. extracting domain concepts. Also I haven't made any significant changes to the tests - we'll still need to move them towards the new-style approach at some point.

External changes

None. This is just an internal refactoring.

@chrisroos chrisroos self-assigned this Jun 10, 2016

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2016

Commit 3a578b5 ("Inline rate_type_formatted into outcome template")

It wasn't immediately obvious to me why you were able to use humanize when inlining the rate_type_formatted precalculate block. Presumably none of the other values of rate_type will be changed by this method?

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2016

Aside from my single comment this all looks great to me, @floehopper!

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2016

Improving commit comment based on feedback, rebasing against master, and force-pushing in preparation for merging.

floehopper added some commits Jun 2, 2016

Remove redundant to_i conversion in ChildMaintenanceCalculator
The response to the `how_many_children_paid_for?` question is already being
converted to an integer in the `number_of_children` calculate block so there's
no need to do it here as well.

The conversion is a user interface concern and so doesn't belong in the "model"
in any case.
Convert ChildMaintenanceCalculator into ActiveModel::Model
This brings the calculator into line with the refactoring documentation [1].

Although this makes the test code considerably more verbose, I think that can be
addressed separately.

[1]: https://github.com/alphagov/smart-answers/blob/master/doc/refactoring.md
Use benefits attribute on calculator vs state variable
Also set attribute in `on_response` block in the question where the response is
given.
Use number_of_children attribute on calculator vs state variable
Also set attribute in `on_response` block in the question where the response is
given.
Use paying? or receiving? calculator methods vs paying_or_receiving
The `paying?` calculator method already existed, but I've introduced the
`receiving?` method.

Although in the longer run I'd prefer the attribute on the calculator to be a
boolean, this feels like a good step forward.
Inline paying_or_receiving_text into quesiton template
This is a presentational concern and so belongs in the question template.

We could consider extracting this functionality into a helper method, but that
can be tackled separately.
Inline paying_or_receiving_hint into question template
This is a presentational concern and so belongs in the question template. This
also has the advantage that we don't need to call `html_safe` on the text.

I've deleted the assertion in the integration test that was checking the hint
text. I don't think this was adding much value, especially given that the hint
is captured in the regression test artefact [1].

[1]: `test/artefacts/calculate-your-child-maintenance/pay.html
Inline benefits_title into question template
This is a presentational concern and so belongs in the question template.
Inline income_title into question template
This is a presentational concern and so belongs in the question template.
Inline number_of_children_title into question template
This is a presentational concern and so belongs in the question template.
Inline how_many_nights_title into question template
This is a presentational concern and so belongs in the question template.
Inline fees_title in flat_rate_result outcome template
I think it's clearer to live with a little bit of duplication here.
Inline fees_title in reduced_and_basic_rates_result outcome template
I think it's clearer to live with a little bit of duplication here.
Inline rate_type_formatted into outcome template
This is a presentational concern and so it belongs in the outcome template.

Inspecting `ChildMaintenanceCalculator#rate_type` and the associated data stored
in `child_maintenance_data.yml`, I can see that the possible return values are:

* `:nil`
* `:flat`
* `:reduced`
* `:basic`
* `:basic_plus`

The only special case in the `rate_type_formatted` block was to remove the
underscore from `:basic_plus` - the only value which includes an underscore.

Considering this and the sentence in which `rate_type_formatted` is used, it
seems pretty obvious that the intention is to convert the symbols into something
suitable for human consumption and thus I was happy to use `String#humanize` to
do the conversion in the template.

I've removed an assertion from the integration test. I don't think this was
serving much purpose and in any case it's covered in the integration tests.
Update regression test checksums for calculate-your-child-maintenance
This pull request doesn't change any behaviour and all the regression tests
still pass with no changes to the artefacts, so I'm happy to update the
checksums.

@floehopper floehopper force-pushed the refactor-calculate-your-child-maintenance-to-new-style branch from aef17bb to 9d4851b Jun 14, 2016

@floehopper floehopper merged commit 0413075 into master Jun 14, 2016

1 check passed

default "Build #5382 succeeded on Jenkins"
Details

@floehopper floehopper deleted the refactor-calculate-your-child-maintenance-to-new-style branch Jun 14, 2016

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2016

It wasn't immediately obvious to me why you were able to use humanize when inlining the rate_type_formatted precalculate block. Presumably none of the other values of rate_type will be changed by this method?

I've expanded the commit note to explain my thinking here: cd118ac.

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

I've expanded the commit note to explain my thinking here: cd118ac.

Awesome. Thanks, @floehopper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.