Merge branch '196147-code-review-analytics-review-calculation' into 'master'

Code Review Analytics: Add first comment metric calculation

Closes #196147

See merge request gitlab-org/gitlab!22988

app/services/notes/destroy_service.rb

@@ -11,3 +11,5 @@ module Notes
     end
   end
 end
+
+Notes::DestroyService.prepend_if_ee('EE::Notes::DestroyService')

ee/app/services/ee/notes/destroy_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Notes
+    module DestroyService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(note)
+        super
+
+        Analytics::RefreshCommentsData.for_note(note)&.execute(force: true)
+      end
+    end
+  end
+end

ee/app/services/ee/notes/post_process_service.rb

@@ -9,9 +9,10 @@ module EE
       override :execute
       def execute
         super
-        return unless create_design_discussion_system_note?
 
-        ::SystemNoteService.design_discussion_added(note)
+        Analytics::RefreshCommentsData.for_note(note)&.execute
+
+        ::SystemNoteService.design_discussion_added(note) if create_design_discussion_system_note?
       end
 
       private

ee/lib/analytics/productivity_calculator.rb

@@ -17,6 +17,12 @@ module Analytics
       }
     end
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def first_comment_at
+      merge_request.related_notes.by_humans.where.not(author_id: merge_request.author_id).fresh.first&.created_at
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     private
 
     attr_reader :merge_request
@@ -27,10 +33,6 @@ module Analytics
       merge_request_diff.lines_count
     end
 
-    def first_comment_at
-      merge_request.notes.by_humans.fresh.first&.created_at
-    end
-
     def first_commit_at
       merge_request_diff&.first_commit&.authored_date
     end

ee/lib/analytics/refresh_comments_data.rb

+# frozen_string_literal: true
+
+module Analytics
+  class RefreshCommentsData
+    # rubocop: disable CodeReuse/ActiveRecord
+    def self.for_note(note)
+      if note.for_commit?
+        merge_requests = note.noteable.merge_requests.includes(:metrics)
+      elsif note.for_merge_request?
+        merge_requests = [note.noteable]
+      else
+        return
+      end
+
+      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|
+        next if !force && mr.metrics.first_comment_at
+
+        mr.metrics.update!(first_comment_at: ProductivityCalculator.new(mr).first_comment_at)
+      end
+    end
+
+    private
+
+    attr_reader :merge_requests
+  end
+end

ee/spec/lib/analytics/productivity_calculator_spec.rb

@@ -5,20 +5,45 @@ require 'spec_helper'
 describe Analytics::ProductivityCalculator do
   subject { described_class.new(merge_request) }
 
-  let(:merge_request) { create(:merge_request_with_diff_notes, :merged, :with_diffs, created_at: 31.days.ago) }
+  let_it_be(:merge_request) { create(:merge_request, :merged, :with_diffs, created_at: 31.days.ago) }
+  let_it_be(:merge_request_note) do
+    create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.source_project, author: create(:user))
+  end
+  let_it_be(:merge_request_author_note) do
+    create(:diff_note_on_merge_request,
+           noteable: merge_request,
+           project: merge_request.source_project,
+           author: merge_request.author,
+           created_at: 11.months.ago
+          )
+  end
+  let_it_be(:merge_request_bot_note) do
+    create(:diff_note_on_merge_request,
+           noteable: merge_request,
+           project: merge_request.source_project,
+           author: create(:user, :bot),
+           created_at: 12.months.ago
+          )
+  end
 
   describe '#productivity_data' do
     it 'calculates productivity data' do
       expected_data = {
-        first_comment_at: merge_request.notes.order(created_at: :asc).first.created_at,
-        first_commit_at: merge_request.first_commit.authored_date,
-        last_commit_at: merge_request.merge_request_diff.last_commit.committed_date,
+        first_comment_at: be_like_time(merge_request_note.created_at),
+        first_commit_at: be_like_time(merge_request.first_commit.authored_date),
+        last_commit_at: be_like_time(merge_request.merge_request_diff.last_commit.committed_date),
         commits_count: merge_request.commits_count,
         diff_size: merge_request.merge_request_diff.lines_count,
         modified_paths_size: merge_request.modified_paths.size
       }
 
