Merge branch 'mr-conflict-notification' into 'master'

MR unmergeable notification

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

app/mailers/emails/merge_requests.rb

@@ -56,6 +56,14 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
+    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
+
     def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id, reason = nil)
       setup_merge_request_mail(merge_request_id, recipient_id)

app/models/merge_request.rb

@@ -104,24 +104,35 @@ class MergeRequest < ActiveRecord::Base
   state_machine :merge_status, initial: :unchecked do
     event :mark_as_unchecked do
-      transition [:can_be_merged, :cannot_be_merged] => :unchecked
+      transition [:can_be_merged, :unchecked] => :unchecked
+      transition [:cannot_be_merged, :cannot_be_merged_recheck] => :cannot_be_merged_recheck
     end
     event :mark_as_mergeable do
-      transition [:unchecked, :cannot_be_merged] => :can_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck] => :can_be_merged
     end
     event :mark_as_unmergeable do
-      transition [:unchecked, :can_be_merged] => :cannot_be_merged
+      transition [:unchecked, :cannot_be_merged_recheck] => :cannot_be_merged
     end
     state :unchecked
+    state :cannot_be_merged_recheck
     state :can_be_merged
     state :cannot_be_merged
     around_transition do |merge_request, transition, block|
       Gitlab::Timeless.timeless(merge_request, &block)
     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)
+    end
+
+    def check_state?(merge_status)
+      [:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym)
+    end
   end
   validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?]
@@ -327,6 +338,16 @@ class MergeRequest < ActiveRecord::Base
     update_column(:merge_jid, jid)
   end
+  def merge_participants
+    participants = [author]
+
+    if merge_when_pipeline_succeeds? && !participants.include?(merge_user)
+      participants << merge_user
+    end
+
+    participants
+  end
+
   def first_commit
     merge_request_diff ? merge_request_diff.first_commit : compare_commits.first
   end
@@ -602,7 +623,7 @@ class MergeRequest < ActiveRecord::Base
   end
   def check_if_can_be_merged
-    return unless unchecked? && Gitlab::Database.read_write?
+    return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write?
     can_be_merged =
       !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch)

app/models/project_services/drone_ci_service.rb

@@ -115,6 +115,6 @@ class DroneCiService < CiService
   def merge_request_valid?(data)
     data[:object_attributes][:state] == 'opened' &&
-      data[:object_attributes][:merge_status] == 'unchecked'
+      MergeRequest.state_machines[:merge_status].check_state?(data[:object_attributes][:merge_status])
   end
 end

app/presenters/merge_request_presenter.rb

@@ -20,6 +20,17 @@ 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)

app/services/merge_requests/merge_when_pipeline_succeeds_service.rb

@@ -24,11 +24,7 @@ module MergeRequests
       pipeline_merge_requests(pipeline) do |merge_request|
         next unless merge_request.merge_when_pipeline_succeeds?
-
-        unless merge_request.mergeable?
-          todo_service.merge_request_became_unmergeable(merge_request)
-          next
-        end
+        next unless merge_request.mergeable?
         merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
       end

app/services/notification_recipient_service.rb

@@ -18,6 +18,10 @@ module NotificationRecipientService
     Builder::NewNote.new(*a).notification_recipients
   end
+  def self.build_merge_request_unmergeable_recipients(*a)
+    Builder::MergeRequestUnmergeable.new(*a).notification_recipients
+  end
+
   module Builder
     class Base
       def initialize(*)
@@ -330,5 +334,26 @@ module NotificationRecipientService
         note.author
       end
     end
+
+    class MergeRequestUnmergeable < Base
+      attr_reader :target
+      def initialize(merge_request)
+        @target = merge_request
+      end
+
+      def build!
+        target.merge_participants.each do |user|
+          add_recipients(user, :participating, nil)
+        end
+      end
+
+      def custom_action
+        :unmergeable_merge_request
+      end
+
+      def acting_user
+        nil
+      end
+    end
   end
 end

app/services/notification_service.rb

@@ -129,6 +129,7 @@ class NotificationService
   # When create a merge request we should send an email to:
   #
+  #  * mr author
   #  * mr assignee if their notification level is not Disabled
   #  * project team members with notification level higher then Participating
   #  * watchers of the mr's labels
@@ -148,6 +149,15 @@ class NotificationService
     end
   end
+  # When a merge request is found to be unmergeable, we should send an email to:
+  #
+  #  * mr author
+  #  * mr merge user if set
+  #
+  def merge_request_unmergeable(merge_request)
+    merge_request_unmergeable_email(merge_request)
+  end
+
   # When merge request text is updated, we should send an email to:
   #
   #  * newly mentioned project team members with notification level higher than Participating
@@ -484,6 +494,14 @@ class NotificationService
     end
   end
+  def merge_request_unmergeable_email(merge_request)
+    recipients = NotificationRecipientService.build_merge_request_unmergeable_recipients(merge_request)
+
+    recipients.each do |recipient|
+      mailer.merge_request_unmergeable_email(recipient.user.id, merge_request.id).deliver_later
+    end
+  end
+
   def mailer
     Notify
   end

app/services/todo_service.rb

@@ -98,12 +98,12 @@ class TodoService
   # When a build fails on the HEAD of a merge request we should:
   #
-  #  * create a todo for author of MR to fix it
-  #  * create a todo for merge_user to keep an eye on it
+  #  * create a todo for each merge participant
   #
   def merge_request_build_failed(merge_request)
-    create_build_failed_todo(merge_request, merge_request.author)
-    create_build_failed_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
+    merge_request.merge_participants.each do |user|
+      create_build_failed_todo(merge_request, user)
+    end
   end
   # When a new commit is pushed to a merge request we should:
@@ -116,20 +116,22 @@ class TodoService
   # When a build is retried to a merge request we should:
   #
-  #  * mark all pending todos related to the merge request for the author as done
-  #  * mark all pending todos related to the merge request for the merge_user as done
+  #  * mark all pending todos related to the merge request as done for each merge participant
   #
   def merge_request_build_retried(merge_request)
-    mark_pending_todos_as_done(merge_request, merge_request.author)
-    mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
+    merge_request.merge_participants.each do |user|
+      mark_pending_todos_as_done(merge_request, user)
+    end
   end
-  # When a merge request could not be automatically merged due to its unmergeable state we should:
+  # When a merge request could not be merged due to its unmergeable state we should:
   #
-  #  * create a todo for a merge_user
+  #  * create a todo for each merge participant
   #
   def merge_request_became_unmergeable(merge_request)
-    create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
+    merge_request.merge_participants.each do |user|
+      create_unmergeable_todo(merge_request, user)
+    end
   end
   # When create a note we should:
@@ -275,9 +277,9 @@ class TodoService
     create_todos(todo_author, attributes)
   end
-  def create_unmergeable_todo(merge_request, merge_user)
-    attributes = attributes_for_todo(merge_request.project, merge_request, merge_user, Todo::UNMERGEABLE)
-    create_todos(merge_user, attributes)
+  def create_unmergeable_todo(merge_request, todo_author)
+    attributes = attributes_for_todo(merge_request.project, merge_request, todo_author, Todo::UNMERGEABLE)
+    create_todos(todo_author, attributes)
   end
   def attributes_for_target(target)

app/views/notify/merge_request_unmergeable_email.html.haml

+%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 #{pluralize(@reasons, 'reason')}:
+
+  %ul
+    - @reasons.each do |reason|
+      %li= reason

app/views/notify/merge_request_unmergeable_email.text.haml

+Merge Request #{@merge_request.to_reference} can no longer be merged due to the following #{pluralize(@reasons, 'reason')}:
+
+- @reasons.each do |reason|
+  * #{reason}
+
+Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+
+= merge_path_description(@merge_request, 'to')
+
+Author: #{@merge_request.author_name}
+Assignee: #{@merge_request.assignee_name}

changelogs/unreleased/mr-conflict-notification.yml

+---
+title: When MR becomes unmergeable, notify and create todo for author and merge user
+merge_request: 18042
+author:
+type: added

doc/workflow/merge_requests.md

-This document was moved to [user/project/merge_requests](../user/project/merge_requests.md).
+This document was moved to [user/project/merge_requests/index.md](../user/project/merge_requests/index.md).

doc/workflow/notifications.md

@@ -106,6 +106,10 @@ 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 a user has also set the merge request to automatically merge once pipeline succeeds,
+then that user will also be notified.
+
 ### Email Headers
 Notification emails include headers that provide extra content about the notification received:

doc/workflow/todos.md

@@ -31,6 +31,9 @@ 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:
+  - the author, or
+  - have set it to automatically merge once pipeline succeeds.
 ### Directly addressed Todos

spec/controllers/projects/merge_requests/conflicts_controller_spec.rb

@@ -5,7 +5,7 @@ describe Projects::MergeRequests::ConflictsController do
   let(:user)    { project.owner }
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
   let(:merge_request_with_conflicts) do
-    create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr|
+    create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr|
       mr.mark_as_unmergeable
     end
   end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -7,7 +7,7 @@ describe Projects::MergeRequestsController do
   let(:user)    { project.owner }
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
   let(:merge_request_with_conflicts) do
-    create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project) do |mr|
+    create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr|
       mr.mark_as_unmergeable
     end
   end

spec/features/merge_request/user_resolves_conflicts_spec.rb

@@ -10,7 +10,7 @@ describe 'Merge request > User resolves conflicts', :js do
   end
   def create_merge_request(source_branch)
-    create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project) do |mr|
+    create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', source_project: project, merge_status: :unchecked) do |mr|
       mr.mark_as_unmergeable
     end
   end

spec/mailers/notify_spec.rb

@@ -390,6 +390,46 @@ describe Notify do
         end
       end
