From 7f0e447e0f6be5dfb46d4c6deb3be835107b9f2b Mon Sep 17 00:00:00 2001 From: iagirre Date: Wed, 15 Nov 2017 08:56:31 +0100 Subject: [PATCH 1/5] One aproach to make the randomness work with kaminari --- .../budgets/investments_controller.rb | 19 ++++++++++++++----- app/models/budget/investment.rb | 2 +- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index bb00f24af..0c3f9a538 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -28,8 +28,8 @@ module Budgets respond_to :html, :js def index - @investments = @investments.apply_filters_and_search(@budget, params, @current_filter) - .send("sort_by_#{@current_order}").page(params[:page]).per(10).for_render + @investments = investments.page(params[:page]).per(10).for_render + @investment_ids = @investments.pluck(:id) load_investment_votes(@investments) @tag_cloud = tag_cloud @@ -94,9 +94,7 @@ module Budgets def set_random_seed if params[:order] == 'random' || params[:order].blank? - params[:random_seed] ||= rand(99) / 100.0 - seed = Float(params[:random_seed]) rescue 0 - Budget::Investment.connection.execute("select setseed(#{seed})") + params[:random_seed] ||= rand(9) else params[:random_seed] = nil end @@ -131,6 +129,17 @@ module Budgets TagCloud.new(Budget::Investment, params[:search]) end + def investments + case @current_order + when 'random' + @investments.apply_filters_and_search(@budget, params, @current_filter) + .send("sort_by_#{@current_order}", params[:random_seed]) + else + @investments.apply_filters_and_search(@budget, params, @current_filter) + .send("sort_by_#{@current_order}") + end + end + end end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index ddf6e819c..8ee768574 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -43,7 +43,7 @@ class Budget scope :sort_by_confidence_score, -> { reorder(confidence_score: :desc, id: :desc) } scope :sort_by_ballots, -> { reorder(ballot_lines_count: :desc, id: :desc) } scope :sort_by_price, -> { reorder(price: :desc, confidence_score: :desc, id: :desc) } - scope :sort_by_random, -> { reorder("RANDOM()") } + scope :sort_by_random, ->(seed) { reorder("budget_investments.id % #{seed}, budget_investments.id") } scope :valuation_open, -> { where(valuation_finished: false) } scope :without_admin, -> { valuation_open.where(administrator_id: nil) } From f3527b1311e3501ea950bb3f816954b3627215c8 Mon Sep 17 00:00:00 2001 From: iagirre Date: Wed, 15 Nov 2017 09:01:35 +0100 Subject: [PATCH 2/5] Test added to check the repetition of elements between pages when random order used. Scope variable initialized to 1 --- app/models/budget/investment.rb | 2 +- spec/features/budgets/investments_spec.rb | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 8ee768574..add7c3587 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -43,7 +43,7 @@ class Budget scope :sort_by_confidence_score, -> { reorder(confidence_score: :desc, id: :desc) } scope :sort_by_ballots, -> { reorder(ballot_lines_count: :desc, id: :desc) } scope :sort_by_price, -> { reorder(price: :desc, confidence_score: :desc, id: :desc) } - scope :sort_by_random, ->(seed) { reorder("budget_investments.id % #{seed}, budget_investments.id") } + scope :sort_by_random, ->(seed) { reorder("budget_investments.id % #{seed || 1}, budget_investments.id") } scope :valuation_open, -> { where(valuation_finished: false) } scope :without_admin, -> { valuation_open.where(administrator_id: nil) } diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index d17bd9c91..ce56dd9b2 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -170,6 +170,25 @@ feature 'Budget Investments' do expect(order).to eq(new_order) end + scenario "Investments are not repeated with random order", :js 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 + + visit budget_investments_path(budget, order: 'random') + + first_page_investments = all(".budget-investment h3").collect {|i| i.text } + + click_link 'Next' + expect(page).to have_content "You're on page 2" + + second_page_investments = all(".budget-investment h3").collect {|i| i.text } + + common_values = first_page_investments & second_page_investments + + expect(common_values.length).to eq(0) + + end + scenario 'Proposals are ordered by confidence_score', :js do create(:budget_investment, heading: heading, title: 'Best proposal').update_column(:confidence_score, 10) create(:budget_investment, heading: heading, title: 'Worst proposal').update_column(:confidence_score, 2) From fbb02095bb02544352130c7f754e749fb9e2e0e4 Mon Sep 17 00:00:00 2001 From: iagirre Date: Wed, 15 Nov 2017 09:02:19 +0100 Subject: [PATCH 3/5] Some fixes to new code to make the test pass --- app/controllers/budgets/investments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index 0c3f9a538..a18c82cdf 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -94,7 +94,7 @@ module Budgets def set_random_seed if params[:order] == 'random' || params[:order].blank? - params[:random_seed] ||= rand(9) + params[:random_seed] ||= rand(1..9) else params[:random_seed] = nil end From 87f6f14d48c5b7b5ee6469da4aaa7673d6ffcd59 Mon Sep 17 00:00:00 2001 From: iagirre Date: Wed, 15 Nov 2017 11:50:06 +0100 Subject: [PATCH 4/5] Bigger random seed range. Tests added to check the randomness between browsers --- .../budgets/investments_controller.rb | 3 +- spec/features/budgets/investments_spec.rb | 47 ++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index a18c82cdf..aaf65765f 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -94,7 +94,8 @@ module Budgets def set_random_seed if params[:order] == 'random' || params[:order].blank? - params[:random_seed] ||= rand(1..9) + seed = rand(10..99) / 10.0 + params[:random_seed] ||= Float(seed) rescue 0 else params[:random_seed] = nil end diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index ce56dd9b2..535e272a3 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require 'sessions_helper' feature 'Budget Investments' do @@ -176,12 +177,12 @@ feature 'Budget Investments' do visit budget_investments_path(budget, order: 'random') - first_page_investments = all(".budget-investment h3").collect {|i| i.text } + first_page_investments = investments_order click_link 'Next' expect(page).to have_content "You're on page 2" - second_page_investments = all(".budget-investment h3").collect {|i| i.text } + second_page_investments = investments_order common_values = first_page_investments & second_page_investments @@ -206,6 +207,48 @@ feature 'Budget Investments' do expect(current_url).to include('order=confidence_score') expect(current_url).to include('page=1') end + + scenario 'Each user as a different and consistent random budget investment order', :js do + 12.times { create(:budget_investment, heading: heading) } + + in_browser(:one) do + visit budget_investments_path(budget, heading: heading) + @first_user_investments_order = investments_order + end + + in_browser(:two) do + visit budget_investments_path(budget, heading: heading) + @second_user_investments_order = investments_order + end + + expect(@first_user_investments_order).not_to eq(@second_user_investments_order) + + in_browser(:one) do + visit budget_investments_path(budget, heading: heading) +# 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(investments_order).to eq(@first_user_investments_order) + end + + in_browser(:two) do + visit budget_investments_path(budget, heading: heading) +# 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(investments_order).to eq(@second_user_investments_order) + end + end + + def investments_order + all(".budget-investment h3").collect {|i| i.text } + end end From d1185662349e31517d2c7cbdb4360b358c855bf0 Mon Sep 17 00:00:00 2001 From: iagirre Date: Wed, 15 Nov 2017 12:49:21 +0100 Subject: [PATCH 5/5] Test changed to check the order when navigating through pages --- spec/features/budgets/investments_spec.rb | 30 +++++++++++------------ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index 535e272a3..fa7749a4b 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -207,7 +207,7 @@ feature 'Budget Investments' do expect(current_url).to include('order=confidence_score') expect(current_url).to include('page=1') end - + scenario 'Each user as a different and consistent random budget investment order', :js do 12.times { create(:budget_investment, heading: heading) } @@ -224,28 +224,26 @@ feature 'Budget Investments' do expect(@first_user_investments_order).not_to eq(@second_user_investments_order) in_browser(:one) do - visit budget_investments_path(budget, heading: heading) -# 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" -# + 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(investments_order).to eq(@first_user_investments_order) end in_browser(:two) do - visit budget_investments_path(budget, heading: heading) -# 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" -# + 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(investments_order).to eq(@second_user_investments_order) end end - + def investments_order all(".budget-investment h3").collect {|i| i.text } end