Commit 2462a96e authored by Zeger-Jan van de Weg's avatar Zeger-Jan van de Weg

Incorporate feedback

parent 25907ebe
...@@ -21,6 +21,9 @@ ...@@ -21,6 +21,9 @@
# locked_at :datetime # locked_at :datetime
# updated_by_id :integer # updated_by_id :integer
# merge_error :string(255) # 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") require Rails.root.join("app/models/commit")
...@@ -124,6 +127,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -124,6 +127,7 @@ class MergeRequest < ActiveRecord::Base
validates :source_branch, presence: true validates :source_branch, presence: true
validates :target_project, presence: true validates :target_project, presence: true
validates :target_branch, presence: true validates :target_branch, presence: true
validates :merge_user, presence: true, if: :merge_when_build_succeeds?
validate :validate_branches validate :validate_branches
validate :validate_fork validate :validate_fork
...@@ -496,8 +500,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -496,8 +500,6 @@ class MergeRequest < ActiveRecord::Base
end end
def ci_commit def ci_commit
if last_commit @ci_commit ||= source_project.ci_commit(last_commit.id) if last_commit
source_project.ci_commit(last_commit.id)
end
end end
end end
module MergeRequests module MergeRequests
class MergeWhenBuildSucceedsService < MergeRequests::BaseService 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 # updates the params for the automatic merge
def execute(merge_request) def execute(merge_request)
merge_request.merge_params.merge!(params) merge_request.merge_params.merge!(params)
...@@ -12,7 +12,7 @@ module MergeRequests ...@@ -12,7 +12,7 @@ module MergeRequests
merge_request.merge_when_build_succeeds = true merge_request.merge_when_build_succeeds = true
merge_request.merge_user = @current_user 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 end
merge_request.save merge_request.save
...@@ -25,8 +25,7 @@ module MergeRequests ...@@ -25,8 +25,7 @@ module MergeRequests
merge_requests.each do |merge_request| merge_requests.each do |merge_request|
next unless merge_request.merge_when_build_succeeds? next unless merge_request.merge_when_build_succeeds?
ci_commit = merge_request.ci_commit if merge_request.ci_commit && merge_request.ci_commit.success? && merge_request.mergeable?
if ci_commit && ci_commit.success? && merge_request.mergeable?
MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params) MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params)
end end
end end
...@@ -34,7 +33,7 @@ module MergeRequests ...@@ -34,7 +33,7 @@ module MergeRequests
# Cancels the automatic merge # Cancels the automatic merge
def cancel(merge_request) 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 merge_request.reset_merge_when_build_succeeds
SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user) SystemNoteService.cancel_merge_when_build_succeeds(merge_request, @project, @current_user)
......
...@@ -131,15 +131,15 @@ class SystemNoteService ...@@ -131,15 +131,15 @@ class SystemNoteService
end end
# Called when 'merge when build succeeds' is executed # Called when 'merge when build succeeds' is executed
def self.merge_when_build_succeeds(noteable, project, author, ci_commit) def self.merge_when_build_succeeds(noteable, project, author, last_commit)
body = "Enabled an automatic merge when the build for #{ci_commit.sha} succeeds" body = "Enabled an automatic merge when the build for #{last_commit.to_reference} succeeds"
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
# Called when 'merge when build succeeds' is canceled # Called when 'merge when build succeeds' is canceled
def self.cancel_merge_when_build_succeeds(noteable, project, author) 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) create_note(noteable: noteable, project: project, author: author, note: body)
end end
......
- ci_commit = merge_request.ci_commit
%li{ class: mr_css_classes(merge_request) } %li{ class: mr_css_classes(merge_request) }
.merge-request-title .merge-request-title
%span.merge-request-title-text %span.merge-request-title-text
...@@ -7,8 +6,8 @@ ...@@ -7,8 +6,8 @@
- merge_request.labels.each do |label| - merge_request.labels.each do |label|
= link_to_label(label, project: merge_request.project) = link_to_label(label, project: merge_request.project)
.pull-right.light .pull-right.light
- if ci_commit - if merge_request.ci_commit
= render_ci_status(ci_commit) = render_ci_status(merge_request.ci_commit)
- if merge_request.merged? - if merge_request.merged?
%span %span
%i.fa.fa-check %i.fa.fa-check
......
- ci_commit = @merge_request.ci_commit - if @merge_request.ci_commit
- if ci_commit - status = @merge_request.ci_commit.status
- status = ci_commit.status
.mr-widget-heading .mr-widget-heading
.ci_widget{class: "ci-#{status}"} .ci_widget{class: "ci-#{status}"}
= ci_status_icon(ci_commit) = ci_status_icon(@merge_request.ci_commit)
%span CI build #{status} %span CI build #{status}
for #{@merge_request.last_commit_short_sha}. for #{@merge_request.last_commit_short_sha}.
%span.ci-coverage %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? - elsif @merge_request.has_ci?
- # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX
......
...@@ -4,8 +4,7 @@ ...@@ -4,8 +4,7 @@
= hidden_field_tag :authenticity_token, form_authenticity_token = hidden_field_tag :authenticity_token, form_authenticity_token
.accept-merge-holder.clearfix.js-toggle-container .accept-merge-holder.clearfix.js-toggle-container
.accept-action .accept-action
- ci_commit = @merge_request.ci_commit - if @merge_request.ci_commit && @merge_request.ci_commit.active?
- if ci_commit && ci_commit.active?
= f.button class: "btn btn-create btn-grouped merge_when_build_succeeds", name: "merge_when_build_succeeds" do = 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 = f.button class: "btn btn-create btn-grouped accept_merge_request #{status_class}" do
......
%h4 %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 to be merged automatically when
#{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds. #{link_to "the build", ci_status_path(@merge_request.ci_commit)} succeeds.
%div %div
- source_branch_removed = @merge_request.merge_params["should_remove_source_branch"].present? - should_remove_source_branch = @merge_request.merge_params["should_remove_source_branch"].present?
- if source_branch_removed %p
= succeed '.' do = succeed '.' do
The changes will be merged into The changes will be merged into
%span.label-branch= @merge_request.target_branch %span.label-branch= @merge_request.target_branch
- if should_remove_source_branch
The source branch will be removed. The source branch will be removed.
- else - else
%p
= succeed '.' do
The changes will be merged into
%span.label-branch= @merge_request.target_branch
The source branch will not be removed. 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 .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 = 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') = icon('times')
Remove Source Branch When Merged 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 = 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 Cancel Automatic Merge
...@@ -65,6 +65,11 @@ FactoryGirl.define do ...@@ -65,6 +65,11 @@ FactoryGirl.define do
target_branch "master" target_branch "master"
end end
trait :merge_when_build_succeeds do
merge_when_build_succeeds true
merge_user author
end
factory :closed_merge_request, traits: [:closed] factory :closed_merge_request, traits: [:closed]
factory :reopened_merge_request, traits: [:reopened] factory :reopened_merge_request, traits: [:reopened]
factory :merge_request_with_diffs, traits: [:with_diffs] factory :merge_request_with_diffs, traits: [:with_diffs]
......
require 'spec_helper' 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 feature 'Merge When Build Succeeds', feature: true, js: true do
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -32,16 +34,20 @@ feature 'Merge When Build Succeeds', feature: true, js: true do ...@@ -32,16 +34,20 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
it 'activates Merge When Build Succeeds feature' do it 'activates Merge When Build Succeeds feature' do
expect(page).to have_link "Cancel Automatic Merge" 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." 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 end
end end
context 'When it is enabled' do context 'When it is enabled' do
# No clue how, but push a new commit to the branch let(:merge_request) do
let(:merge_request) { create(:merge_request_with_diffs, source_project: project, # source_branch: "mepmep", create(:merge_request_with_diffs, source_project: project, author: user,
author: user, title: "Bug NS-04", merge_when_build_succeeds: true) } merge_user: user, title: "MepMep", merge_when_build_succeeds: true)
end
before do before do
merge_request.source_project.team << [user, :master] merge_request.source_project.team << [user, :master]
...@@ -60,10 +66,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do ...@@ -60,10 +66,16 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
click_link "Cancel Automatic Merge" click_link "Cancel Automatic Merge"
expect(page).to have_button "Merge When Build Succeeds" 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 end
it "allows the user to remove the source branch" do it "allows the user to remove the source branch" do
expect(page).to have_link "Remove Source Branch When Merged" 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
end end
...@@ -78,3 +90,4 @@ feature 'Merge When Build Succeeds', feature: true, js: true do ...@@ -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) visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
end end
end end
# rubocop:enable Lint/UselessAssignment
...@@ -48,6 +48,24 @@ describe MergeRequest do ...@@ -48,6 +48,24 @@ describe MergeRequest do
describe 'validation' do describe 'validation' do
it { is_expected.to validate_presence_of(:target_branch) } it { is_expected.to validate_presence_of(:target_branch) }
it { is_expected.to validate_presence_of(:source_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 end
describe 'respond to' do describe 'respond to' do
...@@ -175,31 +193,41 @@ describe MergeRequest do ...@@ -175,31 +193,41 @@ describe MergeRequest do
end end
describe '#can_remove_source_branch' do describe '#can_remove_source_branch' do
let(:user) { build(:user)} let(:user) { create(:user) }
let(:user2) { create(:user) }
before do before do
subject.source_project.team << [user, :master] subject.source_project.team << [user, :master]
end
it "cant be merged when its a a protected branch" do subject.source_branch = "feature"
subject.source_project.protected_branches = []; 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 expect(subject.can_remove_source_branch?(user)).to be_falsey
end end
it "cant remove a root ref" do 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 expect(subject.can_remove_source_branch?(user)).to be_falsey
end end
it "is truthy in all other cases" do it "is unable to remove the source branch for a project the user cannot push to" do
expect(subject.can_remove_source_branch?(user)) 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
end end
describe "#reset_merge_when_build_succeeds" do 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 it "sets the item to false" do
merge_if_green.reset_merge_when_build_succeeds merge_if_green.reset_merge_when_build_succeeds
merge_if_green.reload merge_if_green.reload
......
...@@ -303,7 +303,7 @@ describe API::API, api: true do ...@@ -303,7 +303,7 @@ describe API::API, api: true do
end end
describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do 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 it "should return merge_request in case of success" do
put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user)
......
...@@ -3,37 +3,38 @@ require 'spec_helper' ...@@ -3,37 +3,38 @@ require 'spec_helper'
describe MergeRequests::MergeWhenBuildSucceedsService do describe MergeRequests::MergeWhenBuildSucceedsService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) } let(:merge_request) { create(:merge_request) }
let(:mr_merge_if_green_enabled) { create(:merge_request, merge_when_build_succeeds: true,
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_branch: "source_branch", target_branch: project.default_branch,
source_project: project, target_project: project, state: "opened") } 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) } end
let(:project) { create(:project) } 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') } let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, commit_message: 'Awesome message') }
before do
project.ci_commits = [ci_commit]
project.save!
end
describe "#execute" do describe "#execute" do
context 'first time enabling' do context 'first time enabling' do
before do before do
allow(merge_request).to receive(:ci_commit).and_return(ci_commit) allow(merge_request).to receive(:ci_commit).and_return(ci_commit)
service.execute(merge_request)
end end
it 'sets the params, merge_user, and flag' do it 'sets the params, merge_user, and flag' do
service.execute(merge_request)
expect(merge_request).to be_valid expect(merge_request).to be_valid
expect(merge_request.merge_when_build_succeeds).to be_truthy 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_params).to eq commit_message: 'Awesome message'
expect(merge_request.merge_user).to be user expect(merge_request.merge_user).to be user
end
it 'creates a system note' do
note = merge_request.notes.last 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
end end
context 'allready approved' do context 'already approved' do
let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) } let(:service) { MergeRequests::MergeWhenBuildSucceedsService.new(project, user, new_key: true) }
let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) } let(:build) { create(:ci_build, ref: mr_merge_if_green_enabled.source_branch) }
...@@ -74,5 +75,10 @@ describe MergeRequests::MergeWhenBuildSucceedsService do ...@@ -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_params).to eq({})
expect(mr_merge_if_green_enabled.merge_user).to be nil expect(mr_merge_if_green_enabled.merge_user).to be nil
end 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
end end
...@@ -18,7 +18,8 @@ describe MergeRequests::RefreshService do ...@@ -18,7 +18,8 @@ describe MergeRequests::RefreshService do
source_branch: 'master', source_branch: 'master',
target_branch: 'feature', target_branch: 'feature',
target_project: @project, target_project: @project,
merge_when_build_succeeds: true) merge_when_build_succeeds: true,
merge_user: @user)
@fork_merge_request = create(:merge_request, @fork_merge_request = create(:merge_request,
source_project: @fork_project, source_project: @fork_project,
......
...@@ -208,20 +208,20 @@ describe SystemNoteService do ...@@ -208,20 +208,20 @@ describe SystemNoteService do
end end
describe '.merge_when_build_succeeds' do 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 } 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_behaves_like 'a system note'
it "posts the Merge When Build Succeeds system note" do 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
end end
describe '.cancel_merge_when_build_succeeds' do 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 } let(:noteable) { create :merge_request }
subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) } subject { described_class.cancel_merge_when_build_succeeds(noteable, project, author) }
...@@ -229,7 +229,7 @@ describe SystemNoteService do ...@@ -229,7 +229,7 @@ describe SystemNoteService do
it_behaves_like 'a system note' it_behaves_like 'a system note'
it "posts the Merge When Build Succeeds system note" do 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment