diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index 770fa0e2940b630271f7b26e7644b79ed8aa8225..a06b147ab52b1d56731ef2198e98cb7ab6003fbb 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -2464,7 +2464,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 4eb796f7e1e374fa3ca8aaaeefe3c030cf8cb391..b61b8c9914a5adfa1e820d3b7e3b046f91580b36 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 8ac9eeeaf634f8a9fa3e7414718c5970426bb654..2c3b5cc340bcc2b77cbe2ad075c054e863e1ecac 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 a103144be725e5132cbb5849dd7c96ba784abd7a..cfca452aa96a92e4759fdc6cf1b474135df32cd2 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 0000000000000000000000000000000000000000..f46161bf047b6b6f6ca1b0988ec3392c0f99218c --- /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 5a7584eaf0c4ee0d850d92c6003927bf9d8b9a5c..82821b5ef29767417c459bd1288c21229990c8ac 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 887fe628ba129d934a8469f56c30aa2385303c2c..6587b34f12bcb27b1340c5c4eeaabc10409ea9dd 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 1efec668f6eddfc5c34700a8562f3845a4e7653f..7633d1c2695a205a73e3ad5dc9ca776c568517d9 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 6dc254ad6ecb3da83ceb9c8becd9a6e857fc9fb0..e597f1e0afb08ec69f86cb9e4a54cd4b82e413a2 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 69f630e377bba2bbd94282e3147053876ba3d82e..4ae3438e6817f5a116a3f8efd06e1c563470fd14 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -77057,9 +77057,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 679400517cd8e5b7bc40245b732a5ee2ce260d3c..1a38f66e51a723f7288b26dc68120c80cf7e2626 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 5163b7c9eea66ae2b6bbc990e379b3b6bd8d2d77..703d9809b34c8d78ae5abf328260a95ec2d1a43e 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 47ed9d673002a9ac0721aef6af5caadc83f508a6..162efe9359a7b0d1ec93cf32803c156393b82503 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 0000000000000000000000000000000000000000..6d4f443e6e72cb1be58d13bae99a64ba99c46de0 --- /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 2178aa86e8ddf795b97130c91d0a6487b18a851d..ac65796f3ad9dcb36654e90283284c2ce8790fd0 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 28ac13d8d1feb1b32f51c06de90d3ad9b2a75536..b33cae06f7d736fb9b2e7cac0b4fd20fdc045361 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 547eb536b0f073c1898161c6fb36fa5caa9a3108..99c339cb5d6f909e8e193494a46c387e9a1c4740 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 0000000000000000000000000000000000000000..f9906211cb0fd747994c137eae15b01f2bc74b17 --- /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