Merge branch 'issue_353234-move_related_issues_common_code_to_concerns' into 'master'

Move shared code for related epic links

See merge request gitlab-org/gitlab!81320

app/models/concerns/issuable_link.rb

@@ -10,15 +10,16 @@ module IssuableLink
   extend ActiveSupport::Concern
 
   TYPE_RELATES_TO = 'relates_to'
-  TYPE_BLOCKS = 'blocks'
-  # we don't store is_blocked_by in the db but need it for displaying the relation
-  # from the target
-  TYPE_IS_BLOCKED_BY = 'is_blocked_by'
+  TYPE_BLOCKS = 'blocks' ## EE-only. Kept here to be used on link_type enum.
 
   class_methods do
     def inverse_link_type(type)
       type
     end
+
+    def issuable_type
+      raise NotImplementedError
+    end
   end
 
   included do
@@ -44,12 +45,11 @@ module IssuableLink
       return unless source && target
 
       if self.class.base_class.find_by(source: target, target: source)
-        errors.add(:source, "is already related to this #{issuable_type}")
+        errors.add(:source, "is already related to this #{self.class.issuable_type}")
       end
     end
-
-    def issuable_type
-      raise NotImplementedError
-    end
   end
 end
+
+IssuableLink.prepend_mod_with('IssuableLink')
+IssuableLink::ClassMethods.prepend_mod_with('IssuableLink::ClassMethods')

app/models/issue_link.rb

@@ -10,10 +10,10 @@ class IssueLink < ApplicationRecord
   scope :for_source_issue, ->(issue) { where(source_id: issue.id) }
   scope :for_target_issue, ->(issue) { where(target_id: issue.id) }
 
-  private
-
-  def issuable_type
-    :issue
+  class << self
+    def issuable_type
+      :issue
+    end
   end
 end
 

ee/app/controllers/ee/boards/issues_controller.rb

@@ -7,7 +7,7 @@ module EE
 
       override :serializer_options
       def serializer_options(issues)
-        super.merge(blocked_issue_ids: ::IssueLink.blocked_issue_ids(issues.map(&:id)))
+        super.merge(blocked_issue_ids: ::IssueLink.blocked_issuable_ids(issues.map(&:id)))
       end
     end
   end

ee/app/models/concerns/ee/issuable_link.rb

+# frozen_string_literal: true
+
+module EE
+  module IssuableLink
+    extend ActiveSupport::Concern
+
+    prepended do
+      # we don't store is_blocked_by in the db but need it for displaying the relation
+      # from the target
+      TYPE_IS_BLOCKED_BY = 'is_blocked_by'
+    end
+
+    class_methods do
+      def inverse_link_type(type)
+        return IssuableLink::TYPE_IS_BLOCKED_BY if type == ::IssuableLink::TYPE_BLOCKS
+
+        super
+      end
+
+      def blocked_issuable_ids(issuable_ids)
+        blocked_or_blocking_issuables(issuable_ids).pluck(:target_id)
+      end
+
+      def blocking_issuables_ids_for(issuable)
+        blocked_or_blocking_issuables(issuable.id).pluck(:source_id)
+      end
+
+      def blocking_issuables_for_collection(issuables_ids)
+        open_state_id = ::Issuable::STATE_ID_MAP[:opened]
+        grouping_row_name = "blocking_#{issuable_type}_id"
+        issuable_table_name = issuable_type.to_s.pluralize
+        links_table_name = self.table_name
+
+        select("COUNT(CASE WHEN #{issuable_table_name}.state_id = #{open_state_id} then 1 else null end), #{links_table_name}.source_id AS #{grouping_row_name}")
+          .joins(:target)
+          .where(link_type: self::TYPE_BLOCKS, source_id: issuables_ids)
+          .group(grouping_row_name)
+      end
+
+      def blocked_issuables_for_collection(issuables_ids)
+        grouping_row_name = "blocked_#{issuable_type}_id"
+
+        select("COUNT(*), #{self.table_name}.target_id AS #{grouping_row_name}")
+          .joins(:source)
+          .where(source: { state_id: ::Issuable::STATE_ID_MAP[:opened] })
+          .where(link_type: self::TYPE_BLOCKS)
+          .where(target_id: issuables_ids)
+          .group(grouping_row_name)
+      end
+
+      def blocking_issuables_count_for(issue)
+        blocking_issuables_for_collection(issue.id)[0]&.count.to_i
+      end
+
+      private
+
+      def blocked_or_blocking_issuables(issuables_ids)
+        where(link_type: ::IssuableLink::TYPE_BLOCKS).where(target_id: issuables_ids)
+          .joins(:source)
+          .where(source: { state_id: ::Issuable::STATE_ID_MAP[:opened] })
+      end
+    end
+  end
+end

ee/app/models/ee/issue.rb

@@ -213,14 +213,14 @@ module EE
     end
 
     def update_blocking_issues_count!
-      blocking_count = ::IssueLink.blocking_issues_count_for(self)
+      blocking_count = ::IssueLink.blocking_issuables_count_for(self)
 
       update!(blocking_issues_count: blocking_count)
     end
 
     def refresh_blocking_and_blocked_issues_cache!
       self_and_blocking_issues_ids = [self.id] + blocking_issues_ids
-      blocking_issues_count_by_id = ::IssueLink.blocking_issues_for_collection(self_and_blocking_issues_ids).to_sql
+      blocking_issues_count_by_id = ::IssueLink.blocking_issuables_for_collection(self_and_blocking_issues_ids).to_sql
 
       self.class.connection.execute <<~SQL
         UPDATE issues
@@ -264,7 +264,7 @@ module EE
     private
 
     def blocking_issues_ids
-      @blocking_issues_ids ||= ::IssueLink.blocking_issue_ids_for(self)
+      @blocking_issues_ids ||= ::IssueLink.blocking_issuables_ids_for(self)
     end
 
     def validate_confidential_epic

ee/app/models/ee/issue_link.rb

@@ -5,54 +5,12 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
+      # TODO - Move this to EE::IssuableLink when `blocking_epics_count` is added to epics table.
+      # More information at https://gitlab.com/gitlab-org/gitlab/-/issues/353789.
       after_create :refresh_blocking_issue_cache
       after_destroy :refresh_blocking_issue_cache
     end
 
-    class_methods do
-      def inverse_link_type(type)
-        return ::IssueLink::TYPE_IS_BLOCKED_BY if type == ::IssueLink::TYPE_BLOCKS
-
-        type
-      end
-
-      def blocked_issue_ids(issue_ids)
-        blocked_or_blocking_issues(issue_ids).pluck(:target_id)
-      end
-
-      def blocking_issue_ids_for(issue)
-        blocked_or_blocking_issues(issue.id).pluck(:source_id)
-      end
-
-      def blocked_or_blocking_issues(issue_ids)
-        where(link_type: ::IssueLink::TYPE_BLOCKS).where(target_id: issue_ids)
-          .joins(:source)
-          .where(issues: { state_id: ::Issue.available_states[:opened] })
-      end
-
-      def blocking_issues_for_collection(issues_ids)
-        open_state_id = ::Issuable::STATE_ID_MAP[:opened]
-
-        select("COUNT(CASE WHEN issues.state_id = #{open_state_id} then 1 else null end), issue_links.source_id AS blocking_issue_id")
-          .joins(:target)
-          .where(link_type: ::IssueLink::TYPE_BLOCKS, source_id: issues_ids)
-          .group(:blocking_issue_id)
-      end
-
-      def blocked_issues_for_collection(issues_ids)
-        select('COUNT(*), issue_links.target_id AS blocked_issue_id')
-          .joins(:source)
-          .where(issues: { state_id: ::Issue.available_states[:opened] })
-          .where(link_type: ::IssueLink::TYPE_BLOCKS)
-          .where(target_id: issues_ids)
-          .group(:blocked_issue_id)
-      end
-
-      def blocking_issues_count_for(issue)
-        blocking_issues_for_collection(issue.id)[0]&.count.to_i
-      end
-    end
-
     private
 
     def blocking_issue

ee/app/models/epic/related_epic_link.rb

@@ -8,9 +8,12 @@ class Epic::RelatedEpicLink < ApplicationRecord
 
   self.table_name = 'related_epic_links'
 
-  private
+  class << self
+    extend ::Gitlab::Utils::Override
 
