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

Add step to apply for an overseas passport for child in Afghanistan #2027

Merged
merged 1 commit into from Oct 22, 2015

Conversation

erkde
Copy link
Contributor

@erkde erkde commented Oct 20, 2015

For further details see:
https://www.pivotaltracker.com/n/projects/1270592/stories/105008654

Expected changes

  • Example URL
    • Under How to Apply, a step # 5 with a link to a parent contact details form appears

screen shot 2015-10-20 at 4 11 26 pm

@chrisroos
Copy link
Contributor

Do you need to update the regression test artefacts for this change too, @erik-eide?

@erkde erkde force-pushed the add-step-for-child-passport-apply-in-afghanistan branch from 82189e3 to b8648a7 Compare October 20, 2015 16:14
@erkde
Copy link
Contributor Author

erkde commented Oct 20, 2015

Yep, I just added a new commit with a response that regresses through the new path

@chrisroos chrisroos self-assigned this Oct 21, 2015
@chrisroos
Copy link
Contributor

I hope you don't mind, @erik-eide, but I've had a go at illustrating what I meant when I talked about updating the regression tests to see the effect of this change.

See PR #2028 for a worked example of how we can temporarily update the regression tests to test the effect of a change. It turns out to be overkill in this instance but the technique can be useful when we're making changes that can affect multiple outcomes; particularly when a change inadvertently affects an outcome that we don't expect it to.

If I was going to suggest a small change to this PR then I think it'd be good to squash the two commits. I don't think there's any real value in adding "afghanistan" to the questions-and-responses file in the first commit but then only generating the regression tests in the second.

All that aside, the actual change in this commit looks good and I apologise that the regression tests have made such a small change feel so painful!

@erkde erkde force-pushed the add-step-for-child-passport-apply-in-afghanistan branch from 673c0a5 to 65ad7a9 Compare October 21, 2015 13:25
@erkde
Copy link
Contributor Author

erkde commented Oct 21, 2015

I think normally, I would of liked of to have squashed the commits for the reason you had given, but given we had a comment on the PR about them not being updated, I left them as 2 intentionally to show in the history.

Have squashed them now, thanks for the examples and docs on getting it right!

@floehopper
Copy link
Contributor

Looks good to me 👍

@erkde erkde force-pushed the add-step-for-child-passport-apply-in-afghanistan branch from 65ad7a9 to e31f4d0 Compare October 22, 2015 08:19
erkde added a commit that referenced this pull request Oct 22, 2015
…ply-in-afghanistan

Add step to apply for an overseas passport for child in Afghanistan
@erkde erkde merged commit 1b0dea4 into master Oct 22, 2015
@erkde erkde deleted the add-step-for-child-passport-apply-in-afghanistan branch October 22, 2015 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants