diff options
18 files changed, 268 insertions, 65 deletions
diff --git a/.gitignore b/.gitignore index f5b6427ca03..881b3fb81ac 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ public/assets/ public/uploads.* public/uploads/ shared/artifacts/ +TODO rails_best_practices_output.html /tags tmp/ diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 89f9e8fa6a8..131dfda6b5f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -254,15 +254,14 @@ class MergeRequest < ActiveRecord::Base end end - def can_cancel_merge_when_build_succeeds?(user) - can_be_merged_by?(user) || self.author == user + def can_cancel_merge_when_build_succeeds?(current_user) + can_be_merged_by?(current_user) || self.author == current_user end - def can_remove_source_branch? - for_fork? && - !project.protected_branch(source_branch) && - !project.repository.root_ref(source_branch) && - can?(current_user, :push_code, project) + def can_remove_source_branch?(current_user) + !source_project.protected_branch?(source_branch) && + !source_project.root_ref?(source_branch) && + Ability.abilities.allowed?(current_user, :push_code, source_project) end def mr_and_commit_notes diff --git a/app/services/merge_requests/merge_when_build_succeeds_service.rb b/app/services/merge_requests/merge_when_build_succeeds_service.rb index 15dcace5dfb..2f101e53a3f 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -11,13 +11,11 @@ module MergeRequests unless already_approved merge_request.merge_when_build_succeeds = true merge_request.merge_user = @current_user - end - - merge_request.save - unless already_approved SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit) end + + merge_request.save end # Triggers the automatic merge of merge_request once the build succeeds diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 7de4221c4c4..ed557fef814 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -132,7 +132,7 @@ class SystemNoteService # Called when 'merge when build succeeds' is executed def self.merge_when_build_succeeds(noteable, project, author, ci_commit) - body = "Approved an automatic merge when the build for #{ci_commit.sha} succeeds" + body = "Enabled an automatic merge when the build for #{ci_commit.sha} succeeds" create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/app/views/projects/merge_requests/widget/_merged.html.haml b/app/views/projects/merge_requests/widget/_merged.html.haml index a788fcea23f..52e34ee617e 100644 --- a/app/views/projects/merge_requests/widget/_merged.html.haml +++ b/app/views/projects/merge_requests/widget/_merged.html.haml @@ -13,7 +13,7 @@ %span.label-branch= @merge_request.target_branch The source branch has been removed. - - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) + - elsif @merge_request.can_remove_source_branch?(current_user) .remove_source_branch_widget %p = succeed '.' do @@ -48,5 +48,3 @@ $('.remove_source_branch_in_progress').hide(); $('.remove_source_branch_widget.failed').show(); }); - - 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 5ec623b472c..279e2ec91f8 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -7,13 +7,13 @@ - ci_commit = @merge_request.ci_commit - if ci_commit && ci_commit.active? = f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do - Merge when Build Succeeds + Merge When Build Succeeds = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do Accept Merge Request Now - else = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do Accept Merge Request - - if @merge_request.can_remove_source_branch? + - if @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 51e18f84424..43ba49c5a5e 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 @@ -3,22 +3,25 @@ to be merged automatically when #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. %div - - if @merge_request.merge_params["should_remove_source_branch"].present? + - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present? + - if source_branch_removed = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch The source branch will be removed. + - else %p = succeed '.' do The changes will be merged into %span.label-branch= @merge_request.target_branch The source branch will not be removed. - .clearfix.prepend-top-10 - - if @merge_request.can_remove_source_branch? && !@merge_request.merge_params["should_remove_source_branch"].present? - = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do - = icon('times') - Remove Source Branch When Merged - - if @merge_request.merge_when_build_succeeds - = link_to cancel_merge_when_build_succeeds_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request), remote: true, method: :post, class: "btn btn-grouped btn-warning btn-sm" do - Cancel Automatic Merge + - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + .clearfix.prepend-top-10 + - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed + = link_to merge_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request, merge_when_build_succeeds: true, should_remove_source_branch: true), remote: true, method: :post, class: "btn btn-grouped btn-primary btn-sm remove_source_branch" do + = icon('times') + Remove Source Branch When Merged + - if @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + = link_to cancel_merge_when_build_succeeds_namespace_project_merge_request_path(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request), remote: true, method: :post, class: "btn btn-grouped btn-warning btn-sm" do + Cancel Automatic Merge diff --git a/lib/api/entities.rb b/lib/api/entities.rb index d6aec03d7f5..6f9f71b0945 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -169,6 +169,7 @@ module API expose :description expose :work_in_progress?, as: :work_in_progress expose :milestone, using: Entities::Milestone + expose :merge_when_build_succeeds end class MergeRequestChanges < MergeRequest diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index b72f816932b..32cb1137ef7 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -179,11 +179,11 @@ module API # Merge MR # # Parameters: - # id (required) - The ID of a project - # merge_request_id (required) - ID of MR - # merge_commit_message (optional) - Custom merge commit message - # should_remove_source_branch - When true, the source branch will be deleted if possible - # merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds + # id (required) - The ID of a project + # merge_request_id (required) - ID of MR + # merge_commit_message (optional) - Custom merge commit message + # should_remove_source_branch (optional) - When true, the source branch will be deleted if possible + # merge_when_build_succeeds (optional) - When true, this MR will be merged when the build succeeds # Example: # PUT /projects/:id/merge_request/:merge_request_id/merge # @@ -193,12 +193,11 @@ module API # Merge request can not be merged # because user dont have permissions to push into target branch unauthorized! unless merge_request.can_be_merged_by?(current_user) - - not_allowed! unless merge_request.open? && !merge_request.work_in_progress? + not_allowed! if !merge_request.open? || merge_request.work_in_progress? merge_request.check_if_can_be_merged if merge_request.unchecked? - render_api_error!('Branch cannot be merged', 406) if merge_request.can_be_merged? + render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? merge_params = { commit_message: params[:merge_commit_message], @@ -206,7 +205,7 @@ module API } if parse_boolean(params[:merge_when_build_succeeds]) && merge_request.ci_commit && merge_request.ci_commit.active? - ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params). + ::MergeRequests::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user, merge_params). execute(merge_request) else ::MergeRequests::MergeService.new(merge_request.target_project, current_user, merge_params). @@ -224,11 +223,6 @@ module API post ":id/merge_request/:merge_request_id/cancel_merge_when_build_succeeds" do merge_request = user_project.merge_requests.find(params[:merge_request_id]) - allowed = ::Gitlab::GitAccess.new(current_user, user_project). - can_push_to_branch?(merge_request.target_branch) - - # Merge request can not be merged - # because user dont have permissions to push into target branch unauthorized! unless merge_request.can_cancel_merge_when_build_succeeds?(current_user) ::MergeRequest::MergeWhenBuildSucceedsService.new(merge_request.target_project, current_user).cancel(merge_request) diff --git a/spec/factories/ci/commits.rb b/spec/factories/ci/commits.rb index 79e000b7ccb..70e3fa319c6 100644 --- a/spec/factories/ci/commits.rb +++ b/spec/factories/ci/commits.rb @@ -2,17 +2,18 @@ # # Table name: commits # -# id :integer not null, primary key -# project_id :integer -# ref :string(255) -# sha :string(255) -# before_sha :string(255) -# push_data :text -# created_at :datetime -# updated_at :datetime -# tag :boolean default(FALSE) -# yaml_errors :text -# committed_at :datetime +# id :integer not null, primary key +# project_id :integer +# ref :string(255) +# sha :string(255) +# before_sha :string(255) +# push_data :text +# created_at :datetime +# updated_at :datetime +# tag :boolean default(FALSE) +# yaml_errors :text +# committed_at :datetime +# gl_project_id :integer # # Read about factories at https://github.com/thoughtbot/factory_girl diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb index 52de437052d..8898b71e2a3 100644 --- a/spec/factories/commit_statuses.rb +++ b/spec/factories/commit_statuses.rb @@ -5,7 +5,7 @@ FactoryGirl.define do name 'default' status 'success' description 'commit status' - commit factory: :ci_commit + commit factory: :ci_commit_with_one_job factory :generic_commit_status, class: GenericCommitStatus do name 'generic' diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb new file mode 100644 index 00000000000..b25a3f05e29 --- /dev/null +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +feature 'Merge When Build Succeeds', feature: true, js: true do + let(:user) { create(:user) } + + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") } + + before do + project.team << [user, :master] + project.enable_ci + end + + context "Active build for Merge Request" do + before do + ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) + ci_build = create(:ci_build, commit: ci_commit) + + login_as user + visit_merge_request(merge_request) + end + + it 'displays the Merge When Build Succeeds button' do + expect(page).to have_button "Merge When Build Succeeds" + end + + context "Merge When Build succeeds enabled" do + before do + click_button "Merge When Build Succeeds" + end + + it 'activates Merge When Build Succeeds feature' do + expect(page).to have_link "Cancel Automatic Merge" + + expect(page).to have_content "Approved by #{user.name} to be merged automatically when the build succeeds." + expect(page).to have_content "The source branch will not be removed." + end + end + end + + context 'When it is enabled' do + # No clue how, but push a new commit to the branch + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, # source_branch: "mepmep", + author: user, title: "Bug NS-04", merge_when_build_succeeds: true) } + + before do + merge_request.source_project.team << [user, :master] + merge_request.source_branch = "feature" + merge_request.target_branch = "master" + merge_request.save! + + ci_commit = create(:ci_commit, gl_project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) + ci_build = create(:ci_build, commit: ci_commit) + + login_as user + visit_merge_request(merge_request) + end + + it 'cancels the automatic merge' do + click_link "Cancel Automatic Merge" + + expect(page).to have_button "Merge When Build Succeeds" + end + + it "allows the user to remove the source branch" do + expect(page).to have_link "Remove Source Branch When Merged" + end + end + + context 'Build is not active' do + it "should not allow for enabling" do + visit_merge_request(merge_request) + expect(page).not_to have_button "Merge When Build Succeeds" + end + end + + def visit_merge_request(merge_request) + visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 1bd09a1b0fb..c7a9765825e 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -174,6 +174,30 @@ describe MergeRequest do end end + describe '#can_remove_source_branch' do + let(:user) { build(:user)} + + before do + subject.source_project.team << [user, :master] + end + + it "cant be merged when its a a protected branch" do + subject.source_project.protected_branches = []; + + expect(subject.can_remove_source_branch?(user)).to be_falsey + end + + it "cant remove a root ref" do + subject.source_branch = "master"; + + expect(subject.can_remove_source_branch?(user)).to be_falsey + end + + it "is truthy in all other cases" do + expect(subject.can_remove_source_branch?(user)) + end + end + describe "#reset_merge_when_build_succeeds" do let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true } it "sets the item to false" do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index a68c7b1e461..91ae2059e95 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -303,19 +303,21 @@ describe API::API, api: true do end describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do + let (:ci_commit) { create(:ci_commit_without_jobs) } + it "should return merge_request in case of success" do put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) expect(response.status).to eq(200) end - it "should return 405 if branch can't be merged" do + it "should return 406 if branch can't be merged" do allow_any_instance_of(MergeRequest). to receive(:can_be_merged?).and_return(false) put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) - expect(response.status).to eq(405) + expect(response.status).to eq(406) expect(json_response['message']).to eq('Branch cannot be merged') end @@ -340,6 +342,17 @@ describe API::API, api: true do expect(response.status).to eq(401) expect(json_response['message']).to eq('401 Unauthorized') end + + it "enables merge when build succeeds if the ci is active" do + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:active?).and_return(true) + + put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user), merge_when_build_succeeds: true + + expect(response.status).to eq(200) + expect(json_response['title']).to eq('Test') + expect(json_response['merge_when_build_succeeds']).to eq(true) + end end describe "PUT /projects/:id/merge_request/:merge_request_id" do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 7483f51de03..242524286fa 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -13,12 +13,12 @@ describe MergeRequests::MergeService do describe :execute do context 'valid params' do - let(:service) { MergeRequests::MergeService.new(project, user, {}) } + let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } before do allow(service).to receive(:execute_hooks) - service.execute(merge_request, 'Awesome message') + service.execute(merge_request) end it { expect(merge_request).to be_valid } @@ -37,14 +37,14 @@ describe MergeRequests::MergeService do end context "error handling" do - let(:service) { MergeRequests::MergeService.new(project, user, {}) } + let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } it 'saves error if there is an exception' do allow(service).to receive(:repository).and_raise("error") allow(service).to receive(:execute_hooks) - service.execute(merge_request, 'Awesome message') + service.execute(merge_request) expect(merge_request.merge_error).to eq("Something went wrong during merge") end diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb new file mode 100644 index 00000000000..8638539173b --- /dev/null +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -0,0 +1,78 @@ +require 'spec_helper' + +describe MergeRequests::MergeWhenBuildSucceedsService do + let(:user) { create(:user) } + let(:merge_request) { create(:merge_request) } + let(:mr_merge_if_green_enabled) { create(:merge_request, merge_when_build_succeeds: true, + source_branch: "source_branch", target_branch: project.default_branch, + source_project: project, target_project: project, state: "opened") } + let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch) } + let(:project) { create(:project) } + let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') } + + before do + project.ci_commits = [ci_commit] + project.save! + end + describe "#execute" do + context 'first time enabling' do + before do + allow(merge_request).to receive(:ci_commit).and_return(ci_commit) + end + + it 'sets the params, merge_user, and flag' do + service.execute(merge_request) + + expect(merge_request).to be_valid + expect(merge_request.merge_when_build_succeeds).to be_truthy + expect(merge_request.merge_params).to eq commit_message: 'Awesome message' + expect(merge_request.merge_user).to be user + + note = merge_request.notes.last + expect(note.note).to include 'Enabled an automatic merge when the build for' + end + end + + context 'allready approved' do + let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) } + let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } + + before do + allow(mr_merge_if_green_enabled).to receive(:ci_commit).and_return(ci_commit) + allow(mr_merge_if_green_enabled).to receive(:mergeable?).and_return(true) + allow(ci_commit).to receive(:success?).and_return(true) + end + + it 'updates the merge params' do + expect(SystemNoteService).not_to receive(:merge_when_build_succeeds) + + service.execute(mr_merge_if_green_enabled) + expect(mr_merge_if_green_enabled.merge_params).to have_key(:new_key) + end + end + end + + describe "#trigger" do + let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch, status: "success") } + + it "merges all merge requests with merge when build succeeds enabled" do + allow_any_instance_of(MergeRequest).to receive(:ci_commit).and_return(ci_commit) + allow(ci_commit).to receive(:success?).and_return(true) + + expect(MergeWorker).to receive(:perform_async) + service.trigger(build) + end + end + + describe "#cancel" do + before do + service.cancel(mr_merge_if_green_enabled) + end + + it "resets all the merge_when_build_succeeds params" do + expect(mr_merge_if_green_enabled.merge_when_build_succeeds).to be_falsey + expect(mr_merge_if_green_enabled.merge_params).to eq({}) + expect(mr_merge_if_green_enabled.merge_user).to be nil + end + end +end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 7ee4488521d..18b2659c1f6 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -17,7 +17,8 @@ describe MergeRequests::RefreshService do source_project: @project, source_branch: 'master', target_branch: 'feature', - target_project: @project) + target_project: @project, + merge_when_build_succeeds: true) @fork_merge_request = create(:merge_request, source_project: @fork_project, @@ -46,6 +47,7 @@ describe MergeRequests::RefreshService do it { expect(@merge_request.notes).not_to be_empty } it { expect(@merge_request).to be_open } + it { expect(@merge_request.merge_when_build_succeeds).to be_falsey} it { expect(@fork_merge_request).to be_open } it { expect(@fork_merge_request.notes).to be_empty } end @@ -146,6 +148,7 @@ describe MergeRequests::RefreshService do end end + def reload_mrs @merge_request.reload @fork_merge_request.reload diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 35912ece644..5d41a5cdc69 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -208,18 +208,28 @@ describe SystemNoteService do end describe '.merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit, gl_project: project } - let(:merge_request) { create :merge_request, project: project } + let(:ci_commit) { create :ci_commit_without_jobs } + let(:noteable) { create :merge_request } - subject { described_class.merge_when_build_succeeds(merge_request, project, author) } + subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) } it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - allow(merge_request).to receive(:ci_commit).and_return(ci_commit) - allow(ci_commit).to receive(:short_sha).and_return('12345678') + expect(subject.note).to eq "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds" + end + end + + describe '.cancel_merge_when_build_succeeds' do + let(:ci_commit) { create :ci_commit_without_jobs } + let(:noteable) { create :merge_request } + + subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } - expect(subject.note).to eq "This merge request will be automatically merged when the build for 12345678 succeeds" + it_behaves_like 'a system note' + + it "posts the Merge When Build Succeeds system note" do + expect(subject.note).to eq "Canceled the automatic merge" end end |