Commit 6dff7c17 authored by Rémy Coutable's avatar Rémy Coutable

Improve initial implementation of the 'only_allow_merge_if_build_succeeds.rb' feature

Based on the feedback from reviewers.
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 07dbd6b3
...@@ -260,13 +260,20 @@ class MergeRequest < ActiveRecord::Base ...@@ -260,13 +260,20 @@ class MergeRequest < ActiveRecord::Base
end end
def mergeable? def mergeable?
return false if !open? || work_in_progress? || broken? || cannot_be_merged_because_build_failed? mergeable_state? && check_if_can_be_merged
check_if_can_be_merged
can_be_merged? can_be_merged?
end end
def mergeable_state?
return false unless open?
return false if work_in_progress?
return false if broken?
return false if cannot_be_merged_because_build_is_not_success?
true
end
def gitlab_merge_status def gitlab_merge_status
if work_in_progress? if work_in_progress?
"work_in_progress" "work_in_progress"
...@@ -481,8 +488,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -481,8 +488,10 @@ class MergeRequest < ActiveRecord::Base
::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch) ::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch)
end end
def cannot_be_merged_because_build_failed? def cannot_be_merged_because_build_is_not_success?
project.only_allow_merge_if_build_succeeds? && ci_commit && ci_commit.failed? return false unless project.only_allow_merge_if_build_succeeds?
ci_commit && !ci_commit.success?
end end
def state_human_name def state_human_name
......
...@@ -17,7 +17,7 @@ ...@@ -17,7 +17,7 @@
= render 'projects/merge_requests/widget/open/merge_when_build_succeeds' = render 'projects/merge_requests/widget/open/merge_when_build_succeeds'
- elsif !@merge_request.can_be_merged_by?(current_user) - elsif !@merge_request.can_be_merged_by?(current_user)
= render 'projects/merge_requests/widget/open/not_allowed' = render 'projects/merge_requests/widget/open/not_allowed'
- elsif @merge_request.cannot_be_merged_because_build_failed? - elsif @merge_request.cannot_be_merged_because_build_is_not_success? && @ci_commit && @ci_commit.failed?
= render 'projects/merge_requests/widget/open/build_failed' = render 'projects/merge_requests/widget/open/build_failed'
- elsif @merge_request.can_be_merged? - elsif @merge_request.can_be_merged?
= render 'projects/merge_requests/widget/open/accept' = render 'projects/merge_requests/widget/open/accept'
......
...@@ -228,11 +228,10 @@ module API ...@@ -228,11 +228,10 @@ module API
# Merge request can not be merged # Merge request can not be merged
# because user dont have permissions to push into target branch # because user dont have permissions to push into target branch
unauthorized! unless merge_request.can_be_merged_by?(current_user) unauthorized! unless merge_request.can_be_merged_by?(current_user)
not_allowed! if !merge_request.open? || merge_request.work_in_progress? || merge_request.cannot_be_merged_because_build_failed?
merge_request.check_if_can_be_merged not_allowed! unless merge_request.mergeable_state?
render_api_error!('Branch cannot be merged', 406) unless merge_request.can_be_merged? render_api_error!('Branch cannot be merged', 406) unless merge_request.mergeable?
if params[:sha] && merge_request.source_sha != params[:sha] if params[:sha] && merge_request.source_sha != params[:sha]
render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409) render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)
......
require 'spec_helper' require 'spec_helper'
feature 'Only allow merge requests to be merged if the build succeeds', feature: true, js: true do feature 'Only allow merge requests to be merged if the build succeeds', feature: true do
let(:user) { create(:user) }
let(:project) { create(:project, :public) } let(:project) { create(:project, :public) }
let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user) } let(:merge_request) { create(:merge_request_with_diffs, source_project: project) }
before do before do
login_as user login_as merge_request.author
project.team << [user, :master] project.team << [merge_request.author, :master]
end end
context "project hasn't ci enabled" do context 'project does not have CI enabled' do
it "allows MR to be merged" do it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Accept Merge Request"
expect(page).to have_button 'Accept Merge Request'
end end
end end
context "when project has ci enabled" do context 'when project has CI enabled' do
let!(:ci_commit) { create(:ci_commit, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) } let(:ci_commit) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
let!(:ci_build) { create(:ci_build, commit: ci_commit) }
before do
project.enable_ci
end
context "when merge requests can only be merged if the build succeeds" do context 'when merge requests can only be merged if the build succeeds' do
before do before do
project.update_attribute(:only_allow_merge_if_build_succeeds, true) project.update_attribute(:only_allow_merge_if_build_succeeds, true)
end end
context "when ci is running" do context 'when CI is running' do
it "doesn't allow to merge immediately" do before { ci_commit.update_column(:status, :running) }
ci_commit.statuses.update_all(status: :pending)
it 'does not allow to merge immediately' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Merge When Build Succeeds" expect(page).to have_button 'Merge When Build Succeeds'
expect(page).to_not have_button "Select Merge Moment" expect(page).not_to have_button 'Select Merge Moment'
end end
end end
context "when ci failed" do context 'when CI failed' do
it "doesn't allow MR to be merged" do before { ci_commit.update_column(:status, :failed) }
ci_commit.statuses.update_all(status: :failed)
it 'does not allow MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to_not have_button "Accept Merge Request" expect(page).not_to have_button 'Accept Merge Request'
expect(page).to have_content("Please retry the build or push code to fix the failure.") expect(page).to have_content('Please retry the build or push a new commit to fix the failure.')
end end
end end
context "when ci succeed" do context 'when CI succeeded' do
it "allows MR to be merged" do before { ci_commit.update_column(:status, :success) }
ci_commit.statuses.update_all(status: :success)
it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Accept Merge Request" expect(page).to have_button 'Accept Merge Request'
end end
end end
end end
context "when merge requests can be merged when the build failed" do context 'when merge requests can be merged when the build failed' do
before do before do
project.update_attribute(:only_allow_merge_if_build_succeeds, false) project.update_attribute(:only_allow_merge_if_build_succeeds, false)
end end
context "when ci is running" do context 'when CI is running' do
it "allows MR to be merged immediately" do before { ci_commit.update_column(:status, :running) }
ci_commit.statuses.update_all(status: :pending)
it 'allows MR to be merged immediately', js: true do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Merge When Build Succeeds" expect(page).to have_button 'Merge When Build Succeeds'
click_button "Select Merge Moment" click_button 'Select Merge Moment'
expect(page).to have_content "Merge Immediately" expect(page).to have_content 'Merge Immediately'
end end
end end
context "when ci failed" do context 'when CI failed' do
it "allows MR to be merged" do before { ci_commit.update_column(:status, :failed) }
ci_commit.statuses.update_all(status: :failed)
it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Accept Merge Request" expect(page).to have_button 'Accept Merge Request'
end end
end end
context "when ci succeed" do context 'when CI succeeded' do
it "allows MR to be merged" do before { ci_commit.update_column(:status, :success) }
ci_commit.statuses.update_all(status: :success)
it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
expect(page).to have_button "Accept Merge Request" expect(page).to have_button 'Accept Merge Request'
end end
end end
end end
......
...@@ -491,12 +491,39 @@ describe MergeRequest, models: true do ...@@ -491,12 +491,39 @@ describe MergeRequest, models: true do
end end
describe '#mergeable?' do describe '#mergeable?' do
let(:project) { create(:project, only_allow_merge_if_build_succeeds: true) } let(:project) { create(:project) }
subject { create(:merge_request, source_project: project) }
it 'calls mergeable_state?' do
expect(subject).to receive(:mergeable_state?)
expect(subject.mergeable?).to be_truthy
end
it 'calls check_if_can_be_merged' do
allow(subject).to receive(:mergeable_state?) { true }
expect(subject).to receive(:check_if_can_be_merged)
expect(subject.mergeable?).to be_truthy
end
it 'calls can_be_merged?' do
allow(subject).to receive(:mergeable_state?) { true }
allow(subject).to receive(:can_be_merged?) { true }
expect(subject).to receive(:check_if_can_be_merged)
expect(subject.mergeable?).to be_truthy
end
end
describe '#mergeable_state?' do
let(:project) { create(:project) }
subject { create(:merge_request, source_project: project) } subject { create(:merge_request, source_project: project) }
it "checks if merge request can be merged" do it 'checks if merge request can be merged' do
allow(subject).to receive(:cannot_be_merged_because_build_failed?) { false } allow(subject).to receive(:cannot_be_merged_because_build_is_not_success?) { false }
expect(subject).to receive(:check_if_can_be_merged) expect(subject).to receive(:check_if_can_be_merged)
subject.mergeable? subject.mergeable?
...@@ -506,7 +533,7 @@ describe MergeRequest, models: true do ...@@ -506,7 +533,7 @@ describe MergeRequest, models: true do
before { subject.close } before { subject.close }
it 'returns false' do it 'returns false' do
expect(subject.mergeable?).to be_falsey expect(subject.mergeable_state?).to be_falsey
end end
end end
...@@ -514,7 +541,7 @@ describe MergeRequest, models: true do ...@@ -514,7 +541,7 @@ describe MergeRequest, models: true do
before { subject.title = 'WIP MR' } before { subject.title = 'WIP MR' }
it 'returns false' do it 'returns false' do
expect(subject.mergeable?).to be_falsey expect(subject.mergeable_state?).to be_falsey
end end
end end
...@@ -522,23 +549,27 @@ describe MergeRequest, models: true do ...@@ -522,23 +549,27 @@ describe MergeRequest, models: true do
before { allow(subject).to receive(:broken?) { true } } before { allow(subject).to receive(:broken?) { true } }
it 'returns false' do it 'returns false' do
expect(subject.mergeable?).to be_falsey expect(subject.mergeable_state?).to be_falsey
end end
end end
context 'when failed' do context 'when failed' do
before { allow(subject).to receive(:broken?) { false } } before { allow(subject).to receive(:broken?) { false } }
context "when project settings restrict to merge only if build succeeds" do context 'when project settings restrict to merge only if build succeeds and build failed' do
before { allow(subject).to receive(:cannot_be_merged_because_build_failed?) { true } } before do
it 'returns false if project settings restrict to merge only if build succeeds' do project.only_allow_merge_if_build_succeeds = true
expect(subject.mergeable?).to be_falsey allow(subject).to receive(:cannot_be_merged_because_build_is_not_success?) { true }
end
it 'returns false' do
expect(subject.mergeable_state?).to be_falsey
end end
end end
end end
end end
describe '#cannot_be_merged_because_build_failed?' do describe '#cannot_be_merged_because_build_is_not_success?' do
let(:project) { create(:empty_project, only_allow_merge_if_build_succeeds: true) } let(:project) { create(:empty_project, only_allow_merge_if_build_succeeds: true) }
let(:commit_status) { create(:commit_status, status: 'failed', project: project) } let(:commit_status) { create(:commit_status, status: 'failed', project: project) }
let(:ci_commit) { create(:ci_empty_pipeline) } let(:ci_commit) { create(:ci_empty_pipeline) }
...@@ -550,8 +581,8 @@ describe MergeRequest, models: true do ...@@ -550,8 +581,8 @@ describe MergeRequest, models: true do
allow(subject).to receive(:ci_commit) { ci_commit } allow(subject).to receive(:ci_commit) { ci_commit }
end end
it "returns true if it's only allowed to merge green build and build has been failed" do it 'returns true if it is only allowed to merge green build and build has been failed' do
expect(subject.cannot_be_merged_because_build_failed?).to be_truthy expect(subject.cannot_be_merged_because_build_is_not_success?).to be_truthy
end end
context 'when no ci_commit is associated' do context 'when no ci_commit is associated' do
...@@ -560,15 +591,15 @@ describe MergeRequest, models: true do ...@@ -560,15 +591,15 @@ describe MergeRequest, models: true do
end end
it 'returns false' do it 'returns false' do
expect(subject.cannot_be_merged_because_build_failed?).to be_falsey expect(subject.cannot_be_merged_because_build_is_not_success?).to be_falsey
end end
end end
context "when isn't only allowed to merge green build at project settings" do context 'when is not only allowed to merge green build at project settings' do
subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) } subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) }
it 'returns false' do it 'returns false' do
expect(subject.cannot_be_merged_because_build_failed?).to be_falsey expect(subject.cannot_be_merged_because_build_is_not_success?).to be_falsey
end end
end end
end end
......
...@@ -419,10 +419,11 @@ describe API::API, api: true do ...@@ -419,10 +419,11 @@ describe API::API, api: true do
expect(json_response['message']).to eq('405 Method Not Allowed') expect(json_response['message']).to eq('405 Method Not Allowed')
end end
it "should return 405 if merge_request build is failed it's restrict to merge only when susccess" do it 'returns 405 if the build failed for a merge request that requires success' do
allow_any_instance_of(MergeRequest).to receive(:cannot_be_merged_because_build_failed?).and_return(true) allow_any_instance_of(MergeRequest).to receive(:cannot_be_merged_because_build_is_not_success?).and_return(true)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user) put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
expect(response.status).to eq(405) expect(response.status).to eq(405)
expect(json_response['message']).to eq('405 Method Not Allowed') expect(json_response['message']).to eq('405 Method Not Allowed')
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