Use automatic next node detection in existing flows (with feedback addressed) #2368
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
This was referenced Mar 15, 2016
|
Does this need to be reviewed again, @floehopper? Or have you opened this ready to merge it to master given that PR #2362 is good to merge? |
|
I think it's basically ready to merge as long as you're happy that I've addressed all your feedback. |
|
Rebasing against master and force-pushing in preparation for merging. |
I'm about to change all the flows to use the mechanism for automatically detecting permitted next nodes. Although the existing tests, particularly the regression tests should give a lot of confidence that nothing gets broken, I'm hoping to compare the results of this script before and after the changes as a double-check.
I ran the following command:
$ rails r script/generate-permitted-next-nodes-for-all-flows.rb
I couldn't use `:auto` for all the `next_node` blocks, because some of them have dynamic results which can't be auto-detected.
I ran the following command again:
$ rails r script/generate-permitted-next-nodes-for-all-flows.rb
And the generated YAML was identical to the original version generated before
all the flows were changed to use the mechanism to automatically detect
permitted next nodes. So this file has served its purpose and can be removed.
This script has served its purpose and can be removed.
floehopper
force-pushed
the
use-automatic-next-node-detection-in-existing-flows-with-feedback-addressed
branch
from
5e0b896 to
e381356
Compare
floehopper
added a commit
that referenced
this pull request
Mar 16, 2016
…ion-in-existing-flows-with-feedback-addressed Use automatic next node detection in existing flows (with feedback addressed)
floehopper
deleted the
use-automatic-next-node-detection-in-existing-flows-with-feedback-addressed
branch
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.
This PR incorporates the feedback on #2362.
Changes
This PR changes all the existing flows (and shared logic) to use the mechanism to automatically detect permitted next nodes which was introduced in #2343. This was all relatively straightforward, if a little tedious. The only things worthy of note are:
benefit-cap-calculatorflow uses "dynamic" return values in many of itsnext_nodeblocks. I couldn't see a quick way to use the auto-detect mechanism, so I've left them be for the moment. I'll think about addressing this in a separate PR.plan-adoption-leaveflow doesn't usenext_nodewith a block for any of its questions and so I didn't need to make any changes to it.questionoroutcomemethods are called with the correct node types, there's a danger that some of them are called with the wrong node types. However, since I plan to add such a check in the future and calling with the wrong node type won't do any harm, I think it's more readable to usequestion&outcomerather than some kind of genericnodemethod.Testing
I added a script to generate a YAML representation of the possible next nodes for all the flows. I ran this script before and after the changes to double-check that using the automatic detection hadn't changed anything. However, the fact that the existing tests (including the regression tests) all pass also give me a lot of confidence.
Note
If I can find a way to use the automatic detection mechanism in
benefit-cap-calculator, I plan to remove the code which allows the manual specification of permitted next nodes, but this will be in a separate PR.Expected changes
None. However it might be worth checking that a couple of flow visualisation URLs work OK (not forgetting that the visualisation is currently broken in Chrome for other reasons):