MWPS merges immediately if the pipeline succeeded

When passing MWPS to the merge API, we should merge the MR immediately
when the pipeline already succeeded.

On the other hand, we should not merge the MR when passing MWPS and
the pipeline failed, even if the project allows merging with a failing
pipeline.

app/models/merge_request.rb

@@ -258,7 +258,7 @@ class MergeRequest < ApplicationRecord
   alias_method :issuing_parent, :target_project
 
   delegate :active?, to: :head_pipeline, prefix: true, allow_nil: true
-  delegate :success?, to: :actual_head_pipeline, prefix: true, allow_nil: true
+  delegate :success?, :active?, to: :actual_head_pipeline, prefix: true, allow_nil: true
 
   RebaseLockTimeout = Class.new(StandardError)
 

changelogs/unreleased/bvl-fix-mwps-api.yml

+---
+title: Merge a merge request immediately when passing merge when pipeline succeeds
+  to the merge API when the head pipeline already succeeded
+merge_request: 22777
+author:
+type: fixed

lib/api/merge_requests.rb

@@ -68,8 +68,17 @@ module API
         end
       end
 
-      def not_automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
-        merge_when_pipeline_succeeds && !merge_request.head_pipeline_active? && !merge_request.actual_head_pipeline_success?
+      def automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
+        pipeline_active = merge_request.head_pipeline_active? || merge_request.actual_head_pipeline_active?
+        merge_when_pipeline_succeeds && merge_request.mergeable_state?(skip_ci_check: true) && pipeline_active
+      end
+
+      def immediately_mergeable?(merge_when_pipeline_succeeds, merge_request)
+        if merge_when_pipeline_succeeds
+          merge_request.actual_head_pipeline_success?
+        else
+          merge_request.mergeable_state?
+        end
       end
 
       def serializer_options_for(merge_requests)
@@ -393,16 +402,18 @@ module API
         Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42317')
 
         merge_request = find_project_merge_request(params[:merge_request_iid])
-        merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds])
-        not_automatically_mergeable = not_automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
 
         # Merge request can not be merged
         # because user dont have permissions to push into target branch
         unauthorized! unless merge_request.can_be_merged_by?(current_user)
 
-        not_allowed! if !merge_request.mergeable_state?(skip_ci_check: merge_when_pipeline_succeeds) || not_automatically_mergeable
+        merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds])
+        automatically_mergeable = automatically_mergeable?(merge_when_pipeline_succeeds, merge_request)
+        immediately_mergeable = immediately_mergeable?(merge_when_pipeline_succeeds, merge_request)
+
+        not_allowed! if !immediately_mergeable && !automatically_mergeable
 
-        render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds)
+        render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: automatically_mergeable)
 
         check_sha_param!(params, merge_request)
 
@@ -415,13 +426,13 @@ module API
           sha: params[:sha] || merge_request.diff_head_sha
         )
 
-        if merge_when_pipeline_succeeds
-          AutoMergeService.new(merge_request.target_project, current_user, merge_params)
-            .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
-        else
+        if immediately_mergeable
           ::MergeRequests::MergeService
             .new(merge_request.target_project, current_user, merge_params)
             .execute(merge_request)
+        elsif automatically_mergeable
+          AutoMergeService.new(merge_request.target_project, current_user, merge_params)
+            .execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
         end
 
         present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project

spec/models/merge_request_spec.rb

@@ -2214,6 +2214,16 @@ describe MergeRequest do
     end
   end
 
+  describe "#actual_head_pipeline_active? " do
+    it do
+      is_expected
+        .to delegate_method(:active?)
+        .to(:actual_head_pipeline)
+        .with_prefix
+        .with_arguments(allow_nil: true)
+    end
+  end
+
   describe '#mergeable_ci_state?' do
     let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) }
     let(:pipeline) { create(:ci_empty_pipeline) }

spec/requests/api/merge_requests_spec.rb

@@ -1549,7 +1549,7 @@ describe API::MergeRequests do
     end
   end
 
-  describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge" do
+  describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge", :clean_gitlab_redis_cache do
     let(:pipeline) { create(:ci_pipeline) }
 
     it "returns merge_request in case of success" do
@@ -1637,6 +1637,15 @@ describe API::MergeRequests do
       expect(merge_request.reload.state).to eq('opened')
     end
 
+    it 'merges if the head pipeline already succeeded and `merge_when_pipeline_succeeds` is passed' do
+      create(:ci_pipeline, :success, sha: merge_request.diff_head_sha, merge_requests_as_head_pipeline: [merge_request])
+
+      put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { merge_when_pipeline_succeeds: true }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['state']).to eq('merged')
+    end
+
     it "enables merge when pipeline succeeds if the pipeline is active" do
       allow_any_instance_of(MergeRequest).to receive_messages(head_pipeline: pipeline, actual_head_pipeline: pipeline)
       allow(pipeline).to receive(:active?).and_return(true)