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.

Sign up for GitHub

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

Jump to bottom

WIP: Define permitted next nodes for all flows #2002

Closed
wants to merge 52 commits into from
Closed

WIP: Define permitted next nodes for all flows #2002

wants to merge 52 commits into from

Conversation

chrisroos
Copy link
Contributor

Trello card - https://trello.com/c/jtjRNOW9/97-update-all-flows-to-use-permitted-next-nodes

I've updated Question::Base#next_node_for to raise an exception if the next_node isn't found in the list of permitted_next_nodes.

I've updated the first three flows to use permitted_next_nodes where required to get an idea of how they're going to look.

NOTE. Using next_node :symbol or next_node_if automatically add the nodes to permitted_next_nodes so I've only updated the flows/questions that use next_node with a block.

The advantage of using permitted_next_nodes is that it allows us to visualise the flows.

Visualising additional-commodity-code

Before

permitted-next-nodes-before

After

permitted-next-nodes-after

@chrisroos
Fail fast if next_node not in permitted_next_nodes
f822077 
TODO: Expand this commit message. Consider moving this commit after
all the commits to add the relevant permitted_next_nodes statements
@chrisroos
Use permitted_next_nodes in additional-commodity-code
564cfc6
@chrisroos
Update checksum data for additional-commodity-code
a396dff 
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
    additional-commodity-code
@chrisroos
Use permitted_next_nodes in minimum-wage
e62601c
@chrisroos
Update checksum data for minimum-wage
1e00d38 
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
    am-i-getting-minimum-wage

    $ rails r script/generate-checksums-for-smart-answer.rb \
    minimum-wage-calculator-employers
@chrisroos
Use permitted_next_nodes in benefit-cap-calculator
390dfff
@chrisroos
Update checksum data for benefit-cap-calculator
aa5b4de 
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
    benefit-cap-calculator
@floehopper floehopper self-assigned this Oct 13, 2015
@floehopper
Copy link
Contributor

Other than my comments above, this looks good to me. I agree that initially we could just update the flows which use next_node with a block.

@chrisroos
Pass permitted_next_nodes to next_node
9f5bff4
@chrisroos
Use permitted option in additional-commodity-code
3b494c3
@chrisroos
Update checksum data for additional-commodity-code
4eafdd5
@chrisroos
Use permitted option in minimum-wage
b8df92d
@chrisroos
Update checksum data for minimum-wage
70c2e88 
Updated using:

    $ rails r script/generate-checksums-for-smart-answer.rb \
    am-i-getting-minimum-wage

    $ rails r script/generate-checksums-for-smart-answer.rb \
    minimum-wage-calculator-employers
@chrisroos
Use permitted option in benefit-cap-calculator
22e521e
@chrisroos
Update checksum data for benefit-cap-calculator
5beb0c0
@chrisroos
Use permitted option in redundancy-pay
2fdeaa2
@chrisroos
Use permitted option in calculate-married-couples-allowance
e36cd42
@chrisroos
Update checksum data for calculate-married-couples-allowance
fd15f5a 
Updated using:

    rails r script/generate-checksums-for-smart-answer.rb \
    calculate-married-couples-allowance
@chrisroos
Use permitted option in calculate-state-pension
abea785
@chrisroos
Update checksums for calculate-state-pension
b189bbb
@chrisroos
Use permitted option in calculate-your-holiday-entitlement
9fc65ad
@chrisroos
Update checksum data for calculate-your-holiday-entitlement
77f0711
@chrisroos
Use permitted option in check-uk-visa
fc37c58
@chrisroos
Update checksum data for check-uk-visa
dc40b2f
@chrisroos
Use permitted option in childcare-costs-for-tax-credits
0419fb0
@chrisroos
Update checksum data for childcare-costs-for-tax-credits
d33ad53
@chrisroos
Use permitted option in estimate-self-assessment-penalties
df6f470
@chrisroos
Update checksum data for estimate-self-assessment-penalties
a7edc2c
@chrisroos
Use permitted option in help-if-you-are-arrested-abroad
b21acd0
@chrisroos
Use permitted option in pip-checker
10ced66
@chrisroos
Update checksum data for pip-checker
596008b
@chrisroos
Use permitted option in report-a-lost-or-stolen-passport
08a1c92
@chrisroos
Update checksum data for report-a-lost-or-stolen-passport
5667e60
@chrisroos
Use permitted option in simplified-expenses-checker
7675785
@chrisroos
Update checksum data for simplified-expenses-checker
a2d8405
@chrisroos
Use permitted option in student-finance-calculator
67845f5
@chrisroos
Update checksum data for student-finance-calculator
34327e0
@chrisroos
Use permitted option in graph test flow
1411df9
@chrisroos
Use permitted option in missing_transition test flow
b80ab45
@chrisroos
Use permitted option in bridge-of-death test flow
f680161
@chrisroos
Use permitted option in checkbox-sample test flow
4406cd4
@chrisroos
Use permitted option in custom-errors-sample test flow
3d17ecd
@chrisroos
Use permitted option in multiple_choice_question_test
1692d08
@chrisroos
Update graph_presenter_test
ecd72b8 
We no longer have any graphs that can not be visualised
@chrisroos
Fix Rubocop warning in calculate-married-couples-allowance
b3865af 
Rubocop warning: "Unused block argument"
@chrisroos
Fix Rubocop warnings in minimum_wage.rb
ae5aaa8 
Rubocop warning: "Surrounding space missing for operator ="
@chrisroos
Fix Rubocop warning in redundancy_pay.rb
dd0fb80 
Rubocop warning: "Unused block argument - response".
@chrisroos
Update checksum data for minimum-wage Smart Answers
60df5b7
@chrisroos
Update checksum data for calculate-married-couples-allowance
05477a8
@chrisroos
Copy link
Contributor Author

@floehopper: Can you take another look at this pull request, please? If you think the result is OK then I plan to rewrite history and open another pull result. Here's the plan:

  • Add support for specifying permitted next nodes through the permitted option to next_node
  • Define permitted next nodes for each flow, updating the checksum data each time
  • Fail fast if next_node is given a block but the permitted option isn't set
  • Fail fast in next_node_for if the next node doesn't exist in the array of @permitted_next_nodes

I think this ordering of commits should mean that all the tests pass each time.

@floehopper
Copy link
Contributor

Minor: I might be inclined to combine the checksum update commits with their respective commit which changes the flow. I think separating changes to the test artefacts into separate commits can be useful, because there are often a lot of files changing, but I'm not sure it's justified in the case of just a checksum change.

@floehopper
Copy link
Contributor

Very minor: I might be inclined to incorporate the Rubocop changes in the commits which triggered them.

@floehopper
Copy link
Contributor

Other than my comments, this LGTM 👍

@chrisroos
Copy link
Contributor Author

I might be inclined to combine the checksum update commits with their respective commit which changes the flow.

Yup - I'm planning to do that in the new branch.

@chrisroos
Copy link
Contributor Author

I might be inclined to incorporate the Rubocop changes in the commits which triggered them.

Good idea.

@chrisroos chrisroos mentioned this pull request Oct 14, 2015
Merged
@chrisroos
Copy link
Contributor Author

Closing in favour of PR #2010.

@chrisroos chrisroos closed this Oct 14, 2015
@chrisroos chrisroos deleted the define-permitted-next-nodes-for-all-flows branch October 14, 2015 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@floehopper floehopper

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisroos @floehopper