Make expired_last a sort option

expiredLast is more appropriate
as a sort option since it is a specialized
case of due date sorting.

app/finders/milestones_finder.rb

@@ -7,10 +7,6 @@
 #   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)
@@ -22,6 +18,8 @@ class MilestonesFinder
 
   attr_reader :params
 
+  EXPIRED_LAST_SORTS = %i[expired_last_due_date_asc expired_last_due_date_desc].freeze
+
   def initialize(params = {})
     @params = params
   end
@@ -74,12 +72,16 @@ class MilestonesFinder
   end
 
   def order(items)
-    sort_by = params[:sort].presence || 'due_date_asc'
+    sort_by = params[:sort].presence || :due_date_asc
 
-    if params[:expired_last]
+    if sort_by_expired_last?(sort_by)
       items.sort_with_expired_last(sort_by)
     else
       items.sort_by_attribute(sort_by)
     end
   end
+
+  def sort_by_expired_last?(sort_by)
+    EXPIRED_LAST_SORTS.include?(sort_by)
+  end
 end

app/graphql/resolvers/milestones_resolver.rb

@@ -30,12 +30,10 @@ module Resolvers
              required: false,
              default_value: :due_date_asc
 
-    argument :expired_last, GraphQL::BOOLEAN_TYPE,
-             description: 'Display non-expired milestones first when sorting milestones. In any sort the displayed order would be: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones. Sort order other than due date is ignored.',
-             required: false
-
     type Types::MilestoneType.connection_type, null: true
 
+    NON_STABLE_CURSOR_SORTS = %i[expired_last_due_date_asc expired_last_due_date_desc].freeze
+
     def resolve(**args)
       validate_timeframe_params!(args)
 
@@ -43,7 +41,7 @@ module Resolvers
 
       milestones = MilestonesFinder.new(milestones_finder_params(args)).execute
 
-      if args[:expired_last]
+      if non_stable_cursor_sort?(args[:sort])
         offset_pagination(milestones)
       else
         milestones
@@ -59,7 +57,6 @@ module Resolvers
         title: args[:title],
         search_title: args[:search_title],
         sort: args[:sort],
-        expired_last: args[:expired_last],
         containing_date: args[:containing_date]
       }.merge!(transform_timeframe_parameters(args)).merge!(parent_id_parameters(args))
     end
@@ -81,5 +78,9 @@ module Resolvers
     def parse_gids(gids)
       gids&.map { |gid| GitlabSchema.parse_gid(gid, expected_type: Milestone).model_id }
     end
+
+    def non_stable_cursor_sort?(sort)
+      NON_STABLE_CURSOR_SORTS.include?(sort)
+    end
   end
 end

app/graphql/types/milestone_sort_enum.rb

@@ -7,5 +7,7 @@ module Types
 
     value 'DUE_DATE_ASC', 'Milestone due date by ascending order.', value: :due_date_asc
     value 'DUE_DATE_DESC', 'Milestone due date by descending order.', value: :due_date_desc
+    value 'EXPIRED_LAST_DUE_DATE_ASC', 'Group milestones in this order: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones then sort by due date in ascending order.', value: :expired_last_due_date_asc
+    value 'EXPIRED_LAST_DUE_DATE_DESC', 'Group milestones in this order: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones then sort by due date in descending order.', value: :expired_last_due_date_desc
   end
 end

app/models/milestone.rb

@@ -124,7 +124,7 @@ class Milestone < ApplicationRecord
     # 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 = if sort_by == :expired_last_due_date_desc
                sorted.order(due_date: :desc)
              else
                sorted.order(due_date: :asc)

doc/api/graphql/reference/index.md

