Update merge-when-pipeline-succeeds behavior on API

When the `merge_when_pipeline_succeeds` param is passed in,
an MR can be merged even when the pipeline has failed.

This change adds a check that prevents this from happening
when attempting to merge through the API

app/models/merge_request.rb

@@ -241,6 +241,9 @@ class MergeRequest < ApplicationRecord
   alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
   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
+
   RebaseLockTimeout = Class.new(StandardError)
 
   REBASE_LOCK_MESSAGE = _("Failed to enqueue the rebase operation, possibly due to a long-lived transaction. Try again later.")

app/services/auto_merge/merge_when_pipeline_succeeds_service.rb

@@ -11,7 +11,7 @@ module AutoMerge
     end
 
     def process(merge_request)
-      return unless merge_request.actual_head_pipeline&.success?
+      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)

app/services/merge_requests/update_service.rb

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

changelogs/unreleased/26293-prevent-merge-if-pipeline-has-failed.yml

+---
+title: Update merging an MR behavior on the API when pipeline fails
+merge_request: 19641
+author: briankabiro
+type: fixed

ee/app/services/auto_merge/add_to_merge_train_when_pipeline_succeeds_service.rb

@@ -9,7 +9,7 @@ module AutoMerge
     end
 
     def process(merge_request)
-      return unless merge_request.actual_head_pipeline&.success?
+      return unless merge_request.actual_head_pipeline_success?
 
       merge_train_service = AutoMerge::MergeTrainService.new(project, merge_request.merge_user)
 

lib/api/merge_requests.rb

@@ -68,6 +68,10 @@ 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?
+      end
+
       def serializer_options_for(merge_requests)
         options = { with: Entities::MergeRequestBasic, current_user: current_user }
 
@@ -391,12 +395,13 @@ module API
 
         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! unless merge_request.mergeable_state?(skip_ci_check: merge_when_pipeline_succeeds)
+        not_allowed! if !merge_request.mergeable_state?(skip_ci_check: merge_when_pipeline_succeeds) || not_automatically_mergeable
 
         render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?(skip_ci_check: merge_when_pipeline_succeeds)
 
@@ -411,7 +416,7 @@ module API
           sha: params[:sha] || merge_request.diff_head_sha
         )
 
-        if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active?
+        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

spec/models/merge_request_spec.rb

@@ -2326,6 +2326,26 @@ describe MergeRequest do
     end
   end
 
+  describe "#head_pipeline_active? " do
+    it do
+      is_expected
+        .to delegate_method(:active?)
+        .to(:head_pipeline)
+        .with_prefix
+        .with_arguments(allow_nil: true)
+    end
+  end
+
+  describe "#actual_head_pipeline_success? " do
+    it do
+      is_expected
+        .to delegate_method(:success?)
+        .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

@@ -1567,6 +1567,18 @@ describe API::MergeRequests do
       expect(response).to have_gitlab_http_status(200)
     end
 
+    it 'does not merge if merge_when_pipeline_succeeds is passed and the pipeline has failed' do
+      create(:ci_pipeline,
+        :failed,
+        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(405)
+      expect(merge_request.reload.state).to eq('opened')
+    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)