Refactor overseas passports flow updates #2367
Merged
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
erkde
force-pushed
the
refactor_overseas_passports_flow_updates
branch
3 times, most recently
from
9693aef to
aaecd3d
Compare
|
I think I've covered all the non-minor comments in #2342 as well as most of the minor ones, so now marking as ready for review. |
|
I'm probably missing something obvious, but I don't understand what the following bit of the PR description means:
|
|
I've just reviewed this by going through the comments on #2342 and trying to find the relevant changes made in this PR. Ideally the responses to comments in the older PR would've had links to the new commits. I have not reviewed all the commits again i.e. I have assumed that the only changes between the two PRs are the ones identified in the comments on the old PR. On this basis, this PR looks good to me, although I note that this branch currently has conflicts with master. |
The `application_address` local, when present, is only used for showing content related to sending your application, so this moves the content into a suitably named partial.
Move the call to render `sending_your_application` to the outcome so we don’t need to pass `application_address` through the `making_your_application` partial.
And add ability to read and write the current_location in calculator, and save current_location in the first question.
Move list of countries from outcome partial to calculator, and add method for outcomes to determine if the current_location allows it.
Remove the data declared in partial `make_your_application` for `uk_visa_application_centre_countries` and replace with a method in `OverseasPassportCalculator` class that is now passed to the partial.
Remove the data declared in partial `make_your_application` for `uk_visa_application_with_colour_pictures` and replace with a method in `OverseasPassportCalculator` class that is now passed to the partial.
Remove the data declared in partial `make_your_application` for `non_uk_visa_application_with_colour_pictures` and replace with a method in `OverseasPassportCalculator` class that is now passed to the partial.
With `current_location` now saved in the overseas passports calculator, this is the first step at removing the variable from being passed around the smart answer ERB files by remove it from the `making_your_application` partial.
To further remove the propagation of `current_location` throughout overseas passports, this makes the `calculator` available in the first question and sets `current_location` using a `next_node_calculation` so `save_input_as ‘current_location’` can be removed. The `next_node_calculation` setting the value of `current_location` can be removed in future commits once the reliance on this variable is removed.
Add `EXCLUDE_COUNTRIES` array to overseas passports calculator to allow the data to be removed from the smart answer flow. This type of array may better belong `PassportAndEmbassyDataQuery` data class which can return all the data associated with overseas passports in future commits.
Remove the array data embedded in the smart answers in favour of having a method in the calculator class that can be tested in isolation. Again, this data may better belong in the data query class or even a YML file in future commits.
Add method `apply_in_neighbouring_countries` to the overseas passports calculator to allow the state to be removed from being embedded in the smart answer flow and for tests to be written at unit level.
This code seemed to find a world location and then checked to see if it should find an alternate world location and called find world location again when true. The implementation `ALT_EMBASSIES.has_key?(value)` is replaced in favour of `ALT_EMBASSIES[value] || value`, removing the very small possibility of having an alternate embassy location key with a `nil` or `false` value.
Add method `alternative_embassy_location` to the overseas passports calculator as a first step to remove the dependency on the data query class in the smart answer flow.
Add `world_location` method to overseas passports calculator to allow it to be removed from the smart answer flow and for unit tests to be written around it’s logic.
Replace smart answer flow variable `title_output` by adding a method to the calculator to return the name of it's `world_location`.
Move `application_form` from flow to calculator in overseas passports to add a unit test for it, and to remove the passing of the variable between outcomes and partials.
Remove `application_group` calculation from overseas passports smart flow and add to calculator class along with unit test.
Move the `supporting_document` calculation from the overseas passports flow to the calculator class and add unit tests. The value returned by `supporting_documents` changes if the user visits the birth_location question and enters a value that is not the united kingdom, so to cover the scenario where the method is invoked on a calculator without this value set, we add a `blank?` check to return a correct default value. With the `ips_docs_number` value based on the return value of `supporting_documents`, the calculation of `ips_docs_number` is moved to the end of the flow where the user has entered their birth location if routed to that question.
Move the `ips_docs_number` value from being calculated in the overseas passports flow to calculator and add unit tests.
Move the `application_address` from the overseas passports flow to calculator class, add unit test and update outcomes to access the value from the calculator.
Remove `its_result_type` calculation from overseas passports flow that decides the outcome node, and add an `ips_online_application` method to the calculator that can be used directly in the `next_node` calculations.
Move the `waiting_time` calculation from the overseas passport flow to calculator, add unit test and update outcomes to access value from calculator class.
Move `optimistic_process_time` value from overseas passports flow to a method in the calculator, add unit test and update the outcomes to retrieve the value from the calculator class.
Passport costs no longer appear to be used with overseas passports so removing from the flow, data query class and unit test.
With all values the overseas passport flow needs now available through the calculator class, the data_query reference dependency is no longer needed.
This commit tidies up the remaining calls to render partials with overseas passports outcomes by passing the calculator class as a local, rather a list of parameters, and have the partials retrieve the value they were expecting from the calculator.
OverseasPassportsCalculator#ips_online_application? returns true of ‘online_application’ is present in the passport_data, so this updates the outcome to use the existing method instead of repeating the logic of calculator.
The overseas passports outcomes only checks for the presence of this value in the passport_data, so for now, add method that returns boolean result if application_office is present. This commit is the last to remove all calls into calculator.passport_data values within the overseas passport outcomes.
There are no changes in the test artefacts by this PR, confirmed by the regression tests showing no changes to existing outcomes.
erkde
force-pushed
the
refactor_overseas_passports_flow_updates
branch
from
aaecd3d to
0684d19
Compare
|
I've updated the description, removing that comment which isn't really related to the changes in this PR but rather one of the things the PR doesn't change. |
erkde
pushed a commit
that referenced
this pull request
Mar 17, 2016
…ow_updates Refactor overseas passports flow updates
|
@erikse: It looks to me as if the regression test artefacts have changed and no longer match those in the repo. Are you aware of this? |
erkde
added a commit
that referenced
this pull request
Mar 17, 2016
Add response arg to the next_node for this question which was lost as part of a rebase of PR #2367 with master branch.
erkde
added a commit
that referenced
this pull request
Mar 17, 2016
Add response arg to the next_node for this question which was lost as part of a rebase of PR #2367 with master branch.
erkde
added a commit
that referenced
this pull request
Mar 17, 2016
Covert this question to use the new permitted: :auto option for the next_node block which was missed when rebasing PR #2367 with master.
erkde
added a commit
that referenced
this pull request
Mar 17, 2016
Covert this question to use the new permitted: :auto option for the next_node block which was missed when rebasing PR #2367 with master.
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 aims to tidy up the smart answer flow for overseas passports, extracting logic from the flow definition into a calculator class where it can be unit tested.
This PR supersedes #2342.