Merge branch...

Merge branch '24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running-ee' into 'master'

Fix a wrong "The build for this merge request failed" message

EE port of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7579

See merge request !895

app/models/merge_request.rb

@@ -709,7 +709,7 @@ class MergeRequest < ActiveRecord::Base
   def mergeable_ci_state?
     return true unless project.only_allow_merge_if_build_succeeds?
 
-    !pipeline || pipeline.success?
+    !pipeline || pipeline.success? || pipeline.skipped?
   end
 
   def environments

app/views/projects/merge_requests/widget/_open.html.haml

@@ -29,7 +29,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.mergeable_ci_state?
+    - elsif !@merge_request.mergeable_ci_state? && (@pipeline.failed? || @pipeline.canceled?)
       = render 'projects/merge_requests/widget/open/build_failed'
     - elsif @merge_request.should_be_rebased?
       = render 'projects/merge_requests/widget/open/rebase'

changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml

+---
+title: Fix a wrong "The build for this merge request failed" message
+merge_request: 7579
+author: 

spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb → spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb

 require 'spec_helper'
 
 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) }
+  let(:merge_request) { create(:merge_request_with_diffs) }
+  let(:project)       { merge_request.target_project }
 
   before do
     login_as merge_request.author
@@ -19,7 +19,13 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
   end
 
   context 'when project has CI enabled' do
-    let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) }
+    given!(:pipeline) do
+      create(:ci_empty_pipeline,
+      project: project,
+      sha: merge_request.diff_head_sha,
+      ref: merge_request.source_branch,
+      status: status)
+    end
 
     context 'when merge requests can only be merged if the build succeeds' do
       before do
@@ -27,7 +33,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI is running' do
-        before { pipeline.update_column(:status, :running) }
+        given(:status) { :running }
 
         it 'does not allow to merge immediately' do
           visit_merge_request(merge_request)
@@ -38,7 +44,18 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI failed' do
-        before { pipeline.update_column(:status, :failed) }
+        given(:status) { :failed }
+
+        it 'does not allow MR to be merged' do
+          visit_merge_request(merge_request)
+
+          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 canceled' do
+        given(:status) { :canceled }
 
         it 'does not allow MR to be merged' do
           visit_merge_request(merge_request)
@@ -49,7 +66,17 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI succeeded' do
-        before { pipeline.update_column(:status, :success) }
+        given(:status) { :success }
+
+        it 'allows MR to be merged' do
+          visit_merge_request(merge_request)
+
+          expect(page).to have_button 'Accept Merge Request'
+        end
+      end
+
+      context 'when CI skipped' do
+        given(:status) { :skipped }
 
         it 'allows MR to be merged' do
           visit_merge_request(merge_request)
@@ -65,7 +92,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI is running' do
-        before { pipeline.update_column(:status, :running) }
+        given(:status) { :running }
 
         it 'allows MR to be merged immediately', js: true do
           visit_merge_request(merge_request)
@@ -78,7 +105,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI failed' do
-        before { pipeline.update_column(:status, :failed) }
+        given(:status) { :failed }
 
         it 'allows MR to be merged' do
           visit_merge_request(merge_request)
@@ -88,7 +115,7 @@ feature 'Only allow merge requests to be merged if the build succeeds', feature:
       end
 
       context 'when CI succeeded' do
-        before { pipeline.update_column(:status, :success) }
+        given(:status) { :success }
 
         it 'allows MR to be merged' do
           visit_merge_request(merge_request)

spec/models/merge_request_spec.rb

@@ -1070,13 +1070,31 @@ describe MergeRequest, models: true do
     context 'when it is only allowed to merge when build is green' do
       context 'and a failed pipeline is associated' do
         before do
-          pipeline.statuses << create(:commit_status, status: 'failed', project: project)
+          pipeline.update(status: 'failed')
           allow(subject).to receive(:pipeline) { pipeline }
         end
 
         it { expect(subject.mergeable_ci_state?).to be_falsey }
       end
 
+      context 'and a successful pipeline is associated' do
+        before do
+          pipeline.update(status: 'success')
+          allow(subject).to receive(:pipeline) { pipeline }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_truthy }
+      end
+
+      context 'and a skipped pipeline is associated' do
+        before do
+          pipeline.update(status: 'skipped')
+          allow(subject).to receive(:pipeline) { pipeline }
+        end
+
+        it { expect(subject.mergeable_ci_state?).to be_truthy }
+      end
+
       context 'when no pipeline is associated' do
         before do
           allow(subject).to receive(:pipeline) { nil }