+      describe 'that are unmergeable' do
+        set(:merge_request) do
+          create(:merge_request, :conflict,
+                 source_project: project,
+                 target_project: project,
+                 author: current_user,
+                 assignee: assignee,
+                 description: 'Awesome description')
+        end
+
+        subject { described_class.merge_request_unmergeable_email(recipient.id, merge_request.id) }
+
+        it_behaves_like 'a multiple recipients email'
+        it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+          let(:model) { merge_request }
+        end
+        it_behaves_like 'it should show Gmail Actions View Merge request link'
+        it_behaves_like 'an unsubscribeable thread'
+
+        it 'is sent as the merge request author' do
+          sender = subject.header[:from].addrs[0]
+          expect(sender.display_name).to eq(merge_request.author.name)
+          expect(sender.address).to eq(gitlab_sender)
+        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('reasons:')
+            reasons.each do |reason|
+              is_expected.to have_body_text(reason)
+            end
+          end
+        end
+      end
+
       shared_examples 'a push to an existing merge request' do
         let(:push_user) { create(:user) }

spec/models/merge_request_spec.rb

@@ -1245,38 +1245,50 @@ describe MergeRequest do
   describe '#check_if_can_be_merged' do
     let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
-    subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
-    context 'when it is not broken and has no conflicts' do
-      before do
-        allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?).and_return(true)
+    shared_examples 'checking if can be merged' do
+      context 'when it is not broken and has no conflicts' do
+        before do
+          allow(subject).to receive(:broken?) { false }
+          allow(project.repository).to receive(:can_be_merged?).and_return(true)
+        end
+        it 'is marked as mergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
+        end
       end
-      it 'is marked as mergeable' do
-        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
-      end
-    end
-    context 'when broken' do
-      before do
-        allow(subject).to receive(:broken?) { true }
+      context 'when broken' do
+        before do
+          allow(subject).to receive(:broken?) { true }
+        end
+        it 'becomes unmergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+        end
       end
-      it 'becomes unmergeable' do
-        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+      context 'when it has conflicts' do
+        before do
+          allow(subject).to receive(:broken?) { false }
+          allow(project.repository).to receive(:can_be_merged?).and_return(false)
+        end
+
+        it 'becomes unmergeable' do
+          expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
+        end
       end
     end
-    context 'when it has conflicts' do
-      before do
-        allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?).and_return(false)
-      end
-      it 'becomes unmergeable' do
-        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
-      end
+    context 'when merge_status is unchecked' do
+      subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
+      it_behaves_like 'checking if can be merged'
+    end
+
+    context 'when merge_status is unchecked' do
+      subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) }
+
+      it_behaves_like 'checking if can be merged'
     end
   end
@@ -2064,6 +2076,53 @@ describe MergeRequest do
         expect(subject.merge_jid).to be_nil
       end
     end
+
+    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) }
+
+      before do
+        allow(NotificationService).to receive(:new).and_return(notification_service)
+        allow(TodoService).to receive(:new).and_return(todo_service)
+      end
+
+      it 'notifies, 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 whenever merge request is 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
+    end
+
+    describe 'check_state?' do
+      it 'indicates whether MR is still checking for mergeability' do
+        state_machine = described_class.state_machines[:merge_status]
+        check_states = [:unchecked, :cannot_be_merged_recheck]
+
+        check_states.each do |merge_status|
+          expect(state_machine.check_state?(merge_status)).to be true
+        end
+
+        (state_machine.states.map(&:name) - check_states).each do |merge_status|
+          expect(state_machine.check_state?(merge_status)).to be false
+        end
+      end
+    end
   end
   describe '#should_be_rebased?' do
@@ -2195,4 +2254,39 @@ describe MergeRequest do
       expect(merge_request.can_allow_maintainer_to_push?(user)).to be_truthy
     end
   end
+
+  describe '#merge_participants' do
+    it 'contains author' do
+      expect(subject.merge_participants).to eq([subject.author])
+    end
+
+    describe 'when merge_when_pipeline_succeeds? is true' do
+      describe 'when merge user is author' do
+        let(:user) { create(:user) }
+        subject do
+          create(:merge_request,
+                 merge_when_pipeline_succeeds: true,
+                 merge_user: user,
+                 author: user)
+        end
+
+        it 'contains author only' do
+          expect(subject.merge_participants).to eq([subject.author])
+        end
+      end
+
+      describe 'when merge user and author are different users' do
+        let(:merge_user) { create(:user) }
+        subject do
+          create(:merge_request,
+                 merge_when_pipeline_succeeds: true,
+                 merge_user: merge_user)
+        end
+
+        it 'contains author and merge user' do
+          expect(subject.merge_participants).to eq([subject.author, merge_user])
+        end
+      end
+    end
+  end
 end

spec/presenters/merge_request_presenter_spec.rb

@@ -70,6 +70,41 @@ 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 }