diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 949cafc065fc47f080d1054a20091a7d91e0c926..f919cfe697e03bec9d17e854e0bd75d81264b02e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -260,7 +260,9 @@ class MergeRequest < ActiveRecord::Base
end
def mergeable?
- mergeable_state? && check_if_can_be_merged
+ return false unless mergeable_state?
+
+ check_if_can_be_merged
can_be_merged?
end
@@ -269,7 +271,7 @@ class MergeRequest < ActiveRecord::Base
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?
+ return false unless mergeable_ci_state?
true
end
@@ -488,10 +490,10 @@ class MergeRequest < ActiveRecord::Base
::Gitlab::GitAccess.new(user, project).can_push_to_branch?(target_branch)
end
- def cannot_be_merged_because_build_is_not_success?
- return false unless project.only_allow_merge_if_build_succeeds?
+ def mergeable_ci_state?
+ return true unless project.only_allow_merge_if_build_succeeds?
- ci_commit && !ci_commit.success?
+ !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 9ea4df4357fb1592ab4c7284266fa310a9d8b8b9..84fbf618ec651832dc41641c5bb245773d6451f1 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_is_not_success? && @ci_commit && @ci_commit.failed?
+ - elsif !@merge_request.mergeable_ci_state? && @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/db/schema.rb b/db/schema.rb
index 03070f0d593c2e2e1a9bc0aabed0a379574a9a92..09ff52ccc8ea3d3714fade660a556a026e9f4364 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -747,39 +747,39 @@ ActiveRecord::Schema.define(version: 20160608155312) do
t.datetime "created_at"
t.datetime "updated_at"
t.integer "creator_id"
- t.boolean "issues_enabled", default: true, null: false
- t.boolean "merge_requests_enabled", default: true, null: false
- t.boolean "wiki_enabled", default: true, null: false
+ t.boolean "issues_enabled", default: true, null: false
+ t.boolean "merge_requests_enabled", default: true, null: false
+ t.boolean "wiki_enabled", default: true, null: false
t.integer "namespace_id"
- t.string "issues_tracker", default: "gitlab", null: false
+ t.string "issues_tracker", default: "gitlab", null: false
t.string "issues_tracker_id"
- t.boolean "snippets_enabled", default: true, null: false
+ t.boolean "snippets_enabled", default: true, null: false
t.datetime "last_activity_at"
t.string "import_url"
- t.integer "visibility_level", default: 0, null: false
- t.boolean "archived", default: false, null: false
+ t.integer "visibility_level", default: 0, null: false
+ t.boolean "archived", default: false, null: false
t.string "avatar"
t.string "import_status"
- t.float "repository_size", default: 0.0
- t.integer "star_count", default: 0, null: false
+ t.float "repository_size", default: 0.0
+ t.integer "star_count", default: 0, null: false
t.string "import_type"
t.string "import_source"
- t.integer "commit_count", default: 0
+ t.integer "commit_count", default: 0
t.text "import_error"
t.integer "ci_id"
- t.boolean "builds_enabled", default: true, null: false
- t.boolean "shared_runners_enabled", default: true, null: false
+ t.boolean "builds_enabled", default: true, null: false
+ t.boolean "shared_runners_enabled", default: true, null: false
t.string "runners_token"
t.string "build_coverage_regex"
- t.boolean "build_allow_git_fetch", default: true, null: false
- t.integer "build_timeout", default: 3600, null: false
- t.boolean "pending_delete", default: false
- t.boolean "public_builds", default: true, null: false
- t.integer "pushes_since_gc", default: 0
+ t.boolean "build_allow_git_fetch", default: true, null: false
+ t.integer "build_timeout", default: 3600, null: false
+ t.boolean "pending_delete", default: false
+ t.boolean "public_builds", default: true, null: false
+ t.integer "pushes_since_gc", default: 0
t.boolean "last_repository_check_failed"
t.datetime "last_repository_check_at"
t.boolean "container_registry_enabled"
- t.boolean "only_allow_merge_if_build_succeeds", default: false
+ t.boolean "only_allow_merge_if_build_succeeds", default: false, null: false
end
add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f8f1bbf3036c760ba4e3d3193119a869ec6de2ba..c543cbcfabd495a5fa8ede6709e42b0ea9505b00 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -495,23 +495,16 @@ describe MergeRequest, models: true do
subject { create(:merge_request, source_project: project) }
- it 'calls mergeable_state?' do
- expect(subject).to receive(:mergeable_state?)
+ it 'returns false if #mergeable_state? is false' do
+ expect(subject).to receive(:mergeable_state?) { false }
- 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
+ expect(subject.mergeable?).to be_falsey
end
- it 'calls can_be_merged?' do
+ it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' 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).to receive(:can_be_merged?) { true }
expect(subject.mergeable?).to be_truthy
end
@@ -523,7 +516,7 @@ describe MergeRequest, models: true do
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_is_not_success?) { false }
+ allow(subject).to receive(:mergeable_ci_state?) { true }
expect(subject).to receive(:check_if_can_be_merged)
subject.mergeable?
@@ -559,7 +552,7 @@ describe MergeRequest, models: true do
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 }
+ allow(subject).to receive(:mergeable_ci_state?) { false }
end
it 'returns false' do
@@ -569,37 +562,49 @@ describe MergeRequest, models: true do
end
end
- describe '#cannot_be_merged_because_build_is_not_success?' do
+ describe '#mergeable_ci_state?' 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) }
subject { build(:merge_request, target_project: project) }
- before do
- ci_commit.statuses << commit_status
- allow(subject).to receive(:ci_commit) { ci_commit }
- end
-
- 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 it is only allowed to merge when build is green' do
+ context 'and a failed ci_commit is associated' do
+ before do
+ ci_commit.statuses << create(:commit_status, status: 'failed', project: project)
+ allow(subject).to receive(:ci_commit) { ci_commit }
+ end
- context 'when no ci_commit is associated' do
- before do
- allow(subject).to receive(:ci_commit) { nil }
+ it { expect(subject.mergeable_ci_state?).to be_falsey }
end
- it 'returns false' do
- expect(subject.cannot_be_merged_because_build_is_not_success?).to be_falsey
+ context 'when no ci_commit is associated' do
+ before do
+ allow(subject).to receive(:ci_commit) { nil }
+ end
+
+ it { expect(subject.mergeable_ci_state?).to be_truthy }
end
end
- context 'when is not only allowed to merge green build at project settings' do
+ context 'when merges are not restricted to green builds' 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_is_not_success?).to be_falsey
+ context 'and a failed ci_commit is associated' do
+ before do
+ ci_commit.statuses << create(:commit_status, status: 'failed', project: project)
+ allow(subject).to receive(:ci_commit) { ci_commit }
+ end
+
+ it { expect(subject.mergeable_ci_state?).to be_truthy }
+ end
+
+ context 'when no ci_commit is associated' do
+ before do
+ allow(subject).to receive(:ci_commit) { nil }
+ end
+
+ it { expect(subject.mergeable_ci_state?).to be_truthy }
end
end
end
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index a52148e8b83cd91fb9db98be882835450c54fb0f..1356f87b0e98bfb47ad57512b5a6ff3a5ab390b8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -420,7 +420,7 @@ describe API::API, api: true do
end
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)
+ allow_any_instance_of(MergeRequest).to receive(:mergeable_ci_state?).and_return(false)
put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)