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.

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

Remove Japan from marriage-abroad outcome_os_consular_cni #2349

Merged

Conversation

Projects
None yet
2 participants
@chrisroos
Copy link
Contributor

commented Mar 10, 2016

At some point, information about opposite sex marriages in Japan was moved from the outcome_os_consular_cni outcome to outcome_os_japan. This branch removes the now unused code from outcome_os_consular_cni.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

@erikse: Did you add the LGTM label because you're happy with this change? In general, I'd find it useful if there was a comment also saying that the PR looks good.

@erikeide

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

@chrisroos I'm not sure how the label was added, maybe hit a shortcut by accident, have removed.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

OK, thanks @erikse.

Can someone review this change, please? @floehopper / @erikse / @ikennaokpala / @pmanrubia.

@erikeide erikeide self-assigned this Mar 16, 2016

@erikeide

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

Using git show --ignore-all-space on the second commit makes it a lot easier to follow.

I hope some of the conditionals left over now, like:

%w(germany).include?(calculator.ceremony_country)

and

(cni_notary_public_countries + %w(macedonia) - %w(greece tunisia)).include?(calculator.ceremony_country)

can be improved in a PR at some point. Otherwise, looks good to me

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2016

Thanks, @erikse!

I certainly hope to continue improving some of the remaining conditionals in future. In fact, one of those that you mention %w(germany).include?(calculator.ceremony_country) has been removed in PR #2352.

I'm going to get this rebased on master and merged.

chrisroos added some commits Mar 10, 2016

Remove unreachable blocks of code
Opposite sex marriages in Japan have their own outcome
(outcome_os_japan) which means that `ceremony_country` will never be
"japan" in this outcome.
Remove unnecessary conditions
Opposite sex marriages in Japan have their own outcome
(outcome_os_japan) which means that `ceremony_country` will never be
"japan" in this outcome.

I've removed these unecessary conditions and de-indented the inner
blocks of code where necessary. Use `git show --ignore-all-space` to see
the diff more clearly.
Remove Japan from list of countries
Opposite sex marriages in Japan have their own outcome
(outcome_os_japan) which means that `ceremony_country` will never be
"japan" in this outcome.
Update checksum data for marriage-abroad
All regression tests pass so I can safely update the checksum data.

@chrisroos chrisroos force-pushed the remove-japan-from-marriage-abroad-outcome-os-consular-cni branch from 29302d1 to ab5d2e6 Mar 17, 2016

chrisroos added a commit that referenced this pull request Mar 17, 2016

Merge pull request #2349 from alphagov/remove-japan-from-marriage-abr…
…oad-outcome-os-consular-cni

Remove Japan from marriage-abroad outcome_os_consular_cni

@chrisroos chrisroos merged commit 0c913f0 into master Mar 17, 2016

1 check passed

default "Build #4471 succeeded on Jenkins"
Details

@chrisroos chrisroos deleted the remove-japan-from-marriage-abroad-outcome-os-consular-cni branch Mar 17, 2016

chrisroos added a commit that referenced this pull request Mar 21, 2016

Remove Japan from array in :notary_public_inclusion
This should've been removed as part of PR #2349 but I overlooked it.

chrisroos added a commit that referenced this pull request Mar 21, 2016

Squashed commit of the following:
TODO: Remove this squashed commit bringing in changes from the
remove-marriage-abroad-outcome-os-consular-cni-precalculate-blocks
branch.

commit 8b57641
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 18:24:56 2016 +1100

    Update checksum data for marriage-abroad

    TODO: Double check the regression tests.

    Updated using:

        $ rails r script/generate-checksums-for-smart-answer.rb \
        marriage-abroad

commit 153f3bb
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 18:19:06 2016 +1100

    Add #document_download_link_if_opposite_sex_resident_of_uk_countries?

    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

commit 55dc50d
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 18:06:30 2016 +1100

    Add #notary_public_ceremony_country?

    Use it instead of the `:notary_public_inclusion` precalculate block.

    Part of refactoring marriage-abroad. See doc/refactoring.md for more.

