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 minimum-wage Smart Answers #1856

Merged
merged 66 commits into from Aug 6, 2015

Conversation

@chrisroos
Copy link
Contributor

commented Jul 29, 2015

I've made a number of changes to the minimum-wage shared logic that's used by both am-i-getting-minimum-wage and minimum-wage-calculator-employers.

There are quite a few commits but they're all fairly small and I hope the commit message explains the motivation.

In broad terms, these are the changes I've made:

  • Remove unused code:
    • From unused precalculate blocks.
    • From unused save_input_as values.
    • From unused custom validation where the validation is already provided by the money_question.
    • From parsing that's already handled by questions that use (e.g.) the parse: Float option.
  • Move formatting to templates, e.g. use OutcomeHelper#format_money.
  • Use calculator values in templates and remove no-longer necessary calculate blocks.
  • Use validate blocks instead of custom validation in calculate blocks. Add tests where they didn't exist previously.
  • Set calculator attributes in next_node blocks rather than calculate blocks whose return values aren't used.
  • Consistently use next_node blocks rather than the shortcuts available to multiple_choice questions.
  • Instantiate the calculator early on in the flow and then add each response to it as they come in, rather than instantiating the object in one go later on in the flow. I had to add accessors to the calculator to make this possible. Setting attributes directly on the calculator means that I don't have to pollute the state object in order to share variables within the flow.
  • Move validation logic out of the flow and into the calculator.
  • Change some tests that were asserting against state variables so that they now assert against values on the calculator. I started out deleting the tests that were asserting against state variables that no longer existed but then realised that it's possible I'd be losing some useful tests. These integration tests that now assert against the calculator should move to the calculator tests if they're deemed useful.

NOTE. All but the first two questions appear twice, once for determining minimum wage for the current period and once for a previous period.

Having now gone through the process I can see a slightly better ordering of commits but trying to reorder the commits in this branch would be a bit painful and probably not worth doing.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

I've added a few more commits.

One first commit moves the unit tests from test/integration/smart_answer_flows to test/unit/smart_answer_flows.

The next three commits move policy logic from the flow to the Calculator.

The final two commits update the checksum data for am-i-getting-minimum-wage and minimum-wage-calculator-employers.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

"elligible" -> "eligible"

Thanks @floehopper. Corrected in fixup commit 98f7d55.

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

I might be wrong, but I think the save_input_as :current_or_past_payments statement in what_would_you_like_to_check? is now redundant.

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

Do you know whether the calculate :accommodation_charge blocks are still needed in :current_accommodation_charge? & :past_accommodation_charge?? Also do you know why we need to convert the money object into a Float in these cases?

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

It might be nice to extract response.to_i == 0 from :how_many_hours_overtime_do_you_work? & :how_many_hours_overtime_did_you_work? into a method on the calculator e.g. #any_overtime_worked?. Also I'm pretty sure the to_i is redundant, because 0.0 == 0 # => true.

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

This is great! A heroic effort! 😄

It makes me a lot more confident that this approach is the right one - the flow is now so much easier to follow and the code & behaviour is so much more consistent.

All my comments are pretty nit-picky except for the one about the validation range being accidentally changed in 676b611.

👍

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

I might be wrong, but I think the save_input_as :current_or_past_payments statement in what_would_you_like_to_check? is now redundant.

I think you're right. I'll get rid of it.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

Do you know whether the calculate :accommodation_charge blocks are still needed in :current_accommodation_charge? & :past_accommodation_charge?? Also do you know why we need to convert the money object into a Float in these cases?

The accommodation_charge is passed to the #accommodation_adjustment method in the :current/past_accommodation_usage? questions.

Having just looked at the implementation of #accommodation_adjustment I can see that it converts the parameter to a Float to I think I can probably get away with using save_input_as instead of the calculate :accommodation_charge blocks.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

It might be nice to extract response.to_i == 0 from :how_many_hours_overtime_do_you_work? & :how_many_hours_overtime_did_you_work? into a method on the calculator e.g. #any_overtime_worked?. Also I'm pretty sure the to_i is redundant, because 0.0 == 0 # => true.

Yup, that sounds good. I must've missed this when extracting other things to the calculator.

@chrisroos chrisroos force-pushed the refactor-minimum-wage-smart-answers branch from f08ab5c to ab0bb91 Aug 6, 2015

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

I've made a number of changes suggested by @floehopper, rebased on master and force pushed to this branch. I'll double check that the tests pass, including regression tests, and then manually merge to master.

chrisroos added 18 commits Jul 27, 2015
Remove total_working_pay method
The `MinimumWageCalculator#total_working_pay` method was only used in the
`total_working_pay` precalculate blocks, and those precalculate blocks weren't
used anywhere.
Remove accommodation_rate precalculate block
These aren't being used anywhere.
Remove accommodation_charged precalculate block
These aren't being used anywhere.
Move formatting of total_underpayment to current_payment_below templates
Formatting is a view concern rather than a flow concern.

I've moved the formatting of `total_underpayment` from the
'current_payment_below' outcome to the two 'current_payment_below' ERB
templates.

The `MinimumWageCalculator#format_money` and `OutcomeHelper#format_money`
methods both remove the zeros for whole numbers. The
`OutcomeHelper#format_money` also adds separators to large numbers and prepends
the currency symbol.

I've updated 4 test artefacts that have changed now that larger numbers include
a separator.
Move formatting of total_underpayment to past_payment_below templates
Formatting is a view concern rather than a flow concern.

I've moved the formatting of `total_underpayment` from the 'past_payment_below'
outcome to the two 'past_payment_below' ERB templates.

The `MinimumWageCalculator#format_money` and `OutcomeHelper#format_money`
methods both remove the zeros for whole numbers. The
`OutcomeHelper#format_money` also adds separators to large numbers and prepends
the currency symbol.

I've updated a number of the test artefacts that have changed now that larger
numbers include a separator.
Move formatting of minimum_hourly_rate to templates
Formatting is a view concern rather than a flow concern.

I've moved the formatting of `minimum_hourly_rate` from the flow to the ERB
templates for am-i-getting-minimum-wage and minimum-wage-calculator-employers.

The `MinimumWageCalculator#format_money` and `OutcomeHelper#format_money`
methods both remove the zeros for whole numbers. The
`OutcomeHelper#format_money` also adds separators to large numbers and prepends
the currency symbol.

I've updated a number of the test artefacts that have changed now that larger
numbers include a separator.
Move formatting of total_hourly_rate to templates
Formatting is a view concern rather than a flow concern.

I've moved the formatting of `total_hourly_rate` from the flow to the ERB
templates for am-i-getting-minimum-wage and minimum-wage-calculator-employers.

The `MinimumWageCalculator#format_money` and `OutcomeHelper#format_money`
methods both remove the zeros for whole numbers. The
`OutcomeHelper#format_money` also adds separators to large numbers and prepends
the currency symbol. The `OutcomeHelper#format_money` method also adds the
minus sign ('-') before the pound symbol ('£') rather than after it. So,
negative numbers now appear as, for example, -£55.92 instead of £-55.92.

I've updated a number of the test artefacts that have changed now that larger
numbers include a separator, and the minus sign has moved.
Remove MinimumWageCalculator#format_money method
We're now using the `OutcomeHelper#format_money` method in the
am-i-getting-minimum-wage and minimum-wage-calculator-employers Smart Answers.
Remove `total_underpayment` precalculate block from current_payment_b…
…elow

Using `MinimumWageCalculator#total_underpayment` in the ERB template allows us
to reduce the number of variables that we have to make available to the view.
Remove `total_underpayment` precalculate block from past_payment_below
Using `MinimumWageCalculator#historical_adjustment` in the ERB template allows
us to reduce the number of variables that we have to make available to the view.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `minimum_hourly_rate` precalculate block
Using `MinimumWageCalculator#minimum_hourly_rate` in the ERB template allows us
to reduce the number of variables that we have to make available to the view.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `total_hourly_rate` precalculate block
Using `MinimumWageCalculator#total_hourly_rate` in the ERB template allows us
to reduce the number of variables that we have to make available to the view.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `historical_adjustment` precalculate block
This doesn't appear to be used anywhere.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `above_minimum_wage` precalculate block
This doesn't appear to be used anywhere.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `total_hours` precalculate block
This doesn't appear to be used anywhere.

