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

Only display marriage-abroad payment info when services available #2578

Conversation

Projects
None yet
2 participants
@chrisroos
Copy link
Contributor

commented Jun 13, 2016

Trello card: https://trello.com/c/K2bUzopZ

Some outcomes are incorrectly displaying payment information and links to consular fees even though there are no chargeable services available in that ceremony country for the given responses.

This branch ensures we only display payment information and links to consular fees when there are chargeable services available.

  • The first and second commits contain some small refactoring changes.
  • The third commit ensures we only display the payment information partial when there are chargeable services.
  • The fourth commit ensures we only display the link to consular fees when there are chargeable services.
  • The fifth commit updates the checksum data given that the marriage-abroad regression tests are passing.

I plan to refactor these partials in the near future. I'm aiming for a single "fees" partial that will encapsulate the logic for displaying the table of services/fees, link to consular fees and payment information partial.

Fact check

  • This gist contains a list of all the outcomes where the link to consular fees and/or the payment information have been removed.

Expected changes

Example 1 - Opposite sex ceremonies in Albania for residents of UK

Before - page on GOV.UK

  • Note the link to consular fees ("You normally have to pay fees") toward the bottom of the page
  • Note the payment information ("You can pay by cash") toward the bottom of the page

pr-2578-example-1-before

After - page on Heroku

  • Note the absence of both the link to consular fees and payment information

pr-2578-example-1-after

Example 2 - Any ceremony in Afghanistan

Before - page on GOV.UK

  • Note the link to consular fees ("You normally have to pay fees") toward the bottom of the page

pr-2578-example-2-before

After - page on Heroku

  • Note the absence of the link to consular fees

pr-2578-example-2-after

@@ -4,7 +4,7 @@

<% unless calculator.country_without_consular_facilities? %>
<% if calculator.ceremony_country == 'vietnam' %>
<%= render partial: 'consular_fees_in_cash_visa_master.govspeak.erb',
<%= render partial: 'link_to_consular_fees_for_vietnam.govspeak.erb',

This comment has been minimized.

Copy link
@floehopper

floehopper Jun 14, 2016

Contributor

This change doesn't make sense to me. As far as I can see the content of the template (i.e. the "implementation") is not specific to Vietnam - it's only Vietnam-specific in the sense that it is only used in Vietnam-related outcomes. This doesn't seem like a great justification for the new name, although I understand the motivation of wanting a common prefix to match link_to_consular_fees.

This comment has been minimized.

Copy link
@chrisroos

chrisroos Jun 29, 2016

Author Contributor

The "consular_fees_in_cash_visa_master" partial has been renamed in PR #2592. It's possible that'll be merged before this PR (they're both out for fact check) so I'm not going to change anything in this PR just yet.

This comment has been minimized.

Copy link
@chrisroos

chrisroos Jul 12, 2016

Author Contributor

I've removed the commit that renamed this partial. I considered renaming the partial so that it followed the rename in PR #2592 but figured that would probably make merging #2592 harder.

@floehopper floehopper self-assigned this Jun 14, 2016

@floehopper

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

Other than my one comment, this looks good to me.

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2016

Si Stephens has confirmed that this is good to merge.

I'm going to rebase on master in preparation for merging.

@chrisroos chrisroos force-pushed the only-display-marriage-abroad-payment-info-when-services-available branch from 803c578 to 2050977 Jul 12, 2016

@chrisroos

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2016

I've rebased this on master and force pushed in preparation for merging.

chrisroos added some commits Jun 3, 2016

Use link_to_consular_fees partial
To DRY up four of the outcome templates.

The
outcome_opposite_sex_marriage_in_consular_cni_countries_when_residing_in_uk_or_ceremony_country
template still contains a duplicate of the text in
link_to_consular_fees but it's hardcoded to Kazakhstan so
I'm not yet able to remove that duplication.
Only display payment info when "Fees" section present
Trello: https://trello.com/c/K2bUzopZ

Only display the content of the payment_information partial if there are
chargeable services available in the ceremony country (i.e.
`calculator.services.any?` returns `true`).

Prior to this change we'd always show the payment information partial defined
in marriage_abroad_services.yml, irrespective of whether the available services
applied to the current responses/circumstances.

To sanity check this change, I generated the full set of regression test
artefacts for marriage-abroad and compared the before and after. Based on those
changes, I'm confident that this behaviour is correct.

== Regression test artefact changes

The following countries only have services available for opposite sex
ceremonies when the user is resident of the ceremony country. For these
countries, the payment information partial has been removed from all opposite
sex outcomes where the user is resident of the UK.

* Albania
* Armenia
* Austria
* Azerbaijan
* Croatia
* Denmark
* Estonia
* Greece
* Guatemala
* Iceland
* Jordan
* Kazakhstan
* Kosovo
* Kuwait
* Kyrgyzstan
* Latvia
* Lithuania
* Luxembourg
* Macedonia
* Mexico
* Montenegro
* Poland
* Romania
* Russia
* Serbia
* Spain
* Sweden
* Tunisia
* Turkmenistan
* Uzbekistan

The following countries don't have any services available for opposite sex
ceremonies. For these countries, the payment information partial has been
removed from all opposite sex outcomes.

* Germany
* Nicaragua
Only display link to consular fees when "Fees" section present
Trello: https://trello.com/c/K2bUzopZ

Only display the link to consular fees if there are chargeable services
available in the ceremony country (i.e. `calculator.services.any?` returns
`true`).

Prior to this change, we were relying on logic in the template to decide
whether to render the link to consular fees partial(s).

To sanity check this change, I generated the full set of regression test
artefacts for marriage-abroad and compared the before and after. Based on those
changes, I'm confident that this behaviour is correct.

== Regression test artefact changes

The following countries only have services available for opposite sex
ceremonies when the user is resident of the ceremony country. For these
countries, the link to consular fees has been removed from all opposite sex
outcomes where the user is resident of the UK.

* Albania
* Armenia
* Austria
* Azerbaijan
* Croatia
* Denmark
* Estonia
* Greece
* Guatemala
* Iceland
* Jordan
* Kazakhstan
* Kosovo
* Kuwait
* Kyrgyzstan
* Latvia
* Lithuania
* Luxembourg
* Macedonia
* Mexico
* Montenegro
* Poland
* Romania
* Russia
* Serbia
* Spain
* Sweden
* Tunisia
* Turkmenistan
* Uzbekistan

The following countries don't have any services available for opposite sex
ceremonies. For these countries, the link to consular fees has been removed
from all opposite sex outcomes for these countries:

* Germany
* Nicaragua

The following countries don't have any services available. For these countries,
the link to consular fees has been removed from all the outcomes that were
previously displaying it.

* American Samoa
* Brazil
* Paraguay
* San Marino
Update regression test checksums for marriage-abroad
I'm confident that the regression tests for marriage-abroad are all passing so
I'm happy to update this checksum data.

@chrisroos chrisroos force-pushed the only-display-marriage-abroad-payment-info-when-services-available branch from 2050977 to 53f0570 Jul 12, 2016

@chrisroos chrisroos merged commit 97ef48d into master Jul 12, 2016

1 check passed

default "Build #5690 succeeded on Jenkins"
Details

@chrisroos chrisroos deleted the only-display-marriage-abroad-payment-info-when-services-available branch Jul 12, 2016

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.