Merge branch 'make-explicit-endpoint-abort-in-auto-merge' into 'master'

Split AutoMergeService interfaces into two `cancel` and `abort`

Closes #12134

See merge request gitlab-org/gitlab-ee!14403

app/services/auto_merge/base_service.rb

@@ -29,7 +29,7 @@ module AutoMerge
     end
 
     def cancel(merge_request)
-      if cancel_auto_merge(merge_request)
+      if clear_auto_merge_parameters(merge_request)
         yield if block_given?
 
         success
@@ -38,6 +38,16 @@ module AutoMerge
       end
     end
 
+    def abort(merge_request, reason)
+      if clear_auto_merge_parameters(merge_request)
+        yield if block_given?
+
+        success
+      else
+        error("Can't abort the automatic merge", 406)
+      end
+    end
+
     private
 
     def strategy
@@ -46,7 +56,7 @@ module AutoMerge
       end
     end
 
-    def cancel_auto_merge(merge_request)
+    def clear_auto_merge_parameters(merge_request)
       merge_request.auto_merge_enabled = false
       merge_request.merge_user = nil
 

app/services/auto_merge/merge_when_pipeline_succeeds_service.rb

@@ -19,7 +19,13 @@ module AutoMerge
 
     def cancel(merge_request)
       super do
-        SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
+        SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, project, current_user)
+      end
+    end
+
+    def abort(merge_request, reason)
+      super do
+        SystemNoteService.abort_merge_when_pipeline_succeeds(merge_request, project, current_user, reason)
       end
     end
 

app/services/auto_merge_service.rb

@@ -42,6 +42,12 @@ class AutoMergeService < BaseService
     get_service_instance(merge_request.auto_merge_strategy).cancel(merge_request)
   end
 
+  def abort(merge_request, reason)
+    return error("Can't abort the automatic merge", 406) unless merge_request.auto_merge_enabled?
+
+    get_service_instance(merge_request.auto_merge_strategy).abort(merge_request, reason)
+  end
+
   def available_strategies(merge_request)
     self.class.all_strategies.select do |strategy|
       get_service_instance(strategy).available_for?(merge_request)

app/services/merge_requests/base_service.rb

@@ -68,8 +68,8 @@ module MergeRequests
         !merge_request.for_fork?
     end
 
-    def cancel_auto_merge(merge_request)
-      AutoMergeService.new(project, current_user).cancel(merge_request)
+    def abort_auto_merge(merge_request, reason)
+      AutoMergeService.new(project, current_user).abort(merge_request, reason)
     end
 
     # Returns all origin and fork merge requests from `@project` satisfying passed arguments.

app/services/merge_requests/close_service.rb

@@ -18,7 +18,7 @@ module MergeRequests
         invalidate_cache_counts(merge_request, users: merge_request.assignees)
         merge_request.update_project_counter_caches
         cleanup_environments(merge_request)
-        cancel_auto_merge(merge_request)
+        abort_auto_merge(merge_request, 'merge request was closed')
       end
 
       merge_request

app/services/merge_requests/refresh_service.rb

@@ -24,7 +24,7 @@ module MergeRequests
       reload_merge_requests
       outdate_suggestions
       refresh_pipelines_on_merge_requests
-      cancel_auto_merges
+      abort_auto_merges
       mark_pending_todos_done
       cache_merge_requests_closing_issues
 
@@ -142,9 +142,9 @@ module MergeRequests
       end
     end
 
-    def cancel_auto_merges
+    def abort_auto_merges
       merge_requests_for_source_branch.each do |merge_request|
-        cancel_auto_merge(merge_request)
+        abort_auto_merge(merge_request, 'source branch was updated')
       end
     end
 

app/services/merge_requests/update_service.rb

@@ -44,7 +44,7 @@ module MergeRequests
                                   merge_request.previous_changes['target_branch'].first,
                                   merge_request.target_branch)
 
-        cancel_auto_merge(merge_request)
+        abort_auto_merge(merge_request, 'target branch was changed')
       end
 
       if merge_request.assignees != old_assignees

app/services/system_note_service.rb

@@ -234,6 +234,16 @@ module SystemNoteService
     create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
   end
 
+  # Called when 'merge when pipeline succeeds' is aborted
+  def abort_merge_when_pipeline_succeeds(noteable, project, author, reason)
+    body = "aborted the automatic merge because #{reason}"
+
+    ##
+    # TODO: Abort message should be sent by the system, not a particular user.
+    # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63187.
+    create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+  end
+
   def handle_merge_request_wip(noteable, project, author)
     prefix = noteable.work_in_progress? ? "marked" : "unmarked"
 

ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb

@@ -13,22 +13,23 @@ module AutoMerge
 
       merge_train_service = AutoMerge::MergeTrainService.new(project, merge_request.merge_user)
 
-      ##
-      # We are currently abusing `#cancel` method to cancel the auto merge when
-      # a system failure happens. We should split the interfaces into two
-      # for explicitly telling that the cancel action is not triggered by the merge user directly.
-      # https://gitlab.com/gitlab-org/gitlab-ee/issues/12134
-      return cancel(merge_request) unless merge_train_service.available_for?(merge_request)
+      return abort(merge_request) unless merge_train_service.available_for?(merge_request)
 
       merge_train_service.execute(merge_request)
     end
 
     def cancel(merge_request)
-      super(merge_request) do
+      super do
         SystemNoteService.cancel_add_to_merge_train_when_pipeline_succeeds(merge_request, project, current_user)
       end
     end
 
+    def abort(merge_request, reason)
+      super do
+        SystemNoteService.abort_add_to_merge_train_when_pipeline_succeeds(merge_request, project, current_user, reason)
+      end
+    end
+
     def available_for?(merge_request)
       merge_request.project.merge_trains_enabled? &&
         !merge_request.for_fork? &&

ee/app/services/auto_merge/merge_train_service.rb

@@ -18,19 +18,27 @@ module AutoMerge
       ::MergeTrains::RefreshMergeRequestsService.new(project, nil).execute(merge_request)
     end
 
-    def cancel(merge_request, reason: nil, refresh_next: true)
+    def cancel(merge_request)
       # Before dropping a merge request from a merge train, get the next
       # merge request in order to refresh it later.
-      next_merge_request = merge_request.merge_train&.next if refresh_next
+      next_merge_request = merge_request.merge_train&.next
 
-      super(merge_request) do
+      super do
         if merge_request.merge_train&.delete
-          SystemNoteService.cancel_merge_train(merge_request, project, current_user, reason: reason)
+          SystemNoteService.cancel_merge_train(merge_request, project, current_user)
           AutoMergeProcessWorker.perform_async(next_merge_request.id) if next_merge_request
         end
       end
     end
 
+    def abort(merge_request, reason)
+      super do
+        if merge_request.merge_train&.destroy
+          SystemNoteService.abort_merge_train(merge_request, project, current_user, reason)
+        end
+      end
+    end
+
     def available_for?(merge_request)
       return false unless merge_request.project.merge_trains_enabled?
       return false if merge_request.for_fork?

ee/app/services/ee/system_note_service.rb

@@ -204,13 +204,22 @@ module EE
     end
 
     # Called when 'merge train' is canceled
-    def cancel_merge_train(noteable, project, author, reason: nil)
+    def cancel_merge_train(noteable, project, author)
       body = 'removed this merge request from the merge train'
-      body += " because #{reason}" if reason
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
     end
 
+    # Called when 'merge train' is aborted
+    def abort_merge_train(noteable, project, author, reason)
+      body = "removed this merge request from the merge train because #{reason}"
+
+      ##
+      # TODO: Abort message should be sent by the system, not a particular user.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63187.
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
+
     # Called when 'add to merge train when pipeline succeeds' is executed
     def add_to_merge_train_when_pipeline_succeeds(noteable, project, author, sha)
       body = "enabled automatic add to merge train when the pipeline for #{sha} succeeds"
@@ -224,5 +233,15 @@ module EE
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
     end
+
+    # Called when 'add to merge train when pipeline succeeds' is aborted
+    def abort_add_to_merge_train_when_pipeline_succeeds(noteable, project, author, reason)
+      body = "aborted automatic add to merge train because #{reason}"
+
+      ##
+      # TODO: Abort message should be sent by the system, not a particular user.
+      # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63187.
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'merge'))
+    end
   end
 end

ee/app/services/merge_trains/refresh_merge_request_service.rb

@@ -99,7 +99,7 @@ module MergeTrains
 
     def drop(error)
       AutoMerge::MergeTrainService.new(project, merge_user)
-        .cancel(merge_request, reason: error.message, refresh_next: false)
+        .abort(merge_request, error.message)
 
       error(error.message)
     end

ee/spec/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service_spec.rb

@@ -60,8 +60,8 @@ describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do
           allow(AutoMerge::MergeTrainService).to receive(:new) { train_service }
         end
 
-        it 'cancels auto merge' do
-          expect(service).to receive(:cancel).once
+        it 'aborts auto merge' do
+          expect(service).to receive(:abort).once
 
           subject
         end
@@ -93,6 +93,22 @@ describe AutoMerge::AddToMergeTrainWhenPipelineSucceedsService do
     end
   end
 
+  describe '#abort' do
+    subject { service.abort(merge_request, 'an error') }
+
+    let(:merge_request) { create(:merge_request, :add_to_merge_train_when_pipeline_succeeds, merge_user: user) }
+
+    it 'aborts auto merge' do
+      expect(SystemNoteService)
+        .to receive(:abort_add_to_merge_train_when_pipeline_succeeds)
+        .with(merge_request, project, user, 'an error')
+
+      subject
+
+      expect(merge_request).not_to be_auto_merge_enabled
+    end
+  end
+
   describe '#available_for?' do
     subject { service.available_for?(merge_request) }
 

ee/spec/services/auto_merge/merge_train_service_spec.rb

@@ -120,22 +120,11 @@ describe AutoMerge::MergeTrainService do
 
     it 'writes system note to the merge request' do
       expect(SystemNoteService)
-        .to receive(:cancel_merge_train).with(merge_request, project, user, anything)
+        .to receive(:cancel_merge_train).with(merge_request, project, user)
 
       subject
     end
 
