Update merge options for auto merge strategiesupdate-auto-merge-parameters

Currently, merge options is updated on #execute method,
however, we should have #update interface to make it explicit.

6 files changed, 93 insertions, 4 deletions

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 135117926be..e1f5f0b7966 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -269,9 +269,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
 
     if auto_merge_requested?
-      AutoMergeService.new(project, current_user, merge_params)
-        .execute(merge_request,
-                 params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+      if merge_request.auto_merge_enabled?
+        # TODO: Ideally, we should have a dedicated endpoint for updating merge params.
+        AutoMergeService.new(project, current_user, merge_params).update(merge_request)
+      else
+        AutoMergeService.new(project, current_user, merge_params)
+          .execute(merge_request,
+                   params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+      end
     else
       @merge_request.merge_async(current_user.id, merge_params)
 
diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb
index 058105db3a4..7611eb8bdfd 100644
--- a/app/services/auto_merge/base_service.rb
+++ b/app/services/auto_merge/base_service.rb
@@ -17,6 +17,14 @@ module AutoMerge
       strategy.to_sym
     end
 
+    def update(merge_request)
+      merge_request.merge_params.merge!(params)
+
+      return :failed unless merge_request.save
+
+      strategy.to_sym
+    end
+
     def cancel(merge_request)
       if cancel_auto_merge(merge_request)
         yield if block_given?
diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb
index a3a780ff388..926d2f5fc66 100644
--- a/app/services/auto_merge_service.rb
+++ b/app/services/auto_merge_service.rb
@@ -24,6 +24,12 @@ class AutoMergeService < BaseService
     service.execute(merge_request)
   end
 
+  def update(merge_request)
+    return :failed unless merge_request.auto_merge_enabled?
+
+    get_service_instance(merge_request.auto_merge_strategy).update(merge_request)
+  end
+
   def process(merge_request)
     return unless merge_request.auto_merge_enabled?
 
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index f8c0ab55eb4..34cbf0c8723 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -412,7 +412,7 @@ describe Projects::MergeRequestsController do
         end
       end
 
-      context 'when the pipeline succeeds is passed' do
+      context 'when merge when pipeline succeeds option is passed' do
         let!(:head_pipeline) do
           create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
         end
@@ -462,6 +462,30 @@ describe Projects::MergeRequestsController do
             expect(json_response).to eq('status' => 'merge_when_pipeline_succeeds')
           end
         end
+
+        context 'when auto merge has not been enabled yet' do
+          it 'calls AutoMergeService#execute' do
+            expect_next_instance_of(AutoMergeService) do |service|
+              expect(service).to receive(:execute).with(merge_request, 'merge_when_pipeline_succeeds')
+            end
+
+            merge_when_pipeline_succeeds
+          end
+        end
+
+        context 'when auto merge has already been enabled' do
+          before do
+            merge_request.update!(auto_merge_enabled: true, merge_user: user)
+          end
+
+          it 'calls AutoMergeService#update' do
+            expect_next_instance_of(AutoMergeService) do |service|
+              expect(service).to receive(:update).with(merge_request)
+            end
+
+            merge_when_pipeline_succeeds
+          end
+        end
       end
 
       describe 'only_allow_merge_if_all_discussions_are_resolved? setting' do
diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb
index 197fa16961d..f99b6d13c63 100644
--- a/spec/services/auto_merge/base_service_spec.rb
+++ b/spec/services/auto_merge/base_service_spec.rb
@@ -82,6 +82,30 @@ describe AutoMerge::BaseService do
     end
   end
 
+  describe '#update' do
+    subject { service.update(merge_request) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+    context 'when merge params are specified' 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 'updates merge params' do
+        expect { subject }.to change {
+          merge_request.reload.merge_params.slice(*params.keys)
+        }.from({}).to(params)
+      end
+    end
+  end
+
   describe '#cancel' do
     subject { service.cancel(merge_request) }
 
diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb
index d0eefed3150..cc7c48bb224 100644
--- a/spec/services/auto_merge_service_spec.rb
+++ b/spec/services/auto_merge_service_spec.rb
@@ -92,6 +92,28 @@ describe AutoMergeService do
     end
   end
 
+  describe '#update' do
+    subject { service.update(merge_request) }
+
+    let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+    it 'delegates to a relevant service instance' do
+      expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service|
+        expect(service).to receive(:update).with(merge_request)
+      end
+
+      subject
+    end
+
+    context 'when auto merge is not enabled' do
+      let(:merge_request) { create(:merge_request) }
+
+      it 'returns failed' do
+        is_expected.to eq(:failed)
+      end
+    end
+  end
+
   describe '#process' do
     subject { service.process(merge_request) }