-  def issuable_type
-    :epic
+    override :issuable_type
+    def issuable_type
+      :epic
+    end
   end
 end

ee/app/services/epic_issues/list_service.rb

@@ -24,7 +24,7 @@ module EpicIssues
     def serializer_options
       child_issues_ids = child_issuables.map(&:id)
 
-      blocked_issues_ids = ::IssueLink.blocked_issue_ids(child_issues_ids)
+      blocked_issues_ids = ::IssueLink.blocked_issuable_ids(child_issues_ids)
 
       super.merge(blocked_issues_ids: blocked_issues_ids)
     end

ee/lib/ee/gitlab/issuable_metadata.rb

@@ -21,7 +21,7 @@ module EE
         strong_memoize(:grouped_blocking_issues_count) do
           next ::IssueLink.none unless collection_type == 'Issue'
 
-          ::IssueLink.blocking_issues_for_collection(issuable_ids)
+          ::IssueLink.blocking_issuables_for_collection(issuable_ids)
         end
       end
     end

ee/lib/gitlab/graphql/aggregations/issues/lazy_block_aggregate.rb

@@ -45,7 +45,7 @@ module Gitlab
             # The record hasn't been loaded yet, so
             # hit the database with all pending IDs to prevent N+1
             pending_ids = @lazy_state[:pending_ids].to_a
-            blocked_data = IssueLink.blocked_issues_for_collection(pending_ids).compact.flatten
+            blocked_data = IssueLink.blocked_issuables_for_collection(pending_ids).compact.flatten
 
             blocked_data.each do |blocked|
               @lazy_state[:loaded_objects][blocked.blocked_issue_id] = blocked.count

ee/spec/lib/gitlab/graphql/aggregations/issues/lazy_block_aggregate_spec.rb

@@ -53,7 +53,7 @@ RSpec.describe Gitlab::Graphql::Aggregations::Issues::LazyBlockAggregate do
       end
 
       it 'does not make the query again' do
-        expect(issue_link).not_to receive(:blocked_issues_for_collection)
+        expect(issue_link).not_to receive(:blocked_issuables_for_collection)
 
         subject.block_aggregate
       end
@@ -73,7 +73,7 @@ RSpec.describe Gitlab::Graphql::Aggregations::Issues::LazyBlockAggregate do
       end
 
       before do
-        expect(issue_link).to receive(:blocked_issues_for_collection).and_return(fake_data)
+        expect(issue_link).to receive(:blocked_issuables_for_collection).and_return(fake_data)
       end
 
       it 'clears the pending IDs' do

ee/spec/models/epic/related_epic_link_spec.rb

@@ -9,4 +9,18 @@ RSpec.describe Epic::RelatedEpicLink do
     let(:issuable_class) { 'Epic' }
     let(:issuable_link_factory) { :related_epic_link }
   end
+
+  it_behaves_like 'issuables that can block or be blocked' do
+    def factory_class
+      :related_epic_link
+    end
+
+    let(:issuable_type) { :epic }
+
+    let_it_be(:blocked_issuable_1) { create(:epic) }
+    let_it_be(:blocked_issuable_2) { create(:epic) }
+    let_it_be(:blocked_issuable_3) { create(:epic) }
+    let_it_be(:blocking_issuable_1) { create(:epic) }
+    let_it_be(:blocking_issuable_2) { create(:epic) }
+  end
 end

ee/spec/models/issue_link_spec.rb

@@ -52,81 +52,18 @@ RSpec.describe IssueLink do
     end
   end
 