-    context 'when reason is specified' do
-      let(:params) { { reason: 'Pipeline failed' } }
-
-      it 'passes the reason to SystemNoteService' do
-        expect(SystemNoteService)
-          .to receive(:cancel_merge_train).with(any_args, reason: 'Pipeline failed')
-
-        subject
-      end
-    end
-
     context 'when the other merge request is following the merge request' do
       let!(:merge_request_2) do
         create(:merge_request, :on_train,
@@ -148,16 +137,36 @@ describe AutoMerge::MergeTrainService do
 
         subject
       end
+    end
+  end
 
-      context 'when refresh next is false' do
-        let(:params) { { refresh_next: false } }
+  describe '#abort' do
+    subject { service.abort(merge_request, 'an error') }
 
-        it 'does not process the next merge request on the train' do
-          expect(AutoMergeProcessWorker).not_to receive(:perform_async).with(merge_request_2.id)
+    let!(:merge_request) do
+      create(:merge_request, :on_train,
+        source_project: project, source_branch: 'feature',
+        target_project: project, target_branch: 'master')
+    end
 
-          subject
-        end
-      end
+    it 'aborts auto merge on the merge request' do
+      subject
+
+      merge_request.reload
+      expect(merge_request).not_to be_auto_merge_enabled
+      expect(merge_request.merge_user).to be_nil
+      expect(merge_request.merge_params).not_to include('should_remove_source_branch')
+      expect(merge_request.merge_params).not_to include('commit_message')
+      expect(merge_request.merge_params).not_to include('squash_commit_message')
+      expect(merge_request.merge_params).not_to include('auto_merge_strategy')
+      expect(merge_request.merge_train).not_to be_present
+    end
+
+    it 'writes system note to the merge request' do
+      expect(SystemNoteService)
+        .to receive(:abort_merge_train).with(merge_request, project, user, 'an error')
+
+      subject
     end
   end
 

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -27,7 +27,7 @@ describe MergeTrains::RefreshMergeRequestService do
 
       it do
         expect_next_instance_of(AutoMerge::MergeTrainService) do |service|
-          expect(service).to receive(:cancel).with(merge_request, hash_including(reason: expected_reason))
+          expect(service).to receive(:abort).with(merge_request, kind_of(String))
         end
 
         subject

ee/spec/services/system_note_service_spec.rb

@@ -349,7 +349,7 @@ describe SystemNoteService do
   end
 
   describe '.cancel_merge_train' do
-    subject { described_class.cancel_merge_train(noteable, project, author, reason: reason) }
+    subject { described_class.cancel_merge_train(noteable, project, author) }
 
     let(:noteable) { create(:merge_request, :on_train, source_project: project, target_project: project) }
     let(:reason) { }
@@ -361,13 +361,20 @@ describe SystemNoteService do
     it "posts the 'merge train' system note" do
       expect(subject.note).to eq('removed this merge request from the merge train')
     end
+  end
 
-    context 'when reason is specified' do
-      let(:reason) { 'merge request is not mergeable' }
+  describe '.abort_merge_train' do
+    subject { described_class.abort_merge_train(noteable, project, author, 'source branch was updated') }
 
-      it "posts the 'merge train' system note" do
-        expect(subject.note).to eq('removed this merge request from the merge train because merge request is not mergeable')
-      end
+    let(:noteable) { create(:merge_request, :on_train, source_project: project, target_project: project) }
+    let(:reason) { }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'merge train' system note" do
+      expect(subject.note).to eq('removed this merge request from the merge train because source branch was updated')
     end
   end
 
@@ -404,4 +411,20 @@ describe SystemNoteService do
       expect(subject.note).to eq 'cancelled automatic add to merge train'
     end
   end
+
+  describe '.abort_add_to_merge_train_when_pipeline_succeeds' do
+    subject { described_class.abort_add_to_merge_train_when_pipeline_succeeds(noteable, project, author, 'target branch was changed') }
+
+    let(:noteable) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'add to merge train when pipeline succeeds' system note" do
+      expect(subject.note).to eq 'aborted automatic add to merge train because target branch was changed'
+    end
+  end
 end

spec/services/auto_merge/base_service_spec.rb

@@ -121,11 +121,7 @@ describe AutoMerge::BaseService do
     end
   end
 
-  describe '#cancel' do
-    subject { service.cancel(merge_request) }
-
-    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
-
+  shared_examples_for 'Canceled or Dropped' do
     it 'removes properies from the merge request' do
       subject
 
@@ -173,6 +169,20 @@ describe AutoMerge::BaseService do
       it 'does not yield block' do
         expect { |b| service.execute(merge_request, &b) }.not_to yield_control
       end
+    end
+  end
+
+  describe '#cancel' do
+    subject { service.cancel(merge_request) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+    it_behaves_like 'Canceled or Dropped'
+
+    context 'when failed to save' do
+      before do
+        allow(merge_request).to receive(:save) { false }
+      end
 
       it 'returns error status' do
         expect(subject[:status]).to eq(:error)
@@ -180,4 +190,24 @@ describe AutoMerge::BaseService do
       end
     end
   end
+
+  describe '#abort' do
+    subject { service.abort(merge_request, reason) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+    let(:reason) { 'an error'}
+
+    it_behaves_like 'Canceled or Dropped'
+
+    context 'when failed to save' do
+      before do
+        allow(merge_request).to receive(:save) { false }
+      end
+
+      it 'returns error status' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq("Can't abort the automatic merge")
+      end
+    end
+  end
 end

spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb

@@ -177,6 +177,17 @@ describe AutoMerge::MergeWhenPipelineSucceedsService do
     end
   end
 
+  describe "#abort" do
+    before do
+      service.abort(mr_merge_if_green_enabled, 'an error')
+    end
+
+    it 'posts a system note' do
+      note = mr_merge_if_green_enabled.notes.last
+      expect(note.note).to include 'aborted the automatic merge'
+    end
+  end
+
   describe 'pipeline integration' do
     context 'when there are multiple stages in the pipeline' do
       let(:ref) { mr_merge_if_green_enabled.source_branch }

spec/services/auto_merge_service_spec.rb

@@ -161,4 +161,29 @@ describe AutoMergeService do
       end
     end
   end
+
+  describe '#abort' do
+    subject { service.abort(merge_request, error) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+    let(:error) { 'an error' }
+
+    it 'delegates to a relevant service instance' do
+      expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service|
+        expect(service).to receive(:abort).with(merge_request, error)
+      end
+
+      subject
+    end
+
+    context 'when auto merge is not enabled' do
+      let(:merge_request) { create(:merge_request) }
+
+      it 'returns error' do
+        expect(subject[:message]).to eq("Can't abort the automatic merge")
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:http_status]).to eq(406)
+      end
+    end
+  end
 end

spec/services/system_note_service_spec.rb

@@ -359,6 +359,22 @@ describe SystemNoteService do
     end
   end
 
+  describe '.abort_merge_when_pipeline_succeeds' do
+    let(:noteable) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
+    subject { described_class.abort_merge_when_pipeline_succeeds(noteable, project, author, 'merge request was closed') }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'merge' }
+    end
+
+    it "posts the 'merge when pipeline succeeds' system note" do
+      expect(subject.note).to eq "aborted the automatic merge because merge request was closed"
+    end
+  end
+
   describe '.change_title' do
     let(:noteable) { create(:issue, project: project, title: 'Lorem ipsum') }