Merge branch...

Merge branch 'kerrizor/replace-delegated-predicate-methods-with-boolean-returning-ones-on-mrs' into 'master'

Prefer explicit methods over delegates for predicates

See merge request gitlab-org/gitlab!54610

app/models/merge_request.rb

@@ -376,8 +376,7 @@ class MergeRequest < ApplicationRecord
   alias_attribute :auto_merge_enabled, :merge_when_pipeline_succeeds
   alias_method :issuing_parent, :target_project
 
-  delegate :active?, :builds_with_coverage, to: :head_pipeline, prefix: true, allow_nil: true
-  delegate :success?, :active?, to: :actual_head_pipeline, prefix: true, allow_nil: true
+  delegate :builds_with_coverage, to: :head_pipeline, prefix: true, allow_nil: true
 
   RebaseLockTimeout = Class.new(StandardError)
 
@@ -437,6 +436,18 @@ class MergeRequest < ApplicationRecord
     target_project.latest_pipeline(target_branch, sha)
   end
 
+  def head_pipeline_active?
+    !!head_pipeline&.active?
+  end
+
+  def actual_head_pipeline_active?
+    !!actual_head_pipeline&.active?
+  end
+
+  def actual_head_pipeline_success?
+    !!actual_head_pipeline&.success?
+  end
+
   # Pattern used to extract `!123` merge request references from text
   #
   # This pattern supports cross-project references.

ee/app/models/ee/merge_request.rb

@@ -52,8 +52,6 @@ module EE
 
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
-      delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
-      delegate :merge_requests_disable_committers_approval?, to: :target_project, allow_nil: true
 
       accepts_nested_attributes_for :approval_rules, allow_destroy: true
 
@@ -71,6 +69,14 @@ module EE
       scope :including_merge_train, -> do
         includes(:merge_train)
       end
+
+      def merge_requests_author_approval?
+        !!target_project&.merge_requests_author_approval?
+      end
+
+      def merge_requests_disable_committers_approval?
+        !!target_project&.merge_requests_disable_committers_approval?
+      end
     end
 
     class_methods do

ee/app/models/ee/project.rb

@@ -240,15 +240,11 @@ module EE
       end
 
       def jira_vulnerabilities_integration_enabled?
-        return false unless jira_service
-
-        jira_service.jira_vulnerabilities_integration_enabled?
+        !!jira_service&.jira_vulnerabilities_integration_enabled?
       end
 
       def configured_to_create_issues_from_vulnerabilities?
-        return false unless jira_service
-
-        jira_service.configured_to_create_issues_from_vulnerabilities?
+        !!jira_service&.configured_to_create_issues_from_vulnerabilities?
       end
     end
 

ee/spec/models/merge_request_spec.rb

@@ -93,6 +93,48 @@ RSpec.describe MergeRequest do
         end
       end
     end
+
+    describe '#merge_requests_author_approval?' do
+      context 'when project lacks a target_project relation' do
+        before do
+          merge_request.target_project = nil
+        end
+
+        it 'returns false' do
+          expect(merge_request.merge_requests_author_approval?).to be false
+        end
+      end
+
+      context 'when project has a target_project relation' do
+        it 'accesses the value from the target_project' do
+          expect(merge_request.target_project)
+            .to receive(:merge_requests_author_approval?)
+
+          merge_request.merge_requests_author_approval?
+        end
+      end
+    end
+
+    describe '#merge_requests_disable_committers_approval?' do
+      context 'when project lacks a target_project relation' do
+        before do
+          merge_request.target_project = nil
+        end
+
+        it 'returns false' do
+          expect(merge_request.merge_requests_disable_committers_approval?).to be false
+        end
+      end
+
+      context 'when project has a target_project relation' do
+        it 'accesses the value from the target_project' do
+          expect(merge_request.target_project)
+            .to receive(:merge_requests_disable_committers_approval?)
+
+          merge_request.merge_requests_disable_committers_approval?
+        end
+      end
+    end
   end
 
   it_behaves_like 'an editable mentionable with EE-specific mentions' do

ee/spec/models/project_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe Project do
     describe '#jira_vulnerabilities_integration_enabled?' do
       context 'when project lacks a jira_service relation' do
         it 'returns false' do
-          expect(project.jira_vulnerabilities_integration_enabled?).to be_falsey
+          expect(project.jira_vulnerabilities_integration_enabled?).to be false
         end
       end
 
@@ -83,7 +83,7 @@ RSpec.describe Project do
     describe '#configured_to_create_issues_from_vulnerabilities?' do
       context 'when project lacks a jira_service relation' do
         it 'returns false' do
-          expect(project.configured_to_create_issues_from_vulnerabilities?).to be_falsey
+          expect(project.configured_to_create_issues_from_vulnerabilities?).to be false
         end
       end
 

spec/models/merge_request_spec.rb

@@ -3082,32 +3082,83 @@ RSpec.describe MergeRequest, factory_default: :keep do
   end
 
   describe "#head_pipeline_active? " do
-    it do
-      is_expected
-        .to delegate_method(:active?)
-        .to(:head_pipeline)
-        .with_prefix
-        .with_arguments(allow_nil: true)
+    context 'when project lacks a head_pipeline relation' do
+      before do
+        subject.head_pipeline = nil
+      end
+
+      it 'returns false' do
+        expect(subject.head_pipeline_active?).to be false
+      end
+    end
+
+    context 'when project has a head_pipeline relation' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+
+      before do
+        allow(subject).to receive(:head_pipeline) { pipeline }
+      end
+
+      it 'accesses the value from the head_pipeline' do
+        expect(subject.head_pipeline)
+          .to receive(:active?)
+
+        subject.head_pipeline_active?
+      end
     end
   end
 
   describe "#actual_head_pipeline_success? " do
-    it do
-      is_expected
-        .to delegate_method(:success?)
-        .to(:actual_head_pipeline)
-        .with_prefix
-        .with_arguments(allow_nil: true)
+    context 'when project lacks an actual_head_pipeline relation' do
+      before do
+        allow(subject).to receive(:actual_head_pipeline) { nil }
+      end
+
+      it 'returns false' do
+        expect(subject.actual_head_pipeline_success?).to be false
+      end
+    end
+
+    context 'when project has a actual_head_pipeline relation' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+
+      before do
+        allow(subject).to receive(:actual_head_pipeline) { pipeline }
+      end
+
+      it 'accesses the value from the actual_head_pipeline' do
+        expect(subject.actual_head_pipeline)
+          .to receive(:success?)
+
+        subject.actual_head_pipeline_success?
+      end
     end
   end
 
   describe "#actual_head_pipeline_active? " do
-    it do
-      is_expected
-        .to delegate_method(:active?)
-        .to(:actual_head_pipeline)
-        .with_prefix
-        .with_arguments(allow_nil: true)
+    context 'when project lacks an actual_head_pipeline relation' do
+      before do
+        allow(subject).to receive(:actual_head_pipeline) { nil }
+      end
+
+      it 'returns false' do
+        expect(subject.actual_head_pipeline_active?).to be false
+      end
+    end
+
+    context 'when project has a actual_head_pipeline relation' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+
+      before do
+        allow(subject).to receive(:actual_head_pipeline) { pipeline }
+      end
+
+      it 'accesses the value from the actual_head_pipeline' do
+        expect(subject.actual_head_pipeline)
+          .to receive(:active?)
+
+        subject.actual_head_pipeline_active?
+      end
     end
   end