Unify marriage abroad fees content #2355
Conversation
erkde
force-pushed
the
unify_marriage_abroad_fees_content
branch
from
5f7c365 to
14153b8
Compare
|
With #2350 now merged, this is ready for review |
|
Very minor: In the note for commit 0b12c25 ("Regenerate test artefacts for marriage abroad") you've mentioned two expected space changes but I noticed a third: -Performing a marriage ceremony| £140
+Performing a marriage ceremony | £140It might be worth updating the commit note to explain that this is also intentional. |
|
This all looks great to me, @erikse. Hopefully this'll help avoid the fees getting out of sync in future. The only minor comment I have is about the commit titles. I realise this is possibly/probably subjective but I would've tried to avoid some of the duplication you've ended up with. Having been through the commits, I might've used titles like: 1192199 Add consular service fees table to marriage abroad |
|
To keep things at 50 characters or less, I've been writing pretty generic commit titles, usually mentioning the smart answer effected by the change (if it is specific). But those titles look a bit more useful, are we all okay to start going over the 50 character restriction @floehopper @ikennaokpala @pmanrubia ? |
|
@erikse If you all agree, I am good with going over 50 characters as long as the commit messages are not truncated in Github (I guess the limit is 72 characters in GH?) |
|
It seems like the GDS Styleguide for commit messages still restricts us to 50 characters:
Maybe this could be relaxed to 69 characters, github seems okay with showing titles up to that length with truncating them and showing the elipses |
|
I've always treated the 50 character limit as a guideline, not a hard-and-fast rule i.e. I try really hard to come up with a 50-char summary line, but if I can't then I don't mind going slightly over. For me the key thing is knowing the reason behind the limit i.e. that some git clients will not display more than 50 chars of the summary line. If I go over the limit, I try to keep the most important part at the beginning. My preference would be to keep the 50-character limit, but accept that it's a guideline. I would prefer this to just increasing the limit in order to have it as a hard-and-fast rule. Especially if the new limit is just based on GitHub's current UI. |
|
I'm finding when we have longer variable or file names, it's almost impossible to write a commit message that shows the precise context, so that's when I've been falling back tagging them with a wider scope of identifying the smart answer the change belongs to. What tool(s) in particular restrict you to 50 characters? Maybe it's worth considering if there are alternatives/newer versions that would allow a longer title. 50 characters, given the practice of having long class/method/file names in Ruby, doesn't leave much room to add much else. |
|
In refactorings, for example, the smaller the commits, the more difficult it is to write 50 characters messages without commit duplications across the PR, so I like the idea of relaxing the constraint of 50 characters but trying to stick to it. A balanced solution for me. |
This commit introduces a partial for rendering consular services and their corresponding fees, as shown to users in many of the marriage abroad outcomes. An outcome can use this partial by passing an array of services to show the user in the call to render the partial, and the partial in turn will render each service in the order it was received. As the first commit of this PR, the fees from partial `_consular_cni_os_fees_incl_null_osta_oath_consular_letter` are added to a `consular_fees_table_items`, and is removed. All calls to render the removed partial are replaced with calls to render the new fees table.
Move content from _fee_table_affirmation_55 into the new _consular_fees_table_items partial so it can be removed.
Move the Affidavit for marriage content to partial _consular_fees_table_items so we can delete partial _fee_table_affidavit_55
Move the administering_oath_or_making_declaration content from partial _fee_table_oath_55 partial to _consular_fees_table_items and remove _fee_table_oath_55.
All content contained in partial _fee_table_45_70_55 now exists in _consular_fees_table_items, so it can be removed and replaced with a call to render _consular_fees_table.
Replace duplicate content in _fees with calls to render partial _consular_fees_table.
Replace content in _fees by moving it to the _consular_fees_table_items and render _consular_fees_table in its place.
Replace duplicated content for “Affirmation for freedom to marry in English”, and “Affirmation for freedom to marry in any other language” by moving content from _fees to _consular_fees_table_items.
Remove partial containing consular fees that are now available in _consular_fees_table_items.
As the code in elsif branch is identical to the if branch, we can remove it.
Remove content from _fees partial and replace with calls to render it via _consular_fees_table.
Removes duplicate content for Egypt outcome in the _fees partial, and replaces with call to _consular_fees_table partial.
Removes duplicate content for Peru outcomes in _fees partial, and replace with call to render it via _consular_fees_table.
Move fee content from same sex outcome to _consular_fees_table_items partial and replace duplicate content in same sex outcome with call render _consular_fees_table.
Replace consular fee content in same sex outcome outcome_ss_marriage by moving it to the _consular_fees_table_items partial.
Adds performing a marriage ceremony and posting marriage notice to partial _consular_fees_table_items and remove duplicate content in same sex outcome outcome_ss_marriage.
Replace fee content in outcome outcome_cp_consular by moving "Issuing a civil partnerships certificate" and "Registering a civil partnership" to _consular_fees_table_items rendering that via _consular_fees_table.
Replace content in outcome_cp_or_equivalent by moving fee content to partial _consular_fees_table_items and rendering it via _consular_fees_table.
Add consular fee content for issuing a certificate in custom and law to the `consular_fees_table_items` partial and replace duplicate content in outcome for Croatia.
The artefacts have three white space changes that are fixed as part of this PR, correcting the following artefact outputs: -Receiving a notice of registration | £65 +Receiving a notice of registration | £65 -##Fees +## Fees -Performing a marriage ceremony| £140 +Performing a marriage ceremony | £140
erkde
force-pushed
the
unify_marriage_abroad_fees_content
branch
from
14153b8 to
a34a3f2
Compare
|
I've amended the commit titles to try and give a bit more context to the change, and rebased with master. |
…tent Unify marriage abroad fees content
Pivotal Ticket: https://www.pivotaltracker.com/story/show/109438906
This PR is the second part of this ticket (following on from #2350) and brings together all the fee related content throughout the marriage abroad smart answer into one partial and allows outcomes to selectively enable which consular fees to show the user.