Fix delete all routes journey #1774
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomasiles
approved these changes
Feb 13, 2025
This specs for ConditionRepository#destroy have expectations on the condition ids, but weren't setting ids on the condition objects themselves.
We've found that in production the "delete all routes" journey has stopped working properly; after the user confirms that they want to delete all routes for a question, they see an error page rather than being redirected to the "add and edit questions" page as expected. Debugging locally revealed this is an unintended side-effect of an unrelated change in forms-api, in PR alphagov/forms-api#686, which changes the conditions model to delete secondary skip routes when a skip route is deleted. Because the process to delete all routes deletes the primary condition before the secondary skip, when it comes to delete the secondary skip that no longer exists, and the API responds with a 404. This 404 is then parlayed by ActiveResource into a ResourceNotFound exception, which then bubbles up to the Rails middleware, and is treated as a record not found error, and Rails renders a 404 error page. This commit adds a test that simulates this interaction, so we can make some changes in our system to handle the scenario better.
…ready been deleted `ActiveRecord::Persistence#destroy` does not raise an error if a record has already been deleted, let's change our repository `destroy` methods to behave in the same way.
lfdebrux
force-pushed
the
ldeb-fix-destroy-routes
branch
from
59343d5 to
ec550fd
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this pull request solve?
Trello card: https://trello.com/c/cFZnCzve/2127-bug-delete-all-routes-redirects-to-a-page-not-found-error
Currently when a user tries to delete all routes for a question they are shown a page not found error rather being redirected to the "Add and edit questions" page as expected.
I think this is happening because of a change in forms-api that means deleting a condition can also delete secondary skip conditions in the same request (alphagov/forms-api#686).
This PR adds a test that simulates the behaviour of forms-api currently, and changes the
destroymethods for each repository so that trying to delete a condition that has already been deleted doesn't result in an exception. This behaviour is closer to how ActiveRecord behaves, and fixes the problem so that users won't see the error page any more.Things to consider when reviewing