Merge branch '205597-start-code-review-on-assign' into 'master'

Add merge request first reassign timestamp metric

See merge request gitlab-org/gitlab!26664

app/services/merge_requests/update_service.rb

@@ -43,11 +43,7 @@ module MergeRequests
         abort_auto_merge(merge_request, 'target branch was changed')
       end
 
-      if merge_request.assignees != old_assignees
-        create_assignee_note(merge_request, old_assignees)
-        notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
-        todo_service.reassigned_issuable(merge_request, current_user, old_assignees)
-      end
+      handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees
 
       if merge_request.previous_changes.include?('target_branch') ||
           merge_request.previous_changes.include?('source_branch')
@@ -120,6 +116,12 @@ module MergeRequests
       end
     end
 
+    def handle_assignees_change(merge_request, old_assignees)
+      create_assignee_note(merge_request, old_assignees)
+      notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
+      todo_service.reassigned_issuable(merge_request, current_user, old_assignees)
+    end
+
     def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
       SystemNoteService.change_branch(
         issuable, issuable.project, current_user, branch_type,

db/migrate/20200305121159_add_merge_request_metrics_first_reassigned_at.rb

+# frozen_string_literal: true
+
+class AddMergeRequestMetricsFirstReassignedAt < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_request_metrics, :first_reassigned_at, :datetime_with_timezone
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_request_metrics, :first_reassigned_at, :datetime_with_timezone
+    end
+  end
+end

db/migrate/20200306095654_add_merge_request_assignee_created_at.rb

+# frozen_string_literal: true
+
+class AddMergeRequestAssigneeCreatedAt < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_request_assignees, :created_at, :datetime_with_timezone
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_request_assignees, :created_at
+    end
+  end
+end

db/schema.rb

@@ -2450,6 +2450,7 @@ ActiveRecord::Schema.define(version: 2020_03_06_170531) do
   create_table "merge_request_assignees", force: :cascade do |t|
     t.integer "user_id", null: false
     t.integer "merge_request_id", null: false
+    t.datetime_with_timezone "created_at"
     t.index ["merge_request_id", "user_id"], name: "index_merge_request_assignees_on_merge_request_id_and_user_id", unique: true
     t.index ["merge_request_id"], name: "index_merge_request_assignees_on_merge_request_id"
     t.index ["user_id"], name: "index_merge_request_assignees_on_user_id"
@@ -2564,6 +2565,7 @@ ActiveRecord::Schema.define(version: 2020_03_06_170531) do
     t.integer "modified_paths_size"
     t.integer "commits_count"
     t.datetime_with_timezone "first_approved_at"
+    t.datetime_with_timezone "first_reassigned_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/ee/merge_requests/update_service.rb

@@ -46,6 +46,15 @@ module EE
 
         reset_approvals(merge_request)
       end
+
+      override :handle_assignees_change
+      def handle_assignees_change(merge_request, _old_assignees)
+        super
+
+        return unless merge_request.project.feature_available?(:code_review_analytics)
+
+        Analytics::RefreshReassignData.new(merge_request).execute_async
+      end
     end
   end
 end

ee/changelogs/unreleased/205597-start-code-review-on-assign.yml

+---
+title: Add code review reassign metric structure
+merge_request: 26664
+author:
+type: added

ee/lib/analytics/merge_request_metrics_calculator.rb

@@ -31,6 +31,15 @@ module Analytics
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def first_reassigned_at
+      merge_request.merge_request_assignees
+        .where.not(assignee: merge_request.author)
+        .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

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

ee/lib/analytics/refresh_reassign_data.rb

+# frozen_string_literal: true
+
+module Analytics
+  class RefreshReassignData
+    include MergeRequestMetricsRefresh
+
+    # Override `MergeRequestMetricsRefresh#initialize` to accept single MR only
+    def initialize(merge_request)
+      super
+    end
+
+    private
+
+    def metric_already_present?(metrics)
+      metrics.first_reassigned_at
+    end
+
+    def update_metric!(metrics)
+      metrics.update!(
+        first_reassigned_at: MergeRequestMetricsCalculator.new(metrics.merge_request).first_reassigned_at
+      )
+    end
+  end
+end

ee/spec/lib/analytics/merge_request_metrics_calculator_spec.rb

@@ -55,4 +55,14 @@ describe Analytics::MergeRequestMetricsCalculator do
       expect(subject.first_approved_at).to be_like_time(1.day.ago)
     end
   end
+
+  describe '#first_reassigned_at' do
+    it 'returns earliest non-author assignee creation timestamp' do
+      merge_request.merge_request_assignees.create(assignee: merge_request.author, created_at: 5.days.ago)
+      merge_request.merge_request_assignees.create(assignee: create(:user), created_at: 3.days.ago)
+      merge_request.merge_request_assignees.create(assignee: create(:user), created_at: 1.day.ago)
+
+      expect(subject.first_reassigned_at).to be_like_time(3.days.ago)
+    end
+  end
 end

ee/spec/lib/analytics/refresh_reassign_data_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::RefreshReassignData do
+  subject { described_class.new(merge_request) }
+
+  let(:merge_request) { create :merge_request }
+
+  describe '#execute' do
+    let(:calculated_value) { 2.days.ago.beginning_of_day }
+
+    include_examples 'common merge request metric refresh for', :first_reassigned_at
+  end
+end

ee/spec/services/merge_requests/update_service_spec.rb

@@ -215,5 +215,26 @@ describe MergeRequests::UpdateService, :mailer do
         update_merge_request({})
       end
     end
+
+    context 'when reassigned' do
+      it 'schedules for analytics metric update' do
+        expect(Analytics::CodeReviewMetricsWorker)
+          .to receive(:perform_async).with('Analytics::RefreshReassignData', merge_request.id, {})
+
+        update_merge_request({ assignee_ids: [user2.id] })
+      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 analytics metric update' do
+          expect(Analytics::CodeReviewMetricsWorker).not_to receive(:perform_async)
+
+          update_merge_request({ assignee_ids: [user2.id] })
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/import_export/safe_model_attributes.yml

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