Fix a wrong "The build for this merge request failed" message24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running

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>

7 files changed, 61 insertions, 12 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9d3eab52189..6c3c093d084 100644
--- a/app/models/merge_request.rb
+++ b/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
diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml
index 01314eb37d0..ac26aa569ba 100644
--- a/app/views/projects/merge_requests/widget/_open.html.haml
+++ b/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'
diff --git a/changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml b/changelogs/unreleased/24616-mr-shows-the-build-for-this-merge-request-failed-although-builds-still-running.yml
new file mode 100644
index 00000000000..3773459550f
--- /dev/null
+++ b/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: 
diff --git a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions_spec.rb
index 7f11db3c417..7f11db3c417 100644
--- a/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions.rb
+++ b/spec/features/merge_requests/check_if_mergeable_with_unresolved_discussions_spec.rb
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_spec.rb
index 80e8b8fc642..1ec3103feef 100644
--- a/spec/features/merge_requests/only_allow_merge_if_build_succeeds.rb
+++ b/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)
diff --git a/spec/features/merge_requests/toggle_whitespace_changes.rb b/spec/features/merge_requests/toggle_whitespace_changes_spec.rb
index 0f98737b700..0f98737b700 100644
--- a/spec/features/merge_requests/toggle_whitespace_changes.rb
+++ b/spec/features/merge_requests/toggle_whitespace_changes_spec.rb
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index fb032a89d50..0b4277b1edd 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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 }