Fix tests passing with false positive#2096
Merged
Merged
Conversation
lfdebrux
force-pushed
the
fix-specs-path-arguments
branch
2 times, most recently
from
1991f81 to
6c22565
Compare
SamJamCul
previously approved these changes
Jul 25, 2025
lfdebrux
force-pushed
the
fix-specs-path-arguments
branch
2 times, most recently
from
c24aad3 to
a3c2dd3
Compare
The arguments to a Rails path helper should be either an object such as a record that implements ActiveModel::Naming ActiveModel::Conversions, or a keyword argument [[1]]; if an object `o` is passed as an argument then `o.model_name.route_key` will be used to work out what parameter in the path `o.to_param` should be used for, and if a keyword argument is given then the name of that keyword will be used for the path parameter. If an object is given as a keyword argument, the model name is not used for the path parameter; only the keyword argument name is used. These tests have a subtle bug, where the record objects were mistakenly passed as keyword arguments to path helpers in the spec expectations. The tests passed because the value of `#to_param` was `nil`, because the records were not persisted (see the definition of ActiveModel::Conversion#to_param [[2]]), and thus were ignored by the path helper, but the correct path parameters were accessible from the request. If the records had been persisted then a URL parameter named after the keyword would have been appended as a query parameter; not the desired behaviour. This commit changes these tests so that they all use keyword arguments for path helpers; they are more explicit and allow the tests to pass for the right reason. Note that there is one test failing after this change; it is failing correctly now, previously the bug was causing it to pass incorrectly. [1]: https://guides.rubyonrails.org/routing.html#creating-paths-and-urls-from-objects [2]: https://api.rubyonrails.org/classes/ActiveModel/Conversion.html#method-i-to_param
The path helper for the redirect had the ID of the deleted page passed to it, but did not have a slot for a page ID in the path pattern, so the URL ended up with a random number appended. There was a test, but it was passing incorrectly (see the previous commit), so this was previously missed. It's fixed now.
lfdebrux
force-pushed
the
fix-specs-path-arguments
branch
from
a3c2dd3 to
4195da5
Compare
|
SamJamCul
approved these changes
Jul 25, 2025
|
🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2096.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 For the sign in details and more information, see the review apps wiki page. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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/6hA4UVml/2386-change-repositories-in-forms-admin-to-return-activerecord-models-instead-of-activeresource
The arguments to a Rails path helper should be either an object such as a record that implements ActiveModel::Naming ActiveModel::Conversions, or a keyword argument [1]; if an object
ois passed as an argument theno.model_name.route_keywill be used to work out what parameter in the patho.to_paramshould be used for, and if a keyword argument is given then the name of that keyword will be used for the path parameter.If an object is given as a keyword argument, the model name is not used for the path parameter; only the keyword argument name is used.
These tests have a subtle bug, where the record objects were mistakenly passed as keyword arguments to path helpers in the spec expectations. The tests passed because the value of
#to_paramwasnil, because the records were not persisted (see the definition of ActiveModel::Conversion#to_param [2]), and thus were ignored by the path helper, but the correct path parameters were accessible from the request. If the records had been persisted then a URL parameter named after the keyword would have been appended as a query parameter; not the desired behaviour.This commit fixes these tests so they pass for the right reason.
Things to consider when reviewing