I've chosen to update some of the tests that were previously asserting state
variables so that they now assert against the `MinimumWageCalculator`. I'm not
convinced about this but I think it means that I'm not accidentally losing
potentially important/interesting tests.
Remove `accommodation_provided` precalculate block
This doesn't appear to be used anywhere.
Consistent use of whitespace in flow
Additionally ensure that `save_input_as` comes below `option`s for
`multiple_choice` questions.
Use validate method in how_old_are_you?
I've also introduced unit tests around this validation, rather than having to
do it within the integration test.
chrisroos added 24 commits Jul 29, 2015
Inline amount_paid variable
I don't think the local `amount_paid` variable was adding anything here.
Instantiate the calculator in the first question of the flow
I'm planning to set the responses to questions on the calculator when we
receive those responses, rather than in a single question later in the flow
(:how_much_are/were_you_paid_during_pay_period?).

This should avoid us having to store the responses on the state object.

I've had to override the `date=` method so that I can call
`minimum_wage_data_for_date(@Date)` and set the `@minimum_wage_data` variable.
This was previously happening in the constructor but I can no longer rely on
that as we're constructed the object before having the `date` available. I'm
sure this can be improved but I think it's probably good enough for now.
Set the age on the calculator earlier in the flow
Setting it when we receive the answer means we can avoid polluting the state
object with the `age` attribute.
Set the date on the calculator earlier in the flow
Setting it when we receive the answer means we can avoid polluting the state
object with the `payment_date` attribute.
Set the pay_frequency on the calculator earlier in the flow
Setting it when we receive the answer means we can avoid polluting the state
object with the `pay_frequency` attribute.
Set the basic_hours on the calculator earlier in the flow
Setting it when we receive the answer means we can avoid polluting the state
object with the `basic_hours` attribute.
Set the is_apprentice on the calculator earlier in the flow
Setting it when we receive the answer means we can avoid polluting the state
object with the `is_apprentice` and `was_apprentice` attributes.
Avoid unnecessary calculate block
Now that this is just setting state on the calculator, we can move that logic
to the `next_node` block.
Move validation of age to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the age on the calculator before the `validate` block then we
could avoid having to pass it in to the `valid_age?` method.
Move validation of pay_frequency to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the pay_frequency on the calculator before the `validate` block
then we could avoid having to pass it in to the `valid_pay_frequency?` method.
Move validation of hours_worked to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the hours_worked on the calculator before the `validate` block
then we could avoid having to pass it in to the `valid_hours_worked?` method.
Move validation of overtime_hours_worked to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the `overtime_hours` on the calculator before the `validate`
block then we could avoid having to pass it in to the
`valid_overtime_hours_worked?` method.
Move validation of accommodation_charge to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the `accommodation_charge` on the calculator before the
`validate` block then we could avoid having to pass it in to the
`valid_accommodation_charge?` method.
Move validation of accommodation_usage to MinimumWageCalculator
This feels like an improvement over having this logic in the flow.

If we could set the `accommodation_usage` on the calculator before the
`validate` block then we could avoid having to pass it in to the
`valid_accommodation_usage?` method.
Move unit tests to test/unit/smart_answer_flows
This folder structure/naming comes from the convention James is using in
PR #1859.
Introduce MinimumWageCalculator#apprentice_elligible_for_minimum_wage?
Policy related logic should be encapsulated in the `MinimumWageCalculator`
rather than the flow.

NOTE. I've reversed the condition in the `next_node` block of the
`:were_you_an_apprentice?` question. This was to avoid either using `if !` (or
`unless`) or having to negate the new method (e.g.
`#apprentice_not_elligible_for_minimum_wage?`).
Introduce MinimumWageCalculator#under_school_leaving_age?
Policy related logic should be encapsulated in the `MinimumWageCalculator`
rather than the flow.
Introduce MinimumWageCalculator#historically_receiving_minimum_wage?
Policy related logic should be encapsulated in the `MinimumWageCalculator`
rather than the flow.

