Merge branch 'fix-merged-at-check-for-reverted-mrs' into 'master'

Fix MR revert check when no merged_at is present

See merge request gitlab-org/gitlab-ce!16760

2 files changed, 26 insertions, 16 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4accb08eaf9..f6d4843abc3 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -989,13 +989,13 @@ class MergeRequest < ActiveRecord::Base
     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
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 429b6615131..15f9c68e400 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1064,16 +1064,6 @@ describe MergeRequest do
   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)
@@ -1097,6 +1087,16 @@ describe MergeRequest do
         end
       end
 
+      context 'when there is no merged_at for the MR' do
+        before do
+          subject.metrics.update!(merged_at: nil)
+        end
+
+        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 }
@@ -1127,6 +1127,16 @@ describe MergeRequest do
           end
         end
 
+        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?(current_user)).to be_falsey
+          end
+        end
+
         context 'when the revert commit is mentioned in a note just before the MR was merged' do
           before do
             subject.notes.last.update!(created_at: subject.metrics.merged_at - 30.seconds)