Merge branch '18458-allow-sorting-mrs-by-merged-and-closed-dates' into 'master'

Resolve "Allow sorting MRs by merged and closed dates"

See merge request gitlab-org/gitlab!67041

app/graphql/types/merge_request_sort_enum.rb

@@ -7,5 +7,7 @@ module Types
 
     value 'MERGED_AT_ASC', 'Merge time by ascending order.', value: :merged_at_asc
     value 'MERGED_AT_DESC', 'Merge time by descending order.', value: :merged_at_desc
+    value 'CLOSED_AT_ASC', 'Closed time by ascending order.', value: :closed_at_asc
+    value 'CLOSED_AT_DESC', 'Closed time by descending order.', value: :closed_at_desc
   end
 end

app/helpers/sorting_helper.rb

@@ -3,6 +3,7 @@
 module SortingHelper
   include SortingTitlesValuesHelper
 
+  # rubocop: disable Metrics/AbcSize
   def sort_options_hash
     {
       sort_value_created_date      => sort_title_created_date,
@@ -29,6 +30,9 @@ module SortingHelper
       sort_value_merged_date       => sort_title_merged_date,
       sort_value_merged_recently   => sort_title_merged_recently,
       sort_value_merged_earlier    => sort_title_merged_earlier,
+      sort_value_closed_date       => sort_title_closed_date,
+      sort_value_closed_recently   => sort_title_closed_recently,
+      sort_value_closed_earlier    => sort_title_closed_earlier,
       sort_value_upvotes           => sort_title_upvotes,
       sort_value_contacted_date    => sort_title_contacted_date,
       sort_value_relative_position => sort_title_relative_position,
@@ -36,6 +40,7 @@ module SortingHelper
       sort_value_expire_date       => sort_title_expire_date
     }
   end
+  # rubocop: enable Metrics/AbcSize
 
   def projects_sort_options_hash
     use_old_sorting = Feature.disabled?(:project_list_filter_bar) || current_controller?('admin/projects')
@@ -182,6 +187,7 @@ module SortingHelper
       sort_value_milestone_later => sort_value_milestone,
       sort_value_due_date_later => sort_value_due_date,
       sort_value_merged_recently => sort_value_merged_date,
+      sort_value_closed_recently => sort_value_closed_date,
       sort_value_least_popular => sort_value_popularity
     }
   end
@@ -196,6 +202,8 @@ module SortingHelper
       sort_value_due_date_soon => sort_value_due_date_later,
       sort_value_merged_date => sort_value_merged_recently,
       sort_value_merged_earlier => sort_value_merged_recently,
+      sort_value_closed_date => sort_value_closed_recently,
+      sort_value_closed_earlier => sort_value_closed_recently,
       sort_value_popularity => sort_value_least_popular,
       sort_value_most_popular => sort_value_least_popular
     }.merge(issuable_sort_option_overrides)
@@ -216,7 +224,7 @@ module SortingHelper
 
   def sort_direction_icon(sort_value)
     case sort_value
-    when sort_value_milestone, sort_value_due_date, sort_value_merged_date, /_asc\z/
+    when sort_value_milestone, sort_value_due_date, sort_value_merged_date, sort_value_closed_date, /_asc\z/
       'sort-lowest'
     else
       'sort-highest'

app/helpers/sorting_titles_values_helper.rb

@@ -38,6 +38,18 @@ module SortingTitlesValuesHelper
     s_('SortOptions|Merged earlier')
   end
 
+  def sort_title_closed_date
+    s_('SortOptions|Closed date')
+  end
+
+  def sort_title_closed_recently
+    s_('SortOptions|Closed recently')
+  end
+
+  def sort_title_closed_earlier
+    s_('SortOptions|Closed earlier')
+  end
+
   def sort_title_largest_group
     s_('SortOptions|Largest group')
   end
@@ -199,6 +211,18 @@ module SortingTitlesValuesHelper
     'merged_at_asc'
   end
 
+  def sort_value_closed_date
+    'closed_at'
+  end
+
+  def sort_value_closed_recently
+    'closed_at_desc'
+  end
+
+  def sort_value_closed_earlier
+    'closed_at_asc'
+  end
+
   def sort_value_largest_group
     'storage_size_desc'
   end

app/models/concerns/issuable.rb

@@ -374,6 +374,8 @@ module Issuable
         grouping_columns << milestone_table[:due_date]
       elsif %w(merged_at_desc merged_at_asc).include?(sort)
         grouping_columns << MergeRequest::Metrics.arel_table[:merged_at]
