diff options
author | Zeger-Jan van de Weg <mail@zjvandeweg.nl> | 2015-12-03 12:27:34 +0300 |
---|---|---|
committer | Zeger-Jan van de Weg <mail@zjvandeweg.nl> | 2015-12-05 17:42:38 +0300 |
commit | 2462a96e459c95f987f39e3c380de7c7cc350cfd (patch) | |
tree | 065157ddb9e707a911bc38c878662bfaf0cca3c2 | |
parent | 25907ebe476a24bfdd2c451f18227d4fcf314b07 (diff) |
Incorporate feedback
14 files changed, 134 insertions, 83 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 131dfda6b5f..1f81e23c7a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -2,25 +2,28 @@ # # Table name: merge_requests # -# id :integer not null, primary key -# target_branch :string(255) not null -# source_branch :string(255) not null -# source_project_id :integer not null -# author_id :integer -# assignee_id :integer -# title :string(255) -# created_at :datetime -# updated_at :datetime -# milestone_id :integer -# state :string(255) -# merge_status :string(255) -# target_project_id :integer not null -# iid :integer -# description :text -# position :integer default(0) -# locked_at :datetime -# updated_by_id :integer -# merge_error :string(255) +# id :integer not null, primary key +# target_branch :string(255) not null +# source_branch :string(255) not null +# source_project_id :integer not null +# author_id :integer +# assignee_id :integer +# title :string(255) +# created_at :datetime +# updated_at :datetime +# milestone_id :integer +# state :string(255) +# merge_status :string(255) +# target_project_id :integer not null +# iid :integer +# description :text +# position :integer default(0) +# locked_at :datetime +# updated_by_id :integer +# merge_error :string(255) +# merge_params :text (serialized to hash) +# merge_when_build_succeeds :boolean default(false), not null +# merge_user_id :integer # require Rails.root.join("app/models/commit") @@ -124,6 +127,7 @@ class MergeRequest < ActiveRecord::Base validates :source_branch, presence: true validates :target_project, presence: true validates :target_branch, presence: true + validates :merge_user, presence: true, if: :merge_when_build_succeeds? validate :validate_branches validate :validate_fork @@ -496,8 +500,6 @@ class MergeRequest < ActiveRecord::Base end def ci_commit - if last_commit - source_project.ci_commit(last_commit.id) - end + @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit end end 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 2f101e53a3f..5cf7404a493 100644 --- a/app/services/merge_requests/merge_when_build_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_build_succeeds_service.rb @@ -1,6 +1,6 @@ module MergeRequests class MergeWhenBuildSucceedsService < MergeRequests::BaseService - # Marks the passed `merge_request` to be marked when the build succeeds or + # Marks the passed `merge_request` to be merged when the build succeeds or # updates the params for the automatic merge def execute(merge_request) merge_request.merge_params.merge!(params) @@ -12,7 +12,7 @@ module MergeRequests merge_request.merge_when_build_succeeds = true merge_request.merge_user = @current_user - SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.ci_commit) + SystemNoteService.merge_when_build_succeeds(merge_request, @project, @current_user, merge_request.last_commit) end merge_request.save @@ -25,8 +25,7 @@ module MergeRequests merge_requests.each do |merge_request| next unless merge_request.merge_when_build_succeeds? - ci_commit = merge_request.ci_commit - if ci_commit && ci_commit.success? && merge_request.mergeable? + if merge_request.ci_commit && merge_request.ci_commit.success? && merge_request.mergeable? MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params) end end @@ -34,7 +33,7 @@ module MergeRequests # Cancels the automatic merge def cancel(merge_request) - if merge_request.merge_when_build_succeeds? && merge_request.open? && !merge_request.merged? + if merge_request.merge_when_build_succeeds? && merge_request.open? merge_request.reset_merge_when_build_succeeds SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index ed557fef814..f84e480ca9c 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -131,15 +131,15 @@ class SystemNoteService end # Called when 'merge when build succeeds' is executed - def self.merge_when_build_succeeds(noteable, project, author, ci_commit) - body = "Enabled an automatic merge when the build for #{ci_commit.sha} succeeds" + def self.merge_when_build_succeeds(noteable, project, author, last_commit) + body = "Enabled an automatic merge when the build for #{last_commit.to_reference} succeeds" create_note(noteable: noteable, project: project, author: author, note: body) end # Called when 'merge when build succeeds' is canceled def self.cancel_merge_when_build_succeeds(noteable, project, author) - body = "Canceled the automatic merge" + body = "Cancelled the automatic merge" create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index c5234c0618c..d10ccb30571 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -1,4 +1,3 @@ -- ci_commit = merge_request.ci_commit %li{ class: mr_css_classes(merge_request) } .merge-request-title %span.merge-request-title-text @@ -7,8 +6,8 @@ - merge_request.labels.each do |label| = link_to_label(label, project: merge_request.project) .pull-right.light - - if ci_commit - = render_ci_status(ci_commit) + - if merge_request.ci_commit + = render_ci_status(merge_request.ci_commit) - if merge_request.merged? %span %i.fa.fa-check diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index ba5ad22bca7..9513a18f105 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,13 +1,12 @@ -- ci_commit = @merge_request.ci_commit -- if ci_commit - - status = ci_commit.status +- if @merge_request.ci_commit + - status = @merge_request.ci_commit.status .mr-widget-heading .ci_widget{class: "ci-#{status}"} - = ci_status_icon(ci_commit) + = ci_status_icon(@merge_request.ci_commit) %span CI build #{status} for #{@merge_request.last_commit_short_sha}. %span.ci-coverage - = link_to "View build details", ci_status_path(ci_commit) + = link_to "View build details", ci_status_path(@merge_request.ci_commit) - elsif @merge_request.has_ci? - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX 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 279e2ec91f8..f7d872aa455 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -4,8 +4,7 @@ = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container .accept-action - - ci_commit = @merge_request.ci_commit - - if ci_commit && ci_commit.active? + - if @merge_request.ci_commit && @merge_request.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 = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do 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 43ba49c5a5e..9788d68b54e 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 @@ -1,27 +1,27 @@ %h4 - Approved by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} + Set by #{link_to_member(@project, @merge_request.merge_user, avatar: true)} to be merged automatically when #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. %div - - source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present? - - if source_branch_removed + - 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 - The source branch will be removed. - - else - %p - = succeed '.' do - The changes will be merged into - %span.label-branch= @merge_request.target_branch + - if should_remove_source_branch + The source branch will be removed. + - else The source branch will not be removed. - - if (@merge_request.can_remove_source_branch?(current_user) && !source_branch_removed) || @merge_request.can_cancel_merge_when_build_succeeds?(current_user) + - remove_source_branch_button = @merge_request.can_remove_source_branch?(current_user) && !should_remove_source_branch + - 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 - - if @merge_request.can_remove_source_branch?(current_user) && !source_branch_removed + - if remove_source_branch_button = 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) + + - if user_can_cancel_automatic_merge = 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/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 729a49c9f72..5b4d7f41bc4 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -65,6 +65,11 @@ FactoryGirl.define do target_branch "master" end + trait :merge_when_build_succeeds do + merge_when_build_succeeds true + merge_user author + end + factory :closed_merge_request, traits: [:closed] factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] diff --git a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb index b25a3f05e29..2e64e903d1e 100644 --- a/spec/features/merge_requests/merge_when_build_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_build_succeeds_spec.rb @@ -1,4 +1,6 @@ require 'spec_helper' +# rubocop:disable Lint/UselessAssignment +# As rubocop doesn't see a need for both `ci_commit` and `ci_build` feature 'Merge When Build Succeeds', feature: true, js: true do let(:user) { create(:user) } @@ -32,16 +34,20 @@ feature 'Merge When Build Succeeds', feature: true, js: true do 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 "Set by #{user.name} to be merged automatically when the build succeeds." expect(page).to have_content "The source branch will not be removed." + + visit_merge_request(merge_request) # Needed to refresh the page + expect(page).to have_content /Enabled an automatic merge when the build for [0-9a-f]{8} succeeds/i 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) } + let(:merge_request) do + create(:merge_request_with_diffs, source_project: project, author: user, + merge_user: user, title: "MepMep", merge_when_build_succeeds: true) + end before do merge_request.source_project.team << [user, :master] @@ -60,10 +66,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do click_link "Cancel Automatic Merge" expect(page).to have_button "Merge When Build Succeeds" + + visit_merge_request(merge_request) # Needed to refresh the page + expect(page).to have_content "Cancelled the automatic merge" end it "allows the user to remove the source branch" do expect(page).to have_link "Remove Source Branch When Merged" + + click_link "Remove Source Branch When Merged" + expect(page).to have_content "The source branch will be removed" end end @@ -78,3 +90,4 @@ feature 'Merge When Build Succeeds', feature: true, js: true do visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) end end +# rubocop:enable Lint/UselessAssignment diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c7a9765825e..63e1cd1fb92 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -48,6 +48,24 @@ describe MergeRequest do describe 'validation' do it { is_expected.to validate_presence_of(:target_branch) } it { is_expected.to validate_presence_of(:source_branch) } + + context "Validation of merge user with Merge When Build succeeds" do + it "allows user to be nil when the feature is disabled" do + expect(subject).to be_valid + end + + it "is invalid without merge user" do + subject.merge_when_build_succeeds = true + expect(subject).not_to be_valid + end + + it "is valid with merge user" do + subject.merge_when_build_succeeds = true + subject.merge_user = build(:user) + + expect(subject).to be_valid + end + end end describe 'respond to' do @@ -175,31 +193,41 @@ describe MergeRequest do end describe '#can_remove_source_branch' do - let(:user) { build(:user)} + let(:user) { create(:user) } + let(:user2) { create(: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 = []; + subject.source_branch = "feature" + subject.target_branch = "master" + subject.save! + end + it "can't be removed when its a protected branch" do + allow(subject.source_project).to receive(:protected_branch?).and_return(true) expect(subject.can_remove_source_branch?(user)).to be_falsey end it "cant remove a root ref" do - subject.source_branch = "master"; + subject.source_branch = "master" + subject.target_branch = "feature" 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)) + it "is unable to remove the source branch for a project the user cannot push to" do + expect(subject.can_remove_source_branch?(user2)).to be_falsey + end + + it "is can be removed in all other cases" do + expect(subject.can_remove_source_branch?(user)).to be_truthy end end describe "#reset_merge_when_build_succeeds" do - let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true } + let(:merge_if_green) { create :merge_request, merge_when_build_succeeds: true, merge_user: create(:user) } + it "sets the item to false" do merge_if_green.reset_merge_when_build_succeeds merge_if_green.reload diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 91ae2059e95..6b7a066ac40 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -303,7 +303,7 @@ 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) } + 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) 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 index 8638539173b..a62d88cde86 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -3,37 +3,38 @@ 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(:mr_merge_if_green_enabled) do + create(:merge_request, merge_when_build_succeeds: true, merge_user: user, + source_branch: "source_branch", target_branch: project.default_branch, + source_project: project, target_project: project, state: "opened") + end + let(:project) { create(:project) } + let(:ci_commit) { create(:ci_commit_with_one_job, ref: mr_merge_if_green_enabled.source_branch, gl_project: 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) + service.execute(merge_request) 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 + end + it 'creates a system note' do note = merge_request.notes.last - expect(note.note).to include 'Enabled an automatic merge when the build for' + expect(note.note).to match /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-z]{8}/ end end - context 'allready approved' do + context 'already 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) } @@ -74,5 +75,10 @@ describe MergeRequests::MergeWhenBuildSucceedsService do expect(mr_merge_if_green_enabled.merge_params).to eq({}) expect(mr_merge_if_green_enabled.merge_user).to be nil end + + it 'Posts a system note' do + note = mr_merge_if_green_enabled.notes.last + expect(note.note).to include 'Cancelled the automatic merge' + end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 18b2659c1f6..9a8174f95fd 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -18,7 +18,8 @@ describe MergeRequests::RefreshService do source_branch: 'master', target_branch: 'feature', target_project: @project, - merge_when_build_succeeds: true) + merge_when_build_succeeds: true, + merge_user: @user) @fork_merge_request = create(:merge_request, source_project: @fork_project, diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 5d41a5cdc69..333035f2d2c 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -208,20 +208,20 @@ describe SystemNoteService do end describe '.merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit_without_jobs } + let(:ci_commit) { build :ci_commit_without_jobs } let(:noteable) { create :merge_request } - subject { described_class.merge_when_build_succeeds(noteable, project, author, ci_commit) } + subject { described_class.merge_when_build_succeeds(noteable, project, author, noteable.last_commit) } it_behaves_like 'a system note' it "posts the Merge When Build Succeeds system note" do - expect(subject.note).to eq "Enabled an automatic merge when the build for 97de212e80737a608d939f648d959671fb0a0142 succeeds" + expect(subject.note).to match /Enabled an automatic merge when the build for (\w+\/\w+@)?[0-9a-f]{40} succeeds/ end end describe '.cancel_merge_when_build_succeeds' do - let(:ci_commit) { create :ci_commit_without_jobs } + let(:ci_commit) { build :ci_commit_without_jobs } let(:noteable) { create :merge_request } subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } @@ -229,7 +229,7 @@ describe SystemNoteService do 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" + expect(subject.note).to eq "Cancelled the automatic merge" end end |