From 6dff7c1771e0cfeb6906244649b3683090bc2929 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Tue, 7 Jun 2016 13:01:34 +0200
Subject: [PATCH] Improve initial implementation of the
'only_allow_merge_if_build_succeeds.rb' feature
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Based on the feedback from reviewers.
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
app/models/merge_request.rb | 19 +++-
.../merge_requests/widget/_open.html.haml | 2 +-
lib/api/merge_requests.rb | 5 +-
.../only_allow_merge_if_build_succeeds.rb | 92 +++++++++----------
spec/models/merge_request_spec.rb | 63 +++++++++----
spec/requests/api/merge_requests_spec.rb | 5 +-
6 files changed, 113 insertions(+), 73 deletions(-)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 43c6bcb8715..949cafc065f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -260,13 +260,20 @@ class MergeRequest < ActiveRecord::Base
end
def mergeable?
- return false if !open? || work_in_progress? || broken? || cannot_be_merged_because_build_failed?
-
- check_if_can_be_merged
+ mergeable_state? && check_if_can_be_merged
can_be_merged?
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
if work_in_progress?
"work_in_progress"
@@ -481,8 +488,10 @@ class MergeRequest < ActiveRecord::Base
::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch)
end
- def cannot_be_merged_because_build_failed?
- project.only_allow_merge_if_build_succeeds? && ci_commit && ci_commit.failed?
+ def cannot_be_merged_because_build_is_not_success?
+ return false unless project.only_allow_merge_if_build_succeeds?
+
+ ci_commit && !ci_commit.success?
end
def state_human_name
diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml
index 8de587009e1..9ea4df4357f 100644
--- a/app/views/projects/merge_requests/widget/_open.html.haml
+++ b/app/views/projects/merge_requests/widget/_open.html.haml
@@ -17,7 +17,7 @@
= render 'projects/merge_requests/widget/open/merge_when_build_succeeds'
- elsif !@merge_request.can_be_merged_by?(current_user)
= 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'
- elsif @merge_request.can_be_merged?
= render 'projects/merge_requests/widget/open/accept'
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 5822e19cd44..24df3e397e0 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -228,11 +228,10 @@ 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! 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]
render_api_error!("SHA does not match HEAD of source branch: #{merge_request.source_sha}", 409)
diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
index 1627aa7287a..52612c91824 100644
--- a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
+++ b/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
@@ -1,99 +1,99 @@
require 'spec_helper'
-feature 'Only allow merge requests to be merged if the build succeeds', feature: true, js: true do
- let(:user) { create(:user) }
-
+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, source_project: project, author: user) }
+ let(:merge_request) { create(:merge_request_with_diffs, source_project: project) }
before do
- login_as user
+ login_as merge_request.author
- project.team << [user, :master]
+ project.team << [merge_request.author, :master]
end
- context "project hasn't ci enabled" do
- it "allows MR to be merged" do
+ context 'project does not have CI enabled' do
+ it 'allows MR to be merged' do
visit_merge_request(merge_request)
- expect(page).to have_button "Accept Merge Request"
+
+ expect(page).to have_button 'Accept Merge Request'
end
end
- 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_build) { create(:ci_build, commit: ci_commit) }
+ context 'when project has CI enabled' do
+ let(:ci_commit) { create(:ci_empty_pipeline, project: project, sha: merge_request.last_commit.id, ref: merge_request.source_branch) }
- 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
project.update_attribute(:only_allow_merge_if_build_succeeds, true)
end
- context "when ci is running" do
- it "doesn't allow to merge immediately" do
- ci_commit.statuses.update_all(status: :pending)
+ context 'when CI is running' do
+ before { ci_commit.update_column(:status, :running) }
+
+ it 'does not allow to merge immediately' do
visit_merge_request(merge_request)
- expect(page).to have_button "Merge When Build Succeeds"
- expect(page).to_not have_button "Select Merge Moment"
+ expect(page).to have_button 'Merge When Build Succeeds'
+ expect(page).not_to have_button 'Select Merge Moment'
end
end
- context "when ci failed" do
- it "doesn't allow MR to be merged" do
- ci_commit.statuses.update_all(status: :failed)
+ context 'when CI failed' do
+ before { ci_commit.update_column(:status, :failed) }
+
+ it 'does not allow MR to be merged' do
visit_merge_request(merge_request)
- expect(page).to_not have_button "Accept Merge Request"
- expect(page).to have_content("Please retry the build or push code to fix the failure.")
+ 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 succeed" do
- it "allows MR to be merged" do
- ci_commit.statuses.update_all(status: :success)
+ context 'when CI succeeded' do
+ before { ci_commit.update_column(:status, :success) }
+
+ it 'allows MR to be merged' do
visit_merge_request(merge_request)
- expect(page).to have_button "Accept Merge Request"
+ expect(page).to have_button 'Accept Merge Request'
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
project.update_attribute(:only_allow_merge_if_build_succeeds, false)
end
- context "when ci is running" do
- it "allows MR to be merged immediately" do
- ci_commit.statuses.update_all(status: :pending)
+ context 'when CI is running' do
+ before { ci_commit.update_column(:status, :running) }
+
+ it 'allows MR to be merged immediately', js: true do
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"
- expect(page).to have_content "Merge Immediately"
+ click_button 'Select Merge Moment'
+ expect(page).to have_content 'Merge Immediately'
end
end
- context "when ci failed" do
- it "allows MR to be merged" do
- ci_commit.statuses.update_all(status: :failed)
+ context 'when CI failed' do
+ before { ci_commit.update_column(:status, :failed) }
+
+ it 'allows MR to be merged' do
visit_merge_request(merge_request)
- expect(page).to have_button "Accept Merge Request"
+ expect(page).to have_button 'Accept Merge Request'
end
end
- context "when ci succeed" do
- it "allows MR to be merged" do
- ci_commit.statuses.update_all(status: :success)
+ context 'when CI succeeded' do
+ before { ci_commit.update_column(:status, :success) }
+
+ it 'allows MR to be merged' do
visit_merge_request(merge_request)
- expect(page).to have_button "Accept Merge Request"
+ expect(page).to have_button 'Accept Merge Request'
end
end
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 76912eed834..f8f1bbf3036 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -491,12 +491,39 @@ describe MergeRequest, models: true do
end
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) }
- it "checks if merge request can be merged" do
- allow(subject).to receive(:cannot_be_merged_because_build_failed?) { false }
+ it 'checks if merge request can be merged' do
+ allow(subject).to receive(:cannot_be_merged_because_build_is_not_success?) { false }
expect(subject).to receive(:check_if_can_be_merged)
subject.mergeable?
@@ -506,7 +533,7 @@ describe MergeRequest, models: true do
before { subject.close }
it 'returns false' do
- expect(subject.mergeable?).to be_falsey
+ expect(subject.mergeable_state?).to be_falsey
end
end
@@ -514,7 +541,7 @@ describe MergeRequest, models: true do
before { subject.title = 'WIP MR' }
it 'returns false' do
- expect(subject.mergeable?).to be_falsey
+ expect(subject.mergeable_state?).to be_falsey
end
end
@@ -522,23 +549,27 @@ describe MergeRequest, models: true do
before { allow(subject).to receive(:broken?) { true } }
it 'returns false' do
- expect(subject.mergeable?).to be_falsey
+ expect(subject.mergeable_state?).to be_falsey
end
end
context 'when failed' do
before { allow(subject).to receive(:broken?) { false } }
- context "when project settings restrict to merge only if build succeeds" do
- before { allow(subject).to receive(:cannot_be_merged_because_build_failed?) { true } }
- it 'returns false if project settings restrict to merge only if build succeeds' do
- expect(subject.mergeable?).to be_falsey
+ context 'when project settings restrict to merge only if build succeeds and build failed' do
+ before do
+ project.only_allow_merge_if_build_succeeds = true
+ 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
- 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(:commit_status) { create(:commit_status, status: 'failed', project: project) }
let(:ci_commit) { create(:ci_empty_pipeline) }
@@ -550,8 +581,8 @@ describe MergeRequest, models: true do
allow(subject).to receive(:ci_commit) { ci_commit }
end
- it "returns true if it's only allowed to merge green build and build has been failed" do
- expect(subject.cannot_be_merged_because_build_failed?).to be_truthy
+ 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_is_not_success?).to be_truthy
end
context 'when no ci_commit is associated' do
@@ -560,15 +591,15 @@ describe MergeRequest, models: true do
end
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
- 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)) }
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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 91c25a0948f..a52148e8b83 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -419,10 +419,11 @@ describe API::API, api: true do
expect(json_response['message']).to eq('405 Method Not Allowed')
end
- it "should return 405 if merge_request build is failed it's restrict to merge only when susccess" do
- allow_any_instance_of(MergeRequest).to receive(:cannot_be_merged_because_build_failed?).and_return(true)
+ 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_is_not_success?).and_return(true)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
+
expect(response.status).to eq(405)
expect(json_response['message']).to eq('405 Method Not Allowed')
end
--
GitLab