Commit 77206b07 authored by Jiaan Louw's avatar Jiaan Louw Committed by Stan Hu

Fix compliance report shows empty author for some merge events

parent 2fe79453
# 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
# frozen_string_literal: true # frozen_string_literal: true
class MergeRequestPollCachedWidgetEntity < IssuableEntity class MergeRequestPollCachedWidgetEntity < IssuableEntity
include MergeRequestMetricsHelper
expose :auto_merge_enabled expose :auto_merge_enabled
expose :state expose :state
expose :merged_commit_sha expose :merged_commit_sha
...@@ -158,29 +160,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity ...@@ -158,29 +160,6 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
@presenters ||= {} @presenters ||= {}
@presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter
end 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 end
MergeRequestPollCachedWidgetEntity.prepend_mod_with('MergeRequestPollCachedWidgetEntity') MergeRequestPollCachedWidgetEntity.prepend_mod_with('MergeRequestPollCachedWidgetEntity')
# frozen_string_literal: true # frozen_string_literal: true
class MergeRequestComplianceEntity < Grape::Entity class MergeRequestComplianceEntity < Grape::Entity
include MergeRequestMetricsHelper
include RequestAwareEntity include RequestAwareEntity
SUCCESS_APPROVAL_STATUS = :success SUCCESS_APPROVAL_STATUS = :success
...@@ -75,7 +76,7 @@ class MergeRequestComplianceEntity < Grape::Entity ...@@ -75,7 +76,7 @@ class MergeRequestComplianceEntity < Grape::Entity
end end
def merged_by def merged_by
merge_request.metrics.merged_by build_metrics(merge_request).merged_by
end end
def target_branch_uri def target_branch_uri
......
...@@ -4,7 +4,7 @@ require 'spec_helper' ...@@ -4,7 +4,7 @@ require 'spec_helper'
RSpec.describe MergeRequestSerializer do RSpec.describe MergeRequestSerializer do
let_it_be(:user) { create(:user) } 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 let(:json_entity) do
described_class.new(current_user: user) described_class.new(current_user: user)
......
...@@ -39,6 +39,14 @@ RSpec.describe MergeRequestComplianceEntity do ...@@ -39,6 +39,14 @@ RSpec.describe MergeRequestComplianceEntity do
) )
end 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 describe 'with an approver' do
let_it_be(:approver) { create(:user) } let_it_be(:approver) { create(:user) }
let_it_be(:approval) { create :approval, merge_request: merge_request, user: approver } let_it_be(:approval) { create :approval, merge_request: merge_request, user: approver }
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment