From 49dc8215b407f44eb02a4adea8cb100de5c55a7b Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Mon, 6 Aug 2018 12:26:01 -0300
Subject: [PATCH] Avoid N+1 on MRs page when metrics merging date cannot be
 found

---
 app/models/merge_request.rb                   | 28 +++++---
 ...ix-n-plus-1-for-mrs-without-merge-info.yml |  5 ++
 spec/models/merge_request_spec.rb             | 67 +++++++++++++++++++
 3 files changed, 89 insertions(+), 11 deletions(-)
 create mode 100644 changelogs/unreleased/osw-fix-n-plus-1-for-mrs-without-merge-info.yml

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 6de44751f1b..bfe06bbfd91 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1074,23 +1074,29 @@ class MergeRequest < ActiveRecord::Base
 
   def can_be_reverted?(current_user)
     return false unless merge_commit
+    return false unless merged_at
 
-    merged_at = metrics&.merged_at
-    notes_association = notes_with_associations
+    # 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
 
-    if merged_at
-      # 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
+    notes_association = notes_with_associations.where('created_at >= ?', cutoff)
 
     !merge_commit.has_been_reverted?(current_user, notes_association)
   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?
     merge_commit.present?
   end
diff --git a/changelogs/unreleased/osw-fix-n-plus-1-for-mrs-without-merge-info.yml b/changelogs/unreleased/osw-fix-n-plus-1-for-mrs-without-merge-info.yml
new file mode 100644
index 00000000000..dc8148fa1a5
--- /dev/null
+++ b/changelogs/unreleased/osw-fix-n-plus-1-for-mrs-without-merge-info.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid N+1 on MRs page when metrics merging date cannot be found
+merge_request: 21053
+author:
+type: performance
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index ffdec09deef..8ac4273dd76 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1288,6 +1288,16 @@ describe MergeRequest do
                                           project.default_branch == branch)
         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
           it 'returns false' do
             expect(subject.can_be_reverted?(current_user)).to be_falsey
@@ -1327,6 +1337,63 @@ describe MergeRequest do
     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
     let(:project) { create(:project, :public) }