Merge branch '340406-bug-compliance-report-shows-empty-author-for-some-merge-events' into 'master'

Fix compliance report shows empty author for some merge events

See merge request gitlab-org/gitlab!71485

app/serializers/merge_request_metrics_helper.rb

+# frozen_string_literal: true
+
+module MergeRequestMetricsHelper
+  # There are cases where where metrics object doesn't exist and it needs to be rebuilt.
+  # TODO: Once https://gitlab.com/gitlab-org/gitlab/-/issues/342508 has been resolved and
+  # all merge requests have metrics we can remove this helper method.
+  def build_metrics(merge_request)
+    # There's no need to query and serialize metrics data for merge requests that are not
+    # merged or closed.
+    return unless merge_request.merged? || merge_request.closed?
+    return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id
+    return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id
+
+    build_metrics_from_events(merge_request)
+  end
+
+  private
+
+  def build_metrics_from_events(merge_request)
+    closed_event = merge_request.closed_event
+    merge_event = merge_request.merge_event
+
+    MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
+                              latest_closed_by: closed_event&.author,
+                              merged_at: merge_event&.updated_at,
+                              merged_by: merge_event&.author)
+  end
+end

app/serializers/merge_request_poll_cached_widget_entity.rb

 # frozen_string_literal: true
 
 class MergeRequestPollCachedWidgetEntity < IssuableEntity
+  include MergeRequestMetricsHelper
+
   expose :auto_merge_enabled
   expose :state
   expose :merged_commit_sha
@@ -158,29 +160,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
     @presenters ||= {}
     @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter
   end
-
-  # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs,
-  # we can remove this method and just serialize MergeRequest#metrics
-  # instead. See https://gitlab.com/gitlab-org/gitlab-foss/issues/41587
-  def build_metrics(merge_request)
-    # There's no need to query and serialize metrics data for merge requests that are not
-    # merged or closed.
-    return unless merge_request.merged? || merge_request.closed?
-    return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id
-    return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id
-
-    build_metrics_from_events(merge_request)
-  end
-
-  def build_metrics_from_events(merge_request)
-    closed_event = merge_request.closed_event
-    merge_event = merge_request.merge_event
-
-    MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
-                              latest_closed_by: closed_event&.author,
-                              merged_at: merge_event&.updated_at,
-                              merged_by: merge_event&.author)
-  end
 end
 
 MergeRequestPollCachedWidgetEntity.prepend_mod_with('MergeRequestPollCachedWidgetEntity')

ee/app/serializers/merge_request_compliance_entity.rb

 # frozen_string_literal: true
 
 class MergeRequestComplianceEntity < Grape::Entity
+  include MergeRequestMetricsHelper
   include RequestAwareEntity
 
   SUCCESS_APPROVAL_STATUS = :success
@@ -75,7 +76,7 @@ class MergeRequestComplianceEntity < Grape::Entity
   end
 
   def merged_by
-    merge_request.metrics.merged_by
+    build_metrics(merge_request).merged_by
   end
 
   def target_branch_uri

ee/spec/serializers/ee/merge_request_serializer_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe MergeRequestSerializer do
   let_it_be(:user) { create(:user) }
-  let_it_be(:resource) { create(:merge_request, description: "Description") }
+  let_it_be(:resource) { create(:merge_request, :merged, description: "Description") }
 
   let(:json_entity) do
     described_class.new(current_user: user)

ee/spec/serializers/merge_request_compliance_entity_spec.rb

@@ -39,6 +39,14 @@ RSpec.describe MergeRequestComplianceEntity do
       )
     end
 
+    it 'builds the merge request metrics' do
+      expect_any_instance_of(MergeRequestMetricsHelper) do |instance|
+        expect(instance).to receive(:build_metrics).with(merge_request)
+      end
+
+      subject
+    end
+
     describe 'with an approver' do
       let_it_be(:approver) { create(:user) }
       let_it_be(:approval) { create :approval, merge_request: merge_request, user: approver }

spec/serializers/merge_request_metrics_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequestMetricsHelper do
+  let_it_be(:user) { create(:user) }
+
+  let(:merge_request) { create(:merge_request) }
+  let(:helper) { Class.new.include(described_class).new }
+
+  describe '#build_metrics' do
+    subject do
+      helper.build_metrics(merge_request)
+    end
+
+    shared_examples 'does not rebuild the metrics' do
+      it 'does not call the merge request metrics class' do
+        expect(MergeRequest::Metrics).not_to receive(:new)
+
+        subject
+      end
+
+      it 'returns the metrics for the given merge request' do
+        expect(subject).to be_kind_of(MergeRequest::Metrics)
+        expect(subject[:merge_request_id]).to eq(merge_request.id)
+      end
+    end
+
+    context 'when closed and metrics exists' do
+      before do
+        merge_request.close!
+        merge_request.metrics.update!(latest_closed_by: user)
+      end
+
+      include_examples 'does not rebuild the metrics'
+    end
+
+    context 'when merged and metrics exists' do
+      before do
+        merge_request.mark_as_merged!
+        merge_request.metrics.update!(merged_by: user)
+      end
+
+      include_examples 'does not rebuild the metrics'
+    end
+
+    context 'when merged and metrics do not exists' do
+      before do
+        merge_request.mark_as_merged!
+        merge_request.metrics.destroy!
+        merge_request.reload
+      end
+
+      it 'rebuilds the merge request metrics' do
+        closed_event = merge_request.closed_event
+        merge_event = merge_request.merge_event
+
+        expect(MergeRequest::Metrics).to receive(:new)
+            .with(latest_closed_at: closed_event&.updated_at,
+              latest_closed_by: closed_event&.author,
+              merged_at: merge_event&.updated_at,
+              merged_by: merge_event&.author)
+            .and_call_original
+
+        subject
+      end
+    end
+  end
+end