Merge branch '16388-abort-mwps-on-impossible-ff-merge' into 'master'

Abort MWPS when FF only merge is impossible

See merge request gitlab-org/gitlab!17886

app/models/merge_request.rb

@@ -196,6 +196,10 @@ class MergeRequest < ApplicationRecord
   scope :by_target_branch, ->(branch_name) { where(target_branch: branch_name) }
   scope :preload_source_project, -> { preload(:source_project) }
 
+  scope :with_open_merge_when_pipeline_succeeds, -> do
+    with_state(:opened).where(merge_when_pipeline_succeeds: true)
+  end
+
   after_save :keep_around_commit
 
   alias_attribute :project, :target_project

app/services/merge_requests/refresh_service.rb

@@ -25,6 +25,7 @@ module MergeRequests
       outdate_suggestions
       refresh_pipelines_on_merge_requests
       abort_auto_merges
+      abort_ff_merge_requests_with_when_pipeline_succeeds
       mark_pending_todos_done
       cache_merge_requests_closing_issues
 
@@ -148,6 +149,31 @@ module MergeRequests
       end
     end
 
+    def abort_ff_merge_requests_with_when_pipeline_succeeds
+      return unless @project.ff_merge_must_be_possible?
+
+      requests_with_auto_merge_enabled_to(@push.branch_name).each do |merge_request|
+        next unless merge_request.should_be_rebased?
+
+        abort_auto_merge_with_todo(merge_request, 'target branch was updated')
+      end
+    end
+
+    def abort_auto_merge_with_todo(merge_request, reason)
+      response = abort_auto_merge(merge_request, reason)
+      response = ServiceResponse.new(response)
+      return unless response.success?
+
+      todo_service.merge_request_became_unmergeable(merge_request)
+    end
+
+    def requests_with_auto_merge_enabled_to(target_branch)
+      @project
+        .merge_requests
+        .by_target_branch(target_branch)
+        .with_open_merge_when_pipeline_succeeds
+    end
+
     def mark_pending_todos_done
       merge_requests_for_source_branch.each do |merge_request|
         todo_service.merge_request_push(merge_request, @current_user)

changelogs/unreleased/16388-abort-mwps-on-impossible-ff-merge.yml

+---
+title: Abort Merge When Pipeline Succeeds when Fast Forward merge is impossible
+merge_request: 17886
+author:
+type: fixed

spec/models/merge_request_spec.rb

@@ -3257,4 +3257,38 @@ describe MergeRequest do
       end
     end
   end
+
+  describe '.with_open_merge_when_pipeline_succeeds' do
+    let!(:project) { create(:project) }
+    let!(:fork) { fork_project(project) }
+    let!(:merge_request1) do
+      create(:merge_request,
+             :merge_when_pipeline_succeeds,
+             target_project: project,
+             target_branch: 'master',
+             source_project: project,
+             source_branch: 'feature-1')
+    end
+
+    let!(:merge_request2) do
+      create(:merge_request,
+             :merge_when_pipeline_succeeds,
+             target_project: project,
+             target_branch: 'master',
+             source_project: fork,
+             source_branch: 'fork-feature-1')
+    end
+
+    let!(:merge_request4) do
+      create(:merge_request,
+             target_project: project,
+             target_branch: 'master',
+             source_project: fork,
+             source_branch: 'fork-feature-2')
+    end
+
+    let(:query) { described_class.with_open_merge_when_pipeline_succeeds }
+
+    it { expect(query).to contain_exactly(merge_request1, merge_request2) }
+  end
 end

spec/services/merge_requests/refresh_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe MergeRequests::RefreshService do
   include ProjectForksHelper
+  include ProjectHelpers
 
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
@@ -756,4 +757,127 @@ describe MergeRequests::RefreshService do
       end
     end
   end
+
+  describe '#abort_ff_merge_requests_with_when_pipeline_succeeds' do
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:source_project) { project }
+    let_it_be(:target_project) { project }
+    let_it_be(:author) { create_user_from_membership(target_project, :developer) }
+    let_it_be(:user) { create(:user) }
+
+    let_it_be(:forked_project) do
+      fork_project(target_project, author, repository: true)
+    end
+
+    let_it_be(:merge_request) do
+      create(:merge_request,
+             author: author,
+             source_project: source_project,
+             source_branch: 'feature',
+             target_branch: 'master',
+             target_project: target_project,
+             auto_merge_enabled: true,
+             auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS,
+             merge_user: user)
+    end
+
+    let_it_be(:newrev) do
+      target_project
+        .repository
+        .create_file(user, 'test1.txt', 'Test data',
+                     message: 'Test commit', branch_name: 'master')
+    end
+
+    let_it_be(:oldrev) do
+      target_project
+        .repository
+        .commit(newrev)
+        .parent_id
+    end
+
+    let(:refresh_service) { service.new(project, user) }
+
+    before do
+      target_project.merge_method = merge_method
+      target_project.save!
+
+      refresh_service.execute(oldrev, newrev, 'refs/heads/master')
+      merge_request.reload
+    end
+
+    let(:aborted_message) do
+      /aborted the automatic merge because target branch was updated/
+    end
+
+    shared_examples 'aborted MWPS' do
+      it 'aborts auto_merge' do
+        expect(merge_request.auto_merge_enabled?).to be_falsey
+        expect(merge_request.notes.last.note).to match(aborted_message)
+      end
+
+      it 'removes merge_user' do
+        expect(merge_request.merge_user).to be_nil
+      end
+
+      it 'does not add todos for merge user' do
+        expect(user.todos.for_target(merge_request)).to be_empty
+      end
+
+      it 'adds todos for merge author' do
+        expect(author.todos.for_target(merge_request)).to be_present.and be_all(&:pending?)
+      end
+    end
+
+    context 'when Project#merge_method is set to FF' do
+      let(:merge_method) { :ff }
+
+      it_behaves_like 'aborted MWPS'
+
+      context 'with forked project' do
+        let(:source_project) { forked_project }
+
+        it_behaves_like 'aborted MWPS'
+      end
+    end
+
+    context 'when Project#merge_method is set to rebase_merge' do
+      let(:merge_method) { :rebase_merge }
+
+      it_behaves_like 'aborted MWPS'
+
+      context 'with forked project' do
+        let(:source_project) { forked_project }
+
+        it_behaves_like 'aborted MWPS'
+      end
+    end
+
+    context 'when Project#merge_method is set to merge' do
+      let(:merge_method) { :merge }
+
+      shared_examples 'maintained MWPS' do
+        it 'does not cancel auto merge' do
+          expect(merge_request.auto_merge_enabled?).to be_truthy
+          expect(merge_request.notes).to be_empty
+        end
+
+        it 'does not change merge_user' do
+          expect(merge_request.merge_user).to eq(user)
+        end
+
+        it 'does not add todos' do
+          expect(author.todos.for_target(merge_request)).to be_empty
+          expect(user.todos.for_target(merge_request)).to be_empty
+        end
+      end
+
+      it_behaves_like 'maintained MWPS'
+
+      context 'with forked project' do
+        let(:source_project) { forked_project }
+
+        it_behaves_like 'maintained MWPS'
+      end
+    end
+  end
 end