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.

Sign up for GitHub

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

Jump to bottom

Replace appointment booking link (Marriage abroad SUDAN) #2325

Merged
merged 3 commits into from
Mar 21, 2016
Merged

Replace appointment booking link (Marriage abroad SUDAN) #2325

merged 3 commits into from
Mar 21, 2016

Conversation

ikennaokpala
Copy link
Contributor

@ikennaokpala ikennaokpala changed the title Replace the appointment booking (Marriage abroad SUDAN) Replace appointment booking link (Marriage abroad SUDAN) Mar 2, 2016
@ikennaokpala ikennaokpala force-pushed the marriage-abroad-sudan branch 3 times, most recently from 164bb08 to 1d863fd Compare March 4, 2016 16:25
@ikennaokpala ikennaokpala force-pushed the marriage-abroad-sudan branch 2 times, most recently from aea9ba7 to c47e3fa Compare March 21, 2016 09:43
Change appointment link to address (SUDAN)
7bda232 
This commit replaces the appointment booking link for SUDAN with the address
of the British embassy in Sudan.This also adds telephone, email and other details
for contacting the British Embassy in Khartoum, Sudan. This is a content change
requested by the content team.
@ikennaokpala ikennaokpala force-pushed the marriage-abroad-sudan branch from c47e3fa to 7bda232 Compare March 21, 2016 09:57
@pmanrubia pmanrubia self-assigned this Mar 21, 2016
@pmanrubia
Copy link
Contributor

If we have fully migrated to Trello, it might be useful to add a link to the Trello card, so we have both links (PT and Trello)?

@ikennaokpala
Copy link
Contributor Author

@pmanrubia I have added the trello link for this PR

@pmanrubia
Copy link
Contributor

@ikennaokpala Any reason why there are no updated artefacts?

@ikennaokpala
Copy link
Contributor Author

@pmanrubia that is becuase Sudan is not included in this test data file.. test/data/marriage-abroad-questions-and-responses.yml

@pmanrubia
Copy link
Contributor

@ikennaokpala I guess I might be missing something.
I will hand over the PR to @erikse as I would still expect to see the artefact update as the outcome is being modified. My understanding is that the questions-and-responses file would need to be amended manually in case Sudan is not there.

@pmanrubia pmanrubia assigned erkde and unassigned pmanrubia Mar 21, 2016
@erkde
Copy link
Contributor

erkde commented Mar 21, 2016

@pmanrubia please see the following #2200 (comment) for discussion on this, at the moment, it's kind of undefined when we add a country, and looking at the responses file, there are only 104 entries covering about half of the countries available in question one

Ikenna Okpala added 2 commits March 21, 2016 15:41
Replace absolute URL with relative URL
dd6143f 
This commit changes the gov.uk absolute url with this relative url.
This change is made in line with the new efforts to use relative urls.
Regenerate checksum data (Marriage abroad)
c10faa7
@ikennaokpala ikennaokpala force-pushed the marriage-abroad-sudan branch from 09f59d3 to c10faa7 Compare March 21, 2016 15:50
@floehopper
Copy link
Contributor

@pmanrubia: When @chrisroos & I originally added the regression tests, we added responses to the questions-and-responses file until we had the minimum set that exercised all the paths through the code using a coverage tool (see adding-new-regression-tests docs).

Since we converted from using phrase lists and YAML files (don't ask!) to using ERB templates, I think using a coverage tool might be hard, because I don't think they support ERB templates.

If new "logic paths" have been added then in the short term it would probably be sensible to make the relevant additions to questions-and-responses. However, I'd be wary of adding them just to see content changes.

In the long run we want to replace the regression tests with unit tests and integration tests. part-year-profit-tax-credits was a new flow that was written with no regression tests. However, that flow isn't particularly content-heavy and it's not entirely clear to me what we'd ideally want to do with a content-heavy flow like marriage-abroad. However, I doubt that having brittle tests around the exact content are going to be the answer.

Sorry - that was a bit of a ramble, but hopefully some of it helps explain things.

@pmanrubia pmanrubia assigned pmanrubia and unassigned erkde Mar 21, 2016
@pmanrubia pmanrubia added the LGTM label Mar 21, 2016
@pmanrubia
Copy link
Contributor

Sure it does. Thank you @ikennaokpala, @erikse and @floehopper for the clarifications.

LGTM @ikennaokpala

ikennaokpala added a commit that referenced this pull request Mar 21, 2016
@ikennaokpala
Merge pull request #2325 from alphagov/marriage-abroad-sudan
9d539fb 
Replace appointment booking link (Marriage abroad SUDAN)
@ikennaokpala ikennaokpala merged commit 9d539fb into master Mar 21, 2016
@ikennaokpala ikennaokpala deleted the marriage-abroad-sudan branch March 21, 2016 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmanrubia pmanrubia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ikennaokpala @pmanrubia @erkde @floehopper