Apply reviewer feedback pt2

- Minor stylistics changes.
- Also use the MetricsFinder when totalTimeToMerge is requested

app/finders/merge_request/metrics_finder.rb

@@ -9,9 +9,9 @@ class MergeRequest::MetricsFinder
   end
 
   def execute
-    items = init_collection
-    return items.none if target_project_missing? || user_not_authorized?
+    return klass.none if target_project.blank? || user_not_authorized?
 
+    items = init_collection
     items = by_target_project(items)
     items = by_merged_after(items)
     items = by_merged_before(items)
@@ -28,19 +28,15 @@ class MergeRequest::MetricsFinder
   end
 
   def by_merged_after(items)
-    items = items.merged_after(params[:merged_after]) if params[:merged_after]
+    return items unless merged_after
 
-    items
+    items.merged_after(merged_after)
   end
 
   def by_merged_before(items)
-    items = items.merged_before(params[:merged_before]) if params[:merged_before]
-
-    items
-  end
+    return items unless merged_before
 
-  def target_project_missing?
-    params[:target_project].blank?
+    items.merged_before(merged_before)
   end
 
   def user_not_authorized?
@@ -58,4 +54,12 @@ class MergeRequest::MetricsFinder
   def target_project
     params[:target_project]
   end
+
+  def merged_after
+    params[:merged_after]
+  end
+
+  def merged_before
+    params[:merged_before]
+  end
 end

app/graphql/resolvers/project_merge_requests_resolver.rb

@@ -22,11 +22,7 @@ module Resolvers
     def only_count_is_selected_with_merged_at_filter?(args)
       return unless lookahead
 
-      argument_names = args.keys
-      argument_names.delete(:lookahead)
-      argument_names.delete(:sort)
-      argument_names.delete(:merged_before)
-      argument_names.delete(:merged_after)
+      argument_names = args.except(:lookahead, :sort, :merged_before, :merged_after).keys
 
       # no extra filtering arguments are provided
       return unless argument_names.empty?
@@ -35,8 +31,13 @@ module Resolvers
       # Detecting a specific query pattern:
       # mergeRequests(mergedAfter: "X", mergedBefore: "Y") {
       #   count
+      #   totalTimeToMerge
       # }
-      lookahead.selects?(:count) && lookahead.selections.size == 1 # no other nodes are selected
+      allowed_selected_fields = [:count, :total_time_to_merge]
+      selected_fields = lookahead.selections.map(&:field).map(&:original_name)
+
+      # only the allowed_selected_fields are present
+      (selected_fields - allowed_selected_fields).empty?
     end
   end
 end

app/models/merge_request/metrics.rb

@@ -23,6 +23,12 @@ class MergeRequest::Metrics < ApplicationRecord
   def ensure_target_project_id
     self.target_project_id ||= merge_request.target_project_id
   end
+
+  def self.total_time_to_merge
+    with_valid_time_to_merge
+      .pluck(time_to_merge_expression)
+      .first
+  end
 end
 
 MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics')

spec/finders/merge_request/metrics_finder_spec.rb

@@ -28,11 +28,11 @@ RSpec.describe MergeRequest::MetricsFinder do
       params.delete(:target_project)
     end
 
-    it { expect(subject).to be_empty }
+    it { is_expected.to be_empty }
   end
 
   context 'when the user is not part of the project' do
-    it { expect(subject).to be_empty }
+    it { is_expected.to be_empty }
   end
 
   context 'when user is part of the project' do
@@ -41,11 +41,11 @@ RSpec.describe MergeRequest::MetricsFinder do
     end
 
     it 'returns merge request records' do
-      expect(subject).to eq([merge_request_merged.metrics])
+      is_expected.to eq([merge_request_merged.metrics])
     end
 
     it 'excludes not merged records' do
-      expect(subject).not_to eq([merge_request_not_merged.metrics])
+      is_expected.not_to eq([merge_request_not_merged.metrics])
     end
 
     context 'when only merged_before is given' do
@@ -53,7 +53,7 @@ RSpec.describe MergeRequest::MetricsFinder do
         params.delete(:merged_after)
       end
 
-      it { expect(subject).to eq([merge_request_merged.metrics]) }
+      it { is_expected.to eq([merge_request_merged.metrics]) }
     end
 
     context 'when only merged_after is given' do
@@ -61,7 +61,7 @@ RSpec.describe MergeRequest::MetricsFinder do
         params.delete(:merged_before)
       end
 
-      it { expect(subject).to eq([merge_request_merged.metrics]) }
+      it { is_expected.to eq([merge_request_merged.metrics]) }
     end
 
     context 'when no records matching the date range' do
@@ -70,7 +70,7 @@ RSpec.describe MergeRequest::MetricsFinder do
         params[:merged_after] = merged_at - 2.years
       end
 
-      it { expect(subject).to be_empty }
+      it { is_expected.to be_empty }
     end
   end
 end

spec/models/merge_request/metrics_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe MergeRequest::Metrics do
   describe 'associations' do
     it { is_expected.to belong_to(:merge_request) }
-    it { is_expected.to belong_to(:target_project) }
+    it { is_expected.to belong_to(:target_project).class_name('Project') }
     it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
     it { is_expected.to belong_to(:merged_by).class_name('User') }
   end

spec/models/project_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to have_many(:services) }
     it { is_expected.to have_many(:events) }
     it { is_expected.to have_many(:merge_requests) }
-    it { is_expected.to have_many(:merge_request_metrics) }
+    it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') }
     it { is_expected.to have_many(:issues) }
     it { is_expected.to have_many(:milestones) }
     it { is_expected.to have_many(:iterations) }

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

@@ -415,6 +415,15 @@ RSpec.describe 'getting merge request listings nested in a project' do
         )
       end
 
+      shared_examples 'count examples' do
+        it 'returns the correct count' do
+          post_graphql(query, current_user: current_user)
+
+          count = graphql_data.dig('project', 'mergeRequests', 'count')
+          expect(count).to eq(1)
+        end
+      end
+
       context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is enabled' do
         before do
           stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: true)
@@ -428,19 +437,34 @@ RSpec.describe 'getting merge request listings nested in a project' do
           expect(queries).to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/))
         end
 
-        it 'returns the correct count' do
-          post_graphql(query, current_user: current_user)
+        context 'when total_time_to_merge and count is queried' do
+          let(:query) do
+            graphql_query_for(:project, { full_path: project.full_path },
+            <<~QUERY
+            mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) {
+              totalTimeToMerge
+              count
+            }
+            QUERY
+            )
+          end
 
-          count = graphql_data.dig('project', 'mergeRequests', 'count')
-          expect(count).to eq(1)
+          it 'does not query the merge requests table for the total_time_to_merge' do
+            query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
+
+            queries = query_recorder.data.each_value.first[:occurrences]
+            expect(queries).to include(match(/SELECT.+SUM.+FROM "merge_request_metrics" WHERE/))
+          end
         end
 
+        it_behaves_like 'count examples'
+
         context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is disabled' do
           before do
             stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: false)
           end
 
-          it 'does not query the merge requests table for the count' do
+          it 'queries the merge requests table for the count' do
             query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
 
             queries = query_recorder.data.each_value.first[:occurrences]
@@ -448,12 +472,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
             expect(queries).not_to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/))
           end
 
-          it 'returns the correct count' do
-            post_graphql(query, current_user: current_user)
-
-            count = graphql_data.dig('project', 'mergeRequests', 'count')
-            expect(count).to eq(1)
-          end
+          it_behaves_like 'count examples'
         end
       end
     end