Remove marriage-abroad outcome_os_consular_cni precalculate blocks #2388
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
I can only imagine this was an oversight when the array/string was first added in ff781c5.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
Move this array of countries from the flow to the `MarriageAbroadDataQuery`.
The outcome_os_consular_cni template doesn't use the string that was being returned: it's only interested in whether this value is truthy or falsey.
The outcome_os_consular_cni template doesn't use the string that was being returned: it's only interested in whether this value is truthy or falsey.
It's only used in the `:birth_cert_inclusion` precalculate block.
Use it instead of the `:three_day_residency_requirement_applies` precalculate block. Part of refactoring marriage-abroad. See doc/refactoring.md for more.
I _think_ this makes it clearer that the countries in this array are related to the countries listed in the conditionals just before this block of code in outcome_os_consular_cni. It also makes it clearer that 'italy' shouldn't be in this list as that now has its own outcome.
Use it instead of the `:cni_posted_after_14_days_countries` precalculate block. Part of refactoring marriage-abroad. See doc/refactoring.md for more.
This feels too specific to add to the `MarriageAbroadCalculator`.
This is confusing as the NO_BIRTH_CERT_REQUIREMENT array contains a list of countries that don't require a birth certificate as a supporting document. We then check that the ceremony country isn't in that list in order to determine whether a birth certificate is required. I'm sure this can be improved but I think it's OK for now.
This should've been removed as part of PR #2349 but I overlooked it.
I _think_ Macedonia is in this list because it's handled elsewhere in the outcome_os_consular_cni template, rather than because it's a `:notary_public_inclusion` country. There are already other special cases dealing with Macedonia in the template and I'm hoping this'll make it easy to remove more duplication further down the line.
In preparation for splitting this onto multiple lines. This'll make it easier to see other changes I plan to make to these conditions.
To make it easier to see the changes I want to make to it.
This will make it easier to remove duplication between the use of `cni_notary_public_countries` and `notary_public_inclusion`.
Remove duplication by using `notary_public_inclusion` instead of `cni_notary_public_countries.include?(calculator.ceremony_country`.
It's only used in the `:notary_public_inclusion` precalculate block.
Use it instead of the `:notary_public_inclusion` precalculate block. Part of refactoring marriage-abroad. See doc/refactoring.md for more.
Use it instead of the `:no_document_download_link_if_os_resident_of_uk_countries` precalculate block. Part of refactoring marriage-abroad. See doc/refactoring.md for more. I struggled with the naming of this method as the logic seems confusing. The `NO_DOCUMENT_DOWNLOAD_LINK_IF_OS_RESIDENT_OF_UK_COUNTRIES` array contains a list of ceremony countries that _don't_ allow you to download the notice of and affidavit for marriage forms. We check that the ceremony country _isn't_ in this list in order to decide whether to display the download
TODO: Double check the regression tests.
Updated using:
$ rails r script/generate-checksums-for-smart-answer.rb \
marriage-abroad
|
Other than my minor comments, this looks good to me. |
|
Thanks for reviewing, @floehopper. I've rebased on master and addressed feedback so I'm going to close this and open a new PR in its place. |
chrisroos
deleted the
remove-marriage-abroad-outcome-os-consular-cni-precalculate-blocks
branch
|
This has been superseded by PR #2394. |
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 has been superseded by PR #2394.
I'm continuing to refactor marriage-abroad. This branch removes the
precalculateblocks from outcome_os_consular_cni by adding logic to theMarriageAbroadCalculator.