Skip to content

Commit a3c2dd3

Browse files
committed
Fix tests passing with false positives
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
1 parent ff1705b commit a3c2dd3

1 file changed

Lines changed: 12 additions & 12 deletions

File tree

spec/requests/pages/conditions_controller_spec.rb

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@
146146
end
147147

148148
it "redirects to the page list" do
149-
expect(response).to redirect_to show_routes_path(form:, page:)
149+
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: page.id)
150150
end
151151

152152
it "displays success message" do
@@ -159,7 +159,7 @@
159159
let(:group) { create(:group, organisation: standard_user.organisation) }
160160

161161
it "redirects to the new exit page" do
162-
expect(response).to redirect_to new_exit_page_path(form:, page:, answer_value: "Wales")
162+
expect(response).to redirect_to new_exit_page_path(form_id: form.id, page_id: page.id, answer_value: "Wales")
163163
end
164164
end
165165

@@ -196,7 +196,7 @@
196196

197197
it "does not create the condition and redirects the user to the question routes page" do
198198
expect(ConditionRepository).not_to have_received(:create!)
199-
expect(response).to redirect_to show_routes_path(form.id, selected_page.id)
199+
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: selected_page.id)
200200
end
201201
end
202202
end
@@ -276,7 +276,7 @@
276276
end
277277

278278
it "redirects to the page list" do
279-
expect(response).to redirect_to show_routes_path(form:, page:)
279+
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: page.id)
280280
end
281281

282282
it "displays success message" do
@@ -301,7 +301,7 @@
301301
let(:params) { { pages_conditions_input: { routing_page_id: 1, check_page_id: 1, goto_page_id: "create_exit_page", answer_value: "Wales" } } }
302302

303303
it "redirects to the edit exit page" do
304-
expect(response).to redirect_to edit_exit_page_path(form:, page:, condition:)
304+
expect(response).to redirect_to edit_exit_page_path(form_id: form.id, page_id: page.id, condition_id: condition.id)
305305
end
306306
end
307307

@@ -342,7 +342,7 @@
342342
end
343343

344344
it "redirects to the confirm exit page deletion page" do
345-
expect(response).to redirect_to confirm_change_exit_page_path(form.id, selected_page.id, condition.id, params: { answer_value: "Wales", goto_page_id: 3 })
345+
expect(response).to redirect_to confirm_change_exit_page_path(form_id: form.id, page_id: selected_page.id, condition_id: condition.id, params: { answer_value: "Wales", goto_page_id: 3 })
346346
end
347347

348348
it "does not call save! for the condition" do
@@ -354,7 +354,7 @@
354354
let(:params) { { pages_conditions_input: { routing_page_id: 1, check_page_id: 1, goto_page_id: "exit_page", answer_value: "Wales" } } }
355355

356356
it "redirects to the edit exit page" do
357-
expect(response).to redirect_to show_routes_path(form:, page:, condition:)
357+
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: page.id)
358358
end
359359
end
360360
end
@@ -418,7 +418,7 @@
418418
end
419419

420420
it "redirects to the page list" do
421-
expect(response).to redirect_to form_pages_path(form.id, selected_page.id)
421+
expect(response).to redirect_to form_pages_path(form_id: form.id, page_id: selected_page.id)
422422
end
423423

424424
it "displays success message" do
@@ -430,7 +430,7 @@
430430
let(:confirm) { "no" }
431431

432432
it "redirects to edit the condition" do
433-
expect(response).to redirect_to edit_condition_path(form.id, selected_page.id, condition.id)
433+
expect(response).to redirect_to edit_condition_path(form_id: form.id, page_id: selected_page.id, condition_id: condition.id)
434434
end
435435
end
436436

@@ -548,14 +548,14 @@
548548
end
549549

550550
it "redirects to the question routes page" do
551-
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: selected_page.id)
551+
expect(response).to redirect_to show_routes_path(form_id: form.id, page_id: page.id)
552552
end
553553

554554
context "when confirm is not 'yes'" do
555555
let(:confirm) { "no" }
556556

557557
it "redirects to the edit condition page" do
558-
expect(response).to redirect_to edit_condition_path(form.id, selected_page.id, condition.id)
558+
expect(response).to redirect_to edit_condition_path(form_id: form.id, page_id: selected_page.id, condition_id: condition.id)
559559
end
560560
end
561561

@@ -587,7 +587,7 @@
587587
let(:condition) { build :condition, id: 1, check_page_id: selected_page.id, goto_page_id: 3 }
588588

589589
it "redirects to the form pages path" do
590-
expect(response).to redirect_to form_pages_path(form.id)
590+
expect(response).to redirect_to form_pages_path(form_id: form.id)
591591
end
592592
end
593593

0 commit comments

Comments
 (0)