Skip to content

Check the draft question's answer type is correct for answer type specific routes#2635

Merged
stephencdaly merged 6 commits into
mainfrom
fix-no-method-error-when-draft-question-is-wrong-type
Mar 18, 2026
Merged

Check the draft question's answer type is correct for answer type specific routes#2635
stephencdaly merged 6 commits into
mainfrom
fix-no-method-error-when-draft-question-is-wrong-type

Conversation

@stephencdaly

@stephencdaly stephencdaly commented Mar 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What problem does this pull request solve?

Trello card: https://trello.com/c/QyQTTwCc

For routes for adding or editing a question that expect to be editing a question of a specific answer type, add a before_action that checks the answer type of the draft question matches the expected answer type. If the answer type does not match:

  • If adding a new question, redirect to the first settings page for that answer type. If no DraftQuestion exists, redirect to the select answer type page.
  • If editing an existing question, redirect to the edit question page.

Doing this avoids raising a NoMethodError when a user attempts to visit a route for an answer type that does not match the DraftQuestion that exists, and prevents any other unexpected errors that may occur.

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Do the end to end tests need updating before these changes will pass?
  • Has all relevant documentation been updated?

@stephencdaly stephencdaly changed the title Fix no method error when draft question is wrong type Check the draft question's answer type is correct for answer type specific routes Mar 16, 2026
@stephencdaly stephencdaly marked this pull request as ready for review March 16, 2026 17:27
@stephencdaly stephencdaly force-pushed the fix-no-method-error-when-draft-question-is-wrong-type branch 2 times, most recently from 99ffeb1 to 2c91700 Compare March 17, 2026 09:58
@stephencdaly stephencdaly marked this pull request as draft March 17, 2026 10:22
@stephencdaly stephencdaly force-pushed the fix-no-method-error-when-draft-question-is-wrong-type branch from 2c91700 to f781337 Compare March 17, 2026 11:03
@stephencdaly stephencdaly marked this pull request as ready for review March 17, 2026 11:56
lfdebrux
lfdebrux reviewed Mar 17, 2026
View reviewed changes
Comment thread app/services/pages/add_or_edit_question_service.rb Outdated
@stephencdaly
Refactor method to get start page for answer type to a service
bf40903 
Add a service with a method to get the start page for adding/editing
a question of a given answer type.

This is so that we can re-use this code to redirect the user to the
correct start page if they try to visit a route for editing a question
that does not match the answer type of the draft question.

Determine whether to return the new or edit route by looking at the
presence of a page_id request param.
@stephencdaly
Add before_action to check answer type for address settings routes
0d5dd04 
Add a before_action that checks the draft question has the expected
answer type for the address settings routes. If it doesn't redirect
the user to either the start page for adding a new question, or the
edit question page for an existing question.

Add a private method to PagesController to do the check so this can
be reused for other answer-type specific routes.
@stephencdaly
Add before_action to check answer type for date settings routes
a22163a 
Add a before_action that checks the draft question has the expected
answer type for the date settings routes. If it doesn't redirect
the user to either the start page for adding a new question, or the
edit question page for an existing question.
@stephencdaly
Add before_action to check answer type for name settings routes
fe64597 
Add a before_action that checks the draft question has the expected
answer type for the name settings routes. If it doesn't redirect
the user to either the start page for adding a new question, or the
edit question page for an existing question.
@stephencdaly
Add before_action to check answer type for text settings routes
9307394 
Add a before_action that checks the draft question has the expected
answer type for the text question settings routes. If it doesn't
redirect the user to either the start page for adding a new question,
or the edit question page for an existing question.
@stephencdaly
Add before_action to check answer type for selection settings routes
6401fe9 
Add a before_action that checks the draft question has the expected
answer type for the selection question settings routes. If it doesn't
redirect the user to either the start page for adding a new question,
or the edit question page for an existing question.
@stephencdaly stephencdaly force-pushed the fix-no-method-error-when-draft-question-is-wrong-type branch from f781337 to 6401fe9 Compare March 17, 2026 12:07
@sonarqubecloud

sonarqubecloud Bot commented Mar 17, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@github-actions

github-actions Bot commented Mar 17, 2026

Copy link
Copy Markdown

🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2635.admin.review.forms.service.gov.uk/

It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready
after 5 minutes, there may be something wrong with the ECS task. You will need to go to the integration AWS account
to debug, or otherwise ask an infrastructure person.

For the sign in details and more information, see the review apps wiki page.

@lfdebrux

lfdebrux commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Looks good to me, one small thought...

I wonder if it's worth deleting the DraftQuestion at the point where we've determined they're at the wrong URL... there's an interesting behaviour I just found while testing where if you start creating a selection question, then change to creating a name question (in another tab), you'll end up with the name you chose for your selection question in the new name question page:

Kapture.2026-03-17.at.15.09.14.mp4

To be fair, you'd hope that they'd notice the question text was wrong at some point before making the form live, so it's not a big issue!

@stephencdaly

stephencdaly commented Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Looks good to me, one small thought...

I wonder if it's worth deleting the DraftQuestion at the point where we've determined they're at the wrong URL... there's an interesting behaviour I just found while testing where if you start creating a selection question, then change to creating a name question (in another tab), you'll end up with the name you chose for your selection question in the new name question page:

Kapture.2026-03-17.at.15.09.14.mp4
To be fair, you'd hope that they'd notice the question text was wrong at some point before making the form live, so it's not a big issue!

Interesting point - I don't think that's different to the existing behaviour, so maybe a slightly different issue to the one that's being fixed here? I think a better fix for that issue would be to delete the DraftQuestion when the answer type page is submitted if the answer type has changed - but I think that's slightly unrelated to this issue so is it okay not to do that as part of this?

@lfdebrux

lfdebrux commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Interesting point - I don't think that's different to the existing behaviour, so maybe a slightly different issue to the one that's being fixed here? I think a better fix for that issue would be to delete the DraftQuestion when the answer type page is submitted if the answer type has changed - but I think that's slightly unrelated to this issue so is it okay not to do that as part of this?

Ah okay, I didn't realise that it was existing behaviour, I was trying to do something that would result in a NoMethodError, but you're right, it is possible currently anyway 🤦

@stephencdaly

stephencdaly commented Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

@lfdebrux is this good to be approved now?

lfdebrux
lfdebrux approved these changes Mar 18, 2026
View reviewed changes

@lfdebrux lfdebrux left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephencdaly woops, sorry, I thought I had, turns out I had written something then forgot to hit the button!

Code changes look good to me, and I've tested they fix the problem described in the ticket.

@stephencdaly stephencdaly added this pull request to the merge queue Mar 18, 2026
Merged via the queue into main with commit 433fe6a Mar 18, 2026
6 checks passed
@stephencdaly stephencdaly deleted the fix-no-method-error-when-draft-question-is-wrong-type branch March 18, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lfdebrux lfdebrux lfdebrux approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stephencdaly @lfdebrux