From 87e82b4894671b222656c93f5255471747267340 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Lang <jp_lang@yahoo.fr>
Date: Thu, 20 Jun 2019 06:13:11 +0000
Subject: [PATCH] Issues in paginated views may be lost because sorting
 criteria are not unique (#29581).

Patch by Mizuki ISHIKAWA.

git-svn-id: http://svn.redmine.org/redmine/trunk@18263 e93f8b46-1217-0410-a6f0-8f06a7374b81
---
 app/models/issue_query.rb          |  8 ++++++++
 app/models/time_entry_query.rb     |  1 +
 test/unit/query_test.rb            | 14 ++++++++++++++
 test/unit/time_entry_query_test.rb | 15 +++++++++++++++
 4 files changed, 38 insertions(+)

diff --git a/app/models/issue_query.rb b/app/models/issue_query.rb
index 07eac0aad..589089bad 100644
--- a/app/models/issue_query.rb
+++ b/app/models/issue_query.rb
@@ -297,6 +297,10 @@ class IssueQuery < Query
   # Valid options are :order, :offset, :limit, :include, :conditions
   def issues(options={})
     order_option = [group_by_sort_order, (options[:order] || sort_clause)].flatten.reject(&:blank?)
+    # The default order of IssueQuery is issues.id DESC(by IssueQuery#default_sort_criteria)
+    unless ["#{Issue.table_name}.id ASC", "#{Issue.table_name}.id DESC"].any?{|i| order_option.include?(i)}
+      order_option << "#{Issue.table_name}.id DESC"
+    end
 
     scope = Issue.visible.
       joins(:status, :project).
@@ -339,6 +343,10 @@ class IssueQuery < Query
   # Returns the issues ids
   def issue_ids(options={})
     order_option = [group_by_sort_order, (options[:order] || sort_clause)].flatten.reject(&:blank?)
+    # The default order of IssueQuery is issues.id DESC(by IssueQuery#default_sort_criteria)
+    unless ["#{Issue.table_name}.id ASC", "#{Issue.table_name}.id DESC"].any?{|i| order_option.include?(i)}
+      order_option << "#{Issue.table_name}.id DESC"
+    end
 
     Issue.visible.
       joins(:status, :project).
diff --git a/app/models/time_entry_query.rb b/app/models/time_entry_query.rb
index 9c767266b..5ca95ea91 100644
--- a/app/models/time_entry_query.rb
+++ b/app/models/time_entry_query.rb
@@ -148,6 +148,7 @@ class TimeEntryQuery < Query
   def results_scope(options={})
     order_option = [group_by_sort_order, (options[:order] || sort_clause)].flatten.reject(&:blank?)
 
+    order_option << "#{TimeEntry.table_name}.id ASC"
     base_scope.
       order(order_option).
       joins(joins_for_order_statement(order_option.join(',')))
diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb
index f0a67ee6f..4e590b759 100644
--- a/test/unit/query_test.rb
+++ b/test/unit/query_test.rb
@@ -2329,4 +2329,18 @@ class QueryTest < ActiveSupport::TestCase
     query.filters = {'spent_time' => {:operator => '><', :values => ['1', '2']}}
     assert_equal [3], query.issues.pluck(:id)
   end
+
+  def test_issues_should_be_in_the_same_order_when_paginating
+    q = IssueQuery.new
+    q.sort_criteria = {'0' => ['priority', 'desc']}
+    issue_ids = q.issues.pluck(:id)
+    paginated_issue_ids = []
+    # Test with a maximum of 2 records per page.
+    ((q.issue_count / 2) + 1).times do |i|
+      paginated_issue_ids += q.issues(:offset => (i * 2), :limit => 2).pluck(:id)
+    end
+
+    # Non-paginated issue ids and paginated issue ids should be in the same order.
+    assert_equal issue_ids, paginated_issue_ids
+  end
 end
diff --git a/test/unit/time_entry_query_test.rb b/test/unit/time_entry_query_test.rb
index 0d824bc3d..1f8301f8f 100644
--- a/test/unit/time_entry_query_test.rb
+++ b/test/unit/time_entry_query_test.rb
@@ -129,4 +129,19 @@ class TimeEntryQueryTest < ActiveSupport::TestCase
     query = TimeEntryQuery.new(:project => Project.find(2), :name => '_')
     assert !query.available_filters.has_key?('project.status')
   end
+
+  def test_results_scope_should_be_in_the_same_order_when_paginating
+    4.times { TimeEntry.generate! }
+    q = TimeEntryQuery.new
+    q.sort_criteria = {'0' => ['user', 'asc']}
+    time_entry_ids = q.results_scope.pluck(:id)
+    paginated_time_entry_ids = []
+    # Test with a maximum of 2 records per page.
+    ((q.results_scope.count / 2) + 1).times do |i|
+      paginated_time_entry_ids += q.results_scope.offset((i * 2)).limit(2).pluck(:id)
+    end
+
+    # Non-paginated time entry ids and paginated time entry ids should be in the same order.
+    assert_equal time_entry_ids, paginated_time_entry_ids
+  end
 end
-- 
2.39.5