From ce3cb045f8a1143bc3e8b54fbb600ff34daef1b2 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 1 Mar 2018 22:54:49 +0100 Subject: [PATCH 1/4] Convert random seed to a small value We are trying out a modulus function to return investments in random order https://github.com/consul/consul/pull/2131 However we ran into the gotcha of having a seed value too big for the modulus function to work as expected If the seed is bigger than the investment id, the records are returned ordered by id By dividing the seed by a big number, this problem seems to get fixed --- app/controllers/budgets/investments_controller.rb | 3 ++- spec/features/budgets/investments_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index c48ce7fc4..5d1251d90 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -112,7 +112,8 @@ module Budgets def set_random_seed if params[:order] == 'random' || params[:order].blank? seed = params[:random_seed] || session[:random_seed] || rand(-100000..100000) - params[:random_seed] ||= Float(seed) rescue 0 + params[:random_seed] = Float(seed) / 1000000 rescue 0 + session[:random_seed] = params[:random_seed] else params[:random_seed] = nil end diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index d2ea73390..f5a650c18 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -610,7 +610,15 @@ feature 'Budget Investments' do end expect(@first_user_investments_order).to eq(@second_user_investments_order) + scenario "Convert seed to a value small enough for the modulus function to return investments in random order", :focus do + 12.times { |i| create(:budget_investment, heading: heading, id: i) } + visit budget_investments_path(budget, heading_id: heading.id, random_seed: '12') + + order = investments_order + orderd_by_id = Budget::Investment.order(:id).limit(10).pluck(:title) + + expect(order).to_not eq(orderd_by_id) end def investments_order From ef30dc1efe6c7a2c31a502edbb0fcef508ffea96 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 1 Mar 2018 22:52:37 +0100 Subject: [PATCH 2/4] Add defensive test to display correctly a user's votes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a defensive test, just in case we decide to go back to using `setseed` instead of the `modulus`[1] approach to display investments in random order The reason for this test is that `setseed` only ~works in the next `select` statement. And as when loading a user’s votes for investments we do a second `select` it does not work as expected 😌 To solve this… we could call `set_random_seed` before loading a user’s votes for an investment[2] [1] https://github.com/consul/consul/pull/2131 [2] https://github.com/AyuntamientoMadrid/consul/blob/master/app/controllers /budgets/investments_controller.rb#L37 --- spec/features/budgets/investments_spec.rb | 24 +++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index f5a650c18..280b2fc6b 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -621,6 +621,30 @@ feature 'Budget Investments' do expect(order).to_not eq(orderd_by_id) end + scenario "Set votes for investments randomized with a seed" do + voter = create(:user, :level_two) + login_as(voter) + + 10.times { create(:budget_investment, heading: heading) } + + voted_investments = [] + 10.times do + investment = create(:budget_investment, heading: heading) + create(:vote, votable: investment, voter: voter) + voted_investments << investment + end + + visit budget_investments_path(budget, heading_id: heading.id) + + voted_investments.each do |investment| + if page.has_link?(investment.title) + within("#budget_investment_#{investment.id}") do + expect(page).to have_content "You have already supported this investment" + end + end + end + end + def investments_order all(".budget-investment h3").collect {|i| i.text } end From 3dcbb6b9764f8a41ab71896b737ed0498eafebf1 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 1 Mar 2018 22:39:37 +0100 Subject: [PATCH 3/4] Run random order tests without javascript Speeds it up a little, there is no need to take into account javascript in these tests --- spec/features/budgets/investments_spec.rb | 26 +++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index 280b2fc6b..477ec4b4f 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -470,7 +470,7 @@ feature 'Budget Investments' do end end - context("Orders") do + context "Orders" do before { budget.update(phase: 'selecting') } scenario "Default order is random" do @@ -502,7 +502,7 @@ feature 'Budget Investments' do expect(order).not_to eq(new_order) end - scenario 'Random order maintained with pagination', :js do + scenario 'Random order maintained with pagination' do per_page = Kaminari.config.default_per_page (per_page + 2).times { create(:budget_investment, heading: heading) } @@ -520,7 +520,21 @@ feature 'Budget Investments' do expect(order).to eq(new_order) end - scenario "Investments are not repeated with random order", :js do + scenario 'Random order maintained when going back from show' do + 10.times { |i| create(:budget_investment, heading: heading) } + + visit budget_investments_path(budget, heading_id: heading.id) + + order = all(".budget-investment h3").collect {|i| i.text } + + click_link Budget::Investment.first.title + click_link "Go back" + + new_order = all(".budget-investment h3").collect {|i| i.text } + expect(order).to eq(new_order) + end + + scenario "Investments are not repeated with random order" do 12.times { create(:budget_investment, heading: heading) } # 12 instead of per_page + 2 because in each page there are 10 (in this case), not 25 @@ -539,7 +553,7 @@ feature 'Budget Investments' do end - scenario 'Proposals are ordered by confidence_score', :js do + scenario 'Proposals are ordered by confidence_score' do best_proposal = create(:budget_investment, heading: heading, title: 'Best proposal') best_proposal.update_column(:confidence_score, 10) worst_proposal = create(:budget_investment, heading: heading, title: 'Worst proposal') @@ -560,7 +574,7 @@ feature 'Budget Investments' do expect(current_url).to include('page=1') end - scenario 'Each user has a different and consistent random budget investment order when random_seed is disctint', :js do + scenario 'Each user has a different and consistent random budget investment order when random_seed is disctint' do (Kaminari.config.default_per_page * 1.3).to_i.times { create(:budget_investment, heading: heading) } in_browser(:one) do @@ -596,7 +610,7 @@ feature 'Budget Investments' do end end - scenario 'Each user has a equal and consistent budget investment order when the random_seed is equal', :js do + scenario 'Each user has a equal and consistent budget investment order when the random_seed is equal' do (Kaminari.config.default_per_page * 1.3).to_i.times { create(:budget_investment, heading: heading) } in_browser(:one) do From 65e236065086b90328fd92bf879adc76bf1382bc Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 1 Mar 2018 22:20:07 +0100 Subject: [PATCH 4/4] Use a float smaller than 1 as a random seed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By using a random seed value smaller than 1, we solve the previous situation[1] in a simpler way This test is now obsolete. It’s hard to write a tests to verify that even with a big seed in params, we will covert it to a float smaller than 1. We should refactor these `set_random_seed` methods into a nice model or controller concern and test it thoroughly [1] https://github.com/AyuntamientoMadrid/consul/commit/ba3bf11526fc6ce9c66f 647c414946c61ff945fe --- app/controllers/budgets/investments_controller.rb | 4 ++-- spec/features/budgets/investments_spec.rb | 9 --------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index 5d1251d90..eb875ec23 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -111,8 +111,8 @@ module Budgets def set_random_seed if params[:order] == 'random' || params[:order].blank? - seed = params[:random_seed] || session[:random_seed] || rand(-100000..100000) - params[:random_seed] = Float(seed) / 1000000 rescue 0 + seed = params[:random_seed] || session[:random_seed] || rand + params[:random_seed] = seed session[:random_seed] = params[:random_seed] else params[:random_seed] = nil diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index 477ec4b4f..432261150 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -624,15 +624,6 @@ feature 'Budget Investments' do end expect(@first_user_investments_order).to eq(@second_user_investments_order) - scenario "Convert seed to a value small enough for the modulus function to return investments in random order", :focus do - 12.times { |i| create(:budget_investment, heading: heading, id: i) } - - visit budget_investments_path(budget, heading_id: heading.id, random_seed: '12') - - order = investments_order - orderd_by_id = Budget::Investment.order(:id).limit(10).pluck(:title) - - expect(order).to_not eq(orderd_by_id) end scenario "Set votes for investments randomized with a seed" do