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

MR unmergeable notification

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

10 files changed, 238 insertions, 60 deletions

diff --git a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb b/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb
index 2d7647a6e12..397cc79bde4 100644
--- a/spec/controllers/projects/merge_requests/conflicts_controller_spec.rb
+++ b/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
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c8cc6b374f6..d3042be9e8b 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/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
diff --git a/spec/features/merge_request/user_resolves_conflicts_spec.rb b/spec/features/merge_request/user_resolves_conflicts_spec.rb
index 25a57f7d379..59aa90fc86f 100644
--- a/spec/features/merge_request/user_resolves_conflicts_spec.rb
+++ b/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
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 8a52c151cc4..f310a6854d5 100644
--- a/spec/mailers/notify_spec.rb
+++ b/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) }
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 04379e7d2c3..92e33a64d26 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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) }
+    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
 
-    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)
+        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 }
+        end
 
-    context 'when broken' do
-      before do
-        allow(subject).to receive(:broken?) { true }
+        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
+    context 'when merge_status is unchecked' do
+      subject { create(:merge_request, source_project: project, merge_status: :unchecked) }
 
-      it 'becomes unmergeable' do
-        expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged')
-      end
+      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
diff --git a/spec/presenters/merge_request_presenter_spec.rb b/spec/presenters/merge_request_presenter_spec.rb
index e3b37739e8e..d5fb4a7742c 100644
--- a/spec/presenters/merge_request_presenter_spec.rb
+++ b/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 }
diff --git a/spec/services/merge_requests/conflicts/list_service_spec.rb b/spec/services/merge_requests/conflicts/list_service_spec.rb
index 837b8a56d12..d57852615d9 100644
--- a/spec/services/merge_requests/conflicts/list_service_spec.rb
+++ b/spec/services/merge_requests/conflicts/list_service_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe MergeRequests::Conflicts::ListService do
   describe '#can_be_resolved_in_ui?' do
     def create_merge_request(source_branch)
-      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
+      create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start', merge_status: :unchecked) do |mr|
         mr.mark_as_unmergeable
       end
     end
diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
index 240aa638f79..8838742a637 100644
--- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
+++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb
@@ -112,32 +112,6 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do
         service.trigger(unrelated_pipeline)
       end
     end
-
-    context 'when the merge request is not mergeable' do
-      let(:mr_conflict) do
-        create(:merge_request, merge_when_pipeline_succeeds: true, merge_user: user,
-                               source_branch: 'master', target_branch: 'feature-conflict',
-                               source_project: project, target_project: project)
-      end
-
-      let(:conflict_pipeline) do
-        create(:ci_pipeline, project: project, ref: mr_conflict.source_branch,
-                             sha: mr_conflict.diff_head_sha, status: 'success',
-                             head_pipeline_of: mr_conflict)
-      end
-
-      it 'does not merge the merge request' do
-        expect(MergeWorker).not_to receive(:perform_async)
-
-        service.trigger(conflict_pipeline)
-      end
-
-      it 'creates todos for unmergeability' do
-        expect_any_instance_of(TodoService).to receive(:merge_request_became_unmergeable).with(mr_conflict)
-
-        service.trigger(conflict_pipeline)
-      end
-    end
   end
 
   describe "#cancel" do
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 5f28bc123f3..831678b949d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1224,6 +1224,32 @@ describe NotificationService, :mailer do
       end
     end
 
+    describe '#merge_request_unmergeable' do
+      it "sends email to merge request author" do
+        notification.merge_request_unmergeable(merge_request)
+
+        should_email(merge_request.author)
+        expect(email_recipients.size).to eq(1)
+      end
+
+      describe 'when merge_when_pipeline_succeeds is true' do
+        before do
+          merge_request.update_attributes(
+            merge_when_pipeline_succeeds: true,
+            merge_user: create(:user)
+          )
+        end
+
+        it "sends email to merge request author and merge_user" do
+          notification.merge_request_unmergeable(merge_request)
+
+          should_email(merge_request.author)
+          should_email(merge_request.merge_user)
+          expect(email_recipients.size).to eq(2)
+        end
+      end
+    end
+
     describe '#closed_merge_request' do
       before do
         update_custom_notification(:close_merge_request, @u_guest_custom, resource: project)
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 562b89e6767..9a51c873b30 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -721,17 +721,18 @@ describe TodoService do
     end
 
     describe '#merge_request_build_failed' do
-      it 'creates a pending todo for the merge request author' do
-        service.merge_request_build_failed(mr_unassigned)
+      let(:merge_participants) { [mr_unassigned.author, admin] }
 
-        should_create_todo(user: author, target: mr_unassigned, action: Todo::BUILD_FAILED)
+      before do
+        allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants)
       end
 
-      it 'creates a pending todo for merge_user' do
-        mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin)
+      it 'creates a pending todo for each merge_participant' do
         service.merge_request_build_failed(mr_unassigned)
 
-        should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::BUILD_FAILED)
+        merge_participants.each do |participant|
+          should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::BUILD_FAILED)
+        end
       end
     end
 
@@ -747,11 +748,19 @@ describe TodoService do
     end
 
     describe '#merge_request_became_unmergeable' do
-      it 'creates a pending todo for a merge_user' do
+      let(:merge_participants) { [admin, create(:user)] }
+
+      before do
+        allow(mr_unassigned).to receive(:merge_participants).and_return(merge_participants)
+      end
+
+      it 'creates a pending todo for each merge_participant' do
         mr_unassigned.update(merge_when_pipeline_succeeds: true, merge_user: admin)
         service.merge_request_became_unmergeable(mr_unassigned)
 
-        should_create_todo(user: admin, author: admin, target: mr_unassigned, action: Todo::UNMERGEABLE)
+        merge_participants.each do |participant|
+          should_create_todo(user: participant, author: participant, target: mr_unassigned, action: Todo::UNMERGEABLE)
+        end
       end
     end