Merge branch '199440-consider-approvals-for-review-time-part-3' into 'master'

Resolve "Code review start time doesn't account for approvals"

Closes #199440

See merge request gitlab-org/gitlab!25997

ee/app/models/ee/merge_request.rb

@@ -63,7 +63,8 @@ module EE
       accepts_nested_attributes_for :approval_rules, allow_destroy: true
 
       scope :order_review_time_desc, -> do
-        joins(:metrics).reorder(::Gitlab::Database.nulls_last_order('merge_request_metrics.first_comment_at'))
+        joins(:metrics)
+          .reorder(::Gitlab::Database.nulls_last_order(::MergeRequest::Metrics.review_time_field))
       end
 
       scope :with_code_review_api_entity_associations, -> do
@@ -97,7 +98,7 @@ module EE
       # Returns an array of arel columns
       def grouping_columns(sort)
         grouping_columns = super
-        grouping_columns << ::MergeRequest::Metrics.arel_table[:first_comment_at] if sort.to_s == 'review_time_desc'
+        grouping_columns << ::MergeRequest::Metrics.review_time_field if sort.to_s == 'review_time_desc'
         grouping_columns
       end
     end

ee/app/models/ee/merge_request/metrics.rb

@@ -3,6 +3,14 @@
 module EE
   module MergeRequest
     module Metrics
+      extend ActiveSupport::Concern
+
+      class_methods do
+        def review_time_field
+          @review_time_field ||= Arel.sql("LEAST(merge_request_metrics.first_comment_at, merge_request_metrics.first_approved_at)")
+        end
+      end
+
       def review_time
         return unless review_start_at
 
@@ -10,7 +18,7 @@ module EE
       end
 
       def review_start_at
-        first_comment_at
+        [first_comment_at, first_approved_at].compact.min
       end
 
       def review_end_at

ee/changelogs/unreleased/199440-consider-approvals-for-review-time-part-3.yml

+---
+title: Resolve Code review start time doesnt account for approvals
+merge_request: 25997
+author:
+type: changed

ee/lib/analytics/refresh_approvals_data.rb

@@ -4,7 +4,7 @@ module Analytics
   class RefreshApprovalsData
     include MergeRequestMetricsRefresh
 
-    # Change interface to accept single MR only
+    # Override MergeRequestMetricsRefresh#initialize` to accept single MR only
     def initialize(merge_request)
       super
     end

ee/spec/lib/analytics/merge_request_metrics_refresh_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Analytics::MergeRequestMetricsRefresh do
-  subject { calculator_class.new(merge_request_1) }
+  subject { calculator_class.new(merge_request) }
 
   let(:calculator_class) do
     Class.new do
@@ -23,24 +23,24 @@ describe Analytics::MergeRequestMetricsRefresh do
     end
   end
 
-  let!(:merge_request_1) { create(:merge_request) }
+  let!(:merge_request) { create(:merge_request) }
 
   describe '#execute' do
     it 'updates metric via update_metric! method' do
-      expect { subject.execute }.to change { merge_request_1.metrics.first_comment_at }.to(be_like_time(Time.now))
+      expect { subject.execute }.to change { merge_request.metrics.first_comment_at }.to(be_like_time(Time.now))
     end
 
     context 'when metric is already present' do
       before do
-        merge_request_1.metrics.first_comment_at = 1.day.ago
+        merge_request.metrics.first_comment_at = 1.day.ago
       end
 
       it 'does not update metric' do
-        expect { subject.execute }.not_to change { merge_request_1.metrics.first_comment_at }
+        expect { subject.execute }.not_to change { merge_request.metrics.first_comment_at }
       end
 
       it 'updates metric when forced' do
-        expect { subject.execute(force: true) }.to change { merge_request_1.metrics.first_comment_at }.to(be_like_time(Time.now))
+        expect { subject.execute(force: true) }.to change { merge_request.metrics.first_comment_at }.to(be_like_time(Time.now))
       end
     end
   end
@@ -49,7 +49,7 @@ describe Analytics::MergeRequestMetricsRefresh do
     it 'schedules CodeReviewMetricsWorker with params' do
       expect(Analytics::CodeReviewMetricsWorker)
         .to receive(:perform_async)
-              .with('MyTestClass', merge_request_1.id, force: true)
+              .with('MyTestClass', merge_request.id, force: true)
 
       subject.execute_async(force: true)
     end

ee/spec/models/ee/merge_request/metrics_spec.rb

@@ -4,10 +4,25 @@ require 'spec_helper'
 
 describe MergeRequest::Metrics do
   describe '#review_start_at' do
-    it 'is first_comment_at' do
+    it 'is the earliest date from first_comment_at or first_approved_at' do
+      subject.first_approved_at = 1.day.ago
       subject.first_comment_at = 1.hour.ago
 
-      expect(subject.review_start_at).to be_like_time(1.hour.ago)
+      expect(subject.review_start_at).to be_like_time(1.day.ago)
+    end
+
+    context 'when all review start candidates are nil' do
+      it 'is nil' do
+        expect(subject.review_start_at).to eq nil
+      end
+    end
+
+    context 'when one of review start candidates is nil' do
+      it 'is earliest date from non-nil values' do
+        subject.first_approved_at = 1.day.ago
+
+        expect(subject.review_start_at).to be_like_time(1.day.ago)
+      end
     end
   end
 

ee/spec/models/merge_request_spec.rb

@@ -743,13 +743,19 @@ describe MergeRequest do
   end
 
   describe 'review time sorting' do
-    it 'orders by first_comment_at' do
-      merge_request_1 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: 1.day.ago })
-      merge_request_2 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: 3.days.ago })
-      merge_request_3 = create(:merge_request, :with_productivity_metrics, metrics_data: { first_comment_at: nil })
+    def create_mr(metrics_data = {})
+      create(:merge_request, :with_productivity_metrics, metrics_data: metrics_data)
+    end
+
+    it 'orders by first_comment_at or first_approved_at whatever is earlier' do
+      mr1 = create_mr(first_comment_at: 1.day.ago)
+      mr2 = create_mr(first_comment_at: 3.days.ago)
+      mr3 = create_mr(first_approved_at: 5.days.ago)
+      mr4 = create_mr(first_comment_at: 1.day.ago, first_approved_at: 4.days.ago)
+      mr5 = create_mr(first_comment_at: nil, first_approved_at: nil)
 
-      expect(described_class.order_review_time_desc).to match([merge_request_2, merge_request_1, merge_request_3])
-      expect(described_class.sort_by_attribute('review_time_desc')).to match([merge_request_2, merge_request_1, merge_request_3])
+      expect(described_class.order_review_time_desc).to match([mr3, mr4, mr2, mr1, mr5])
+      expect(described_class.sort_by_attribute('review_time_desc')).to match([mr3, mr4, mr2, mr1, mr5])
     end
   end
 end