Merge branch '6598-notify-only-open-unmergeable-mr' into 'master'

Notify conflict only for opened/locked merge requests

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

5 files changed, 52 insertions, 27 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4b78ba1029f..91e6d391986 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -129,9 +129,7 @@ class MergeRequest < ActiveRecord::Base
 
     after_transition unchecked: :cannot_be_merged do |merge_request, transition|
       begin
-        # Merge request can become unmergeable due to many reasons.
-        # We only notify if it is due to conflict.
-        unless merge_request.project.repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
+        if merge_request.notify_conflict?
           NotificationService.new.merge_request_unmergeable(merge_request)
           TodoService.new.merge_request_became_unmergeable(merge_request)
         end
@@ -715,6 +713,10 @@ class MergeRequest < ActiveRecord::Base
     should_remove_source_branch? || force_remove_source_branch?
   end
 
+  def notify_conflict?
+    (opened? || locked?) && !project.repository.can_be_merged?(diff_head_sha, target_branch)
+  end
+
   def related_notes
     # Fetch comments only from last 100 commits
     commits_for_notes_limit = 100
diff --git a/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml
new file mode 100644
index 00000000000..ae92c20fa1a
--- /dev/null
+++ b/changelogs/unreleased/6598-notify-only-open-unmergeable-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Notify conflict for only open merge request
+merge_request: 20125
+author:
+type: fixed
diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md
index edb0c6bdc30..5dc62a30128 100644
--- a/doc/workflow/notifications.md
+++ b/doc/workflow/notifications.md
@@ -111,7 +111,7 @@ by yourself (except when an issue is due). You will only receive automatic
 notifications when somebody else comments or adds changes to the ones that
 you've created or mentions you.
 
-If a merge request becomes unmergeable, its author will be notified about the cause.
+If an open merge request becomes unmergeable due to conflict, its author will be notified about the cause.
 If a user has also set the merge request to automatically merge once pipeline succeeds,
 then that user will also be notified.
 
diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md
index 762bf616268..760cd87d4cc 100644
--- a/doc/workflow/todos.md
+++ b/doc/workflow/todos.md
@@ -31,7 +31,7 @@ A Todo appears in your Todos dashboard when:
 - you are `@mentioned` in a comment on a commit,
 - a job in the CI pipeline running for your merge request failed, but this
   job is not allowed to fail.
-- a merge request becomes unmergeable, and you are either:
+- an open merge request becomes unmergeable due to conflict, and you are either:
   - the author, or
   - have set it to automatically merge once pipeline succeeds.
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 7ae70c3afb4..4b957c8a29f 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2145,8 +2145,7 @@ describe MergeRequest do
     describe 'transition to cannot_be_merged' do
       let(:notification_service) { double(:notification_service) }
       let(:todo_service) { double(:todo_service) }
-
-      subject { create(:merge_request, merge_status: :unchecked) }
+      subject { create(:merge_request, state, merge_status: :unchecked) }
 
       before do
         allow(NotificationService).to receive(:new).and_return(notification_service)
@@ -2155,33 +2154,52 @@ describe MergeRequest do
         allow(subject.project.repository).to receive(:can_be_merged?).and_return(false)
       end
 
-      it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do
-        expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once
-        expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
+      [:opened, :locked].each do |state|
+        context state do
+          let(:state) { state }
 
-        subject.mark_as_unmergeable
-        subject.mark_as_unchecked
-        subject.mark_as_unmergeable
-      end
+          it 'notifies conflict, but does not notify again if rechecking still results in cannot_be_merged' do
+            expect(notification_service).to receive(:merge_request_unmergeable).with(subject).once
+            expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).once
+
+            subject.mark_as_unmergeable
+            subject.mark_as_unchecked
+            subject.mark_as_unmergeable
+          end
+
+          it 'notifies conflict, whenever newly unmergeable' do
+            expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice
+            expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
+
+            subject.mark_as_unmergeable
+            subject.mark_as_unchecked
+            subject.mark_as_mergeable
+            subject.mark_as_unchecked
+            subject.mark_as_unmergeable
+          end
+
+          it 'does not notify whenever merge request is newly unmergeable due to other reasons' do
+            allow(subject.project.repository).to receive(:can_be_merged?).and_return(true)
 
-      it 'notifies conflict, whenever newly unmergeable' do
-        expect(notification_service).to receive(:merge_request_unmergeable).with(subject).twice
-        expect(todo_service).to receive(:merge_request_became_unmergeable).with(subject).twice
+            expect(notification_service).not_to receive(:merge_request_unmergeable)
+            expect(todo_service).not_to receive(:merge_request_became_unmergeable)
 
-        subject.mark_as_unmergeable
-        subject.mark_as_unchecked
-        subject.mark_as_mergeable
-        subject.mark_as_unchecked
-        subject.mark_as_unmergeable
+            subject.mark_as_unmergeable
+          end
+        end
       end
 
-      it 'does not notify whenever merge request is newly unmergeable due to other reasons' do
-        allow(subject.project.repository).to receive(:can_be_merged?).and_return(true)
+      [:closed, :merged].each do |state|
+        let(:state) { state }
 
-        expect(notification_service).not_to receive(:merge_request_unmergeable)
-        expect(todo_service).not_to receive(:merge_request_became_unmergeable)
+        context state do
+          it 'does not notify' do
+            expect(notification_service).not_to receive(:merge_request_unmergeable)
+            expect(todo_service).not_to receive(:merge_request_became_unmergeable)
 
-        subject.mark_as_unmergeable
+            subject.mark_as_unmergeable
+          end
+        end
       end
     end