diff options
| -rw-r--r-- | app/models/commit.rb | 5 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 11 | ||||
| -rw-r--r-- | changelogs/unreleased/41807-15665-consistently-502s-because-it-fetches-every-commit.yml | 6 | ||||
| -rw-r--r-- | spec/models/commit_range_spec.rb | 6 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 77 |
5 files changed, 99 insertions, 6 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index 39d7f5b159d..ede8ad301e4 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -342,10 +342,11 @@ class Commit @merged_merge_request_hash[current_user] end - def has_been_reverted?(current_user, noteable = self) + def has_been_reverted?(current_user, notes_association = nil) ext = all_references(current_user) + notes_association ||= notes_with_associations - noteable.notes_with_associations.system.each do |note| + notes_association.system.each do |note| note.all_references(current_user, extractor: ext) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ef58816937c..8efe7d41f37 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -982,7 +982,16 @@ class MergeRequest < ActiveRecord::Base end def can_be_reverted?(current_user) - merge_commit && !merge_commit.has_been_reverted?(current_user, self) + return false unless merge_commit + + merged_at = metrics&.merged_at + notes_association = notes_with_associations + + if merged_at + notes_association = notes_association.where('created_at > ?', merged_at) + end + + !merge_commit.has_been_reverted?(current_user, notes_association) end def can_be_cherry_picked? diff --git a/changelogs/unreleased/41807-15665-consistently-502s-because-it-fetches-every-commit.yml b/changelogs/unreleased/41807-15665-consistently-502s-because-it-fetches-every-commit.yml new file mode 100644 index 00000000000..146ae12afbd --- /dev/null +++ b/changelogs/unreleased/41807-15665-consistently-502s-because-it-fetches-every-commit.yml @@ -0,0 +1,6 @@ +--- +title: Speed up loading merged merge requests when they contained a lot of commits + before merging +merge_request: 16320 +author: +type: performance diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb index 38829773599..f2efcd9d0e9 100644 --- a/spec/models/commit_range_spec.rb +++ b/spec/models/commit_range_spec.rb @@ -151,11 +151,11 @@ describe CommitRange do .with(commit1, user) .and_return(true) - expect(commit1.has_been_reverted?(user, issue)).to eq(true) + expect(commit1.has_been_reverted?(user, issue.notes_with_associations)).to eq(true) end - it 'returns false a commit has not been reverted' do - expect(commit1.has_been_reverted?(user, issue)).to eq(false) + it 'returns false if the commit has not been reverted' do + expect(commit1.has_been_reverted?(user, issue.notes_with_associations)).to eq(false) end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 07b3e1c1758..02ccef9becc 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1035,6 +1035,83 @@ describe MergeRequest do end end + describe '#can_be_reverted?' do + context 'when there is no merged_at for the MR' do + before do + subject.metrics.update!(merged_at: nil) + end + + it 'returns false' do + expect(subject.can_be_reverted?(nil)).to be_falsey + end + end + + context 'when there is no merge_commit for the MR' do + before do + subject.metrics.update!(merged_at: Time.now.utc) + end + + it 'returns false' do + expect(subject.can_be_reverted?(nil)).to be_falsey + end + end + + context 'when the MR has been merged' do + before do + MergeRequests::MergeService + .new(subject.target_project, subject.author) + .execute(subject) + end + + context 'when there is no revert commit' do + it 'returns true' do + expect(subject.can_be_reverted?(nil)).to be_truthy + end + end + + context 'when there is a revert commit' do + let(:current_user) { subject.author } + let(:branch) { subject.target_branch } + let(:project) { subject.target_project } + + let(:revert_commit_id) do + params = { + commit: subject.merge_commit, + branch_name: branch, + start_branch: branch + } + + Commits::RevertService.new(project, current_user, params).execute[:result] + end + + before do + project.add_master(current_user) + + ProcessCommitWorker.new.perform(project.id, + current_user.id, + project.commit(revert_commit_id).to_hash, + project.default_branch == branch) + end + + context 'when the revert commit is mentioned in a note after the MR was merged' do + 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 before the MR was merged' do + before do + subject.notes.last.update!(created_at: subject.metrics.merged_at - 1.second) + end + + it 'returns true' do + expect(subject.can_be_reverted?(current_user)).to be_truthy + end + end + end + end + end + describe '#participants' do let(:project) { create(:project, :public) } |