Automatic merge of gitlab-org/gitlab-ce master

app/models/merge_request.rb

@@ -986,21 +986,6 @@ class MergeRequest < ApplicationRecord
     end
   end
 
-  def reset_auto_merge
-    return unless auto_merge_enabled?
-
-    self.auto_merge_enabled = false
-    self.merge_user = nil
-    if merge_params
-      merge_params.delete('should_remove_source_branch')
-      merge_params.delete('commit_message')
-      merge_params.delete('squash_commit_message')
-      merge_params.delete('auto_merge_strategy')
-    end
-
-    self.save
-  end
-
   # Return array of possible target branches
   # depends on target project of MR
   def target_branches

app/services/auto_merge/base_service.rb

+# frozen_string_literal: true
+
+module AutoMerge
+  class BaseService < ::BaseService
+    include Gitlab::Utils::StrongMemoize
+
+    def execute(merge_request)
+      merge_request.merge_params.merge!(params)
+      merge_request.auto_merge_enabled = true
+      merge_request.merge_user = current_user
+      merge_request.auto_merge_strategy = strategy
+
+      return :failed unless merge_request.save
+
+      yield if block_given?
+
+      strategy.to_sym
+    end
+
+    def cancel(merge_request)
+      if cancel_auto_merge(merge_request)
+        yield if block_given?
+
+        success
+      else
+        error("Can't cancel the automatic merge", 406)
+      end
+    end
+
+    private
+
+    def strategy
+      strong_memoize(:strategy) do
+        self.class.name.demodulize.remove('Service').underscore
+      end
+    end
+
+    def cancel_auto_merge(merge_request)
+      merge_request.auto_merge_enabled = false
+      merge_request.merge_user = nil
+
+      merge_request.merge_params&.except!(
+        'should_remove_source_branch',
+        'commit_message',
+        'squash_commit_message',
+        'auto_merge_strategy'
+      )
+
+      merge_request.save
+    end
+  end
+end

app/services/auto_merge/merge_when_pipeline_succeeds_service.rb

 # frozen_string_literal: true
 
 module AutoMerge
-  class MergeWhenPipelineSucceedsService < BaseService
+  class MergeWhenPipelineSucceedsService < AutoMerge::BaseService
     def execute(merge_request)
-      return :failed unless merge_request.actual_head_pipeline
-
-      if merge_request.actual_head_pipeline.active?
-        merge_request.merge_params.merge!(params)
-
-        unless merge_request.auto_merge_enabled?
-          merge_request.auto_merge_enabled = true
-          merge_request.merge_user = @current_user
-          merge_request.auto_merge_strategy = AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS
-
-          SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
+      super do
+        if merge_request.saved_change_to_auto_merge_enabled?
+          SystemNoteService.merge_when_pipeline_succeeds(merge_request, project, current_user, merge_request.diff_head_commit)
         end
-
-        return :failed unless merge_request.save
-
-        :merge_when_pipeline_succeeds
-      elsif merge_request.actual_head_pipeline.success?
-        # This can be triggered when a user clicks the auto merge button while
-        # the tests finish at about the same time
-        merge_request.merge_async(current_user.id, merge_params)
-
-        :success
-      else
-        :failed
       end
     end
 
@@ -38,12 +18,8 @@ module AutoMerge
     end
 
     def cancel(merge_request)
-      if merge_request.reset_auto_merge
+      super do
         SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
-
-        success
-      else
-        error("Can't cancel the automatic merge", 406)
       end
     end
 

spec/models/merge_request_spec.rb

@@ -1086,22 +1086,6 @@ describe MergeRequest do
     end
   end
 
-  describe "#reset_auto_merge" do
-    let(:merge_if_green) do
-      create :merge_request, merge_when_pipeline_succeeds: true, merge_user: create(:user),
-                             merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" }
-    end
-
-    it "sets the item to false" do
-      merge_if_green.reset_auto_merge
-      merge_if_green.reload
-
-      expect(merge_if_green.merge_when_pipeline_succeeds).to be_falsey
-      expect(merge_if_green.merge_params["should_remove_source_branch"]).to be_nil
-      expect(merge_if_green.merge_params["commit_message"]).to be_nil
-    end
-  end
-
   describe '#committers' do
     it 'returns all the committers of every commit in the merge request' do
       users = subject.commits.without_merge_commits.map(&:committer_email).uniq.map do |email|

spec/services/auto_merge/base_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe AutoMerge::BaseService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(project, user, params) }
+  let(:merge_request) { create(:merge_request) }
+  let(:params) { {} }
+
+  describe '#execute' do
+    subject { service.execute(merge_request) }
+
+    it 'sets properies to the merge request' do
+      subject
+
+      merge_request.reload
+      expect(merge_request).to be_auto_merge_enabled
+      expect(merge_request.merge_user).to eq(user)
+      expect(merge_request.auto_merge_strategy).to eq('base')
+    end
+
+    it 'yields block' do
+      expect { |b| service.execute(merge_request, &b) }.to yield_control.once
+    end
+
+    it 'returns activated strategy name' do
+      is_expected.to eq(:base)
+    end
+
+    context 'when merge parameters are given' do
+      let(:params) do
+        {
+          'commit_message' => "Merge branch 'patch-12' into 'master'",
+          'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18",
+          'should_remove_source_branch' => false,
+          'squash' => false,
+          'squash_commit_message' => "Update README.md"
+        }
+      end
+
+      it 'sets merge parameters' do
+        subject
+
+        merge_request.reload
+        expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'")
+        expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18')
+        expect(merge_request.merge_params['should_remove_source_branch']).to eq(false)
+        expect(merge_request.merge_params['squash']).to eq(false)
+        expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md')
+      end
+    end
+
+    context 'when strategy is merge when pipeline succeeds' do
+      let(:service) { AutoMerge::MergeWhenPipelineSucceedsService.new(project, user) }
+
+      it 'sets the auto merge strategy' do
+        subject
+
+        merge_request.reload
+        expect(merge_request.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+      end
+
+      it 'returns activated strategy name' do
+        is_expected.to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS.to_sym)
+      end
+    end
+
+    context 'when failed to save' do
+      before do
+        allow(merge_request).to receive(:save) { false }
+      end
+
+      it 'does not yield block' do
+        expect { |b| service.execute(merge_request, &b) }.not_to yield_control
+      end
+
+      it 'returns failed' do
+        is_expected.to eq(:failed)
+      end
+    end
+  end
+
+  describe '#cancel' do
+    subject { service.cancel(merge_request) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+    it 'removes properies from 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.auto_merge_strategy).to be_nil
+    end
+
+    it 'yields block' do
+      expect { |b| service.cancel(merge_request, &b) }.to yield_control.once
+    end
+
+    it 'returns success status' do
+      expect(subject[:status]).to eq(:success)
+    end
+
+    context 'when merge params are set' do
+      before do
+        merge_request.update!(merge_params:
+          {
+            'should_remove_source_branch' => false,
+            'commit_message' => "Merge branch 'patch-12' into 'master'",
+            'squash_commit_message' => "Update README.md",
+            'auto_merge_strategy' => 'merge_when_pipeline_succeeds'
+          })
+      end
+
+      it 'removes merge parameters' do
+        subject
+
+        merge_request.reload
+        expect(merge_request.merge_params['should_remove_source_branch']).to be_nil
+        expect(merge_request.merge_params['commit_message']).to be_nil
+        expect(merge_request.merge_params['squash_commit_message']).to be_nil
+        expect(merge_request.merge_params['auto_merge_strategy']).to be_nil
+      end
+    end
+
+    context 'when failed to save' do
+      before do
+        allow(merge_request).to receive(:save) { false }
+      end
+
+      it 'does not yield block' do
+        expect { |b| service.execute(merge_request, &b) }.not_to yield_control
+      end
+
+      it 'returns error status' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq("Can't cancel the automatic merge")
+      end
+    end
+  end
+end