diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-05-21 00:41:02 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2016-05-21 20:26:33 +0300 |
commit | 2cbaf3c43a81ab73ddc84ced16ab200b91653a46 (patch) | |
tree | f2b987c2fd1f8c7ca73212fb97022b882de0b121 | |
parent | 5dc64ce3790f08e6020cadf57a3169c60513e2cb (diff) |
Merge branch 'rm-source-branch' into 'master'
Allows MR authors to have the source branch removed when merging the MR
closes #13191
The location of the checkbox might not be optimal so any feedback is welcome. Any other feedback too obviously.
Screenshot:
![Screenshot_2016-02-17_21.25.24](/uploads/a9c3eaafb39c6f5b4f0949a2278af6da/Screenshot_2016-02-17_21.25.24.png)
See merge request !2801
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 3 | ||||
-rw-r--r-- | app/models/merge_request.rb | 17 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 7 | ||||
-rw-r--r-- | app/services/merge_requests/merge_service.rb | 8 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_accept.html.haml | 5 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_merge_when_build_succeeds.html.haml | 5 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_not_allowed.html.haml | 4 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 7 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 7 | ||||
-rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 15 | ||||
-rw-r--r-- | spec/services/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). |