Extract marriage-abroad payment information from templates #2502
Conversation
Add full set of input countries using:
$ rails r script/generate-questions-and-responses-for-smart-answer.rb \
marriage-abroad
Using:
$ rails r script/generate-responses-and-expected-results-for-smart-answer.rb \
marriage-abroad
This appears to be a special case for the Philippines. I'm extracting it in preparation for moving this payment method information to the marriage_abroad_services.yml file.
…peak.erb This appears to be specific to South Korea, although it's incredibly similar to the existing `_pay_by_cash_or_credit_card_no_cheque` partial. I'll make a note to follow that up separately. This is in preparation for moving the payment method information to marriage_abroad_services.yml.
The `outcome_opposite_sex_marriage_in_affirmation_countries` outcome is the
only outcome that renders the `fees` partial. I used the following command to
determine the countries that lead to this outcome:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_opposite_sex_marriage_in_affirmation_countries
I created the add-payment-partials-to-marriage-abroad-services-data.rb to
update the data in marriage_abroad_services.yml. I plan to update this script
as I continue to extract the payment info from the templates.
Running the script in this commit results in no output which means that the
payment information for each of the countries listed has been successfully
added to marriage_abroad_services.yml.
So that I can reuse it in other templates.
In preparation for moving this information to the marriage_abroad_services.yml file.
In preparation for extracting this payment info to the marriage_abroad_services.yml file.
In preparation for moving this data to the marriage_abroad_services.yml file.
…ni_countries_when_residing_in_uk_or_ceremony_country
I determined the list of countries that can lead to this outcome by using:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_opposite_sex_marriage_in_consular_cni_countries_when_residing_in_uk_or_ceremony_country
I've updated the add-payment-partials-to-marriage-abroad-services-data.rb
script to extract the payment info for these countries.
The output of running the script was:
Entry not found for afghanistan.
Entry not found for american-samoa.
Entry not found for andorra.
Entry not found for benin.
Entry not found for bhutan.
Entry not found for brazil.
Entry not found for burkina-faso.
Entry not found for cape-verde.
Entry not found for central-african-republic.
Entry not found for chad.
Entry not found for comoros.
Entry not found for congo.
Entry not found for djibouti.
Entry not found for equatorial-guinea.
Entry not found for eritrea.
Entry not found for gabon.
Entry not found for guinea.
Entry not found for guinea-bissau.
Entry not found for haiti.
Entry not found for iraq.
Entry not found for libya.
Entry not found for liechtenstein.
Entry not found for mali.
Entry not found for marshall-islands.
Entry not found for mauritania.
Entry not found for micronesia.
Entry not found for niger.
Payment method partial already set for norway. Existing value: pay_by_visas_or_mastercard. New value: pay_by_cash_or_credit_card_no_cheque.
Entry not found for palau.
Entry not found for paraguay.
Entry not found for rwanda.
Entry not found for san-marino.
Entry not found for sao-tome-and-principe.
Entry not found for south-sudan.
Entry not found for suriname.
Entry not found for timor-leste.
Entry not found for togo.
Entry not found for western-sahara.
I've confirmed that the changes the regression test artefacts are all expected
based on the output from running the script. The payment information has been
removed from all those listed, apart from Norway. The payment information for
Norway has changed. I plan to follow up on this to work out what the payment
information should be.
…_residing_in_uk_or_italy
In preparation for moving this data to the marriage_abroad_services.yml file.
…n_residing_in_uk_or_kosovo
…tries Although this outcome is used for a number of ceremony countries, only Saudi Arabia displays the _consular_fees_table partial, so that's the only country we need to add payment information for in marriage_abroad_services.yml.
…n_residing_in_uk_or_poland
…hen_residing_in_uk_or_slovenia
…mation_countries
I determined the countries that lead to this outcome using:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_same_sex_civil_partnership_in_affirmation_countries
belgium
norway
The payment partial to use for both Belgium and Norway is already in
marriage_abroad_services.yml.
…lar_countries
I used the following script to determine which countries lead to this outcome:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_same_sex_civil_partnership_in_consular_countries
I updated the add-payment-partials-to-marriage-abroad-services-data.rb script
and ran it. There was no output and the marriage_abroad_services.yml file
didn't change which means that the payment information for these countries must
already be present.
I determined the countries that lead to this outcome by running:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_same_sex_civil_partnership
I updated the add-payment-partials-to-marriage-abroad-services-data.rb script
and ran it. This was the output:
Entry not found for brazil.
Payment method partial already set for finland. Existing value:
pay_in_euros_or_visa_electron. New value: pay_by_cash_or_credit_card_no_cheque.
I'll add a separate task to investigate the difference for Finland.
I've updated the regression test artefacts. The changes to Brazil and Finland
are expected based on the output from the script above.
…ship
I determined which countries lead to this outcome by using:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_same_sex_marriage_and_civil_partnership
I updated and ran the add-payment-partials-to-marriage-abroad-services-data.rb
script, which gave the following output:
Payment method partial already set for japan. Existing value: pay_by_credit_card_but_not_personal_cheque. New value: pay_by_cash_or_credit_card_no_cheque.
Payment method partial already set for latvia. Existing value: pay_in_local_currency_ceremony_country_name. New value: pay_by_cash_or_credit_card_no_cheque.
Payment method partial already set for philippines. Existing value: pay_by_cash_or_bank_cheque. New value: pay_by_cash_or_credit_card_no_cheque.
Payment method partial already set for russia. Existing value: pay_by_mastercard_and_visa_only. New value: pay_by_cash_or_credit_card_no_cheque.
I'll add a separate task to investigate the correct information for each of
these countries.
I've updated the regression test artefacts as part of this commit:
* Payment info for same sex ceremonies in Japan, Latvia, Philippines and Russia
has been updated. The info displayed is the info already in the
marriage_abroad_services.yml file.
* Payment info for same sex ceremonies in Vietnam has been added. This looks
correct as we're displaying the services/fees section for these outcomes.
…brazil_or_third_country This outcome doesn't display the _consular_fees_table partial so shouldn't render the payment information partial either. NOTE. The link_to_consular_fees_pay_in_local_currency partial should almost certainly be removed too but I'll deal with that separately.
…n_residing_in_ceremony_or_third_country
This outcome doesn't display the _consular_fees_table partial so shouldn't
render the payment information partial either.
NOTE. The link_to_consular_fees_pay_in_local_currency partial should almost
certainly be removed too but I'll deal with that separately.
I've confirmed that the changes to the regression test artefacts look correct.
I compared the list of countries that lead to this outcome to the changes to
the artefacts:
I used the following command to get the list of countries leading to this
outcome:
$ rails r script/marriage-abroad-countries-leading-to-outcome.rb \
outcome_opposite_sex_in_no_cni_countries_when_residing_in_ceremony_or_third_country
NOTE. The remainder of this commit note was written when I had a complete set
(i.e. for every ceremony country) of regression test artefacts for
marriage-abroad. I think the information is still useful so I'm leaving it in.
The following countries lead to this outcome and have had the payment
information removed from their artefacts:
* afghanistan
* american-samoa
* andorra
* benin
* bhutan
* burkina-faso
* cape-verde
* central-african-republic
* chad
* comoros
* congo
* djibouti
* equatorial-guinea
* eritrea
* gabon
* guinea
* guinea-bissau
* haiti
* iraq
* liechtenstein
* mali
* marshall-islands
* mauritania
* micronesia
* niger
* palau
* paraguay
* rwanda
* san-marino
* sao-tome-and-principe
* south-sudan
* suriname
* timor-leste
* togo
* western-sahara
The following countries lead to this outcome but are in the
`COUNTRIES_WITHOUT_CONSULAR_FACILITIES` array (i.e.
`#country_without_consular_facilities?` is true) so they weren't displaying any
payment information anyway.
* aruba
* bonaire-st-eustatius-saba
* burundi
* curacao
* saint-barthelemy
* st-maarten
* st-martin
I prefer having these related partials in a single directory.
This has served its purpose and can be safely removed.
This reverts commit 401e6dfd570d40f1cce5fca92ae7ccf6df0859a6. This has served its purpose now that I've extracted all the payment information from the marriage-abroad outcome templates.
This reverts commit 4685f7e0e793989e3ca10162ef06383d10583988. This has served its purpose now that I've extracted all the payment information from the marriage-abroad outcome templates.
The marriage-abroad regression tests are passing.
| if @services_data[ceremony_country].present? | ||
| @services_data[ceremony_country]['payment_partial_name'] | ||
| end | ||
| end |
There was a problem hiding this comment.
Minor: I think you could use the (relatively new) Ruby Hash#dig method here to make the code a bit more succinct:
def services_payment_partial_name
@services_data.dig(ceremony_country, 'payment_partial_name')
endThere was a problem hiding this comment.
Minor: Since this is arguably presentational logic, did you consider making this a view helper method? I think I'm happy with the approach you've taken, because you've talked about ideally wanting to store the source data (as opposed to the partial template name) in the YAML.
There was a problem hiding this comment.
Minor: Thinking about it a bit more, rather than use Hash#dig, perhaps we could extract a method like this to avoid the if statement:
def ceremony_country_data
@services_data[ceremony_country] || {}
end
def services_payment_partial_name
ceremony_country_data['payment_partial_name']
end
There was a problem hiding this comment.
Minor: what about renaming payment_partial_name into payment_type? And getting the partial name using something like the code below in the calculator?
def services_payment_partial_name
ceremony_country_data['payment_type']
endNot sure about talking about partials in the config file?
There was a problem hiding this comment.
Minor: Since this is arguably presentational logic, did you consider making this a view helper method? I think I'm happy with the approach you've taken, because you've talked about ideally wanting to store the source data (as opposed to the partial template name) in the YAML.
@floehopper: It didn't cross my mind to add it as a helper method. The MarriageAbroadCalculator felt like the natural place for it given how closely this relates to the other service information the calculator is responsible for. If we are able to store acceptable payment types, instead of names of partials, then I can imagine using the MarriageAbroadCalculator to retrieve the data and a new helper method to turn it into a sentence.
There was a problem hiding this comment.
Minor: what about renaming
payment_partial_nameintopayment_type?
Not sure about talking about partials in the config file?
@pmanrubia: I agree that mentioning partials in the data isn't ideal. Given that I can't see an easy way round it at the moment I prefer to make it explicit that the data represents the name of a partial to render. Using something like payment_type might lead someone to think they can store an arbitrary string against that key.
There was a problem hiding this comment.
Minor: Thinking about it a bit more, rather than use
Hash#dig, perhaps we could extract a method like this to avoid the if statement:
@floehopper: I've extracted such a method in 3fb1059.
|
As you will probably have gathered from my previous comments, I found this PR a bit hard to follow:
|
|
Thanks for reviewing, @floehopper and @pmanrubia. I'm going to close this open a new PR that addresses your feedback. |
chrisroos
deleted the
extract-marriage-abroad-payment-information-from-templates
branch
chrisroos
restored the
extract-marriage-abroad-payment-information-from-templates
branch
chrisroos
deleted the
extract-marriage-abroad-payment-information-from-templates
branch
|
|
||
| countries.each do |country| | ||
| payment_partial_name = payment_method_partial[country] || | ||
| payment_method_partial['default'] |
There was a problem hiding this comment.
What's the purpose of this default value? It doesn't look as if it's being used in the app code.
@floehopper: It's used on this line in the temporary script. It's the default value stored in the marriage_abroad_services.yml file when no country specific value exists.
@floehopper: I've attempted to explain the use of this script in the following paragraph in the description of PR #2507:
I've also got a separate note to add some more information about that script. Either to the script itself, or to some docs. |
@floehopper: I've attempted to improve this slightly in PR #2507 by making it clearer when the commit extracts a partial, versus when it extracts payment information from a template. I can see that it's still not very clear but hopefully it's slightly better. |
@floehopper: I've attempted to improve this in PR #2507. See 9a91b1f for an example. |
@floehopper: I've renamed the branch in PR #2507 to (hopefully) make its purpose clearer. |
@floehopper. That's correct in the case of some commits. I think it would've been possible to create a branch that just refactored how the payment information was displayed before then separately removing it for those countries that shouldn't display it. While possible, I think this would've been quite a lot more work as I would've had to support displaying different payment information for a single country. I believe this only exists by mistake (i.e. templates have been split causing differences between the information displayed for the same country). |
This has been superseded by PR #2507, which should hopefully incorporate all the feedback from this PR.
Trello card: https://trello.com/c/K2bUzopZ
The Trello card is about removing payment information and links to consular fees from outcomes that don't display services/fees.
This branch removes the payment information but not the links to consular fees. I plan to deal with that in a separate branch.
Overview of the changes in this branch:
I would've preferred to store a set of acceptable payment methods in the YAML file (rather than the partial name) but that doesn't seem possible at the moment given how varied the payment information can be.
I've only added the payment information to the ceremony countries that already have entries in marriage_abroad_services.yml (i.e. the services/fees section is displayed for at least some of the outcomes). This ensures that we're no longer displaying the payment information on outcomes where the services/fees section isn't displayed.
While working on this branch, I used the complete set of regression test artefacts for marriage-abroad so that I could sanity check the changes I was making. I've removed these from the commits now that I'm happy with the changes that have been made.
This branch has exposed a couple of other issues.
I've added comments to the Trello card to ask how we address both of these issues.
I plan to add another Trello card to work out whether we can reduce the number of payment information partials in the system.