Add cannot_be_merged_recheck merge_status

First, transitions between can_be_merged & cannot_be_merged are removed,
as they are currently blocked in `check_if_can_be_merged`.
`can_be_merge` always returns to `unchecked` first,
before it can transition to `cannot_be_merged` (and vice versa).

We want to avoid repeated notification triggered by repeated transition
between `cannot_be_merged` & `unchecked`.

So we added `cannot_be_merged_recheck` state, similar to `unchecked`,
but as a mean to remember it’s from cannot_be_merged.

See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18042/#note_65945407

Since `unchecked` and `cannot_be_merged_recheck` both mean
“we are in the middle of checking if it is mergeable”,
quite often we need to see if merge_status is in either one of them,
so `check_state?` is added to achieve this.

7 files changed, 62 insertions, 9 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 628c61d5d69..ea5057c2a78 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -104,24 +104,30 @@ 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
+      transition [:cannot_be_merged] => :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
+
+    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?]
@@ -602,7 +608,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)
diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb
index 71b10fc6bc1..a4bf427ac0b 100644
--- a/app/models/project_services/drone_ci_service.rb
+++ b/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
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 19995559fae..ac3c734599c 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/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 04379e7d2c3..bcc836acefa 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2064,6 +2064,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
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