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: Rémy Coutable <remy@rymai.me>

5 files changed, 64 insertions, 57 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 949cafc065f..f919cfe697e 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 9ea4df4357f..84fbf618ec6 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 03070f0d593..09ff52ccc8e 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 f8f1bbf3036..c543cbcfabd 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 a52148e8b83..1356f87b0e9 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)