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

Fix no method error undefined method transition on outcome #2494

Merged

Conversation

Projects
None yet
2 participants
@floehopper
Copy link
Contributor

commented Apr 29, 2016

Description

This reverts commit 0929036.

It was a mistake to remove the Outcome#transition method which raised a SmartAnswer::InvalidNode exception. I hadn't noticed that this exception is rescued and the app responds with a 404 Not Found. The latter seems like sensible behaviour and so I'm restoring the previous implementation in this PR. This should prevent exceptions like this one from appearing in Errbit.

This PR also adds test coverage to hopefully avoid the same mistake happening again.

External changes

Adding extra responses to the path of an outcome page should result in a 404 No Found error instead of a 5XX error and no exception should be recorded in Errbit.

Before

https://www.gov.uk/overseas-passports/y/australia/renewing_new/adult/born-in-uk-pre-1983 -> "Sorry, we are experiencing technical difficulties"`

After

https://www.gov.uk/overseas-passports/y/australia/renewing_new/adult/born-in-uk-pre-1983 -> "Page not found"

floehopper added some commits Apr 29, 2016

Revert "Remove Outcome#transition method"
This reverts commit 0929036.

It was a mistake to remove the `Outcome#transition` method which raised a
`SmartAnswer::InvalidNode` exception. I hadn't noticed that this exception is
rescued and the app responds with a "404 Not Found" [1]. The latter seems like
sensible behaviour and so I'm restoring the previous implementation in this
commit. This should prevent exceptions like this one [2] from appearing in
Errbit.

[1]: https://github.com/alphagov/smart-answers/blob/bf77a584737d8dbfee5694b64d5ad8870ebe0445/app/controllers/smart_answers_controller.rb#L7
[2]: https://errbit.publishing.service.gov.uk/apps/533c35ae0da1159384044f5f/problems/572087f96578631e0c660b00
Add test coverage for raising/rescuing of InvalidNode
I hope this might help avoid a repeat of #2474 in which the implementation of
`Outcome#transition` was accidentally removed [1].

I've also added test coverage for the rescuing of `FlowRegistry::NotFound` while
I was at it.

[1]: 0929036
@pmanrubia

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

Good catch @floehopper; LGTM

@pmanrubia pmanrubia merged commit 23479f0 into master Apr 29, 2016

@pmanrubia pmanrubia deleted the fix-no-method-error-undefined-method-transition-on-outcome branch Apr 29, 2016

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

@pmanrubia: Thanks. Sorry for making the mistake in the first place!

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

For some reason it doesn't look as if a CI build ran against this build. And it looks as if the CI build of master has just failed. I'm running the build locally to see whether I can see what's going on.

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

Oh - it's a Rubocop violation. I'll submit a PR to fix it.

floehopper added a commit that referenced this pull request Apr 29, 2016

Fix Lint/UselessAssignment Rubocop violation
This was accidentally introduced in #2494.
@pmanrubia

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

Thank you for the follow-up @floehopper, and for adding the missing tests covering Outcome#transition 👍

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

See #2495

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.