Merge branch '353406-extract-mergeable-discussions-state-check' into 'master'

Extract mergeable_discussions_state? to mergeability checks framework

See merge request gitlab-org/gitlab!81230

app/models/merge_request.rb

@@ -1155,12 +1155,18 @@ class MergeRequest < ApplicationRecord
     return false unless open?
     return false if work_in_progress?
     return false if broken?
-    return false unless skip_discussions_check || mergeable_discussions_state?
 
     if Feature.enabled?(:improved_mergeability_checks, self.project, default_enabled: :yaml)
-      additional_checks = MergeRequests::Mergeability::RunChecksService.new(merge_request: self, params: { skip_ci_check: skip_ci_check })
+      additional_checks = MergeRequests::Mergeability::RunChecksService.new(
+        merge_request: self,
+        params: {
+          skip_ci_check: skip_ci_check,
+          skip_discussions_check: skip_discussions_check
+        }
+      )
       additional_checks.execute.all?(&:success?)
     else
+      return false unless skip_discussions_check || mergeable_discussions_state?
       return false unless skip_ci_check || mergeable_ci_state?
 
       true

app/services/merge_requests/mergeability/check_discussions_status_service.rb

+# frozen_string_literal: true
+module MergeRequests
+  module Mergeability
+    class CheckDiscussionsStatusService < CheckBaseService
+      def execute
+        if merge_request.mergeable_discussions_state?
+          success
+        else
+          failure
+        end
+      end
+
+      def skip?
+        params[:skip_discussions_check].present?
+      end
+
+      def cacheable?
+        false
+      end
+    end
+  end
+end

app/services/merge_requests/mergeability/run_checks_service.rb

@@ -7,6 +7,7 @@ module MergeRequests
       # We want to have the cheapest checks first in the list,
       # that way we can fail fast before running the more expensive ones
       CHECKS = [
+        CheckDiscussionsStatusService,
         CheckCiStatusService
       ].freeze
 

spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService do
+  subject(:check_discussions_status) { described_class.new(merge_request: merge_request, params: params) }
+
+  let(:merge_request) { build(:merge_request) }
+  let(:params) { { skip_discussions_check: skip_check } }
+  let(:skip_check) { false }
+
+  describe '#execute' do
+    before do
+      expect(merge_request).to receive(:mergeable_discussions_state?).and_return(mergeable)
+    end
+
+    context 'when the merge request is in a mergable state' do
+      let(:mergeable) { true }
+
+      it 'returns a check result with status success' do
+        expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS
+      end
+    end
+
+    context 'when the merge request is not in a mergeable state' do
+      let(:mergeable) { false }
+
+      it 'returns a check result with status failed' do
+        expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS
+      end
+    end
+  end
+
+  describe '#skip?' do
+    context 'when skip check is true' do
+      let(:skip_check) { true }
+
+      it 'returns true' do
+        expect(check_discussions_status.skip?).to eq true
+      end
+    end
+
+    context 'when skip check is false' do
+      let(:skip_check) { false }
+
+      it 'returns false' do
+        expect(check_discussions_status.skip?).to eq false
+      end
+    end
+  end
+
+  describe '#cacheable?' do
+    it 'returns false' do
+      expect(check_discussions_status.cacheable?).to eq false
+    end
+  end
+end

spec/services/merge_requests/mergeability/run_checks_service_spec.rb

@@ -35,12 +35,19 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do
 
     context 'when a check is skipped' do
       it 'does not execute the check' do
+        described_class::CHECKS.each do |check|
+          allow_next_instance_of(check) do |service|
+            allow(service).to receive(:skip?).and_return(false)
+            allow(service).to receive(:execute).and_return(success_result)
+          end
+        end
+
         expect_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service|
           expect(service).to receive(:skip?).and_return(true)
           expect(service).not_to receive(:execute)
         end
 
-        expect(execute).to match_array([])
+        expect(execute).to match_array([success_result])
       end
     end
 
@@ -49,6 +56,12 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do
       let(:merge_check) { instance_double(MergeRequests::Mergeability::CheckCiStatusService) }
 
       before do
+        described_class::CHECKS.each do |check|
+          allow_next_instance_of(check) do |service|
+            allow(service).to receive(:skip?).and_return(true)
+          end
+        end
+
         expect(MergeRequests::Mergeability::CheckCiStatusService).to receive(:new).and_return(merge_check)
         expect(merge_check).to receive(:skip?).and_return(false)
         allow(merge_check).to receive(:cacheable?).and_return(cacheable)