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

Code Review analytics refactoring

See merge request gitlab-org/gitlab!25811

app/models/merge_request/metrics.rb

@@ -6,3 +6,5 @@ class MergeRequest::Metrics < ApplicationRecord
   belongs_to :latest_closed_by, class_name: 'User'
   belongs_to :merged_by, class_name: 'User'
 end
+
+MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics')

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

+# frozen_string_literal: true
+
+module EE
+  module MergeRequest
+    module Metrics
+      def review_time
+        return unless review_start_at
+
+        review_end_at - review_start_at
+      end
+
+      def review_start_at
+        first_comment_at
+      end
+
+      def review_end_at
+        merged_at || Time.now
+      end
+    end
+  end
+end

ee/app/services/ee/merge_request_metrics_service.rb

@@ -11,15 +11,15 @@ module EE
       data = {
         merged_by_id: event.author_id,
         merged_at: event.created_at
-      }.merge(productivity_calculator.productivity_data)
+      }.merge(metrics_calculator.productivity_data)
 
       update!(data)
     end
 
     private
 
-    def productivity_calculator
-      @productivity_calculator ||= Analytics::ProductivityCalculator.new(merge_request)
+    def metrics_calculator
+      @metrics_calculator ||= Analytics::MergeRequestMetricsCalculator.new(merge_request)
     end
   end
 end

ee/app/services/merge_requests/approval_service.rb

@@ -51,7 +51,7 @@ module MergeRequests
     def calculate_approvals_metrics(merge_request)
       return unless merge_request.project.feature_available?(:code_review_analytics)
 
-      Analytics::CodeReviewMetricsWorker.perform_async('::Analytics::RefreshApprovalsData', merge_request.id)
+      Analytics::RefreshApprovalsData.new(merge_request).execute_async
     end
   end
 end

ee/app/services/merge_requests/remove_approval_service.rb

@@ -35,7 +35,7 @@ module MergeRequests
     def recalculate_approvals_metrics(merge_request)
       return unless merge_request.project.feature_available?(:code_review_analytics)
 
-      Analytics::CodeReviewMetricsWorker.perform_async('::Analytics::RefreshApprovalsData', merge_request.id, force: true)
+      Analytics::RefreshApprovalsData.new(merge_request).execute_async(force: true)
     end
   end
 end

ee/lib/analytics/productivity_calculator.rb → ee/lib/analytics/merge_request_metrics_calculator.rb

 # frozen_string_literal: true
 
 module Analytics
-  class ProductivityCalculator
+  class MergeRequestMetricsCalculator
     def initialize(merge_request)
       @merge_request = merge_request
     end

ee/lib/analytics/merge_request_metrics_refresh.rb

+# frozen_string_literal: true
+
+module Analytics
+  module MergeRequestMetricsRefresh
+    def initialize(*merge_requests)
+      @merge_requests = merge_requests
+    end
+
+    def execute(force: false)
+      merge_requests.each do |mr|
+        metrics = mr.ensure_metrics
+
+        next if !force && metric_already_present?(metrics)
+
+        update_metric!(metrics)
+      end
+    end
+
+    def execute_async(*args)
+      merge_requests.each do |mr|
+        CodeReviewMetricsWorker.perform_async(self.class.name, mr.id, *args)
+      end
+    end
+
+    private
+
+    attr_reader :merge_requests
+
+    def metric_already_present?(metrics)
+      raise NotImplementedError
+    end
+
+    def update_metric!(metrics)
+      raise NotImplementedError
+    end
+  end
+end

ee/lib/analytics/refresh_approvals_data.rb

@@ -2,20 +2,23 @@
 
 module Analytics
   class RefreshApprovalsData
+    include MergeRequestMetricsRefresh
+
+    # Change interface to accept single MR only
     def initialize(merge_request)
-      @merge_request = merge_request
+      super
     end
 
-    def execute(force: false)
-      metrics = merge_request.ensure_metrics
-
-      return if !force && metrics.first_approved_at
+    private
 
-      metrics.update!(first_approved_at: ProductivityCalculator.new(merge_request).first_approved_at)
+    def metric_already_present?(metrics)
+      metrics.first_approved_at
     end
 
-    private
-
-    attr_reader :merge_request
+    def update_metric!(metrics)
+      metrics.update!(
+        first_approved_at: MergeRequestMetricsCalculator.new(metrics.merge_request).first_approved_at
+      )
+    end
   end
 end

ee/lib/analytics/refresh_comments_data.rb

@@ -2,6 +2,8 @@
 
 module Analytics
   class RefreshCommentsData
+    include MergeRequestMetricsRefresh
+
     # rubocop: disable CodeReuse/ActiveRecord
     def self.for_note(note)
       if note.for_commit?
@@ -12,26 +14,20 @@ module Analytics
         return
       end
 
-      new(merge_requests)
+      new(*merge_requests)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def initialize(merge_requests)
-      @merge_requests = merge_requests
-    end
-
-    def execute(force: false)
-      merge_requests.each do |mr|
-        metrics = mr.ensure_metrics
-
-        next if !force && metrics.first_comment_at
+    private
 
-        metrics.update!(first_comment_at: ProductivityCalculator.new(mr).first_comment_at)
+    def metric_already_present?(metrics)
+      metrics.first_comment_at
     end
-    end
-
-    private
 
-    attr_reader :merge_requests
+    def update_metric!(metrics)
+      metrics.update!(
+        first_comment_at: MergeRequestMetricsCalculator.new(metrics.merge_request).first_comment_at
+      )
+    end
   end
 end

ee/lib/ee/api/entities.rb

@@ -930,11 +930,11 @@ module EE
               end
             end
             expose :review_time do |mr|
-              next unless mr.metrics.first_comment_at
+              time = mr.metrics.review_time
 
-              review_time = (mr.metrics.merged_at || Time.now) - mr.metrics.first_comment_at
+              next unless time
 
-              (review_time / ActiveSupport::Duration::SECONDS_PER_HOUR).floor
+              (time / ActiveSupport::Duration::SECONDS_PER_HOUR).floor
             end
             expose :diff_stats
 

ee/spec/lib/analytics/productivity_calculator_spec.rb → ee/spec/lib/analytics/merge_request_metrics_calculator_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Analytics::ProductivityCalculator do
+describe Analytics::MergeRequestMetricsCalculator do
   subject { described_class.new(merge_request) }
 
   let_it_be(:merge_request) { create(:merge_request, :merged, :with_diffs, created_at: 31.days.ago) }

ee/spec/lib/analytics/merge_request_metrics_refresh_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::MergeRequestMetricsRefresh do
+  subject { calculator_class.new(merge_request_1) }
+
+  let(:calculator_class) do
+    Class.new do
+      include Analytics::MergeRequestMetricsRefresh
+
+      def self.name
+        'MyTestClass'
+      end
+
+      def metric_already_present?(metrics)
+        metrics.first_comment_at
+      end
+
+      def update_metric!(metrics)
+        metrics.first_comment_at = Time.now
+      end
+    end
+  end
+
+  let!(:merge_request_1) { 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))
+    end
+
+    context 'when metric is already present' do
+      before do
+        merge_request_1.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 }
+      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))
+      end
+    end
+  end
+
+  describe '#execute_async' do
+    it 'schedules CodeReviewMetricsWorker with params' do
+      expect(Analytics::CodeReviewMetricsWorker)
+        .to receive(:perform_async)
+              .with('MyTestClass', merge_request_1.id, force: true)
+
+      subject.execute_async(force: true)
+    end
+  end
+end

ee/spec/lib/analytics/refresh_approvals_data_spec.rb

@@ -48,4 +48,13 @@ describe Analytics::RefreshApprovalsData do
       end
     end
   end
+
+  describe '#execute_async' do
+    it 'schedules async execution' do
+      expect(Analytics::CodeReviewMetricsWorker)
+        .to receive(:perform_async).with(described_class.name, merge_request.id, force: true)
+
+      subject.execute_async(force: true)
+    end
+  end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequest::Metrics do
+  describe '#review_start_at' do
+    it 'is first_comment_at' do
+      subject.first_comment_at = 1.hour.ago
+
+      expect(subject.review_start_at).to be_like_time(1.hour.ago)
+    end
+  end
+
+  describe '#review_end_at' do
+    context 'when MR is merged' do
+      before do
+        subject.merged_at = 1.day.ago
+      end
+
+      it 'is merged_at' do
+        expect(subject.review_end_at).to be_like_time(1.day.ago)
+      end
+    end
+
+    context 'when MR is not merged' do
+      it 'is Time.now' do
+        expect(subject.review_end_at).to be_like_time(Time.now)
+      end
+    end
+  end
+
+  describe '#review_time' do
+    it 'is nil if there is no review_start_at' do
+      expect(subject.review_time).to eq nil
+    end
+
+    it 'is review_end_at - review_start_at' do
+      subject.merged_at = 1.day.ago
+      subject.first_comment_at = 1.week.ago
+
+      expect(subject.review_time).to be_like_time(6.days)
+    end
+  end
+end

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -98,7 +98,7 @@ describe MergeRequests::ApprovalService do
 
           it 'schedules RefreshApprovalsData' do
             expect(Analytics::CodeReviewMetricsWorker)
-              .to receive(:perform_async).with('::Analytics::RefreshApprovalsData', merge_request.id)
+              .to receive(:perform_async).with('Analytics::RefreshApprovalsData', merge_request.id)
 
             service.execute(merge_request)
           end

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -59,7 +59,7 @@ describe MergeRequests::RemoveApprovalService do
 
           it 'schedules RefreshApprovalsData' do
             expect(Analytics::CodeReviewMetricsWorker)
-              .to receive(:perform_async).with('::Analytics::RefreshApprovalsData', merge_request.id, force: true)
+              .to receive(:perform_async).with('Analytics::RefreshApprovalsData', merge_request.id, force: true)
 
             service.execute(merge_request)
           end