Commit 5324c936 authored by Rémy Coutable's avatar Rémy Coutable

Rename MergeRequest#cannot_be_merged_because_build_is_not_success? to #mergeable_ci_state?

The logic of the method was obviously inverted.
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 282674c1
...@@ -260,7 +260,9 @@ class MergeRequest < ActiveRecord::Base ...@@ -260,7 +260,9 @@ class MergeRequest < ActiveRecord::Base
end end
def mergeable? def mergeable?
mergeable_state? && check_if_can_be_merged return false unless mergeable_state?
check_if_can_be_merged
can_be_merged? can_be_merged?
end end
...@@ -269,7 +271,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -269,7 +271,7 @@ class MergeRequest < ActiveRecord::Base
return false unless open? return false unless open?
return false if work_in_progress? return false if work_in_progress?
return false if broken? return false if broken?
return false if cannot_be_merged_because_build_is_not_success? return false unless mergeable_ci_state?
true true
end end
...@@ -488,10 +490,10 @@ class MergeRequest < ActiveRecord::Base ...@@ -488,10 +490,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_is_not_success? def mergeable_ci_state?
return false unless project.only_allow_merge_if_build_succeeds? return true unless project.only_allow_merge_if_build_succeeds?
ci_commit && !ci_commit.success? !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_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' = 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'
......
...@@ -747,39 +747,39 @@ ActiveRecord::Schema.define(version: 20160608155312) do ...@@ -747,39 +747,39 @@ ActiveRecord::Schema.define(version: 20160608155312) do
t.datetime "created_at" t.datetime "created_at"
t.datetime "updated_at" t.datetime "updated_at"
t.integer "creator_id" t.integer "creator_id"
t.boolean "issues_enabled", default: true, null: false t.boolean "issues_enabled", default: true, null: false
t.boolean "merge_requests_enabled", default: true, null: false t.boolean "merge_requests_enabled", default: true, null: false
t.boolean "wiki_enabled", default: true, null: false t.boolean "wiki_enabled", default: true, null: false
t.integer "namespace_id" 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.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.datetime "last_activity_at"
t.string "import_url" t.string "import_url"
t.integer "visibility_level", default: 0, null: false t.integer "visibility_level", default: 0, null: false
t.boolean "archived", default: false, null: false t.boolean "archived", default: false, null: false
t.string "avatar" t.string "avatar"
t.string "import_status" t.string "import_status"
t.float "repository_size", default: 0.0 t.float "repository_size", default: 0.0
t.integer "star_count", default: 0, null: false t.integer "star_count", default: 0, null: false
t.string "import_type" t.string "import_type"
t.string "import_source" t.string "import_source"
t.integer "commit_count", default: 0 t.integer "commit_count", default: 0
t.text "import_error" t.text "import_error"
t.integer "ci_id" t.integer "ci_id"
t.boolean "builds_enabled", default: true, null: false t.boolean "builds_enabled", default: true, null: false
t.boolean "shared_runners_enabled", default: true, null: false t.boolean "shared_runners_enabled", default: true, null: false
t.string "runners_token" t.string "runners_token"
t.string "build_coverage_regex" t.string "build_coverage_regex"
t.boolean "build_allow_git_fetch", default: true, null: false t.boolean "build_allow_git_fetch", default: true, null: false
t.integer "build_timeout", default: 3600, null: false t.integer "build_timeout", default: 3600, null: false
t.boolean "pending_delete", default: false t.boolean "pending_delete", default: false
t.boolean "public_builds", default: true, null: false t.boolean "public_builds", default: true, null: false
t.integer "pushes_since_gc", default: 0 t.integer "pushes_since_gc", default: 0
t.boolean "last_repository_check_failed" t.boolean "last_repository_check_failed"
t.datetime "last_repository_check_at" t.datetime "last_repository_check_at"
t.boolean "container_registry_enabled" 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 end
add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree
......
...@@ -495,23 +495,16 @@ describe MergeRequest, models: true do ...@@ -495,23 +495,16 @@ describe MergeRequest, models: true do
subject { create(:merge_request, source_project: project) } subject { create(:merge_request, source_project: project) }
it 'calls mergeable_state?' do it 'returns false if #mergeable_state? is false' do
expect(subject).to receive(:mergeable_state?) expect(subject).to receive(:mergeable_state?) { false }
expect(subject.mergeable?).to be_truthy expect(subject.mergeable?).to be_falsey
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 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(:mergeable_state?) { true }
allow(subject).to receive(:can_be_merged?) { true }
expect(subject).to receive(:check_if_can_be_merged) expect(subject).to receive(:check_if_can_be_merged)
expect(subject).to receive(:can_be_merged?) { true }
expect(subject.mergeable?).to be_truthy expect(subject.mergeable?).to be_truthy
end end
...@@ -523,7 +516,7 @@ describe MergeRequest, models: true do ...@@ -523,7 +516,7 @@ describe MergeRequest, models: true do
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_is_not_success?) { false } allow(subject).to receive(:mergeable_ci_state?) { true }
expect(subject).to receive(:check_if_can_be_merged) expect(subject).to receive(:check_if_can_be_merged)
subject.mergeable? subject.mergeable?
...@@ -559,7 +552,7 @@ describe MergeRequest, models: true do ...@@ -559,7 +552,7 @@ describe MergeRequest, models: true do
context 'when project settings restrict to merge only if build succeeds and build failed' do context 'when project settings restrict to merge only if build succeeds and build failed' do
before do before do
project.only_allow_merge_if_build_succeeds = true 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 end
it 'returns false' do it 'returns false' do
...@@ -569,37 +562,49 @@ describe MergeRequest, models: true do ...@@ -569,37 +562,49 @@ describe MergeRequest, models: true do
end end
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(: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) } let(:ci_commit) { create(:ci_empty_pipeline) }
subject { build(:merge_request, target_project: project) } subject { build(:merge_request, target_project: project) }
before do context 'when it is only allowed to merge when build is green' do
ci_commit.statuses << commit_status context 'and a failed ci_commit is associated' do
allow(subject).to receive(:ci_commit) { ci_commit } before do
end ci_commit.statuses << create(:commit_status, status: 'failed', project: project)
allow(subject).to receive(:ci_commit) { ci_commit }
it 'returns true if it is only allowed to merge green build and build has been failed' do end
expect(subject.cannot_be_merged_because_build_is_not_success?).to be_truthy
end
context 'when no ci_commit is associated' do it { expect(subject.mergeable_ci_state?).to be_falsey }
before do
allow(subject).to receive(:ci_commit) { nil }
end end
it 'returns false' do context 'when no ci_commit is associated' do
expect(subject.cannot_be_merged_because_build_is_not_success?).to be_falsey before do
allow(subject).to receive(:ci_commit) { nil }
end
it { expect(subject.mergeable_ci_state?).to be_truthy }
end end
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)) } subject { build(:merge_request, target_project: build(:empty_project, only_allow_merge_if_build_succeeds: false)) }
it 'returns false' do context 'and a failed ci_commit is associated' do
expect(subject.cannot_be_merged_because_build_is_not_success?).to be_falsey 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 end
end end
......
...@@ -420,7 +420,7 @@ describe API::API, api: true do ...@@ -420,7 +420,7 @@ describe API::API, api: true do
end end
it 'returns 405 if the build failed for a merge request that requires success' 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_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) put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user)
......
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