Merge branch 'improve-retarget-merge-requests' into 'master'

Improve MR Retarget Chain to prefer usage of `MR::DeleteSourceBranchWorker`

See merge request gitlab-org/gitlab!56233

app/services/merge_requests/post_merge_service.rb

@@ -20,7 +20,6 @@ module MergeRequests
       merge_request_activity_counter.track_merge_mr_action(user: current_user)
       notification_service.merge_mr(merge_request, current_user)
       execute_hooks(merge_request, 'merge')
-      retarget_chain_merge_requests(merge_request)
       invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers)
       merge_request.update_project_counter_caches
       delete_non_latest_diffs(merge_request)
@@ -31,34 +30,6 @@ module MergeRequests
 
     private
 
-    def retarget_chain_merge_requests(merge_request)
-      return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project)
-
-      # we can only retarget MRs that are targeting the same project
-      # and have a remove source branch set
-      return unless merge_request.for_same_project? && merge_request.remove_source_branch?
-
-      # find another merge requests that
-      # - as a target have a current source project and branch
-      other_merge_requests = merge_request.source_project
-        .merge_requests
-        .opened
-        .by_target_branch(merge_request.source_branch)
-        .preload_source_project
-        .at_most(MAX_RETARGET_MERGE_REQUESTS)
-
-      other_merge_requests.find_each do |other_merge_request|
-        # Update only MRs on projects that we have access to
-        next unless can?(current_user, :update_merge_request, other_merge_request.source_project)
-
-        ::MergeRequests::UpdateService
-          .new(other_merge_request.source_project, current_user,
-            target_branch: merge_request.target_branch,
-            target_branch_was_deleted: true)
-          .execute(other_merge_request)
-      end
-    end
-
     def close_issues(merge_request)
       return unless merge_request.target_branch == project.default_branch
 

app/services/merge_requests/retarget_chain_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class RetargetChainService < MergeRequests::BaseService
+    MAX_RETARGET_MERGE_REQUESTS = 4
+
+    def execute(merge_request)
+      return unless Feature.enabled?(:retarget_merge_requests, merge_request.target_project, default_enabled: :yaml)
+
+      # we can only retarget MRs that are targeting the same project
+      return unless merge_request.for_same_project? && merge_request.merged?
+
+      # find another merge requests that
+      # - as a target have a current source project and branch
+      other_merge_requests = merge_request.source_project
+        .merge_requests
+        .opened
+        .by_target_branch(merge_request.source_branch)
+        .preload_source_project
+        .at_most(MAX_RETARGET_MERGE_REQUESTS)
+
+      other_merge_requests.find_each do |other_merge_request|
+        # Update only MRs on projects that we have access to
+        next unless can?(current_user, :update_merge_request, other_merge_request.source_project)
+
+        ::MergeRequests::UpdateService
+          .new(other_merge_request.source_project, current_user,
+            target_branch: merge_request.target_branch,
+            target_branch_was_deleted: true)
+          .execute(other_merge_request)
+      end
+    end
+  end
+end

app/workers/merge_requests/delete_source_branch_worker.rb

@@ -16,6 +16,9 @@ class MergeRequests::DeleteSourceBranchWorker
 
     ::Branches::DeleteService.new(merge_request.source_project, user)
       .execute(merge_request.source_branch)
+
+    ::MergeRequests::RetargetChainService.new(merge_request.source_project, user)
+      .execute(merge_request)
   rescue ActiveRecord::RecordNotFound
   end
 end

changelogs/unreleased/improve-retarget-merge-requests.yml

+---
+title: Automatically retarget merge requests upon merge (default on)
+merge_request: 56233
+author:
+type: added

config/feature_flags/development/retarget_merge_requests.yml

@@ -5,4 +5,4 @@ rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/320895
 milestone: '13.9'
 type: development
 group: group::memory
-default_enabled: false
+default_enabled: true

spec/services/merge_requests/post_merge_service_spec.rb

@@ -130,139 +130,5 @@ RSpec.describe MergeRequests::PostMergeService do
         expect(deploy_job.reload.canceled?).to be false
       end
     end
-
-    context 'for a merge request chain' do
-      before do
-        ::MergeRequests::UpdateService
-          .new(project, user, force_remove_source_branch: '1')
-          .execute(merge_request)
-      end
-
-      context 'when there is another MR' do
-        let!(:another_merge_request) do
-          create(:merge_request,
-            source_project: source_project,
-            source_branch: 'my-awesome-feature',
-            target_project: merge_request.source_project,
-            target_branch: merge_request.source_branch
-          )
-        end
-
-        shared_examples 'does not retarget merge request' do
-          it 'another merge request is unchanged' do
-            expect { subject }.not_to change { another_merge_request.reload.target_branch }
-              .from(merge_request.source_branch)
-          end
-        end
-
-        shared_examples 'retargets merge request' do
-          it 'another merge request is retargeted' do
-            expect(SystemNoteService)
-              .to receive(:change_branch).once
-              .with(another_merge_request, another_merge_request.project, user,
-                'target', 'delete',
-                merge_request.source_branch, merge_request.target_branch)
-
-            expect { subject }.to change { another_merge_request.reload.target_branch }
-              .from(merge_request.source_branch)
-              .to(merge_request.target_branch)
-          end
-
-          context 'when FF retarget_merge_requests is disabled' do
-            before do
-              stub_feature_flags(retarget_merge_requests: false)
-            end
-
-            include_examples 'does not retarget merge request'
-          end
-
-          context 'when source branch is to be kept' do
-            before do
-              ::MergeRequests::UpdateService
-                .new(project, user, force_remove_source_branch: false)
-                .execute(merge_request)
-            end
-
-            include_examples 'does not retarget merge request'
-          end
-        end
-
-        context 'in the same project' do
-          let(:source_project) { project }
-
-          it_behaves_like 'retargets merge request'
-
-          context 'and is closed' do
-            before do
-              another_merge_request.close
-            end
-
-            it_behaves_like 'does not retarget merge request'
-          end
-
-          context 'and is merged' do
-            before do
-              another_merge_request.mark_as_merged
-            end
-
-            it_behaves_like 'does not retarget merge request'
-          end
-        end
-
-        context 'in forked project' do
-          let!(:source_project) { fork_project(project) }
-
-          context 'when user has access to source project' do
-            before do
-              source_project.add_developer(user)
-            end
-
-            it_behaves_like 'retargets merge request'
-          end
-
-          context 'when user does not have access to source project' do
-            it_behaves_like 'does not retarget merge request'
-          end
-        end
-
-        context 'and current and another MR is from a fork' do
-          let(:project) { create(:project) }
-          let(:source_project) { fork_project(project) }
-
-          let(:merge_request) do
-            create(:merge_request,
-              source_project: source_project,
-              target_project: project
-            )
-          end
-
-          before do
-            source_project.add_developer(user)
-          end
-
-          it_behaves_like 'does not retarget merge request'
-        end
-      end
-
-      context 'when many merge requests are to be retargeted' do
-        let!(:many_merge_requests) do
-          create_list(:merge_request, 10, :unique_branches,
-            source_project: merge_request.source_project,
-            target_project: merge_request.source_project,
-            target_branch: merge_request.source_branch
-          )
-        end
-
-        it 'retargets only 4 of them' do
-          subject
-
-          expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally)
-            .to eq(
-              merge_request.source_branch => 6,
-              merge_request.target_branch => 4
-            )
-        end
-      end
-    end
   end
 end

spec/services/merge_requests/retarget_chain_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::RetargetChainService do
+  include ProjectForksHelper
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:merge_request, reload: true) { create(:merge_request, assignees: [user]) }
+  let_it_be(:project) { merge_request.project }
+
+  subject { described_class.new(project, user).execute(merge_request) }
+
+  before do
+    project.add_maintainer(user)
+  end
+
+  describe '#execute' do
+    context 'when there is another MR' do
+      let!(:another_merge_request) do
+        create(:merge_request,
+          source_project: source_project,
+          source_branch: 'my-awesome-feature',
+          target_project: merge_request.source_project,
+          target_branch: merge_request.source_branch
+        )
+      end
+
+      shared_examples 'does not retarget merge request' do
+        it 'another merge request is unchanged' do
+          expect { subject }.not_to change { another_merge_request.reload.target_branch }
+            .from(merge_request.source_branch)
+        end
+      end
+
+      shared_examples 'retargets merge request' do
+        it 'another merge request is retargeted' do
+          expect(SystemNoteService)
+            .to receive(:change_branch).once
+            .with(another_merge_request, another_merge_request.project, user,
+              'target', 'delete',
+              merge_request.source_branch, merge_request.target_branch)
+
+          expect { subject }.to change { another_merge_request.reload.target_branch }
+            .from(merge_request.source_branch)
+            .to(merge_request.target_branch)
+        end
+
+        context 'when FF retarget_merge_requests is disabled' do
+          before do
+            stub_feature_flags(retarget_merge_requests: false)
+          end
+
+          include_examples 'does not retarget merge request'
+        end
+      end
+
+      context 'in the same project' do
+        let(:source_project) { project }
+
+        context 'and current is merged' do
+          before do
+            merge_request.mark_as_merged
+          end
+
+          it_behaves_like 'retargets merge request'
+        end
+
+        context 'and current is closed' do
+          before do
+            merge_request.close
+          end
+
+          it_behaves_like 'does not retarget merge request'
+        end
+
+        context 'and another is closed' do
+          before do
+            another_merge_request.close
+          end
+
+          it_behaves_like 'does not retarget merge request'
+        end
+
+        context 'and another is merged' do
+          before do
+            another_merge_request.mark_as_merged
+          end
+
+          it_behaves_like 'does not retarget merge request'
+        end
+      end
+
+      context 'in forked project' do
+        let!(:source_project) { fork_project(project) }
+
+        context 'when user has access to source project' do
+          before do
+            source_project.add_developer(user)
+            merge_request.mark_as_merged
+          end
+
+          it_behaves_like 'retargets merge request'
+        end
+
+        context 'when user does not have access to source project' do
+          it_behaves_like 'does not retarget merge request'
+        end
+      end
+
+      context 'and current and another MR is from a fork' do
+        let(:project) { create(:project) }
+        let(:source_project) { fork_project(project) }
+
+        let(:merge_request) do
+          create(:merge_request,
+            source_project: source_project,
+            target_project: project
+          )
+        end
+
+        before do
+          source_project.add_developer(user)
+        end
+
+        it_behaves_like 'does not retarget merge request'
+      end
+    end
+
+    context 'when many merge requests are to be retargeted' do
+      let!(:many_merge_requests) do
+        create_list(:merge_request, 10, :unique_branches,
+          source_project: merge_request.source_project,
+          target_project: merge_request.source_project,
+          target_branch: merge_request.source_branch
+        )
+      end
+
+      before do
+        merge_request.mark_as_merged
+      end
+
+      it 'retargets only 4 of them' do
+        subject
+
+        expect(many_merge_requests.each(&:reload).pluck(:target_branch).tally)
+          .to eq(
+            merge_request.source_branch => 6,
+            merge_request.target_branch => 4
+          )
+      end
+    end
+  end
+end

spec/workers/merge_requests/delete_source_branch_worker_spec.rb

@@ -13,6 +13,7 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do
     context 'with a non-existing merge request' do
       it 'does nothing' do
         expect(::Branches::DeleteService).not_to receive(:new)
+        expect(::MergeRequests::RetargetChainService).not_to receive(:new)
 
         worker.perform(non_existing_record_id, sha, user.id)
       end
@@ -21,6 +22,7 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do
     context 'with a non-existing user' do
       it 'does nothing' do
         expect(::Branches::DeleteService).not_to receive(:new)
+        expect(::MergeRequests::RetargetChainService).not_to receive(:new)
 
         worker.perform(merge_request.id, sha, non_existing_record_id)
       end
@@ -35,9 +37,18 @@ RSpec.describe MergeRequests::DeleteSourceBranchWorker do
         worker.perform(merge_request.id, sha, user.id)
       end
 
+      it 'calls service to try retarget merge requests' do
+        expect_next_instance_of(::MergeRequests::RetargetChainService) do |instance|
+          expect(instance).to receive(:execute).with(merge_request)
+        end
+
+        worker.perform(merge_request.id, sha, user.id)
+      end
+
       context 'source branch sha does not match' do
         it 'does nothing' do
           expect(::Branches::DeleteService).not_to receive(:new)
+          expect(::MergeRequests::RetargetChainService).not_to receive(:new)
 
           worker.perform(merge_request.id, 'new-source-branch-sha', user.id)
         end