Merge branch 'osw-fix-missing-and-duplicated-milestones-on-list' into 'master'

Fix missing and duplicates on project milestone listing page

Closes #37078

See merge request gitlab-org/gitlab-ce!21058

5 files changed, 57 insertions, 19 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 7a459078151..1588f76989b 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -154,7 +154,7 @@ module Issuable
         end
 
       # Break ties with the ID column for pagination
-      sorted.order(id: :desc)
+      sorted.with_order_id_desc
     end
 
     def order_due_date_and_labels_priority(excluded_labels: [])
diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index cb76ae971d4..409255fb68b 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -6,6 +6,7 @@ module Sortable
   extend ActiveSupport::Concern
 
   included do
+    scope :with_order_id_desc, -> { order(id: :desc) }
     scope :order_id_desc, -> { reorder(id: :desc) }
     scope :order_id_asc, -> { reorder(id: :asc) }
     scope :order_created_desc, -> { reorder(created_at: :desc) }
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index f2b2d291da9..cb1def1b422 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -146,22 +146,25 @@ class Milestone < ActiveRecord::Base
   end
 
   def self.sort_by_attribute(method)
-    case method.to_s
-    when 'due_date_asc'
-      reorder(Gitlab::Database.nulls_last_order('due_date', 'ASC'))
-    when 'due_date_desc'
-      reorder(Gitlab::Database.nulls_last_order('due_date', 'DESC'))
-    when 'name_asc'
-      reorder(Arel::Nodes::Ascending.new(arel_table[:title].lower))
-    when 'name_desc'
-      reorder(Arel::Nodes::Descending.new(arel_table[:title].lower))
-    when 'start_date_asc'
-      reorder(Gitlab::Database.nulls_last_order('start_date', 'ASC'))
-    when 'start_date_desc'
-      reorder(Gitlab::Database.nulls_last_order('start_date', 'DESC'))
-    else
-      order_by(method)
-    end
+    sorted =
+      case method.to_s
+      when 'due_date_asc'
+        reorder(Gitlab::Database.nulls_last_order('due_date', 'ASC'))
+      when 'due_date_desc'
+        reorder(Gitlab::Database.nulls_last_order('due_date', 'DESC'))
+      when 'name_asc'
+        reorder(Arel::Nodes::Ascending.new(arel_table[:title].lower))
+      when 'name_desc'
+        reorder(Arel::Nodes::Descending.new(arel_table[:title].lower))
+      when 'start_date_asc'
+        reorder(Gitlab::Database.nulls_last_order('start_date', 'ASC'))
+      when 'start_date_desc'
+        reorder(Gitlab::Database.nulls_last_order('start_date', 'DESC'))
+      else
+        order_by(method)
+      end
+
+    sorted.with_order_id_desc
   end
 
   ##
diff --git a/changelogs/unreleased/osw-fix-missing-and-duplicated-milestones-on-list.yml b/changelogs/unreleased/osw-fix-missing-and-duplicated-milestones-on-list.yml
new file mode 100644
index 00000000000..62416b7f87e
--- /dev/null
+++ b/changelogs/unreleased/osw-fix-missing-and-duplicated-milestones-on-list.yml
@@ -0,0 +1,5 @@
+---
+title: Fix missing and duplicates on project milestone listing page
+merge_request: 21058
+author:
+type: fixed
diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb
index 6c2d1c7e92b..3190f1ce9d4 100644
--- a/spec/controllers/projects/milestones_controller_spec.rb
+++ b/spec/controllers/projects/milestones_controller_spec.rb
@@ -42,16 +42,45 @@ describe Projects::MilestonesController do
 
   describe "#index" do
     context "as html" do
-      before do
-        get :index, namespace_id: project.namespace.id, project_id: project.id
+      def render_index(project:, page:)
+        get :index, namespace_id: project.namespace.id,
+                    project_id: project.id,
+                    page: page
       end
 
       it "queries only projects milestones" do
+        render_index project: project, page: 1
+
         milestones = assigns(:milestones)
 
         expect(milestones.count).to eq(1)
         expect(milestones.where(project_id: nil)).to be_empty
       end
+
+      it 'renders paginated milestones without missing or duplicates' do
+        allow(Milestone).to receive(:default_per_page).and_return(2)
+        create_list(:milestone, 5, project: project)
+
+        render_index project: project, page: 1
+        page_1_milestones = assigns(:milestones)
+        expect(page_1_milestones.size).to eq(2)
+
+        render_index project: project, page: 2
+        page_2_milestones = assigns(:milestones)
+        expect(page_2_milestones.size).to eq(2)
+
+        render_index project: project, page: 3
+        page_3_milestones = assigns(:milestones)
+        expect(page_3_milestones.size).to eq(2)
+
+        rendered_milestone_ids =
+          page_1_milestones.pluck(:id) +
+          page_2_milestones.pluck(:id) +
+          page_3_milestones.pluck(:id)
+
+        expect(rendered_milestone_ids)
+          .to match_array(project.milestones.pluck(:id))
+      end
     end
 
     context "as json" do