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

Add merge request metrics first approved at timestamp

See merge request gitlab-org/gitlab!25681

config/sidekiq_queues.yml

@@ -28,6 +28,8 @@
   - 1
 - - admin_emails
   - 1
+- - analytics_code_review_metrics
+  - 1
 - - authorized_projects
   - 2
 - - auto_devops

db/migrate/20200221074028_add_mr_metrics_first_approved_at.rb

+# frozen_string_literal: true
+
+class AddMrMetricsFirstApprovedAt < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column :merge_request_metrics, :first_approved_at, :datetime_with_timezone
+  end
+
+  def down
+    remove_column :merge_request_metrics, :first_approved_at
+  end
+end

db/schema.rb

@@ -2537,6 +2537,7 @@ ActiveRecord::Schema.define(version: 2020_02_24_163804) do
     t.integer "diff_size"
     t.integer "modified_paths_size"
     t.integer "commits_count"
+    t.datetime_with_timezone "first_approved_at"
     t.index ["first_deployed_to_production_at"], name: "index_merge_request_metrics_on_first_deployed_to_production_at"
     t.index ["latest_closed_at"], name: "index_merge_request_metrics_on_latest_closed_at", where: "(latest_closed_at IS NOT NULL)"
     t.index ["latest_closed_by_id"], name: "index_merge_request_metrics_on_latest_closed_by_id"

ee/app/services/merge_requests/approval_service.rb

@@ -16,6 +16,7 @@ module MergeRequests
 
         create_approval_note(merge_request)
         mark_pending_todos_as_done(merge_request)
+        calculate_approvals_metrics(merge_request)
 
         if merge_request.approvals_left.zero?
           notification_service.async.approve_mr(merge_request, current_user)
@@ -46,5 +47,11 @@ module MergeRequests
     def mark_pending_todos_as_done(merge_request)
       todo_service.mark_pending_todos_as_done(merge_request, current_user)
     end
+
+    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)
+    end
   end
 end

ee/app/services/merge_requests/remove_approval_service.rb

@@ -14,6 +14,7 @@ module MergeRequests
       if approval.destroy_all # rubocop: disable DestroyAll
         merge_request.reset_approval_cache!
         create_note(merge_request)
+        recalculate_approvals_metrics(merge_request)
 
         if currently_approved
           notification_service.async.unapprove_mr(merge_request, current_user)
@@ -30,5 +31,11 @@ module MergeRequests
     def create_note(merge_request)
       SystemNoteService.unapprove_mr(merge_request, current_user)
     end
+
+    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)
+    end
   end
 end

ee/app/workers/all_queues.yml

@@ -430,6 +430,13 @@
   :resource_boundary: :unknown
   :weight: 1
   :idempotent: 
+- :name: analytics_code_review_metrics
+  :feature_category: :code_analytics
+  :has_external_dependencies: 
+  :latency_sensitive: 
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
 - :name: create_github_webhook
   :feature_category: :integrations
   :has_external_dependencies: true

ee/app/workers/analytics/code_review_metrics_worker.rb

+# frozen_string_literal: true
+
+module Analytics
+  class CodeReviewMetricsWorker
+    include ApplicationWorker
+
+    feature_category :code_analytics
+
+    idempotent!
+
+    def perform(operation, merge_request_id, **execute_args)
+      ::MergeRequest.find_by_id(merge_request_id).try do |merge_request|
+        break unless merge_request.project.feature_available?(:code_review_analytics)
+
+        operation_klass = operation.constantize
+        operation_klass.new(merge_request).execute(**execute_args)
+      end
+    end
+  end
+end

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

+---
+title: Add merge request metrics first approved at timestamp
+merge_request: 25681
+author:
+type: added

ee/lib/analytics/productivity_calculator.rb

@@ -25,6 +25,12 @@ module Analytics
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def first_approved_at
+      merge_request.approvals.order(id: :asc).limit(1).pluck(:created_at).first
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     private
 
     attr_reader :merge_request

ee/lib/analytics/refresh_approvals_data.rb

+# frozen_string_literal: true
+
+module Analytics
+  class RefreshApprovalsData
+    def initialize(merge_request)
+      @merge_request = merge_request
+    end
+
+    def execute(force: false)
+      metrics = merge_request.ensure_metrics
+
+      return if !force && metrics.first_approved_at
+
+      metrics.update!(first_approved_at: ProductivityCalculator.new(merge_request).first_approved_at)
+    end
+
+    private
+
+    attr_reader :merge_request
+  end
+end

ee/spec/lib/analytics/productivity_calculator_spec.rb

@@ -46,4 +46,13 @@ describe Analytics::ProductivityCalculator do
       expect(subject.first_comment_at).to be_like_time(merge_request_note.created_at)
     end
   end
+
+  describe '#first_approved_at' do
+    it 'returns first approval creation timestamp' do
+      create :approval, merge_request: merge_request, created_at: 1.day.ago
+      create :approval, merge_request: merge_request, created_at: 1.minute.ago
+
+      expect(subject.first_approved_at).to be_like_time(1.day.ago)
+    end
+  end
 end

ee/spec/lib/analytics/refresh_approvals_data_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::RefreshApprovalsData do
+  subject { described_class.new(merge_request) }
+
+  let!(:approval) { create(:approval) }
+  let(:merge_request) { approval.merge_request }
+
+  describe '#execute' do
+    it 'updates mr first_approved_at metric' do
+      expect do
+        subject.execute
+        merge_request.metrics.reload
+      end.to change { merge_request.metrics.first_approved_at }.from(nil).to(be_like_time(approval.created_at))
+    end
+
+    context 'when first_approved_at is already present' do
+      before do
+        merge_request.metrics.update(first_approved_at: 3.days.ago.beginning_of_day)
+      end
+
+      it 'does not change mr first_approved_at metric' do
+        expect do
+          subject.execute
+          merge_request.metrics.reload
+        end.not_to change { merge_request.metrics.first_approved_at }
+      end
+
+      it 'updates mr first_approved_at metric if forced' do
+        expect do
+          subject.execute(force: true)
+          merge_request.metrics.reload
+        end.to change { merge_request.metrics.first_approved_at }.to(be_like_time(approval.created_at))
+      end
+    end
+
+    context 'when no merge request metric is present' do
+      before do
+        merge_request.metrics.destroy
+        merge_request.reload
+      end
+
+      it 'creates one' do
+        expect { subject.execute }
+          .to change { merge_request.metrics&.first_approved_at }.from(nil).to(be_like_time(approval.created_at))
+      end
+    end
+  end
+end

ee/spec/services/merge_requests/approval_service_spec.rb

@@ -89,6 +89,33 @@ describe MergeRequests::ApprovalService do
           service.execute(merge_request)
         end
       end
+
+      context 'approvals metrics calculation' do
+        context 'when code_review_analytics project feature is available' do
+          before do
+            stub_licensed_features(code_review_analytics: true)
+          end
+
+          it 'schedules RefreshApprovalsData' do
+            expect(Analytics::CodeReviewMetricsWorker)
+              .to receive(:perform_async).with('::Analytics::RefreshApprovalsData', merge_request.id)
+
+            service.execute(merge_request)
+          end
+        end
+
+        context 'when code_review_analytics is not available' do
+          before do
+            stub_licensed_features(code_review_analytics: false)
+          end
+
+          it 'does not schedule for RefreshApprovalsData' do
+            expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
+
+            service.execute(merge_request)
+          end
+        end
+      end
     end
 
     context 'when project requires force auth for approval' do

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -50,6 +50,33 @@ describe MergeRequests::RemoveApprovalService do
 
         execute!
       end
+
+      context 'approvals metrics calculation' do
+        context 'when code_review_analytics project feature is available' do
+          before do
+            stub_licensed_features(code_review_analytics: true)
+          end
+
+          it 'schedules RefreshApprovalsData' do
+            expect(Analytics::CodeReviewMetricsWorker)
+              .to receive(:perform_async).with('::Analytics::RefreshApprovalsData', merge_request.id, force: true)
+
+            service.execute(merge_request)
+          end
+        end
+
+        context 'when code_review_analytics is not available' do
+          before do
+            stub_licensed_features(code_review_analytics: false)
+          end
+
+          it 'does not schedule for RefreshApprovalsData' do
+            expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
+
+            service.execute(merge_request)
+          end
+        end
+      end
     end
 
     context 'with an approved merge request' do

ee/spec/workers/analytics/code_review_metrics_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::CodeReviewMetricsWorker do
+  subject(:worker) { described_class.new }
+
+  describe "#perform" do
+    let(:operation) { '::Analytics::RefreshApprovalsData' }
+    let(:merge_request) { create(:merge_request) }
+
+    context 'with code review analytics feature available' do
+      before do
+        stub_licensed_features(code_review_analytics: true)
+      end
+
+      it 'executes operation for provided MR' do
+        expect_next_instance_of(operation.constantize, merge_request) do |instance|
+          expect(instance).to receive(:execute).with(force: true)
+        end
+
+        worker.perform(operation, merge_request.id, force: true)
+      end
+
+      context 'for invalid MR id' do
+        it 'does not  execute  operation' do
+          expect(operation.constantize).not_to receive(:new)
+
+          worker.perform(operation, 1992291)
+        end
+      end
+
+      context 'for invalid operation' do
+        let(:operation) { 'SomeInvalidClassName' }
+
+        it 'raises an error' do
+          expect do
+            worker.perform(operation, merge_request.id)
+          end.to raise_error NameError, 'uninitialized constant SomeInvalidClassName'
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -271,6 +271,7 @@ MergeRequest::Metrics:
 - diff_size
 - modified_paths_size
 - commits_count
+- first_approved_at
 Ci::Pipeline:
 - id
 - project_id