Add a custom ordering option for milestones

A new method is added to the milestone model
to support fetching milestones in this particular order:
1. Current milestones (due date > current date)
2. Milestones without due dates
3. Milestones that are expired (due date <= current_date)

The milestones are then sorted by due date (asc or desc)
and ties are broken by id in descending order.

The milestone finder is also updated to accept
a new Boolean parameter 'expired_last' to utilize the
newly added custom ordering to fetch milestones.

When 'expired_last' is specified, order param must be
one of 'due_date_asc' or 'due_date_desc'. For non-supported
order param values (including when it's not provided),
the default is 'due_date_asc'.

app/finders/milestones_finder.rb

@@ -7,6 +7,10 @@
 #   project_ids: Array of project ids or single project id or ActiveRecord relation.
 #   group_ids: Array of group ids or single group id or ActiveRecord relation.
 #   order - Orders by field default due date asc.
+#   expired_last - Custom orders milestones by first listing milestones with due dates
+#                  then those without due dates followed by expired milestones.
+#                  Milestones are ordered by due date.
+#                  'order' param is ignored when it isn't either due_date_asc or due_date_desc
 #   title - filter by title.
 #   state - filters by state.
 #   start_date & end_date - filters by timeframe (see TimeFrameFilter)
@@ -71,6 +75,11 @@ class MilestonesFinder
 
   def order(items)
     sort_by = params[:sort].presence || 'due_date_asc'
+
+    if params[:expired_last]
+      items.sort_with_expired_last(sort_by)
+    else
       items.sort_by_attribute(sort_by)
     end
+  end
 end

app/models/concerns/milestoneish.rb

@@ -101,6 +101,10 @@ module Milestoneish
     due_date && due_date.past?
   end
 
+  def expired
+    expired? || false
+  end
+
   def total_time_spent
     @total_time_spent ||= issues.joins(:timelogs).sum(:time_spent) + merge_requests.joins(:timelogs).sum(:time_spent)
   end

app/models/milestone.rb

@@ -120,6 +120,19 @@ class Milestone < ApplicationRecord
     sorted.with_order_id_desc
   end
 
+  def self.sort_with_expired_last(sort_by)
+    # NOTE: this is a custom ordering of milestones
+    # to prioritize displaying non-expired milestones and milestones without due dates
+    sorted = reorder(Arel.sql('(CASE WHEN due_date IS NULL THEN 1 WHEN due_date > now() THEN 0 ELSE 2 END) ASC'))
+    sorted = if sort_by == 'due_date_desc'
+               sorted.order(due_date: :desc)
+             else
+               sorted.order(due_date: :asc)
+             end
+
+    sorted.with_order_id_desc
+  end
+
   def self.states_count(projects, groups = nil)
     return STATE_COUNT_HASH unless projects || groups
 

spec/finders/milestones_finder_spec.rb

@@ -11,8 +11,9 @@ RSpec.describe MilestonesFinder do
   context 'without filters' do
     let_it_be(:milestone_1) { create(:milestone, group: group, title: 'one test', start_date: now - 1.day, due_date: now) }
     let_it_be(:milestone_2) { create(:milestone, group: group, start_date: now + 1.day, due_date: now + 2.days) }
-    let_it_be(:milestone_3) { create(:milestone, project: project_1, state: 'active', start_date: now + 2.days, due_date: now + 3.days) }
+    let_it_be(:milestone_3) { create(:milestone, project: project_1, state: 'active', start_date: now + 2.days) }
     let_it_be(:milestone_4) { create(:milestone, project: project_2, state: 'active', start_date: now + 4.days, due_date: now + 5.days) }
+    let_it_be(:milestone_5) { create(:milestone, group: group, due_date: now - 2.days) }
 
     it 'returns milestones for projects' do
       result = described_class.new(project_ids: [project_1.id, project_2.id], state: 'all').execute
@@ -23,27 +24,36 @@ RSpec.describe MilestonesFinder do
     it 'returns milestones for groups' do
       result = described_class.new(group_ids: group.id,  state: 'all').execute
 
-      expect(result).to contain_exactly(milestone_1, milestone_2)
+      expect(result).to contain_exactly(milestone_5, milestone_1, milestone_2)
     end
 
     context 'milestones for groups and project' do
+      let(:extra_params) {{}}
       let(:result) do
-        described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute
+        described_class.new({ project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all' }.merge(extra_params)).execute
       end
 
       it 'returns milestones for groups and projects' do
-        expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4)
+        expect(result).to contain_exactly(milestone_5, milestone_1, milestone_2, milestone_3, milestone_4)
       end
 
       it 'orders milestones by due date' do
-        milestone = create(:milestone, group: group, due_date: now - 2.days)
-
-        expect(result.first).to eq(milestone)
+        aggregate_failures do
+          expect(result.first).to eq(milestone_5)
           expect(result.second).to eq(milestone_1)
           expect(result.third).to eq(milestone_2)
         end
       end
 
+      context 'when expired_last param is used' do
+        let(:extra_params) { { expired_last: true } }
+
+        it 'current milestones are returned first, then milestones without due date followed by expired milestones, sorted by due date in ascending order' do
+          expect(result).to eq([milestone_2, milestone_4, milestone_3, milestone_5, milestone_1])
+        end
+      end
+    end
+
     describe '#find_by' do
       it 'finds a single milestone' do
         finder = described_class.new(project_ids: [project_1.id], state: 'all')

spec/models/milestone_spec.rb

@@ -148,7 +148,7 @@ RSpec.describe Milestone do
     end
   end
 
-  describe '#expired?' do
+  describe '#expired? and #expired' do
     let_it_be(:milestone) { build(:milestone, project: project) }
 
     context "expired" do
@@ -157,7 +157,10 @@ RSpec.describe Milestone do
       end
 
       it 'returns true when due_date is in the past' do
+        aggregate_failures do
           expect(milestone.expired?).to be_truthy
+          expect(milestone.expired).to eq true
+        end
       end
     end
 
@@ -167,7 +170,10 @@ RSpec.describe Milestone do
       end
 
       it 'returns false when due_date is in the future' do
+        aggregate_failures do
           expect(milestone.expired?).to be_falsey
+          expect(milestone.expired).to eq false
+        end
       end
     end
   end
@@ -460,6 +466,42 @@ RSpec.describe Milestone do
     end
   end
 
+  describe '.sort_with_expired_last' do
+    let_it_be(:milestone_1) { create(:milestone, title: 'Current 1',  due_date: Time.current + 1.day) }
+    let_it_be(:milestone_2) { create(:milestone, title: 'Current 2',  due_date: Time.current + 2.days) }
+    let_it_be(:milestone_3) { create(:milestone, title: 'Without due date') }
+    let_it_be(:milestone_4) { create(:milestone, title: 'Expired 1',  due_date: Time.current - 2.days) }
+    let_it_be(:milestone_5) { create(:milestone, title: 'Expired 2',  due_date: Time.current - 1.day) }
+    let_it_be(:milestone_6) { create(:milestone, title: 'Without due date2') }
+
+    context 'ordering by due_date ascending' do
+      it 'sorts by due date in ascending order (ties broken by id in desc order)' do
+        aggregate_failures do
+          expect(milestone_3.id).to be < (milestone_6.id)
+          expect(described_class.sort_with_expired_last('due_date_asc'))
+            .to eq([milestone_1, milestone_2, milestone_6, milestone_3, milestone_4, milestone_5])
+        end
+      end
+
+      context 'unsupported sort_by param is presented' do
+        it 'sorts by due date in ascending order by default' do
+          expect(described_class.sort_with_expired_last('name_asc'))
+            .to eq([milestone_1, milestone_2, milestone_6, milestone_3, milestone_4, milestone_5])
+        end
+      end
+    end
+
+    context 'ordering by due_date descending' do
+      it 'sorts by due date in descending order (ties broken by id in desc order)' do
+        aggregate_failures do
+          expect(milestone_3.id).to be < (milestone_6.id)
+          expect(described_class.sort_with_expired_last('due_date_desc'))
+            .to eq([milestone_2, milestone_1, milestone_6, milestone_3, milestone_5, milestone_4])
+        end
+      end
+    end
+  end
+
   describe '.sort_by_attribute' do
     let_it_be(:milestone_1) { create(:milestone, title: 'Foo') }
     let_it_be(:milestone_2) { create(:milestone, title: 'Bar') }