From c00470dbd23703b68962c93178ef825e1b8c6237 Mon Sep 17 00:00:00 2001 From: Matt D'Angelo Date: Mon, 1 Sep 2025 16:03:54 +0930 Subject: [PATCH 1/2] Apply changes from !200898 --- .rubocop_todo/rspec/named_subject.yml | 1 - .../resolvers/work_items/types_resolver.rb | 7 - app/models/work_item.rb | 18 +- app/models/work_items/parent_link.rb | 14 +- .../system_defined/hierarchy_restriction.rb | 66 +++++ app/models/work_items/type.rb | 25 +- .../models/ee/work_items/parent_link_spec.rb | 54 +--- .../ee/notes/quick_actions_service_spec.rb | 13 +- .../issue_promote_service_spec.rb | 13 +- locale/gitlab.pot | 3 - .../work_items/work_item_hierarchy_spec.rb | 96 ++++-- spec/models/work_item_spec.rb | 276 ++++++------------ spec/models/work_items/parent_link_spec.rb | 130 +++------ .../hierarchy_restriction_spec.rb | 91 ++++++ spec/models/work_items/type_spec.rb | 181 ++++++------ .../work_item_type_list_shared_examples.rb | 2 +- ...eable_and_moveable_data_stared_examples.rb | 11 +- .../hierarchy_depth_shared_examples.rb | 29 ++ 18 files changed, 529 insertions(+), 501 deletions(-) create mode 100644 app/models/work_items/system_defined/hierarchy_restriction.rb create mode 100644 spec/models/work_items/system_defined/hierarchy_restriction_spec.rb create mode 100644 spec/support/shared_examples/work_items/hierarchy_depth_shared_examples.rb diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index 9a875882441b99..818603c9bcc09c 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -2454,7 +2454,6 @@ RSpec/NamedSubject: - 'spec/models/users/namespace_commit_email_spec.rb' - 'spec/models/web_ide_terminal_spec.rb' - 'spec/models/wiki_page/meta_spec.rb' - - 'spec/models/work_items/type_spec.rb' - 'spec/models/x509_certificate_spec.rb' - 'spec/models/zoom_meeting_spec.rb' - 'spec/policies/concerns/policy_actor_spec.rb' diff --git a/app/graphql/resolvers/work_items/types_resolver.rb b/app/graphql/resolvers/work_items/types_resolver.rb index 4eb796f7e1e374..b61b8c9914a5ad 100644 --- a/app/graphql/resolvers/work_items/types_resolver.rb +++ b/app/graphql/resolvers/work_items/types_resolver.rb @@ -29,13 +29,6 @@ def preloads widget_definitions: :enabled_widget_definitions } end - - def nested_preloads - { - widget_definitions: { allowed_child_types: :allowed_child_types_by_name, - allowed_parent_types: :allowed_parent_types_by_name } - } - end end end end diff --git a/app/models/work_item.rb b/app/models/work_item.rb index 8ac9eeeaf634f8..2c3b5cc340bcc2 100644 --- a/app/models/work_item.rb +++ b/app/models/work_item.rb @@ -393,9 +393,9 @@ def start_date end def max_depth_reached?(child_type) - restriction = ::WorkItems::HierarchyRestriction.find_by_parent_type_id_and_child_type_id( - work_item_type_id, - child_type.id + restriction = ::WorkItems::SystemDefined::HierarchyRestriction.find_by( + parent_type_id: work_item_type_id, + child_type_id: child_type.id ) return false unless restriction&.maximum_depth @@ -472,8 +472,8 @@ def validate_parent_restrictions(parent_link) def validate_child_restrictions(child_links) return if child_links.empty? - child_type_ids = child_links.joins(:work_item).select(self.class.arel_table[:work_item_type_id]).distinct - restrictions = ::WorkItems::HierarchyRestriction.where( + child_type_ids = child_links.joins(:work_item).distinct.pluck('issues.work_item_type_id') # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Limited number of work item types + restrictions = ::WorkItems::SystemDefined::HierarchyRestriction.where( parent_type_id: work_item_type_id, child_type_id: child_type_ids ) @@ -488,9 +488,9 @@ def validate_child_restrictions(child_links) end def validate_depth(parent_link, child_links) - restriction = ::WorkItems::HierarchyRestriction.find_by_parent_type_id_and_child_type_id( - work_item_type_id, - work_item_type_id + restriction = ::WorkItems::SystemDefined::HierarchyRestriction.find_by( + parent_type_id: work_item_type_id, + child_type_id: work_item_type_id ) return unless restriction&.maximum_depth @@ -511,7 +511,7 @@ def validate_depth(parent_link, child_links) end def hierarchy_supports_parent? - ::WorkItems::HierarchyRestriction.find_by_child_type_id(work_item_type_id).present? + ::WorkItems::SystemDefined::HierarchyRestriction.find_by(child_type_id: work_item_type_id).present? end end diff --git a/app/models/work_items/parent_link.rb b/app/models/work_items/parent_link.rb index a103144be725e5..cfca452aa96a92 100644 --- a/app/models/work_items/parent_link.rb +++ b/app/models/work_items/parent_link.rb @@ -74,8 +74,10 @@ def validate_confidentiality def validate_hierarchy_restrictions return unless work_item && work_item_parent - restriction = ::WorkItems::HierarchyRestriction - .find_by_parent_type_id_and_child_type_id(work_item_parent.work_item_type_id, work_item.work_item_type_id) + restriction = ::WorkItems::SystemDefined::HierarchyRestriction.find_by( + parent_type_id: work_item_parent.work_item_type_id, + child_type_id: work_item.work_item_type_id + ) if restriction.nil? errors.add :work_item, _("it's not allowed to add this type of parent item") @@ -83,14 +85,6 @@ def validate_hierarchy_restrictions end validate_depth(restriction.maximum_depth) - validate_cross_hierarchy(restriction.cross_hierarchy_enabled) - end - - def validate_cross_hierarchy(cross_hierarchy_enabled) - return if cross_hierarchy_enabled - return if work_item.resource_parent == work_item_parent.resource_parent - - errors.add :work_item_parent, _('parent must be in the same project or group as child.') end def validate_depth(depth) diff --git a/app/models/work_items/system_defined/hierarchy_restriction.rb b/app/models/work_items/system_defined/hierarchy_restriction.rb new file mode 100644 index 00000000000000..f46161bf047b6b --- /dev/null +++ b/app/models/work_items/system_defined/hierarchy_restriction.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module WorkItems + module SystemDefined + class HierarchyRestriction + include ActiveRecord::FixedItemsModel::Model + + attribute :parent_type_id, :integer + attribute :child_type_id, :integer + attribute :maximum_depth, :integer + + EPIC_ID = ::WorkItems::Type::BASE_TYPES[:epic][:id] + ISSUE_ID = ::WorkItems::Type::BASE_TYPES[:issue][:id] + TASK_ID = ::WorkItems::Type::BASE_TYPES[:task][:id] + OBJECTIVE_ID = ::WorkItems::Type::BASE_TYPES[:objective][:id] + KEY_RESULT_ID = ::WorkItems::Type::BASE_TYPES[:key_result][:id] + INCIDENT_ID = ::WorkItems::Type::BASE_TYPES[:incident][:id] + TICKET_ID = ::WorkItems::Type::BASE_TYPES[:ticket][:id] + + ITEMS = [ + { + id: 1, + parent_type_id: OBJECTIVE_ID, + child_type_id: OBJECTIVE_ID, + maximum_depth: 9 + }, + { + id: 2, + parent_type_id: OBJECTIVE_ID, + child_type_id: KEY_RESULT_ID, + maximum_depth: 1 + }, + { + id: 3, + parent_type_id: ISSUE_ID, + child_type_id: TASK_ID, + maximum_depth: 1 + }, + { + id: 4, + parent_type_id: INCIDENT_ID, + child_type_id: TASK_ID, + maximum_depth: 1 + }, + { + id: 5, + parent_type_id: EPIC_ID, + child_type_id: EPIC_ID, + maximum_depth: 7 + }, + { + id: 6, + parent_type_id: EPIC_ID, + child_type_id: ISSUE_ID, + maximum_depth: 1 + }, + { + id: 7, + parent_type_id: TICKET_ID, + child_type_id: TASK_ID, + maximum_depth: 1 + } + ].freeze + end + end +end diff --git a/app/models/work_items/type.rb b/app/models/work_items/type.rb index 5a7584eaf0c4ee..82821b5ef29767 100644 --- a/app/models/work_items/type.rb +++ b/app/models/work_items/type.rb @@ -64,16 +64,6 @@ class Type < ApplicationRecord has_many :widget_definitions, foreign_key: :work_item_type_id, inverse_of: :work_item_type has_many :enabled_widget_definitions, -> { where(disabled: false) }, foreign_key: :work_item_type_id, inverse_of: :work_item_type, class_name: 'WorkItems::WidgetDefinition' - has_many :child_restrictions, class_name: 'WorkItems::HierarchyRestriction', foreign_key: :parent_type_id, - inverse_of: :parent_type - has_many :parent_restrictions, class_name: 'WorkItems::HierarchyRestriction', foreign_key: :child_type_id, - inverse_of: :child_type - has_many :allowed_child_types_by_name, -> { order_by_name_asc }, - through: :child_restrictions, class_name: 'WorkItems::Type', - foreign_key: :child_type_id, source: :child_type - has_many :allowed_parent_types_by_name, -> { order_by_name_asc }, - through: :parent_restrictions, class_name: 'WorkItems::Type', - foreign_key: :parent_type_id, source: :parent_type has_many :user_preferences, class_name: 'WorkItems::UserPreference', inverse_of: :work_item_type @@ -146,6 +136,21 @@ def default_issue? name == WorkItems::Type::TYPE_NAMES[:issue] end + def allowed_child_types_by_name + child_type_ids = WorkItems::SystemDefined::HierarchyRestriction + .where(parent_type_id: id) + .map(&:child_type_id) + + WorkItems::Type.where(id: child_type_ids).order_by_name_asc + end + + def allowed_parent_types_by_name + parent_type_ids = WorkItems::SystemDefined::HierarchyRestriction + .where(child_type_id: id) + .map(&:parent_type_id) + WorkItems::Type.where(id: parent_type_ids).order_by_name_asc + end + def calculate_reactive_cache { allowed_child_types_by_name: allowed_child_types_by_name, diff --git a/ee/spec/models/ee/work_items/parent_link_spec.rb b/ee/spec/models/ee/work_items/parent_link_spec.rb index 887fe628ba129d..6587b34f12bcb2 100644 --- a/ee/spec/models/ee/work_items/parent_link_spec.rb +++ b/ee/spec/models/ee/work_items/parent_link_spec.rb @@ -20,53 +20,23 @@ end end - describe 'validations' do - describe 'validate_hierarchy_restrictions' do - context 'when assigning parent from a different resource parent' do - let_it_be(:issue) { create(:work_item, :issue, project: project) } - let_it_be(:epic) { create(:work_item, :epic, namespace: create(:group)) } + describe '#validate_legacy_hierarchy' do + context 'when assigning a parent with type Epic' do + let_it_be_with_reload(:issue) { create(:work_item, project: project) } + let_it_be(:legacy_epic) { create(:epic, group: group) } + let_it_be(:epic) { create(:work_item, :epic, project: project) } - let(:restriction) do - WorkItems::HierarchyRestriction - .find_by_parent_type_id_and_child_type_id(epic.work_item_type_id, issue.work_item_type_id) - end + subject { described_class.new(work_item: issue, work_item_parent: epic) } - it 'is valid when cross-hierarchy is enabled' do - restriction.update!(cross_hierarchy_enabled: true) - link = build(:parent_link, work_item_parent: epic, work_item: issue) - - expect(link).to be_valid - expect(link.errors).to be_empty - end - - it 'is not valid when cross-hierarchy is not enabled' do - restriction.update!(cross_hierarchy_enabled: false) - link = build(:parent_link, work_item_parent: epic, work_item: issue) - - expect(link).not_to be_valid - expect(link.errors[:work_item_parent]).to include('parent must be in the same project or group as child.') - end + it 'is valid for child with no legacy epic' do + expect(subject).to be_valid end - end - - describe 'validate_legacy_hierarchy' do - context 'when assigning a parent with type Epic' do - let_it_be_with_reload(:issue) { create(:work_item, project: project) } - let_it_be(:legacy_epic) { create(:epic, group: group) } - let_it_be(:epic) { create(:work_item, :epic, project: project) } - - subject { described_class.new(work_item: issue, work_item_parent: epic) } - - it 'is valid for child with no legacy epic' do - expect(subject).to be_valid - end - it 'is invalid for child with existing legacy epic', :aggregate_failures do - create(:epic_issue, epic: legacy_epic, issue: issue) + it 'is invalid for child with existing legacy epic', :aggregate_failures do + create(:epic_issue, epic: legacy_epic, issue: issue) - expect(subject).to be_invalid - expect(subject.errors.full_messages).to include('Work item already assigned to an epic') - end + expect(subject).to be_invalid + expect(subject.errors.full_messages).to include('Work item already assigned to an epic') end end end diff --git a/ee/spec/services/ee/notes/quick_actions_service_spec.rb b/ee/spec/services/ee/notes/quick_actions_service_spec.rb index 1efec668f6eddf..7633d1c2695a20 100644 --- a/ee/spec/services/ee/notes/quick_actions_service_spec.rb +++ b/ee/spec/services/ee/notes/quick_actions_service_spec.rb @@ -882,8 +882,17 @@ def execute(note, include_message: false) before do epic_type = WorkItems::Type.default_by_type(:epic) - WorkItems::HierarchyRestriction.where(parent_type: epic_type, - child_type: epic_type).update!(maximum_depth: 0) + allow(WorkItems::SystemDefined::HierarchyRestriction).to receive(:find_by).with( + parent_type_id: epic_type.id, + child_type_id: epic_type.id + ).and_return( + instance_double( + WorkItems::SystemDefined::HierarchyRestriction, + maximum_depth: 0, + parent_type_id: epic_type.id, + child_type_id: epic_type.id + ) + ) end it 'does not promote the issue and returns an error message' do diff --git a/ee/spec/services/work_items/legacy_epics/issue_promote_service_spec.rb b/ee/spec/services/work_items/legacy_epics/issue_promote_service_spec.rb index 6dc254ad6ecb3d..e597f1e0afb08e 100644 --- a/ee/spec/services/work_items/legacy_epics/issue_promote_service_spec.rb +++ b/ee/spec/services/work_items/legacy_epics/issue_promote_service_spec.rb @@ -260,8 +260,17 @@ before do epic_type = WorkItems::Type.default_by_type(:epic) - WorkItems::HierarchyRestriction.where(parent_type: epic_type, - child_type: epic_type).update!(maximum_depth: 0) + allow(WorkItems::SystemDefined::HierarchyRestriction).to receive(:find_by).with( + parent_type_id: epic_type.id, + child_type_id: epic_type.id + ).and_return( + instance_double( + WorkItems::SystemDefined::HierarchyRestriction, + maximum_depth: 0, + parent_type_id: epic_type.id, + child_type_id: epic_type.id + ) + ) end it 'rejects promoting an issue to an epic' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6edf16a7898f54..3f336ef5b243e2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -77656,9 +77656,6 @@ msgstr[1] "" msgid "parent already has maximum number of children." msgstr "" -msgid "parent must be in the same project or group as child." -msgstr "" - msgid "password" msgstr "" diff --git a/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb b/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb index 679400517cd8e5..1a38f66e51a723 100644 --- a/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb +++ b/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb @@ -4,46 +4,50 @@ RSpec.describe Gitlab::WorkItems::WorkItemHierarchy, feature_category: :portfolio_management do let_it_be(:project) { create(:project) } - let_it_be(:type1) { create(:work_item_type, :non_default) } - let_it_be(:type2) { create(:work_item_type, :non_default) } - let_it_be(:hierarchy_restriction1) { create(:hierarchy_restriction, parent_type: type1, child_type: type2) } - let_it_be(:hierarchy_restriction2) { create(:hierarchy_restriction, parent_type: type2, child_type: type2) } - let_it_be(:hierarchy_restriction3) { create(:hierarchy_restriction, parent_type: type2, child_type: type1) } - let_it_be(:item1) { create(:work_item, work_item_type: type1, project: project) } - let_it_be(:item2) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:item3) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:item4) { create(:work_item, work_item_type: type1, project: project) } - let_it_be(:ignored1) { create(:work_item, work_item_type: type1, project: project) } - let_it_be(:ignored2) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:link1) { create(:parent_link, work_item_parent: item1, work_item: item2) } - let_it_be(:link2) { create(:parent_link, work_item_parent: item2, work_item: item3) } - let_it_be(:link3) { create(:parent_link, work_item_parent: item3, work_item: item4) } + + let_it_be(:epic1) { create(:work_item, :epic, project: project) } + let_it_be(:epic2) { create(:work_item, :epic, project: project) } + let_it_be(:epic3) { create(:work_item, :epic, project: project) } + let_it_be(:issue1) { create(:work_item, :issue, project: project) } + let_it_be(:task1) { create(:work_item, :task, project: project) } + + let_it_be(:ignored_epic) { create(:work_item, :epic, project: project) } + let_it_be(:ignored_issue) { create(:work_item, :issue, project: project) } + + # Create hierarchy: epic1 -> epic2 -> epic3 -> issue1 -> task1 + let_it_be(:link1) { create(:parent_link, work_item_parent: epic1, work_item: epic2) } + let_it_be(:link2) { create(:parent_link, work_item_parent: epic2, work_item: epic3) } + let_it_be(:link3) { create(:parent_link, work_item_parent: epic3, work_item: issue1) } + let_it_be(:link4) { create(:parent_link, work_item_parent: issue1, work_item: task1) } let(:options) { {} } describe '#base_and_ancestors' do - subject { described_class.new(::WorkItem.where(id: item3.id), options: options) } + subject { described_class.new(::WorkItem.where(id: issue1.id), options: options) } it 'includes the base and its ancestors' do relation = subject.base_and_ancestors - expect(relation).to eq([item3, item2, item1]) + expect(relation).to match_array([issue1, epic3, epic2, epic1]) end context 'when same_type option is used' do + # Use epic3 as subject since it has same-type ancestors + subject { described_class.new(::WorkItem.where(id: epic3.id), options: options) } + let(:options) { { same_type: true } } - it 'includes the base and its ancestors' do + it 'includes the base and its ancestors of the same type' do relation = subject.base_and_ancestors - expect(relation).to eq([item3, item2]) + expect(relation).to match_array([epic3, epic2, epic1]) end end it 'can find ancestors upto a certain level' do - relation = subject.base_and_ancestors(upto: item1) + relation = subject.base_and_ancestors(upto: epic2) - expect(relation).to eq([item3, item2]) + expect(relation).to match_array([issue1, epic3]) end describe 'hierarchy_order option' do @@ -55,7 +59,7 @@ let(:hierarchy_order) { :asc } it 'orders by child to ancestor' do - expect(relation).to eq([item3, item2, item1]) + expect(relation).to match_array([issue1, epic3, epic2, epic1]) end end @@ -63,28 +67,28 @@ let(:hierarchy_order) { :desc } it 'orders by ancestor to child' do - expect(relation).to eq([item1, item2, item3]) + expect(relation).to match_array([epic1, epic2, epic3, issue1]) end end end end describe '#base_and_descendants' do - subject { described_class.new(::WorkItem.where(id: item2.id), options: options) } + subject { described_class.new(::WorkItem.where(id: epic2.id), options: options) } it 'includes the base and its descendants' do relation = subject.base_and_descendants - expect(relation).to eq([item2, item3, item4]) + expect(relation).to match_array([epic2, epic3, issue1, task1]) end context 'when same_type option is used' do let(:options) { { same_type: true } } - it 'includes the base and its ancestors' do + it 'includes the base and its descendants of the same type' do relation = subject.base_and_descendants - expect(relation).to eq([item2, item3]) + expect(relation).to match_array([epic2, epic3]) end end @@ -95,9 +99,10 @@ it 'includes depth in the results' do object_depths = { - item2.id => 1, - item3.id => 2, - item4.id => 3 + epic2.id => 1, + epic3.id => 2, + issue1.id => 3, + task1.id => 4 } relation.each do |object| @@ -106,4 +111,37 @@ end end end + + describe 'with objective hierarchy' do + let_it_be(:objective1) { create(:work_item, :objective, project: project) } + let_it_be(:objective2) { create(:work_item, :objective, project: project) } + let_it_be(:objective3) { create(:work_item, :objective, project: project) } + let_it_be(:key_result) { create(:work_item, :key_result, project: project) } + + let_it_be(:obj_link1) { create(:parent_link, work_item_parent: objective1, work_item: objective2) } + let_it_be(:obj_link2) { create(:parent_link, work_item_parent: objective2, work_item: objective3) } + let_it_be(:obj_link3) { create(:parent_link, work_item_parent: objective3, work_item: key_result) } + + describe '#base_and_ancestors for objectives' do + subject { described_class.new(::WorkItem.where(id: key_result.id), options: options) } + + it 'includes key result and all objective ancestors' do + relation = subject.base_and_ancestors + + expect(relation).to match_array([key_result, objective3, objective2, objective1]) + end + + context 'when same_type option is used' do + subject { described_class.new(::WorkItem.where(id: objective3.id), options: options) } + + let(:options) { { same_type: true } } + + it 'includes only objectives' do + relation = subject.base_and_ancestors + + expect(relation).to match_array([objective3, objective2, objective1]) + end + end + end + end end diff --git a/spec/models/work_item_spec.rb b/spec/models/work_item_spec.rb index 5163b7c9eea66a..703d9809b34c8d 100644 --- a/spec/models/work_item_spec.rb +++ b/spec/models/work_item_spec.rb @@ -670,62 +670,61 @@ end context 'with hierarchy' do - let_it_be(:type1) { create(:work_item_type, :non_default) } - let_it_be(:type2) { create(:work_item_type, :non_default) } - let_it_be(:type3) { create(:work_item_type, :non_default) } - let_it_be(:type4) { create(:work_item_type, :non_default) } - let_it_be(:hierarchy_restriction1) { create(:hierarchy_restriction, parent_type: type1, child_type: type2) } - let_it_be(:hierarchy_restriction2) { create(:hierarchy_restriction, parent_type: type2, child_type: type2) } - let_it_be(:hierarchy_restriction3) { create(:hierarchy_restriction, parent_type: type2, child_type: type3) } - let_it_be(:hierarchy_restriction4) { create(:hierarchy_restriction, parent_type: type3, child_type: type3) } - let_it_be(:hierarchy_restriction5) { create(:hierarchy_restriction, parent_type: type3, child_type: type4) } - let_it_be(:item1) { create(:work_item, work_item_type: type1, project: reusable_project) } - let_it_be(:item2_1) { create(:work_item, work_item_type: type2, project: reusable_project) } - let_it_be(:item2_2) { create(:work_item, work_item_type: type2, project: reusable_project) } - let_it_be(:item3_1) { create(:work_item, work_item_type: type3, project: reusable_project) } - let_it_be(:item3_2) { create(:work_item, work_item_type: type3, project: reusable_project) } - let_it_be(:item4) { create(:work_item, work_item_type: type4, project: reusable_project) } - let_it_be(:ignored_ancestor) { create(:work_item, work_item_type: type1, project: reusable_project) } - let_it_be(:ignored_descendant) { create(:work_item, work_item_type: type4, project: reusable_project) } - let_it_be(:link1) { create(:parent_link, work_item_parent: item1, work_item: item2_1) } - let_it_be(:link2) { create(:parent_link, work_item_parent: item2_1, work_item: item2_2) } - let_it_be(:link3) { create(:parent_link, work_item_parent: item2_2, work_item: item3_1) } - let_it_be(:link4) { create(:parent_link, work_item_parent: item3_1, work_item: item3_2) } - let_it_be(:link5) { create(:parent_link, work_item_parent: item3_2, work_item: item4) } + let_it_be(:epic1) { create(:work_item, :epic, project: reusable_project) } + let_it_be(:epic2) { create(:work_item, :epic, project: reusable_project) } + let_it_be(:epic3) { create(:work_item, :epic, project: reusable_project) } + let_it_be(:issue1) { create(:work_item, :issue, project: reusable_project) } + let_it_be(:issue2) { create(:work_item, :issue, project: reusable_project) } + let_it_be(:task1) { create(:work_item, :task, project: reusable_project) } + let_it_be(:task2) { create(:work_item, :task, project: reusable_project) } + + let_it_be(:ignored_ancestor) { create(:work_item, :epic, project: reusable_project) } + let_it_be(:ignored_descendant) { create(:work_item, :task, project: reusable_project) } + + # Create the hierarchy: + # epic1 -> epic2 -> epic3 -> issue1 -> task1 + # \-> issue2 -> task2 + let_it_be(:link1) { create(:parent_link, work_item_parent: epic1, work_item: epic2) } + let_it_be(:link2) { create(:parent_link, work_item_parent: epic2, work_item: epic3) } + let_it_be(:link3) { create(:parent_link, work_item_parent: epic3, work_item: issue1) } + let_it_be(:link4) { create(:parent_link, work_item_parent: epic3, work_item: issue2) } + let_it_be(:link5) { create(:parent_link, work_item_parent: issue1, work_item: task1) } + let_it_be(:link6) { create(:parent_link, work_item_parent: issue2, work_item: task2) } describe '#ancestors' do it 'returns all ancestors in ascending order' do - expect(item3_1.ancestors).to eq([item2_2, item2_1, item1]) + expect(task1.ancestors).to eq([issue1, epic3, epic2, epic1]) end it 'returns an empty array if there are no ancestors' do - expect(item1.ancestors).to be_empty + expect(epic1.ancestors).to be_empty end end describe '#descendants' do it 'returns all descendants' do - expect(item1.descendants).to match_array([item2_1, item2_2, item3_1, item3_2, item4]) + expect(epic1.descendants).to match_array([epic2, epic3, issue1, issue2, task1, task2]) end end describe '#same_type_base_and_ancestors' do it 'returns self and all ancestors of the same type in ascending order' do - expect(item3_2.same_type_base_and_ancestors).to eq([item3_2, item3_1]) + expect(epic3.same_type_base_and_ancestors).to eq([epic3, epic2, epic1]) end it 'returns self if there are no ancestors of the same type' do - expect(item3_1.same_type_base_and_ancestors).to match_array([item3_1]) + expect(issue1.same_type_base_and_ancestors).to match_array([issue1]) end end describe '#same_type_descendants_depth' do it 'returns max descendants depth including self' do - expect(item3_1.same_type_descendants_depth).to eq(2) + # epic1 has epic2 and epic3 as same-type descendants, so depth is 3 + expect(epic1.same_type_descendants_depth).to eq(3) end it 'returns 1 if there are no descendants' do - expect(item1.same_type_descendants_depth).to eq(1) + expect(task1.same_type_descendants_depth).to eq(1) end end end @@ -746,165 +745,74 @@ end context 'with ParentLink relation' do - let_it_be(:old_type) { create(:work_item_type, :non_default) } - let_it_be(:new_type) { create(:work_item_type, :non_default) } + describe 'type conversion validations' do + let_it_be_with_reload(:issue_without_children) { create(:work_item, :issue, project: reusable_project) } + let_it_be_with_reload(:issue_with_task) { create(:work_item, :issue, project: reusable_project) } + let_it_be_with_reload(:task_child) { create(:work_item, :task, project: reusable_project) } - context 'with hierarchy restrictions' do - let_it_be(:child_type) { create(:work_item_type, :non_default) } - - let_it_be_with_reload(:parent) { create(:work_item, work_item_type: old_type, project: reusable_project) } - let_it_be_with_reload(:child) { create(:work_item, work_item_type: child_type, project: reusable_project) } - - let_it_be(:hierarchy_restriction) do - create(:hierarchy_restriction, parent_type: old_type, child_type: child_type) + before do + create(:parent_link, work_item_parent: issue_with_task, work_item: task_child) end - let_it_be(:link) { create(:parent_link, work_item_parent: parent, work_item: child) } - - context 'when child items restrict the type change' do - before do - parent.work_item_type = new_type - end - - context 'when child items are compatible with the new type' do - let_it_be(:hierarchy_restriction_new_type) do - create(:hierarchy_restriction, parent_type: new_type, child_type: child_type) - end - - it 'allows to change types' do - expect(parent).to be_valid - expect(parent.errors).to be_empty - end - end - - context 'when child items are not compatible with the new type' do - it 'does not allow to change types' do - expect(parent).not_to be_valid - expect(parent.errors[:work_item_type_id]) - .to include("cannot be changed to #{new_type.name} with these child item types.") - end - end - end + it 'allows type conversion when no children exist' do + supported = issue_without_children.work_item_type.supported_conversion_types( + reusable_project, + issue_without_children.author + ) - context 'when the parent restricts the type change' do - before do - child.work_item_type = new_type - end - - it 'does not allow to change types' do - expect(child.valid?).to eq(false) - expect(child.errors[:work_item_type_id]).to include( - format( - "cannot be changed to %{type_name} when linked to a parent %{parent_name}.", - type_name: new_type.name.downcase, - parent_name: parent.work_item_type.name.downcase - ) - ) - end + issue_without_children.work_item_type = supported.first + expect(issue_without_children).to be_valid end - end - context 'with hierarchy depth restriction' do - let_it_be_with_reload(:item1) { create(:work_item, work_item_type: new_type, project: reusable_project) } - let_it_be_with_reload(:item2) { create(:work_item, work_item_type: new_type, project: reusable_project) } - let_it_be_with_reload(:item3) { create(:work_item, work_item_type: new_type, project: reusable_project) } - let_it_be_with_reload(:item4) { create(:work_item, work_item_type: new_type, project: reusable_project) } + it 'prevents type conversion when it would break hierarchy rules' do + # Task with a parent shouldn't be able to convert + task_child.reload # Make sure it knows about its parent - let_it_be(:hierarchy_restriction1) do - create(:hierarchy_restriction, parent_type: old_type, child_type: new_type) - end - - let_it_be(:hierarchy_restriction2) do - create(:hierarchy_restriction, parent_type: new_type, child_type: old_type) - end + # Try to change task to issue (which would create Issue->Issue, not allowed) + issue_type = WorkItems::Type.find_by(base_type: :issue) + task_child.work_item_type = issue_type - let_it_be_with_reload(:hierarchy_restriction3) do - create(:hierarchy_restriction, parent_type: new_type, child_type: new_type, maximum_depth: 4) + expect(task_child).not_to be_valid + expect(task_child.errors[:work_item_type_id]).to contain_exactly( + "cannot be changed to issue when linked to a parent issue.") end + end + end + end - let_it_be(:link1) { create(:parent_link, work_item_parent: item1, work_item: item2) } - let_it_be(:link2) { create(:parent_link, work_item_parent: item2, work_item: item3) } - let_it_be(:link3) { create(:parent_link, work_item_parent: item3, work_item: item4) } - - before do - hierarchy_restriction3.update!(maximum_depth: maximum_depth) - end - - shared_examples 'validates the depth correctly' do - before do - work_item.update!(work_item_type: old_type) - end - - context 'when it is valid' do - let(:maximum_depth) { 4 } - - it 'allows to change types' do - work_item.work_item_type = new_type - - expect(work_item).to be_valid - end - end - - context 'when it is not valid' do - let(:maximum_depth) { 3 } - - it 'does not allow to change types' do - work_item.work_item_type = new_type - - expect(work_item).not_to be_valid - expect(work_item.errors[:work_item_type_id]).to include("reached maximum depth") - end - end - end + describe '#validate_child_restrictions' do + let_it_be(:project) { create(:project) } + let_it_be(:parent_work_item) { create(:work_item, :issue, project: project) } + let_it_be(:task_child) { create(:work_item, :task, project: project) } - context 'with the highest ancestor' do - let_it_be_with_reload(:work_item) { item1 } + context 'when there are no child links' do + it 'does not add any errors' do + parent_work_item.send(:validate_child_restrictions, parent_work_item.child_links) + expect(parent_work_item.errors[:work_item_type_id]).to be_empty + end + end - it_behaves_like 'validates the depth correctly' - end + context 'when there are child links' do + let_it_be(:parent_link) { create(:parent_link, work_item_parent: parent_work_item, work_item: task_child) } - context 'with a child item' do - let_it_be_with_reload(:work_item) { item2 } + context 'when restriction exists for the parent-child combination' do + it 'does not add errors' do + parent_work_item.send(:validate_child_restrictions, parent_work_item.child_links) - it_behaves_like 'validates the depth correctly' + expect(parent_work_item.errors[:work_item_type_id]).to be_empty end + end - context 'with the last child item' do - let_it_be_with_reload(:work_item) { item4 } - - it_behaves_like 'validates the depth correctly' + context 'when restriction does not exist for the parent-child combination' do + before do + parent_work_item.work_item_type_id = WorkItems::Type.find_by(base_type: :epic).id end - context 'when ancestor is still the old type' do - let_it_be(:hierarchy_restriction4) do - create(:hierarchy_restriction, parent_type: old_type, child_type: old_type) - end - - before do - item1.update!(work_item_type: old_type) - item2.update!(work_item_type: old_type) - end + it 'adds an error' do + parent_work_item.send(:validate_child_restrictions, parent_work_item.child_links) - context 'when it exceeds maximum depth' do - let(:maximum_depth) { 2 } - - it 'does not allow to change types' do - item2.work_item_type = new_type - - expect(item2).not_to be_valid - expect(item2.errors[:work_item_type_id]).to include("reached maximum depth") - end - end - - context 'when it does not exceed maximum depth' do - let(:maximum_depth) { 3 } - - it 'does allow to change types' do - item2.work_item_type = new_type - - expect(item2).to be_valid - end - end + expect(parent_work_item.errors[:work_item_type_id]) + .to include("cannot be changed to Epic with these child item types.") end end end @@ -1080,31 +988,12 @@ end end - context 'when there is a hierarchy restriction without maximum depth' do - before do - create(:hierarchy_restriction, - parent_type_id: work_item.work_item_type_id, - child_type_id: child_type.id, - maximum_depth: nil) - end - - it 'returns false' do - expect(work_item.max_depth_reached?(child_type)).to be false - end - end - context 'when there is a hierarchy restriction with maximum depth' do - let(:max_depth) { 3 } - - before do - create(:hierarchy_restriction, - parent_type_id: work_item.work_item_type_id, - child_type_id: child_type.id, - maximum_depth: max_depth) - end - context 'when work item type is the same as child type' do + # Epic can have Epic children with max depth 7 + let(:work_item) { create(:work_item, :epic, project: reusable_project) } let(:child_type) { work_item.work_item_type } + let(:max_depth) { 7 } # Epic->Epic has max depth 7 it 'returns true when depth is reached' do allow(work_item).to receive_message_chain(:same_type_base_and_ancestors, :count).and_return(max_depth) @@ -1118,6 +1007,11 @@ end context 'when work item type is different from child type' do + # Epic can have Issue children with max depth 1 + let(:work_item) { create(:work_item, :epic, project: reusable_project) } + let(:child_type) { WorkItems::Type.find_by(base_type: :issue) } + let(:max_depth) { 1 } # Epic->Issue has max depth 1 + it 'returns true when depth is reached' do allow(work_item).to receive_message_chain(:hierarchy, :base_and_ancestors, :count).and_return(max_depth) expect(work_item.max_depth_reached?(child_type)).to be true diff --git a/spec/models/work_items/parent_link_spec.rb b/spec/models/work_items/parent_link_spec.rb index 47ed9d673002a9..162efe9359a7b0 100644 --- a/spec/models/work_items/parent_link_spec.rb +++ b/spec/models/work_items/parent_link_spec.rb @@ -17,124 +17,70 @@ it { is_expected.to validate_presence_of(:work_item_parent) } it { is_expected.to validate_uniqueness_of(:work_item) } - describe 'hierarchy' do + describe 'hierarchy validations' do let_it_be(:issue) { create(:work_item, project: project) } - let_it_be(:incident) { build(:work_item, :incident, project: project) } let_it_be(:task1) { create(:work_item, :task, project: project) } let_it_be(:task2) { build(:work_item, :task, project: project) } - it 'is valid if issue parent has task child' do - expect(build(:parent_link, work_item: task1, work_item_parent: issue)).to be_valid - end - - it 'is valid if incident parent has task child' do - expect(build(:parent_link, work_item: task1, work_item_parent: incident)).to be_valid - end - - context 'when assigning to various parent types' do - using RSpec::Parameterized::TableSyntax - - where(:parent_type_sym, :child_type_sym, :is_valid) do - :issue | :task | true - :incident | :task | true - :task | :issue | false - :issue | :issue | false - :objective | :objective | true - :objective | :key_result | true - :key_result | :objective | false - :key_result | :key_result | false - :objective | :issue | false - :task | :objective | false - end + describe '#validate_hierarchy_restrictions' do + it 'prevents invalid parent-child type combinations' do + task = create(:work_item, :task, project: project) + issue = create(:work_item, :issue, project: project) + link = build(:parent_link, work_item_parent: task, work_item: issue) - with_them do - it 'validates if child can be added to the parent' do - parent_type = WorkItems::Type.default_by_type(parent_type_sym) - child_type = WorkItems::Type.default_by_type(child_type_sym) - parent = build(:work_item, work_item_type: parent_type, project: project) - child = build(:work_item, work_item_type: child_type, project: project) - link = build(:parent_link, work_item: child, work_item_parent: parent) - - expect(link.valid?).to eq(is_valid) - end + expect(link).not_to be_valid + expect(link.errors[:work_item]).to include("it's not allowed to add this type of parent item") end end - context 'with nested ancestors' do - let_it_be(:type1) { create(:work_item_type, :non_default) } - let_it_be(:type2) { create(:work_item_type, :non_default) } - let_it_be(:item1) { create(:work_item, work_item_type: type1, project: project) } - let_it_be(:item2) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:item3) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:item4) { create(:work_item, work_item_type: type2, project: project) } - let_it_be(:hierarchy_restriction1) { create(:hierarchy_restriction, parent_type: type1, child_type: type2) } - let_it_be(:hierarchy_restriction2) { create(:hierarchy_restriction, parent_type: type2, child_type: type1) } - - let_it_be(:hierarchy_restriction3) do - create(:hierarchy_restriction, parent_type: type2, child_type: type2, maximum_depth: 2) - end + describe '#validate_depth' do + it_behaves_like 'validates hierarchy depth', :epic, 7 + it_behaves_like 'validates hierarchy depth', :objective, 9 - let_it_be(:link1) { create(:parent_link, work_item_parent: item1, work_item: item2) } - let_it_be(:link2) { create(:parent_link, work_item_parent: item3, work_item: item4) } + context 'with cross-type hierarchies (objective to key_result)' do + let_it_be(:objective1) { create(:work_item, :objective, project: project) } + let_it_be(:key_result) { create(:work_item, :key_result, project: project) } + let_it_be(:objective3) { create(:work_item, :objective, project: project) } - describe '#validate_depth' do - it 'is valid if depth is in limit' do - link = build(:parent_link, work_item_parent: item1, work_item: item3) + it 'validates maximum depth of 1 for key_results under objectives' do + create(:parent_link, work_item_parent: objective1, work_item: key_result) - expect(link).to be_valid - end - - it 'is not valid when maximum depth is reached' do - link = build(:parent_link, work_item_parent: item2, work_item: item3) + key_result2 = create(:work_item, :key_result, project: project) + link = build(:parent_link, work_item_parent: key_result, work_item: key_result2) expect(link).not_to be_valid - expect(link.errors[:work_item]).to include('reached maximum depth') - end - end - - describe '#validate_cyclic_reference' do - it 'is not valid if parent and child are same' do - link1.work_item_parent = item2 - - expect(link1).not_to be_valid - expect(link1.errors[:work_item]).to include('is not allowed to point to itself') - end - - it 'is not valid if child is already in ancestors' do - link = build(:parent_link, work_item_parent: item4, work_item: item3) - - expect(link).not_to be_valid - expect(link.errors[:work_item]).to include("it's already present in this item's hierarchy") end end end - context 'when assigning parent from different project' do - let_it_be(:cross_project_issue) { create(:work_item, project: create(:project)) } + describe '#validate_cyclic_reference' do + let_it_be(:epic_a) { create(:work_item, :epic, project: project) } + let_it_be(:epic_b) { create(:work_item, :epic, project: project) } + let_it_be(:epic_c) { create(:work_item, :epic, project: project) } - let(:restriction) do - WorkItems::HierarchyRestriction - .find_by_parent_type_id_and_child_type_id(cross_project_issue.work_item_type_id, task1.work_item_type_id) + before do + # Create a chain: epic_a -> epic_b -> epic_c + create(:parent_link, work_item_parent: epic_a, work_item: epic_b) + create(:parent_link, work_item_parent: epic_b, work_item: epic_c) end - it 'is valid when cross-hierarchy is enabled' do - restriction.update!(cross_hierarchy_enabled: true) - link = build(:parent_link, work_item_parent: cross_project_issue, work_item: task1) + it 'is not valid if parent and child are same' do + link = build(:parent_link, work_item_parent: epic_a, work_item: epic_a) - expect(link).to be_valid - expect(link.errors).to be_empty + expect(link).not_to be_valid + expect(link.errors[:work_item]).to include('is not allowed to point to itself') end - it 'is not valid when cross-hierarchy is not enabled' do - restriction.update!(cross_hierarchy_enabled: false) - link = build(:parent_link, work_item_parent: cross_project_issue, work_item: task1) + it 'is not valid if child is already in ancestors' do + # epic_c is a descendant of epic_a, so epic_a cannot be a child of epic_c + link = build(:parent_link, work_item_parent: epic_c, work_item: epic_a) expect(link).not_to be_valid - expect(link.errors[:work_item_parent]).to include('parent must be in the same project or group as child.') + expect(link.errors[:work_item]).to include("it's already present in this item's hierarchy") end end - context 'when parent already has maximum number of links' do + describe '#validate_max_children' do let_it_be(:link1) { create(:parent_link, work_item_parent: issue, work_item: task1) } before do @@ -168,7 +114,7 @@ end end - context 'when parent is already linked' do + describe '#check_existing_related_link' do shared_examples 'invalid link' do |link_factory| let_it_be(:parent_link) { build(:parent_link, work_item_parent: issue, work_item: task1) } let(:error_msg) { 'cannot assign a linked work item as a parent' } @@ -228,7 +174,7 @@ it_behaves_like 'invalid link', :issue_link end - context 'when setting confidentiality' do + describe '#validate_confidentiality' do using RSpec::Parameterized::TableSyntax where(:confidential_parent, :confidential_child, :valid) do diff --git a/spec/models/work_items/system_defined/hierarchy_restriction_spec.rb b/spec/models/work_items/system_defined/hierarchy_restriction_spec.rb new file mode 100644 index 00000000000000..6d4f443e6e72cb --- /dev/null +++ b/spec/models/work_items/system_defined/hierarchy_restriction_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::SystemDefined::HierarchyRestriction, feature_category: :team_planning do + describe 'included modules' do + subject { described_class } + + it { is_expected.to include(ActiveRecord::FixedItemsModel::Model) } + end + + describe 'ITEMS configuration' do + it 'has the correct structure for each item' do + expect(described_class::ITEMS).to all( + include(:id, :parent_type_id, :child_type_id, :maximum_depth) + ) + end + + it 'defines unique IDs' do + ids = described_class::ITEMS.map { |item| item[:id] } # rubocop:disable Rails/Pluck -- Not an ActiveRecord object + expect(ids).to eq(ids.uniq) + end + + it 'defines unique hierarchy restrictions' do + parent_child_pairs = described_class::ITEMS.map do |item| + [item[:parent_type_id], item[:child_type_id]] + end + + expect(parent_child_pairs).to eq(parent_child_pairs.uniq) + end + + it 'references valid work item type IDs' do + valid_type_ids = [ + described_class::EPIC_ID, + described_class::ISSUE_ID, + described_class::TASK_ID, + described_class::OBJECTIVE_ID, + described_class::KEY_RESULT_ID, + described_class::INCIDENT_ID, + described_class::TICKET_ID + ] + + described_class::ITEMS.each do |item| + expect(valid_type_ids).to include(item[:parent_type_id]) + expect(valid_type_ids).to include(item[:child_type_id]) + end + end + + describe 'defined hierarchies' do + using RSpec::Parameterized::TableSyntax + + # This tests that the system defined data contains the expected restrictions + where(:parent_type_sym, :child_type_sym, :max_depth) do + :issue | :task | 1 + :incident | :task | 1 + :epic | :epic | 7 + :epic | :issue | 1 + :objective | :objective | 9 + :objective | :key_result | 1 + :ticket | :task | 1 + end + + with_them do + it 'correctly defines hierarchy restrictions' do + parent_id = described_class.const_get("#{parent_type_sym.upcase}_ID", false) + child_id = described_class.const_get("#{child_type_sym.upcase}_ID", false) + + expect(described_class.all).to include( + have_attributes( + parent_type_id: parent_id, + child_type_id: child_id, + maximum_depth: max_depth + ) + ) + end + end + end + end + + describe 'usage by ParentLink' do + it 'can be queried by parent and child type IDs' do + restriction = described_class.find_by( + parent_type_id: described_class::EPIC_ID, + child_type_id: described_class::ISSUE_ID + ) + + expect(restriction).not_to be_nil + expect(restriction.maximum_depth).to eq(1) + end + end +end diff --git a/spec/models/work_items/type_spec.rb b/spec/models/work_items/type_spec.rb index 2178aa86e8ddf7..ac65796f3ad9dc 100644 --- a/spec/models/work_items/type_spec.rb +++ b/spec/models/work_items/type_spec.rb @@ -22,63 +22,47 @@ expect(type.enabled_widget_definitions).to match_array([widget1]) end - it 'has many `child_restrictions`' do - is_expected.to have_many(:child_restrictions) - .class_name('WorkItems::HierarchyRestriction') - .with_foreign_key('parent_type_id') - end + describe '#allowed_child_types_by_name' do + it 'returns child types from hierarchy restrictions' do + epic_type = described_class.find_by(base_type: :epic) + issue_type = described_class.find_by(base_type: :issue) - it 'has many `parent_restrictions`' do - is_expected.to have_many(:parent_restrictions) - .class_name('WorkItems::HierarchyRestriction') - .with_foreign_key('child_type_id') - end + expect(epic_type.allowed_child_types_by_name).to include(issue_type) + end - describe 'allowed_child_types_by_name' do - it 'defines association' do - is_expected.to have_many(:allowed_child_types_by_name) - .through(:child_restrictions) - .class_name('::WorkItems::Type') - .with_foreign_key(:child_type_id) + it 'returns empty array when no child types are defined' do + custom_type = create(:work_item_type, :non_default) + + expect(custom_type.allowed_child_types_by_name).to be_empty end it 'sorts by name ascending' do - expected_type_names = %w[Atype Ztype gtype] - parent_type = create(:work_item_type, :non_default) - - expected_type_names.shuffle.each do |name| - create( - :hierarchy_restriction, - parent_type: parent_type, - child_type: create(:work_item_type, :non_default, name: name) - ) - end + issue_type = described_class.find_by(base_type: :issue) - expect(parent_type.allowed_child_types_by_name.pluck(:name)).to match_array(expected_type_names) + names = issue_type.allowed_child_types_by_name.pluck(:name) + expect(names).to eq(names.sort_by(&:downcase)) end end - describe 'allowed_parent_types_by_name' do - it 'defines association' do - is_expected.to have_many(:allowed_parent_types_by_name) - .through(:parent_restrictions) - .class_name('::WorkItems::Type') - .with_foreign_key(:parent_type_id) + describe '#allowed_parent_types_by_name' do + it 'returns parent types from hierarchy restrictions' do + task_type = described_class.find_by(base_type: :task) + issue_type = described_class.find_by(base_type: :issue) + + expect(task_type.allowed_parent_types_by_name).to include(issue_type) + end + + it 'returns empty array when no parent types are defined' do + custom_type = create(:work_item_type, :non_default) + + expect(custom_type.allowed_parent_types_by_name).to be_empty end it 'sorts by name ascending' do - expected_type_names = %w[Atype Ztype gtype] - child_type = create(:work_item_type, :non_default) - - expected_type_names.shuffle.each do |name| - create( - :hierarchy_restriction, - parent_type: create(:work_item_type, :non_default, name: name), - child_type: child_type - ) - end + task_type = described_class.find_by(base_type: :task) - expect(child_type.allowed_parent_types_by_name.pluck(:name)).to match_array(expected_type_names) + names = task_type.allowed_parent_types_by_name.pluck(:name) + expect(names).to eq(names.sort) end end end @@ -157,7 +141,7 @@ expect(Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter).not_to receive(:upsert_widgets) expect(Gitlab::DatabaseImporters::WorkItems::HierarchyRestrictionsImporter).not_to receive(:upsert_restrictions) - expect(subject).to eq(default_issue_type) + is_expected.to eq(default_issue_type) end context 'when default types are missing' do @@ -165,18 +149,20 @@ described_class.delete_all end + subject(:default_by_type) { described_class.default_by_type(base_type) } + it 'raises an error' do expect do - subject + default_by_type end.to raise_error( WorkItems::Type::DEFAULT_TYPES_NOT_SEEDED, <<~STRING - Default work item types have not been created yet. Make sure the DB has been seeded successfully. - See related documentation in - https://docs.gitlab.com/omnibus/settings/database.html#seed-the-database-fresh-installs-only + Default work item types have not been created yet. Make sure the DB has been seeded successfully. + See related documentation in + https://docs.gitlab.com/omnibus/settings/database.html#seed-the-database-fresh-installs-only - If you have additional questions, you can ask in - https://gitlab.com/gitlab-org/gitlab/-/issues/423483 + If you have additional questions, you can ask in + https://gitlab.com/gitlab-org/gitlab/-/issues/423483 STRING ) end @@ -188,7 +174,7 @@ it 'does not raise an error' do expect do - subject + default_by_type end.not_to raise_error end end @@ -263,23 +249,23 @@ end describe '#allowed_child_types' do - let_it_be(:work_item_type) { create(:work_item_type, :non_default) } - let_it_be(:child_type) { create(:work_item_type, :non_default) } - let_it_be(:restriction) { create(:hierarchy_restriction, parent_type: work_item_type, child_type: child_type) } + let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } + let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } - subject { work_item_type.allowed_child_types(cache: cached) } + subject { epic_type.allowed_child_types(cache: cached) } context 'when cache is true' do let(:cached) { true } before do - allow(work_item_type).to receive(:with_reactive_cache).and_call_original + allow(epic_type).to receive(:with_reactive_cache).and_call_original end it 'returns the cached data' do - expect(work_item_type).to receive(:with_reactive_cache) - expect(Rails.cache).to receive(:exist?).with("work_items_type:#{work_item_type.id}:alive") - is_expected.to eq([child_type]) + expect(epic_type).to receive(:with_reactive_cache) + expect(Rails.cache).to receive(:exist?).with("work_items_type:#{epic_type.id}:alive") + + is_expected.to include(issue_type) # Changed from expect(subject) end end @@ -287,30 +273,31 @@ let(:cached) { false } it 'returns queried data' do - expect(work_item_type).not_to receive(:with_reactive_cache) - is_expected.to eq([child_type]) + expect(epic_type).not_to receive(:with_reactive_cache) + + is_expected.to include(issue_type) # Changed from expect(subject) end end end describe '#allowed_parent_types' do - let_it_be(:work_item_type) { create(:work_item_type, :non_default) } - let_it_be(:parent_type) { create(:work_item_type, :non_default) } - let_it_be(:restriction) { create(:hierarchy_restriction, parent_type: parent_type, child_type: work_item_type) } + let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } + let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } - subject { work_item_type.allowed_parent_types(cache: cached) } + subject { issue_type.allowed_parent_types(cache: cached) } context 'when cache is true' do let(:cached) { true } before do - allow(work_item_type).to receive(:with_reactive_cache).and_call_original + allow(issue_type).to receive(:with_reactive_cache).and_call_original end it 'returns the cached data' do - expect(work_item_type).to receive(:with_reactive_cache) - expect(Rails.cache).to receive(:exist?).with("work_items_type:#{work_item_type.id}:alive") - is_expected.to eq([parent_type]) + expect(issue_type).to receive(:with_reactive_cache) + expect(Rails.cache).to receive(:exist?).with("work_items_type:#{issue_type.id}:alive") + + is_expected.to include(epic_type) end end @@ -318,44 +305,42 @@ let(:cached) { false } it 'returns queried data' do - expect(work_item_type).not_to receive(:with_reactive_cache) - is_expected.to eq([parent_type]) + expect(issue_type).not_to receive(:with_reactive_cache) + + is_expected.to include(epic_type) end end end describe '#descendant_types' do - let(:epic_type) { create(:work_item_type, :non_default) } - let(:issue_type) { create(:work_item_type, :non_default) } - let(:task_type) { create(:work_item_type, :non_default) } + let(:epic_type) { described_class.find_by(base_type: :epic) } + let(:issue_type) { described_class.find_by(base_type: :issue) } + let(:task_type) { described_class.find_by(base_type: :task) } - subject { epic_type.descendant_types } + subject(:descendant_types) { epic_type.descendant_types } - before do - create(:hierarchy_restriction, parent_type: epic_type, child_type: epic_type) - create(:hierarchy_restriction, parent_type: epic_type, child_type: issue_type) - create(:hierarchy_restriction, parent_type: issue_type, child_type: task_type) + it 'returns all possible descendant types' do + is_expected.to include(epic_type, issue_type, task_type) end - it 'returns all possible descendant types' do - is_expected.to contain_exactly(epic_type, issue_type, task_type) + it 'handles circular dependencies correctly' do + expect { descendant_types }.not_to raise_error end end describe '#calculate_reactive_cache' do - let(:work_item_type) { build(:work_item_type) } + let(:work_item_type) { described_class.find_by(base_type: :issue) } subject { work_item_type.calculate_reactive_cache } - it 'returns cache data for allowed child types' do - child_types = create_list(:work_item_type, 2) - parent_types = create_list(:work_item_type, 2) - cache_data = { allowed_child_types_by_name: child_types, allowed_parent_types_by_name: parent_types } - - expect(work_item_type).to receive(:allowed_child_types_by_name).and_return(child_types) - expect(work_item_type).to receive(:allowed_parent_types_by_name).and_return(parent_types) + it 'returns cache data for allowed child and parent types' do + child_types = work_item_type.allowed_child_types_by_name + parent_types = work_item_type.allowed_parent_types_by_name - is_expected.to eq(cache_data) + is_expected.to eq({ + allowed_child_types_by_name: child_types, + allowed_parent_types_by_name: parent_types + }) end end @@ -394,8 +379,8 @@ let(:work_item_type) { issue_type } it 'returns all supported types except itself' do - expect(subject).to include(incident_type, task_type, ticket_type) - expect(subject).not_to include(issue_type) + is_expected.to include(incident_type, task_type, ticket_type) + is_expected.not_to include(issue_type) end end @@ -403,8 +388,8 @@ let(:work_item_type) { incident_type } it 'returns all supported types except itself' do - expect(subject).to include(issue_type, task_type, ticket_type) - expect(subject).not_to include(incident_type) + is_expected.to include(issue_type, task_type, ticket_type) + is_expected.not_to include(incident_type) end end @@ -412,7 +397,7 @@ let(:work_item_type) { create(:work_item_type, :epic) } it 'does not include epic as it is excluded from supported conversion types' do - expect(subject).not_to include(work_item_type) + is_expected.not_to include(work_item_type) end end @@ -420,7 +405,7 @@ let(:work_item_type) { create(:work_item_type, :objective) } it 'returns empty array as objective is excluded from supported conversion types' do - expect(subject).not_to include(work_item_type) + is_expected.not_to include(work_item_type) end end @@ -432,7 +417,7 @@ .with(resource_parent, developer_user) .and_call_original - subject + work_item_type.supported_conversion_types(resource_parent, developer_user) end end end diff --git a/spec/support/shared_examples/requests/api/graphql/work_item_type_list_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/work_item_type_list_shared_examples.rb index 28ac13d8d1feb1..b33cae06f7d736 100644 --- a/spec/support/shared_examples/requests/api/graphql/work_item_type_list_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/graphql/work_item_type_list_shared_examples.rb @@ -50,7 +50,7 @@ # TODO: Followup to solve the extra queries - https://gitlab.com/gitlab-org/gitlab/-/issues/512617 expect do post_graphql(query, current_user: current_user) - end.to issue_same_number_of_queries_as(control).with_threshold(2) + end.to issue_same_number_of_queries_as(control).with_threshold(5) expect(graphql_errors).to be_blank end end diff --git a/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb b/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb index 547eb536b0f073..99c339cb5d6f90 100644 --- a/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb +++ b/spec/support/shared_examples/services/work_items/data_sync/cloneable_and_moveable_data_stared_examples.rb @@ -246,10 +246,13 @@ def wi_linked_items(work_item) [:group_level, { namespace: original_work_item.namespace }] end - child_item_type1 = WorkItems::HierarchyRestriction.where(parent_type: original_work_item.work_item_type).order( - id: :asc).first.child_type.base_type - child_item_type2 = WorkItems::HierarchyRestriction.where(parent_type: original_work_item.work_item_type).order( - id: :asc).last.child_type.base_type + child_item_type1 = ::WorkItems::Type.find(WorkItems::SystemDefined::HierarchyRestriction.where( + parent_type_id: original_work_item.work_item_type.id + ).min_by(&:id).child_type_id).base_type + + child_item_type2 = ::WorkItems::Type.find(WorkItems::SystemDefined::HierarchyRestriction.where( + parent_type_id: original_work_item.work_item_type.id + ).max_by(&:id).child_type_id).base_type child_item1 = create(:work_item, child_item_type1, *namespace_params) create(:parent_link, work_item: child_item1, work_item_parent: original_work_item) diff --git a/spec/support/shared_examples/work_items/hierarchy_depth_shared_examples.rb b/spec/support/shared_examples/work_items/hierarchy_depth_shared_examples.rb new file mode 100644 index 00000000000000..f9906211cb0fd7 --- /dev/null +++ b/spec/support/shared_examples/work_items/hierarchy_depth_shared_examples.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'validates hierarchy depth' do |work_item_type, max_depth| + describe "#{work_item_type} hierarchy" do + it "allows creating hierarchies up to maximum depth of #{max_depth}" do + work_items = (1..max_depth).map { create(:work_item, work_item_type, project: project) } + + (max_depth - 2).times do |i| + create(:parent_link, work_item_parent: work_items[i], work_item: work_items[i + 1]) + end + + link = build(:parent_link, work_item_parent: work_items[max_depth - 2], work_item: work_items[max_depth - 1]) + expect(link).to be_valid + end + + it "is not valid when maximum depth of #{max_depth} is exceeded" do + work_items = create_list(:work_item, max_depth + 1, work_item_type, project: project) + + (max_depth - 1).times do |i| + create(:parent_link, work_item_parent: work_items[i], work_item: work_items[i + 1]) + end + + link = build(:parent_link, work_item_parent: work_items[max_depth - 1], work_item: work_items[max_depth]) + + expect(link).not_to be_valid + expect(link.errors[:work_item]).to include('reached maximum depth') + end + end +end -- GitLab From 07fb17f4854ca55cf7769be46fec0f14ba573f86 Mon Sep 17 00:00:00 2001 From: Matt D'Angelo Date: Tue, 2 Sep 2025 12:59:55 +0930 Subject: [PATCH 2/2] Remove reactive caching --- .../widget_definitions/hierarchy_type.rb | 4 +- .../work_items/hierarchy_restriction.rb | 9 --- app/models/work_items/type.rb | 31 ++----- .../work_items/widgets/start_and_due_date.rb | 2 +- .../hierarchy_restrictions_importer.rb | 6 +- .../work_items/hierarchy_restriction_spec.rb | 20 ----- spec/models/work_items/type_spec.rb | 81 ++----------------- ...rk_item_hierarchy_restrictions_importer.rb | 17 ---- 8 files changed, 17 insertions(+), 153 deletions(-) diff --git a/app/graphql/types/work_items/widget_definitions/hierarchy_type.rb b/app/graphql/types/work_items/widget_definitions/hierarchy_type.rb index 3f6872856e8465..91e0b2d3de5586 100644 --- a/app/graphql/types/work_items/widget_definitions/hierarchy_type.rb +++ b/app/graphql/types/work_items/widget_definitions/hierarchy_type.rb @@ -24,11 +24,11 @@ class HierarchyType < BaseObject description: 'Allowed parent types for the work item type.' def allowed_child_types(parent:) - parent.allowed_child_types(cache: true, authorize: true, resource_parent: context[:resource_parent]) + parent.allowed_child_types(authorize: true, resource_parent: context[:resource_parent]) end def allowed_parent_types(parent:) - parent.allowed_parent_types(cache: true, authorize: true, resource_parent: context[:resource_parent]) + parent.allowed_parent_types(authorize: true, resource_parent: context[:resource_parent]) end end # rubocop:enable Graphql/AuthorizeTypes diff --git a/app/models/work_items/hierarchy_restriction.rb b/app/models/work_items/hierarchy_restriction.rb index f74f2f037b19d1..a253447a8db330 100644 --- a/app/models/work_items/hierarchy_restriction.rb +++ b/app/models/work_items/hierarchy_restriction.rb @@ -7,17 +7,8 @@ class HierarchyRestriction < ApplicationRecord belongs_to :parent_type, class_name: 'WorkItems::Type' belongs_to :child_type, class_name: 'WorkItems::Type' - after_destroy :clear_parent_type_cache! - after_save :clear_parent_type_cache! - validates :parent_type, presence: true validates :child_type, presence: true validates :child_type, uniqueness: { scope: :parent_type_id } - - private - - def clear_parent_type_cache! - parent_type.clear_reactive_cache! - end end end diff --git a/app/models/work_items/type.rb b/app/models/work_items/type.rb index 82821b5ef29767..b22bf84518b1b2 100644 --- a/app/models/work_items/type.rb +++ b/app/models/work_items/type.rb @@ -13,11 +13,6 @@ class Type < ApplicationRecord self.table_name = 'work_item_types' include CacheMarkdownField - include ReactiveCaching - - self.reactive_cache_work_type = :no_dependency - self.reactive_cache_refresh_interval = 10.minutes - self.reactive_cache_lifetime = 1.hour # type name is used in restrictions DB seeder to assure restrictions for # default types are pre-filled @@ -69,7 +64,6 @@ class Type < ApplicationRecord inverse_of: :work_item_type before_validation :strip_whitespace - after_save :clear_reactive_cache! # TODO: review validation rules # https://gitlab.com/gitlab-org/gitlab/-/issues/336919 @@ -151,13 +145,6 @@ def allowed_parent_types_by_name WorkItems::Type.where(id: parent_type_ids).order_by_name_asc end - def calculate_reactive_cache - { - allowed_child_types_by_name: allowed_child_types_by_name, - allowed_parent_types_by_name: allowed_parent_types_by_name - } - end - def supported_conversion_types(resource_parent, user) type_names = supported_conversion_base_types(resource_parent, user) - [base_type] WorkItems::Type.by_type(type_names).order_by_name_asc @@ -170,20 +157,16 @@ def unavailable_widgets_on_conversion(target_type, resource_parent) source_widgets.reject { |widget| target_widget_types.include?(widget.widget_type) } end - def allowed_child_types(cache: false, authorize: false, resource_parent: nil) - cached_data = cache ? with_reactive_cache { |query_data| query_data[:allowed_child_types_by_name] } : nil - - types = cached_data || allowed_child_types_by_name + def allowed_child_types(authorize: false, resource_parent: nil) + types = allowed_child_types_by_name return types unless authorize authorized_types(types, resource_parent, :child) end - def allowed_parent_types(cache: false, authorize: false, resource_parent: nil) - cached_data = cache ? with_reactive_cache { |query_data| query_data[:allowed_parent_types_by_name] } : nil - - types = cached_data || allowed_parent_types_by_name + def allowed_parent_types(authorize: false, resource_parent: nil) + types = allowed_parent_types_by_name return types unless authorize @@ -192,15 +175,13 @@ def allowed_parent_types(cache: false, authorize: false, resource_parent: nil) def descendant_types descendant_types = [] - next_level_child_types = allowed_child_types(cache: true) + next_level_child_types = allowed_child_types loop do descendant_types += next_level_child_types # We remove types that we've already seen to avoid circular dependencies - next_level_child_types = next_level_child_types.flat_map do |type| - type.allowed_child_types(cache: true) - end - descendant_types + next_level_child_types = next_level_child_types.flat_map(&:allowed_child_types) - descendant_types break if next_level_child_types.empty? end diff --git a/ee/app/models/ee/work_items/widgets/start_and_due_date.rb b/ee/app/models/ee/work_items/widgets/start_and_due_date.rb index eb0b71b3ee0f93..9e19cfcb23d625 100644 --- a/ee/app/models/ee/work_items/widgets/start_and_due_date.rb +++ b/ee/app/models/ee/work_items/widgets/start_and_due_date.rb @@ -16,7 +16,7 @@ def sync_params override :can_rollup? def can_rollup? - work_item&.work_item_type&.allowed_child_types(cache: true).present? + work_item&.work_item_type&.allowed_child_types.present? end strong_memoize_attr :can_rollup? diff --git a/lib/gitlab/database_importers/work_items/hierarchy_restrictions_importer.rb b/lib/gitlab/database_importers/work_items/hierarchy_restrictions_importer.rb index 2641009a5a8292..99cb39ed1cd135 100644 --- a/lib/gitlab/database_importers/work_items/hierarchy_restrictions_importer.rb +++ b/lib/gitlab/database_importers/work_items/hierarchy_restrictions_importer.rb @@ -66,10 +66,8 @@ def self.upsert_restrictions def self.find_or_create_type(name) type = ::WorkItems::Type.find_by_name(name) - if type - type.clear_reactive_cache! - return type - end + + return type if type Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter.upsert_types ::WorkItems::Type.find_by_name(name) diff --git a/spec/models/work_items/hierarchy_restriction_spec.rb b/spec/models/work_items/hierarchy_restriction_spec.rb index 890c007b6cd3ca..2c4d5d32fb8012 100644 --- a/spec/models/work_items/hierarchy_restriction_spec.rb +++ b/spec/models/work_items/hierarchy_restriction_spec.rb @@ -15,24 +15,4 @@ it { is_expected.to validate_presence_of(:child_type) } it { is_expected.to validate_uniqueness_of(:child_type).scoped_to(:parent_type_id) } end - - describe '#clear_parent_type_cache!' do - subject(:hierarchy_restriction) { build(:hierarchy_restriction) } - - context 'when a hierarchy restriction is saved' do - it 'calls #clear_reactive_cache! on parent type' do - expect(hierarchy_restriction.parent_type).to receive(:clear_reactive_cache!).once - - hierarchy_restriction.save! - end - end - - context 'when a hierarchy restriction is destroyed' do - it 'calls #clear_reactive_cache! on parent type' do - expect(hierarchy_restriction.parent_type).to receive(:clear_reactive_cache!).once - - hierarchy_restriction.destroy! - end - end - end end diff --git a/spec/models/work_items/type_spec.rb b/spec/models/work_items/type_spec.rb index ac65796f3ad9dc..826e12220dd242 100644 --- a/spec/models/work_items/type_spec.rb +++ b/spec/models/work_items/type_spec.rb @@ -67,17 +67,6 @@ end end - describe 'callbacks' do - describe 'after_save' do - subject(:work_item_type) { build(:work_item_type) } - - it 'calls #clear_reactive_cache!' do - is_expected.to receive(:clear_reactive_cache!) - work_item_type.save!(name: 'foo') - end - end - end - describe 'scopes' do describe 'order_by_name_asc' do subject { described_class.order_by_name_asc.pluck(:name) } @@ -252,31 +241,10 @@ let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } - subject { epic_type.allowed_child_types(cache: cached) } - - context 'when cache is true' do - let(:cached) { true } - - before do - allow(epic_type).to receive(:with_reactive_cache).and_call_original - end + subject { epic_type.allowed_child_types } - it 'returns the cached data' do - expect(epic_type).to receive(:with_reactive_cache) - expect(Rails.cache).to receive(:exist?).with("work_items_type:#{epic_type.id}:alive") - - is_expected.to include(issue_type) # Changed from expect(subject) - end - end - - context 'when cache is false' do - let(:cached) { false } - - it 'returns queried data' do - expect(epic_type).not_to receive(:with_reactive_cache) - - is_expected.to include(issue_type) # Changed from expect(subject) - end + it 'returns queried data' do + is_expected.to include(issue_type) # Changed from expect(subject) end end @@ -284,31 +252,10 @@ let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } - subject { issue_type.allowed_parent_types(cache: cached) } - - context 'when cache is true' do - let(:cached) { true } + subject { issue_type.allowed_parent_types } - before do - allow(issue_type).to receive(:with_reactive_cache).and_call_original - end - - it 'returns the cached data' do - expect(issue_type).to receive(:with_reactive_cache) - expect(Rails.cache).to receive(:exist?).with("work_items_type:#{issue_type.id}:alive") - - is_expected.to include(epic_type) - end - end - - context 'when cache is false' do - let(:cached) { false } - - it 'returns queried data' do - expect(issue_type).not_to receive(:with_reactive_cache) - - is_expected.to include(epic_type) - end + it 'returns queried data' do + is_expected.to include(epic_type) end end @@ -328,22 +275,6 @@ end end - describe '#calculate_reactive_cache' do - let(:work_item_type) { described_class.find_by(base_type: :issue) } - - subject { work_item_type.calculate_reactive_cache } - - it 'returns cache data for allowed child and parent types' do - child_types = work_item_type.allowed_child_types_by_name - parent_types = work_item_type.allowed_parent_types_by_name - - is_expected.to eq({ - allowed_child_types_by_name: child_types, - allowed_parent_types_by_name: parent_types - }) - end - end - describe '.allowed_group_level_types' do let_it_be(:group) { create(:group) } let_it_be(:non_ee_types) { described_class.base_types.keys.excluding('epic') } diff --git a/spec/support/shared_examples/work_item_hierarchy_restrictions_importer.rb b/spec/support/shared_examples/work_item_hierarchy_restrictions_importer.rb index 4f6b27a99c6e16..0545be7c741324 100644 --- a/spec/support/shared_examples/work_item_hierarchy_restrictions_importer.rb +++ b/spec/support/shared_examples/work_item_hierarchy_restrictions_importer.rb @@ -7,23 +7,12 @@ end end - shared_examples 'clears type reactive cache' do - specify do - expect_next_found_instances_of(WorkItems::Type, 7) do |instance| - expect(instance).to receive(:clear_reactive_cache!) - end - - subject - end - end - context 'when restrictions are missing' do before do WorkItems::HierarchyRestriction.delete_all end it_behaves_like 'adds restrictions' - it_behaves_like 'clears type reactive cache' end context 'when base types are missing' do @@ -52,8 +41,6 @@ change { restriction.maximum_depth }.from(depth + 1).to(depth) ) end - - it_behaves_like 'clears type reactive cache' end context 'when some restrictions are missing' do @@ -68,8 +55,6 @@ ) expect(WorkItems::HierarchyRestriction.count).to eq(7) end - - it_behaves_like 'clears type reactive cache' end context 'when restrictions contain attributes not present in the table' do @@ -85,7 +70,5 @@ subject end - - it_behaves_like 'clears type reactive cache' end end -- GitLab