+      elsif %w(closed_at_desc closed_at_asc).include?(sort)
+        grouping_columns << MergeRequest::Metrics.arel_table[:closed_at]
       end
 
       grouping_columns

app/models/merge_request.rb

@@ -329,16 +329,16 @@ class MergeRequest < ApplicationRecord
     where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%'))
   end
   scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) }
-  scope :order_merged_at, ->(direction) do
+  scope :order_by_metric, ->(metric, direction) do
     reverse_direction = { 'ASC' => 'DESC', 'DESC' => 'ASC' }
     reversed_direction = reverse_direction[direction] || raise("Unknown sort direction was given: #{direction}")
 
     order = Gitlab::Pagination::Keyset::Order.build([
       Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-        attribute_name: 'merge_request_metrics_merged_at',
-        column_expression: MergeRequest::Metrics.arel_table[:merged_at],
-        order_expression: Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', direction),
-        reversed_order_expression: Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', reversed_direction),
+        attribute_name: "merge_request_metrics_#{metric}",
+        column_expression: MergeRequest::Metrics.arel_table[metric],
+        order_expression: Gitlab::Database.nulls_last_order("merge_request_metrics.#{metric}", direction),
+        reversed_order_expression: Gitlab::Database.nulls_first_order("merge_request_metrics.#{metric}", reversed_direction),
         order_direction: direction,
         nullable: :nulls_last,
         distinct: false,
@@ -353,8 +353,10 @@ class MergeRequest < ApplicationRecord
 
     order.apply_cursor_conditions(join_metrics).order(order)
   end
-  scope :order_merged_at_asc, -> { order_merged_at('ASC') }
-  scope :order_merged_at_desc, -> { order_merged_at('DESC') }
+  scope :order_merged_at_asc, -> { order_by_metric(:merged_at, 'ASC') }
+  scope :order_merged_at_desc, -> { order_by_metric(:merged_at, 'DESC') }
+  scope :order_closed_at_asc, -> { order_by_metric(:latest_closed_at, 'ASC') }
+  scope :order_closed_at_desc, -> { order_by_metric(:latest_closed_at, 'DESC') }
   scope :preload_source_project, -> { preload(:source_project) }
   scope :preload_target_project, -> { preload(:target_project) }
   scope :preload_routables, -> do
@@ -452,7 +454,9 @@ class MergeRequest < ApplicationRecord
   def self.sort_by_attribute(method, excluded_labels: [])
     case method.to_s
     when 'merged_at', 'merged_at_asc' then order_merged_at_asc
+    when 'closed_at', 'closed_at_asc' then order_closed_at_asc
     when 'merged_at_desc' then order_merged_at_desc
+    when 'closed_at_desc' then order_closed_at_desc
     else
       super
     end

app/views/shared/issuable/_sort_dropdown.html.haml

@@ -19,6 +19,7 @@
           = sortable_item(sort_title_popularity,        page_filter_path(sort: sort_value_popularity),        sort_title)
           = sortable_item(sort_title_label_priority,    page_filter_path(sort: sort_value_label_priority),    sort_title)
           = sortable_item(sort_title_merged_date,       page_filter_path(sort: sort_value_merged_date),       sort_title) if viewing_merge_requests
+          = sortable_item(sort_title_closed_date,       page_filter_path(sort: sort_value_closed_date),       sort_title) if viewing_merge_requests
           = sortable_item(sort_title_relative_position, page_filter_path(sort: sort_value_relative_position), sort_title) if viewing_issues
           = render_if_exists('shared/ee/issuable/sort_dropdown', viewing_issues: viewing_issues, sort_title: sort_title)
     = issuable_sort_direction_button(sort_value)

doc/api/graphql/reference/index.md

@@ -15251,6 +15251,8 @@ Values for sorting merge requests.
 
 | Value | Description |
 | ----- | ----------- |
+| <a id="mergerequestsortclosed_at_asc"></a>`CLOSED_AT_ASC` | Closed time by ascending order. |
+| <a id="mergerequestsortclosed_at_desc"></a>`CLOSED_AT_DESC` | Closed time by descending order. |
 | <a id="mergerequestsortcreated_asc"></a>`CREATED_ASC` | Created at ascending order. |
 | <a id="mergerequestsortcreated_desc"></a>`CREATED_DESC` | Created at descending order. |
 | <a id="mergerequestsortlabel_priority_asc"></a>`LABEL_PRIORITY_ASC` | Label priority by ascending order. |

locale/gitlab.pot

@@ -31228,6 +31228,15 @@ msgstr ""
 msgid "SortOptions|Blocking"
 msgstr ""
 
+msgid "SortOptions|Closed date"
+msgstr ""
+
+msgid "SortOptions|Closed earlier"
+msgstr ""
+
+msgid "SortOptions|Closed recently"
+msgstr ""
+
 msgid "SortOptions|Created date"
 msgstr ""
 

spec/features/merge_requests/user_lists_merge_requests_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe 'Merge requests > User lists merge requests' do
                   milestone: create(:milestone, project: project, due_date: '2013-12-11'),
                   created_at: 1.minute.ago,
                   updated_at: 1.minute.ago)
-    @fix.metrics.update_column(:merged_at, 10.seconds.ago)
+    @fix.metrics.update!(merged_at: 10.seconds.ago, latest_closed_at: 10.seconds.ago)
 
     @markdown = create(:merge_request,
            title: 'markdown',
@@ -34,7 +34,7 @@ RSpec.describe 'Merge requests > User lists merge requests' do
            milestone: create(:milestone, project: project, due_date: '2013-12-12'),
            created_at: 2.minutes.ago,
            updated_at: 2.minutes.ago)
-    @markdown.metrics.update_column(:merged_at, 50.seconds.ago)
+    @markdown.metrics.update!(merged_at: 10.minutes.ago, latest_closed_at: 10.seconds.ago)
 
     @merge_test = create(:merge_request,
            title: 'merge-test',
@@ -42,7 +42,15 @@ RSpec.describe 'Merge requests > User lists merge requests' do
            source_branch: 'merge-test',
            created_at: 3.minutes.ago,
            updated_at: 10.seconds.ago)
-    @merge_test.metrics.update_column(:merged_at, 10.seconds.ago)
+    @merge_test.metrics.update!(merged_at: 10.seconds.ago, latest_closed_at: 10.seconds.ago)
+
+    @feature = create(:merge_request,
+           title: 'feature',
+           source_project: project,
+           source_branch: 'feautre',
+           created_at: 2.minutes.ago,
+           updated_at: 1.minute.ago)
+    @feature.metrics.update!(merged_at: 10.seconds.ago, latest_closed_at: 10.minutes.ago)
   end
 
   context 'merge request reviewers' do
@@ -71,9 +79,10 @@ RSpec.describe 'Merge requests > User lists merge requests' do
 
     expect(current_path).to eq(project_merge_requests_path(project))
     expect(page).to have_content 'merge-test'
+    expect(page).to have_content 'feature'
     expect(page).not_to have_content 'fix'
     expect(page).not_to have_content 'markdown'
-    expect(count_merge_requests).to eq(1)
+    expect(count_merge_requests).to eq(2)
   end
 
   it 'filters on a specific assignee' do
@@ -90,28 +99,35 @@ RSpec.describe 'Merge requests > User lists merge requests' do
 
     expect(first_merge_request).to include('fix')
     expect(last_merge_request).to include('merge-test')
-    expect(count_merge_requests).to eq(3)
+    expect(count_merge_requests).to eq(4)
   end
 
   it 'sorts by last updated' do
     visit_merge_requests(project, sort: sort_value_recently_updated)
 
     expect(first_merge_request).to include('merge-test')
-    expect(count_merge_requests).to eq(3)
+    expect(count_merge_requests).to eq(4)
   end
 
   it 'sorts by milestone' do
     visit_merge_requests(project, sort: sort_value_milestone)
 
     expect(first_merge_request).to include('fix')
-    expect(count_merge_requests).to eq(3)
+    expect(count_merge_requests).to eq(4)
   end
 
   it 'sorts by merged at' do
     visit_merge_requests(project, sort: sort_value_merged_date)
 
     expect(first_merge_request).to include('markdown')
-    expect(count_merge_requests).to eq(3)
+    expect(count_merge_requests).to eq(4)
+  end
+
+  it 'sorts by closed at' do
+    visit_merge_requests(project, sort: sort_value_closed_date)
+
+    expect(first_merge_request).to include('feature')
+    expect(count_merge_requests).to eq(4)
   end
 
   it 'filters on one label and sorts by due date' do

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -303,6 +303,29 @@ RSpec.describe Resolvers::MergeRequestsResolver do
             expect { resolve_mr(project, sort: :merged_at_desc, labels: %w[a b]) }.not_to raise_error
           end
         end
+
+        context 'when sorting by closed at' do
+          before do
+            merge_request_1.metrics.update!(latest_closed_at: 10.days.ago)
+            merge_request_3.metrics.update!(latest_closed_at: 5.days.ago)
+          end
+
+          it 'sorts merge requests ascending' do
+            expect(resolve_mr(project, sort: :closed_at_asc))
+              .to match_array(mrs)
+              .and be_sorted(->(mr) { [closed_at(mr), -mr.id] })
+          end
+
+          it 'sorts merge requests descending' do
+            expect(resolve_mr(project, sort: :closed_at_desc))
+              .to match_array(mrs)
+              .and be_sorted(->(mr) { [-closed_at(mr), -mr.id] })
+          end
+
+          def closed_at(mr)
+            nils_last(mr.metrics.latest_closed_at)
+          end
+        end
       end
     end
   end

spec/models/merge_request_spec.rb

@@ -80,6 +80,24 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
+  describe '.order_closed_at_asc' do
+    let_it_be(:older_mr) { create(:merge_request, :closed_last_month) }
+    let_it_be(:newer_mr) { create(:merge_request, :closed_last_month) }
+
+    it 'returns MRs ordered by closed_at ascending' do
+      expect(described_class.order_closed_at_asc).to eq([older_mr, newer_mr])
+    end
+  end
+
+  describe '.order_closed_at_desc' do
+    let_it_be(:older_mr) { create(:merge_request, :closed_last_month) }
+    let_it_be(:newer_mr) { create(:merge_request, :closed_last_month) }
+
+    it 'returns MRs ordered by closed_at descending' do
+      expect(described_class.order_closed_at_desc).to eq([newer_mr, older_mr])
+    end
+  end
+
   describe '.with_jira_issue_keys' do
     let_it_be(:mr_with_jira_title) { create(:merge_request, :unique_branches, title: 'Fix TEST-123') }
     let_it_be(:mr_with_jira_description) { create(:merge_request, :unique_branches, description: 'this closes TEST-321') }
@@ -577,6 +595,26 @@ RSpec.describe MergeRequest, factory_default: :keep do
         expect(merge_requests).to eq([newer_mr, older_mr])
       end
     end
+
+    context 'closed_at' do
+      let_it_be(:older_mr) { create(:merge_request, :closed_last_month) }
+      let_it_be(:newer_mr) { create(:merge_request, :closed_last_month) }
+
+      it 'sorts asc' do
+        merge_requests = described_class.sort_by_attribute(:closed_at_asc)
+        expect(merge_requests).to eq([older_mr, newer_mr])
+      end
+
+      it 'sorts desc' do
+        merge_requests = described_class.sort_by_attribute(:closed_at_desc)
+        expect(merge_requests).to eq([newer_mr, older_mr])
+      end
+
+      it 'sorts asc when its closed_at' do
+        merge_requests = described_class.sort_by_attribute(:closed_at)
+        expect(merge_requests).to eq([older_mr, newer_mr])
+      end
+    end
   end
 
   describe 'time to merge calculations' do

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -422,6 +422,46 @@ RSpec.describe 'getting merge request listings nested in a project' do
         end
       end
     end
+
+    context 'when sorting by closed_at DESC' do
+      let(:sort_param) { :CLOSED_AT_DESC }
+      let(:expected_results) do
+        [
+          merge_request_b,
+          merge_request_d,
+          merge_request_c,
+          merge_request_e,
+          merge_request_a
+        ].map { |mr| global_id_of(mr) }
+      end
+
+      before do
+        five_days_ago = 5.days.ago
+
+        merge_request_d.metrics.update!(latest_closed_at: five_days_ago)
+
+        # same merged_at, the second order column will decide (merge_request.id)
+        merge_request_c.metrics.update!(latest_closed_at: five_days_ago)
+
+        merge_request_b.metrics.update!(latest_closed_at: 1.day.ago)
+      end
+
+      it_behaves_like 'sorted paginated query' do
+        let(:first_param) { 2 }
+      end
+
+      context 'when last parameter is given' do
+        let(:params) { graphql_args(sort: sort_param, last: 2) }
+        let(:page_info) { nil }
+
+        it 'takes the last 2 records' do
+          query = pagination_query(params)
+          post_graphql(query, current_user: current_user)
+
+          expect(results.map { |item| item["id"] }).to eq(expected_results.last(2))
+        end
+      end
+    end
   end
 
   context 'when only the count is requested' do