@@ -9564,7 +9564,6 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | ---- | ---- | ----------- |
 | <a id="groupmilestonescontainingdate"></a>`containingDate` | [`Time`](#time) | A date that the milestone contains. |
 | <a id="groupmilestonesenddate"></a>`endDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.end. |
-| <a id="groupmilestonesexpiredlast"></a>`expiredLast` | [`Boolean`](#boolean) | Display non-expired milestones first when sorting milestones. In any sort the displayed order would be: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones. Sort order other than due date is ignored. |
 | <a id="groupmilestonesids"></a>`ids` | [`[ID!]`](#id) | Array of global milestone IDs, e.g., `"gid://gitlab/Milestone/1"`. |
 | <a id="groupmilestonesincludeancestors"></a>`includeAncestors` | [`Boolean`](#boolean) | Include milestones from all parent groups. |
 | <a id="groupmilestonesincludedescendants"></a>`includeDescendants` | [`Boolean`](#boolean) | Include milestones from all subgroups and subprojects. |
@@ -11892,7 +11891,6 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | ---- | ---- | ----------- |
 | <a id="projectmilestonescontainingdate"></a>`containingDate` | [`Time`](#time) | A date that the milestone contains. |
 | <a id="projectmilestonesenddate"></a>`endDate` **{warning-solid}** | [`Time`](#time) | **Deprecated** in 13.5. Use timeframe.end. |
-| <a id="projectmilestonesexpiredlast"></a>`expiredLast` | [`Boolean`](#boolean) | Display non-expired milestones first when sorting milestones. In any sort the displayed order would be: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones. Sort order other than due date is ignored. |
 | <a id="projectmilestonesids"></a>`ids` | [`[ID!]`](#id) | Array of global milestone IDs, e.g., `"gid://gitlab/Milestone/1"`. |
 | <a id="projectmilestonesincludeancestors"></a>`includeAncestors` | [`Boolean`](#boolean) | Also return milestones in the project's parent group and its ancestors. |
 | <a id="projectmilestonessearchtitle"></a>`searchTitle` | [`String`](#string) | A search string for the title. |
@@ -14750,6 +14748,8 @@ Values for sorting milestones.
 | <a id="milestonesortcreated_desc"></a>`CREATED_DESC` | Created at descending order. |
 | <a id="milestonesortdue_date_asc"></a>`DUE_DATE_ASC` | Milestone due date by ascending order. |
 | <a id="milestonesortdue_date_desc"></a>`DUE_DATE_DESC` | Milestone due date by descending order. |
+| <a id="milestonesortexpired_last_due_date_asc"></a>`EXPIRED_LAST_DUE_DATE_ASC` | Group milestones in this order: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones then sort by due date in ascending order. |
+| <a id="milestonesortexpired_last_due_date_desc"></a>`EXPIRED_LAST_DUE_DATE_DESC` | Group milestones in this order: non-expired milestones with due dates, non-expired milestones without due dates and expired milestones then sort by due date in descending order. |
 | <a id="milestonesortupdated_asc"></a>`UPDATED_ASC` | Updated at ascending order. |
 | <a id="milestonesortupdated_desc"></a>`UPDATED_DESC` | Updated at descending order. |
 | <a id="milestonesortcreated_asc"></a>`created_asc` **{warning-solid}** | **Deprecated** in 13.5. This was renamed. Use: `CREATED_ASC`. |

spec/finders/milestones_finder_spec.rb

@@ -43,8 +43,8 @@ RSpec.describe MilestonesFinder do
         expect(result.third).to eq(milestone_2)
       end
 
-      context 'when expired_last param is used' do
-        let(:extra_params) { { expired_last: true } }
+      context 'when grouping and sorting by expired_last' do
+        let(:extra_params) { { sort: :expired_last_due_date_asc } }
 
         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])

spec/graphql/resolvers/group_milestones_resolver_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Resolvers::GroupMilestonesResolver do
+  using RSpec::Parameterized::TableSyntax
   include GraphqlHelpers
 
   describe '#resolve' do
@@ -87,23 +88,15 @@ RSpec.describe Resolvers::GroupMilestonesResolver do
 
         resolve_group_milestones(sort: :due_date_desc)
       end
-    end
-
-    context 'when expired_last is set' do
-      it 'calls MilestonesFinder with correct parameters' do
-        expect(MilestonesFinder).to receive(:new)
-          .with(args(group_ids: group.id, state: 'all', expired_last: true))
-          .and_call_original
-
-        resolve_group_milestones(expired_last: true)
-      end
 
-      it 'uses offset-pagination' do
-        resolved = resolve_group_milestones(expired_last: true)
+      %i[expired_last_due_date_asc expired_last_due_date_desc].each do |sort_by|
+        it "uses offset-pagination when sorting by #{sort_by}" do
+          resolved = resolve_group_milestones(sort: sort_by)
 
           expect(resolved).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
         end
       end
+    end
 
     context 'by timeframe' do
       context 'when start_date and end_date are present' do

spec/graphql/resolvers/project_milestones_resolver_spec.rb

@@ -79,15 +79,13 @@ RSpec.describe Resolvers::ProjectMilestonesResolver do
 
         resolve_project_milestones(sort: :due_date_desc)
       end
-    end
 
-    context 'when expired_last is set' do
-      it 'calls MilestonesFinder with correct parameters' do
-        expect(MilestonesFinder).to receive(:new)
-          .with(args(project_ids: project.id, state: 'all', expired_last: true))
-          .and_call_original
+      %i[expired_last_due_date_asc expired_last_due_date_desc].each do |sort_by|
+        it "uses offset-pagination when sorting by #{sort_by}" do
+          resolved = resolve_project_milestones(sort: sort_by)
 
-        resolve_project_milestones(expired_last: true)
+          expect(resolved).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+        end
       end
     end
 

spec/models/milestone_spec.rb

@@ -461,22 +461,15 @@ RSpec.describe Milestone do
     context 'ordering by due_date ascending' do
       it 'sorts by due date in ascending order (ties broken by id in desc order)', :aggregate_failures do
         expect(milestone_3.id).to be < (milestone_6.id)
-        expect(described_class.sort_with_expired_last('due_date_asc'))
+        expect(described_class.sort_with_expired_last(:expired_last_due_date_asc))
           .to eq([milestone_1, milestone_2, milestone_6, milestone_3, milestone_4, milestone_5])
       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)', :aggregate_failures do
         expect(milestone_3.id).to be < (milestone_6.id)
-        expect(described_class.sort_with_expired_last('due_date_desc'))
+        expect(described_class.sort_with_expired_last(:expired_last_due_date_desc))
           .to eq([milestone_2, milestone_1, milestone_6, milestone_3, milestone_5, milestone_4])
       end
     end