Avoid N+1 on MRs page when metrics merging date cannot be found
This commit is contained in:
parent
415b2f943b
commit
49dc8215b4
3 changed files with 89 additions and 11 deletions
|
@ -1074,23 +1074,29 @@ class MergeRequest < ActiveRecord::Base
|
||||||
|
|
||||||
def can_be_reverted?(current_user)
|
def can_be_reverted?(current_user)
|
||||||
return false unless merge_commit
|
return false unless merge_commit
|
||||||
|
return false unless merged_at
|
||||||
|
|
||||||
merged_at = metrics&.merged_at
|
# It is not guaranteed that Note#created_at will be strictly later than
|
||||||
notes_association = notes_with_associations
|
# MergeRequestMetric#merged_at. Nanoseconds on MySQL may break this
|
||||||
|
# comparison, as will a HA environment if clocks are not *precisely*
|
||||||
|
# synchronized. Add a minute's leeway to compensate for both possibilities
|
||||||
|
cutoff = merged_at - 1.minute
|
||||||
|
|
||||||
if merged_at
|
notes_association = notes_with_associations.where('created_at >= ?', cutoff)
|
||||||
# It is not guaranteed that Note#created_at will be strictly later than
|
|
||||||
# MergeRequestMetric#merged_at. Nanoseconds on MySQL may break this
|
|
||||||
# comparison, as will a HA environment if clocks are not *precisely*
|
|
||||||
# synchronized. Add a minute's leeway to compensate for both possibilities
|
|
||||||
cutoff = merged_at - 1.minute
|
|
||||||
|
|
||||||
notes_association = notes_association.where('created_at >= ?', cutoff)
|
|
||||||
end
|
|
||||||
|
|
||||||
!merge_commit.has_been_reverted?(current_user, notes_association)
|
!merge_commit.has_been_reverted?(current_user, notes_association)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def merged_at
|
||||||
|
strong_memoize(:merged_at) do
|
||||||
|
next unless merged?
|
||||||
|
|
||||||
|
metrics&.merged_at ||
|
||||||
|
merge_event&.created_at ||
|
||||||
|
notes.system.reorder(nil).find_by(note: 'merged')&.created_at
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
def can_be_cherry_picked?
|
def can_be_cherry_picked?
|
||||||
merge_commit.present?
|
merge_commit.present?
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Avoid N+1 on MRs page when metrics merging date cannot be found
|
||||||
|
merge_request: 21053
|
||||||
|
author:
|
||||||
|
type: performance
|
|
@ -1288,6 +1288,16 @@ describe MergeRequest do
|
||||||
project.default_branch == branch)
|
project.default_branch == branch)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'but merged at timestamp cannot be found' do
|
||||||
|
before do
|
||||||
|
allow(subject).to receive(:merged_at) { nil }
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns false' do
|
||||||
|
expect(subject.can_be_reverted?(current_user)).to be_falsey
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'when the revert commit is mentioned in a note after the MR was merged' do
|
context 'when the revert commit is mentioned in a note after the MR was merged' do
|
||||||
it 'returns false' do
|
it 'returns false' do
|
||||||
expect(subject.can_be_reverted?(current_user)).to be_falsey
|
expect(subject.can_be_reverted?(current_user)).to be_falsey
|
||||||
|
@ -1327,6 +1337,63 @@ describe MergeRequest do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#merged_at' do
|
||||||
|
context 'when MR is not merged' do
|
||||||
|
let(:merge_request) { create(:merge_request, :closed) }
|
||||||
|
|
||||||
|
it 'returns nil' do
|
||||||
|
expect(merge_request.merged_at).to be_nil
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when metrics has merged_at data' do
|
||||||
|
let(:merge_request) { create(:merge_request, :merged) }
|
||||||
|
|
||||||
|
before do
|
||||||
|
merge_request.metrics.update!(merged_at: 1.day.ago)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns metrics merged_at' do
|
||||||
|
expect(merge_request.merged_at).to eq(merge_request.metrics.merged_at)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when merged event is persisted, but no metrics merged_at is persisted' do
|
||||||
|
let(:user) { create(:user) }
|
||||||
|
let(:merge_request) { create(:merge_request, :merged) }
|
||||||
|
|
||||||
|
before do
|
||||||
|
EventCreateService.new.merge_mr(merge_request, user)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns merged event creation date' do
|
||||||
|
expect(merge_request.merge_event).to be_persisted
|
||||||
|
expect(merge_request.merged_at).to eq(merge_request.merge_event.created_at)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when merging note is persisted, but no metrics or merge event exists' do
|
||||||
|
let(:user) { create(:user) }
|
||||||
|
let(:merge_request) { create(:merge_request, :merged) }
|
||||||
|
|
||||||
|
before do
|
||||||
|
merge_request.metrics.destroy!
|
||||||
|
|
||||||
|
SystemNoteService.change_status(merge_request,
|
||||||
|
merge_request.target_project,
|
||||||
|
user,
|
||||||
|
merge_request.state, nil)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'returns merging note creation date' do
|
||||||
|
expect(merge_request.reload.metrics).to be_nil
|
||||||
|
expect(merge_request.merge_event).to be_nil
|
||||||
|
expect(merge_request.notes.count).to eq(1)
|
||||||
|
expect(merge_request.merged_at).to eq(merge_request.notes.first.created_at)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#participants' do
|
describe '#participants' do
|
||||||
let(:project) { create(:project, :public) }
|
let(:project) { create(:project, :public) }
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue