From ba7ca11cd8a13ee45f93319e936d80c9fb5aa240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 20:32:02 +0200 Subject: [PATCH 1/8] Fix buggy parallel assignment In ruby, when we assign two variables to one value, the second variable is set to `nil`. --- app/controllers/legislation/processes_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index c0ac5648e..51b353395 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -122,7 +122,9 @@ class Legislation::ProcessesController < Legislation::BaseController rescue 0 end - session[:random_seed], params[:random_seed] = seed + + session[:random_seed] = seed + params[:random_seed] = seed seed = (-1..1).cover?(seed) ? seed : 1 ::Legislation::Proposal.connection.execute "select setseed(#{seed})" end From 6f62d76c71c1be602f5ea0b627f8097487c36242 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 20:44:08 +0200 Subject: [PATCH 2/8] Simplify random seed conversion to float The method `to_f` already returns `0.0` instead of raising an exception when handling non-numeric values. --- app/controllers/legislation/processes_controller.rb | 7 +------ spec/features/legislation/proposals_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index 51b353395..359527b43 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -117,12 +117,7 @@ class Legislation::ProcessesController < Legislation::BaseController end def set_random_seed - seed = begin - Float(params[:random_seed] || session[:random_seed] || (rand(99) / 100.0)) - rescue - 0 - end - + seed = (params[:random_seed] || session[:random_seed] || (rand(99) / 100.0)).to_f session[:random_seed] = seed params[:random_seed] = seed seed = (-1..1).cover?(seed) ? seed : 1 diff --git a/spec/features/legislation/proposals_spec.rb b/spec/features/legislation/proposals_spec.rb index d956964c5..3a40ad43f 100644 --- a/spec/features/legislation/proposals_spec.rb +++ b/spec/features/legislation/proposals_spec.rb @@ -64,6 +64,19 @@ feature 'Legislation Proposals' do expect(legislation_proposals_order).to eq(first_page_proposals_order) end + scenario 'Does not crash when the seed is not a number' do + create_list( + :legislation_proposal, + (Legislation::Proposal.default_per_page + 2), + process: process + ) + + login_as user + visit legislation_process_proposals_path(process, random_seed: "Spoof") + + expect(page).to have_content "You're on page 1" + end + context 'Selected filter' do scenario 'apperars even if there are not any selected poposals' do create(:legislation_proposal, legislation_process_id: process.id) From 07c22d289c8073778f0acd91cd416b8fb16ea347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 20:49:24 +0200 Subject: [PATCH 3/8] Change the random seed before storing it Even though it probably doesn't change the behaviour, it's a bit strange to set a seed, then storing it in the session, and then modifying it again. --- app/controllers/legislation/processes_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index 359527b43..145a44871 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -118,9 +118,11 @@ class Legislation::ProcessesController < Legislation::BaseController def set_random_seed seed = (params[:random_seed] || session[:random_seed] || (rand(99) / 100.0)).to_f + seed = (-1..1).cover?(seed) ? seed : 1 + session[:random_seed] = seed params[:random_seed] = seed - seed = (-1..1).cover?(seed) ? seed : 1 + ::Legislation::Proposal.connection.execute "select setseed(#{seed})" end end From 1b46ba9ee6ac7b1a379ac9dcf856d9bfaf09fcff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 20:54:03 +0200 Subject: [PATCH 4/8] Make legislation proposals random seed more robust Using a number with only two decimals means the seed is going to be the same 1% of the time. Using ruby's default value for random numbers makes it almost impossible to generate the same seed twice. Using `rand` also simplifies the code, and it's what we use in the budget investments controller. --- app/controllers/legislation/processes_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index 145a44871..fd301fb07 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -117,7 +117,7 @@ class Legislation::ProcessesController < Legislation::BaseController end def set_random_seed - seed = (params[:random_seed] || session[:random_seed] || (rand(99) / 100.0)).to_f + seed = (params[:random_seed] || session[:random_seed] || rand).to_f seed = (-1..1).cover?(seed) ? seed : 1 session[:random_seed] = seed From f391023b7db79ce8ebd68a2315eca9d17dc9625b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 21:00:40 +0200 Subject: [PATCH 5/8] Group related specs together --- spec/features/legislation/proposals_spec.rb | 98 ++++++++++----------- 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/spec/features/legislation/proposals_spec.rb b/spec/features/legislation/proposals_spec.rb index 3a40ad43f..8e0e4659d 100644 --- a/spec/features/legislation/proposals_spec.rb +++ b/spec/features/legislation/proposals_spec.rb @@ -20,61 +20,61 @@ feature 'Legislation Proposals' do end end - scenario 'Each user has a different and consistent random proposals order', :js do - create_list(:legislation_proposal, 10, process: process) + feature "Random pagination" do + before do + create_list( + :legislation_proposal, + (Legislation::Proposal.default_per_page + 2), + process: process + ) + end - in_browser(:one) do + scenario 'Each user has a different and consistent random proposals order', :js do + in_browser(:one) do + login_as user + visit legislation_process_proposals_path(process) + @first_user_proposals_order = legislation_proposals_order + end + + in_browser(:two) do + login_as user2 + visit legislation_process_proposals_path(process) + @second_user_proposals_order = legislation_proposals_order + end + + expect(@first_user_proposals_order).not_to eq(@second_user_proposals_order) + + in_browser(:one) do + visit legislation_process_proposals_path(process) + expect(legislation_proposals_order).to eq(@first_user_proposals_order) + end + + in_browser(:two) do + visit legislation_process_proposals_path(process) + expect(legislation_proposals_order).to eq(@second_user_proposals_order) + end + end + + scenario 'Random order maintained with pagination', :js do login_as user visit legislation_process_proposals_path(process) - @first_user_proposals_order = legislation_proposals_order + first_page_proposals_order = legislation_proposals_order + + click_link 'Next' + expect(page).to have_content "You're on page 2" + + click_link 'Previous' + expect(page).to have_content "You're on page 1" + + expect(legislation_proposals_order).to eq(first_page_proposals_order) end - in_browser(:two) do - login_as user2 - visit legislation_process_proposals_path(process) - @second_user_proposals_order = legislation_proposals_order + scenario 'Does not crash when the seed is not a number' do + login_as user + visit legislation_process_proposals_path(process, random_seed: "Spoof") + + expect(page).to have_content "You're on page 1" end - - expect(@first_user_proposals_order).not_to eq(@second_user_proposals_order) - - in_browser(:one) do - visit legislation_process_proposals_path(process) - expect(legislation_proposals_order).to eq(@first_user_proposals_order) - end - - in_browser(:two) do - visit legislation_process_proposals_path(process) - expect(legislation_proposals_order).to eq(@second_user_proposals_order) - end - end - - scenario 'Random order maintained with pagination', :js do - create_list(:legislation_proposal, (Kaminari.config.default_per_page + 2), process: process) - - login_as user - visit legislation_process_proposals_path(process) - first_page_proposals_order = legislation_proposals_order - - click_link 'Next' - expect(page).to have_content "You're on page 2" - - click_link 'Previous' - expect(page).to have_content "You're on page 1" - - expect(legislation_proposals_order).to eq(first_page_proposals_order) - end - - scenario 'Does not crash when the seed is not a number' do - create_list( - :legislation_proposal, - (Legislation::Proposal.default_per_page + 2), - process: process - ) - - login_as user - visit legislation_process_proposals_path(process, random_seed: "Spoof") - - expect(page).to have_content "You're on page 1" end context 'Selected filter' do From 637c188beef5ded76e7c01afc507fb366f591c9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 21:26:31 +0200 Subject: [PATCH 6/8] Make test easier to follow Checking the contents of the second page while on the second page makes more sense than checking them after going back to the first page. --- spec/features/legislation/proposals_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/features/legislation/proposals_spec.rb b/spec/features/legislation/proposals_spec.rb index 8e0e4659d..45842fdf4 100644 --- a/spec/features/legislation/proposals_spec.rb +++ b/spec/features/legislation/proposals_spec.rb @@ -61,11 +61,13 @@ feature 'Legislation Proposals' do first_page_proposals_order = legislation_proposals_order click_link 'Next' + expect(page).to have_content "You're on page 2" + expect(first_page_proposals_order & legislation_proposals_order).to eq([]) click_link 'Previous' - expect(page).to have_content "You're on page 1" + expect(page).to have_content "You're on page 1" expect(legislation_proposals_order).to eq(first_page_proposals_order) end From 64167a86b43ecd52e9a2b27319e40cd5f70452a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 1 Oct 2018 21:29:32 +0200 Subject: [PATCH 7/8] Be more consistent using double quotes --- spec/features/legislation/proposals_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/features/legislation/proposals_spec.rb b/spec/features/legislation/proposals_spec.rb index 45842fdf4..66d0bd9a9 100644 --- a/spec/features/legislation/proposals_spec.rb +++ b/spec/features/legislation/proposals_spec.rb @@ -29,7 +29,7 @@ feature 'Legislation Proposals' do ) end - scenario 'Each user has a different and consistent random proposals order', :js do + scenario "Each user has a different and consistent random proposals order", :js do in_browser(:one) do login_as user visit legislation_process_proposals_path(process) @@ -55,23 +55,23 @@ feature 'Legislation Proposals' do end end - scenario 'Random order maintained with pagination', :js do + scenario "Random order maintained with pagination", :js do login_as user visit legislation_process_proposals_path(process) first_page_proposals_order = legislation_proposals_order - click_link 'Next' + click_link "Next" expect(page).to have_content "You're on page 2" expect(first_page_proposals_order & legislation_proposals_order).to eq([]) - click_link 'Previous' + click_link "Previous" expect(page).to have_content "You're on page 1" expect(legislation_proposals_order).to eq(first_page_proposals_order) end - scenario 'Does not crash when the seed is not a number' do + scenario "Does not crash when the seed is not a number" do login_as user visit legislation_process_proposals_path(process, random_seed: "Spoof") From 09add3554ff0016e9fc328789d398776df71c067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 2 Oct 2018 11:57:40 +0200 Subject: [PATCH 8/8] Create less records in random pagination tests We make the tests considerably faster, we make them more robust against changes in the number of records shown per page, and we generate enough records so the chance of randomly getting the same results twice in a row is extremely low. --- spec/features/legislation/proposals_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/features/legislation/proposals_spec.rb b/spec/features/legislation/proposals_spec.rb index 66d0bd9a9..2b005e3a3 100644 --- a/spec/features/legislation/proposals_spec.rb +++ b/spec/features/legislation/proposals_spec.rb @@ -22,6 +22,8 @@ feature 'Legislation Proposals' do feature "Random pagination" do before do + allow(Legislation::Proposal).to receive(:default_per_page).and_return(12) + create_list( :legislation_proposal, (Legislation::Proposal.default_per_page + 2),