Merge branch 'abstract-auto-merge' into 'master'

Refactor and abstract Auto Merge Processes

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

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merge_when_pipeline_succeeds.vue

@@ -57,7 +57,7 @@ export default {
     removeSourceBranch() {
       const options = {
         sha: this.mr.sha,
-        merge_when_pipeline_succeeds: true,
+        auto_merge_strategy: 'merge_when_pipeline_succeeds',
         should_remove_source_branch: true,
       };
@@ -85,7 +85,7 @@ export default {
       <h4 class="d-flex align-items-start">
         <span class="append-right-10">
           {{ s__('mrWidget|Set by') }}
-          <mr-widget-author :author="mr.setToMWPSBy" />
+          <mr-widget-author :author="mr.setToAutoMergeBy" />
           {{ s__('mrWidget|to be merged automatically when the pipeline succeeds') }}
         </span>
         <a

app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue

@@ -31,7 +31,7 @@ export default {
     return {
       removeSourceBranch: this.mr.shouldRemoveSourceBranch,
       mergeWhenBuildSucceeds: false,
-      setToMergeWhenPipelineSucceeds: false,
+      autoMergeStrategy: undefined,
       isMakingRequest: false,
       isMergingImmediately: false,
       commitMessage: this.mr.commitMessage,
@@ -42,7 +42,7 @@ export default {
     };
   },
   computed: {
-    shouldShowMergeWhenPipelineSucceedsText() {
+    shouldShowAutoMergeText() {
       return this.mr.isPipelineActive;
     },
     status() {
@@ -87,7 +87,7 @@ export default {
     mergeButtonText() {
       if (this.isMergingImmediately) {
         return __('Merge in progress');
-      } else if (this.shouldShowMergeWhenPipelineSucceedsText) {
+      } else if (this.shouldShowAutoMergeText) {
         return __('Merge when pipeline succeeds');
       }
@@ -104,7 +104,7 @@ export default {
       return enableSquashBeforeMerge && commitsCount > 1;
     },
     shouldShowMergeControls() {
-      return this.mr.isMergeAllowed || this.shouldShowMergeWhenPipelineSucceedsText;
+      return this.mr.isMergeAllowed || this.shouldShowAutoMergeText;
     },
     shouldShowSquashEdit() {
       return this.squashBeforeMerge && this.shouldShowSquashBeforeMerge;
@@ -126,12 +126,12 @@ export default {
         this.isMergingImmediately = true;
       }
-      this.setToMergeWhenPipelineSucceeds = mergeWhenBuildSucceeds === true;
+      this.autoMergeStrategy = mergeWhenBuildSucceeds ? 'merge_when_pipeline_succeeds' : undefined;
       const options = {
         sha: this.mr.sha,
         commit_message: this.commitMessage,
-        merge_when_pipeline_succeeds: this.setToMergeWhenPipelineSucceeds,
+        auto_merge_strategy: this.autoMergeStrategy,
         should_remove_source_branch: this.removeSourceBranch === true,
         squash: this.squashBeforeMerge,
         squash_commit_message: this.squashCommitMessage,

app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js

@@ -23,8 +23,8 @@ export default function deviseState(data) {
     return stateKey.pipelineBlocked;
   } else if (this.isSHAMismatch) {
     return stateKey.shaMismatch;
-  } else if (this.mergeWhenPipelineSucceeds) {
-    return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds;
+  } else if (this.autoMergeEnabled) {
+    return this.mergeError ? stateKey.autoMergeFailed : stateKey.autoMergeEnabled;
   } else if (!this.canMerge) {
     return stateKey.notAllowedToMerge;
   } else if (this.canBeMerged) {

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -61,7 +61,7 @@ export default class MergeRequestStore {
     this.updatedAt = data.updated_at;
     this.metrics = MergeRequestStore.buildMetrics(data.metrics);
-    this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
+    this.setToAutoMergeBy = MergeRequestStore.formatUserObject(data.merge_user || {});
     this.mergeUserId = data.merge_user_id;
     this.currentUserId = gon.current_user_id;
     this.sourceBranchPath = data.source_branch_path;
@@ -70,12 +70,13 @@ export default class MergeRequestStore {
     this.targetBranchPath = data.target_branch_commits_path;
     this.targetBranchTreePath = data.target_branch_tree_path;
     this.conflictResolutionPath = data.conflict_resolution_path;
-    this.cancelAutoMergePath = data.cancel_merge_when_pipeline_succeeds_path;
+    this.cancelAutoMergePath = data.cancel_auto_merge_path;
     this.removeWIPPath = data.remove_wip_path;
     this.sourceBranchRemoved = !data.source_branch_exists;
     this.shouldRemoveSourceBranch = data.remove_source_branch || false;
     this.onlyAllowMergeIfPipelineSucceeds = data.only_allow_merge_if_pipeline_succeeds || false;
-    this.mergeWhenPipelineSucceeds = data.merge_when_pipeline_succeeds || false;
+    this.autoMergeEnabled = Boolean(data.auto_merge_enabled);
+    this.autoMergeStrategy = data.auto_merge_strategy;
     this.mergePath = data.merge_path;
     this.ffOnlyEnabled = data.ff_only_enabled;
     this.shouldBeRebased = !!data.should_be_rebased;
@@ -93,7 +94,7 @@ export default class MergeRequestStore {
     this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
     this.canMerge = !!data.merge_path;
     this.canCreateIssue = currentUser.can_create_issue || false;
-    this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path;
+    this.canCancelAutomaticMerge = !!data.cancel_auto_merge_path;
     this.isSHAMismatch = this.sha !== data.diff_head_sha;
     this.canBeMerged = data.can_be_merged || false;
     this.isMergeAllowed = data.mergeable || false;

app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js

@@ -13,7 +13,7 @@ const stateToComponentMap = {
   unresolvedDiscussions: 'mr-widget-unresolved-discussions',
   pipelineBlocked: 'mr-widget-pipeline-blocked',
   pipelineFailed: 'mr-widget-pipeline-failed',
-  mergeWhenPipelineSucceeds: 'mr-widget-merge-when-pipeline-succeeds',
+  autoMergeEnabled: 'mr-widget-merge-when-pipeline-succeeds',
   failedToMerge: 'mr-widget-failed-to-merge',
   autoMergeFailed: 'mr-widget-auto-merge-failed',
   shaMismatch: 'sha-mismatch',
@@ -45,7 +45,7 @@ export const stateKey = {
   pipelineBlocked: 'pipelineBlocked',
   shaMismatch: 'shaMismatch',
   autoMergeFailed: 'autoMergeFailed',
-  mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds',
+  autoMergeEnabled: 'autoMergeEnabled',
   notAllowedToMerge: 'notAllowedToMerge',
   readyToMerge: 'readyToMerge',
   rebase: 'rebase',

app/controllers/projects/merge_requests_controller.rb

@@ -145,14 +145,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     render partial: 'projects/merge_requests/widget/commit_change_content', layout: false
   end
-  def cancel_merge_when_pipeline_succeeds
-    unless @merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user)
+  def cancel_auto_merge
+    unless @merge_request.can_cancel_auto_merge?(current_user)
       return access_denied!
     end
-    ::MergeRequests::MergeWhenPipelineSucceedsService
-      .new(@project, current_user)
-      .cancel(@merge_request)
+    AutoMergeService.new(project, current_user).cancel(@merge_request)
     render json: serialize_widget(@merge_request)
   end
@@ -229,12 +227,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
   def merge_params_attributes
-    [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash]
+    [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash, :auto_merge_strategy]
   end
-  def merge_when_pipeline_succeeds_active?
-    params[:merge_when_pipeline_succeeds].present? &&
-      @merge_request.head_pipeline && @merge_request.head_pipeline.active?
+  def auto_merge_requested?
+    # Support params[:merge_when_pipeline_succeeds] during the transition period
+    params[:auto_merge_strategy].present? || params[:merge_when_pipeline_succeeds].present?
   end
   def close_merge_request_if_no_source_project
@@ -258,9 +256,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
   def merge!
-    # Disable the CI check if merge_when_pipeline_succeeds is enabled since we have
+    # Disable the CI check if auto_merge_strategy is specified since we have
     # to wait until CI completes to know
-    unless @merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds_active?)
+    unless @merge_request.mergeable?(skip_ci_check: auto_merge_requested?)
       return :failed
     end
@@ -274,24 +272,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
-    if params[:merge_when_pipeline_succeeds].present?
-      return :failed unless @merge_request.actual_head_pipeline
-
-      if @merge_request.actual_head_pipeline.active?
-        ::MergeRequests::MergeWhenPipelineSucceedsService
-          .new(@project, current_user, merge_params)
-          .execute(@merge_request)
-
-        :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
+    if auto_merge_requested?
+      AutoMergeService.new(project, current_user, merge_params)
+        .execute(merge_request,
+                 params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
     else
       @merge_request.merge_async(current_user.id, merge_params)

app/helpers/merge_requests_helper.rb

@@ -103,7 +103,7 @@ module MergeRequestsHelper
   def merge_params(merge_request)
     {
-      merge_when_pipeline_succeeds: true,
+      auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS,
       should_remove_source_branch: true,
       sha: merge_request.diff_head_sha,
       squash: merge_request.squash

app/models/merge_request.rb

@@ -165,7 +165,7 @@ class MergeRequest < ApplicationRecord
   validates :source_branch, presence: true
   validates :target_project, presence: true
   validates :target_branch, presence: true
-  validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing?
+  validates :merge_user, presence: true, if: :auto_merge_enabled?, unless: :importing?
   validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
   validate :validate_fork, unless: :closed_without_fork?
   validate :validate_target_project, on: :create
@@ -196,6 +196,7 @@ class MergeRequest < ApplicationRecord
   alias_attribute :project, :target_project
   alias_attribute :project_id, :target_project_id
+  alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
   def self.reference_prefix
     '!'
@@ -391,7 +392,7 @@ class MergeRequest < ApplicationRecord
   def merge_participants
     participants = [author]
-    if merge_when_pipeline_succeeds? && !participants.include?(merge_user)
+    if auto_merge_enabled? && !participants.include?(merge_user)
       participants << merge_user
     end
@@ -782,7 +783,7 @@ class MergeRequest < ApplicationRecord
     project.ff_merge_must_be_possible? && !ff_merge_possible?
   end
-  def can_cancel_merge_when_pipeline_succeeds?(current_user)
+  def can_cancel_auto_merge?(current_user)
     can_be_merged_by?(current_user) || self.author == current_user
   end
@@ -801,6 +802,16 @@ class MergeRequest < ApplicationRecord
     Gitlab::Utils.to_boolean(merge_params['force_remove_source_branch'])
   end
+  def auto_merge_strategy
+    return unless auto_merge_enabled?
+
+    merge_params['auto_merge_strategy'] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS
+  end
+
+  def auto_merge_strategy=(strategy)
+    merge_params['auto_merge_strategy'] = strategy
+  end
+
   def remove_source_branch?
     should_remove_source_branch? || force_remove_source_branch?
   end
@@ -973,15 +984,16 @@ class MergeRequest < ApplicationRecord
     end
   end
-  def reset_merge_when_pipeline_succeeds
-    return unless merge_when_pipeline_succeeds?
-    self.merge_when_pipeline_succeeds = false
+  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

app/presenters/merge_request_presenter.rb

@@ -22,9 +22,9 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     end
   end
-  def cancel_merge_when_pipeline_succeeds_path
-    if can_cancel_merge_when_pipeline_succeeds?(current_user)
-      cancel_merge_when_pipeline_succeeds_project_merge_request_path(project, merge_request)
+  def cancel_auto_merge_path
+    if can_cancel_auto_merge?(current_user)
+      cancel_auto_merge_project_merge_request_path(project, merge_request)
     end
   end

app/serializers/merge_request_widget_entity.rb

@@ -9,7 +9,11 @@ class MergeRequestWidgetEntity < IssuableEntity
   expose :merge_params
   expose :merge_status
   expose :merge_user_id
-  expose :merge_when_pipeline_succeeds
+  expose :auto_merge_enabled
+  expose :auto_merge_strategy
+  expose :available_auto_merge_strategies do |merge_request|
+    AutoMergeService.new(merge_request.project, current_user).available_strategies(merge_request) # rubocop: disable CodeReuse/ServiceClass
+  end
   expose :source_branch
   expose :source_branch_protected do |merge_request|
     merge_request.source_project.present? && ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch)
@@ -182,8 +186,8 @@ class MergeRequestWidgetEntity < IssuableEntity
     presenter(merge_request).remove_wip_path
   end
-  expose :cancel_merge_when_pipeline_succeeds_path do |merge_request|
-    presenter(merge_request).cancel_merge_when_pipeline_succeeds_path
+  expose :cancel_auto_merge_path do |merge_request|
+    presenter(merge_request).cancel_auto_merge_path
   end
   expose :create_issue_to_resolve_discussions_path do |merge_request|

app/services/auto_merge/merge_when_pipeline_succeeds_service.rb

+# frozen_string_literal: true
+
+module AutoMerge
+  class MergeWhenPipelineSucceedsService < 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)
+        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
+
+    def process(merge_request)
+      return unless merge_request.actual_head_pipeline&.success?
+      return unless merge_request.mergeable?
+
+      merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
+    end
+
+    def cancel(merge_request)
+      if merge_request.reset_auto_merge
+        SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
+
+        success
+      else
+        error("Can't cancel the automatic merge", 406)
+      end
+    end
+
+    def available_for?(merge_request)
+      merge_request.actual_head_pipeline&.active?
+    end
+  end
+end

app/services/auto_merge_service.rb

+# frozen_string_literal: true
+
+class AutoMergeService < BaseService
+  STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS = 'merge_when_pipeline_succeeds'.freeze
+  STRATEGIES = [STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS].freeze
+
+  class << self
+    def all_strategies
+      STRATEGIES
+    end
+
+    def get_service_class(strategy)
+      return unless all_strategies.include?(strategy)
+
+      "::AutoMerge::#{strategy.camelize}Service".constantize
+    end
+  end
+
+  def execute(merge_request, strategy)
+    service = get_service_instance(strategy)
+
+    return :failed unless service&.available_for?(merge_request)
+
+    service.execute(merge_request)
+  end
+
+  def process(merge_request)
+    return unless merge_request.auto_merge_enabled?
+
+    get_service_instance(merge_request.auto_merge_strategy).process(merge_request)
+  end
+
+  def cancel(merge_request)
+    return error("Can't cancel the automatic merge", 406) unless merge_request.auto_merge_enabled?
+
+    get_service_instance(merge_request.auto_merge_strategy).cancel(merge_request)
+  end
+
+  def available_strategies(merge_request)
+    self.class.all_strategies.select do |strategy|
+      get_service_instance(strategy).available_for?(merge_request)
+    end
+  end
+
+  private
+
+  def get_service_instance(strategy)
+    self.class.get_service_class(strategy)&.new(project, current_user, params)
+  end
+end

app/services/merge_requests/merge_when_pipeline_succeeds_service.rb

-# frozen_string_literal: true
-
-module MergeRequests
-  class MergeWhenPipelineSucceedsService < MergeRequests::BaseService
-    # Marks the passed `merge_request` to be merged when the pipeline succeeds or
-    # updates the params for the automatic merge
-    def execute(merge_request)
-      merge_request.merge_params.merge!(params)
-
-      # The service is also called when the merge params are updated.
-      already_approved = merge_request.merge_when_pipeline_succeeds?
-
-      unless already_approved
-        merge_request.merge_when_pipeline_succeeds = true
-        merge_request.merge_user = @current_user
-
-        SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit)
-      end
-
-      merge_request.save
-    end
-
-    # Triggers the automatic merge of merge_request once the pipeline succeeds
-    def trigger(pipeline)
-      return unless pipeline.success?
-
-      pipeline_merge_requests(pipeline) do |merge_request|
-        next unless merge_request.merge_when_pipeline_succeeds?
-        next unless merge_request.mergeable?
-
-        merge_request.merge_async(merge_request.merge_user_id, merge_request.merge_params)
-      end
-    end
-
-    # Cancels the automatic merge
-    def cancel(merge_request)
-      if merge_request.merge_when_pipeline_succeeds? && merge_request.open?
-        merge_request.reset_merge_when_pipeline_succeeds
-        SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user)
-
-        success
-      else
-        error("Can't cancel the automatic merge", 406)
-      end
-    end
-  end
-end

app/services/merge_requests/refresh_service.rb

@@ -24,7 +24,7 @@ module MergeRequests
       reload_merge_requests
       outdate_suggestions
       refresh_pipelines_on_merge_requests
-      reset_merge_when_pipeline_succeeds
+      cancel_auto_merge
       mark_pending_todos_done
       cache_merge_requests_closing_issues
@@ -142,8 +142,10 @@ module MergeRequests
       end
     end
-    def reset_merge_when_pipeline_succeeds
-      merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
+    def cancel_auto_merge
+      merge_requests_for_source_branch.each do |merge_request|
+        AutoMergeService.new(project, current_user).cancel(merge_request)
+      end
     end
     def mark_pending_todos_done

app/services/merge_requests/update_service.rb

@@ -89,7 +89,7 @@ module MergeRequests
       merge_request.update(merge_error: nil)
       if merge_request.head_pipeline && merge_request.head_pipeline.active?
-        MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request)
+        AutoMergeService.new(project, current_user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
       else
         merge_request.merge_async(current_user.id, {})
       end

app/services/notification_service.rb

@@ -238,7 +238,7 @@ class NotificationService
       merge_request,
       current_user,
       :merged_merge_request_email,
-      skip_current_user: !merge_request.merge_when_pipeline_succeeds?
+      skip_current_user: !merge_request.auto_merge_enabled?
     )
   end

app/workers/pipeline_success_worker.rb

@@ -9,9 +9,10 @@ class PipelineSuccessWorker
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(pipeline_id)
     Ci::Pipeline.find_by(id: pipeline_id).try do |pipeline|
-      MergeRequests::MergeWhenPipelineSucceedsService
-        .new(pipeline.project, nil)
-        .trigger(pipeline)
+      pipeline.all_merge_requests.preload(:merge_user).each do |merge_request|
+        AutoMergeService.new(pipeline.project, merge_request.merge_user)
+                        .process(merge_request)
+      end
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord

changelogs/unreleased/abstract-auto-merge.yml

+---
+title: Refactor and abstract Auto Merge Processes
+merge_request: 28595
+author:
+type: other

config/routes/project.rb

@@ -208,7 +208,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         member do
           get :commit_change_content
           post :merge
-          post :cancel_merge_when_pipeline_succeeds
+          post :cancel_auto_merge
           get :pipeline_status
           get :ci_environments_status
           post :toggle_subscription

lib/api/merge_requests.rb

@@ -386,9 +386,8 @@ module API
         )
         if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active?
-          ::MergeRequests::MergeWhenPipelineSucceedsService
-            .new(merge_request.target_project, current_user, merge_params)
-            .execute(merge_request)
+          AutoMergeService.new(merge_request.target_project, current_user, merge_params)
+            .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
         else
           ::MergeRequests::MergeService
             .new(merge_request.target_project, current_user, merge_params)
@@ -429,11 +428,9 @@ module API
       post ':id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds' do
         merge_request = find_project_merge_request(params[:merge_request_iid])
-        unauthorized! unless merge_request.can_cancel_merge_when_pipeline_succeeds?(current_user)
-        ::MergeRequests::MergeWhenPipelineSucceedsService
-          .new(merge_request.target_project, current_user)
-          .cancel(merge_request)
+        unauthorized! unless merge_request.can_cancel_auto_merge?(current_user)
+        AutoMergeService.new(merge_request.target_project, current_user).cancel(merge_request)
       end
       desc 'Rebase the merge request against its target branch' do