-  describe '.blocked_issue_ids' do
-    it 'returns only ids of issues which are blocked' do
-      link1 = create(:issue_link, link_type: ::IssueLink::TYPE_BLOCKS)
-      link2 = create(:issue_link, link_type: ::IssueLink::TYPE_RELATES_TO)
-      link3 = create(:issue_link, source: create(:issue, :closed), link_type: ::IssueLink::TYPE_BLOCKS)
-
-      expect(described_class.blocked_issue_ids([link1.target_id, link2.source_id, link3.target_id]))
-        .to match_array([link1.target_id])
-    end
-  end
-
-  describe '.blocking_issue_ids_for' do
-    it 'returns blocking issue ids' do
-      issue = create(:issue)
-      blocking_issue = create(:issue, project: issue.project)
-      blocked_by_issue = create(:issue, project: issue.project)
-      create(:issue_link, source: blocking_issue, target: issue, link_type: ::IssueLink::TYPE_BLOCKS)
-      create(:issue_link, source: blocked_by_issue, target: issue, link_type: ::IssueLink::TYPE_BLOCKS)
-
-      blocking_ids = described_class.blocking_issue_ids_for(issue)
-
-      expect(blocking_ids).to match_array([blocking_issue.id, blocked_by_issue.id])
-    end
-  end
-
-  describe '.inverse_link_type' do
-    it 'returns reverse type of link' do
-      expect(described_class.inverse_link_type('relates_to')).to eq 'relates_to'
-      expect(described_class.inverse_link_type('blocks')).to eq 'is_blocked_by'
-    end
-  end
-
-  context 'blocking issues count' do
-    let_it_be(:blocked_issue_1) { create(:issue) }
-    let_it_be(:project) { blocked_issue_1.project }
-    let_it_be(:blocked_issue_2) { create(:issue, project: project) }
-    let_it_be(:blocked_issue_3) { create(:issue, project: project) }
-    let_it_be(:blocking_issue_1) { create(:issue, project: project) }
-    let_it_be(:blocking_issue_2) { create(:issue, project: project) }
-
-    before :all do
-      create(:issue_link, source: blocking_issue_1, target: blocked_issue_1, link_type: ::IssueLink::TYPE_BLOCKS)
-      create(:issue_link, source: blocking_issue_1, target: blocked_issue_2, link_type: ::IssueLink::TYPE_BLOCKS)
-      create(:issue_link, source: blocking_issue_2, target: blocked_issue_3, link_type: ::IssueLink::TYPE_BLOCKS)
+  it_behaves_like 'issuables that can block or be blocked' do
+    def factory_class
+      :issue_link
     end
 
-    describe '.blocking_issues_for_collection' do
-      it 'returns blocking issues count grouped by issue id' do
-        results = described_class.blocking_issues_for_collection([blocking_issue_1, blocking_issue_2])
-
-        expect(results.find { |link| link.blocking_issue_id == blocking_issue_1.id }.count).to eq(2)
-        expect(results.find { |link| link.blocking_issue_id == blocking_issue_2.id }.count).to eq(1)
-      end
-    end
-
-    describe '.blocked_issues_for_collection' do
-      it 'returns blocked issues count grouped by issue id' do
-        results = described_class.blocked_issues_for_collection([blocked_issue_1, blocked_issue_2, blocked_issue_3])
-
-        expect(result_by(results, blocked_issue_1.id).count).to eq(1)
-        expect(result_by(results, blocked_issue_2.id).count).to eq(1)
-        expect(result_by(results, blocked_issue_3.id).count).to eq(1)
-      end
-    end
-
-    describe '.blocking_issues_count_for' do
-      it 'returns blocked issues count for single issue' do
-        blocking_count = described_class.blocking_issues_count_for(blocking_issue_1)
-
-        expect(blocking_count).to eq(2)
-      end
-    end
-  end
+    let(:issuable_type) { :issue }
 
-  def result_by(results, id)
-    results.find { |link| link.blocked_issue_id == id }
+    let_it_be(:blocked_issuable_1) { create(:issue) }
+    let_it_be(:project) { blocked_issuable_1.project }
+    let_it_be(:blocked_issuable_2) { create(:issue, project: project) }
+    let_it_be(:blocked_issuable_3) { create(:issue, project: project) }
+    let_it_be(:blocking_issuable_1) { create(:issue, project: project) }
+    let_it_be(:blocking_issuable_2) { create(:issue, project: project) }
   end
 end

