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

Also allow merge request to be merged with skipped pipeline and the
"only allow merge when pipeline is green" feature enabled

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

app/models/merge_request.rb

@@ -686,7 +686,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

@@ -23,7 +23,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.mergeable_discussions_state?
       = render 'projects/merge_requests/widget/open/unresolved_discussions'

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

@@ -0,0 +1,4 @@
+---
+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_spec.rb

@@ -1,8 +1,8 @@
 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

@@ -856,13 +856,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 }