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
Convert marriage-abroad to use ERB templates #1882
Merged
chrisroos
merged 56 commits into
master
from
convert-marriage-abroad-to-use-erb-templates
Aug 17, 2015
Merged
Convert marriage-abroad to use ERB templates #1882
chrisroos
merged 56 commits into
master
from
convert-marriage-abroad-to-use-erb-templates
Aug 17, 2015
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
5 tasks
Except for the minor comments above this LGTM, good job 👍 It was also useful to look through the changes and I've made some notes about potential improvements #1885. |
Thanks for reviewing this, @tadast. I'll address your comments and get this merged to master :-) |
== Notes * I've introduced the MarriageAbroadHelper and added the `ceremony_type` helper method. This replicates the behaviour of the `ceremony_type` calculate block in the `partner_opposite_or_same_sex?` question. I considered using a partial instead of helper method but the method feels like a better fit. == Inlined the following outcome-specific phrases: * outcome_ireland_opposite_sex * outcome_ireland_same_sex
== Notes * I've introduced the `ceremony_type_lowercase` helper method to replicate the functionality offered by the calculate block of the same name in the `partner_opposite_or_same_sex?` question. == Added the following partials: * what_you_need_to_do == Inlined the following outcome-specific phrases: * switzerland_not_resident * switzerland_not_resident_two * switzerland_os_not_resident * switzerland_os_variant * switzerland_ss_not_resident * switzerland_ss_variant * what_you_need_to_do_switzerland_resident_uk
== Added the following partials: * contact_local_authorities * get_legal_advice * partner_naturalisation_in_uk
== Notes * I've noticed that the `portugal_phraselist` precalculate block isn't being used but I'll remove that separately. == Inlined the following outcome-specific phrases: * marriage_title * same_sex_marriage_title
This isn't being used anywhere.
chrisroos
force-pushed
the
convert-marriage-abroad-to-use-erb-templates
branch
from
August 17, 2015 15:47
99cfae8
to
a41a110
Compare
== Notes: * I've introduced the '_contact_method.govspeak.erb' partial to replicate the `contact_method_key` calculate block in the marriage-abroad flow. * I've had to update the test artefacts as a leading space is now being stripped from the '10310' in the address. == Added the following partials: * documents_for_divorced_or_widowed * fee_table_45_70_55 == Inlined the following outcome-specific phrases: * appointment_for_affidavit_indonesia * partner_affidavit_needed
== Updated the marriage-abroad integration test: * I've excluded 'laos' from `OS_COUNTRIES_WITH_APPOINTMENTS` in the "when appointment links for opposite sex marriage exist" test. == Added the following partials: * affirmation_os_translation_in_local_language_text * change_of_name_evidence * contact_embassy_of_ceremony_country_in_uk_marriage * contact_local_authorities_in_country_marriage * divorced_or_widowed_evidences * docs_decree_and_death_certificate * fee_and_required_supporting_documents_for_appointment * fee_table_affirmation_55 * get_legal_and_travel_advice * legalisation_and_translation * link_to_consular_fees * names_on_documents_must_match * pay_by_cash_or_credit_card_no_cheque == Inlined the following outcome-specific phrases: * what_to_do_laos
== Inlined the following outcome-specific phrases: * fee_table_oath_declaration_55 * japan_legal_advice * payment_methods_japan * what_happens_next_os_local_japan * what_to_do_os_local_japan
== Inlined the following outcome-specific phrases: * kosovo_local_resident * kosovo_uk_resident
To make it easier to understand when I convert this outcome to ERB.
== Inlined the following outcome-specific phrases: * consular_cni_os_download_affidavit_notary_public * contact_local_authorities * download_affidavit_brazil * download_affidavit_forms_but_do_not_sign * make_an_appointment_bring_passport_and_pay_55_brazil * notary_public_will_charge_a_fee
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added and additional blank lines don't affect the HTML that's produced.
== Inlined the following outcome-specific phrases: * make_an_appointment_bring_passport_and_pay_55_colombia
This should make it slightly easier to understand when I convert this outcome to use an ERB template.
== Inlined the following outcome-specific phrases: * monaco_marriage * monaco_pacs
== Inlined the following outcome-specific phrases: * cant_issue_cni_for_commonwealth * cant_issue_cni_for_zimbabwe * commonwealth_os_marriage_subtleties_in_brunei * commonwealth_os_marriage_subtleties_in_cyprus * commonwealth_os_marriage_subtleties_in_india * commonwealth_os_marriage_subtleties_in_malaysia * commonwealth_os_marriage_subtleties_in_singapore * commonwealth_os_marriage_subtleties_in_south_africa * contact_high_comission_of_ceremony_country_in_uk * contact_zimbabwean_embassy_in_uk
== Inlined the following outcome-specific phrases: * bot_os_ceremony_biot * bot_os_ceremony_bvi * bot_os_ceremony_non_biot
== Notes: * I had to add the `precalculate :data_query` block so that it's available to the template. == Added the following partials: * cni_os_consular_facilities_unavailable * you_may_be_asked_for_cni == Inlined the following outcome-specific phrases: * getting_cni_from_costa_rica_when_in_third_country * standard_ways_to_get_cni_in_third_country * what_you_need_to_do_to_marry_in_norway_when_in_third_country * what_you_need_to_do_to_marry_in_norway_when_in_third_country
I'll need access to these variables as I make the switch to using ERB templates.
== Updated the marriage-abroad integration test: * Removed checks for `:consular_cni_os_start` and `:consular_cni_os_remainder` in the marriage-abroad integration test now that they've been removed from the `outcome_os_consular_cni` outcome. * Added a condition around the "when appointment links for opposite sex marriage exist" test. A number of these were failing because they reached `outcome_os_consular_cni` and we're no longer building in a PhraseList in that outcome. The condition ensures that the test continues to run for other outcomes. == Added the following partials: * callout_partner_equivalent_document * cni_at_local_register_office * cni_issued_locally_validity * display_notice_of_marriage_7_days * download_and_fill_notice_and_affidavit_but_not_sign - Used twice in this outcome * gulf_states_os_consular_cni * gulf_states_os_consular_cni_local_resident * legalise_translate_and_check_with_authorities * legisation_and_translation_intro_uk * pay_in_cash_visa_or_mastercard * pay_in_euros_or_visa_electron * pay_in_local_currency_ceremony_country_name * required_supporting_documents * required_supporting_documents_egypt - instead of dynamically constructing phrase key == Inlined the following outcome-specific phrases: * arrange_cni_via_costa_rica * bilingual_statutory_declaration_download_for_italy * check_if_cni_needs_to_be_legalised * check_with_notary_public_if_you_need_cni * cni_exception_for_permanent_residents_estonia * cni_subject_to_objection_14_days * consular_cni_os_ceremony_21_day_requirement * consular_cni_os_denmark * consular_cni_os_foreign_resident_21_days_jordan * consular_cni_os_foreign_resident_3_days_macedonia * consular_cni_os_foreign_resident_ceremony_notary_public * consular_cni_os_foreign_resident_ceremony_notary_public_greece * consular_cni_os_giving_notice_in_ceremony_country * consular_cni_os_local_resident_italy * consular_cni_os_local_resident_table * consular_cni_os_not_uk_resident_ceremony_jordan * consular_cni_os_other_resident_ceremony_italy * consular_cni_os_uk_resident_montenegro * consular_cni_requirements_in_germany * contact_dutch_embassy_for_dutch_caribbean_islands * documents_must_be_originals_when_in_sharia_court * evidence_if_divorced_outside_uk * fee_table_croatia * germany_legalisation_and_translation * getting_statutory_declaration_for_italy_partner_british * getting_statutory_declaration_for_italy_partner_non_british * get_cni_from_uk * issuing_cni_in_italy * italy_consular_cni_os_partner_british * italy_consular_cni_os_partner_not_british * italy_os_consular_cni_ceremony_italy * japan_consular_cni_os_local_resident * japan_intro * kazakhstan_os_local_resident * legalisation_and_translation_check_with_authorities * legalising_italian_statutory_declaration * list_of_consular_fees_italy * list_of_consular_kazakhstan * living_in_ceremony_country_3_days * no_need_to_stay_after_posting_notice * partner_cni_requirements_the_same * pay_by_card_no_amex_no_cheque * pay_by_mastercard_or_visa * pay_in_local_currency_ceremony_in_kazakhstan * required_supporting_documents_greece - only used by this outcome * required_supporting_documents_incl_birth_cert * required_supporting_documents_incl_birth_cert_notary_public * required_supporting_documents_notary_public * russia_os_local_resident * same_cni_process_and_fees_for_partner * statutory_declaration_for_non_british_partner * tunisia_legalisation_and_translation * wait_300_days_before_remarrying * what_to_do_croatia == Deleted phrases that are already in partials and are no longer used: * consular_cni_os_not_uk_resident_ceremony_not_germany * you_may_be_asked_for_cni
This ensures we continue to generate the same govspeak when the `ceremony_country` is "cote-d-ivoire". If we don't mark the resulting string ("Cote d'Ivoire") as HTML safe then the outcomes contain "Cote d'Ivoire" instead of "Cote d'Ivoire". The "'" is then further escaped which means that we end up seeing that on the page instead of the single quote.
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
== Inlined the following outcome-specific phrases: * fot_os_rules_similar_to_france
== Updated the marriage-abroad integration test: * Removed checks for `:affirmation_os_outcome` now that it's been removed from the `outcome_os_affirmation` outcome. * Added `assert_current_node :outcome_os_affirmation` in a test whose name suggested the line should exist. * Removed the "when appointment links for opposite sex marriage exist" test. It would appear that the countries in `OS_COUNTRIES_WITH_APPOINTMENTS` end up at either the `:outcome_os_consular_cni` or `:outcome_os_affirmation` outcomes and both of those have now been removed. * Removed `OS_COUNTRIES_WITH_APPOINTMENTS` now that the test using it had been removed. NOTE. Removing this meant that I had to add 'democratic-republic-of-cong', 'kyrgyzstan', and 'macao' to the `@location_slugs` instance variable to ensure that a number of the tests continued to work. == Added the following partials: * appointment_for_affidavit * appointment_for_affidavit_norway * complete_affirmation_or_affidavit_forms * contact_laadoul (only used in this outcome) * documents_guidance_belgium * download_affidavit_and_affirmation_belgium * download_and_fill_but_not_sign * partner_needs_affirmation * partner_probably_needs_affirmation * pay_by_cash_or_us_dollars_only * pay_by_visas_or_mastercard == Inlined the following outcome-specific phrases: * affirmation_os_all_fees_45_70 * affirmation_os_download_affidavit_philippines * affirmation_os_legalised * affirmation_os_legalised_in_turkey * affirmation_os_uae * appointment_for_affidavit_in_hong_kong * appointment_for_affidavit_notary * book_online_china_affirmation_affidavit * book_online_china_local_prelude * book_online_china_non_local_prelude * check_legalised_document * complete_affidavit * contact_for_affidavit * documents_for_divorced_or_widowed_cambodia * documents_for_divorced_or_widowed_ecuador * documents_for_divorced_or_widowed_philippines * download_affidavit * download_affidavit_and_affirmation_macao * fee_table_55_55 * fee_table_55_70 * fee_table_affidavit_55 * fee_table_affirmation_65 * make_an_appointment * morocco_affidavit_length * one_must_be_a_resident * partner_declaration * partner_equivalent_document * partner_needs_egyptian_id * partner_probably_needs_affirmation_or_affidavit * what_you_need_to_do_affirmation_21_days == Deleted phrases that are already in partials and are no longer used: * change_of_name_evidence * contact_embassy_of_ceremony_country_in_uk_marriage * divorced_or_widowed_evidences * docs_decree_and_death_certificate * documents_for_divorced_or_widowed * documents_for_divorced_or_widowed_china_colombia * fee_and_required_supporting_documents_for_appointment * fee_table_45_70_55 * gulf_states_os_consular_cni * gulf_states_os_consular_cni_local_resident * pay_in_euros_or_visa_electron * required_supporting_documents_egypt * required_supporting_documents_philippines
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
== Updated the marriage-abroad integration test: * Removed checks for `:france_pacs_law_cp_outcome` PhraseList now that it's been removed from the `outcome_cp_france_pacs` outcome. == Inlined the following outcome-specific phrases: * fot_cp_all * france_pacs_law_cp_outcome
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
== Notes: * This updates the outcomes such that Burundi Civil Partnerships no longer contain "synonyms_of_cp_in_burundi" (e.g. https://www.gov.uk/marriage-abroad/y/burundi/uk/partner_local/same_sex). == Updated the marriage-abroad integration test: * Removed checks for `:no_cni_required_cp_outcome` PhraseList now that it's been removed from the `outcome_cp_no_cni` outcome. == Inlined the following outcome-specific phrases: * contact_dutch_embassy_in_uk_cp * contact_embassy_or_consulate_representing_ceremony_country_in_uk_cp * no_consular_facilities_to_register_ss == Deleted phrases that are already in partials and are no longer used: * country_is_dutch_caribbean_island
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text. NOTE. We're no longer falling back to 'synonyms_of_cp_in_NNN'. This means that 'synonyms_of_cp_in_burundi' no longer appears in some of these rendered outcomes.
== Updated the marriage-abroad integration test: * Removed checks for `:consular_cp_outcome` PhraseList now that it's been removed from the `outcome_cp_consular` outcome. == Added the following partials: * contact_to_make_appointment == Inlined the following outcome-specific phrases: * additional_non_british_partner_documents_cp * cant_register_cp_with_country_national * consular_cp_standard_fees * consular_cp_what_you_need_to_do * cp_may_be_possible * documents_for_both_partners_cp * documents_needed_7_days_residency
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
This was a simple conversion and it hasn't resulted in any failing regression tests.
== Notes: * There's no fees table for the case where `marriage_and_partnership_phrases` is 'ss_marriage_and_partnership' and `MarriageAbroadDataQuery#ss_alt_fees_table_country?` returns `true`. This was already the case in master but it's not clear whether it's intentional or not. Previously, the `ss_fees_table` precalculate block could return 'ss_marriage_and_partnership_alt', which was prefixed by 'fees_table_' in the `ss_ceremony_body` precalculate block. There's no corresponding PhraseList for this key. == Updated the marriage-abroad integration test: * Removed checks for `:ss_ceremony_body` and `:ss_title` PhraseLists now that it's been removed from the `outcome_ss_marriage` outcome. * Updated the "when appointment links for same sex marriage exist" test so that it doesn't check for the `appointment_links.same_sex.#{country}` PhraseLists in this `outcome_ss_marriage` outcome. == Added the following partials: * documents_needed_21_days_residency * documents_needed_ss_british * no_objection_in_14_days_ss_marriage_and_partnership * provide_two_witnesses_ss_marriage_and_partnership == Inlined the following outcome-specific phrases: * able_to_ss_marriage * able_to_ss_marriage_and_partnership * australia_ss_relationships * contact_british_embassy_or_consulate_berlin * contact_embassy_or_consulate * documents_needed_ss_not_british * documents_needed_ss_not_british_germany_same_sex * fees_table_ss_marriage * fees_table_ss_marriage_alt * provide_two_witnesses_ss_marriage * ss_marriage_footnote * what_to_do_ss_marriage * what_to_do_ss_marriage_and_partnership * will_display_in_14_days == Deleted phrases that are already in partials and are no longer used: * pay_by_cash_or_us_dollars_only
Relying on the `contact_method_key` meant that I'd have to retain the duplication between the various 'appointment_links' Phrases in marriage-abroad.yml and the '_contact_method.govspeak.erb' partial. Checking that `contact_method_key == :embassies_data` is equivalent to the `case` statement in '_contact_method.govspeak.erb' falling through to the `else` clause. So, instead of relying on a value of `contact_method_key` I can pass the text we want displayed to the '_contact_method.govspeak.erb' partial and have that decide whether to display it. This change doesn't break any of the regression tests for the outcome_ss_marriage outcome.
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
This was a trivial conversion. None of the rendered outcomes have changed.
== Updated the marriage-abroad integration test: * Removed checks for `:ss_body` PhraseList now that it's been removed from the `outcome_ss_marriage_malta` outcome. == Inlined the following outcome-specific phrases:: * able_to_ss_marriage_and_partnership_hc * ss_marriage_footnote_hc * what_to_do_ss_marriage_and_partnership_hc * will_display_in_14_days_hc == Deleted phrases that are already in partials and are no longer used: * contact_to_make_appointment * convert_cc_to_ss_marriage * documents_needed_21_days_residency * documents_needed_ss_british * fees_table_ss_marriage_and_partnership * no_objection_in_14_days_ss_marriage_and_partnership * provide_two_witnesses_ss_marriage_and_partnership
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
== Updated the marriage-abroad integration test: * Removed checks for `:body` PhraseList now that it's been removed from the `outcome_ss_affirmation` outcome. * Removed the "when appointment links for same sex marriage exist" test. The only two outcomes that this appears to relate to are `:outcome_ss_marriage` and `:outcome_ss_affirmation`, both of which have now been converted to ERB. * Removed `SS_COUNTRIES_WITH_APPOINTMENTS` now that the test using it has been removed. NOTE. Removing this meant that I had to add 'montenegro', and 'norway' to the `@location_slugs` instance variable to ensure that a number of the tests continued to work. == Inlined the following outcome-specific phrases: * divorce_proof_cp == Deleted phrases that are already in partials and are no longer used: * affirmation_os_translation_in_local_language_text * appointment_for_affidavit * appointment_for_affidavit_norway * callout_partner_equivalent_document * complete_affirmation_or_affidavit_forms * contact_embassy_of_ceremony_country_in_uk_cp * documents_guidance_belgium * download_affidavit_and_affirmation_belgium * download_and_fill_but_not_sign * fee_table_affirmation_55 * legalisation_and_translation * partner_needs_affirmation * partner_probably_needs_affirmation * pay_by_cash_or_credit_card_no_cheque * required_supporting_documents * what_you_need_to_do_affirmation
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
This was a trivial conversion. No regression test artefacts have changed.
== Updated the marriage-abroad integration test: * Removed checks for `:commonwealth_countries_cp_outcome` PhraseList now that it's been removed from the `outcome_cp_commonwealth_countries` outcome. == Inlined the following outcome-specific phrases: * contact_high_comission_of_ceremony_country_in_uk_cp * title_civil_partnership == Deleted phrases that are already in partials and are no longer used: * contact_local_authorities_in_country_cp * synonyms_of_cp_in_nnn
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
== Updated the marriage-abroad integration test: * Removed checks for `:body` PhraseList now that it's been removed from the `outcome_spain` outcome. == Inlined the following outcome-specific phrases: * civil_weddings_in_spain * cni_maritial_status_certificate_spain * get_cni_in_spain * get_cni_in_uk_for_spain * get_cni_in_uk_for_spain_title * get_maritial_status_certificate_spain * legal_restrictions_for_non_residents_spain * other_requirements_in_spain * other_requirements_in_spain_for_residents_intro * other_requirements_in_spain_intro * ss_process_in_spain * what_you_need_to_do_spain * what_you_need_to_do_spain_third_country == Deleted phrases that are already in partials and are no longer used: * cni_at_local_register_office * consular_cni_os_fees_incl_null_osta_oath_consular_letter * contact_local_authorities_in_country_marriage * get_legal_advice * get_legal_and_travel_advice * link_to_consular_fees * names_on_documents_must_match * partner_naturalisation_in_uk * pay_by_visas_or_mastercard * what_you_need_to_do
These changes were introduced by the conversion from YAML to ERB in the previous commit. I'm confident that these changes don't affect the HTML that's ultimately generated. The changes are all related to blank lines being added or removed. In each case where a blank line has been removed, we still have at least one blank line between the previous and next blocks of text.
Removed these phrases that don't appear to be used anywhere (including in master): * commonwealth_countries_cp_australia_partner_local * commonwealth_countries_cp_australia_partner_other * fees_spain * list_of_consular_fees_france * no_cp_with_vietnamese_national
The last use of the `contact_method_key` calculate block was removed in the commit titled, "Convert outcome_cp_commonwealth_countries". Removing the `contact_method_key` meant that I was also able to remove: * The `:embassies_data` Phrase as it wasn't used outside of this calculate block. * The `MarriageAbroadDataQuery#appointment_link_key_for` method. This was only used by the `contact_method_key` calculate block. * The `MarriageAbroadDataQuery#phrase_exists?` method. This was only used by the `#appointment_link_key_for` method. * The marriage_abroad_data_query_test.rb. This only contained tests for the `appointment_link_key_for` method. * The 'appointment_links' Phrases. These were only used by the `#appointment_link_key_for` method.
We can DRY up the flow now that all outcomes have been converted to ERB.
Updated using: $ rails r script/generate-checksums-for-smart-answer.rb marriage-abroad
chrisroos
force-pushed
the
convert-marriage-abroad-to-use-erb-templates
branch
from
August 17, 2015 16:27
a41a110
to
e93caf1
Compare
chrisroos
added a commit
that referenced
this pull request
Aug 17, 2015
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.
Apologies for the size of this Pull Request.
There's still more tidying up I can do but I figured it's probably better to try to get this merged as soon as possible. The tidying can happen later.
I'm fairly confident that I've converted everything correctly so I guess I'm really interested in whether anyone can spot any obvious mistake that I've made.