Commit af3c98bb authored by Sean McGivern's avatar Sean McGivern

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
parents f160b2db d1acadce
Pipeline #12732064 passed with stages
in 96 minutes and 2 seconds
...@@ -60,13 +60,9 @@ module MergeRequests ...@@ -60,13 +60,9 @@ module MergeRequests
def after_merge def after_merge
MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
if params[:should_remove_source_branch].present? || @merge_request.force_remove_source_branch? if delete_source_branch?
# Verify again that the source branch can be removed, since branch may be protected, DeleteBranchService.new(@merge_request.source_project, branch_deletion_user)
# or the source branch may have been updated. .execute(merge_request.source_branch)
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
end end
end end
...@@ -78,6 +74,14 @@ module MergeRequests ...@@ -78,6 +74,14 @@ module MergeRequests
@merge_request.force_remove_source_branch? ? @merge_request.author : current_user @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
end 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`. # Logs merge error message and cleans `MergeRequest#merge_jid`.
# #
def handle_merge_error(log_message:, save_message_on_model: false) def handle_merge_error(log_message:, save_message_on_model: false)
......
---
title: Fixed 'Removed source branch' checkbox in merge widget being ignored.
merge_request: 14832
author:
type: fixed
...@@ -73,6 +73,12 @@ FactoryGirl.define do ...@@ -73,6 +73,12 @@ FactoryGirl.define do
merge_user author merge_user author
end end
trait :remove_source_branch do
merge_params do
{ 'force_remove_source_branch' => '1' }
end
end
after(:build) do |merge_request| after(:build) do |merge_request|
target_project = merge_request.target_project target_project = merge_request.target_project
source_project = merge_request.source_project source_project = merge_request.source_project
......
...@@ -185,7 +185,7 @@ describe MergeRequests::MergeService do ...@@ -185,7 +185,7 @@ describe MergeRequests::MergeService do
context 'source branch removal' do context 'source branch removal' do
context 'when the source branch is protected' do context 'when the source branch is protected' do
let(:service) 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 end
before do before do
...@@ -200,7 +200,7 @@ describe MergeRequests::MergeService do ...@@ -200,7 +200,7 @@ describe MergeRequests::MergeService do
context 'when the source branch is the default branch' do context 'when the source branch is the default branch' do
let(:service) 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 end
before do before do
...@@ -215,10 +215,10 @@ describe MergeRequests::MergeService do ...@@ -215,10 +215,10 @@ describe MergeRequests::MergeService do
context 'when the source branch can be removed' do context 'when the source branch can be removed' do
context 'when MR author set the source branch to be removed' do context 'when MR author set the source branch to be removed' do
let(:service) do let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
merge_request.merge_params['force_remove_source_branch'] = '1'
merge_request.save! before do
described_class.new(project, user, commit_message: 'Awesome message') merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' })
end end
it 'removes the source branch using the author user' do it 'removes the source branch using the author user' do
...@@ -227,11 +227,20 @@ describe MergeRequests::MergeService do ...@@ -227,11 +227,20 @@ describe MergeRequests::MergeService do
.and_call_original .and_call_original
service.execute(merge_request) service.execute(merge_request)
end 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 end
context 'when MR merger set the source branch to be removed' do context 'when MR merger set the source branch to be removed' do
let(:service) 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 end
it 'removes the source branch using the current user' do it 'removes the source branch using the current user' do
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment