Merge pull request #2131 from wairbut-m2c/iagirre-budget-pagination-bug

Iagirre budget pagination bug

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,8 @@ 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})")
+          seed = rand(10..99) / 10.0
+          params[:random_seed] ||= Float(seed) rescue 0
         else
           params[:random_seed] = nil
         end
@@ -131,6 +130,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

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 || 1}, budget_investments.id") }
 
     scope :valuation_open,              -> { where(valuation_finished: false) }
     scope :without_admin,               -> { valuation_open.where(administrator_id: nil) }

spec/features/budgets/investments_spec.rb

@@ -1,4 +1,5 @@
 require 'rails_helper'
+require 'sessions_helper'
 
 feature 'Budget Investments' do
 
@@ -170,6 +171,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 = investments_order
+
+      click_link 'Next'
+      expect(page).to have_content "You're on page 2"
+
+      second_page_investments = investments_order
+
+      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)
@@ -188,6 +208,46 @@ feature 'Budget Investments' do
       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
+        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
+        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
 
   context 'Phase I - Accepting' do