Merge branch '51546-rails5-milestone-groups' into 'master'

Don't use MilestoneFinder for group milestones

Closes #51546

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

3 files changed, 27 insertions, 2 deletions

diff --git a/app/models/dashboard_group_milestone.rb b/app/models/dashboard_group_milestone.rb
index 067e14dda1c..32e8104125c 100644
--- a/app/models/dashboard_group_milestone.rb
+++ b/app/models/dashboard_group_milestone.rb
@@ -13,7 +13,11 @@ class DashboardGroupMilestone < GlobalMilestone
   end
 
   def self.build_collection(groups)
-    MilestonesFinder.new(group_ids: groups.select(:id)).execute.map { |m| new(m) } # rubocop: disable CodeReuse/Finder
+    Milestone.of_groups(groups.select(:id))
+             .reorder_by_due_date_asc
+             .order_by_name_asc
+             .active
+             .map { |m| new(m) }
   end
 
   override :group_milestone?
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index cb1def1b422..892a680f221 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -46,6 +46,9 @@ class Milestone < ActiveRecord::Base
     where(conditions.reduce(:or))
   end
 
+  scope :order_by_name_asc, -> { order(Arel::Nodes::Ascending.new(arel_table[:title].lower)) }
+  scope :reorder_by_due_date_asc, -> { reorder(Gitlab::Database.nulls_last_order('due_date', 'ASC')) }
+
   validates :group, presence: true, unless: :project
   validates :project, presence: true, unless: :group
 
@@ -149,7 +152,7 @@ class Milestone < ActiveRecord::Base
     sorted =
       case method.to_s
       when 'due_date_asc'
-        reorder(Gitlab::Database.nulls_last_order('due_date', 'ASC'))
+        reorder_by_due_date_asc
       when 'due_date_desc'
         reorder(Gitlab::Database.nulls_last_order('due_date', 'DESC'))
       when 'name_asc'
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 55b984faecf..27d4e622710 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -95,6 +95,24 @@ describe Milestone do
     end
   end
 
+  describe '.order_by_name_asc' do
+    it 'sorts by name ascending' do
+      milestone1 = create(:milestone, title: 'Foo')
+      milestone2 = create(:milestone, title: 'Bar')
+
+      expect(described_class.order_by_name_asc).to eq([milestone2, milestone1])
+    end
+  end
+
+  describe '.reorder_by_due_date_asc' do
+    it 'reorders the input relation' do
+      milestone1 = create(:milestone, due_date: Date.new(2018, 9, 30))
+      milestone2 = create(:milestone, due_date: Date.new(2018, 10, 20))
+
+      expect(described_class.reorder_by_due_date_asc).to eq([milestone1, milestone2])
+    end
+  end
+
   describe "#percent_complete" do
     it "does not count open issues" do
       milestone.issues << issue