From 7880a300dc9ef3fcceb7f1a6af6a6500b0b46e5c Mon Sep 17 00:00:00 2001
From: Jeroen Jacobs <git@jeroenj.be>
Date: Fri, 12 Feb 2016 21:41:31 +0100
Subject: [PATCH] Allows MR authors to have the source branch removed when
merging the MR
---
CHANGELOG | 1 +
.../projects/merge_requests_controller.rb | 3 ++-
app/models/merge_request.rb | 17 ++++++++++++++++-
app/services/merge_requests/create_service.rb | 7 +++++--
app/services/merge_requests/merge_service.rb | 8 ++++++--
app/services/merge_requests/update_service.rb | 2 ++
.../widget/open/_accept.html.haml | 5 ++++-
.../open/_merge_when_build_succeeds.html.haml | 5 ++---
.../widget/open/_not_allowed.html.haml | 4 +++-
app/views/shared/issuable/_form.html.haml | 7 +++++++
spec/models/merge_request_spec.rb | 7 ++++++-
.../merge_requests/create_service_spec.rb | 4 +++-
.../merge_requests/merge_service_spec.rb | 15 +++++++++++++++
.../merge_requests/update_service_spec.rb | 4 +++-
14 files changed, 75 insertions(+), 14 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index 4c810ab21f2..ca33eeef8d9 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -65,6 +65,7 @@ v 8.8.0 (unreleased)
- All Grape API helpers are now instrumented
- Improve Issue formatting for the Slack Service (Jeroen van Baarsen)
- Fixed advice on invalid permissions on upload path !2948 (Ludovic Perrine)
+ - Allows MR authors to have the source branch removed when merging the MR. !2801 (Jeroen Jacobs)
v 8.7.6
- Fix links on wiki pages for relative url setups. !4131 (Artem Sidorenko)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index c5757a24624..f137c12d215 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -334,7 +334,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
params.require(:merge_request).permit(
:title, :assignee_id, :source_project_id, :source_branch,
:target_project_id, :target_branch, :milestone_id,
- :state_event, :description, :task_num, label_ids: []
+ :state_event, :description, :task_num, :force_remove_source_branch,
+ label_ids: []
)
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 45ddcf6812a..722c258244c 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -286,6 +286,18 @@ class MergeRequest < ActiveRecord::Base
last_commit == source_project.commit(source_branch)
end
+ def should_remove_source_branch?
+ merge_params['should_remove_source_branch'].present?
+ end
+
+ def force_remove_source_branch?
+ merge_params['force_remove_source_branch'].present?
+ end
+
+ def remove_source_branch?
+ should_remove_source_branch? || force_remove_source_branch?
+ end
+
def mr_and_commit_notes
# Fetch comments only from last 100 commits
commits_for_notes_limit = 100
@@ -426,7 +438,10 @@ class MergeRequest < ActiveRecord::Base
self.merge_when_build_succeeds = false
self.merge_user = nil
- self.merge_params = nil
+ if merge_params
+ merge_params.delete('should_remove_source_branch')
+ merge_params.delete('commit_message')
+ end
self.save
end
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 33609d01f20..96a25330af1 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -8,11 +8,14 @@ module MergeRequests
@project = Project.find(params[:target_project_id]) if params[:target_project_id]
filter_params
- label_params = params[:label_ids]
- merge_request = MergeRequest.new(params.except(:label_ids))
+ label_params = params.delete(:label_ids)
+ force_remove_source_branch = params.delete(:force_remove_source_branch)
+
+ merge_request = MergeRequest.new(params)
merge_request.source_project = source_project
merge_request.target_project ||= source_project
merge_request.author = current_user
+ merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch
if merge_request.save
merge_request.update_attributes(label_ids: label_params)
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 9a58383b398..9aaf5a5e561 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -45,10 +45,14 @@ module MergeRequests
def after_merge
MergeRequests::PostMergeService.new(project, current_user).execute(merge_request)
- if params[:should_remove_source_branch].present?
- DeleteBranchService.new(@merge_request.source_project, current_user).
+ if params[:should_remove_source_branch].present? || @merge_request.force_remove_source_branch?
+ DeleteBranchService.new(@merge_request.source_project, branch_deletion_user).
execute(merge_request.source_branch)
end
end
+
+ def branch_deletion_user
+ @merge_request.force_remove_source_branch? ? @merge_request.author : current_user
+ end
end
end
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 477c64e7377..026a37997d4 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -11,6 +11,8 @@ module MergeRequests
params.except!(:target_project_id)
params.except!(:source_branch)
+ merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
+
update(merge_request)
end
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index 807833741af..cfdf4edac37 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -25,7 +25,10 @@
- else
= f.button class: "btn btn-create btn-grouped js-merge-button accept_merge_request #{status_class}" do
Accept Merge Request
- - if @merge_request.can_remove_source_branch?(current_user)
+ - if @merge_request.force_remove_source_branch?
+ .accept-control
+ The source branch will be removed.
+ - elsif @merge_request.can_remove_source_branch?(current_user)
.accept-control.checkbox
= label_tag :should_remove_source_branch, class: "remove_source_checkbox" do
= check_box_tag :should_remove_source_branch
diff --git a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
index 2168294c683..b83ddcab3a4 100644
--- a/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml
@@ -2,17 +2,16 @@
Set by #{link_to_member(@project, @merge_request.merge_user, avatar: true)}
to be merged automatically when the build succeeds.
%div
- - should_remove_source_branch = @merge_request.merge_params["should_remove_source_branch"].present?
%p
= succeed '.' do
The changes will be merged into
%span.label-branch= @merge_request.target_branch
- - if should_remove_source_branch
+ - if @merge_request.remove_source_branch?
The source branch will be removed.
- else
The source branch will not be removed.
- - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch && @merge_request.merge_user == current_user
+ - remove_source_branch_button = !@merge_request.remove_source_branch? && @merge_request.can_remove_source_branch?(current_user) && @merge_request.merge_user == current_user
- user_can_cancel_automatic_merge = @merge_request.can_cancel_merge_when_build_succeeds?(current_user)
- if remove_source_branch_button || user_can_cancel_automatic_merge
.clearfix.prepend-top-10
diff --git a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml
index a8145558ca8..57ce1959021 100644
--- a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml
@@ -1,4 +1,6 @@
-%h4
+%h4
Ready to be merged automatically
%p
Ask someone with write access to this repository to merge this request.
+ - if @merge_request.force_remove_source_branch?
+ The source branch will be removed.
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 5c52cc6d1da..1181edec3f9 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -114,6 +114,13 @@
- if @merge_request.new_record?
= link_to 'Change branches', mr_change_branches_path(@merge_request)
+ - if @merge_request.can_remove_source_branch?(current_user)
+ .form-group
+ .col-sm-10.col-sm-offset-2
+ .checkbox
+ = label_tag 'merge_request[force_remove_source_branch]' do
+ = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
+ Remove source branch when merge request is accepted.
- is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
.row-content-block{class: (is_footer ? "footer-block" : "middle-block")}
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 9eef08c6d00..e269ff26a04 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -260,13 +260,18 @@ describe MergeRequest, models: true do
end
describe "#reset_merge_when_build_succeeds" do
- let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user) }
+ let(:merge_if_green) do
+ create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user),
+ merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" }
+ end
it "sets the item to false" do
merge_if_green.reset_merge_when_build_succeeds
merge_if_green.reload
expect(merge_if_green.merge_when_build_succeeds).to be_falsey
+ expect(merge_if_green.merge_params["should_remove_source_branch"]).to be_nil
+ expect(merge_if_green.merge_params["commit_message"]).to be_nil
end
end
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index 120f4d6a669..e433f49872d 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -12,7 +12,8 @@ describe MergeRequests::CreateService, services: true do
title: 'Awesome merge_request',
description: 'please fix',
source_branch: 'feature',
- target_branch: 'master'
+ target_branch: 'master',
+ force_remove_source_branch: '1'
}
end
@@ -29,6 +30,7 @@ describe MergeRequests::CreateService, services: true do
it { expect(@merge_request).to be_valid }
it { expect(@merge_request.title).to eq('Awesome merge_request') }
it { expect(@merge_request.assignee).to be_nil }
+ it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') }
it 'should execute hooks with default action' do
expect(service).to have_received(:execute_hooks).with(@merge_request)
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index ceb3f97280e..1b0396eb686 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -38,6 +38,21 @@ describe MergeRequests::MergeService, services: true do
end
end
+ context 'remove source branch by author' do
+ let(:service) do
+ merge_request.merge_params['force_remove_source_branch'] = '1'
+ merge_request.save!
+ MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message')
+ end
+
+ it 'removes the source branch' do
+ expect(DeleteBranchService).to receive(:new).
+ with(merge_request.source_project, merge_request.author).
+ and_call_original
+ service.execute(merge_request)
+ end
+ end
+
context "error handling" do
let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') }
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 213e8c2eb3a..cc7dc216a76 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -39,7 +39,8 @@ describe MergeRequests::UpdateService, services: true do
assignee_id: user2.id,
state_event: 'close',
label_ids: [label.id],
- target_branch: 'target'
+ target_branch: 'target',
+ force_remove_source_branch: '1'
}
end
@@ -61,6 +62,7 @@ describe MergeRequests::UpdateService, services: true do
it { expect(@merge_request.labels.count).to eq(1) }
it { expect(@merge_request.labels.first.title).to eq(label.name) }
it { expect(@merge_request.target_branch).to eq('target') }
+ it { expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') }
it 'should execute hooks with update action' do
expect(service).to have_received(:execute_hooks).
--
GitLab