I'm not entirely happy about stubbing `#historical_adjustment` in the tests but
I don't want to get too involved in understanding/making changes to the
`MinimumWageCalculator` at the moment.
Use Flow#node in unit tests
@floehopper pointed out this improvement in PR #1859.
Remove unused save_input_as from :what_would_you_like_to_check?
As far as I can tell this isn't even being used in the version of this Smart
Answer currently in master.
Avoid calculate block in :current/past_accommodation_charge?
The `accommodation_charge` variable is used in the call to
`#accommodation_adjustment` in the `:current/past_accommodation_usage?`
questions. The `#accommodation_adjustment` method already converts the `charge`
argument to a `Float` which means we can avoid doing that in this `calculate`
block, and simply rely on using `save_input_as` instead.
Introduce MinimumWageCalculator#any_overtime_hours_worked?
Policy related logic should be encapsulated in the `MinimumWageCalculator`
rather than the flow.

NOTE. I've reversed the condition in the `next_node` block of the
`:how_many_hours_overtime_do/did_you_work?` questions. This was to avoid either
using `if !` (or `unless`) or having to negate the new method (e.g.
`#no_overtime_hours_worked?`).
Update am-i-getting-minimum-wage-files.yml
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
      am-i-getting-minimum-wage
Update minimum-wage-calculator-employers-files.yml
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
      minimum-wage-calculator-employers

@chrisroos chrisroos force-pushed the refactor-minimum-wage-smart-answers branch from ab0bb91 to 4bb2400 Aug 6, 2015

@chrisroos chrisroos merged commit 4bb2400 into master Aug 6, 2015

1 check passed

default "Build #2687 succeeded on Jenkins"
Details
chrisroos added a commit that referenced this pull request Aug 6, 2015
Merge branch 'refactor-minimum-wage-smart-answers'
Merges PR #1856.

I've made a number of changes to the minimum-wage shared logic that's used by
both am-i-getting-minimum-wage and minimum-wage-calculator-employers.

There are quite a few commits but they're all fairly small and I hope the
commit message explains the motivation.

In broad terms, these are the changes I've made:

* Remove unused code:

 * From unused `precalculate` blocks. * From unused `save_input_as` values. *
From unused custom validation where the validation is already provided by the
`money_question`. * From parsing that's already handled by questions that use
(e.g.) the `parse: Float` option.

* Move formatting to templates, e.g. use `OutcomeHelper#format_money`.

* Use calculator values in templates and remove no-longer necessary calculate
blocks.

* Use `validate` blocks instead of custom validation in calculate blocks. Add
tests where they didn't exist previously.

* Set calculator attributes in `next_node` blocks rather than `calculate`
blocks whose return values aren't used.

* Consistently use `next_node` blocks rather than the shortcuts available to
`multiple_choice` questions.

* Instantiate the calculator early on in the flow and then add each response to
it as they come in, rather than instantiating the object in one go later on in
the flow. I had to add accessors to the calculator to make this possible.
Setting attributes directly on the calculator means that I don't have to
pollute the state object in order to share variables within the flow.

* Move validation logic out of the flow and into the calculator.

* Change some tests that were asserting against state variables so that they
now assert against values on the calculator. I started out deleting the tests
that were asserting against state variables that no longer existed but then
realised that it's possible I'd be losing some useful tests. These integration
tests that now assert against the calculator should move to the calculator
tests if they're deemed useful.

__NOTE__. All but the first two questions appear twice, once for determining
minimum wage for the current period and once for a previous period.

Having now gone through the process I can see a slightly better ordering of
commits but trying to reorder the commits in this branch would be a bit painful
and probably not worth doing.
@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

Manually merged in af48943.

@chrisroos chrisroos deleted the refactor-minimum-wage-smart-answers branch Aug 6, 2015

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

Yay! 😄 ⭐️ ⭐️ ⭐️

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