ee/spec/support/shared_examples/models/concerns/issuable_links_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'issuables that can block or be blocked' do
+  describe '.issuable_type' do
+    it { expect(described_class.issuable_type).to eq(issuable_type) }
+  end
+
+  describe '.inverse_link_type' do
+    it 'returns the inverse type of link' do
+      expect(described_class.inverse_link_type('relates_to')).to eq('relates_to')
+      expect(described_class.inverse_link_type('is_blocked_by')).to eq('is_blocked_by')
+      expect(described_class.inverse_link_type('blocks')).to eq('is_blocked_by')
+    end
+  end
+
+  describe '.blocked_issuable_ids' do
+    it 'returns only ids of issues which are blocked' do
+      link1 = create(factory_class, link_type: ::IssuableLink::TYPE_BLOCKS)
+      link2 = create(factory_class, link_type: ::IssuableLink::TYPE_RELATES_TO)
+      link3 = create(factory_class, source: create(issuable_type, :closed), link_type: ::IssuableLink::TYPE_BLOCKS)
+
+      expect(described_class.blocked_issuable_ids([link1.target_id, link2.source_id, link3.target_id]))
+        .to match_array([link1.target_id])
+    end
+  end
+
+  describe '.blocking_issuables_ids_for' do
+    it 'returns blocking issuables ids' do
+      create(factory_class, source: blocking_issuable_1, target: blocked_issuable_1, link_type: ::IssuableLink::TYPE_BLOCKS)
+      create(factory_class, source: blocking_issuable_2, target: blocked_issuable_1, link_type: ::IssuableLink::TYPE_BLOCKS)
+
+      blocking_ids = described_class.blocking_issuables_ids_for(blocked_issuable_1)
+
+      expect(blocking_ids).to match_array([blocking_issuable_1.id, blocking_issuable_2.id])
+    end
+  end
+
+  context 'blocking issuables count' do
+    before :all do
+      create(factory_class, source: blocking_issuable_1, target: blocked_issuable_1, link_type: ::IssuableLink::TYPE_BLOCKS)
+      create(factory_class, source: blocking_issuable_1, target: blocked_issuable_2, link_type: ::IssuableLink::TYPE_BLOCKS)
+      create(factory_class, source: blocking_issuable_2, target: blocked_issuable_3, link_type: ::IssuableLink::TYPE_BLOCKS)
+    end
+
+    describe '.blocking_issuables_for_collection' do
+      it 'returns blocking issues count grouped by issue id' do
+        grouping_row = "blocking_#{issuable_type}_id"
+
+        results = described_class.blocking_issuables_for_collection([blocking_issuable_1, blocking_issuable_2])
+
+        expect(results.find { |link| link[grouping_row] == blocking_issuable_1.id }.count).to eq(2)
+        expect(results.find { |link| link[grouping_row] == blocking_issuable_2.id }.count).to eq(1)
+      end
+    end
+
+    describe '.blocked_issuables_for_collection' do
+      it 'returns blocked issues count grouped by issue id' do
+        grouping_row = "blocked_#{issuable_type}_id"
+
+        results = described_class.blocked_issuables_for_collection([blocked_issuable_1, blocked_issuable_2, blocked_issuable_3])
+
+        expect(results.find { |link| link[grouping_row] == blocked_issuable_1.id }.count).to eq(1)
+        expect(results.find { |link| link[grouping_row] == blocked_issuable_1.id }.count).to eq(1)
+        expect(results.find { |link| link[grouping_row] == blocked_issuable_1.id }.count).to eq(1)
+      end
+    end
+
+    describe '.blocking_issuables_count_for' do
+      it 'returns blocked issues count for single issue' do
+        blocking_count = described_class.blocking_issuables_count_for(blocking_issuable_1)
+
+        expect(blocking_count).to eq(2)
+      end
+    end
+  end
+end

spec/models/concerns/issuable_link_spec.rb

@@ -21,8 +21,6 @@ RSpec.describe IssuableLink do
   describe '.inverse_link_type' do
     it 'returns the inverse type of link' do
       expect(test_class.inverse_link_type('relates_to')).to eq('relates_to')
-      expect(test_class.inverse_link_type('is_blocked_by')).to eq('is_blocked_by')
-      expect(test_class.inverse_link_type('blocks')).to eq('blocks')
     end
   end
 

spec/models/issue_link_spec.rb

@@ -10,6 +10,10 @@ RSpec.describe IssueLink do
     let(:issuable_link_factory) { :issue_link }
   end
 
+  describe '.issuable_type' do
+    it { expect(described_class.issuable_type).to eq(:issue) }
+  end
+
   describe 'Scopes' do
     let_it_be(:issue1) { create(:issue) }
     let_it_be(:issue2) { create(:issue) }