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>

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