Refactor overseas passports flow #2342
Closed
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
Remove all content related to sending your application from the `making_your_application` partial, and add to partial `sending_your_application`. Without this content, the variable `application_address` doesn’t need to be passed to the `making_your_application` partial as a local.
Extract a `book_appointment_online?` method from the partials and into a Ruby class that can be used by the smart answer flow and passed to partials. At the moment, I’m leaving in the `current_location` checks within the partials, even thought the value is now available on calculator class, to a). to keep the commit small, and b). avoid those calls all together and instead add richer methods that don’t require lists of countries to be defined in partials.
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. Add note in the `cost` partial, that it’s list of countries for the same thing does not match to that in the calculator class, so for now leaving as is whilst clarifying with FCO.
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.
`current_location` can now be read from the overseas passports calculator so this is the first step at removing this variable being passed around the smart answer files by hosing it out of partial `making_your_application`. In future commits, we can further reduce calls to this variable with methods that return true/false for conditions instead of having data embedded in the partials to check against `current_location`.
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 ‘calculator’` 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.
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.
With the calculator instance now visible to outcomes, add method to find the name of the world_location based on the current location in the calculator, and remove the precalculation in the smart answer flow that passes `title_output` to the outcome `apply_in_neighbouring_country`.
This value is now available in the calculator method `current_location`, which is available in the outcome.
Replace `current_location` with call to calculator#current_location which now returns the same value and will allow us to remove the `current_location` variable.
Remove the precalculation of `organisation` from the overseas passport flow and replace with method on the `OverseasPassportCalculator` class.
Store the response for the birth_location question in the overseas passports calculator, so it can be access via the calculator variable which now has visibility in the outcomes, rather than precalculating a value for it in each outcome it is used in.
Extend the overseas passports calculator to accept a data_query instance via the constructor, or create one if not received. Add method `cash_only_country?` which delegates to the data_query instance and allows us to remove the reference to the data_query in outcome and partial and instead check this value on the calculator.
Add `renewing_country?` method to overseas passport calculator to delegate to the data_query object, and allow us to remove the data_query instance from the outcome and partial.
Remove the `application_action` state saved in the overseas passport flow and add an attr_accessor to store and access the value via the overseas passports calculator. Remove passing `application_action` from outcomes to partials.
Remove all hard-coded values in the smart answer flow and outcomes for overseas passports and replace with calls to calculator class which now stores the application_action.
Because overseas passport question `which_opt?` allows the user to select locations which aren't available with `WorldLocation.find`, we calculate a value for this variable from the calculator after the user answers the first location questions, so that users in Gaza, Jerusalem and Westbank will see an embassy address for The Occupied Palestine Territories rather a message 'No embassy is avaialble'
This doesn’t appear to be used in the smart answer flow, outcomes or partials, even the integration asserts it has a nil value.
This variable doesn’t appear to be used in the smart answer flow, outcomes or partials, and the integration only asserts it has a nil value.
With the calculator object available with the overseas passports flow, outcome and being passed to partials, there is no need to set this state in the flow. Remove references to `current_location` and replace with `calculator.current_location`.
Add `general_action` method to overseas passports calculator and remove logic in flow. Update references to `general_action` to `calculator.general_action` through outcomes and partials.
With `world_location` now available on the overseas passports calculator class, this variable is no longer needed within the smart answer flow or outcomes. Checking `world_location` is still valid is moved to a validate block which will keep the user on the same page if they enter an invalid value.
Remove `passport_data` state from smart answer flow and add method to overseas passport calculator along with unit tests. Remove all references to `passport_data` in flow, outcomes and partials and update to find value on calculator instead.
Add `application_type` method in overseas passport calculator so it can be tested with a unit test
Move logic of determining if the passport is an IPS application type from the smart answer flow in to a calculator method with a test around it.
Add `ips_number` method to the smart answer calculator so we can put a unit test around it and remove calculate block setting its value in the smart answer flow. Update outcomes and partials to retrieve values from the calculator. For now, partials in the cost/ don’t receive the calculator, so am leaving these as they are, receiving an ips_number rather than calculator for now.
Remove the save_input_as statement for this response in the overseas passport flow and add it as an attr_accessor on the calculator so it can be accessed in outcomes and partials without having to pass the value around.
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. 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 sets 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.
These no longer appear to be in use within the overseas passports smart answer.
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.
erkde
force-pushed
the
refactor_overseas_passports_flow
branch
from
8c0b416 to
b12a9b5
Compare
|
With PR #2341 merged, which this PRs branch was created from, I've just based this with master |
|
Minor: Quite a lot of the commit subjects talk about "adding" or "removing" things when actually they are really "moving" logic (often from a block to a method on the calculator). |
|
Ideally I'd like to see at least the non-minor comments addressed before this is merged, but overall this is a massive improvement. Great work, @erikse! 🎉 |
|
Thanks for the review and all your comments @floehopper. I've started addressing them and will reply to each one here, and then push changes to a new PR based off branch |
|
This PR is superseded by #2367 where I'm addressing the comments. |
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.
With the exception of the
overseas_passports_embassiesvalue calculated in question 1 (which are not currently available for the location options of question 1a), the calculator class provides a complete encapsulation of the logic required by the flow, outcomes and partials in overseas passports.This PR supersedes #2331, and is based on #2341, which should be merged before this PR.