Introduce on_response blocks to set attributes on calculator #2408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
floehopper
force-pushed
the
introduce-on-response-blocks-to-set-attributes-on-calculator
branch
from
7d03222 to
f7f5093
Compare
|
Since no comments have been added to the commits in this PR, I'm going to rebase it against master and force-push to fix the conflicts reported by GitHub above. |
floehopper
force-pushed
the
introduce-on-response-blocks-to-set-attributes-on-calculator
branch
from
f7f5093 to
4bde089
Compare
|
Rebase & force-push done - GitHub is now showing no conflicts. This is ready for review. |
|
Would you be able to update the commit note for 4a26b6c ("Make CalculateStatutorySickPayFlowTest more realistic") to explain why you've used a real instead instead of a stub? |
You might be able to change this back now that we've stopped using the |
|
This looks great to me, @floehopper. Good work! |
floehopper
force-pushed
the
introduce-on-response-blocks-to-set-attributes-on-calculator
branch
from
cea2332 to
dacb3c9
Compare
I tried reverting that commit, but the violations still happened, so I think I'll leave it in place. I've added an empty squash-commit to remind me to update the commit note accordingly: dacb3c9. |
|
I'm going to do an interactive rebase to incorporate the squash-commits, then rebase against master to resolve the conflicts reported by GitHub, before force-pushing (since there are no commit comments) in preparation for merging. |
Note that this is just documenting the existing behaviour - it's not intended as an endorsement of the existing behaviour.
This does pretty much everything that `SmartAnswer::Calculation` does, except that it doesn't assign the result of the block to a specified state variable. Hopefully you can see this by comparing the unit tests for the two classes. I'm planning to use this to introduce a new type of block to the Flow/Question DSL which will be called immediately after receiving the response from the user. However, I've named the new class very generically, because there's no reason it couldn't be used at other points in the DSL. I did contemplate other slightly more specific names e.g. `NonAssigningCalculation` and `BlockEvaluatableInContextOfState`. For the former, I thought that the `Calculation` suffix was a bit confusing, because I think it conveys the idea that it has a result. I thought the latter was unwieldy and came to the conclusion that really this is just a block - it's just enhanced with a method that allows it to be called in the context of a `SmartAnswer::State`. And the method signature of the `#evaluate` method is enough to explain the latter. Thus I think naming it `SmartAnswer::Block` is the best option.
The idea is that we'll use these blocks to set attributes on the calculator/policy object instead of doing that in `next_node` blocks. One advantage of doing this is to separate out the attribute-setting code which should make it easier to do automatically in future. Another advantage is that we can avoid having to pass the response into any validation-related methods on the calculator which should make them more ActiveModel-like. The new `on_response` blocks are run *before* both `next_node_calculation` and `validate` blocks. The existing unit tests around this area of the codebase leave a bit to be desired, but I've added new tests as best I can roughly following existing patterns. I did also contemplate adding an `EngineIntegrationTest` with a new fixture flow, but I decided that it would be overkill and added the test to `FlowTest` instead. I feel as if this gives sufficient test coverage for the new functionality.
Use a real instance of the calculator instead of a stub: * Using a real object rather than a stub means that we'll see an error if we stub a non-existent method. * We can re-use the calculator instance created in the top-level setup. * Stubbing simple accessor methods seems like overkill.
I think these are remnants from when we weren't stubbing the validation methods on the calculator.
It seems better to separate the setting of attributes on the calculator from the routing logic in `next_node` blocks. The recently introduced `on_response` block allows us to do this. It also means the response will be available on the calculator as an attribute when validation methods are called which means we won't need to pass the response into these methods, thus simplifying the code. Note that I've had to set the `calculator` state variable in the first `on_response` block, because these blocks are executed before `next_node_calculation` blocks.
Now that the responses are available as attributes we don't need to pass the response into the validation methods. This makes the validations closer to the ActiveModel style. I've had to use a slight bodge in `#valid_last_payday_before_offset?`. I've added 1 day to the RHS to counteract the fact that the validation was previously done on the raw response rather than `relevant_period_from` which is the raw response plus 1 day. I'm planning to improve this later as part of this Trello card [1]. [1]: https://trello.com/c/uu0HeZee
The regression tests all pass.
It seems better to separate the setting of attributes on the calculator from the routing logic in `next_node` blocks. The recently introduced `on_response` block allows us to do this. It also means the response will be available on the calculator as an attribute when validation methods are called which means we won't need to pass the response into these methods, thus simplifying the code. Note that I've had to set the `calculator` state variable in the first `on_response` block, because these blocks are executed before `next_node_calculation` blocks.
Now that the responses are available as attributes we don't need to pass the response into the validation methods. This makes the validations closer to the ActiveModel style.
It seems better to separate the setting of attributes on the calculator from the routing logic in `next_node` blocks. The recently introduced `on_response` block allows us to do this. Note that I've had to set the `calculator` state variable in the first `on_response` block, because these blocks are executed before `next_node_calculation` blocks.
The regression tests all pass.
It seems better to separate the setting of attributes on the calculator from the routing logic in `next_node` blocks. The recently introduced `on_response` block allows us to do this. Note that I've had to set the `calculator` state variable in the first `on_response` block, because these blocks are executed before `next_node_calculation` blocks.
The regression tests all pass.
It seems better to separate the setting of attributes on the calculator from the routing logic in `next_node` blocks. The recently introduced `on_response` block allows us to do this. It also means the response will be available on the calculator as an attribute when validation methods are called which means we won't need to pass the response into these methods, thus simplifying the code. Note that I've had to set the `calculator` state variable in the first `on_response` block, because these blocks are executed before `next_node_calculation` blocks.
Now that the responses are available as attributes we don't need to pass the response into the validation methods. This makes the validations closer to the ActiveModel style. Note that I've extracted a new `valid_current_location?` method on the `OverseasPassportsCalculator` to better match the style we've used elsewhere.
The regression tests all pass.
floehopper
force-pushed
the
introduce-on-response-blocks-to-set-attributes-on-calculator
branch
from
dacb3c9 to
a93ae65
Compare
floehopper
deleted the
introduce-on-response-blocks-to-set-attributes-on-calculator
branch
floehopper
added a commit
that referenced
this pull request
Jun 30, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable.
floehopper
added a commit
that referenced
this pull request
Jun 30, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable. This commit updates the documentation to reflect the new state of affairs.
floehopper
added a commit
that referenced
this pull request
Jul 5, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable. This commit updates the documentation to reflect the new state of affairs.
floehopper
added a commit
that referenced
this pull request
Jul 6, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable.
floehopper
added a commit
that referenced
this pull request
Jul 6, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable. This commit updates the documentation to reflect the new state of affairs.
floehopper
added a commit
that referenced
this pull request
Jul 6, 2016
In #2408 we introduced `on_response` blocks with the idea of using them to build and store a calculator object in a `calculator` state variable, and to manually store the response to each question on the calculator. This is instead of using `save_input_as` which stores the response as a state variable. This commit updates the documentation to reflect the new state of affairs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Supersedes #2405.
Trello card: https://trello.com/c/V6nqIWcJ
The idea is that we'll use these
on_responseblocks in a question block to set attributes on the calculator/policy object instead of doing that innext_nodeblocks which is the current "new-style" approach.I've updated the following flows to use the new
on_responseblocks:These are the only flows which use the new-style pattern of instantiating a calculator object in the first question, setting each response as an attribute on the calculator, and encapsulating all the policy logic in the calculator. I don't think it makes sense to apply the same approach to any of the other flows until they have been converted closer to the new-style.
Motivation
One advantage of doing this is to separate out the attribute-setting code which should make it easier to do automatically in future. Theoretically the
next_nodeblock will only need access to the calculator and not to the response.Another advantage is that we can avoid having to pass the response into any validation-related methods on the calculator which should make them more ActiveModel-like.
The new
on_responseblocks are run before bothnext_node_calculationandvalidateblocks.External changes
None. This is just a refactoring.