-      expect(subject.productivity_data).to eq(expected_data)
+      expect(subject.productivity_data).to match(expected_data)
+    end
+  end
+
+  describe '#first_comment_at' do
+    it 'returns first non-author comment' do
+      expect(subject.first_comment_at).to be_like_time(merge_request_note.created_at)
     end
   end
 end

ee/spec/lib/analytics/refresh_comments_data_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Analytics::RefreshCommentsData do
+  subject { described_class.for_note(note) }
+
+  describe '.for_note' do
+    context 'for non-commit, non-mr note' do
+      let(:note) { create(:note, :on_issue) }
+
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#execute' do
+    context 'when note is for a merge request' do
+      let(:noteable) { create(:merge_request) }
+      let(:note) { create(:note, :on_merge_request, noteable: noteable, project: noteable.project, author: create(:user)) }
+
+      it 'updates mr first_comment_at metric' do
+        expect do
+          subject.execute
+          noteable.metrics.reload
+        end.to change { noteable.metrics.first_comment_at }.from(nil).to(be_like_time(note.created_at))
+      end
+
+      context 'and first_comment_at is already filled' do
+        before do
+          noteable.metrics.update(first_comment_at: 3.days.ago.beginning_of_day)
+        end
+
+        it 'does not change mr first_comment_at metric' do
+          expect do
+            subject.execute
+            noteable.metrics.reload
+          end.not_to change { noteable.metrics.first_comment_at }
+        end
+
+        it 'updates mr first_comment_at metric if forced' do
+          expect do
+            subject.execute(force: true)
+            noteable.metrics.reload
+          end.to change { noteable.metrics.first_comment_at }.to(be_like_time(note.created_at))
+        end
+      end
+    end
+
+    context 'when noteable is a commit' do
+      let(:note) { create(:diff_note_on_commit, author: create(:user)) }
+      let!(:merge_request) { create :merge_request, source_project: note.project }
+
+      before do
+        allow(note.noteable).to receive(:merge_requests).and_return(note.project.merge_requests)
+      end
+
+      it 'updates mr first_comment_at metric' do
+        expect do
+          subject.execute
+          merge_request.metrics.reload
+        end.to change { merge_request.metrics.first_comment_at }.from(nil).to(be_like_time(note.created_at))
+      end
+
+      context 'and first_comment_at is already filled' do
+        let!(:merge_request) do
+          create :merge_request, :with_productivity_metrics,
+                 source_project: note.project, metrics_data: { first_comment_at: 3.days.ago.beginning_of_day }
+        end
+
+        it 'does not change mr first_comment_at metric' do
+          expect do
+            subject.execute
+            merge_request.metrics.reload
+          end.not_to change { merge_request.metrics.first_comment_at }
+        end
+
+        it 'updates mr first_comment_at metric if forced' do
+          expect do
+            subject.execute(force: true)
+            merge_request.metrics.reload
+          end.to change { merge_request.metrics.first_comment_at }.to(be_like_time(note.created_at))
+        end
+      end
+    end
+  end
+end

ee/spec/services/ee/notes/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Notes::DestroyService do
+  subject { described_class.new(note.project) }
+
+  let(:note) { create(:note) }
+
+  describe '#execute' do
+    let(:analytics_mock) { instance_double('Analytics::RefreshCommentsData') }
+
+    it 'invokes forced Analytics::RefreshCommentsData' do
+      allow(Analytics::RefreshCommentsData).to receive(:for_note).with(note).and_return(analytics_mock)
+
+      expect(analytics_mock).to receive(:execute).with(force: true)
+
+      subject.execute(note)
+    end
+  end
+end

ee/spec/services/ee/notes/post_process_service_spec.rb

@@ -30,5 +30,20 @@ describe Notes::PostProcessService do
         end
       end
     end
+
+    context 'analytics' do
+      subject { described_class.new(note) }
+
+      let(:note) { create(:note) }
+      let(:analytics_mock) { instance_double('Analytics::RefreshCommentsData') }
+
+      it 'invokes Analytics::RefreshCommentsData' do
+        allow(Analytics::RefreshCommentsData).to receive(:for_note).with(note).and_return(analytics_mock)
+
+        expect(analytics_mock).to receive(:execute)
+
+        subject.execute
+      end
+    end
   end
 end