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

Statutory sick pay calculator bugfix #1998

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@floehopper
Contributor

floehopper commented Oct 12, 2015

Supersedes #1967.

Currently when calculating sickness duration the calculator does not take into account employee's usual work days, it simply subtracts the dates, which is incorrect. In order to fix this issue we first need to move the question that collects information about employees working days pattern earlier in the process.

Unfortunately this used to be the last question of the flow that carried all the logic to decide the outcome node.

Before

calculate-statutory-sick-pay-before-question-move

After

calculate-statutory-sick-pay-after-question-move

Removing this question from it's position means this logic has to be used in three different questions leading to those outcomes. This has been achieved by extracting the decision logic into an inner class, OutcomeDecision within SmartAnswer::CalculateStatutorySickPayFlow.

Having moved the question, we then need to take the user's working pattern into account when calculating the number of days worked in the linked sickness period. The last few commits are an attempt at doing that.

tadast and others added some commits Sep 16, 2015

Declare many methods in StatutorySickPayCalculator as private
StatutorySickPayCalculator public API is rather extensive, making significant
changes to it a bit harder. Declare all methods that are not used outside of
this class as private. I didn't feel comfortable enough to re-factor the tests
yet and where tests are testing methods that are now private I used
Object#send. This is an anti-pattern, but I see it as exposing a symptom of
poor class API design that was there already rather than a newly introduced
problem.
Extract outcome decision logic to a class in SSP calculator
I'm adding this refactoring in order to make it easier to move the
usual_work_days? question in the following commit. It is required because we
need to access information gathered in this question earlier in the process
(see the next few commits).
Make employee AWE variable available to next_node blocks
We want to move the `:usual_work_days?` question earlier in the flow. This means
we'll need to instantiate the `OutcomeDecision` class in the `next_node` blocks
of these three questions. That class depends on the following state variable:

    employee_average_weekly_earnings

By converting the `calculate` block to a `next_node_calculation` block we can
make the variable available.
Remove redundant save_input_as statements
These was made redundant when the `calculation` blocks for
`:employee_average_weekly_earnings` in these questions was converted to
`next_node_calculation` blocks and the `response` passed into the blocks was
used directly.
Move calculate blocks in Q to precalculate blocks in outcome
We want to move the `:usual_work_days?` question much earlier in the flow and
so we need to move these `calculate` blocks to the relevant outcome and convert
them into `precalculate` blocks so they are available in the outcome template.

Note that the block for `formatted_sick_pay_weekly_amounts` was instantiating
its own copy of the calculator, but since we no longer have easy access to the
`response` in the `precalculate` block and we already have access to an instance
of the calculator, it's simpler to use that.
Fix question number comments
I'm not convinced these question numbers are very useful. I assume they're
related to question numbers in the logic document, but don't know whether
the logic document has been kept up-to-date and it's obvious from the changes
in this commit that the comments haven't been kept up-to-date. My inclination
is to delete them, but for now I've just fixed them as best I can so they are
at least consistent in the context of the flow class.
Update regression test checksums for SSP
The regression tests were all passing at this point and I wanted to indicate
this was the case just before we move the `:usual_work_days?` question to a
point earlier in the flow.
Move usual_work_days? question up in the calculate SSP flow
It has been reported by HMRC that currently this Smart Answer does not
calculate the number of sick days correctly e.g. when calculating whether the
employee has been sick for the minimum of 3 days it does not take into account
the usual work days. In order to fix this we need to move up the question that
collects employee's working patterns.

The usual_work_days? question used to be the last one in the flow and had the
logic for deciding the next outcome node (out of 4 possible). Three questions
were leading to the usual_work_days? question. Removing it from the last spot
in the flow means those three questions will need to use the logic to decide
the outcome node.

In order to avoid duplicating this non-trivial logic I've extracted it to the
private OutcomeDecision class (see earlier commit).

I've updated the question number comments as best I can. I assume they refer to
question numbers in the logic document, but I don't know if that has been
updated to reflect these changes. Anyway, hopefully it's obvious that Q11 has
moved up and been inserted as a new Q4. All the subsequent question numbers
have been incremented by 1.
Re-generate responses & expected results for SSP
I ran the following command:

    rails r script/generate-responses-and-expected-results-for-smart-answer.rb calculate-statutory-sick-pay
WIP: Update regression test artefacts after moving usual_work_days Q
Theoretically it feels as if this should just be a bunch of renames of outcome
page artefacts where the path changes to reflect the different order of
questions. However, it seems as if git diff doesn't manage to work out the
renames correctly.

I also expect to both the path and the content of question page artefacts
change. The path should've changed for the same reason as the outcome pages
and the content will have changed because of things like the 'Previous answers'
section changing.

Since it's proved very difficult to get confidence that these changes as are I
expect from looking at the diff, I've used the `visualise` action to generate
a diagram of the flow with the connections between its questions and outcomes
before and after the moving of the `:usual_work_days?` question. This has
convinced me that these changes are probably correct.
WIP: Update regression test checksums for SSP
The regression tests were all passing at this point and I wanted to indicate
this was the case.
Use keyword args in StatutorySickPayCalculator constructor
I found it very hard to understand what the unit tests were trying to achieve,
especially as many of them just have a scenario number as their context. By
naming the `StatutorySickPayCalculator` constructor arguments I'm hoping this
will improve matters.
Extract StatutorySickPayCalculator.dates_matching_pattern method
I want to use this to calculate the number of working days in the linked
sickness period.
Only consider number of days *worked* in linked sickness period
This fixes a bug reported in Zendesk ticket #1128193 [1].

I've added a new integration test based on one of the examples which have been
fact-checked by HMRC.

I've updated the assertion in three other existing integration tests to match
the new table output, but I don't have complete confidence that these changes
are correct.

The changes to the first row in the table in each case makes sense to me,
because presumably there's been a reduction in the number of working days
in the linked sickness period, meaning that it takes longer to use up the
three "waiting days", and hence the SSP amount for the first week is reduced.

However, the removal of a number of rows towards the end of the table makes
less sence. Presumably this is something to do with hitting the 28-week (?)
maximum, but it's not obvious to me why we would hit the maximum sooner. I
plan to check this with @lutgendorff before merging.

I'm not completely happy with the code change, but the rest of the logic is so
hard to follow that I've purposefully tried to keep the scope of the change as
small as possible.

Really this smart answer needs some major refactoring, but that's going to have
to wait for now.

[1]: https://govuk.zendesk.com/tickets/1128193
WIP: Update regression test artefacts for SSP
I'm not sure these changes are correct.
@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

I've force-pushed up my latest work on this. I still have some uncertainty about the effect of the changes I've made and I've asked @lutgendorff for help by email. I'm going to add the "before" & "after" visualisations of the flow which I've captured to the PR description. Pending feedback from Liz, this is still not ready to merge.

Contributor

floehopper commented Oct 15, 2015

I've force-pushed up my latest work on this. I still have some uncertainty about the effect of the changes I've made and I've asked @lutgendorff for help by email. I'm going to add the "before" & "after" visualisations of the flow which I've captured to the PR description. Pending feedback from Liz, this is still not ready to merge.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 21, 2015

Contributor

Superseded by #2031.

Contributor

floehopper commented Oct 21, 2015

Superseded by #2031.

@floehopper floehopper closed this Oct 21, 2015

@floehopper floehopper deleted the jm_ssp_bugfix_alternative branch Jun 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment