Merge branch '34897-delete-branch-after-merge' into 'master'

Resolve "remove source branch" checkbox from merge widget being ignored

Closes #34897

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

4 files changed, 38 insertions, 14 deletions

diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index a110abf8256..8c5821aa870 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -60,13 +60,9 @@ module MergeRequests
     def after_merge
       MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
 
-      if params[:should_remove_source_branch].present? || @merge_request.force_remove_source_branch?
-        # Verify again that the source branch can be removed, since branch may be protected,
-        # or the source branch may have been updated.
-        if @merge_request.can_remove_source_branch?(branch_deletion_user)
-          DeleteBranchService.new(@merge_request.source_project, branch_deletion_user)
-            .execute(merge_request.source_branch)
-        end
+      if delete_source_branch?
+        DeleteBranchService.new(@merge_request.source_project, branch_deletion_user)
+          .execute(merge_request.source_branch)
       end
     end
 
@@ -78,6 +74,14 @@ module MergeRequests
       @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
     end
 
+    # Verify again that the source branch can be removed, since branch may be protected,
+    # or the source branch may have been updated, or the user may not have permission
+    #
+    def delete_source_branch?
+      params.fetch('should_remove_source_branch', @merge_request.force_remove_source_branch?) &&
+        @merge_request.can_remove_source_branch?(branch_deletion_user)
+    end
+
     # Logs merge error message and cleans `MergeRequest#merge_jid`.
     #
     def handle_merge_error(log_message:, save_message_on_model: false)
diff --git a/changelogs/unreleased/34897-delete-branch-after-merge.yml b/changelogs/unreleased/34897-delete-branch-after-merge.yml
new file mode 100644
index 00000000000..96631aa95c8
--- /dev/null
+++ b/changelogs/unreleased/34897-delete-branch-after-merge.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed 'Removed source branch' checkbox in merge widget being ignored.
+merge_request: 14832
+author:
+type: fixed
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 2c732aaf4ed..7c4a22c94c2 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -73,6 +73,12 @@ FactoryGirl.define do
       merge_user author
     end
 
+    trait :remove_source_branch do
+      merge_params do
+        { 'force_remove_source_branch' => '1' }
+      end
+    end
+
     after(:build) do |merge_request|
       target_project = merge_request.target_project
       source_project = merge_request.source_project
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index 80213d093f1..d1043f99b5a 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -185,7 +185,7 @@ describe MergeRequests::MergeService do
     context 'source branch removal' do
       context 'when the source branch is protected' do
         let(:service) do
-          described_class.new(project, user, should_remove_source_branch: '1')
+          described_class.new(project, user, 'should_remove_source_branch' => true)
         end
 
         before do
@@ -200,7 +200,7 @@ describe MergeRequests::MergeService do
 
       context 'when the source branch is the default branch' do
         let(:service) do
-          described_class.new(project, user, should_remove_source_branch: '1')
+          described_class.new(project, user, 'should_remove_source_branch' => true)
         end
 
         before do
@@ -215,10 +215,10 @@ describe MergeRequests::MergeService do
 
       context 'when the source branch can be removed' do
         context 'when MR author set the source branch to be removed' do
-          let(:service) do
-            merge_request.merge_params['force_remove_source_branch'] = '1'
-            merge_request.save!
-            described_class.new(project, user, commit_message: 'Awesome message')
+          let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+
+          before do
+            merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' })
           end
 
           it 'removes the source branch using the author user' do
@@ -227,11 +227,20 @@ describe MergeRequests::MergeService do
               .and_call_original
             service.execute(merge_request)
           end
+
+          context 'when the merger set the source branch not to be removed' do
+            let(:service) { described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => false) }
+
+            it 'does not delete the source branch' do
+              expect(DeleteBranchService).not_to receive(:new)
+              service.execute(merge_request)
+            end
+          end
         end
 
         context 'when MR merger set the source branch to be removed' do
           let(:service) do
-            described_class.new(project, user, commit_message: 'Awesome message', should_remove_source_branch: '1')
+            described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => true)
           end
 
           it 'removes the source branch using the current user' do