Merge branch '47488-remove-unmergeable-notification-for-no-commits' into 'master'

Notify only merge request unmergeable due to conflict

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

9 files changed, 33 insertions, 70 deletions

diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 5ba3a4a322c..70509e9066d 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/app/mailers/emails/merge_requests.rb
@@ -59,8 +59,6 @@ module Emails
     def merge_request_unmergeable_email(recipient_id, merge_request_id, reason = nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
-      @reasons = MergeRequestPresenter.new(@merge_request, current_user: current_user).unmergeable_reasons
-
       mail_answer_thread(@merge_request, merge_request_thread_options(@merge_request.author_id, recipient_id, reason))
     end
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 324065c1162..4b78ba1029f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -128,8 +128,17 @@ class MergeRequest < ActiveRecord::Base
     end
 
     after_transition unchecked: :cannot_be_merged do |merge_request, transition|
-      NotificationService.new.merge_request_unmergeable(merge_request)
-      TodoService.new.merge_request_became_unmergeable(merge_request)
+      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)
+          NotificationService.new.merge_request_unmergeable(merge_request)
+          TodoService.new.merge_request_became_unmergeable(merge_request)
+        end
+      rescue Gitlab::Git::CommandError
+        # Checking mergeability can trigger exception, e.g. non-utf8
+        # We ignore this type of errors.
+      end
     end
 
     def check_state?(merge_status)
diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb
index 8d466c33510..eb54ab2cda6 100644
--- a/app/presenters/merge_request_presenter.rb
+++ b/app/presenters/merge_request_presenter.rb
@@ -20,17 +20,6 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     end
   end
 
-  def unmergeable_reasons
-    strong_memoize(:unmergeable_reasons) do
-      reasons = []
-      reasons << "no commits" if merge_request.has_no_commits?
-      reasons << "source branch is missing" unless merge_request.source_branch_exists?
-      reasons << "target branch is missing" unless merge_request.target_branch_exists?
-      reasons << "has merge conflicts" unless merge_request.project.repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch)
-      reasons
-    end
-  end
-
   def cancel_merge_when_pipeline_succeeds_path
     if can_cancel_merge_when_pipeline_succeeds?(current_user)
       cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request)
diff --git a/app/views/notify/merge_request_unmergeable_email.html.haml b/app/views/notify/merge_request_unmergeable_email.html.haml
index 578fa1fbce7..7ec0c1ef390 100644
--- a/app/views/notify/merge_request_unmergeable_email.html.haml
+++ b/app/views/notify/merge_request_unmergeable_email.html.haml
@@ -1,6 +1,2 @@
 %p
-  Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to the following #{'reason'.pluralize(@reasons.count)}:
-
-  %ul
-    - @reasons.each do |reason|
-      %li= reason
+  Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to conflict.
diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml
index e4f9f1bf5e7..dcdd6db69d6 100644
--- a/app/views/notify/merge_request_unmergeable_email.text.haml
+++ b/app/views/notify/merge_request_unmergeable_email.text.haml
@@ -1,7 +1,4 @@
-Merge Request #{@merge_request.to_reference} can no longer be merged due to the following #{'reason'.pluralize(@reasons.count)}:
-
-- @reasons.each do |reason|
-  * #{reason}
+Merge Request #{@merge_request.to_reference} can no longer be merged due to conflict.
 
 Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 77543206e13..b7965315eb2 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1413,8 +1413,11 @@ module Gitlab
       end
 
       def can_be_merged?(source_sha, target_branch)
-        target_sha = find_branch(target_branch, true).target
-        !gitaly_conflicts_client(source_sha, target_sha).conflicts?
+        if target_sha = find_branch(target_branch, true)&.target
+          !gitaly_conflicts_client(source_sha, target_sha).conflicts?
+        else
+          false
+        end
       end
 
       def search_files_by_name(query, ref)
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 775ca4ba0eb..a9a45367b4a 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -416,16 +416,10 @@ describe Notify do
         end
 
         it 'has the correct subject and body' do
-          reasons = %w[foo bar]
-
-          allow_any_instance_of(MergeRequestPresenter).to receive(:unmergeable_reasons).and_return(reasons)
           aggregate_failures do
             is_expected.to have_referable_subject(merge_request, reply: true)
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
-            is_expected.to have_body_text('following reasons:')
-            reasons.each do |reason|
-              is_expected.to have_body_text(reason)
-            end
+            is_expected.to have_body_text('due to conflict.')
           end
         end
       end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 3f028b3bd5c..7ae70c3afb4 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1324,6 +1324,7 @@ describe MergeRequest do
       context 'when broken' do
         before do
           allow(subject).to receive(:broken?) { true }
+          allow(project.repository).to receive(:can_be_merged?).and_return(false)
         end
 
         it 'becomes unmergeable' do
@@ -2150,9 +2151,11 @@ describe MergeRequest do
       before do
         allow(NotificationService).to receive(:new).and_return(notification_service)
         allow(TodoService).to receive(:new).and_return(todo_service)
+
+        allow(subject.project.repository).to receive(:can_be_merged?).and_return(false)
       end
 
-      it 'notifies, but does not notify again if rechecking still results in cannot_be_merged' do
+      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
 
@@ -2161,7 +2164,7 @@ describe MergeRequest do
         subject.mark_as_unmergeable
       end
 
-      it 'notifies whenever merge request is newly unmergeable' do
+      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
 
@@ -2171,6 +2174,15 @@ describe MergeRequest do
         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)
+
+        expect(notification_service).not_to receive(:merge_request_unmergeable)
+        expect(todo_service).not_to receive(:merge_request_became_unmergeable)
+
+        subject.mark_as_unmergeable
+      end
     end
 
     describe 'check_state?' do
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index d5fb4a7742c..e3b37739e8e 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/spec/presenters/merge_request_presenter_spec.rb
@@ -70,41 +70,6 @@ describe MergeRequestPresenter do
     end
   end
 
-  describe "#unmergeable_reasons" do
-    let(:presenter) { described_class.new(resource, current_user: user) }
-
-    before do
-      # Mergeable base state
-      allow(resource).to receive(:has_no_commits?).and_return(false)
-      allow(resource).to receive(:source_branch_exists?).and_return(true)
-      allow(resource).to receive(:target_branch_exists?).and_return(true)
-      allow(resource.project.repository).to receive(:can_be_merged?).and_return(true)
-    end
-
-    it "handles mergeable request" do
-      expect(presenter.unmergeable_reasons).to eq([])
-    end
-
-    it "handles no commit" do
-      allow(resource).to receive(:has_no_commits?).and_return(true)
-
-      expect(presenter.unmergeable_reasons).to eq(["no commits"])
-    end
-
-    it "handles branches missing" do
-      allow(resource).to receive(:source_branch_exists?).and_return(false)
-      allow(resource).to receive(:target_branch_exists?).and_return(false)
-
-      expect(presenter.unmergeable_reasons).to eq(["source branch is missing", "target branch is missing"])
-    end
-
-    it "handles merge conflict" do
-      allow(resource.project.repository).to receive(:can_be_merged?).and_return(false)
-
-      expect(presenter.unmergeable_reasons).to eq(["has merge conflicts"])
-    end
-  end
-
   describe '#conflict_resolution_path' do
     let(:project) { create :project }
     let(:user) { create :user }