From 491928a3fd3bc1f0f1a726734f736b09e12cbae1 Mon Sep 17 00:00:00 2001 From: ulferts Date: Sat, 19 Mar 2022 23:26:32 +0100 Subject: [PATCH 1/8] replace typed_dag with closure_tree and CTE --- Gemfile | 5 +- Gemfile.lock | 4 + app/contracts/relations/base_contract.rb | 74 +--- app/contracts/work_packages/base_contract.rb | 30 +- .../work_packages/bulk_controller.rb | 2 +- .../work_packages/moves_controller.rb | 4 +- .../queries/relations/filters/type_filter.rb | 10 +- .../queries/relations/relation_query.rb | 5 - .../filter_on_directed_relations_mixin.rb | 6 +- .../filter_on_undirected_relations_mixin.rb | 13 +- .../work_packages/filter/parent_filter.rb | 18 +- .../work_packages/filter/relatable_filter.rb | 24 +- app/models/relation.rb | 165 +------ .../scopes/follows_non_manual_ancestors.rb | 12 +- app/models/relations/scopes/types.rb | 41 ++ app/models/relations/scopes/visible.rb | 21 +- app/models/version.rb | 2 +- app/models/work_package.rb | 117 +---- app/models/work_package/ancestors.rb | 19 +- app/models/work_package/journalized.rb | 11 +- app/models/work_package/parent.rb | 161 ------- app/models/work_package/typed_dag_defaults.rb | 75 ---- app/models/work_packages/derived_dates.rb | 3 +- app/models/work_packages/relations.rb | 101 +++++ .../work_packages/scopes/for_scheduling.rb | 96 ++-- .../scopes/include_derived_dates.rb | 15 +- .../scopes/left_join_self_and_descendants.rb | 42 +- app/models/work_packages/scopes/relatable.rb | 207 +++++++++ app/seeders/demo_data/work_package_seeder.rb | 6 +- .../copy/work_packages_dependent_service.rb | 8 +- app/services/work_packages/delete_service.rb | 6 +- .../work_packages/schedule_dependency.rb | 38 +- .../work_packages/update_ancestors_service.rb | 1 + app/services/work_packages/update_service.rb | 42 +- config/brakeman.ignore | 20 - config/initializers/typed_dag.rb | 62 --- db/migrate/20180105130053_rebuild_dag.rb | 5 +- .../20220319211253_add_parent_id_to_wp.rb | 238 ++++++++++ db/migrate/migration_utils/typed_dag.rb | 86 ++++ lib/api/v3/relations/relations_api.rb | 1 - .../work_packages/eager_loading/hierarchy.rb | 8 +- .../form_configurations/query_representer.rb | 2 +- lib/api/v3/work_packages/watchers_api.rb | 2 +- .../work_package_relations_api.rb | 1 - .../work_packages/work_package_representer.rb | 2 - modules/backlogs/app/models/impediment.rb | 26 +- .../backlogs/patches/update_service_patch.rb | 8 +- .../backlogs/patches/work_package_patch.rb | 16 +- .../work_packages/base_contract_spec.rb | 20 +- .../backlogs/spec/models/impediment_spec.rb | 11 +- .../impediments/create_services_spec.rb | 10 +- .../impediments/update_service_spec.rb | 16 +- .../xls_export/work_package/exporter/xls.rb | 2 +- .../exporter/xls_integration_spec.rb | 8 +- .../work_packages/base_contract_spec.rb | 52 +-- .../work_packages/update_contract_spec.rb | 3 +- .../work_packages/bulk_controller_spec.rb | 11 +- spec/factories/relation_factory.rb | 4 - .../types/form_configuration_query_spec.rb | 21 +- .../query_groups/relation_query_group_spec.rb | 30 +- .../details/relations/relations_spec.rb | 13 +- .../scheduling/scheduling_mode_spec.rb | 27 +- .../work_package_representer_spec.rb | 10 +- .../queries/relations/relation_query_spec.rb | 4 +- .../filter/blocked_filter_spec.rb | 2 +- .../filter/duplicated_filter_spec.rb | 5 +- .../filter/duplicates_filter_spec.rb | 5 +- .../filter/partof_filter_spec.rb | 2 +- .../filter/relates_filter_spec.rb | 2 +- .../filter/required_filter_spec.rb | 2 +- spec/models/query/results_spec.rb | 2 +- spec/models/relation_spec.rb | 129 +----- .../scopes/for_scheduling_spec.rb | 111 ++++- .../work_packages/scopes/relatable_spec.rb | 323 ++++++++++++++ .../api/v3/relations/relations_api_spec.rb | 54 +-- ...able_relation_candidates_resource_spec.rb} | 6 +- .../v3/work_packages/show_resource_spec.rb | 4 +- .../projects/copy_service_integration_spec.rb | 8 +- .../work_packages/delete_service_spec.rb | 2 +- .../set_schedule_service_spec.rb | 412 +++++++++--------- .../update_service_integration_spec.rb | 42 +- .../work_packages/update_service_spec.rb | 30 +- .../queries/filters/shared_filter_examples.rb | 16 +- spec_legacy/fixtures/relations.yml | 4 +- 84 files changed, 1806 insertions(+), 1458 deletions(-) create mode 100644 app/models/relations/scopes/types.rb delete mode 100644 app/models/work_package/parent.rb delete mode 100644 app/models/work_package/typed_dag_defaults.rb create mode 100644 app/models/work_packages/relations.rb create mode 100644 app/models/work_packages/scopes/relatable.rb delete mode 100644 config/initializers/typed_dag.rb create mode 100644 db/migrate/20220319211253_add_parent_id_to_wp.rb create mode 100644 db/migrate/migration_utils/typed_dag.rb create mode 100644 spec/models/work_packages/scopes/relatable_spec.rb rename spec/requests/api/v3/work_packages/{available_relation_candidates_spec.rb => available_relation_candidates_resource_spec.rb} (96%) diff --git a/Gemfile b/Gemfile index 9ad3774e190c..25fa93a8e8ba 100644 --- a/Gemfile +++ b/Gemfile @@ -55,8 +55,11 @@ gem 'friendly_id', '~> 5.4.0' gem 'acts_as_list', '~> 1.0.1' gem 'acts_as_tree', '~> 2.9.0' gem 'awesome_nested_set', '~> 3.5.0' +gem 'closure_tree', '~> 7.4.0' gem 'rubytree', '~> 1.0.0' -gem 'typed_dag', '~> 2.0.2' +# Only used in down migrations now. +# Is to be removed once the referencing migrations have been squashed. +gem 'typed_dag', '~> 2.0.2', require: false gem 'addressable', '~> 2.8.0' diff --git a/Gemfile.lock b/Gemfile.lock index 655663015dc0..c11078072b46 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -348,6 +348,9 @@ GEM cork nap open4 (~> 1.3) + closure_tree (7.4.0) + activerecord (>= 4.2.10) + with_advisory_lock (>= 4.0.0) coderay (1.1.3) colored2 (3.1.2) commonmarker (0.23.4) @@ -998,6 +1001,7 @@ DEPENDENCIES carrierwave_direct (~> 2.1.0) cells-erb (~> 0.1.0) cells-rails (~> 0.1.4) + closure_tree (~> 7.4.0) commonmarker (~> 0.23.0) compare-xml (~> 0.66) costs! diff --git a/app/contracts/relations/base_contract.rb b/app/contracts/relations/base_contract.rb index 9324780d697f..fbe2f3266f78 100644 --- a/app/contracts/relations/base_contract.rb +++ b/app/contracts/relations/base_contract.rb @@ -39,7 +39,7 @@ class BaseContract < ::ModelContract validate :manage_relations_permission? validate :validate_from_exists validate :validate_to_exists - validate :validate_only_one_follow_direction_between_hierarchies + validate :validate_nodes_relatable validate :validate_accepted_type def self.model @@ -63,22 +63,21 @@ def validate_to_exists errors.add :to, :error_not_found unless visible_work_packages.exists? model.to_id end - def validate_only_one_follow_direction_between_hierarchies - return unless [Relation::TYPE_HIERARCHY, Relation::TYPE_FOLLOWS].include? model.relation_type - - if follow_relations_in_opposite_direction.exists? + def validate_nodes_relatable + if (model.from_id_changed? || model.to_id_changed?) && + WorkPackage.relatable(model.from, model.relation_type).where(id: model.to).empty? errors.add :base, I18n.t(:'activerecord.errors.messages.circular_dependency') end end def validate_accepted_type - return if (Relation::TYPES.keys + [Relation::TYPE_HIERARCHY]).include?(model.relation_type) + return if (Relation::TYPES.keys + [Relation::TYPE_PARENT]).include?(model.relation_type) errors.add :relation_type, :inclusion end def manage_relations_permission? - if !manage_relations? + unless manage_relations? errors.add :base, :error_unauthorized end end @@ -90,66 +89,5 @@ def visible_work_packages def manage_relations? user.allowed_to? :manage_work_package_relations, model.from.project end - - # Go up to's hierarchy to the highest ancestor not shared with from. - # Fetch all endpoints of relations that are reachable by following at least one follows - # and zero or more hierarchy relations. - # We now need to check whether those endpoints include any that - # - # * are an ancestor of from - # * are a descendant of from - # * are from itself - # - # Siblings and sibling subtrees of ancestors are ok to have relations - def follow_relations_in_opposite_direction - to_set = hierarchy_or_follows_of - - follows_relations_to_ancestors(to_set) - .or(follows_relations_to_descendants(to_set)) - .or(follows_relations_to_from(to_set)) - end - - def hierarchy_or_follows_of - to_root_ancestor = tos_highest_ancestor_not_shared_by_from - - Relation - .hierarchy_or_follows - .where(from_id: to_root_ancestor) - .where('follows > 0') - end - - def tos_highest_ancestor_not_shared_by_from - # mysql does not support a limit inside a subquery. - # we thus join/subselect the query for ancestors of to not shared by from - # with itself and exclude all that have a hierarchy value smaller than hierarchy - 1 - unshared_ancestors = tos_ancestors_not_shared_by_from - - unshared_ancestors - .where.not(hierarchy: unshared_ancestors.select('hierarchy - 1')) - .select(:from_id) - end - - def tos_ancestors_not_shared_by_from - Relation - .hierarchy_or_reflexive - .where(to_id: model.to_id) - .where.not(from_id: Relation.hierarchy_or_reflexive - .where(to_id: model.from_id) - .select(:from_id)) - end - - def follows_relations_to_ancestors(to_set) - ancestors = Relation.hierarchy.where(to_id: model.from) - to_set.where(to_id: ancestors.select(:from_id)) - end - - def follows_relations_to_descendants(to_set) - descendants = Relation.hierarchy.where(from_id: model.from) - to_set.where(to_id: descendants.select(:to_id)) - end - - def follows_relations_to_from(to_set) - to_set.where(to_id: model.from_id) - end end end diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index ac0a98a58e1c..993ef02ea3b3 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -114,8 +114,8 @@ class BaseContract < ::ModelContract validate :validate_parent_exists validate :validate_parent_in_same_project - validate :validate_parent_not_subtask validate :validate_parent_not_self + validate :validate_parent_not_subtask validate :validate_status_exists validate :validate_status_transition @@ -245,8 +245,11 @@ def validate_parent_in_same_project # have to validate ourself as the parent relation is created after saving def validate_parent_not_subtask - if model.parent_id_changed? && model.parent && invalid_relations_with_new_hierarchy.exists? - errors.add :base, :cant_link_a_work_package_with_a_descendant + if model.parent_id_changed? && + model.parent_id && + errors.exclude?(:parent) && + WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty? + errors.add :parent, :cant_link_a_work_package_with_a_descendant end end @@ -357,25 +360,6 @@ def status_transition_exists? assignable_statuses.exists?(model.status_id) end - def invalid_relations_with_new_hierarchy - query = Relation.from_parent_to_self_and_descendants(model) - .or(Relation.from_self_and_descendants_to_ancestors(model)) - .direct - - # Ignore the immediate relation from the old parent to the model - # since that will still exist before saving. - old_parent_id = model.parent_id_was - - if old_parent_id.present? - query - .where.not(hierarchy: 1) - .where.not(from_id: old_parent_id) - .where.not(to_id: model.id) - else - query - end - end - def type_context_changed? model.project && !type_inexistent? && (model.type_id_changed? || model.project_id_changed?) end @@ -386,7 +370,7 @@ def type_inexistent? # Returns a scope of status the user is able to apply def new_statuses_allowed_from(status) - return Status.where('1=0') if status.nil? + return Status.none if status.nil? current_status = Status.where(id: status.id) diff --git a/app/controllers/work_packages/bulk_controller.rb b/app/controllers/work_packages/bulk_controller.rb index df7d46e58645..69cb591a4d80 100644 --- a/app/controllers/work_packages/bulk_controller.rb +++ b/app/controllers/work_packages/bulk_controller.rb @@ -114,7 +114,7 @@ def attributes_for_update permitted_params .update_work_package .tap { |attributes| attributes[:custom_field_values]&.reject! { |_k, v| v.blank? } } - .reject { |_k, v| v.blank? } + .compact_blank .transform_values { |v| v == 'none' ? '' : v } .to_h end diff --git a/app/controllers/work_packages/moves_controller.rb b/app/controllers/work_packages/moves_controller.rb index 1cd45a9422ea..ce3b342f85ef 100644 --- a/app/controllers/work_packages/moves_controller.rb +++ b/app/controllers/work_packages/moves_controller.rb @@ -96,7 +96,6 @@ def check_project_uniqueness end def prepare_for_work_package_move - @work_packages = @work_packages.includes(:ancestors) @copy = params.has_key? :copy @allowed_projects = WorkPackage.allowed_target_projects_on_move(current_user) @target_project = @allowed_projects.detect { |p| p.id.to_s == params[:new_project_id].to_s } if params[:new_project_id] @@ -104,8 +103,7 @@ def prepare_for_work_package_move @types = @target_project.types @available_versions = @target_project.assignable_versions @available_statuses = Workflow.available_statuses(@project) - @notes = params[:notes] - @notes ||= '' + @notes = params[:notes] || '' end def permitted_create_params diff --git a/app/models/queries/relations/filters/type_filter.rb b/app/models/queries/relations/filters/type_filter.rb index cd4af92ea971..256f667d05ea 100644 --- a/app/models/queries/relations/filters/type_filter.rb +++ b/app/models/queries/relations/filters/type_filter.rb @@ -45,13 +45,9 @@ def self.key end def where - Array(values).map do |value| - column = Relation.relation_column(value) - - operator_strategy.sql_for_field(['1'], - self.class.model.table_name, - column) - end.join(' OR ') + operator_strategy.sql_for_field(values.map { |value| Relation.canonical_type(value) }, + self.class.model.table_name, + :relation_type) end end end diff --git a/app/models/queries/relations/relation_query.rb b/app/models/queries/relations/relation_query.rb index 81c274a482bd..6e41340165de 100644 --- a/app/models/queries/relations/relation_query.rb +++ b/app/models/queries/relations/relation_query.rb @@ -33,11 +33,6 @@ def self.model Relation end - def default_scope - Relation - .direct - end - def results # Filters marked to already check visibility free us from the need # to check it here. diff --git a/app/models/queries/work_packages/filter/filter_on_directed_relations_mixin.rb b/app/models/queries/work_packages/filter/filter_on_directed_relations_mixin.rb index 2cf5395ac339..88e41acdef41 100644 --- a/app/models/queries/work_packages/filter/filter_on_directed_relations_mixin.rb +++ b/app/models/queries/work_packages/filter/filter_on_directed_relations_mixin.rb @@ -30,13 +30,9 @@ module Queries::WorkPackages::Filter::FilterOnDirectedRelationsMixin include ::Queries::WorkPackages::Filter::FilterForWpMixin def where - # The order in which we call the methods on `Relation` matters, as - # the `Relation`'s association `includes` is overwritten with the method `includes` - # otherwise. relations_subselect = Relation - .send(normalized_relation_type) - .direct .where(relation_filter) + .where(relation_type: normalized_relation_type) .select(relation_select) operator = if operator_class == Queries::Operators::Equals diff --git a/app/models/queries/work_packages/filter/filter_on_undirected_relations_mixin.rb b/app/models/queries/work_packages/filter/filter_on_undirected_relations_mixin.rb index 03da04ef81b8..c15a348bb270 100644 --- a/app/models/queries/work_packages/filter/filter_on_undirected_relations_mixin.rb +++ b/app/models/queries/work_packages/filter/filter_on_undirected_relations_mixin.rb @@ -54,18 +54,19 @@ def operator_and_junction end def relations_subselect_to_from - Relation - .direct - .send(relation_type) + relation_subselect .where(to_id: values) .select(:from_id) end def relations_subselect_from_to - Relation - .direct - .send(relation_type) + relation_subselect .where(from_id: values) .select(:to_id) end + + def relation_subselect + Relation + .where(relation_type: relation_type) + end end diff --git a/app/models/queries/work_packages/filter/parent_filter.rb b/app/models/queries/work_packages/filter/parent_filter.rb index 4fddac0f2770..a5e5bbc3401e 100644 --- a/app/models/queries/work_packages/filter/parent_filter.rb +++ b/app/models/queries/work_packages/filter/parent_filter.rb @@ -28,19 +28,17 @@ class Queries::WorkPackages::Filter::ParentFilter < Queries::WorkPackages::Filter::WorkPackageFilter - include ::Queries::WorkPackages::Filter::FilterOnDirectedRelationsMixin + include ::Queries::WorkPackages::Filter::FilterForWpMixin def relation_type - ::Relation::TYPE_HIERARCHY + # While this is not a relation (in the sense of it being stored in a different database table) we still + # want it to be used same as every other relation filter. + Relation::TYPE_PARENT end - private - - def relation_filter - { from_id: values } - end - - def relation_select - :to_id + def where + # The filter had been called parent before and it is stored in the database like that. + # The other association filters all have _id in their self.key. + operator_strategy.sql_for_field(no_templated_values, self.class.model.table_name, :parent_id) end end diff --git a/app/models/queries/work_packages/filter/relatable_filter.rb b/app/models/queries/work_packages/filter/relatable_filter.rb index 8f6ecb8fb48c..0e1ceed63b4c 100644 --- a/app/models/queries/work_packages/filter/relatable_filter.rb +++ b/app/models/queries/work_packages/filter/relatable_filter.rb @@ -47,33 +47,11 @@ def where end def scope - if operator == Relation::TYPE_RELATES - relateable_from_or_to - elsif operator != 'parent' && canonical_operator == operator - relateable_to - else - relateable_from - end + WorkPackage.relatable(WorkPackage.find_by(id: values.first), Relation.canonical_type(operator)) end private - def relateable_from_or_to - relateable_to.or(relateable_from) - end - - def relateable_from - WorkPackage.relateable_from(from) - end - - def relateable_to - WorkPackage.relateable_to(from) - end - - def from - WorkPackage.find(values.first) - end - def canonical_operator Relation.canonical_type(operator) end diff --git a/app/models/relation.rb b/app/models/relation.rb index f53562688b83..bc4ea4c51b23 100644 --- a/app/models/relation.rb +++ b/app/models/relation.rb @@ -27,30 +27,8 @@ #++ class Relation < ApplicationRecord - include VirtualAttribute - - include ::Scopes::Scoped - - scopes :follows_non_manual_ancestors, - :visible - - scope :of_work_package, - ->(work_package) { where('from_id = ? OR to_id = ?', work_package, work_package) } - - virtual_attribute :relation_type do - types = ((TYPES.keys + [TYPE_HIERARCHY]) & Relation.column_names).select do |name| - send(name).positive? - end - - case types.length - when 1 - types[0] - when 0 - nil - else - TYPE_MIXED - end - end + belongs_to :from, class_name: 'WorkPackage' + belongs_to :to, class_name: 'WorkPackage' TYPE_RELATES = 'relates'.freeze TYPE_DUPLICATES = 'duplicates'.freeze @@ -63,8 +41,10 @@ class Relation < ApplicationRecord TYPE_PARTOF = 'partof'.freeze TYPE_REQUIRES = 'requires'.freeze TYPE_REQUIRED = 'required'.freeze - TYPE_HIERARCHY = 'hierarchy'.freeze - TYPE_MIXED = 'mixed'.freeze + # The parent relation is maintained separately + # (in WorkPackage and WorkPackageHierarchy) and a relation cannot + # have the type 'parent' but this is abstracted to simplify the code. + TYPE_PARENT = 'parent'.freeze TYPES = { TYPE_RELATES => { @@ -110,94 +90,20 @@ class Relation < ApplicationRecord } }.freeze - validates_numericality_of :delay, allow_nil: true - - validate :validate_sanity_of_relation - - before_validation :reverse_if_needed - - before_save :set_type_column - - [TYPE_RELATES, - TYPE_DUPLICATES, - TYPE_BLOCKS, - TYPE_PRECEDES, - TYPE_FOLLOWS, - TYPE_INCLUDES, - TYPE_REQUIRES, - TYPE_HIERARCHY].each do |type| - define_method "#{type}=" do |value| - instance_variable_set(:"@relation_type_set", nil) - super(value) - end - end - - def self.relation_column(type) - if TYPES.key?(type) && TYPES[type][:reverse] - TYPES[type][:reverse] - elsif TYPES.key?(type) || type == TYPE_HIERARCHY - type - end - end - - def self.from_work_package_or_ancestors(work_package) - ancestor_or_self_ids = work_package - .ancestors_relations - .or(where(from_id: work_package.id)) - .select(:from_id) - - where(from_id: ancestor_or_self_ids) - end - - def self.from_parent_to_self_and_descendants(work_package) - from_work_package_or_ancestors(work_package.parent) - .where(to_id: work_package.self_and_descendants.select(:id)) - end - - def self.from_self_and_descendants_to_ancestors(work_package) - # using parent.self_and_ancestors to be able to cope with unpersisted parent - where(from_id: work_package.self_and_descendants.select(:id)) - .where(to_id: work_package.parent.self_and_ancestors.select(:id)) - end - - def self.hierarchy_or_follows - with_type_columns_0(_dag_options.type_columns - %i(hierarchy follows)) - .non_reflexive - end - - def self.hierarchy_or_reflexive - with_type_columns_0(_dag_options.type_columns - %i(hierarchy)) - end + include ::Scopes::Scoped - def self.non_hierarchy_of_work_package(work_package) - of_work_package(work_package) - .non_hierarchy - .direct - end + scopes :follows_non_manual_ancestors, + :types, + :visible - def self.to_root(work_package) - # MySQL does not support limit inside a subquery. - # As this is intended to be used inside a subquery, we have to avoid using limit - joins("LEFT OUTER JOIN relations r2 - ON relations.to_id = r2.to_id - AND relations.hierarchy < r2.hierarchy") - .where('r2.id IS NULL') - .where(to_id: work_package.id) - .hierarchy_or_reflexive - end + scope :of_work_package, + ->(work_package) { where(from: work_package).or(where(to: work_package)) } - def self.tree_of(work_package) - root_id = to_root(work_package) - .select(:from_id) + validates :delay, numericality: { allow_nil: true } - hierarchy - .where(from_id: root_id) - end + validates :to, uniqueness: { scope: :from } - def self.sibling_of(work_package) - hierarchy - .where(from_id: work_package.parent_id) - end + before_validation :reverse_if_needed def other_work_package(work_package) from_id == work_package.id ? to : from @@ -241,6 +147,12 @@ def delay self[:delay] end + TYPES.each_key do |type| + define_method "#{type}?" do + canonical_type == self.class.canonical_type(type) + end + end + def canonical_type self.class.canonical_type(relation_type) end @@ -256,37 +168,6 @@ def self.canonical_type(relation_type) private - def shared_hierarchy? - to_from = hierarchy_but_not_self(to: to, from: from) - from_to = hierarchy_but_not_self(to: from, from: to) - - to_from - .or(from_to) - .any? - end - - def validate_sanity_of_relation - return unless from && to - - errors.add :to_id, :invalid if from_id == to_id - errors.add :to_id, :not_same_project unless from.project_id == to.project_id || - Setting.cross_project_work_package_relations? - errors.add :base, :cant_link_a_work_package_with_a_descendant if shared_hierarchy? - end - - def set_type_column - if relation_type_changed? && relation_type_was - was_column = self.class.relation_column(relation_type_was) - write_attribute was_column, 0 - end - - return unless relation_type - - new_column = self.class.relation_column(relation_type) - - send("#{new_column}=", 1) if new_column - end - # Reverses the relation if needed so that it gets stored in the proper way def reverse_if_needed if TYPES.key?(relation_type) && TYPES[relation_type][:reverse] @@ -296,8 +177,4 @@ def reverse_if_needed self.relation_type = TYPES[relation_type][:reverse] end end - - def hierarchy_but_not_self(to:, from:) - Relation.hierarchy.where(to: to, from: from).where.not(id: id) - end end diff --git a/app/models/relations/scopes/follows_non_manual_ancestors.rb b/app/models/relations/scopes/follows_non_manual_ancestors.rb index 2ee375b2c1f3..3ae5fccf9e3a 100644 --- a/app/models/relations/scopes/follows_non_manual_ancestors.rb +++ b/app/models/relations/scopes/follows_non_manual_ancestors.rb @@ -34,11 +34,11 @@ module FollowsNonManualAncestors # Returns all follows relationships of work package ancestors or work package unless # the ancestor or a work package between the ancestor and self is manually scheduled. def follows_non_manual_ancestors(work_package) - ancestor_relations_non_manual = hierarchy_or_reflexive - .where(to_id: work_package.id) - .where.not(from_id: from_manual_ancestors(work_package).select(:from_id)) + ancestor_relations_non_manual = WorkPackageHierarchy + .where(descendant_id: work_package.id) + .where.not(ancestor_id: from_manual_ancestors(work_package).select(:ancestor_id)) - where(from_id: ancestor_relations_non_manual.select(:from_id)) + where(from_id: ancestor_relations_non_manual.select(:ancestor_id)) .follows end @@ -47,8 +47,8 @@ def follows_non_manual_ancestors(work_package) def from_manual_ancestors(work_package) manually_schedule_ancestors = work_package.ancestors.where(schedule_manually: true) - hierarchy_or_reflexive - .where(to_id: manually_schedule_ancestors.select(:id)) + WorkPackageHierarchy + .where(descendant_id: manually_schedule_ancestors.select(:id)) end end end diff --git a/app/models/relations/scopes/types.rb b/app/models/relations/scopes/types.rb new file mode 100644 index 000000000000..826f62d6b9e4 --- /dev/null +++ b/app/models/relations/scopes/types.rb @@ -0,0 +1,41 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. + +module Relations::Scopes + module Types + extend ActiveSupport::Concern + + class_methods do + ::Relation::TYPES.each do |type, definition| + next if definition[:reverse] || type == ::Relation::TYPE_INCLUDES + + define_method type do + where(relation_type: type) + end + end + end + end +end diff --git a/app/models/relations/scopes/visible.rb b/app/models/relations/scopes/visible.rb index b7874a47933c..a052dd02c782 100644 --- a/app/models/relations/scopes/visible.rb +++ b/app/models/relations/scopes/visible.rb @@ -32,29 +32,12 @@ module Visible class_methods do # Returns all relationships visible to the user. The relationships have to be: - # * Direct (not transitive and not reflexive) so they have to be user generated and not a relationship - # generated by the system. # * Start (from_id) on a work package visible to the user (view_work_packages in the work package's project) # * End (to_id) on a work package visible to the user (view_work_packages in the work package's project) # @param [User] user def visible(user = User.current) - # Initially, a where(from_id: WorkPackage.visible).where(to_id: WorkPackage.visible) - # was used. That approach lead to the partial index - # '(hierarchy + relates + duplicates + follows + blocks + includes + requires = 1) AND relations.hierarchy = 0' - # not being used, however. - direct - .joins(visible_join(user, :from_id, 'visible_from')) - .joins(visible_join(user, :to_id, 'visible_to')) - end - - def visible_join(user, relation_column, alias_name) - relations_table = Relation.arel_table - visible_table = Arel::Nodes::TableAlias.new(WorkPackage.visible(user).select(:id).arel, alias_name) - - relations_table - .join(visible_table) - .on(visible_table[:id].eq(relations_table[relation_column])) - .join_sources + where(from_id: WorkPackage.visible(user)) + .where(to_id: WorkPackage.visible(user)) end end end diff --git a/app/models/version.rb b/app/models/version.rb index b2de329603aa..c40c9cf5da85 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -75,7 +75,7 @@ def due_date # Returns the total estimated time for this version # (sum of leaves estimated_hours) def estimated_hours - @estimated_hours ||= work_packages.hierarchy_leaves.sum(:estimated_hours).to_f + @estimated_hours ||= work_packages.leaves.sum(:estimated_hours).to_f end # Returns the total reported time for this version diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 8da46d1c2865..cbce24a982bc 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -33,13 +33,12 @@ class WorkPackage < ApplicationRecord include WorkPackage::AskBeforeDestruction include WorkPackage::TimeEntriesCleaner include WorkPackage::Ancestors - prepend WorkPackage::Parent - include WorkPackage::TypedDagDefaults include WorkPackage::CustomActioned include WorkPackage::Hooks include WorkPackages::DerivedDates include WorkPackages::SpentTime include WorkPackages::Costs + include WorkPackages::Relations include ::Scopes::Scoped include OpenProject::Journal::AttachmentHelper @@ -124,7 +123,8 @@ class WorkPackage < ApplicationRecord scopes :for_scheduling, :include_derived_dates, :include_spent_time, - :left_join_self_and_descendants + :left_join_self_and_descendants, + :relatable acts_as_watchable @@ -154,6 +154,8 @@ class WorkPackage < ApplicationRecord # sort by id so that limited eager loading doesn't break with postgresql order_column: "#{table_name}.id" + has_closure_tree + ##################### WARNING ##################### # Do not change the order of acts_as_attachable # # and acts_as_journalized! # @@ -205,10 +207,18 @@ def activity_type end # RELATIONS + def blockers + # return work_packages that block me + return WorkPackage.none if closed? + + WorkPackage + .where(id: Relation.blocks.where(to_id: self)) + .with_status_open + end + # Returns true if this work package is blocked by another work package that is still open def blocked? - blocked_by - .with_status_open + blockers .exists? end @@ -217,38 +227,14 @@ def relations end def visible_relations(user) - # This duplicates chaining - # .relations.visible - # The duplication is made necessary to achieve a performant sql query on MySQL. - # Chaining would result in - # WHERE (relations.from_id = [ID] OR relations.to_id = [ID]) - # AND relations.from_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES]) - # AND relations.to_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES]) - # This performs OK on postgresql but is very slow on MySQL - # The SQL generated by this method: - # WHERE (relations.from_id = [ID] AND relations.to_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES]) - # OR (relations.to_id = [ID] AND relations.from_id IN (SELECT [IDs OF VISIBLE WORK_PACKAGES])) - # is arguably easier to read and performs equally good on both DBs. - relations_from = Relation - .where(from: self) - .where(to: WorkPackage.visible(user)) - - relations_to = Relation - .where(to: self) - .where(from: WorkPackage.visible(user)) - - relations_from - .or(relations_to) + relations + .visible(user) end def relation(id) Relation.of_work_package(self).find(id) end - def new_relation - relations_to.build - end - def add_time_entry(attributes = {}) attributes.reverse_merge!( project: project, @@ -455,68 +441,13 @@ def self.by_subproject(project) ).to_a end - def self.relateable_to(wp) - # can't relate to itself and not to a descendant (see relations) - relateable_shared(wp) - .not_having_relations_from(wp) # can't relate to wp that relates to us (direct or transitively) - .not_having_direct_relation_to(wp) # can't relate to wp we relate to directly - end - - def self.relateable_from(wp) - # can't relate to itself and not to a descendant (see relations) - relateable_shared(wp) - .not_having_relations_to(wp) # can't relate to wp that relates to us (direct or transitively) - .not_having_direct_relation_from(wp) # can't relate to wp we relate to directly - end - - def self.relateable_shared(wp) - visible - .not_self(wp) # can't relate to itself - .not_being_descendant_of(wp) # can't relate to a descendant (see relations) - .satisfying_cross_project_setting(wp) - end - private_class_method :relateable_shared - - def self.satisfying_cross_project_setting(wp) - if Setting.cross_project_work_package_relations? - all - else - where(project_id: wp.project_id) - end - end - - def self.not_self(wp) - where.not(id: wp.id) - end - - def self.not_having_direct_relation_to(wp) - where.not(id: wp.relations_to.direct.select(:to_id)) - end - - def self.not_having_direct_relation_from(wp) - where.not(id: wp.relations_from.direct.select(:from_id)) - end - - def self.not_having_relations_from(wp) - where.not(id: wp.relations_from.select(:from_id)) - end - - def self.not_having_relations_to(wp) - where.not(id: wp.relations_to.select(:to_id)) - end - - def self.not_being_descendant_of(wp) - where.not(id: wp.descendants.select(:to_id)) - end - def self.order_by_ancestors(direction) - max_relation_depth = Relation - .hierarchy - .group(:to_id) - .select(:to_id, - "MAX(hierarchy) AS depth") + max_relation_depth = WorkPackageHierarchy + .group(:descendant_id) + .select(:descendant_id, + "MAX(generations) AS depth") - joins("LEFT OUTER JOIN (#{max_relation_depth.to_sql}) AS max_depth ON max_depth.to_id = work_packages.id") + joins("LEFT OUTER JOIN (#{max_relation_depth.to_sql}) AS max_depth ON max_depth.descendant_id = work_packages.id") .reorder(Arel.sql("COALESCE(max_depth.depth, 0) #{direction}")) .select("#{table_name}.*, COALESCE(max_depth.depth, 0)") end @@ -596,11 +527,11 @@ def default_assign end end - # Closes duplicates if the issue is being closed + # Closes duplicates if the work_package is being closed def close_duplicates return unless closing? - duplicates.each do |duplicate| + duplicated_relations.includes(:from).map(&:from).each do |duplicate| # Reload is needed in case the duplicate was updated by a previous duplicate duplicate.reload # Don't re-close it if it's already closed diff --git a/app/models/work_package/ancestors.rb b/app/models/work_package/ancestors.rb index 202fd5d46d5e..7e9ab9d97577 100644 --- a/app/models/work_package/ancestors.rb +++ b/app/models/work_package/ancestors.rb @@ -66,21 +66,22 @@ def results hash[id] = [] end - results = with_work_package_ancestors - .map { |wp| [wp.id, wp.ancestors] } - .to_h + results = ancestors_by_work_package default.merge(results) end private - def with_work_package_ancestors - WorkPackage - .where(id: @ids) - .includes(:ancestors) - .where(ancestors_work_packages: { project_id: Project.allowed_to(user, :view_work_packages) }) - .order(Arel.sql('relations.hierarchy DESC')) + def ancestors_by_work_package + WorkPackageHierarchy + .where(descendant_id: @ids) + .includes(:ancestor) + .where(ancestor: { project_id: Project.allowed_to(user, :view_work_packages) }) + .where('generations > 0') + .order(generations: :desc) + .group_by(&:descendant_id) + .transform_values { |hierarchies| hierarchies.map(&:ancestor) } end end end diff --git a/app/models/work_package/journalized.rb b/app/models/work_package/journalized.rb index c912635381b2..21b82cb31528 100644 --- a/app/models/work_package/journalized.rb +++ b/app/models/work_package/journalized.rb @@ -30,16 +30,7 @@ module WorkPackage::Journalized extend ActiveSupport::Concern included do - acts_as_journalized data_sql: ->(journable) do - <<~SQL - LEFT OUTER JOIN - ( - #{Relation.hierarchy.direct.where(to_id: journable.id).limit(1).select('from_id parent_id, to_id').to_sql} - ) parent_relation - ON - #{journable.class.table_name}.id = parent_relation.to_id - SQL - end + acts_as_journalized # This one is here only to ease reading module JournalizedProcs diff --git a/app/models/work_package/parent.rb b/app/models/work_package/parent.rb deleted file mode 100644 index e2a16ce53d5f..000000000000 --- a/app/models/work_package/parent.rb +++ /dev/null @@ -1,161 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2022 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module WorkPackage::Parent - def self.prepended(base) - base.after_save :update_parent_relation, if: :saved_change_to_parent_id? - base.include VirtualAttribute - - base.virtual_attribute 'parent_id', cast_type: :integer - - base.define_attribute_method 'parent' - - base.scope :with_parent, ->(*args) do - opts = Hash(args.first) - # noinspection RubySimplifyBooleanInspection - neg = opts[:present] == false ? "NOT" : "" - rel = Relation.table_name - wp = WorkPackage.table_name - - query = "#{neg} EXISTS (SELECT 1 FROM #{rel} WHERE #{rel}.to_id = #{wp}.id AND #{rel}.hierarchy > 0" - - if opts[:in].respond_to? :arel - subset = opts[:in].arel # .select() (or project()) will only add columns - subset.projections = [WorkPackage.arel_table[:id]] # but we only need the ID, so we reset the projections - - query += " AND relations.from_id IN (#{subset.to_sql})" - end - - query += " LIMIT 1)" - - where(query) - end - - base.scope :without_parent, ->(*args) do - with_parent Hash(args.first).merge(present: false) - end - - base.scope :with_children, ->(*args) do - opts = Hash(args.first) - # noinspection RubySimplifyBooleanInspection - neg = opts[:present] == false ? "NOT" : "" - rel = Relation.table_name - wp = WorkPackage.table_name - - query = "#{neg} EXISTS (SELECT 1 FROM #{rel} WHERE #{rel}.from_id = #{wp}.id AND #{rel}.hierarchy > 0" - - if opts[:in].respond_to? :arel - subset = opts[:in].arel # .select() (or project()) will only add columns - subset.projections = [WorkPackage.arel_table[:id]] # but we only need the ID, so we reset the projections - - query += " AND relations.to_id IN (#{subset.to_sql})" - end - - query += " LIMIT 1)" - - where(query) - end - - base.scope :without_children, ->(*args) do - with_children Hash(args.first).merge(present: false) - end - end - - attr_accessor :parent_object, - :do_halt - - def parent=(work_package) - id = work_package&.id - - self.parent_id = id - - @parent_object = work_package - end - - def parent - if @parent_id_set - @parent_object || parent_from_id - else - @parent_object || parent_from_relation || parent_from_id - end - end - - def has_parent? - !parent_relation.nil? - end - - def reload(*args) - @parent_object = nil - # The is_leaf resetting stems from typed_dag_defaults.rb where it was impossible to add - # another #reload method without interfering with the virtual attribute handling defined here. - reset_is_leaf - - super - end - - def parent_id=(id) - id = id.to_i > 0 ? id.to_i : nil - - super(id) - @parent_object = nil if @parent_object && @parent_object.id != id - - @parent_id - end - - def parent_id - return @parent_id if @parent_id_set - - @parent_id || parent&.id - end - - def update_parent_relation - parent_relation&.destroy - - if parent_object - create_parent_relation from: parent_object - elsif @parent_id - create_parent_relation from_id: @parent_id - end - end - - private - - def parent_from_relation - if parent_relation && ((@parent_id && parent_relation.from.id == @parent_id) || !@parent_id) - set_virtual_attribute_was('parent_id', parent_relation.from_id) - @parent_object = parent_relation.from - end - end - - def parent_from_id - if @parent_id - set_virtual_attribute_was('parent_id', @parent_id) - @parent_object = WorkPackage.find(@parent_id) - end - end -end diff --git a/app/models/work_package/typed_dag_defaults.rb b/app/models/work_package/typed_dag_defaults.rb deleted file mode 100644 index adc6164ab7cb..000000000000 --- a/app/models/work_package/typed_dag_defaults.rb +++ /dev/null @@ -1,75 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2022 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -# Provides aliases to hierarchy_* -# methods to stay compatible with code written for awesome_nested_set - -module WorkPackage::TypedDagDefaults - extend ActiveSupport::Concern - - included do - # Can't use .alias here - # as the dag methods are mixed in later - - def leaves - hierarchy_leaves - end - - def self.leaves - hierarchy_leaves - end - - def leaf? - # The leaf? implementation relies on the children relations. If that relation is not loaded, - # rails will attempt to do the performant check on whether such a relation exists at all. While - # This is performant for one call, subsequent calls have to again fetch from the db (cached admittedly) - # as the relations are still not loaded. - # For reasons I could not find out, adding a #reload method here lead to the virtual attribute management for parent - # to no longer work. Resetting the @is_leaf method was hence moved to the WorkPackage::Parent module - @is_leaf ||= hierarchy_leaf? - end - - def root - hierarchy_roots.first - end - - def self.roots - hierarchy_roots - end - - def root? - hierarchy_root? - end - - private - - def reset_is_leaf - @is_leaf = nil - end - end -end diff --git a/app/models/work_packages/derived_dates.rb b/app/models/work_packages/derived_dates.rb index 3074c5d3f93f..51725062a70b 100644 --- a/app/models/work_packages/derived_dates.rb +++ b/app/models/work_packages/derived_dates.rb @@ -79,8 +79,7 @@ def compute_derived_dates values = if persisted? WorkPackage .from(WorkPackage.include_derived_dates.where(id: self)) - .pluck(*attributes.each { |a| Arel.sql(a) }) - .first || [] + .pick(*attributes.each { |a| Arel.sql(a) }) || [] else [] end diff --git a/app/models/work_packages/relations.rb b/app/models/work_packages/relations.rb new file mode 100644 index 000000000000..f3df1fd05b42 --- /dev/null +++ b/app/models/work_packages/relations.rb @@ -0,0 +1,101 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. + +module WorkPackages::Relations + extend ActiveSupport::Concern + + included do + # Relations pointing to another work package. + # In this case, + # * from is self + # * to is the other work package involved + has_many :relations_to, + class_name: 'Relation', + foreign_key: :from_id, + autosave: true, + dependent: :nullify, + inverse_of: :from + + # Relations pointing away from the work package. + # In this case, + # * to is self + # * from is the other work package involved + has_many :relations_from, + class_name: 'Relation', + foreign_key: :to_id, + autosave: true, + dependent: :nullify, + inverse_of: :to + + # Relations where the current work package follows another one. + # In this case, + # * from is self.id + # * to is the followed work package + has_many :follows_relations, + -> { where(relation_type: Relation::TYPE_FOLLOWS) }, + class_name: 'Relation', + foreign_key: :from_id, + autosave: true, + dependent: :nullify, + inverse_of: :from + + # Relations where the current work package blocks another one. + # In this case, + # * from is self.id + # * to is the blocked work package + has_many :blocks_relations, + -> { where(relation_type: Relation::TYPE_BLOCKS) }, + class_name: 'Relation', + foreign_key: :from_id, + autosave: true, + dependent: :nullify, + inverse_of: :from + + # Relations where the current work package duplicates another one. + # In this case, + # * from is self.id + # * to is the duplicated work package + has_many :duplicates_relations, + -> { where(relation_type: Relation::TYPE_DUPLICATES) }, + class_name: 'Relation', + foreign_key: :from_id, + autosave: true, + dependent: :nullify, + inverse_of: :from + + # Relations where the current work package is duplicated by another one. + # In this case, + # * from is the duplicate work package + # * to is self + has_many :duplicated_relations, + -> { where(relation_type: Relation::TYPE_DUPLICATES) }, + class_name: 'Relation', + foreign_key: :to_id, + autosave: true, + dependent: :nullify, + inverse_of: :to + end +end diff --git a/app/models/work_packages/scopes/for_scheduling.rb b/app/models/work_packages/scopes/for_scheduling.rb index 8d801f6869a1..35dd8c3e0c45 100644 --- a/app/models/work_packages/scopes/for_scheduling.rb +++ b/app/models/work_packages/scopes/for_scheduling.rb @@ -36,20 +36,21 @@ module ForScheduling # and hierarchy) work package is modified or created. # # The SQL relies on a recursive CTE which will fetch all work packages that are connected to the rescheduled work package - # via relations (follows/precedes and/or hierarchy) either directly or transitively. It will do so by increasing the relation path - # length one at a time and will stop on that path if the work package evaluated to be added is either: + # via relations (follows/precedes and/or hierarchy) either directly or transitively. It will do so by increasing the + # relation path length one at a time and will stop on that path if the work package evaluated to be added is either: # * itself scheduled manually # * having all of it's children scheduled manually # - # The children themselves are scheduled manually if all of their children are scheduled manually which repeats itself down to the leaf - # work packages. So another way of putting it, and that is how the sql statement works, is that a work package is considered to - # be scheduled manually if *all* of the paths to their leafs have at least one work package that is scheduled manually. + # The children themselves are scheduled manually if all of their children are scheduled manually which repeats itself down + # to the leaf work packages. So another way of putting it, and that is how the sql statement works, is that a work package + # is considered to be scheduled manually if *all* of its descendants are scheduled manually. # For example in case of the hierarchy: # A and B <- hierarchy (C is parent of both A and B) - C <- hierarchy - D # if A and B are both scheduled manually, C is also scheduled manually and so is D. But if only A is scheduled manually, - # B, C and D are scheduled automatically. + # B, C and D are scheduled automatically. If only C is scheduled manually, then D is still scheduled automatically since + # A and B are scheduled manually. # - # The recursiveness will of course also stop if no more work packages can be added. + # The recursion will of course also stop if no more work packages can be added. # # The work packages can either be connected via a follows relationship, a hierarchy relationship # or a combination of both. @@ -77,10 +78,10 @@ module ForScheduling def for_scheduling(work_packages) return none if work_packages.empty? - sql = <<~SQL + sql = <<~SQL.squish WITH RECURSIVE - #{paths_sql(work_packages)} + #{scheduling_paths_sql(work_packages)} SELECT id FROM to_schedule @@ -114,8 +115,12 @@ def for_scheduling(work_packages) # whether *all* of the added work package's descendants are automatically or manually scheduled. # # Paths whose ending work package is marked to be manually scheduled are not joined with any more. - def paths_sql(work_packages) - values = work_packages.map { |wp| "(#{wp.id}, false)" }.join(', ') + def scheduling_paths_sql(work_packages) + values = work_packages.map do |wp| + ::OpenProject::SqlSanitization + .sanitize "(:id, false)", + id: wp.id + end.join(', ') <<~SQL.squish to_schedule (id, manually) AS ( @@ -124,49 +129,52 @@ def paths_sql(work_packages) UNION SELECT - CASE - WHEN relations.to_id = to_schedule.id - THEN relations.from_id - ELSE relations.to_id - END id, - (related_work_packages.schedule_manually OR COALESCE(descendants.schedule_manually, false)) manually + relations.from_id id, + (related_work_packages.schedule_manually OR COALESCE(descendants.manually, false)) manually FROM to_schedule - JOIN - relations - ON NOT to_schedule.manually - AND (#{relations_condition_sql}) - AND - ((relations.to_id = to_schedule.id) - OR (relations.from_id = to_schedule.id AND relations.follows = 0)) + JOIN LATERAL + ( + SELECT + from_id, + to_id + FROM + relations + WHERE NOT to_schedule.manually + AND (relations.to_id = to_schedule.id AND relations.relation_type = '#{Relation::TYPE_FOLLOWS}') + UNION + SELECT + CASE + WHEN work_package_hierarchies.ancestor_id = to_schedule.id + THEN work_package_hierarchies.descendant_id + ELSE work_package_hierarchies.ancestor_id + END from_id, + to_schedule.id to_id + FROM + work_package_hierarchies + WHERE + NOT to_schedule.manually + AND work_package_hierarchies.generations = 1 + AND (work_package_hierarchies.ancestor_id = to_schedule.id + OR work_package_hierarchies.descendant_id = to_schedule.id) + ) relations ON relations.to_id = to_schedule.id LEFT JOIN work_packages related_work_packages - ON (CASE - WHEN relations.to_id = to_schedule.id - THEN relations.from_id - ELSE relations.to_id - END) = related_work_packages.id + ON relations.from_id = related_work_packages.id LEFT JOIN LATERAL ( SELECT - relations.from_id, - bool_and(COALESCE(work_packages.schedule_manually, false)) schedule_manually - FROM relations relations - JOIN work_packages + descendant_hierarchies.ancestor_id from_id, + bool_and(COALESCE(descendant_work_packages.schedule_manually, false)) manually + FROM work_package_hierarchies descendant_hierarchies + JOIN work_packages descendant_work_packages ON - work_packages.id = relations.to_id - AND related_work_packages.id = relations.from_id - AND relations.follows = 0 AND #{relations_condition_sql(transitive: true)} - GROUP BY relations.from_id + descendant_hierarchies.ancestor_id = relations.from_id + AND descendant_hierarchies.generations > 0 + AND descendant_hierarchies.descendant_id = descendant_work_packages.id + GROUP BY descendant_hierarchies.ancestor_id ) descendants ON related_work_packages.id = descendants.from_id ) SQL end - - def relations_condition_sql(transitive: false) - <<~SQL.squish - "relations"."relates" = 0 AND "relations"."duplicates" = 0 AND "relations"."blocks" = 0 AND "relations"."includes" = 0 AND "relations"."requires" = 0 - AND (relations.hierarchy + relations.relates + relations.duplicates + relations.follows + relations.blocks + relations.includes + relations.requires #{transitive ? '>' : ''}= 1) - SQL - end end end end diff --git a/app/models/work_packages/scopes/include_derived_dates.rb b/app/models/work_packages/scopes/include_derived_dates.rb index 7f8a07767c64..01ec20c59873 100644 --- a/app/models/work_packages/scopes/include_derived_dates.rb +++ b/app/models/work_packages/scopes/include_derived_dates.rb @@ -31,18 +31,27 @@ module WorkPackages::Scopes::IncludeDerivedDates class_methods do def include_derived_dates - left_joins(:descendants) - .select(*select_statement) + joins(derived_dates_join_statement) + .select(*derived_dates_select_statement) .group(:id) end private - def select_statement + def derived_dates_select_statement ["LEAST(MIN(#{descendants_alias}.start_date), MIN(#{descendants_alias}.due_date)) AS derived_start_date", "GREATEST(MAX(#{descendants_alias}.start_date), MAX(#{descendants_alias}.due_date)) AS derived_due_date"] end + def derived_dates_join_statement + <<~SQL.squish + LEFT JOIN work_package_hierarchies wp_hierarchies + ON wp_hierarchies.ancestor_id = work_packages.id AND wp_hierarchies.generations > 0 + LEFT JOIN work_packages #{descendants_alias} + ON wp_hierarchies.descendant_id = #{descendants_alias}.id + SQL + end + def descendants_alias 'descendants_work_packages' end diff --git a/app/models/work_packages/scopes/left_join_self_and_descendants.rb b/app/models/work_packages/scopes/left_join_self_and_descendants.rb index f2c382606429..2c91846c0d79 100644 --- a/app/models/work_packages/scopes/left_join_self_and_descendants.rb +++ b/app/models/work_packages/scopes/left_join_self_and_descendants.rb @@ -38,32 +38,22 @@ def left_join_self_and_descendants(user, work_package = nil) def join_descendants(user, work_package) wp_table - .outer_join(relations_table) - .on(relations_join_descendants_condition(work_package)) - .outer_join(wp_descendants) + .outer_join(hierarchies_table) + .on(hierarchies_join_condition(work_package)) + .outer_join(wp_descendants_table) .on(hierarchy_and_allowed_condition(user)) end - def relations_from_and_type_matches_condition - relations_join_condition = relation_of_wp_and_hierarchy_condition - - non_hierarchy_type_columns.each do |type| - relations_join_condition = relations_join_condition.and(relations_table[type].eq(0)) - end - - relations_join_condition + def hierarchy_of_wp_condition + wp_table[:id].eq(hierarchies_table[:ancestor_id]) end - def relation_of_wp_and_hierarchy_condition - wp_table[:id].eq(relations_table[:from_id]).and(relations_table[:hierarchy].gteq(0)) - end - - def relations_join_descendants_condition(work_package) + def hierarchies_join_condition(work_package) if work_package - relations_from_and_type_matches_condition + hierarchy_of_wp_condition .and(wp_table[:id].eq(work_package.id)) else - relations_from_and_type_matches_condition + hierarchy_of_wp_condition end end @@ -73,27 +63,23 @@ def hierarchy_and_allowed_condition(user) end def allowed_to_view_work_packages(user) - wp_descendants[:project_id].in(Project.allowed_to(user, :view_work_packages).select(:id).arel) + wp_descendants_table[:project_id].in(Project.allowed_to(user, :view_work_packages).select(:id).arel) end def self_or_descendant_condition - relations_table[:to_id].eq(wp_descendants[:id]) - end - - def non_hierarchy_type_columns - TypedDag::Configuration[WorkPackage].type_columns - [:hierarchy] + hierarchies_table[:descendant_id].eq(wp_descendants_table[:id]) end def wp_table @wp_table ||= WorkPackage.arel_table end - def relations_table - @relations || Relation.arel_table + def hierarchies_table + @relations || WorkPackageHierarchy.arel_table end - def wp_descendants - @wp_descendants ||= wp_table.alias('descendants') + def wp_descendants_table + @wp_descendants_table ||= wp_table.alias('descendants') end end end diff --git a/app/models/work_packages/scopes/relatable.rb b/app/models/work_packages/scopes/relatable.rb new file mode 100644 index 000000000000..c5ae34d5b37f --- /dev/null +++ b/app/models/work_packages/scopes/relatable.rb @@ -0,0 +1,207 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. + +module WorkPackages::Scopes + module Relatable + extend ActiveSupport::Concern + + class_methods do + # Returns all work packages that are relatable to the provided work package with a relation of the provided type. + # + # For most relation types, e.g. includes the three following rules must be satisfied: + # * Non circular: relations cannot form a circle, e.g. a -> b -> c -> a. + # * Single relation: only one relation can be created between any two work packages. E.g. it is not possible to create + # two relations like this: + # * WP 1 --- follows ---> WP 2 + # * WP 1 --- includes ---> WP 2 + # * Ancestor/descendant: relations cannot be drawn between ancestor and descendants. It is important to note though, + # that relations can be created between any two work packages within the same tree as long as they are not in a direct + # or transitive ancestor/descendant relationship. So a relation between siblings is just as possible as one between + # aunt and nephew. + # * No circles between trees: The ancestor/descendant chain is considered bidirectional when calculating relatability. + # This means that starting from the work package both the descendants as well as the ancestors are considered. That + # way, relations like + # Child 1 --- parent ---> Parent 1 ---> follows ---> Parent 2 ---> child ---> Child 2 --- follows ---> Child 1 + # are prevented. + # + # For the sake of this scope, the parent relation (Relation::TYPE_PARENT) is also included in the list of relation_types + # even though it is not stored in the same data structure. All Relation::TYPE_* values can be provided even those that + # are not canonical, e.g. Relation::TYPE_PRECEDES. The calculation for those relation types are then inverted and the + # canonical type is used, e.g. Relation::TYPE_FOLLOWS. + # + # There are a couple of exceptions and additions to the limitations outlined above for the following types: + # * Relation::TYPE_RELATES: Since this is essentially unrelated and does not carry a lot of semantic, the work packages + # are simply somehow related, such relations do not follow the "non circular" nor the "ancestor/descendant" rule. + # * Relation::TYPE_PARENT: Since creating a new relationship will remove the old parent relationship, current ancestors + # (except the direct parent) are relatable to. Descendants however are not since that would create a circle. + # + # The implementation right away excludes all work packages with which a direct relation already exists + # and uses a CTE to find all the work packages with which a transitive relationship based on the rules outlined above + # exist. + def relatable(work_package, relation_type) + return all if work_package.new_record? + + scope = not_having_directed_relation(work_package, relation_type) + .not_having_direct_relation(work_package) + .where.not(id: work_package) + + if Setting.cross_project_work_package_relations + scope + else + scope.where(project: work_package.project) + end + end + + def not_having_direct_relation(work_package) + where.not(id: Relation.where(from: work_package).select(:to_id)) + .where.not(id: Relation.where(to: work_package).select(:from_id)) + end + + def not_having_directed_relation(work_package, relation_type) + sql = <<~SQL.squish + WITH + RECURSIVE + #{non_relatable_paths_sql(work_package, relation_type)} + + SELECT id + FROM #{related_cte_name} + SQL + + scope = where("work_packages.id NOT IN (#{sql})") + + if relation_type == Relation::TYPE_PARENT + # Explicitly allow ancestors except the parent. + # This only works because an ancestor cannot be already linked to its work package. + # The #parent_id field of the work package cannot be trusted at this point since it might have + # an unpersisted change. + ancestors_without_parent = WorkPackageHierarchy + .where(descendant_id: work_package.id) + .where('generations > 1') + + scope + .or(where(id: ancestors_without_parent.select(:ancestor_id))) + else + scope + end + end + + private + + def non_relatable_paths_sql(work_package, relation_type) + <<~SQL.squish + #{related_cte_name} (id, from_hierarchy) AS ( + + #{non_recursive_relatable_values(work_package)} + + UNION + + SELECT + relations.id, + relations.from_hierarchy + FROM + #{related_cte_name} + JOIN LATERAL ( + #{joined_existing_connections(relation_type)} + ) relations ON 1 = 1 + ) + SQL + end + + def non_recursive_relatable_values(work_package) + sql = <<~SQL.squish + SELECT * FROM (VALUES(:id, false)) AS t(id, from_hierarchy) + SQL + + ::OpenProject::SqlSanitization + .sanitize sql, + id: work_package.id + end + + def joined_existing_connections(relation_type) + unions = [existing_hierarchy_lateral] + + if relation_type != Relation::TYPE_RELATES + unions << existing_relation_of_type_lateral(relation_type) + end + + unions.join(' UNION ') + end + + def existing_relation_of_type_lateral(relation_type) + # In case a 'parent' is queried for, when it comes to relations, + # it is in fact relations of type 'follows' that are of interest + # which is why they are switched for here. + # Otherwise, the canonical type has to be used as that is the only one + # that is stored in the database. + canonical_type = if relation_type == Relation::TYPE_PARENT + Relation::TYPE_FOLLOWS + else + Relation.canonical_type(relation_type) + end + + direction1, direction2 = if canonical_type == relation_type + %w[from_id to_id] + else + %w[to_id from_id] + end + + sql = <<~SQL.squish + SELECT + #{direction1} id, + false from_hierarchy + FROM + relations + WHERE (relations.#{direction2} = #{related_cte_name}.id AND relations.relation_type = :relation_type) + SQL + + ::OpenProject::SqlSanitization + .sanitize sql, + relation_type: canonical_type + end + + def existing_hierarchy_lateral + <<~SQL.squish + SELECT + CASE + WHEN work_package_hierarchies.ancestor_id = related.id + THEN work_package_hierarchies.descendant_id + ELSE work_package_hierarchies.ancestor_id + END id, + true from_hierarchy + FROM + work_package_hierarchies + WHERE + #{related_cte_name}.from_hierarchy = false AND + (work_package_hierarchies.ancestor_id = #{related_cte_name}.id OR work_package_hierarchies.descendant_id = #{related_cte_name}.id) + SQL + end + + def related_cte_name + 'related' + end + end + end +end diff --git a/app/seeders/demo_data/work_package_seeder.rb b/app/seeders/demo_data/work_package_seeder.rb index 028ca696f81e..a6f71017473c 100644 --- a/app/seeders/demo_data/work_package_seeder.rb +++ b/app/seeders/demo_data/work_package_seeder.rb @@ -197,11 +197,7 @@ def create_relations(attributes) end def create_relation(to:, from:, type:) - from.new_relation.tap do |relation| - relation.to = to - relation.relation_type = type - relation.save! - end + from.relations.create!(from: from, to: to, relation_type: type) end def calculate_start_date(days_ahead) diff --git a/app/services/projects/copy/work_packages_dependent_service.rb b/app/services/projects/copy/work_packages_dependent_service.rb index 58d9e3a026c8..a44fcec84615 100644 --- a/app/services/projects/copy/work_packages_dependent_service.rb +++ b/app/services/projects/copy/work_packages_dependent_service.rb @@ -95,9 +95,9 @@ def copy_work_package(source_work_package, parent_id, user_cf_ids) end def copy_relations(wp, new_wp_id, work_packages_map) - wp.relations_to.non_hierarchy.direct.each do |source_relation| + wp.relations_to.each do |source_relation| new_relation = Relation.new - new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id', 'relation_type') + new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id') new_relation.to_id = work_packages_map[source_relation.to_id] if new_relation.to_id.nil? && Setting.cross_project_work_package_relations? new_relation.to_id = source_relation.to_id @@ -106,9 +106,9 @@ def copy_relations(wp, new_wp_id, work_packages_map) new_relation.save end - wp.relations_from.non_hierarchy.direct.each do |source_relation| + wp.relations_from.each do |source_relation| new_relation = Relation.new - new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id', 'relation_type') + new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id') new_relation.from_id = work_packages_map[source_relation.from_id] if new_relation.from_id.nil? && Setting.cross_project_work_package_relations? new_relation.from_id = source_relation.from_id diff --git a/app/services/work_packages/delete_service.rb b/app/services/work_packages/delete_service.rb index 54dd75912b4d..90347933ef04 100644 --- a/app/services/work_packages/delete_service.rb +++ b/app/services/work_packages/delete_service.rb @@ -49,12 +49,14 @@ def persist(service_result) end def destroy(work_package) - work_package.reload.destroy + work_package.destroy + rescue ActiveRecord::StaleObjectError + destroy(work_package.reload) end def destroy_descendants(descendants, result) descendants.each do |descendant| - result.add_dependent!(ServiceResult.new(success: descendant.destroy, result: descendant)) + result.add_dependent!(ServiceResult.new(success: destroy(descendant), result: descendant)) end end diff --git a/app/services/work_packages/schedule_dependency.rb b/app/services/work_packages/schedule_dependency.rb index b3fcb280b9da..7364b5cc2bbc 100644 --- a/app/services/work_packages/schedule_dependency.rb +++ b/app/services/work_packages/schedule_dependency.rb @@ -59,10 +59,6 @@ def scheduled_work_packages_by_id private def build_dependencies - load_all_following(work_packages) - end - - def load_all_following(work_packages) following = load_following(work_packages) # Those variables are pure optimizations. @@ -79,10 +75,26 @@ def load_all_following(work_packages) def load_following(work_packages) WorkPackage .for_scheduling(work_packages) - .includes(parent_relation: :from, + .includes(:parent, follows_relations: :to) end + def add_dependencies(dependent_work_packages) + added = dependent_work_packages.inject({}) do |new_dependencies, dependent_work_package| + dependency = Dependency.new dependent_work_package, self + + new_dependencies[dependent_work_package] = dependency + + new_dependencies + end + + moved = find_moved(added) + + moved.except(*dependencies.keys) + + dependencies.merge!(moved) + end + def find_moved(candidates) candidates.select do |following, dependency| dependency.ancestors.any? { |ancestor| included_in_follows?(ancestor, candidates) } || @@ -100,22 +112,6 @@ def included_in_follows?(wp, candidates) (tos & work_packages).any? end - def add_dependencies(dependent_work_packages) - added = dependent_work_packages.inject({}) do |new_dependencies, dependent_work_package| - dependency = Dependency.new dependent_work_package, self - - new_dependencies[dependent_work_package] = dependency - - new_dependencies - end - - moved = find_moved(added) - - moved.except(*dependencies.keys) - - dependencies.merge!(moved) - end - def each_while_unhandled unhandled_by_id = dependencies.keys.group_by(&:id).transform_values(&:last) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 2786deadfe45..9b2fb21e45e0 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -192,6 +192,7 @@ def leaves_for_work_package(work_package) def related_for_work_package(work_package, relation_type) scope = work_package .send(relation_type) + .where.not(id: work_package.id) if send("#{relation_type}_joins") scope = scope.joins(send("#{relation_type}_joins")) diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index 0015a9eaefdf..c8dcfe5bd0cd 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -123,7 +123,7 @@ def cleanup def delete_relations(work_packages) unless Setting.cross_project_work_package_relations? Relation - .non_hierarchy_of_work_package(work_packages) + .of_work_package(work_packages) .destroy_all end end @@ -141,23 +141,39 @@ def reset_custom_values def reschedule_related result = ServiceResult.new(success: true, result: work_package) - if work_package.parent_id_changed? - # HACK: we need to persist the parent relation before rescheduling the parent - # and the former parent - work_package.send(:update_parent_relation) + with_temporarily_persisted_parent_changes do + if work_package.parent_id_changed? && work_package.parent_id_was + result.merge!(reschedule_former_siblings) + end - result.merge!(reschedule_former_parent) if work_package.parent_id_was + result.merge!(reschedule(work_package)) end - result.merge!(reschedule(work_package)) - result end - def reschedule_former_parent - former_siblings = WorkPackage.includes(:parent_relation).where(relations: { from_id: work_package.parent_id_was }) + def with_temporarily_persisted_parent_changes + # Explicitly using requires_new: true since we are already within a transaction. + # Because of that, raising ActiveRecord::Rollback would have no effect: + # https://www.bigbinary.com/learn-rubyonrails-book/activerecord-transactions-in-depth#nested-transactions + WorkPackage.transaction(requires_new: true) do + if work_package.parent_id_changed? + # HACK: we need to persist the parent relation before rescheduling the parent + # and the former parent since we rely on the database for scheduling. + WorkPackage.where(id: work_package.id).update_all(parent_id: work_package.parent_id) + work_package.rebuild! # using the ClosureTree#rebuild! method to update the transitive hierarchy information + end + + yield + + # Always rolling back the changes we made in here + raise ActiveRecord::Rollback + end + end - reschedule(former_siblings) + # Rescheduling the former siblings will lead to the whole former tree being rescheduled. + def reschedule_former_siblings + reschedule(WorkPackage.where(parent_id: work_package.parent_id_was)) end def reschedule(work_packages) @@ -181,9 +197,7 @@ def consolidated_results(result) master = instances.pop instances.each do |instance| - master.attributes = instance.changes.map do |attribute, values| - [attribute, values.last] - end.to_h + master.attributes = instance.changes.transform_values(&:last) end a + [master] diff --git a/config/brakeman.ignore b/config/brakeman.ignore index d25a6f10c157..28540c5b6262 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -311,26 +311,6 @@ "confidence": "Medium", "note": "Never called with user input" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "cd1b3c94dc92e20efe2c696ee1c086a4da2491b5d839a44617f828359fcd42f2", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/models/work_package.rb", - "line": 625, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "where(\"id IN (SELECT common_id FROM (#{[Relation.hierarchy.where(:from_id => Relation.where(:to => work_packages).hierarchy_or_follows.select(:from_id)).select(\"to_id common_id\"), Relation.where(:to => work_packages).hierarchy_or_follows.select(\"from_id common_id\")].map(&:to_sql).join(\" UNION \")}) following_relations)\")", - "render_path": null, - "location": { - "type": "method", - "class": "WorkPackage", - "method": "WorkPackage.hierarchy_tree_following" - }, - "user_input": "Relation.where(:to => work_packages).hierarchy_or_follows", - "confidence": "High", - "note": "static SQL" - }, { "warning_type": "SQL Injection", "warning_code": 0, diff --git a/config/initializers/typed_dag.rb b/config/initializers/typed_dag.rb deleted file mode 100644 index 6c61bbdcbc57..000000000000 --- a/config/initializers/typed_dag.rb +++ /dev/null @@ -1,62 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2022 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -TypedDag::Configuration.set node_class_name: 'WorkPackage', - edge_class_name: 'Relation', - ancestor_column: 'from_id', - descendant_column: 'to_id', - types: { - hierarchy: { from: { name: :parent, limit: 1 }, - to: :children, - all_from: :ancestors, - all_to: :descendants }, - relates: { from: :related_to, - to: :relates_to, - all_from: :all_related_to, - all_to: :all_relates_to }, - duplicates: { from: :duplicates, - to: :duplicated, - all_from: :all_duplicates, - all_to: :all_duplicated }, - follows: { from: :precedes, - to: :follows, - all_from: :all_precedes, - all_to: :all_follows }, - blocks: { from: :blocked_by, - to: :blocks, - all_from: :all_blocked_by, - all_to: :all_blocks }, - includes: { from: :part_of, - to: :includes, - all_from: :all_part_of, - all_to: :all_includes }, - requires: { from: :required_by, - to: :requires, - all_from: :all_required_by, - all_to: :all_requires } - } diff --git a/db/migrate/20180105130053_rebuild_dag.rb b/db/migrate/20180105130053_rebuild_dag.rb index c411162362b2..ba15c84b7906 100644 --- a/db/migrate/20180105130053_rebuild_dag.rb +++ b/db/migrate/20180105130053_rebuild_dag.rb @@ -26,11 +26,14 @@ # See COPYRIGHT and LICENSE files for more details. #++ require_relative './migration_utils/utils' +require_relative 'migration_utils/typed_dag' class RebuildDag < ActiveRecord::Migration[5.0] include ::Migration::Utils def up + Migration::MigrationUtils::TypedDag.configure + truncate_closure_entries remove_duplicate_relations @@ -48,7 +51,7 @@ def up unique: true say_with_time 'Building the directed acyclic graph of all relations. This might take a while.' do - WorkPackage.rebuild_dag! 1000 + Migration::MigrationUtils::TypedDag::WorkPackage.rebuild_dag! 1000 end add_count_index diff --git a/db/migrate/20220319211253_add_parent_id_to_wp.rb b/db/migrate/20220319211253_add_parent_id_to_wp.rb new file mode 100644 index 000000000000..38e2883d76a6 --- /dev/null +++ b/db/migrate/20220319211253_add_parent_id_to_wp.rb @@ -0,0 +1,238 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class AddParentIdToWp < ActiveRecord::Migration[6.1] + RELATION_TYPES = %i[relates duplicates blocks follows includes requires hierarchy].freeze + + def up + migrate_parent_information + + add_relation_type + + add_relation_index + + add_closure_tree_table + + ClosureTreeWorkPackage.rebuild! + + cleanup_transitive_relations + + remove_typed_dag_columns + end + + def down + add_relation_type_column + + update_relation_column_from_relation_type + + insert_hierarchy_relation_for_parent + + remove_closure_tree + + remove_closure_tree_columns_on_foreign_tables + + build_typed_dag + end + + private + + def migrate_parent_information + add_column :work_packages, :parent_id, :integer, null: true + + add_parent_index + + execute <<~SQL.squish + UPDATE + work_packages + SET + parent_id = from_id + FROM relations + WHERE + hierarchy = 1 + AND relates = 0 + AND duplicates = 0 + AND blocks = 0 + AND follows = 0 + AND includes = 0 + AND requires = 0 + AND work_packages.id = relations.to_id + SQL + + execute <<~SQL.squish + DELETE + FROM + relations + WHERE + hierarchy = 1 + AND #{(RELATION_TYPES - [:hierarchy]).join(' = 0 AND ')} = 0 + SQL + end + + def add_relation_type + add_column :relations, :relation_type, :string + + (RELATION_TYPES - [:hierarchy]).each do |type| + execute <<~SQL.squish + UPDATE + relations + SET + relation_type = '#{type}' + WHERE + #{type} = 1 + AND #{(RELATION_TYPES - [type]).join(' = 0 AND ')} = 0 + SQL + end + end + + def add_closure_tree_table + # Copied from closure tree migration + # rubocop:disable Rails/CreateTableWithTimestamps + create_table :work_package_hierarchies, id: false do |t| + t.integer :ancestor_id, null: false + t.integer :descendant_id, null: false + t.integer :generations, null: false + end + + add_index :work_package_hierarchies, %i[ancestor_id descendant_id generations], + unique: true, + name: "work_package_anc_desc_idx" + + add_index :work_package_hierarchies, [:descendant_id], + name: "work_package_desc_idx" + # End copied from closure tree migration + # rubocop:enable Rails/CreateTableWithTimestamps + end + + def add_relation_index + add_index :relations, %i[from_id to_id relation_type], + unique: true + end + + def add_parent_index + add_index :work_packages, :parent_id + end + + def add_relation_type_column + change_table :relations do |r| + RELATION_TYPES.each do |column| + r.column column, :integer, default: 0, null: false + end + end + end + + def cleanup_transitive_relations + execute <<~SQL.squish + DELETE + FROM + relations + WHERE + #{RELATION_TYPES.join(' + ')} != 1 + SQL + end + + def remove_typed_dag_columns + RELATION_TYPES.each do |type| + remove_column :relations, type + end + + remove_column :relations, :count + end + + def update_relation_column_from_relation_type + ActiveRecord::Base.connection.execute <<-SQL.squish + UPDATE + relations + SET + relates = CASE + WHEN relations.relation_type = 'relates' + THEN 1 + ELSE 0 + END, + duplicates = CASE + WHEN relations.relation_type = 'duplicates' + THEN 1 + ELSE 0 + END, + blocks = CASE + WHEN relations.relation_type = 'blocks' + THEN 1 + ELSE 0 + END, + follows = CASE + WHEN relations.relation_type = 'precedes' + THEN 1 + ELSE 0 + END, + includes = CASE + WHEN relations.relation_type = 'includes' + THEN 1 + ELSE 0 + END, + requires = CASE + WHEN relations.relation_type = 'requires' + THEN 1 + ELSE 0 + END + SQL + end + + def remove_closure_tree + drop_table :work_package_hierarchies + end + + def remove_closure_tree_columns_on_foreign_tables + remove_column :relations, :relation_type + + remove_column :work_packages, :parent_id + end + + def build_typed_dag + require_relative '20180105130053_rebuild_dag' + + ::RebuildDag.new.up + end + + def insert_hierarchy_relation_for_parent + ActiveRecord::Base.connection.execute <<-SQL.squish + INSERT INTO relations + (from_id, to_id, hierarchy) + SELECT w1.id, w2.id, 1 + FROM work_packages w1 + JOIN work_packages w2 + ON w1.id = w2.parent_id + SQL + end + + # rubocop:disable Rails/ApplicationRecord + class ClosureTreeWorkPackage < ActiveRecord::Base + self.table_name = 'work_packages' + + has_closure_tree + end + # rubocop:enable Rails/ApplicationRecord +end diff --git a/db/migrate/migration_utils/typed_dag.rb b/db/migrate/migration_utils/typed_dag.rb new file mode 100644 index 000000000000..4161e481405f --- /dev/null +++ b/db/migrate/migration_utils/typed_dag.rb @@ -0,0 +1,86 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require 'typed_dag' + +module Migration + module MigrationUtils + module TypedDag + # rubocop:disable Rails/ApplicationRecord + class WorkPackage < ActiveRecord::Base + self.table_name = 'work_packages' + end + + class Relation < ActiveRecord::Base + self.table_name = 'relations' + end + # rubocop:enable Rails/ApplicationRecord + + def self.configure + ::TypedDag::Configuration.set node_class_name: 'Migration::MigrationUtils::TypedDag::WorkPackage', + edge_class_name: 'Migration::MigrationUtils::TypedDag::Relation', + ancestor_column: 'from_id', + descendant_column: 'to_id', + types: { + hierarchy: { from: { name: :parent, limit: 1 }, + to: :children, + all_from: :ancestors, + all_to: :descendants }, + relates: { from: :related_to, + to: :relates_to, + all_from: :all_related_to, + all_to: :all_relates_to }, + duplicates: { from: :duplicates, + to: :duplicated, + all_from: :all_duplicates, + all_to: :all_duplicated }, + follows: { from: :precedes, + to: :follows, + all_from: :all_precedes, + all_to: :all_follows }, + blocks: { from: :blocked_by, + to: :blocks, + all_from: :all_blocked_by, + all_to: :all_blocks }, + includes: { from: :part_of, + to: :includes, + all_from: :all_part_of, + all_to: :all_includes }, + requires: { from: :required_by, + to: :requires, + all_from: :all_required_by, + all_to: :all_requires } + } + + # Needs to be included after the configuration. + WorkPackage.include(::TypedDag::Node) + Relation.include(::TypedDag::Edge) + end + end + end +end diff --git a/lib/api/v3/relations/relations_api.rb b/lib/api/v3/relations/relations_api.rb index 9d99362a062a..80df512bb58c 100644 --- a/lib/api/v3/relations/relations_api.rb +++ b/lib/api/v3/relations/relations_api.rb @@ -34,7 +34,6 @@ class RelationsAPI < ::API::OpenProjectAPI get &::API::V3::Utilities::Endpoints::Index.new(model: Relation, scope: -> { Relation - .non_hierarchy .includes(::API::V3::Relations::RelationRepresenter.to_eager_load) }, render_representer: RelationPaginatedCollectionRepresenter) diff --git a/lib/api/v3/work_packages/eager_loading/hierarchy.rb b/lib/api/v3/work_packages/eager_loading/hierarchy.rb index 792803634e9b..ace3bd9fb3af 100644 --- a/lib/api/v3/work_packages/eager_loading/hierarchy.rb +++ b/lib/api/v3/work_packages/eager_loading/hierarchy.rb @@ -40,10 +40,10 @@ def apply(work_package) def children(id) @children ||= WorkPackage - .joins(:parent_relation) - .where(relations: { from_id: work_packages.map(&:id) }) - .select(:id, :subject, :project_id, :from_id) - .group_by(&:from_id).to_h + .where(parent_id: work_packages.map(&:id)) + .select(:id, :subject, :project_id, :parent_id) + .group_by(&:parent_id) + .to_h @children[id] || [] end diff --git a/lib/api/v3/work_packages/schema/form_configurations/query_representer.rb b/lib/api/v3/work_packages/schema/form_configurations/query_representer.rb index b30e4a73f40e..2681ab23fa59 100644 --- a/lib/api/v3/work_packages/schema/form_configurations/query_representer.rb +++ b/lib/api/v3/work_packages/schema/form_configurations/query_representer.rb @@ -54,7 +54,7 @@ class QueryRepresenter < ::API::Decorators::Single end def _type - if relation_type == ::Relation::TYPE_HIERARCHY + if relation_type == ::Relation::TYPE_PARENT "WorkPackageFormChildrenQueryGroup" else "WorkPackageFormRelationQueryGroup" diff --git a/lib/api/v3/work_packages/watchers_api.rb b/lib/api/v3/work_packages/watchers_api.rb index 52d17d04622d..188f9de28309 100644 --- a/lib/api/v3/work_packages/watchers_api.rb +++ b/lib/api/v3/work_packages/watchers_api.rb @@ -57,7 +57,7 @@ class WatchersAPI < ::API::OpenProjectAPI resources :watchers do helpers do def watchers_collection - watchers = @work_package.watcher_users.merge(Principal.not_locked) + watchers = @work_package.watcher_users.merge(Principal.not_locked, rewhere: true) self_link = api_v3_paths.work_package_watchers(@work_package.id) Users::UnpaginatedUserCollectionRepresenter.new(watchers, self_link: self_link, diff --git a/lib/api/v3/work_packages/work_package_relations_api.rb b/lib/api/v3/work_packages/work_package_relations_api.rb index 243965c6ebe6..ef0e50c26b08 100644 --- a/lib/api/v3/work_packages/work_package_relations_api.rb +++ b/lib/api/v3/work_packages/work_package_relations_api.rb @@ -42,7 +42,6 @@ class WorkPackageRelationsAPI < ::API::OpenProjectAPI relations = query .where(:involved, '=', @work_package.id) .results - .non_hierarchy .includes(::API::V3::Relations::RelationCollectionRepresenter.to_eager_load) ::API::V3::Relations::RelationCollectionRepresenter.new( diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index 0d08c9f14480..392ee70d3bbf 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -537,8 +537,6 @@ def relations self_path = api_v3_paths.work_package_relations(represented.id) visible_relations = represented .visible_relations(current_user) - .direct - .non_hierarchy .includes(::API::V3::Relations::RelationCollectionRepresenter.to_eager_load) ::API::V3::Relations::RelationCollectionRepresenter.new(visible_relations, diff --git a/modules/backlogs/app/models/impediment.rb b/modules/backlogs/app/models/impediment.rb index a811ea3cef46..10f205e7a403 100644 --- a/modules/backlogs/app/models/impediment.rb +++ b/modules/backlogs/app/models/impediment.rb @@ -29,7 +29,7 @@ class Impediment < Task extend OpenProject::Backlogs::Mixins::PreventIssueSti - after_save :update_blocks_list + before_save :update_blocks_list validate :validate_blocks_list @@ -39,23 +39,25 @@ def self.default_scope end def blocks_ids=(ids) - @blocks_ids_list = [ids] if ids.is_a?(Integer) - @blocks_ids_list = ids.split(/\D+/).map(&:to_i) if ids.is_a?(String) - @blocks_ids_list = ids.map(&:to_i) if ids.is_a?(Array) + @blocks_ids = [ids] if ids.is_a?(Integer) + @blocks_ids = ids.split(/\D+/).map(&:to_i) if ids.is_a?(String) + @blocks_ids = ids.map(&:to_i) if ids.is_a?(Array) end def blocks_ids - @blocks_ids_list ||= block_ids + @blocks_ids ||= blocks_relations.map(&:to_id) end private def update_blocks_list - self.block_ids = blocks_ids + mark_blocks_to_destroy + + build_new_blocks end def validate_blocks_list - if blocks_ids.size == 0 + if blocks_ids.empty? errors.add :blocks_ids, :must_block_at_least_one_work_package else other_version_ids = WorkPackage.where(id: blocks_ids).pluck(:version_id).uniq @@ -65,4 +67,14 @@ def validate_blocks_list end end end + + def mark_blocks_to_destroy + blocks_relations.reject { |relation| blocks_ids.include?(relation.to_id) }.each(&:mark_for_destruction) + end + + def build_new_blocks + (blocks_ids - blocks_relations.select { |relation| blocks_ids.include?(relation.to_id) }.map(&:to_id)).each do |id| + blocks_relations.build(to_id: id) + end + end end diff --git a/modules/backlogs/lib/open_project/backlogs/patches/update_service_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/update_service_patch.rb index 34813860e385..7740752f3e0b 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/update_service_patch.rb +++ b/modules/backlogs/lib/open_project/backlogs/patches/update_service_patch.rb @@ -45,13 +45,13 @@ def update_descendants def inherit_version_to_descendants(result) all_descendants = work_package .descendants - .includes(:parent_relation, project: :enabled_modules) - .order(Arel.sql('relations.hierarchy asc')) - .select('work_packages.*, relations.hierarchy') + .includes(project: :enabled_modules) + .order_by_ancestors('asc') + .select('work_packages.*') stop_descendants_ids = [] descendant_tasks = all_descendants.reject do |t| - if stop_descendants_ids.include?(t.parent_relation.from_id) || !t.is_task? + if stop_descendants_ids.include?(t.parent_id) || !t.is_task? stop_descendants_ids << t.id end end diff --git a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb index 5682a457deca..58d68214ee50 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb +++ b/modules/backlogs/lib/open_project/backlogs/patches/work_package_patch.rb @@ -63,14 +63,7 @@ def backlogs_types end def children_of(ids) - includes(:parent_relation) - .where(relations: { from_id: ids }) - end - - # Prevent problems with subclasses of WorkPackage - # not having a TypedDag configuration - def _dag_options - TypedDag::Configuration[WorkPackage] + where(parent_id: ids) end end @@ -130,13 +123,6 @@ def blocks blocks_relations.includes(:to).merge(WorkPackage.with_status_open).map(&:to) end - def blockers - # return work_packages that block me - return [] if closed? - - blocked_by_relations.includes(:from).merge(WorkPackage.with_status_open).map(&:from) - end - def backlogs_enabled? !!project.try(:module_enabled?, 'backlogs') end diff --git a/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb b/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb index b0dcbc46361a..e6fe876c34f8 100644 --- a/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb +++ b/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb @@ -137,11 +137,29 @@ priority: issue_priority) end + let(:relatable_scope) do + scope = instance_double('ActiveRecord::Relation') + + allow(scope) + .to receive(:where) + .and_return(scope) + + allow(scope) + .to receive(:empty?) + .and_return(false) + + scope + end + subject(:valid) { instance.validate } - before(:each) do + before do project.save! + allow(WorkPackage) + .to receive(:relatable) + .and_return(relatable_scope) + allow(Setting).to receive(:plugin_openproject_backlogs).and_return({ 'points_burn_direction' => 'down', 'wiki_template' => '', 'card_spec' => 'Sattleford VM-5040', diff --git a/modules/backlogs/spec/models/impediment_spec.rb b/modules/backlogs/spec/models/impediment_spec.rb index 64f6da464cf0..05354d96ab0f 100644 --- a/modules/backlogs/spec/models/impediment_spec.rb +++ b/modules/backlogs/spec/models/impediment_spec.rb @@ -71,7 +71,7 @@ status: status) end - before(:each) do + before do allow(Setting) .to receive(:plugin_openproject_backlogs) .and_return({ 'points_burn_direction' => 'down', @@ -106,18 +106,19 @@ end end - describe 'WITH only prior blockers defined' do - before(:each) do + describe 'WITH loading from the backend' do + before do feature.version = version feature.save task.version = version task.save # Using the default association method block_ids (without s) here - impediment.block_ids = [feature.id, task.id] + impediment.blocks_ids = [feature.id, task.id] + impediment.save end - it { expect(impediment.blocks_ids).to eql [feature.id, task.id] } + it { expect(described_class.find(impediment.id).blocks_ids).to eql [feature.id, task.id] } end end end diff --git a/modules/backlogs/spec/services/impediments/create_services_spec.rb b/modules/backlogs/spec/services/impediments/create_services_spec.rb index de97bba8c5fe..76e6e48a33b8 100644 --- a/modules/backlogs/spec/services/impediments/create_services_spec.rb +++ b/modules/backlogs/spec/services/impediments/create_services_spec.rb @@ -83,14 +83,14 @@ shared_examples_for 'impediment creation with 1 blocking relationship' do it_should_behave_like 'impediment creation' - it { expect(subject.relations_to.direct.size).to eq(1) } - it { expect(subject.relations_to.direct[0].to).to eql feature } - it { expect(subject.relations_to.direct[0].relation_type).to eql Relation::TYPE_BLOCKS } + it { expect(subject.relations_to.size).to eq(1) } + it { expect(subject.relations_to[0].to).to eql feature } + it { expect(subject.relations_to[0].relation_type).to eql Relation::TYPE_BLOCKS } end shared_examples_for 'impediment creation with no blocking relationship' do it_should_behave_like 'impediment creation' - it { expect(subject.relations_to.direct.size).to eq(0) } + it { expect(subject.relations_to.size).to eq(0) } end describe 'WITH a blocking relationship to a story' do @@ -113,7 +113,7 @@ it_should_behave_like 'impediment creation with 1 blocking relationship' it { expect(subject).not_to be_new_record } - it { expect(subject.relations_to.direct[0]).not_to be_new_record } + it { expect(subject.relations_to[0]).not_to be_new_record } end describe 'WITH the story having another version' do diff --git a/modules/backlogs/spec/services/impediments/update_service_spec.rb b/modules/backlogs/spec/services/impediments/update_service_spec.rb index f4b986b3b90a..9edb6dd6f8ae 100644 --- a/modules/backlogs/spec/services/impediments/update_service_spec.rb +++ b/modules/backlogs/spec/services/impediments/update_service_spec.rb @@ -111,18 +111,18 @@ shared_examples_for 'impediment update with changed blocking relationship' do it_should_behave_like 'impediment update' - it { expect(subject.relations_to.direct.size).to eq(1) } - it { expect(subject.relations_to.direct[0]).not_to be_new_record } - it { expect(subject.relations_to.direct[0].to).to eql story } - it { expect(subject.relations_to.direct[0].relation_type).to eql Relation::TYPE_BLOCKS } + it { expect(subject.relations.size).to eq(1) } + it { expect(subject.relations[0]).not_to be_new_record } + it { expect(subject.relations[0].to).to eql story } + it { expect(subject.relations[0].relation_type).to eql Relation::TYPE_BLOCKS } end shared_examples_for 'impediment update with unchanged blocking relationship' do it_should_behave_like 'impediment update' - it { expect(subject.relations_to.direct.size).to eq(1) } - it { expect(subject.relations_to.direct[0]).not_to be_changed } - it { expect(subject.relations_to.direct[0].to).to eql feature } - it { expect(subject.relations_to.direct[0].relation_type).to eql Relation::TYPE_BLOCKS } + it { expect(subject.relations.size).to eq(1) } + it { expect(subject.relations[0]).not_to be_changed } + it { expect(subject.relations[0].to).to eql feature } + it { expect(subject.relations[0].relation_type).to eql Relation::TYPE_BLOCKS } end subject do diff --git a/modules/xls_export/app/models/xls_export/work_package/exporter/xls.rb b/modules/xls_export/app/models/xls_export/work_package/exporter/xls.rb index 3c6c36a38cbf..0fb9f7a26d3f 100644 --- a/modules/xls_export/app/models/xls_export/work_package/exporter/xls.rb +++ b/modules/xls_export/app/models/xls_export/work_package/exporter/xls.rb @@ -151,7 +151,7 @@ def with_relations_headers end def work_package_relations(work_package) - work_package.relations.direct.non_hierarchy.visible + work_package.relations.visible end end end diff --git a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb index 747b01735f09..2ac9d1b45a48 100644 --- a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb +++ b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb @@ -55,13 +55,7 @@ end let(:relation) do - child_2.new_relation.tap do |r| - r.to = followed - r.relation_type = 'follows' - r.delay = 0 - r.description = 'description foobar' - r.save - end + create(:follows_relation, from: child_2, to: followed, description: 'description foobar') end let(:relations) { [relation] } diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index 5036daa10b41..bfe9198a3117 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -195,7 +195,7 @@ end it 'is not writable' do - expect(contract.writable?(:status)).to be_falsey + expect(contract).not_to be_writable(:status) end context 'if we only switched into that status now' do @@ -206,7 +206,7 @@ end it 'is writable' do - expect(contract.writable?(:status)).to be_truthy + expect(contract).to be_writable(:status) end end end @@ -564,7 +564,6 @@ end describe 'parent' do - let(:child) { build_stubbed(:stubbed_work_package) } let(:parent) { build_stubbed(:stubbed_work_package) } before do @@ -576,47 +575,36 @@ # while we do validate the parent # the errors are still put on :base so that the messages can be reused - contract.errors.symbols_for(:base) + contract.errors.symbols_for(:parent) end context 'when self assigning' do let(:parent) { work_package } - it 'returns an error for the aparent' do - expect(contract.validate).to eq false - expect(contract.errors.symbols_for(:parent)).to eq [:cannot_be_self_assigned] + it 'returns an error for the parent' do + expect(subject) + .to eq [:cannot_be_self_assigned] end end - context 'a relation exists between the parent and its ancestors and the work package and its descendants' do - let(:parent) { child } - + context 'when the intended parent is not relatable' do before do - from_parent_stub = double('from parent stub') - allow(Relation) - .to receive(:from_parent_to_self_and_descendants) - .with(work_package) - .and_return(from_parent_stub) - - from_descendants_stub = double('from descendants stub') - allow(Relation) - .to receive(:from_self_and_descendants_to_ancestors) - .with(work_package) - .and_return(from_descendants_stub) - - allow(from_parent_stub) - .to receive(:or) - .with(from_descendants_stub) - .and_return(from_parent_stub) - - allow(from_parent_stub) - .to receive_message_chain(:direct, :exists?) - .and_return(true) + scope = instance_double('ActiveRecord::Relation') + + allow(WorkPackage) + .to receive(:relatable) + .with(work_package, Relation::TYPE_PARENT) + .and_return(scope) + + allow(scope) + .to receive(:where) + .with(id: parent.id) + .and_return([]) end it 'is invalid' do - expect(subject.include?(:cant_link_a_work_package_with_a_descendant)) - .to be_truthy + expect(subject) + .to include(:cant_link_a_work_package_with_a_descendant) end end end diff --git a/spec/contracts/work_packages/update_contract_spec.rb b/spec/contracts/work_packages/update_contract_spec.rb index d5e94e8ddd57..a9e66a7d28c9 100644 --- a/spec/contracts/work_packages/update_contract_spec.rb +++ b/spec/contracts/work_packages/update_contract_spec.rb @@ -244,10 +244,11 @@ describe 'with children' do context 'changing to milestone' do let(:milestone) { build_stubbed :type, is_milestone: true } + let(:children) { [build_stubbed(:work_package)] } before do work_package.type = milestone - allow(work_package).to receive_message_chain(:children, :any?).and_return true + allow(work_package).to receive(:children).and_return children contract.validate end diff --git a/spec/controllers/work_packages/bulk_controller_spec.rb b/spec/controllers/work_packages/bulk_controller_spec.rb index a96faf32d0fb..e63e15210042 100644 --- a/spec/controllers/work_packages/bulk_controller_spec.rb +++ b/spec/controllers/work_packages/bulk_controller_spec.rb @@ -28,6 +28,7 @@ require 'spec_helper' +# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::BulkController, type: :controller, with_settings: { journal_aggregation_time_minutes: 0 } do let(:user) { create(:user) } let(:user2) { create(:user) } @@ -566,9 +567,6 @@ end before do - expect(new_parent.start_date).to be_nil - expect(new_parent.due_date).to be_nil - put :update, params: { ids: [task1.id, task2.id], @@ -577,7 +575,7 @@ } end - it 'should update the parent dates as well' do + it 'updates the parent dates as well' do expect(response.response_code).to eq(302) task1.reload @@ -626,7 +624,7 @@ end end - it 'should redirect to the project' do + it 'redirects to the project' do expect(response).to redirect_to(project_work_packages_path(stub_work_package.project)) end end @@ -641,9 +639,10 @@ end end - it 'should redirect to the project' do + it 'redirects to the project' do expect(response).to render_template('destroy') end end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/factories/relation_factory.rb b/spec/factories/relation_factory.rb index dd349a42211d..13e45b6d3888 100644 --- a/spec/factories/relation_factory.rb +++ b/spec/factories/relation_factory.rb @@ -39,8 +39,4 @@ relation_type { 'follows' } delay { 0 } end - - factory :hierarchy_relation, parent: :relation do - relation_type { 'hierarchy' } - end end diff --git a/spec/features/types/form_configuration_query_spec.rb b/spec/features/types/form_configuration_query_spec.rb index 0f6485b26b7c..3573a18cf0b3 100644 --- a/spec/features/types/form_configuration_query_spec.rb +++ b/spec/features/types/form_configuration_query_spec.rb @@ -37,11 +37,22 @@ let(:project) { create :project, types: [type_bug, type_task] } let(:other_project) { create :project, types: [type_task] } let!(:work_package) do - create :work_package, - new_relation.merge( - project: project, - type: type_bug - ) + create(:work_package, + project: project, + type: type_bug).tap do |wp| + case wp_relation_type + when :children + wp.children = [related_bug, related_task, related_task_other_project] + when :blocks + [related_bug, related_task, related_task_other_project].each do |related| + create(:relation, from: wp, to: related, relation_type: Relation::TYPE_BLOCKS) + end + when :relates_to + [related_bug, related_task, related_task_other_project].each do |related| + create(:relation, from: wp, to: related, relation_type: Relation::TYPE_RELATES) + end + end + end end let(:wp_relation_type) { :children } let(:frontend_relation_type) { wp_relation_type } diff --git a/spec/features/work_packages/details/query_groups/relation_query_group_spec.rb b/spec/features/work_packages/details/query_groups/relation_query_group_spec.rb index f3c022eab2c2..bfc64caed121 100644 --- a/spec/features/work_packages/details/query_groups/relation_query_group_spec.rb +++ b/spec/features/work_packages/details/query_groups/relation_query_group_spec.rb @@ -35,11 +35,6 @@ let(:project) { create :project, types: [type] } let(:relation_type) { :parent } let(:relation_target) { work_package } - let(:new_relation) do - rel = Hash.new - rel[relation_type] = relation_target - rel - end let(:type) do create :type_with_relation_query_group, relation_filter: relation_type @@ -50,11 +45,15 @@ type: type end let!(:related_work_package) do - create :work_package, - new_relation.merge( - project: project, - type: type - ) + create(:work_package, + project: project, + type: type).tap do |wp| + if relation_type == :parent + wp.update(parent: relation_target) + else + create(:follows_relation, from: wp, to: relation_target) + end + end end let(:work_packages_page) { ::Pages::SplitWorkPackage.new(work_package) } @@ -114,10 +113,11 @@ let!(:project3) { create(:project, types: [type]) } let(:relation_type) { :follows } let!(:related_work_package) do - create :work_package, + create(:work_package, project: project2, - type: type, - follows: [work_package] + type: type).tap do |wp| + create(:follows_relation, from: wp, to: work_package) + end end let(:type) do @@ -186,7 +186,7 @@ context 'follower table' do let(:relation_type) { :follows } - let(:relation_target) { [work_package] } + let(:relation_target) { work_package } let!(:independent_work_package) do create :work_package, project: project @@ -211,7 +211,7 @@ it 'add existing, remove it, add it from relations tab, remove from relations tab' do embedded_table.table_container.find('button', text: I18n.t('js.relation_buttons.add_existing')).click - container = embedded_table.table_container.find('.wp-relations-create--form', wait: 10) + embedded_table.table_container.find('.wp-relations-create--form', wait: 10) autocomplete = page.find("[data-qa-selector='wp-relations-autocomplete']") select_autocomplete autocomplete, results_selector: '.ng-dropdown-panel-items', diff --git a/spec/features/work_packages/details/relations/relations_spec.rb b/spec/features/work_packages/details/relations/relations_spec.rb index 71ade8fb1622..540fba8c210d 100644 --- a/spec/features/work_packages/details/relations/relations_spec.rb +++ b/spec/features/work_packages/details/relations/relations_spec.rb @@ -176,7 +176,7 @@ def visit_relations end let!(:relatable) { create(:work_package, project: project) } - it 'should allow to manage relations' do + it 'allows to manage relations' do relations.add_relation(type: 'follows', to: relatable) # Relations counter badge should increase number of relations @@ -189,10 +189,10 @@ def visit_relations tabs.expect_no_counter(relations_tab) work_package.reload - expect(work_package.relations.direct).to be_empty + expect(work_package.relations).to be_empty end - it 'should allow to move between split and full view (Regression #24194)' do + it 'allows to move between split and full view (Regression #24194)' do relations.add_relation(type: 'follows', to: relatable) # Relations counter should increase tabs.expect_counter(relations_tab, 1) @@ -214,7 +214,7 @@ def visit_relations expect(page).to have_no_selector('.wp-relations--subject-field', text: relatable.subject) end - it 'should follow the relation links (Regression #26794)' do + it 'follows the relation links (Regression #26794)' do relations.add_relation(type: 'follows', to: relatable) relations.click_relation(relatable) @@ -226,7 +226,7 @@ def visit_relations subject.expect_state_text work_package.subject end - it 'should allow to change relation descriptions' do + it 'allows to change relation descriptions' do relations.add_relation(type: 'follows', to: relatable) ## Toggle description @@ -268,8 +268,7 @@ def visit_relations created_row.find('.wp-relation--description-read-value', text: 'my description!').click - relation = work_package.relations.direct.first - relation.reload + relation = work_package.relations.first expect(relation.description).to eq('my description!') # Toggle to close diff --git a/spec/features/work_packages/scheduling/scheduling_mode_spec.rb b/spec/features/work_packages/scheduling/scheduling_mode_spec.rb index 6596ff6ed85a..032462dd5dca 100644 --- a/spec/features/work_packages/scheduling/scheduling_mode_spec.rb +++ b/spec/features/work_packages/scheduling/scheduling_mode_spec.rb @@ -50,16 +50,12 @@ # v v # wp_child wp_suc_child # - let!(:wp) { create :work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05' } + let!(:wp) { create :work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05', parent: wp_parent } let!(:wp_parent) do - create(:work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05').tap do |parent| - create(:hierarchy_relation, from: parent, to: wp) - end + create(:work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05') end let!(:wp_child) do - create(:work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05').tap do |child| - create(:hierarchy_relation, from: wp, to: child) - end + create(:work_package, project: project, start_date: '2016-01-01', due_date: '2016-01-05', parent: wp) end let!(:wp_pre) do create(:work_package, project: project, start_date: '2015-12-15', due_date: '2015-12-31').tap do |pre| @@ -67,21 +63,16 @@ end end let!(:wp_suc) do - create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10').tap do |suc| + create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10', parent: wp_suc_parent).tap do |suc| create(:follows_relation, from: suc, to: wp) end end let!(:wp_suc_parent) do - create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10').tap do |parent| - create(:hierarchy_relation, from: parent, to: wp_suc) - end + create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10') end let!(:wp_suc_child) do - create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10').tap do |child| - create(:hierarchy_relation, from: wp_suc, to: child) - end + create(:work_package, project: project, start_date: '2016-01-06', due_date: '2016-01-10', parent: wp_suc) end - let(:user) { create :admin } let(:work_packages_page) { Pages::SplitWorkPackage.new(wp, project) } let(:combined_field) { work_packages_page.edit_field(:combinedDate) } @@ -92,15 +83,15 @@ def expect_dates(work_package, start_date, due_date) expect(work_package.due_date).to eql Date.parse(due_date) end - before do - login_as(user) + current_user { create :admin } + before do work_packages_page.visit! work_packages_page.ensure_page_loaded end it 'can toggle the scheduling mode through the date modal' do - expect(wp.schedule_manually).to eq false + expect(wp.schedule_manually).to be_falsey # Editing the start/due dates of a parent work package is possible if the # work package is manually scheduled diff --git a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb index 5ff32d31d21e..aef7b7c8cfd5 100644 --- a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb @@ -1123,9 +1123,15 @@ end before do + scope = instance_double('ActiveRecord::Relation') + allow(work_package) - .to receive_message_chain(:visible_relations, :direct, :non_hierarchy, :includes) - .and_return([relation]) + .to receive(:visible_relations) + .with(current_user) + .and_return(scope) + allow(scope) + .to receive(:includes) + .and_return([relation]) end it 'embeds a collection' do diff --git a/spec/models/queries/relations/relation_query_spec.rb b/spec/models/queries/relations/relation_query_spec.rb index 07bebdf8152b..9d2c3a85b809 100644 --- a/spec/models/queries/relations/relation_query_spec.rb +++ b/spec/models/queries/relations/relation_query_spec.rb @@ -30,7 +30,7 @@ describe Queries::Relations::RelationQuery, type: :model do let(:instance) { described_class.new } - let(:base_scope) { Relation.direct.order(id: :desc) } + let(:base_scope) { Relation.order(id: :desc) } context 'without a filter' do describe '#results' do @@ -55,7 +55,7 @@ it 'is the same as handwriting the query' do expected = base_scope .merge(Relation - .where("relations.follows IN ('1') OR relations.blocks IN ('1')")) + .where("relations.relation_type IN ('follows','blocks')")) .visible expect(instance.results.to_sql).to eql expected.to_sql diff --git a/spec/models/queries/work_packages/filter/blocked_filter_spec.rb b/spec/models/queries/work_packages/filter/blocked_filter_spec.rb index bb9affa5e897..620f7b41798a 100644 --- a/spec/models/queries/work_packages/filter/blocked_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/blocked_filter_spec.rb @@ -33,7 +33,7 @@ let(:class_key) { :blocked } it_behaves_like 'filter for relation' do - let(:relation_type) { :blocked_by } + let(:relation_type) { :blocked } end end end diff --git a/spec/models/queries/work_packages/filter/duplicated_filter_spec.rb b/spec/models/queries/work_packages/filter/duplicated_filter_spec.rb index 4d933c543c1c..c1eac61c6e4d 100644 --- a/spec/models/queries/work_packages/filter/duplicated_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/duplicated_filter_spec.rb @@ -33,10 +33,7 @@ let(:class_key) { :duplicated } it_behaves_like 'filter for relation' do - # WP filter forward id duplicated, backwards is duplicates! - # 1 -- duplicates --> 2 - # then to_id: 2, type :duplicated - let(:relation_type) { :duplicates } + let(:relation_type) { :duplicated } end end end diff --git a/spec/models/queries/work_packages/filter/duplicates_filter_spec.rb b/spec/models/queries/work_packages/filter/duplicates_filter_spec.rb index 6bb617e263b7..62bea3642dbc 100644 --- a/spec/models/queries/work_packages/filter/duplicates_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/duplicates_filter_spec.rb @@ -33,10 +33,7 @@ let(:class_key) { :duplicates } it_behaves_like 'filter for relation' do - # WP filter forward id duplicated, backwards is duplicates! - # 1 -- duplicates --> 2 - # then to_id: 2, type :duplicated - let(:relation_type) { :duplicated } + let(:relation_type) { :duplicates } end end end diff --git a/spec/models/queries/work_packages/filter/partof_filter_spec.rb b/spec/models/queries/work_packages/filter/partof_filter_spec.rb index 676f9f36e06d..4896739e787c 100644 --- a/spec/models/queries/work_packages/filter/partof_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/partof_filter_spec.rb @@ -33,7 +33,7 @@ let(:class_key) { :partof } it_behaves_like 'filter for relation' do - let(:relation_type) { :part_of } + let(:relation_type) { :partof } end end end diff --git a/spec/models/queries/work_packages/filter/relates_filter_spec.rb b/spec/models/queries/work_packages/filter/relates_filter_spec.rb index 513282a90e8a..11aa901144de 100644 --- a/spec/models/queries/work_packages/filter/relates_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/relates_filter_spec.rb @@ -33,7 +33,7 @@ let(:class_key) { :relates } it_behaves_like 'filter for relation' do - let(:relation_type) { :related_to } + let(:relation_type) { :relates } end end end diff --git a/spec/models/queries/work_packages/filter/required_filter_spec.rb b/spec/models/queries/work_packages/filter/required_filter_spec.rb index ec3a457c5709..3d7db1b455ce 100644 --- a/spec/models/queries/work_packages/filter/required_filter_spec.rb +++ b/spec/models/queries/work_packages/filter/required_filter_spec.rb @@ -33,7 +33,7 @@ let(:class_key) { :required } it_behaves_like 'filter for relation' do - let(:relation_type) { :required_by } + let(:relation_type) { :required } end end end diff --git a/spec/models/query/results_spec.rb b/spec/models/query/results_spec.rb index 8da8901b0e10..e46033b0f5de 100644 --- a/spec/models/query/results_spec.rb +++ b/spec/models/query/results_spec.rb @@ -453,7 +453,7 @@ before do login_as(user_1) - wp_p1[1].precedes << wp_p1[0] + create(:follows_relation, to: wp_p1[1], from: wp_p1[0]) query.add_filter('precedes', '=', [wp_p1[0].id.to_s]) diff --git a/spec/models/relation_spec.rb b/spec/models/relation_spec.rb index ac3e5bd42566..9f6abb85acb7 100644 --- a/spec/models/relation_spec.rb +++ b/spec/models/relation_spec.rb @@ -36,7 +36,6 @@ describe 'all relation types' do Relation::TYPES.each do |key, type_hash| let(:type) { key } - let(:column_name) { type_hash[:sym] } let(:reversed) { type_hash[:reverse] } before do @@ -50,86 +49,15 @@ expect(relation.relation_type).to eq(reversed) end end - - it "sets the correct column for '#{key}' to 1" do - expect(relation.send(column_name)) - .to eql 1 - end - end - end - - describe '#relation_type' do - Relation::TYPES.each do |key, type_hash| - let(:column_name) { type_hash[:sym] } - let(:type) { key } - let(:reversed) { type_hash[:reverse] } - let(:relation) do - build_stubbed(:relation, - relation_type: nil, - column_name => column_count) - end - - context 'with the column set to 1' do - let(:column_count) { 1 } - - it 'deduces the name from the column' do - if reversed.nil? - expect(relation.relation_type).to eq(type) - else - expect(relation.relation_type).to eq(reversed) - end - end - end - - context 'with the column set to 2' do - let(:column_count) { 2 } - - it 'deduces the name from the column' do - if reversed.nil? - expect(relation.relation_type).to eq(type) - else - expect(relation.relation_type).to eq(reversed) - end - end - end - - context 'with the column set to 1 and another column also set to 1' do - let(:column_count) { 1 } - let(:other_column) do - if type == Relation::TYPE_RELATES - Relation::TYPE_DUPLICATES - else - Relation::TYPE_RELATES - end - end - let(:relation) do - build_stubbed(:relation, - relation_type: nil, - column_name => 1, - other_column => 1) - end - - it 'is "mixed"' do - expect(relation.relation_type) - .to eql 'mixed' - end - end end end - describe '#relation_type=' do + describe '#relation_type= / #relation_type' do let(:type) { Relation::TYPE_RELATES } - it 'updates the column value' do - relation.save! - expect(relation.relates).to eq 1 - - relation.relation_type = 'duplicates' - relation.save! - expect(relation.relation_type).to eq('duplicates') - - expect(relation.relates).to eq 0 - expect(relation.duplicates).to eq 1 + it 'sets the type' do + relation.relation_type = Relation::TYPE_BLOCKS + expect(relation.relation_type).to eq(Relation::TYPE_BLOCKS) end end @@ -171,41 +99,32 @@ end end - describe 'it should validate circular dependency' do - let(:otherwp) { create(:work_package) } - let(:relation) do - build(:relation, from: from, to: to, relation_type: Relation::TYPE_PRECEDES) - end - let(:relation2) do - build(:relation, from: to, to: otherwp, relation_type: Relation::TYPE_PRECEDES) - end + describe '#follows?' do + context 'for a follows relation' do + let(:type) { Relation::TYPE_FOLLOWS } - let(:invalid_precedes_relation) do - build(:relation, from: otherwp, to: from, relation_type: Relation::TYPE_PRECEDES) + it 'is truthy' do + expect(relation) + .to be_follows + end end - let(:invalid_follows_relation) do - build(:relation, from: from, to: otherwp, relation_type: Relation::TYPE_FOLLOWS) - end + context 'for a precedes relation' do + let(:type) { Relation::TYPE_PRECEDES } - it 'prevents invalid precedes relations' do - expect(relation.save).to eq(true) - expect(relation2.save).to eq(true) - from.reload - to.reload - otherwp.reload - expect(invalid_precedes_relation.save).to eq(false) - expect(invalid_precedes_relation.errors[:base]).not_to be_empty + it 'is truthy' do + expect(relation) + .to be_follows + end end - it 'prevents invalid follows relations' do - expect(relation.save).to eq(true) - expect(relation2.save).to eq(true) - from.reload - to.reload - otherwp.reload - expect(invalid_follows_relation.save).to eq(false) - expect(invalid_follows_relation.errors[:base]).not_to be_empty + context 'for a blocks relation' do + let(:type) { Relation::TYPE_BLOCKS } + + it 'is falsey' do + expect(relation) + .not_to be_follows + end end end end diff --git a/spec/models/work_packages/scopes/for_scheduling_spec.rb b/spec/models/work_packages/scopes/for_scheduling_spec.rb index fcf5845ee970..1326a8438405 100644 --- a/spec/models/work_packages/scopes/for_scheduling_spec.rb +++ b/spec/models/work_packages/scopes/for_scheduling_spec.rb @@ -28,6 +28,7 @@ require 'spec_helper' +# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::Scopes::ForScheduling, 'allowed scope' do let(:project) { create(:project) } let(:origin) { create(:work_package, project: project) } @@ -38,12 +39,12 @@ end let(:parent) do create(:work_package, project: project).tap do |par| - create(:hierarchy_relation, from: par, to: origin) + origin.update(parent: par) end end let(:grandparent) do create(:work_package, project: project).tap do |grand| - create(:hierarchy_relation, from: grand, to: parent) + parent.update(parent: grand) end end let(:successor) do @@ -58,18 +59,17 @@ end let(:successor_parent) do create(:work_package, project: project).tap do |par| - create(:hierarchy_relation, from: par, to: successor) + successor.update(parent: par) end end let(:successor_child) do - create(:work_package, project: project).tap do |chi| - create(:hierarchy_relation, from: successor, to: chi) - end + create(:work_package, project: project, parent: successor) + end + let(:successor_grandchild) do + create(:work_package, project: project, parent: successor_child) end let(:successor_child2) do - create(:work_package, project: project).tap do |chi| - create(:hierarchy_relation, from: successor, to: chi) - end + create(:work_package, project: project, parent: successor) end let(:successor_successor) do create(:work_package, project: project).tap do |suc| @@ -83,13 +83,11 @@ end let(:parent_successor_parent) do create(:work_package, project: project).tap do |par| - create(:hierarchy_relation, from: par, to: parent_successor) + parent_successor.update(parent: par) end end let(:parent_successor_child) do - create(:work_package, project: project).tap do |chi| - create(:hierarchy_relation, from: parent_successor, to: chi) - end + create(:work_package, project: project, parent: parent_successor) end let(:blocker) do create(:work_package, project: project).tap do |blo| @@ -211,7 +209,7 @@ let!(:existing_work_packages) { [successor, successor_child, successor_parent, successor_successor] } before do - create(:hierarchy_relation, from: successor_parent, to: successor_successor) + successor_successor.update(parent: successor_parent) end context 'with all scheduled automatically' do @@ -421,5 +419,90 @@ end end end + + context 'for a work package with a successor that has a child and grandchild' do + let!(:existing_work_packages) { [successor, successor_child, successor_grandchild] } + + context 'with all scheduled automatically' do + it 'consists of both successors' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array([successor, successor_child, successor_grandchild]) + end + end + + context 'with the successor\'s child scheduled manually' do + before do + successor_child.update_column(:schedule_manually, true) + end + + it 'contains the successor' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array [successor] + end + end + end + + context 'for a work package with a successor that has a child and two grandchildren' do + let(:successor_grandchild2) do + create(:work_package, project: project, parent: successor_child) + end + + let!(:existing_work_packages) { [successor, successor_child, successor_grandchild, successor_grandchild2] } + + context 'with all scheduled automatically' do + it 'consists of the successor with its descendants' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array([successor, successor_child, successor_grandchild, successor_grandchild2]) + end + end + + context 'with one of the successor\'s grandchildren scheduled manually' do + before do + successor_grandchild.update_column(:schedule_manually, true) + end + + it 'contains the successor and the non automatically scheduled descendants' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array([successor, successor_child, successor_grandchild2]) + end + end + + context 'with both of the successor\'s grandchildren scheduled manually' do + before do + successor_grandchild.update_column(:schedule_manually, true) + successor_grandchild2.update_column(:schedule_manually, true) + end + + it 'includes successor' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array([successor]) + end + end + + context 'with both of the successor\'s grandchildren and child scheduled manually' do + before do + successor_child.update_column(:schedule_manually, true) + successor_grandchild.update_column(:schedule_manually, true) + successor_grandchild2.update_column(:schedule_manually, true) + end + + it 'is empty' do + expect(WorkPackage.for_scheduling([origin])) + .to be_empty + end + end + + context 'with the successor\'s child scheduled manually' do + before do + successor_child.update_column(:schedule_manually, true) + end + + it 'contains the successor' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array [successor] + end + end + end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/models/work_packages/scopes/relatable_spec.rb b/spec/models/work_packages/scopes/relatable_spec.rb new file mode 100644 index 000000000000..7bde9fa7188b --- /dev/null +++ b/spec/models/work_packages/scopes/relatable_spec.rb @@ -0,0 +1,323 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. + +require 'spec_helper' + +# rubocop:disable RSpec/MultipleMemoizedHelpers +describe WorkPackages::Scopes::Relatable, '.relatable scope' do + let(:project) { create(:project) } + let(:origin) { create(:work_package, project: project) } + let(:unrelated_work_package) { create(:work_package, project: project) } + + let(:directly_related_work_package) do + create(:work_package, project: project).tap do |related_wp| + create(:relation, + relation_type: directly_related_work_package_type, + from: origin, + to: related_wp) + end + end + let(:directly_related_work_package_type) { relation_type } + let(:transitively_related_work_package) do + create(:work_package, project: project).tap do |related_wp| + create(:relation, + relation_type: transitively_related_work_package_type, + from: directly_related_work_package, + to: related_wp) + end + end + let(:transitively_related_work_package_type) { relation_type } + + let(:parent) do + create(:work_package, project: project).tap do |p| + origin.update(parent: p) + end + end + let(:sibling) do + create(:work_package, project: project, parent: parent) + end + let(:grandparent) do + create(:work_package, project: project).tap do |p| + parent.update(parent: p) + end + end + let(:aunt) do + create(:work_package, project: project, parent: grandparent) + end + let(:origin_child) do + create(:work_package, project: project, parent: origin) + end + let(:existing_work_packages) { [] } + + let(:relation_type) { Relation::TYPE_FOLLOWS } + + subject(:relatable) { WorkPackage.relatable(origin, relation_type) } + + it 'is an AR scope' do + expect(relatable) + .to be_a ActiveRecord::Relation + end + + context 'for an unpersisted work package' do + let(:origin) { WorkPackage.new } + let!(:existing_work_packages) { [unrelated_work_package] } + + it 'contains every other work package' do + expect(relatable) + .to match_array([unrelated_work_package]) + end + end + + context 'with a completely unrelated work package' do + let!(:existing_work_packages) { [unrelated_work_package] } + + Relation::TYPES.each_key do |current_type| + context "for the '#{current_type}' type" do + let(:relation_type) { current_type } + + it 'contains the unrelated_work_package' do + expect(relatable) + .to match_array([unrelated_work_package]) + end + end + + context "for the '#{current_type}' type with the other project being in a different project "\ + "and having cross project relations disabled", with_settings: { cross_project_work_package_relations: false } do + let(:relation_type) { current_type } + let(:unrelated_work_package) { create(:work_package, project: create(:project)) } + + it 'contains the unrelated_work_package' do + expect(relatable) + .to be_empty + end + end + + context "for the '#{current_type}' type with the other project being in a different project "\ + "and having cross project relations enabled", with_settings: { cross_project_work_package_relations: true } do + let(:relation_type) { current_type } + let(:unrelated_work_package) { create(:work_package, project: create(:project)) } + + it 'contains the unrelated_work_package' do + expect(relatable) + .to match_array([unrelated_work_package]) + end + end + end + + context "for the '#{Relation::TYPE_PARENT}'" do + let(:relation_type) { Relation::TYPE_PARENT } + + it 'contains the unrelated_work_package' do + expect(relatable) + .to match_array([unrelated_work_package]) + end + end + end + + context 'with a directly related work package' do + let!(:existing_work_packages) { [directly_related_work_package] } + + Relation::TYPES.each_key do |current_type| + context "with the existing relation and the queried being '#{current_type}' typed" do + let(:relation_type) { current_type } + + it 'is empty' do + expect(relatable) + .to be_empty + end + end + end + end + + context 'with a parent and a sibling' do + let!(:existing_work_packages) { [parent, sibling] } + + Relation::TYPES.each_key do |current_type| + context "for the '#{current_type}' type" do + let(:relation_type) { current_type } + + it 'contains the sibling' do + expect(relatable) + .to match_array([sibling]) + end + end + end + end + + context 'with a transitively related work package' do + let!(:existing_work_packages) { [directly_related_work_package, transitively_related_work_package] } + + context "for a 'follows' relation and the existing relations being in the same direction" do + it 'contains the transitively related work package' do + expect(relatable) + .to match_array([transitively_related_work_package]) + end + end + + context "for a 'follows' relation and the existing relations being in the opposite direction" do + let(:directly_related_work_package_type) { Relation::TYPE_PRECEDES } + let(:transitively_related_work_package_type) { Relation::TYPE_PRECEDES } + + it 'is empty' do + expect(relatable) + .to be_empty + end + end + + context "for a 'precedes' relation and the existing relations being in the opposite direction" do + let(:relation_type) { Relation::TYPE_PRECEDES } + let(:directly_related_work_package_type) { Relation::TYPE_FOLLOWS } + let(:transitively_related_work_package_type) { Relation::TYPE_FOLLOWS } + + it 'is empty' do + expect(relatable) + .to be_empty + end + end + + context "for a 'related' relation and the existing relations being in the opposite direction" do + let(:relation_type) { Relation::TYPE_RELATES } + let(:directly_related_work_package_type) { Relation::TYPE_RELATES } + let(:transitively_related_work_package_type) { Relation::TYPE_RELATES } + + # Switching the origin for transitively_related_work_package here since it would be more + # complicated to switch around the relations + subject(:relatable) { WorkPackage.relatable(transitively_related_work_package, relation_type) } + + it 'is empty' do + expect(relatable) + .to match_array [origin] + end + end + end + + context "with a child" do + let!(:existing_work_packages) { [origin_child] } + + Relation::TYPES.each_key do |current_type| + context "for a '#{current_type}' type" do + let(:relation_type) { current_type } + + it 'is empty' do + expect(relatable) + .to be_empty + end + end + end + end + + context "with two parent child pairs connected by a relation" do + let(:other_parent) do + create(:work_package, project: project) + end + let(:other_child) do + create(:work_package, project: project, parent: other_parent).tap do |wp| + create(:relation, from: wp, to: origin_child, relation_type: other_relation_type) + end + end + let!(:existing_work_packages) { [origin_child, other_parent, other_child] } + + context "for a 'follows' and the existing relation being a follows in the opposite direction" do + let(:other_relation_type) { Relation::TYPE_FOLLOWS } + let(:relation_type) { Relation::TYPE_FOLLOWS } + + it 'is empty' do + expect(relatable) + .to be_empty + end + end + + context "for a 'follows' and the existing relation being a follows in the same direction" do + # Using precedes will lead to the relation being reversed + let(:other_relation_type) { Relation::TYPE_PRECEDES } + let(:relation_type) { Relation::TYPE_FOLLOWS } + + it 'contains the work packages in the other hierarchy' do + expect(relatable) + .to match_array [other_parent, other_child] + end + end + end + + context 'with a child, parent, grandparent and aunt' do + let!(:existing_work_packages) { [origin, origin_child, parent, grandparent, aunt] } + + context "for a 'parent' relation" do + let(:relation_type) { Relation::TYPE_PARENT } + + it 'contains grandparent and aunt' do + expect(relatable) + .to match_array [grandparent, aunt] + end + end + + context "for a 'parent' relation with a follows relation between child and aunt" do + let(:relation_type) { Relation::TYPE_PARENT } + + before do + create(:follows_relation, from: origin_child, to: aunt) + end + + it 'contains grandparent' do + expect(relatable) + .to match_array [grandparent] + end + end + + context "for a 'relates' relation with a follows relation between child and aunt" do + let(:relation_type) { Relation::TYPE_RELATES } + + before do + create(:follows_relation, from: origin_child, to: aunt) + end + + it 'contains aunt' do + expect(relatable) + .to match_array [aunt] + end + end + end + + context 'with an ancestor chain of 3 work packages' do + let(:grand_grandparent) do + create(:work_package, project: project).tap do |par| + grandparent.update(parent: par) + end + end + + let!(:existing_work_packages) { [parent, grandparent, grand_grandparent] } + + context "for a 'parent' relation" do + let(:relation_type) { Relation::TYPE_PARENT } + + it 'contains grandparent and grand_grandparent' do + expect(relatable) + .to match_array [grandparent, grand_grandparent] + end + end + end +end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/requests/api/v3/relations/relations_api_spec.rb b/spec/requests/api/v3/relations/relations_api_spec.rb index 6b29fff9e4bf..dbcb8a6733fa 100644 --- a/spec/requests/api/v3/relations/relations_api_spec.rb +++ b/spec/requests/api/v3/relations/relations_api_spec.rb @@ -82,26 +82,27 @@ end end - let(:setup) {} + let(:setup) do + # empty to be overwritten + end + before do setup - header "Content-Type", "application/json" - post "/api/v3/work_packages/#{from.id}/relations", params.to_json + post api_v3_paths.work_package_relations(from.id), params.to_json end - it 'should return 201 (created)' do + it 'returns 201 (created)' do expect(last_response.status).to eq(201) end - it 'should have created a new relation' do - # reflexive relations + created one - expect(Relation.count).to eq 3 + it 'has created a new relation' do + expect(Relation.count).to eq 1 end it_behaves_like 'creates the relation' - context 'relation that would create a circular scheduling dependency' do + context 'when the relation would create a circular scheduling dependency' do let(:from_child) do create(:work_package, parent: from) end @@ -130,7 +131,7 @@ end end - context "'relates to' relation that would create a circular dependency" do + context "with a 'relates to' relation that creates a circular dependency" do let(:work_package_a) { create(:work_package) } let(:work_package_b) { create(:work_package, project: work_package_a.project) } let(:work_package_c) { create(:work_package, project: work_package_b.project) } @@ -157,18 +158,18 @@ relation_b_c end - it 'returns 201 (created) and creates the relation with an inverted direction' do + it 'returns 201 (created) and creates the relation' do expect(last_response.status) .to eq(201) - expect(Relation.direct.count).to eq 3 + expect(Relation.count).to eq 3 - new_relation = Relation.direct.last + new_relation = Relation.last - expect(new_relation.to) + expect(new_relation.from) .to eql work_package_c - expect(new_relation.from) + expect(new_relation.to) .to eql work_package_a end end @@ -237,11 +238,10 @@ before do relation - header "Content-Type", "application/json" - patch "/api/v3/relations/#{relation.id}", update.to_json + patch api_v3_paths.relation(relation.id), update.to_json end - it "should return 200 (ok)" do + it "returns 200 (ok)" do expect(last_response.status).to eq 200 end @@ -253,7 +253,7 @@ expect(relation.reload.delay).to eq new_delay end - it "should return the updated relation" do + it "returns the updated relation" do rel = ::API::V3::Relations::RelationPayloadRepresenter.new(Relation.new, current_user: user).from_json last_response.body expect(rel).to eq relation.reload @@ -266,11 +266,11 @@ } end - it "should return 422" do + it "returns 422" do expect(last_response.status).to eq 422 end - it "should indicate an error with the type attribute" do + it "indicates an error with the type attribute" do attr = JSON.parse(last_response.body).dig "_embedded", "details", "attribute" expect(attr).to eq "type" @@ -290,17 +290,17 @@ } end - it "should return 422" do + it "returns 422" do expect(last_response.status).to eq 422 end - it "should indicate an error with the `from` attribute" do + it "indicates an error with the `from` attribute" do attr = JSON.parse(last_response.body).dig "_embedded", "details", "attribute" expect(attr).to eq "from" end - it "should let the user know the attribute is read-only" do + it "lets the user know the attribute is read-only" do msg = JSON.parse(last_response.body)["message"] expect(msg).to include "Work package an existing relation's `from` link is immutable" @@ -348,17 +348,17 @@ context "without manage_work_package_relations" do let!(:to) { create :work_package } - it "should return 422" do + it "returns 422" do expect(last_response.status).to eq 422 end - it "should indicate an error with the `to` attribute" do + it "indicates an error with the `to` attribute" do attr = JSON.parse(last_response.body).dig "_embedded", "details", "attribute" expect(attr).to eq "to" end - it "should have a localized error message" do + it "has a localized error message" do message = JSON.parse(last_response.body)["message"] expect(message).not_to include "translation missing" @@ -391,7 +391,7 @@ delete path end - it "should return 204 and destroy the relation" do + it "returns 204 and destroy the relation" do expect(last_response.status).to eq 204 expect(Relation.exists?(relation.id)).to be_falsey end diff --git a/spec/requests/api/v3/work_packages/available_relation_candidates_spec.rb b/spec/requests/api/v3/work_packages/available_relation_candidates_resource_spec.rb similarity index 96% rename from spec/requests/api/v3/work_packages/available_relation_candidates_spec.rb rename to spec/requests/api/v3/work_packages/available_relation_candidates_resource_spec.rb index e09e260979ea..931a5ccc224e 100644 --- a/spec/requests/api/v3/work_packages/available_relation_candidates_spec.rb +++ b/spec/requests/api/v3/work_packages/available_relation_candidates_resource_spec.rb @@ -28,7 +28,7 @@ require 'spec_helper' -describe ::API::V3::Relations::RelationRepresenter, type: :request do +describe ::API::V3::WorkPackages::AvailableRelationCandidatesAPI, type: :request do let(:user) { create :admin } let(:project_1) { create :project } @@ -141,8 +141,8 @@ def work_packages end context 'for a follows relationship' do - it 'does not contain the work packages with which a relationship already exists' do - expect(subjects).to match_array ["WP 1.2", "WP 1.2.1", "WP 2.1"] + it 'does not contain the work packages with which a relationship already exists but the parent' do + expect(subjects).to match_array ["WP 1", "WP 1.2", "WP 1.2.1", "WP 2.1"] end end diff --git a/spec/requests/api/v3/work_packages/show_resource_spec.rb b/spec/requests/api/v3/work_packages/show_resource_spec.rb index b5af92218c30..c9650df2a4d4 100644 --- a/spec/requests/api/v3/work_packages/show_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/show_resource_spec.rb @@ -161,8 +161,8 @@ create(:work_package, project_id: project.id, description: 'lorem ipsum').tap do |wp| - create(:relation, relates: 1, from: wp, to: directly_related_wp) - create(:relation, relates: 1, from: directly_related_wp, to: transitively_related_wp) + create(:relation, relation_type: Relation::TYPE_RELATES, from: wp, to: directly_related_wp) + create(:relation, relation_type: Relation::TYPE_RELATES, from: directly_related_wp, to: transitively_related_wp) end end diff --git a/spec/services/projects/copy_service_integration_spec.rb b/spec/services/projects/copy_service_integration_spec.rb index 5c8df5800e64..a06fc8571871 100644 --- a/spec/services/projects/copy_service_integration_spec.rb +++ b/spec/services/projects/copy_service_integration_spec.rb @@ -232,7 +232,7 @@ let(:only_args) { %w[work_packages] } - it 'should the relations relations' do + it 'copies relations' do expect(subject).to be_success expect(source.work_packages.count).to eq(project_copy.work_packages.count) @@ -241,14 +241,14 @@ # First issue with a relation on project # copied relation + reflexive relation - expect(copied_wp.relations.direct.count).to eq 2 - relates_relation = copied_wp.relations.direct.find { |r| r.relation_type == 'relates' } + expect(copied_wp.relations.count).to eq 2 + relates_relation = copied_wp.relations.find { |r| r.relation_type == 'relates' } expect(relates_relation.from_id).to eq copied_wp.id expect(relates_relation.to_id).to eq copied_wp_2.id # Second issue with a cross project relation # copied relation + reflexive relation - duplicates_relation = copied_wp.relations.direct.find { |r| r.relation_type == 'duplicates' } + duplicates_relation = copied_wp.relations.find { |r| r.relation_type == 'duplicates' } expect(duplicates_relation.from_id).to eq copied_wp.id expect(duplicates_relation.to_id).to eq other_wp.id end diff --git a/spec/services/work_packages/delete_service_spec.rb b/spec/services/work_packages/delete_service_spec.rb index a690263af7f9..e7fba80b1e24 100644 --- a/spec/services/work_packages/delete_service_spec.rb +++ b/spec/services/work_packages/delete_service_spec.rb @@ -45,7 +45,7 @@ subject { instance.call } before do - expect(work_package) + allow(work_package) .to receive(:reload) .and_return(work_package) diff --git a/spec/services/work_packages/set_schedule_service_spec.rb b/spec/services/work_packages/set_schedule_service_spec.rb index 401c785a519f..70d3b4bfd5aa 100644 --- a/spec/services/work_packages/set_schedule_service_spec.rb +++ b/spec/services/work_packages/set_schedule_service_spec.rb @@ -28,13 +28,14 @@ require 'spec_helper' +# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::SetScheduleService do let(:work_package) do build_stubbed(:stubbed_work_package, start_date: work_package_start_date, due_date: work_package_due_date) end - let(:work_package_due_date) { Date.today } + let(:work_package_due_date) { Time.zone.today } let(:work_package_start_date) { nil } let(:instance) do described_class.new(user: user, work_package: work_package) @@ -43,65 +44,29 @@ let(:user) { build_stubbed(:user) } let(:type) { build_stubbed(:type) } - def stub_follower(start_date, due_date, predecessors) - work_package = build_stubbed(:stubbed_work_package, - type: type, - start_date: start_date, - due_date: due_date) - - relations = predecessors.map do |predecessor, delay| - build_stubbed(:follows_relation, - delay: delay, - from: work_package, - to: predecessor) - end - - allow(work_package) - .to receive(:follows_relations) - .and_return relations - - work_package - end - - def stub_follower_child(parent, start, due) - child = stub_follower(start, - due, - {}) - - relation = build_stubbed(:hierarchy_relation, - from: parent, - to: child) - - allow(child) - .to receive(:parent_relation) - .and_return relation - - child - end - - let(:follower1_start_date) { Date.today + 1.day } - let(:follower1_due_date) { Date.today + 3.day } + let(:follower1_start_date) { Time.zone.today + 1.day } + let(:follower1_due_date) { Time.zone.today + 3.days } let(:follower1_delay) { 0 } let(:following_work_package1) do stub_follower(follower1_start_date, follower1_due_date, - work_package => follower1_delay) + { work_package => follower1_delay }) end - let(:follower2_start_date) { Date.today + 4.day } - let(:follower2_due_date) { Date.today + 8.day } + let(:follower2_start_date) { Time.zone.today + 4.days } + let(:follower2_due_date) { Time.zone.today + 8.days } let(:follower2_delay) { 0 } let(:following_work_package2) do stub_follower(follower2_start_date, follower2_due_date, - following_work_package1 => follower2_delay) + { following_work_package1 => follower2_delay }) end - let(:follower3_start_date) { Date.today + 9.day } - let(:follower3_due_date) { Date.today + 10.day } + let(:follower3_start_date) { Time.zone.today + 9.days } + let(:follower3_due_date) { Time.zone.today + 10.days } let(:follower3_delay) { 0 } let(:following_work_package3) do stub_follower(follower3_start_date, follower3_due_date, - following_work_package2 => follower3_delay) + { following_work_package2 => follower3_delay }) end let(:parent_follower1_start_date) { follower1_start_date } @@ -112,31 +77,46 @@ def stub_follower_child(parent, start, due) parent_follower1_due_date, {}) - relation = build_stubbed(:hierarchy_relation, - from: work_package, - to: following_work_package1) - - allow(following_work_package1) - .to receive(:parent_relation) - .and_return relation + following_work_package1.parent = work_package work_package end let(:follower_sibling_work_package) do - sibling = stub_follower(follower1_due_date + 2.days, - follower1_due_date + 4.days, - {}) + stub_follower(follower1_due_date + 2.days, + follower1_due_date + 4.days, + {}, + parent: parent_following_work_package1) + end + + let(:attributes) { [:start_date] } + + def stub_follower(start_date, due_date, predecessors, parent: nil) + work_package = build_stubbed(:stubbed_work_package, + type: type, + start_date: start_date, + due_date: due_date, + parent: parent) + + relations = predecessors.map do |predecessor, delay| + build_stubbed(:follows_relation, + delay: delay, + from: work_package, + to: predecessor) + end - relation = build_stubbed(:hierarchy_relation, - from: parent_following_work_package1, - to: sibling) + allow(work_package) + .to receive(:follows_relations) + .and_return relations - allow(sibling) - .to receive(:parent_relation) - .and_return relation + work_package + end - sibling + def stub_follower_child(parent, start, due) + stub_follower(start, + due, + {}, + parent: parent) end subject { instance.call(attributes) } @@ -151,7 +131,6 @@ def stub_follower_child(parent, start, due) .to receive(:includes) .and_return(following) end - let(:attributes) { [:start_date] } shared_examples_for 'reschedules' do before do @@ -198,14 +177,14 @@ def stub_follower_child(parent, start, due) [following_work_package1] end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.day] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end end @@ -216,7 +195,7 @@ def stub_follower_child(parent, start, due) it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, nil] } + { following_work_package1 => [Time.zone.today + 6.days, nil] } end end end @@ -226,33 +205,33 @@ def stub_follower_child(parent, start, due) it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 7.day] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 7.days] } end end end end - context 'moving forward with the follower having some space left' do - let(:follower1_start_date) { Date.today + 3.day } - let(:follower1_due_date) { Date.today + 5.day } + context 'when moving forward with the follower having some space left' do + let(:follower1_start_date) { Time.zone.today + 3.days } + let(:follower1_due_date) { Time.zone.today + 5.days } before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.day] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end end end - context 'moving forward with the follower having enough space left to not be moved at all' do - let(:follower1_start_date) { Date.today + 10.day } - let(:follower1_due_date) { Date.today + 12.day } + context 'when moving forward with the follower having enough space left to not be moved at all' do + let(:follower1_start_date) { Time.zone.today + 10.days } + let(:follower1_due_date) { Time.zone.today + 12.days } before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do @@ -265,33 +244,33 @@ def stub_follower_child(parent, start, due) end end - context 'moving forward with the follower having some space left and a delay' do - let(:follower1_start_date) { Date.today + 5.day } - let(:follower1_due_date) { Date.today + 7.day } + context 'when moving forward with the follower having some space left and a delay' do + let(:follower1_start_date) { Time.zone.today + 5.days } + let(:follower1_due_date) { Time.zone.today + 7.days } let(:follower1_delay) { 3 } before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 9.days, Date.today + 11.day] } + { following_work_package1 => [Time.zone.today + 9.days, Time.zone.today + 11.days] } end end end - context 'moving forward with the follower not needing to be moved' do - let(:follower1_start_date) { Date.today + 6.day } - let(:follower1_due_date) { Date.today + 8.day } + context 'when moving forward with the follower not needing to be moved' do + let(:follower1_start_date) { Time.zone.today + 6.days } + let(:follower1_due_date) { Time.zone.today + 8.days } before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.day] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end let(:unchanged) do [following_work_package1] @@ -299,34 +278,34 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards' do + context 'when moving backwards' do before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.day] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days] } end end end - context 'moving backwards with space between' do - let(:follower1_start_date) { Date.today + 3.day } - let(:follower1_due_date) { Date.today + 5.day } + context 'when moving backwards with space between' do + let(:follower1_start_date) { Time.zone.today + 3.days } + let(:follower1_due_date) { Time.zone.today + 5.days } before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 2.days, Date.today] } + { following_work_package1 => [Time.zone.today - 2.days, Time.zone.today] } end end end - context 'moving backwards with the follower having another relation limiting movement' do + context 'when moving backwards with the follower having another relation limiting movement' do let(:other_work_package) do build_stubbed(:stubbed_work_package, type: type, @@ -352,12 +331,12 @@ def stub_follower_child(parent, start, due) .to receive(:follows_relations) .and_return [other_follow_relation, follow_relation] - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today, Date.today + 2.days], + { following_work_package1 => [Time.zone.today, Time.zone.today + 2.days], other_work_package => [follower1_start_date - 8.days, follower1_start_date - 5.days] } end let(:unchanged) do @@ -366,21 +345,21 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards with the follower having no start date (which should not happen)' do + context 'when moving backwards with the follower having no start date (which should not happen)' do let(:follower1_start_date) { nil } before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, follower1_due_date] } + { following_work_package1 => [Time.zone.today - 4.days, follower1_due_date] } end end end - context 'removing the dates on the predecessor' do + context 'when removing the dates on the predecessor' do before do work_package.start_date = work_package.due_date = nil end @@ -396,7 +375,7 @@ def stub_follower_child(parent, start, due) context 'when the follower has no start date but a due date' do let(:follower1_start_date) { nil } - let(:follower1_due_date) { Date.today + 15.days } + let(:follower1_due_date) { Time.zone.today + 15.days } it_behaves_like 'reschedules' do # Nothing should be rescheduled @@ -407,7 +386,7 @@ def stub_follower_child(parent, start, due) end end - context 'not moving and the successor not having start & due date (e.g. creating relation)' do + context 'when not moving and the successor not having start & due date (e.g. creating relation)' do let(:follower1_start_date) { nil } let(:follower1_due_date) { nil } @@ -418,7 +397,7 @@ def stub_follower_child(parent, start, due) end end - context 'not moving and the successor having start before predecessor due date (e.g. creating relation)' do + context 'when not moving and the successor having start before predecessor due date (e.g. creating relation)' do let(:follower1_start_date) { work_package_due_date - 5.days } let(:follower1_due_date) { nil } @@ -429,18 +408,18 @@ def stub_follower_child(parent, start, due) end end - context 'not moving and the successor having start and due before predecessor due date (e.g. creating relation)' do + context 'when not moving and the successor having start and due before predecessor due date (e.g. creating relation)' do let(:follower1_start_date) { work_package_due_date - 5.days } let(:follower1_due_date) { work_package_due_date - 2.days } it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [work_package.due_date + 1.day, work_package.due_date + 4.day] } + { following_work_package1 => [work_package.due_date + 1.day, work_package.due_date + 4.days] } end end end - context 'not having dates and the successor not having start & due date (e.g. creating relation)' do + context 'when not having dates and the successor not having start & due date (e.g. creating relation)' do let(:work_package_due_date) { nil } let(:follower1_start_date) { nil } let(:follower1_due_date) { nil } @@ -459,8 +438,8 @@ def stub_follower_child(parent, start, due) let(:following_work_package1) do stub_follower(follower1_start_date, follower1_due_date, - work_package => follower1_delay, - another_successor => 0) + { work_package => follower1_delay, + another_successor => 0 }) end let(:another_successor) do build_stubbed(:stubbed_work_package, @@ -468,26 +447,26 @@ def stub_follower_child(parent, start, due) due_date: nil) end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.day] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end end end - context 'moving backwards' do + context 'when moving backwards' do before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.day] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days] } end end end @@ -498,7 +477,7 @@ def stub_follower_child(parent, start, due) let(:parent_work_package) do build_stubbed(:stubbed_work_package) end - let(:work_package_start_date) { Date.today - 5.days } + let(:work_package_start_date) { Time.zone.today - 5.days } let!(:following) do [parent_work_package] end @@ -508,7 +487,6 @@ def stub_follower_child(parent, start, due) allow(parent_work_package) .to receive(:descendants) .and_return([work_package]) - work_package.build_parent_relation from_id: parent_work_package.id end it_behaves_like 'reschedules' do @@ -524,20 +502,20 @@ def stub_follower_child(parent, start, due) parent_following_work_package1] end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - parent_following_work_package1 => [Date.today + 6.days, Date.today + 8.days] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + parent_following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end end end - context 'moving forward with the parent having another child not being moved' do + context 'when moving forward with the parent having another child not being moved' do let(:parent_follower1_start_date) { follower1_start_date } let(:parent_follower1_due_date) { follower1_due_date + 4.days } @@ -548,13 +526,13 @@ def stub_follower_child(parent, start, due) end before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - parent_following_work_package1 => [Date.today + 5.days, Date.today + 8.days], + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + parent_following_work_package1 => [Time.zone.today + 5.days, Time.zone.today + 8.days], follower_sibling_work_package => [follower1_due_date + 2.days, follower1_due_date + 4.days] } end let(:unchanged) do @@ -563,25 +541,25 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards' do + context 'when moving backwards' do before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - parent_following_work_package1 => [Date.today - 4.days, Date.today - 2.days] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + parent_following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days] } end end end - context 'moving backwards with the parent having another relation limiting movement' do + context 'when moving backwards with the parent having another relation limiting movement' do let(:other_work_package) do build_stubbed(:stubbed_work_package, type: type, - start_date: Date.today - 8.days, - due_date: Date.today - 4.days) + start_date: Time.zone.today - 8.days, + due_date: Time.zone.today - 4.days) end let(:other_follow_relation) do @@ -596,14 +574,14 @@ def stub_follower_child(parent, start, due) .to receive(:follows_relations) .and_return [other_follow_relation] - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 1.day, Date.today + 1.day], - parent_following_work_package1 => [Date.today - 1.day, Date.today + 1.day], - other_work_package => [Date.today - 8.days, Date.today - 4.days] } + { following_work_package1 => [Time.zone.today - 1.day, Time.zone.today + 1.day], + parent_following_work_package1 => [Time.zone.today - 1.day, Time.zone.today + 1.day], + other_work_package => [Time.zone.today - 8.days, Time.zone.today - 4.days] } end let(:unchanged) do [other_work_package] @@ -611,12 +589,12 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards with the parent having another relation not limiting movement' do + context 'when moving backwards with the parent having another relation not limiting movement' do let(:other_work_package) do build_stubbed(:stubbed_work_package, type: type, - start_date: Date.today - 10.days, - due_date: Date.today - 9.days) + start_date: Time.zone.today - 10.days, + due_date: Time.zone.today - 9.days) end let(:other_follow_relation) do @@ -631,14 +609,14 @@ def stub_follower_child(parent, start, due) .to receive(:follows_relations) .and_return [other_follow_relation] - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - parent_following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - other_work_package => [Date.today - 10.days, Date.today - 9.days] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + parent_following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + other_work_package => [Time.zone.today - 10.days, Time.zone.today - 9.days] } end let(:unchanged) do [other_work_package] @@ -646,7 +624,7 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards with the parent having another child not being moved' do + context 'when moving backwards with the parent having another child not being moved' do let(:parent_follower1_start_date) { follower1_start_date } let(:parent_follower1_due_date) { follower1_due_date + 4.days } @@ -657,13 +635,13 @@ def stub_follower_child(parent, start, due) end before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - parent_following_work_package1 => [Date.today - 4.days, Date.today + 7.days], + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + parent_following_work_package1 => [Time.zone.today - 4.days, Time.zone.today + 7.days], follower_sibling_work_package => [follower1_due_date + 2.days, follower1_due_date + 4.days] } end let(:unchanged) do @@ -684,15 +662,15 @@ def stub_follower_child(parent, start, due) child_work_package] end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - child_work_package => [Date.today + 6.days, Date.today + 8.days] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + child_work_package => [Time.zone.today + 6.days, Time.zone.today + 8.days] } end end end @@ -721,7 +699,7 @@ def stub_follower_child(parent, start, due) child2_work_package end - context 'unchanged dates (e.g. when creating a follows relation) and successor starting 1 day after scheduled' do + context 'with unchanged dates (e.g. when creating a follows relation) and successor starting 1 day after scheduled' do it_behaves_like 'reschedules' do let(:expected) do { following_work_package1 => [follower1_start_date, follower1_due_date], @@ -734,7 +712,7 @@ def stub_follower_child(parent, start, due) end end - context 'unchanged dates (e.g. when creating a follows relation) and successor starting 3 days after scheduled' do + context 'with unchanged dates (e.g. when creating a follows relation) and successor starting 3 days after scheduled' do let(:follower1_start_date) { work_package_due_date + 3.days } let(:follower1_due_date) { follower1_start_date + 10.days } let(:child1_start_date) { follower1_start_date } @@ -754,7 +732,7 @@ def stub_follower_child(parent, start, due) end end - context 'unchanged dates (e.g. when creating a follows relation) and successor\'s first child' do + context 'with unchanged dates (e.g. when creating a follows relation) and successor\'s first child needs to be rescheduled' do let(:follower1_start_date) { work_package_due_date - 3.days } let(:follower1_due_date) { work_package_due_date + 10.days } let(:child1_start_date) { follower1_start_date } @@ -775,7 +753,7 @@ def stub_follower_child(parent, start, due) end end - context 'unchanged dates (e.g. when creating a follows relation) and successor\s children need to be rescheduled' do + context 'with unchanged dates (e.g. when creating a follows relation) and successor\s children need to be rescheduled' do let(:follower1_start_date) { work_package_due_date - 8.days } let(:follower1_due_date) { work_package_due_date + 10.days } let(:child1_start_date) { follower1_start_date } @@ -795,12 +773,12 @@ def stub_follower_child(parent, start, due) end context 'with a chain of successors' do - let(:follower1_start_date) { Date.today + 1.day } - let(:follower1_due_date) { Date.today + 3.day } - let(:follower2_start_date) { Date.today + 4.day } - let(:follower2_due_date) { Date.today + 8.day } - let(:follower3_start_date) { Date.today + 9.day } - let(:follower3_due_date) { Date.today + 10.day } + let(:follower1_start_date) { Time.zone.today + 1.day } + let(:follower1_due_date) { Time.zone.today + 3.days } + let(:follower2_start_date) { Time.zone.today + 4.days } + let(:follower2_due_date) { Time.zone.today + 8.days } + let(:follower3_start_date) { Time.zone.today + 9.days } + let(:follower3_due_date) { Time.zone.today + 10.days } let!(:following) do [following_work_package1, @@ -808,37 +786,37 @@ def stub_follower_child(parent, start, due) following_work_package3] end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - following_work_package2 => [Date.today + 9.days, Date.today + 13.days], - following_work_package3 => [Date.today + 14.days, Date.today + 15.days] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + following_work_package2 => [Time.zone.today + 9.days, Time.zone.today + 13.days], + following_work_package3 => [Time.zone.today + 14.days, Time.zone.today + 15.days] } end end end - context 'moving forward with some space between the followers' do - let(:follower1_start_date) { Date.today + 1.day } - let(:follower1_due_date) { Date.today + 3.day } - let(:follower2_start_date) { Date.today + 7.day } - let(:follower2_due_date) { Date.today + 10.day } - let(:follower3_start_date) { Date.today + 17.day } - let(:follower3_due_date) { Date.today + 18.day } + context 'when moving forward with some space between the followers' do + let(:follower1_start_date) { Time.zone.today + 1.day } + let(:follower1_due_date) { Time.zone.today + 3.days } + let(:follower2_start_date) { Time.zone.today + 7.days } + let(:follower2_due_date) { Time.zone.today + 10.days } + let(:follower3_start_date) { Time.zone.today + 17.days } + let(:follower3_due_date) { Time.zone.today + 18.days } before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - following_work_package2 => [Date.today + 9.days, Date.today + 12.days], - following_work_package3 => [Date.today + 17.days, Date.today + 18.days] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + following_work_package2 => [Time.zone.today + 9.days, Time.zone.today + 12.days], + following_work_package3 => [Time.zone.today + 17.days, Time.zone.today + 18.days] } end let(:unchanged) do [following_work_package3] @@ -846,39 +824,38 @@ def stub_follower_child(parent, start, due) end end - context 'moving backwards' do + context 'when moving backwards' do before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - following_work_package2 => [Date.today - 1.days, Date.today + 3.days], - following_work_package3 => [Date.today + 4.days, Date.today + 5.days] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + following_work_package2 => [Time.zone.today - 1.day, Time.zone.today + 3.days], + following_work_package3 => [Time.zone.today + 4.days, Time.zone.today + 5.days] } end end end end context 'with a chain of successors with two paths leading to the same work package in the end' do - let(:follower3_start_date) { Date.today + 4.day } - let(:follower3_due_date) { Date.today + 7.day } + let(:follower3_start_date) { Time.zone.today + 4.days } + let(:follower3_due_date) { Time.zone.today + 7.days } let(:follower3_delay) { 0 } let(:following_work_package3) do stub_follower(follower3_start_date, follower3_due_date, - work_package => follower3_delay) + { work_package => follower3_delay }) end - let(:follower4_start_date) { Date.today + 9.days } - let(:follower4_due_date) { Date.today + 10.days } - let(:follower4_delay_2) { 0 } - let(:follower4_delay_3) { 0 } + let(:follower4_start_date) { Time.zone.today + 9.days } + let(:follower4_due_date) { Time.zone.today + 10.days } + let(:follower4_delay2) { 0 } + let(:follower4_delay3) { 0 } let(:following_work_package4) do stub_follower(follower4_start_date, follower4_due_date, - following_work_package2 => follower4_delay_2, - following_work_package3 => follower4_delay_3) + { following_work_package2 => follower4_delay2, following_work_package3 => follower4_delay3 }) end let!(:following) do [following_work_package1, @@ -887,32 +864,32 @@ def stub_follower_child(parent, start, due) following_work_package4] end - context 'moving forward' do + context 'when moving forward' do before do - work_package.due_date = Date.today + 5.days + work_package.due_date = Time.zone.today + 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today + 6.days, Date.today + 8.days], - following_work_package2 => [Date.today + 9.days, Date.today + 13.days], - following_work_package3 => [Date.today + 6.days, Date.today + 9.days], - following_work_package4 => [Date.today + 14.days, Date.today + 15.days] } + { following_work_package1 => [Time.zone.today + 6.days, Time.zone.today + 8.days], + following_work_package2 => [Time.zone.today + 9.days, Time.zone.today + 13.days], + following_work_package3 => [Time.zone.today + 6.days, Time.zone.today + 9.days], + following_work_package4 => [Time.zone.today + 14.days, Time.zone.today + 15.days] } end end end - context 'moving backwards' do + context 'when moving backwards' do before do - work_package.due_date = Date.today - 5.days + work_package.due_date = Time.zone.today - 5.days end it_behaves_like 'reschedules' do let(:expected) do - { following_work_package1 => [Date.today - 4.days, Date.today - 2.days], - following_work_package2 => [Date.today - 1.days, Date.today + 3.days], - following_work_package3 => [Date.today - 1.days, Date.today + 2.days], - following_work_package4 => [Date.today + 4.days, Date.today + 5.days] } + { following_work_package1 => [Time.zone.today - 4.days, Time.zone.today - 2.days], + following_work_package2 => [Time.zone.today - 1.day, Time.zone.today + 3.days], + following_work_package3 => [Time.zone.today - 1.day, Time.zone.today + 2.days], + following_work_package4 => [Time.zone.today + 4.days, Time.zone.today + 5.days] } end end end @@ -932,37 +909,37 @@ def stub_follower_child(parent, start, due) end context "with the parent being restricted in it's ability to be moved" do - let(:soonest_date) { Date.today + 3.days } + let(:soonest_date) { Time.zone.today + 3.days } it 'sets the start date to the earliest possible date' do subject - expect(work_package.start_date).to eql(Date.today + 3.days) + expect(work_package.start_date).to eql(Time.zone.today + 3.days) end end context 'with the parent being restricted but work package already having dates set' do - let(:soonest_date) { Date.today + 3.days } + let(:soonest_date) { Time.zone.today + 3.days } before do - work_package.start_date = Date.today + 4.days - work_package.due_date = Date.today + 5.days + work_package.start_date = Time.zone.today + 4.days + work_package.due_date = Time.zone.today + 5.days end it 'sets the dates to provided dates' do subject - expect(work_package.start_date).to eql(Date.today + 4.days) - expect(work_package.due_date).to eql(Date.today + 5.days) + expect(work_package.start_date).to eql(Time.zone.today + 4.days) + expect(work_package.due_date).to eql(Time.zone.today + 5.days) end end context 'with the parent being restricted but the attributes define an earlier date' do - let(:soonest_date) { Date.today + 3.days } + let(:soonest_date) { Time.zone.today + 3.days } before do - work_package.start_date = Date.today + 1.days - work_package.due_date = Date.today + 2.days + work_package.start_date = Time.zone.today + 1.day + work_package.due_date = Time.zone.today + 2.days end # This would be invalid but the dates should be set nevertheless @@ -970,9 +947,10 @@ def stub_follower_child(parent, start, due) it 'sets the dates to provided dates' do subject - expect(work_package.start_date).to eql(Date.today + 1.days) - expect(work_package.due_date).to eql(Date.today + 2.days) + expect(work_package.start_date).to eql(Time.zone.today + 1.day) + expect(work_package.due_date).to eql(Time.zone.today + 2.days) end end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index c02ca5dd2ac9..f87939e8188d 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -28,6 +28,7 @@ require 'spec_helper' +# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::UpdateService, 'integration tests', type: :model, with_mail: false do let(:user) do create(:user, @@ -524,19 +525,15 @@ role: role) end end - let(:duplicate_work_package) do + let!(:duplicate_work_package) do create(:work_package, work_package_attributes).tap do |wp| - wp.duplicated << work_package + create(:relation, relation_type: Relation::TYPE_DUPLICATES, from: wp, to: work_package) end end let(:attributes) { { status: status_closed } } - before do - duplicate_work_package - end - it 'works and closes duplicates' do expect(subject) .to be_success @@ -577,7 +574,7 @@ let(:following_work_package) do create(:work_package, following_attributes).tap do |wp| - wp.follows << work_package + create(:follows_relation, from: wp, to: work_package) end end let(:following_parent_attributes) do @@ -607,7 +604,7 @@ let(:following2_parent_work_package) do create(:work_package, following2_parent_attributes).tap do |wp| - wp.follows << following_parent_work_package + create(:follows_relation, from: wp, to: following_parent_work_package) end end let(:following3_attributes) do @@ -619,7 +616,7 @@ let(:following3_work_package) do create(:work_package, following3_attributes).tap do |wp| - wp.follows << following2_work_package + create(:follows_relation, from: wp, to: following2_work_package) end end let(:following3_parent_attributes) do @@ -751,7 +748,7 @@ let(:following_work_package) do create(:work_package, following_attributes).tap do |wp| - wp.follows << work_package + create(:follows_relation, from: wp, to: work_package) end end let(:following_parent_attributes) do @@ -790,7 +787,7 @@ let(:following2_parent_work_package) do following2 = create(:work_package, following2_parent_attributes).tap do |wp| - wp.follows << following_parent_work_package + create(:follows_relation, from: wp, to: following_parent_work_package) end create(:relation, @@ -809,7 +806,7 @@ let(:following3_work_package) do create(:work_package, following3_attributes).tap do |wp| - wp.follows << following2_work_package + create(:follows_relation, from: wp, to: following2_work_package) end end @@ -963,14 +960,14 @@ expect(work_package.due_date) .to eql work_package_attributes[:due_date] - # updates the former parent's dates + # updates the former parent's dates based on the only remaining child (former sibling) former_parent_work_package.reload expect(former_parent_work_package.start_date) .to eql former_sibling_attributes[:start_date] expect(former_parent_work_package.due_date) .to eql former_sibling_attributes[:due_date] - # updates the new parent's dates + # updates the new parent's dates based on the moved work package and its now sibling new_parent_work_package.reload expect(new_parent_work_package.start_date) .to eql work_package_attributes[:start_date] @@ -1002,11 +999,9 @@ ) end let(:new_parent_predecessor_work_package) do - wp = create(:work_package, new_parent_predecessor_attributes) - - wp.precedes << new_parent_work_package - - wp + create(:work_package, new_parent_predecessor_attributes).tap do |wp| + create(:follows_relation, from: new_parent_work_package, to: wp) + end end let(:work_package_attributes) do @@ -1095,11 +1090,9 @@ end let(:sibling_work_package) do - wp = create(:work_package, sibling_attributes) - - wp.follows << work_package - - wp + create(:work_package, sibling_attributes).tap do |wp| + create(:follows_relation, from: wp, to: work_package) + end end before do @@ -1242,3 +1235,4 @@ end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/work_packages/update_service_spec.rb b/spec/services/work_packages/update_service_spec.rb index ffd666e39449..cf10e09ac787 100644 --- a/spec/services/work_packages/update_service_spec.rb +++ b/spec/services/work_packages/update_service_spec.rb @@ -197,31 +197,37 @@ it_behaves_like 'service call' context 'relations' do + let!(:scope) do + instance_double('ActiveRecord::Relations').tap do |relations| + allow(Relation) + .to receive(:of_work_package) + .with([work_package]) + .and_return(relations) + allow(relations) + .to receive(:destroy_all) + end + end + it 'removes the relations if the setting does not permit cross project relations' do allow(Setting) .to receive(:cross_project_work_package_relations?) .and_return false - relations = double('relations') - expect(Relation) - .to receive(:non_hierarchy_of_work_package) - .with([work_package]) - .and_return(relations) - expect(relations) - .to receive(:destroy_all) instance.call(project: target_project) + + expect(scope) + .to have_received(:destroy_all) end it 'leaves the relations unchanged if the setting allows cross project relations' do allow(Setting) .to receive(:cross_project_work_package_relations?) - .and_return true - expect(work_package) - .to_not receive(:relations_from) - expect(work_package) - .to_not receive(:relations_to) + .and_return true instance.call(project: target_project) + + expect(scope) + .not_to have_received(:destroy_all) end end diff --git a/spec/support/queries/filters/shared_filter_examples.rb b/spec/support/queries/filters/shared_filter_examples.rb index 901a280dc7ee..be37e547ffaa 100644 --- a/spec/support/queries/filters/shared_filter_examples.rb +++ b/spec/support/queries/filters/shared_filter_examples.rb @@ -704,9 +704,19 @@ let!(:filter_value_wp) { create(:work_package) } let(:wp_relation_type) { defined?(:relation_type) ? relation_type : raise('needs to be defined') } let!(:related_wp) do - relation = Hash.new - relation[wp_relation_type] = [filter_value_wp] - create(:work_package, relation) + create(:work_package).tap do |wp| + if Relation.canonical_type(wp_relation_type) == wp_relation_type + create(:relation, + from: wp, + to: filter_value_wp, + relation_type: wp_relation_type) + else + create(:relation, + to: wp, + from: filter_value_wp, + relation_type: wp_relation_type) + end + end end let!(:unrelated_wp) { create(:work_package) } diff --git a/spec_legacy/fixtures/relations.yml b/spec_legacy/fixtures/relations.yml index 5ace78efda47..03c9806b227d 100644 --- a/spec_legacy/fixtures/relations.yml +++ b/spec_legacy/fixtures/relations.yml @@ -30,11 +30,11 @@ relation_001: id: 1 from_id: 10 to_id: 9 - blocks: 1 + relation_type: 'blocks' delay: relation_002: id: 2 from_id: 2 to_id: 3 - relates: 1 + relation_type: 'relates' delay: From fe3ff0eb822b4b71a92bc042e770977111784654 Mon Sep 17 00:00:00 2001 From: ulferts Date: Sat, 26 Mar 2022 22:57:30 +0100 Subject: [PATCH 2/8] optimize work package fetching for scheduling --- .../work_packages/scopes/for_scheduling.rb | 19 ++++++++------ .../work_packages/schedule_dependency.rb | 26 ++++++++++++++----- .../scopes/for_scheduling_spec.rb | 16 ++++++++++++ 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/app/models/work_packages/scopes/for_scheduling.rb b/app/models/work_packages/scopes/for_scheduling.rb index 35dd8c3e0c45..a6bfdb751d50 100644 --- a/app/models/work_packages/scopes/for_scheduling.rb +++ b/app/models/work_packages/scopes/for_scheduling.rb @@ -118,26 +118,29 @@ def for_scheduling(work_packages) def scheduling_paths_sql(work_packages) values = work_packages.map do |wp| ::OpenProject::SqlSanitization - .sanitize "(:id, false)", + .sanitize "(:id, false, false)", id: wp.id end.join(', ') <<~SQL.squish to_schedule (id, manually) AS ( - SELECT * FROM (VALUES#{values}) AS t(id, manually) + + SELECT * FROM (VALUES#{values}) AS t(id, manually, hierarchy_up) UNION SELECT relations.from_id id, - (related_work_packages.schedule_manually OR COALESCE(descendants.manually, false)) manually + (related_work_packages.schedule_manually OR COALESCE(descendants.manually, false)) manually, + relations.hierarchy_up FROM to_schedule JOIN LATERAL ( SELECT from_id, - to_id + to_id, + false hierarchy_up FROM relations WHERE NOT to_schedule.manually @@ -149,14 +152,14 @@ def scheduling_paths_sql(work_packages) THEN work_package_hierarchies.descendant_id ELSE work_package_hierarchies.ancestor_id END from_id, - to_schedule.id to_id + to_schedule.id to_id, + work_package_hierarchies.descendant_id = to_schedule.id hierarchy_up FROM work_package_hierarchies WHERE NOT to_schedule.manually - AND work_package_hierarchies.generations = 1 - AND (work_package_hierarchies.ancestor_id = to_schedule.id - OR work_package_hierarchies.descendant_id = to_schedule.id) + AND ((work_package_hierarchies.ancestor_id = to_schedule.id AND NOT to_schedule.hierarchy_up AND work_package_hierarchies.generations = 1) + OR (work_package_hierarchies.descendant_id = to_schedule.id AND work_package_hierarchies.generations > 0)) ) relations ON relations.to_id = to_schedule.id LEFT JOIN work_packages related_work_packages ON relations.from_id = related_work_packages.id diff --git a/app/services/work_packages/schedule_dependency.rb b/app/services/work_packages/schedule_dependency.rb index 7364b5cc2bbc..f85de1ccbe47 100644 --- a/app/services/work_packages/schedule_dependency.rb +++ b/app/services/work_packages/schedule_dependency.rb @@ -59,24 +59,23 @@ def scheduled_work_packages_by_id private def build_dependencies - following = load_following(work_packages) + following = load_following # Those variables are pure optimizations. # We want to reuse the already loaded work packages as much as possible # and we want to have them readily available as hashes. self.known_work_packages += following known_work_packages.uniq! - self.known_work_packages_by_id = known_work_packages.group_by(&:id) - self.known_work_packages_by_parent_id = known_work_packages.group_by(&:parent_id) + self.known_work_packages_by_id = known_work_packages.group_by(&:id).transform_values(&:first) + self.known_work_packages_by_parent_id = fetch_descendants.group_by(&:parent_id) add_dependencies(following) end - def load_following(work_packages) + def load_following WorkPackage .for_scheduling(work_packages) - .includes(:parent, - follows_relations: :to) + .includes(follows_relations: :to) end def add_dependencies(dependent_work_packages) @@ -127,6 +126,19 @@ def each_while_unhandled end end + # Use a mixture of work packages that are already loaded to be scheduled themselves but also load + # all the rest of the descendants. There are two cases in which descendants are not loaded for scheduling: + # * manual scheduling: A descendant is either scheduled manually itself or all of its descendants are scheduled manually. + # * sibling: the descendant is not below a work package to be scheduled (e.g. one following another) but below an ancestor of + # a schedule work package. + def fetch_descendants + descendants = known_work_packages_by_id.values + + descendants + WorkPackage + .with_ancestor(descendants) + .where.not(id: known_work_packages_by_id.keys) + end + class Dependency def initialize(work_package, schedule_dependency) self.schedule_dependency = schedule_dependency @@ -195,7 +207,7 @@ def ancestors_from_preloaded(work_package) parent = known_work_packages_by_id[work_package.parent_id] if parent - parent + ancestors_from_preloaded(parent.first) + [parent] + ancestors_from_preloaded(parent) end end || [] end diff --git a/spec/models/work_packages/scopes/for_scheduling_spec.rb b/spec/models/work_packages/scopes/for_scheduling_spec.rb index 1326a8438405..df1062bd80cf 100644 --- a/spec/models/work_packages/scopes/for_scheduling_spec.rb +++ b/spec/models/work_packages/scopes/for_scheduling_spec.rb @@ -503,6 +503,22 @@ end end end + + context 'for a work package with a sibling and a successor that also has a sibling' do + let(:sibling) do + create(:work_package, project: project, parent: parent) + end + let(:successor_sibling) do + create(:work_package, project: project, parent: successor_parent) + end + + let!(:existing_work_packages) { [parent, sibling, successor, successor_parent, successor_sibling] } + + it 'contains the successor and the parents but not the siblings' do + expect(WorkPackage.for_scheduling([origin])) + .to match_array([successor, parent, successor_parent]) + end + end end end # rubocop:enable RSpec/MultipleMemoizedHelpers From 647b455572be26027e762d4a49fdbb63edc4388a Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 09:28:52 +0200 Subject: [PATCH 3/8] replace CT.rebuid! by CTE for speed --- .../20220319211253_add_parent_id_to_wp.rb | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/db/migrate/20220319211253_add_parent_id_to_wp.rb b/db/migrate/20220319211253_add_parent_id_to_wp.rb index 38e2883d76a6..fab053bead9a 100644 --- a/db/migrate/20220319211253_add_parent_id_to_wp.rb +++ b/db/migrate/20220319211253_add_parent_id_to_wp.rb @@ -38,7 +38,7 @@ def up add_closure_tree_table - ClosureTreeWorkPackage.rebuild! + build_closure_tree cleanup_transitive_relations @@ -128,6 +128,41 @@ def add_closure_tree_table # rubocop:enable Rails/CreateTableWithTimestamps end + # Creates the actual closure tree data. + # This recursive query is used over ClosureTreeWorkPackage.rebuild! for speed + # but its result is equivalent. + def build_closure_tree + execute <<~SQL.squish + WITH RECURSIVE closure_tree(ancestor_id, descendant_id, generations) AS ( + SELECT + id ancestor_id, + id descendant_id, + 0 generations + FROM work_packages + UNION + SELECT + closure_tree.ancestor_id, + work_packages.id descendant_id, + closure_tree.generations + 1 + FROM closure_tree + JOIN work_packages ON work_packages.parent_id = closure_tree.descendant_id + ) + + INSERT INTO + work_package_hierarchies ( + ancestor_id, + descendant_id, + generations + ) + SELECT + ancestor_id, + descendant_id, + generations + FROM + closure_tree + SQL + end + def add_relation_index add_index :relations, %i[from_id to_id relation_type], unique: true From ccf1c2de0edeca9e87ca0843b5690f6f1726b050 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 17:06:10 +0200 Subject: [PATCH 4/8] linting --- .../spec/contracts/work_packages/base_contract_spec.rb | 2 +- spec/contracts/work_packages/base_contract_spec.rb | 2 +- spec/models/work_packages/scopes/for_scheduling_spec.rb | 8 ++------ spec/models/work_packages/scopes/relatable_spec.rb | 2 -- spec/services/work_packages/update_service_spec.rb | 2 +- 5 files changed, 5 insertions(+), 11 deletions(-) diff --git a/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb b/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb index 8aea656c9f73..0a9e237c4b03 100644 --- a/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb +++ b/modules/backlogs/spec/contracts/work_packages/base_contract_spec.rb @@ -138,7 +138,7 @@ end let(:relatable_scope) do - scope = instance_double('ActiveRecord::Relation') + scope = instance_double(ActiveRecord::Relation) allow(scope) .to receive(:where) diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index bfe9198a3117..be1853fdd9c6 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -589,7 +589,7 @@ context 'when the intended parent is not relatable' do before do - scope = instance_double('ActiveRecord::Relation') + scope = instance_double(ActiveRecord::Relation) allow(WorkPackage) .to receive(:relatable) diff --git a/spec/models/work_packages/scopes/for_scheduling_spec.rb b/spec/models/work_packages/scopes/for_scheduling_spec.rb index df1062bd80cf..06f2fdb5a55e 100644 --- a/spec/models/work_packages/scopes/for_scheduling_spec.rb +++ b/spec/models/work_packages/scopes/for_scheduling_spec.rb @@ -28,7 +28,6 @@ require 'spec_helper' -# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::Scopes::ForScheduling, 'allowed scope' do let(:project) { create(:project) } let(:origin) { create(:work_package, project: project) } @@ -101,8 +100,6 @@ end let(:existing_work_packages) { [] } - subject {} - describe '.for_scheduling' do it 'is a AR scope' do expect(WorkPackage.for_scheduling([origin])) @@ -205,7 +202,7 @@ end end - context 'for a work package with a successor which has parent and child and a successor of its own which is also a child of parent' do + context 'for a work package with a successor having a parent and child and a successor of its own which is a child itself' do let!(:existing_work_packages) { [successor, successor_child, successor_parent, successor_successor] } before do @@ -278,7 +275,7 @@ end end - context 'both scheduled manually' do + context 'with both scheduled manually' do before do successor.update_column(:schedule_manually, true) successor_parent.update_column(:schedule_manually, true) @@ -521,4 +518,3 @@ end end end -# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/models/work_packages/scopes/relatable_spec.rb b/spec/models/work_packages/scopes/relatable_spec.rb index 7bde9fa7188b..dade6bdd8e49 100644 --- a/spec/models/work_packages/scopes/relatable_spec.rb +++ b/spec/models/work_packages/scopes/relatable_spec.rb @@ -26,7 +26,6 @@ require 'spec_helper' -# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::Scopes::Relatable, '.relatable scope' do let(:project) { create(:project) } let(:origin) { create(:work_package, project: project) } @@ -320,4 +319,3 @@ end end end -# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/work_packages/update_service_spec.rb b/spec/services/work_packages/update_service_spec.rb index cf10e09ac787..10cf7a2a4ed4 100644 --- a/spec/services/work_packages/update_service_spec.rb +++ b/spec/services/work_packages/update_service_spec.rb @@ -198,7 +198,7 @@ context 'relations' do let!(:scope) do - instance_double('ActiveRecord::Relations').tap do |relations| + instance_double(ActiveRecord::Relation).tap do |relations| allow(Relation) .to receive(:of_work_package) .with([work_package]) From 0c6d2a663f1ec9092daeaa4544131dd12a108b53 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 17:28:49 +0200 Subject: [PATCH 5/8] extend comment to clarify intend --- app/services/work_packages/update_service.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index c8dcfe5bd0cd..61f36a72fe79 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -160,6 +160,9 @@ def with_temporarily_persisted_parent_changes if work_package.parent_id_changed? # HACK: we need to persist the parent relation before rescheduling the parent # and the former parent since we rely on the database for scheduling. + # The following will update the parent_id of the work package without that being noticed by the + # work package instance (work_package) that is already instantiated. That way, the change can be rolled + # back without any side effects to the instance (e.g. dirty tracking). WorkPackage.where(id: work_package.id).update_all(parent_id: work_package.parent_id) work_package.rebuild! # using the ClosureTree#rebuild! method to update the transitive hierarchy information end From 1f7f2eb7617616d0c5181a3015d39550596e996a Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 17:29:28 +0200 Subject: [PATCH 6/8] remove outdated comment --- modules/backlogs/spec/models/impediment_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/backlogs/spec/models/impediment_spec.rb b/modules/backlogs/spec/models/impediment_spec.rb index 05354d96ab0f..561605c03e5e 100644 --- a/modules/backlogs/spec/models/impediment_spec.rb +++ b/modules/backlogs/spec/models/impediment_spec.rb @@ -113,7 +113,6 @@ task.version = version task.save - # Using the default association method block_ids (without s) here impediment.blocks_ids = [feature.id, task.id] impediment.save end From b02e738bb9761a84975a20ccc7e1db7c8f58df59 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 17:31:40 +0200 Subject: [PATCH 7/8] add index for other direction of relations --- db/migrate/20220319211253_add_parent_id_to_wp.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/migrate/20220319211253_add_parent_id_to_wp.rb b/db/migrate/20220319211253_add_parent_id_to_wp.rb index fab053bead9a..781f66891fe6 100644 --- a/db/migrate/20220319211253_add_parent_id_to_wp.rb +++ b/db/migrate/20220319211253_add_parent_id_to_wp.rb @@ -166,6 +166,8 @@ def build_closure_tree def add_relation_index add_index :relations, %i[from_id to_id relation_type], unique: true + add_index :relations, %i[to_id from_id relation_type], + unique: true end def add_parent_index From 2583e03c077ed83008df8d46f01d4cc169686341 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 17 May 2022 18:19:44 +0200 Subject: [PATCH 8/8] remove confusing relations associations --- app/models/work_packages/relations.rb | 22 ---------- .../copy/work_packages_dependent_service.rb | 41 +++++++++---------- .../impediments/create_services_spec.rb | 11 ++--- 3 files changed, 26 insertions(+), 48 deletions(-) diff --git a/app/models/work_packages/relations.rb b/app/models/work_packages/relations.rb index f3df1fd05b42..e61e8d55fda9 100644 --- a/app/models/work_packages/relations.rb +++ b/app/models/work_packages/relations.rb @@ -28,28 +28,6 @@ module WorkPackages::Relations extend ActiveSupport::Concern included do - # Relations pointing to another work package. - # In this case, - # * from is self - # * to is the other work package involved - has_many :relations_to, - class_name: 'Relation', - foreign_key: :from_id, - autosave: true, - dependent: :nullify, - inverse_of: :from - - # Relations pointing away from the work package. - # In this case, - # * to is self - # * from is the other work package involved - has_many :relations_from, - class_name: 'Relation', - foreign_key: :to_id, - autosave: true, - dependent: :nullify, - inverse_of: :to - # Relations where the current work package follows another one. # In this case, # * from is self.id diff --git a/app/services/projects/copy/work_packages_dependent_service.rb b/app/services/projects/copy/work_packages_dependent_service.rb index d7953c8d2067..d28db629db23 100644 --- a/app/services/projects/copy/work_packages_dependent_service.rb +++ b/app/services/projects/copy/work_packages_dependent_service.rb @@ -94,27 +94,14 @@ def copy_work_package(source_work_package, parent_id, user_cf_ids) end end - def copy_relations(wp, new_wp_id, work_packages_map) - wp.relations_to.each do |source_relation| - new_relation = Relation.new - new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id') - new_relation.to_id = work_packages_map[source_relation.to_id] - if new_relation.to_id.nil? && Setting.cross_project_work_package_relations? - new_relation.to_id = source_relation.to_id - end - new_relation.from_id = new_wp_id - new_relation.save - end - - wp.relations_from.each do |source_relation| - new_relation = Relation.new - new_relation.attributes = source_relation.attributes.dup.except('id', 'from_id', 'to_id') - new_relation.from_id = work_packages_map[source_relation.from_id] - if new_relation.from_id.nil? && Setting.cross_project_work_package_relations? - new_relation.from_id = source_relation.from_id - end - new_relation.to_id = new_wp_id - new_relation.save + def copy_relations(source_wp, new_wp_id, work_packages_map) + Relation.of_work_package(source_wp).each do |source_relation| + from_id, to_id = relations_from_to(source_relation, source_wp, new_wp_id, work_packages_map) + + Relation.create(source_relation + .attributes + .except('id', 'from_id', 'to_id') + .merge(to_id: to_id, from_id: from_id)) end end @@ -165,5 +152,17 @@ def possible_principal_id(principal_id, project) @principals ||= Principal.possible_assignee(project).pluck(:id).to_set principal_id if @principals.include?(principal_id) end + + def relations_from_to(source_relation, source_wp, new_wp_id, work_packages_map) + if source_relation.from_id == source_wp.id + [new_wp_id, + work_packages_map[source_relation.to_id] || + (Setting.cross_project_work_package_relations? && source_relation.to_id)] + else + [work_packages_map[source_relation.from_id] || + (Setting.cross_project_work_package_relations? && source_relation.from_id), + new_wp_id] + end + end end end diff --git a/modules/backlogs/spec/services/impediments/create_services_spec.rb b/modules/backlogs/spec/services/impediments/create_services_spec.rb index 76e6e48a33b8..f92cf8d25bcc 100644 --- a/modules/backlogs/spec/services/impediments/create_services_spec.rb +++ b/modules/backlogs/spec/services/impediments/create_services_spec.rb @@ -83,14 +83,15 @@ shared_examples_for 'impediment creation with 1 blocking relationship' do it_should_behave_like 'impediment creation' - it { expect(subject.relations_to.size).to eq(1) } - it { expect(subject.relations_to[0].to).to eql feature } - it { expect(subject.relations_to[0].relation_type).to eql Relation::TYPE_BLOCKS } + + it { expect(subject.blocks_relations.size).to eq(1) } + it { expect(subject.blocks_relations[0].to).to eql feature } end shared_examples_for 'impediment creation with no blocking relationship' do it_should_behave_like 'impediment creation' - it { expect(subject.relations_to.size).to eq(0) } + + it { expect(subject.blocks_relations.size).to eq(0) } end describe 'WITH a blocking relationship to a story' do @@ -113,7 +114,7 @@ it_should_behave_like 'impediment creation with 1 blocking relationship' it { expect(subject).not_to be_new_record } - it { expect(subject.relations_to[0]).not_to be_new_record } + it { expect(subject.blocks_relations[0]).not_to be_new_record } end describe 'WITH the story having another version' do