From 660c59016b5bc53c6f62287daf9227a0bdaee8b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Dec 2018 16:43:19 +0100 Subject: [PATCH 1/3] Fix random proposals order in the same session Using `setseed` and ordering by `RAND()` doesn't always return the same results because, although the generated random numbers will always be the same, PostgreSQL doesn't guarantee the order of the rows it will apply those random numbers to, similar to the way it doesn't guarantee an order when the `ORDER BY` clause isn't specified. Using something like `reorder("legislation_proposals.id % #{seed}")`, like we do in budget investments, is certainly more elegant but it makes the test checking two users get different results fail sometimes, so that approach might need some adjustments in order to make the results more random. --- .../legislation/processes_controller.rb | 12 +++++++----- app/models/legislation/proposal.rb | 14 ++++++++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index 0e5d030be..de4213191 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -103,7 +103,12 @@ class Legislation::ProcessesController < Legislation::BaseController @proposals = @proposals.search(params[:search]) if params[:search].present? @current_filter = "winners" if params[:filter].blank? && @proposals.winners.any? - @proposals = @proposals.send(@current_filter).page(params[:page]) + + if @current_filter == "random" + @proposals = @proposals.send(@current_filter, session[:random_seed]).page(params[:page]) + else + @proposals = @proposals.send(@current_filter).page(params[:page]) + end if @process.proposals_phase.started? || (current_user && current_user.administrator?) legislation_proposal_votes(@proposals) @@ -125,12 +130,9 @@ class Legislation::ProcessesController < Legislation::BaseController end def set_random_seed - seed = (params[:random_seed] || session[:random_seed] || rand).to_f - seed = (-1..1).cover?(seed) ? seed : 1 + seed = (params[:random_seed] || session[:random_seed] || rand(10_000_000)).to_i session[:random_seed] = seed params[:random_seed] = seed - - ::Legislation::Proposal.connection.execute "select setseed(#{seed})" end end diff --git a/app/models/legislation/proposal.rb b/app/models/legislation/proposal.rb index 15ed53428..531a46387 100644 --- a/app/models/legislation/proposal.rb +++ b/app/models/legislation/proposal.rb @@ -48,13 +48,23 @@ class Legislation::Proposal < ActiveRecord::Base scope :sort_by_title, -> { reorder(title: :asc) } scope :sort_by_id, -> { reorder(id: :asc) } scope :sort_by_supports, -> { reorder(cached_votes_score: :desc) } - scope :sort_by_random, -> { reorder("RANDOM()") } scope :sort_by_flags, -> { order(flags_count: :desc, updated_at: :desc) } scope :last_week, -> { where("proposals.created_at >= ?", 7.days.ago)} scope :selected, -> { where(selected: true) } - scope :random, -> { sort_by_random } + scope :random, -> (seed) { sort_by_random(seed) } scope :winners, -> { selected.sort_by_confidence_score } + def self.sort_by_random(seed) + ids = pluck(:id).shuffle(random: Random.new(seed)) + + return all if ids.empty? + + ids_with_order = ids.map.with_index { |id, order| "(#{id}, #{order})" }.join(", ") + + joins("LEFT JOIN (VALUES #{ids_with_order}) AS ids(id, ordering) ON #{table_name}.id = ids.id") + .order("ids.ordering") + end + def to_param "#{id}-#{title}".parameterize end From e3ca700e17ccbdebe166b39a8018bd2bb24a0a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Dec 2018 16:53:50 +0100 Subject: [PATCH 2/3] Add concerns to set and order by a random seed --- app/controllers/concerns/random_seed.rb | 10 ++++++++++ .../legislation/processes_controller.rb | 9 ++------- app/models/concerns/randomizable.rb | 18 ++++++++++++++++++ app/models/legislation/proposal.rb | 13 +------------ 4 files changed, 31 insertions(+), 19 deletions(-) create mode 100644 app/controllers/concerns/random_seed.rb create mode 100644 app/models/concerns/randomizable.rb diff --git a/app/controllers/concerns/random_seed.rb b/app/controllers/concerns/random_seed.rb new file mode 100644 index 000000000..a54d6832e --- /dev/null +++ b/app/controllers/concerns/random_seed.rb @@ -0,0 +1,10 @@ +module RandomSeed + extend ActiveSupport::Concern + + def set_random_seed + seed = (params[:random_seed] || session[:random_seed] || rand(10_000_000)).to_i + + session[:random_seed] = seed + params[:random_seed] = seed + end +end diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index de4213191..5c796224f 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -1,4 +1,6 @@ class Legislation::ProcessesController < Legislation::BaseController + include RandomSeed + has_filters %w[open past], only: :index has_filters %w[random winners], only: :proposals @@ -128,11 +130,4 @@ class Legislation::ProcessesController < Legislation::BaseController return if member_method? @process = ::Legislation::Process.find(params[:process_id]) end - - def set_random_seed - seed = (params[:random_seed] || session[:random_seed] || rand(10_000_000)).to_i - - session[:random_seed] = seed - params[:random_seed] = seed - end end diff --git a/app/models/concerns/randomizable.rb b/app/models/concerns/randomizable.rb new file mode 100644 index 000000000..e632b06ce --- /dev/null +++ b/app/models/concerns/randomizable.rb @@ -0,0 +1,18 @@ +module Randomizable + extend ActiveSupport::Concern + + class_methods do + def sort_by_random(seed) + ids = pluck(:id).shuffle(random: Random.new(seed)) + + return all if ids.empty? + + ids_with_order = ids.map.with_index { |id, order| "(#{id}, #{order})" }.join(", ") + + joins("LEFT JOIN (VALUES #{ids_with_order}) AS ids(id, ordering) ON #{table_name}.id = ids.id") + .order("ids.ordering") + end + + alias_method :random, :sort_by_random + end +end diff --git a/app/models/legislation/proposal.rb b/app/models/legislation/proposal.rb index 531a46387..2f97141b5 100644 --- a/app/models/legislation/proposal.rb +++ b/app/models/legislation/proposal.rb @@ -12,6 +12,7 @@ class Legislation::Proposal < ActiveRecord::Base include Documentable include Notifiable include Imageable + include Randomizable documentable max_documents_allowed: 3, max_file_size: 3.megabytes, @@ -51,20 +52,8 @@ class Legislation::Proposal < ActiveRecord::Base scope :sort_by_flags, -> { order(flags_count: :desc, updated_at: :desc) } scope :last_week, -> { where("proposals.created_at >= ?", 7.days.ago)} scope :selected, -> { where(selected: true) } - scope :random, -> (seed) { sort_by_random(seed) } scope :winners, -> { selected.sort_by_confidence_score } - def self.sort_by_random(seed) - ids = pluck(:id).shuffle(random: Random.new(seed)) - - return all if ids.empty? - - ids_with_order = ids.map.with_index { |id, order| "(#{id}, #{order})" }.join(", ") - - joins("LEFT JOIN (VALUES #{ids_with_order}) AS ids(id, ordering) ON #{table_name}.id = ids.id") - .order("ids.ordering") - end - def to_param "#{id}-#{title}".parameterize end From 6682121069f48f14a9838ec9e1f23b119239ddf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Dec 2018 19:18:10 +0100 Subject: [PATCH 3/3] Reuse code to set and order by a random seed --- app/controllers/budgets/investments_controller.rb | 13 ++----------- app/controllers/legislation/processes_controller.rb | 2 +- app/models/budget/investment.rb | 2 +- app/models/concerns/randomizable.rb | 4 +--- app/models/debate.rb | 2 +- app/models/proposal.rb | 2 +- spec/features/budgets/investments_spec.rb | 6 +++--- 7 files changed, 10 insertions(+), 21 deletions(-) diff --git a/app/controllers/budgets/investments_controller.rb b/app/controllers/budgets/investments_controller.rb index a48fa3188..0c054381d 100644 --- a/app/controllers/budgets/investments_controller.rb +++ b/app/controllers/budgets/investments_controller.rb @@ -4,6 +4,7 @@ module Budgets include FeatureFlags include CommentableActions include FlagActions + include RandomSeed include ImageAttributes PER_PAGE = 10 @@ -119,16 +120,6 @@ module Budgets @investment_votes = current_user ? current_user.budget_investment_votes(investments) : {} end - def set_random_seed - if params[:order] == 'random' || params[:order].blank? - seed = params[:random_seed] || session[:random_seed] || rand - params[:random_seed] = seed - session[:random_seed] = params[:random_seed] - else - params[:random_seed] = nil - end - end - def investment_params params.require(:budget_investment) .permit(:title, :description, :heading_id, :tag_list, @@ -170,7 +161,7 @@ module Budgets def investments if @current_order == 'random' @budget.investments.apply_filters_and_search(@budget, params, @current_filter) - .send("sort_by_#{@current_order}", params[:random_seed]) + .sort_by_random(session[:random_seed]) else @budget.investments.apply_filters_and_search(@budget, params, @current_filter) .send("sort_by_#{@current_order}") diff --git a/app/controllers/legislation/processes_controller.rb b/app/controllers/legislation/processes_controller.rb index 5c796224f..eeef4fd78 100644 --- a/app/controllers/legislation/processes_controller.rb +++ b/app/controllers/legislation/processes_controller.rb @@ -107,7 +107,7 @@ class Legislation::ProcessesController < Legislation::BaseController @current_filter = "winners" if params[:filter].blank? && @proposals.winners.any? if @current_filter == "random" - @proposals = @proposals.send(@current_filter, session[:random_seed]).page(params[:page]) + @proposals = @proposals.sort_by_random(session[:random_seed]).page(params[:page]) else @proposals = @proposals.send(@current_filter).page(params[:page]) end diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 8da3a1bc1..a4d3be921 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -25,6 +25,7 @@ class Budget include Filterable include Flaggable include Milestoneable + include Randomizable belongs_to :author, -> { with_hidden }, class_name: 'User', foreign_key: 'author_id' belongs_to :heading @@ -55,7 +56,6 @@ 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.to_f.nonzero? ? seed.to_f : 1}, budget_investments.id") } scope :sort_by_id, -> { order("id DESC") } scope :sort_by_title, -> { order("title ASC") } diff --git a/app/models/concerns/randomizable.rb b/app/models/concerns/randomizable.rb index e632b06ce..c346f1f02 100644 --- a/app/models/concerns/randomizable.rb +++ b/app/models/concerns/randomizable.rb @@ -2,7 +2,7 @@ module Randomizable extend ActiveSupport::Concern class_methods do - def sort_by_random(seed) + def sort_by_random(seed = rand(10_000_000)) ids = pluck(:id).shuffle(random: Random.new(seed)) return all if ids.empty? @@ -12,7 +12,5 @@ module Randomizable joins("LEFT JOIN (VALUES #{ids_with_order}) AS ids(id, ordering) ON #{table_name}.id = ids.id") .order("ids.ordering") end - - alias_method :random, :sort_by_random end end diff --git a/app/models/debate.rb b/app/models/debate.rb index 2aa1bdce4..08743848b 100644 --- a/app/models/debate.rb +++ b/app/models/debate.rb @@ -12,6 +12,7 @@ class Debate < ActiveRecord::Base include Graphqlable include Relationable include Notifiable + include Randomizable acts_as_votable acts_as_paranoid column: :hidden_at @@ -37,7 +38,6 @@ class Debate < ActiveRecord::Base scope :sort_by_confidence_score, -> { reorder(confidence_score: :desc) } scope :sort_by_created_at, -> { reorder(created_at: :desc) } scope :sort_by_most_commented, -> { reorder(comments_count: :desc) } - scope :sort_by_random, -> { reorder("RANDOM()") } scope :sort_by_relevance, -> { all } scope :sort_by_flags, -> { order(flags_count: :desc, updated_at: :desc) } scope :sort_by_recommendations, -> { order(cached_votes_total: :desc) } diff --git a/app/models/proposal.rb b/app/models/proposal.rb index 50c7abd09..3cf6811e4 100644 --- a/app/models/proposal.rb +++ b/app/models/proposal.rb @@ -21,6 +21,7 @@ class Proposal < ActiveRecord::Base include EmbedVideosHelper include Relationable include Milestoneable + include Randomizable acts_as_votable acts_as_paranoid column: :hidden_at @@ -58,7 +59,6 @@ class Proposal < ActiveRecord::Base scope :sort_by_confidence_score, -> { reorder(confidence_score: :desc) } scope :sort_by_created_at, -> { reorder(created_at: :desc) } scope :sort_by_most_commented, -> { reorder(comments_count: :desc) } - scope :sort_by_random, -> { reorder("RANDOM()") } scope :sort_by_relevance, -> { all } scope :sort_by_flags, -> { order(flags_count: :desc, updated_at: :desc) } scope :sort_by_archival_date, -> { archived.sort_by_confidence_score } diff --git a/spec/features/budgets/investments_spec.rb b/spec/features/budgets/investments_spec.rb index 00cc74239..52ce89fd2 100644 --- a/spec/features/budgets/investments_spec.rb +++ b/spec/features/budgets/investments_spec.rb @@ -685,16 +685,16 @@ 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' do + scenario "Each user has a different and consistent random budget investment order" do (Kaminari.config.default_per_page * 1.3).to_i.times { create(:budget_investment, heading: heading) } in_browser(:one) do - visit budget_investments_path(budget, heading: heading, random_seed: rand) + 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, random_seed: rand) + visit budget_investments_path(budget, heading: heading) @second_user_investments_order = investments_order end