commit ff7f413
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:48:36 2016 +1100

    Inline :cni_notary_public_countries

    It's only used in the `:notary_public_inclusion` precalculate block.

commit f6c45f3
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:47:11 2016 +1100

    Use notary_public_inclusion

    Remove duplication by using `notary_public_inclusion` instead of
    `cni_notary_public_countries.include?(calculator.ceremony_country`.

commit ef9297f
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:45:16 2016 +1100

    Split condition

    This will make it easier to remove duplication between the use of
    `cni_notary_public_countries` and `notary_public_inclusion`.

commit 20b9094
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:44:03 2016 +1100

    Split condition onto multiple lines

    To make it easier to see the changes I want to make to it.

commit 962ea8a
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:42:23 2016 +1100

    Add parenthesis to condition

    In preparation for splitting this onto multiple lines. This'll make it
    easier to see other changes I plan to make to these conditions.

commit 5624a80
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:39:02 2016 +1100

    Move Macedonia condition out of :notary_public_inclusion

    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.

commit fea1d12
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 10:34:10 2016 +1100

    Remove Japan from array in :notary_public_inclusion

    This should've been removed as part of PR #2349 but I overlooked it.

commit dfc240b
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 09:12:58 2016 +1100

    Add #birth_certificate_required_as_supporting_document?

    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.

commit 9a50aa8
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 08:52:02 2016 +1100

    Inline :ceremony_and_residency_in_croatia

    This feels too specific to add to the `MarriageAbroadCalculator`.

commit 4978212
Author: Chris Roos <chris@seagul.co.uk>
Date:   Sat Mar 19 08:48:05 2016 +1100

    Add #cni_posted_after_14_days?

    Use it instead of the `:cni_posted_after_14_days_countries` precalculate
    block.

    Part of refactoring marriage-abroad. See doc/refactoring.md for more.

commit 74db26a
Author: Chris Roos <chris@seagul.co.uk>
Date:   Fri Mar 18 08:20:04 2016 +1100

    Inline :three_day_residency_handled_by_exception

    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.

commit 801afe6
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:56:09 2016 +1100

    Add #three_day_residency_requirement_applies?

    Use it instead of the `:three_day_residency_requirement_applies`
    precalculate block.

    Part of refactoring marriage-abroad. See doc/refactoring.md for more.

commit f5964bb
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:46:27 2016 +1100

    Inline :no_birth_cert_requirement

    It's only used in the `:birth_cert_inclusion` precalculate block.

commit 486ad01
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:45:10 2016 +1100

    Return boolean from notary_public_inclusion

    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.

commit f3f00af
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:43:41 2016 +1100

    Return boolean from :birth_cert_inclusion

    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.

commit 4147bfe
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:36:45 2016 +1100

    Add CNI_POSTED_AFTER_14_DAYS_COUNTRIES

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit 8362ae6
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:35:34 2016 +1100

    Add NO_DOCUMENT_DOWNLOAD_LINK_IF_OS_RESIDENT_OF_UK_COUNTRIES

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit 9de2002
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:34:39 2016 +1100

    Add CNI_NOTARY_PUBLIC_COUNTRIES

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit d655186
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:34:00 2016 +1100

    Add NO_BIRTH_CERT_REQUIREMENT

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit 3aae842
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:31:38 2016 +1100

    Add THREE_DAY_RESIDENCY_HANDLED_BY_EXCEPTION

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit 68c2c3f
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:30:58 2016 +1100

    Add THREE_DAY_RESIDENCY_REQUIREMENT_COUNTRIES

    Move this array of countries from the flow to the
    `MarriageAbroadDataQuery`.

commit bb9d2cb
Author: Chris Roos <chris@seagul.co.uk>
Date:   Thu Mar 10 13:32:28 2016 +1100

    Convert OS_21_DAYS_RESIDENCY_REQUIRED_COUNTRIES to array

    I can only imagine this was an oversight when the array/string was first
    added in ff781c5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.