Commit 9a09cedb authored by Rémy Coutable's avatar Rémy Coutable

Fix a wrong "The build for this merge request failed" message

Also allow merge request to be merged with skipped pipeline and the
"only allow merge when pipeline is green" feature enabled
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 2e2d9400
...@@ -709,7 +709,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -709,7 +709,7 @@ class MergeRequest < ActiveRecord::Base
def mergeable_ci_state? def mergeable_ci_state?
return true unless project.only_allow_merge_if_build_succeeds? return true unless project.only_allow_merge_if_build_succeeds?
!pipeline || pipeline.success? !pipeline || pipeline.success? || pipeline.skipped?
end end
def environments def environments
......
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,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.mergeable_ci_state? - elsif !@merge_request.mergeable_ci_state? && (@pipeline.failed? || @pipeline.canceled?)
= render 'projects/merge_requests/widget/open/build_failed' = render 'projects/merge_requests/widget/open/build_failed'
- elsif @merge_request.should_be_rebased? - elsif @merge_request.should_be_rebased?
= render 'projects/merge_requests/widget/open/rebase' = render 'projects/merge_requests/widget/open/rebase'
......
---
title: Fix a wrong "The build for this merge request failed" message
merge_request: 7579
author:
require 'spec_helper' require 'spec_helper'
feature 'Only allow merge requests to be merged if the build succeeds', feature: true do feature 'Only allow merge requests to be merged if the build succeeds', feature: true do
let(:project) { create(:project, :public) } let(:merge_request) { create(:merge_request_with_diffs) }
let(:merge_request) { create(:merge_request_with_diffs, source_project: project) } let(:project) { merge_request.target_project }
before do before do
login_as merge_request.author login_as merge_request.author
...@@ -19,7 +19,13 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -19,7 +19,13 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when project has CI enabled' do context 'when project has CI enabled' do
let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) } given!(:pipeline) do
create(:ci_empty_pipeline,
project: project,
sha: merge_request.diff_head_sha,
ref: merge_request.source_branch,
status: status)
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
...@@ -27,7 +33,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -27,7 +33,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI is running' do context 'when CI is running' do
before { pipeline.update_column(:status, :running) } given(:status) { :running }
it 'does not allow to merge immediately' do it 'does not allow to merge immediately' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
...@@ -38,7 +44,18 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -38,7 +44,18 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI failed' do context 'when CI failed' do
before { pipeline.update_column(:status, :failed) } given(:status) { :failed }
it 'does not allow MR to be merged' do
visit_merge_request(merge_request)
expect(page).not_to have_button 'Accept Merge Request'
expect(page).to have_content('Please retry the build or push a new commit to fix the failure.')
end
end
context 'when CI canceled' do
given(:status) { :canceled }
it 'does not allow MR to be merged' do it 'does not allow MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
...@@ -49,7 +66,17 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -49,7 +66,17 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI succeeded' do context 'when CI succeeded' do
before { pipeline.update_column(:status, :success) } given(:status) { :success }
it 'allows MR to be merged' do
visit_merge_request(merge_request)
expect(page).to have_button 'Accept Merge Request'
end
end
context 'when CI skipped' do
given(:status) { :skipped }
it 'allows MR to be merged' do it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
...@@ -65,7 +92,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -65,7 +92,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI is running' do context 'when CI is running' do
before { pipeline.update_column(:status, :running) } given(:status) { :running }
it 'allows MR to be merged immediately', js: true do it 'allows MR to be merged immediately', js: true do
visit_merge_request(merge_request) visit_merge_request(merge_request)
...@@ -78,7 +105,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -78,7 +105,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI failed' do context 'when CI failed' do
before { pipeline.update_column(:status, :failed) } given(:status) { :failed }
it 'allows MR to be merged' do it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
...@@ -88,7 +115,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature: ...@@ -88,7 +115,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
end end
context 'when CI succeeded' do context 'when CI succeeded' do
before { pipeline.update_column(:status, :success) } given(:status) { :success }
it 'allows MR to be merged' do it 'allows MR to be merged' do
visit_merge_request(merge_request) visit_merge_request(merge_request)
......
...@@ -1070,13 +1070,31 @@ describe MergeRequest, models: true do ...@@ -1070,13 +1070,31 @@ describe MergeRequest, models: true do
context 'when it is only allowed to merge when build is green' do context 'when it is only allowed to merge when build is green' do
context 'and a failed pipeline is associated' do context 'and a failed pipeline is associated' do
before do before do
pipeline.statuses << create(:commit_status, status: 'failed', project: project) pipeline.update(status: 'failed')
allow(subject).to receive(:pipeline) { pipeline } allow(subject).to receive(:pipeline) { pipeline }
end end
it { expect(subject.mergeable_ci_state?).to be_falsey } it { expect(subject.mergeable_ci_state?).to be_falsey }
end end
context 'and a successful pipeline is associated' do
before do
pipeline.update(status: 'success')
allow(subject).to receive(:pipeline) { pipeline }
end
it { expect(subject.mergeable_ci_state?).to be_truthy }
end
context 'and a skipped pipeline is associated' do
before do
pipeline.update(status: 'skipped')
allow(subject).to receive(:pipeline) { pipeline }
end
it { expect(subject.mergeable_ci_state?).to be_truthy }
end
context 'when no pipeline is associated' do context 'when no pipeline is associated' do
before do before do
allow(subject).to receive(:pipeline) { nil } allow(subject).to receive(:pipeline) { nil }
......
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