diff --git a/.rubocop_todo/rspec/named_subject.yml b/.rubocop_todo/rspec/named_subject.yml index 018a0103eb77e950abb1fdaaa4e21a441d689201..50dbe86f27334d44efb4e4cf46bdca8041ac61fe 100644 --- a/.rubocop_todo/rspec/named_subject.yml +++ b/.rubocop_todo/rspec/named_subject.yml @@ -2464,6 +2464,7 @@ 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 b61b8c9914a5adfa1e820d3b7e3b046f91580b36..4eb796f7e1e374fa3ca8aaaeefe3c030cf8cb391 100644 --- a/app/graphql/resolvers/work_items/types_resolver.rb +++ b/app/graphql/resolvers/work_items/types_resolver.rb @@ -29,6 +29,13 @@ 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 2c3b5cc340bcc2b77cbe2ad075c054e863e1ecac..8ac9eeeaf634f8a9fa3e7414718c5970426bb654 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::SystemDefined::HierarchyRestriction.find_by( - parent_type_id: work_item_type_id, - child_type_id: child_type.id + restriction = ::WorkItems::HierarchyRestriction.find_by_parent_type_id_and_child_type_id( + work_item_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).distinct.pluck('issues.work_item_type_id') # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Limited number of work item types - restrictions = ::WorkItems::SystemDefined::HierarchyRestriction.where( + child_type_ids = child_links.joins(:work_item).select(self.class.arel_table[:work_item_type_id]).distinct + restrictions = ::WorkItems::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::SystemDefined::HierarchyRestriction.find_by( - parent_type_id: work_item_type_id, - child_type_id: work_item_type_id + restriction = ::WorkItems::HierarchyRestriction.find_by_parent_type_id_and_child_type_id( + work_item_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::SystemDefined::HierarchyRestriction.find_by(child_type_id: work_item_type_id).present? + ::WorkItems::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 cfca452aa96a92e4759fdc6cf1b474135df32cd2..a103144be725e5132cbb5849dd7c96ba784abd7a 100644 --- a/app/models/work_items/parent_link.rb +++ b/app/models/work_items/parent_link.rb @@ -74,10 +74,8 @@ def validate_confidentiality def validate_hierarchy_restrictions return unless work_item && work_item_parent - 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 - ) + 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) if restriction.nil? errors.add :work_item, _("it's not allowed to add this type of parent item") @@ -85,6 +83,14 @@ 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 deleted file mode 100644 index f46161bf047b6b6f6ca1b0988ec3392c0f99218c..0000000000000000000000000000000000000000 --- a/app/models/work_items/system_defined/hierarchy_restriction.rb +++ /dev/null @@ -1,66 +0,0 @@ -# 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 82821b5ef29767417c459bd1288c21229990c8ac..5a7584eaf0c4ee0d850d92c6003927bf9d8b9a5c 100644 --- a/app/models/work_items/type.rb +++ b/app/models/work_items/type.rb @@ -64,6 +64,16 @@ 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 @@ -136,21 +146,6 @@ 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 6587b34f12bcb27b1340c5c4eeaabc10409ea9dd..887fe628ba129d934a8469f56c30aa2385303c2c 100644 --- a/ee/spec/models/ee/work_items/parent_link_spec.rb +++ b/ee/spec/models/ee/work_items/parent_link_spec.rb @@ -20,23 +20,53 @@ 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) } + 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)) } - subject { described_class.new(work_item: issue, work_item_parent: epic) } + 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 - it 'is valid for child with no legacy epic' do - expect(subject).to be_valid + 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 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') + expect(subject).to be_invalid + expect(subject.errors.full_messages).to include('Work item already assigned to an epic') + end 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 7633d1c2695a205a73e3ad5dc9ca776c568517d9..1efec668f6eddfc5c34700a8562f3845a4e7653f 100644 --- a/ee/spec/services/ee/notes/quick_actions_service_spec.rb +++ b/ee/spec/services/ee/notes/quick_actions_service_spec.rb @@ -882,17 +882,8 @@ def execute(note, include_message: false) before do epic_type = WorkItems::Type.default_by_type(:epic) - 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 - ) - ) + WorkItems::HierarchyRestriction.where(parent_type: epic_type, + child_type: epic_type).update!(maximum_depth: 0) 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 e597f1e0afb08ec69f86cb9e4a54cd4b82e413a2..6dc254ad6ecb3da83ceb9c8becd9a6e857fc9fb0 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,17 +260,8 @@ before do epic_type = WorkItems::Type.default_by_type(:epic) - 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 - ) - ) + WorkItems::HierarchyRestriction.where(parent_type: epic_type, + child_type: epic_type).update!(maximum_depth: 0) end it 'rejects promoting an issue to an epic' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 44160335cd3f26118bd53f80b39deb34bc4bed06..4374915d313b82ae1e0bbc69e255b339b240bff9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -77144,6 +77144,9 @@ 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 1a38f66e51a723f7288b26dc68120c80cf7e2626..679400517cd8e5b7bc40245b732a5ee2ce260d3c 100644 --- a/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb +++ b/spec/lib/gitlab/work_items/work_item_hierarchy_spec.rb @@ -4,50 +4,46 @@ RSpec.describe Gitlab::WorkItems::WorkItemHierarchy, feature_category: :portfolio_management do let_it_be(:project) { create(:project) } - - 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_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(:options) { {} } describe '#base_and_ancestors' do - subject { described_class.new(::WorkItem.where(id: issue1.id), options: options) } + subject { described_class.new(::WorkItem.where(id: item3.id), options: options) } it 'includes the base and its ancestors' do relation = subject.base_and_ancestors - expect(relation).to match_array([issue1, epic3, epic2, epic1]) + expect(relation).to eq([item3, item2, item1]) 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 of the same type' do + it 'includes the base and its ancestors' do relation = subject.base_and_ancestors - expect(relation).to match_array([epic3, epic2, epic1]) + expect(relation).to eq([item3, item2]) end end it 'can find ancestors upto a certain level' do - relation = subject.base_and_ancestors(upto: epic2) + relation = subject.base_and_ancestors(upto: item1) - expect(relation).to match_array([issue1, epic3]) + expect(relation).to eq([item3, item2]) end describe 'hierarchy_order option' do @@ -59,7 +55,7 @@ let(:hierarchy_order) { :asc } it 'orders by child to ancestor' do - expect(relation).to match_array([issue1, epic3, epic2, epic1]) + expect(relation).to eq([item3, item2, item1]) end end @@ -67,28 +63,28 @@ let(:hierarchy_order) { :desc } it 'orders by ancestor to child' do - expect(relation).to match_array([epic1, epic2, epic3, issue1]) + expect(relation).to eq([item1, item2, item3]) end end end end describe '#base_and_descendants' do - subject { described_class.new(::WorkItem.where(id: epic2.id), options: options) } + subject { described_class.new(::WorkItem.where(id: item2.id), options: options) } it 'includes the base and its descendants' do relation = subject.base_and_descendants - expect(relation).to match_array([epic2, epic3, issue1, task1]) + expect(relation).to eq([item2, item3, item4]) end context 'when same_type option is used' do let(:options) { { same_type: true } } - it 'includes the base and its descendants of the same type' do + it 'includes the base and its ancestors' do relation = subject.base_and_descendants - expect(relation).to match_array([epic2, epic3]) + expect(relation).to eq([item2, item3]) end end @@ -99,10 +95,9 @@ it 'includes depth in the results' do object_depths = { - epic2.id => 1, - epic3.id => 2, - issue1.id => 3, - task1.id => 4 + item2.id => 1, + item3.id => 2, + item4.id => 3 } relation.each do |object| @@ -111,37 +106,4 @@ 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 703d9809b34c8d78ae5abf328260a95ec2d1a43e..5163b7c9eea66ae2b6bbc990e379b3b6bd8d2d77 100644 --- a/spec/models/work_item_spec.rb +++ b/spec/models/work_item_spec.rb @@ -670,61 +670,62 @@ end context 'with hierarchy' do - 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) } + 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) } describe '#ancestors' do it 'returns all ancestors in ascending order' do - expect(task1.ancestors).to eq([issue1, epic3, epic2, epic1]) + expect(item3_1.ancestors).to eq([item2_2, item2_1, item1]) end it 'returns an empty array if there are no ancestors' do - expect(epic1.ancestors).to be_empty + expect(item1.ancestors).to be_empty end end describe '#descendants' do it 'returns all descendants' do - expect(epic1.descendants).to match_array([epic2, epic3, issue1, issue2, task1, task2]) + expect(item1.descendants).to match_array([item2_1, item2_2, item3_1, item3_2, item4]) end end describe '#same_type_base_and_ancestors' do it 'returns self and all ancestors of the same type in ascending order' do - expect(epic3.same_type_base_and_ancestors).to eq([epic3, epic2, epic1]) + expect(item3_2.same_type_base_and_ancestors).to eq([item3_2, item3_1]) end it 'returns self if there are no ancestors of the same type' do - expect(issue1.same_type_base_and_ancestors).to match_array([issue1]) + expect(item3_1.same_type_base_and_ancestors).to match_array([item3_1]) end end describe '#same_type_descendants_depth' do it 'returns max descendants depth including self' do - # epic1 has epic2 and epic3 as same-type descendants, so depth is 3 - expect(epic1.same_type_descendants_depth).to eq(3) + expect(item3_1.same_type_descendants_depth).to eq(2) end it 'returns 1 if there are no descendants' do - expect(task1.same_type_descendants_depth).to eq(1) + expect(item1.same_type_descendants_depth).to eq(1) end end end @@ -745,74 +746,165 @@ end context 'with ParentLink relation' do - 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) } + let_it_be(:old_type) { create(:work_item_type, :non_default) } + let_it_be(:new_type) { create(:work_item_type, :non_default) } - before do - create(:parent_link, work_item_parent: issue_with_task, work_item: task_child) + 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) 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 - ) + 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 - issue_without_children.work_item_type = supported.first - expect(issue_without_children).to be_valid + 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 end + end - 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 + 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) } - # 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(:hierarchy_restriction1) do + create(:hierarchy_restriction, parent_type: old_type, child_type: new_type) + end - 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.") + let_it_be(:hierarchy_restriction2) do + create(:hierarchy_restriction, parent_type: new_type, child_type: old_type) end - 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) } + let_it_be_with_reload(:hierarchy_restriction3) do + create(:hierarchy_restriction, parent_type: new_type, child_type: new_type, maximum_depth: 4) + end - 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 + 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 } - 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) } + it 'allows to change types' do + work_item.work_item_type = new_type - 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) + expect(work_item).to be_valid + end + end - expect(parent_work_item.errors[:work_item_type_id]).to be_empty + 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 - end - 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 + context 'with the highest ancestor' do + let_it_be_with_reload(:work_item) { item1 } + + it_behaves_like 'validates the depth correctly' end - it 'adds an error' do - parent_work_item.send(:validate_child_restrictions, parent_work_item.child_links) + context 'with a child item' do + let_it_be_with_reload(:work_item) { item2 } - expect(parent_work_item.errors[:work_item_type_id]) - .to include("cannot be changed to Epic with these child item types.") + it_behaves_like 'validates the depth correctly' + end + + context 'with the last child item' do + let_it_be_with_reload(:work_item) { item4 } + + it_behaves_like 'validates the depth correctly' + 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 + + 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 end end end @@ -988,12 +1080,31 @@ 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) @@ -1007,11 +1118,6 @@ 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 162efe9359a7b0d1ec93cf32803c156393b82503..47ed9d673002a9ac0721aef6af5caadc83f508a6 100644 --- a/spec/models/work_items/parent_link_spec.rb +++ b/spec/models/work_items/parent_link_spec.rb @@ -17,70 +17,124 @@ it { is_expected.to validate_presence_of(:work_item_parent) } it { is_expected.to validate_uniqueness_of(:work_item) } - describe 'hierarchy validations' do + describe 'hierarchy' 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) } - 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) + 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 - expect(link).not_to be_valid - expect(link.errors[:work_item]).to include("it's not allowed to add this type of parent item") + 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 + + 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 end end - describe '#validate_depth' do - it_behaves_like 'validates hierarchy depth', :epic, 7 - it_behaves_like 'validates hierarchy depth', :objective, 9 + 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 - 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) } + 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) } - it 'validates maximum depth of 1 for key_results under objectives' do - create(:parent_link, work_item_parent: objective1, work_item: key_result) + describe '#validate_depth' do + it 'is valid if depth is in limit' do + link = build(:parent_link, work_item_parent: item1, 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).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) 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 - 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) } + context 'when assigning parent from different project' do + let_it_be(:cross_project_issue) { create(:work_item, project: create(:project)) } - 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) + 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) end - it 'is not valid if parent and child are same' do - link = build(:parent_link, work_item_parent: epic_a, work_item: epic_a) + 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) - expect(link).not_to be_valid - expect(link.errors[:work_item]).to include('is not allowed to point to itself') + expect(link).to be_valid + expect(link.errors).to be_empty end - 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) + 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) expect(link).not_to be_valid - expect(link.errors[:work_item]).to include("it's already present in this item's hierarchy") + expect(link.errors[:work_item_parent]).to include('parent must be in the same project or group as child.') end end - describe '#validate_max_children' do + context 'when parent already has maximum number of links' do let_it_be(:link1) { create(:parent_link, work_item_parent: issue, work_item: task1) } before do @@ -114,7 +168,7 @@ end end - describe '#check_existing_related_link' do + context 'when parent is already linked' 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' } @@ -174,7 +228,7 @@ it_behaves_like 'invalid link', :issue_link end - describe '#validate_confidentiality' do + context 'when setting 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 deleted file mode 100644 index 6d4f443e6e72cb1be58d13bae99a64ba99c46de0..0000000000000000000000000000000000000000 --- a/spec/models/work_items/system_defined/hierarchy_restriction_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -# 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 ac65796f3ad9dcb36654e90283284c2ce8790fd0..2178aa86e8ddf795b97130c91d0a6487b18a851d 100644 --- a/spec/models/work_items/type_spec.rb +++ b/spec/models/work_items/type_spec.rb @@ -22,47 +22,63 @@ expect(type.enabled_widget_definitions).to match_array([widget1]) 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) - - expect(epic_type.allowed_child_types_by_name).to include(issue_type) - 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 - it 'returns empty array when no child types are defined' do - custom_type = create(:work_item_type, :non_default) + 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(custom_type.allowed_child_types_by_name).to be_empty + 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) end it 'sorts by name ascending' do - issue_type = described_class.find_by(base_type: :issue) + 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 - names = issue_type.allowed_child_types_by_name.pluck(:name) - expect(names).to eq(names.sort_by(&:downcase)) + expect(parent_type.allowed_child_types_by_name.pluck(:name)).to match_array(expected_type_names) end end - 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 + 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) end it 'sorts by name ascending' do - task_type = described_class.find_by(base_type: :task) + 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 - names = task_type.allowed_parent_types_by_name.pluck(:name) - expect(names).to eq(names.sort) + expect(child_type.allowed_parent_types_by_name.pluck(:name)).to match_array(expected_type_names) end end end @@ -141,7 +157,7 @@ expect(Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter).not_to receive(:upsert_widgets) expect(Gitlab::DatabaseImporters::WorkItems::HierarchyRestrictionsImporter).not_to receive(:upsert_restrictions) - is_expected.to eq(default_issue_type) + expect(subject).to eq(default_issue_type) end context 'when default types are missing' do @@ -149,20 +165,18 @@ described_class.delete_all end - subject(:default_by_type) { described_class.default_by_type(base_type) } - it 'raises an error' do expect do - default_by_type + subject 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 @@ -174,7 +188,7 @@ it 'does not raise an error' do expect do - default_by_type + subject end.not_to raise_error end end @@ -249,23 +263,23 @@ end describe '#allowed_child_types' do - let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } - let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } + 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) } - subject { epic_type.allowed_child_types(cache: cached) } + subject { work_item_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 + allow(work_item_type).to receive(:with_reactive_cache).and_call_original end 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) + 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]) end end @@ -273,31 +287,30 @@ 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) + expect(work_item_type).not_to receive(:with_reactive_cache) + is_expected.to eq([child_type]) end end end describe '#allowed_parent_types' do - let_it_be(:issue_type) { described_class.find_by(base_type: :issue) } - let_it_be(:epic_type) { described_class.find_by(base_type: :epic) } + 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) } - subject { issue_type.allowed_parent_types(cache: cached) } + subject { work_item_type.allowed_parent_types(cache: cached) } context 'when cache is true' do let(:cached) { true } before do - allow(issue_type).to receive(:with_reactive_cache).and_call_original + allow(work_item_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) + 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]) end end @@ -305,42 +318,44 @@ let(:cached) { false } it 'returns queried data' do - expect(issue_type).not_to receive(:with_reactive_cache) - - is_expected.to include(epic_type) + expect(work_item_type).not_to receive(:with_reactive_cache) + is_expected.to eq([parent_type]) end end end describe '#descendant_types' do - 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) } + 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) } - subject(:descendant_types) { epic_type.descendant_types } + subject { epic_type.descendant_types } - it 'returns all possible descendant types' do - is_expected.to include(epic_type, issue_type, task_type) + 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) end - it 'handles circular dependencies correctly' do - expect { descendant_types }.not_to raise_error + it 'returns all possible descendant types' do + is_expected.to contain_exactly(epic_type, issue_type, task_type) end end describe '#calculate_reactive_cache' do - let(:work_item_type) { described_class.find_by(base_type: :issue) } + let(:work_item_type) { build(:work_item_type) } 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 + 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) - is_expected.to eq({ - allowed_child_types_by_name: child_types, - allowed_parent_types_by_name: parent_types - }) + is_expected.to eq(cache_data) end end @@ -379,8 +394,8 @@ let(:work_item_type) { issue_type } it 'returns all supported types except itself' do - is_expected.to include(incident_type, task_type, ticket_type) - is_expected.not_to include(issue_type) + expect(subject).to include(incident_type, task_type, ticket_type) + expect(subject).not_to include(issue_type) end end @@ -388,8 +403,8 @@ let(:work_item_type) { incident_type } it 'returns all supported types except itself' do - is_expected.to include(issue_type, task_type, ticket_type) - is_expected.not_to include(incident_type) + expect(subject).to include(issue_type, task_type, ticket_type) + expect(subject).not_to include(incident_type) end end @@ -397,7 +412,7 @@ let(:work_item_type) { create(:work_item_type, :epic) } it 'does not include epic as it is excluded from supported conversion types' do - is_expected.not_to include(work_item_type) + expect(subject).not_to include(work_item_type) end end @@ -405,7 +420,7 @@ let(:work_item_type) { create(:work_item_type, :objective) } it 'returns empty array as objective is excluded from supported conversion types' do - is_expected.not_to include(work_item_type) + expect(subject).not_to include(work_item_type) end end @@ -417,7 +432,7 @@ .with(resource_parent, developer_user) .and_call_original - work_item_type.supported_conversion_types(resource_parent, developer_user) + subject 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 b33cae06f7d736fb9b2e7cac0b4fd20fdc045361..28ac13d8d1feb1b32f51c06de90d3ad9b2a75536 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(5) + end.to issue_same_number_of_queries_as(control).with_threshold(2) 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 99c339cb5d6f909e8e193494a46c387e9a1c4740..547eb536b0f073c1898161c6fb36fa5caa9a3108 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,13 +246,10 @@ def wi_linked_items(work_item) [:group_level, { namespace: original_work_item.namespace }] end - 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_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_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 deleted file mode 100644 index f9906211cb0fd747994c137eae15b01f2bc74b17..0000000000000000000000000000000000000000 --- a/spec/support/shared_examples/work_items/hierarchy_depth_shared_examples.rb +++ /dev/null @@ -1,29 +0,0 @@ -# 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