Improve initial implementation of the 'only_allow_merge_if_build_succeeds.rb' feature

Based on the feedback from reviewers.

Signed-off-by: Rémy Coutable <remy@rymai.me>

2 files changed, 15 insertions, 6 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'