diff options
| -rw-r--r-- | app/services/merge_requests/merge_service.rb | 18 | ||||
| -rw-r--r-- | changelogs/unreleased/34897-delete-branch-after-merge.yml | 5 | ||||
| -rw-r--r-- | spec/factories/merge_requests.rb | 6 | ||||
| -rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 23 |
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 |