From f53d97f734f121be99271235b633be9e1b0143c7 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 8 Jun 2022 17:03:45 +0200 Subject: [PATCH 01/21] handle invalid work packages needing to be updated --- app/services/work_packages/update_service.rb | 2 +- .../update_service_integration_spec.rb | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index 9753409c92..4fc505c269 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -69,7 +69,7 @@ class WorkPackages::UpdateService < ::BaseServices::BaseCallable def save_if_valid(result) if result.success? result.success = consolidated_results(result) - .all?(&:save) + .all? { |wp| wp.save(validate: false) } end result.success? diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index 1ac30356b6..88012e00c7 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, @@ -1228,4 +1229,55 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma end end end + + describe 'removing an invalid parent' do + # The parent does not have a required custom field set but will need to be touched since. + # the dates, inherited from its children (and then the only remaining child) will have to be updated. + let!(:parent) do + create(:work_package, + type: project.types.first, + project: project, + start_date: Time.zone.today - 1.day, + due_date: Time.zone.today + 5.days) + end + let!(:custom_field) do + create(:int_wp_custom_field, is_required: true, is_for_all: true, default_value: nil).tap do |cf| + project.types.first.custom_fields << cf + project.work_package_custom_fields << cf + end + end + let!(:sibling) do + create(:work_package, + type: project.types.first, + project: project, + parent: parent, + start_date: Time.zone.today + 1.day, + due_date: Time.zone.today + 5.days, + "custom_field_#{custom_field.id}": 5) + end + let!(:attributes) { { parent: nil } } + + let(:work_package_attributes) do + { + start_date: Time.zone.today - 1.day, + due_date: Time.zone.today + 1.day, + project: project, + type: project.types.first, + parent: parent, + "custom_field_#{custom_field.id}": 8 + } + end + + it 'removes the parent successfully and reschedules the parent' do + expect(subject).to be_success + + expect(work_package.reload.parent).to be_nil + + expect(parent.reload.start_date) + .to eql(sibling.start_date) + expect(parent.due_date) + .to eql(sibling.due_date) + end + end end +# rubocop:enable RSpec/MultipleMemoizedHelpers From b1e453d662cd9ba63757d30a074aa312b1713d53 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 8 Jun 2022 17:22:41 +0200 Subject: [PATCH 02/21] use default update service for wps --- .../attachments/replace_attachments.rb | 10 +- .../work_packages/set_attributes_service.rb | 13 +- .../work_packages/set_schedule_service.rb | 4 +- app/services/work_packages/update_service.rb | 173 +- .../lib/open_project/backlogs/list.rb | 6 +- .../backlogs/patches/update_service_patch.rb | 44 +- .../set_attributes_service_spec.rb | 27 + .../update_service_integration_spec.rb | 1944 +++++++++-------- .../work_packages/update_service_spec.rb | 235 +- 9 files changed, 1153 insertions(+), 1303 deletions(-) diff --git a/app/services/attachments/replace_attachments.rb b/app/services/attachments/replace_attachments.rb index 3ae3f2bec9..3f7f2f26de 100644 --- a/app/services/attachments/replace_attachments.rb +++ b/app/services/attachments/replace_attachments.rb @@ -34,13 +34,11 @@ module Attachments private def set_attributes(attributes) - call = super - - if call.success? && call.result.attachments_replacements - call.result.attachments = call.result.attachments_replacements + super.tap do |call| + if call.success? && call.result.attachments_replacements + call.result.attachments = call.result.attachments_replacements + end end - - call end end end diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 77afb91e0b..ef1994a0ea 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -90,7 +90,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes elsif parent_start_earlier_than_due? work_package.parent.start_date elsif Setting.work_package_startdate_is_adddate? - Date.today + Time.zone.today end end @@ -111,7 +111,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes # And the new type has a default text default_description = work_package.type&.description - return unless default_description.present? + return if default_description.blank? # And the current description matches ANY current default text return unless work_package.description.blank? || default_description? @@ -155,6 +155,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes model.change_by_system do set_version_to_nil reassign_category + set_parent_to_nil reassign_type unless work_package.type_id_changed? end @@ -194,6 +195,14 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes end end + def set_parent_to_nil + if !Setting.cross_project_work_package_relations? && + !work_package.parent_changed? + + work_package.parent = nil + end + end + def reassign_category # work_package is moved to another project # reassign to the category with same name if any diff --git a/app/services/work_packages/set_schedule_service.rb b/app/services/work_packages/set_schedule_service.rb index dc0d0df4b2..1c8d5e6f9d 100644 --- a/app/services/work_packages/set_schedule_service.rb +++ b/app/services/work_packages/set_schedule_service.rb @@ -176,9 +176,9 @@ class WorkPackages::SetScheduleService def date_rescheduling_delta(predecessor) if predecessor.due_date.present? - predecessor.due_date - (predecessor.due_date_was || predecessor.due_date) + predecessor.due_date - (predecessor.due_date_before_last_save || predecessor.due_date_was || predecessor.due_date) elsif predecessor.start_date.present? - predecessor.start_date - (predecessor.start_date_was || predecessor.start_date) + predecessor.start_date - (predecessor.start_date_before_last_save || predecessor.start_date_was || predecessor.start_date) else 0 end diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index 4fc505c269..33ef46869f 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -26,97 +26,63 @@ # See COPYRIGHT and LICENSE files for more details. #++ -# TODO: use default update base class -class WorkPackages::UpdateService < ::BaseServices::BaseCallable +class WorkPackages::UpdateService < ::BaseServices::Update include ::WorkPackages::Shared::UpdateAncestors - include ::Shared::ServiceContext + include Attachments::ReplaceAttachments - attr_accessor :user, - :model, - :contract_class + private - def initialize(user:, model:, contract_class: WorkPackages::UpdateContract) - self.user = user - self.model = model - self.contract_class = contract_class - end + def after_perform(service_call) + update_related_work_packages(service_call) + cleanup(service_call.result) - def perform(send_notifications: true, **attributes) - in_context(model, send_notifications) do - update(attributes) - end + service_call end - private - - def update(attributes) - result = set_attributes(attributes) - - if result.success? - work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements - result.merge!(update_dependent) + def update_related_work_packages(service_call) + update_ancestors([service_call.result]).each do |ancestor_service_call| + service_call.merge!(ancestor_service_call) end - if save_if_valid(result) - update_ancestors([work_package]).each do |ancestor_result| - result.merge!(ancestor_result) - end + update_related(service_call.result).each do |related_service_call| + service_call.merge!(related_service_call) end - - result end - def save_if_valid(result) - if result.success? - result.success = consolidated_results(result) - .all? { |wp| wp.save(validate: false) } - end - - result.success? + def update_related(work_package) + consolidated_calls(update_descendants(work_package) + reschedule_related(work_package)) + .reject { |dependent_call| dependent_call.result.id == work_package.id } + .each { |dependent_call| dependent_call.result.save(validate: false) } end - def update_dependent - result = ServiceResult.new(success: true, result: work_package) - - result.merge!(update_descendants) - - cleanup if result.success? - - result.merge!(reschedule_related) + def update_descendants(work_package) + if work_package.saved_change_to_project_id? + attributes = { project: work_package.project } - result + work_package.descendants.map do |descendant| + set_descendant_attributes(attributes, descendant) + end + else + [] + end end - def set_attributes(attributes, wp = work_package) + def set_descendant_attributes(attributes, descendant) WorkPackages::SetAttributesService .new(user:, - model: wp, - contract_class:) + model: descendant, + contract_class: EmptyContract) .call(attributes) end - def update_descendants - result = ServiceResult.new(success: true, result: work_package) - - if work_package.project_id_changed? - attributes = { project: work_package.project } - - work_package.descendants.each do |descendant| - result.add_dependent!(set_attributes(attributes, descendant)) - end - end - - result - end - - def cleanup - if work_package.project_id_changed? + def cleanup(work_package) + if work_package.saved_change_to_project_id? moved_work_packages = [work_package] + work_package.descendants delete_relations(moved_work_packages) move_time_entries(moved_work_packages, work_package.project_id) end - if work_package.type_id_changed? - reset_custom_values + if work_package.saved_change_to_type_id? + reset_custom_values(work_package) end end @@ -134,60 +100,29 @@ class WorkPackages::UpdateService < ::BaseServices::BaseCallable .update_all(project_id:) end - def reset_custom_values + def reset_custom_values(work_package) work_package.reset_custom_values! end - def reschedule_related - result = ServiceResult.new(success: true, result: work_package) - - 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(work_package)) - end - - result - end - - 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. - # 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 - - yield + def reschedule_related(work_package) + rescheduled = if work_package.saved_change_to_parent_id? && work_package.parent_id_before_last_save + reschedule_former_siblings(work_package).dependent_results + else + [] + end - # Always rolling back the changes we made in here - raise ActiveRecord::Rollback - end + rescheduled + reschedule(work_package, [work_package]).dependent_results end - # 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)) + def reschedule_former_siblings(work_package) + reschedule(work_package, WorkPackage.where(parent_id: work_package.parent_id_before_last_save)) end - def reschedule(work_packages) + def reschedule(work_package, work_packages) WorkPackages::SetScheduleService .new(user:, work_package: work_packages) - .call(changed_attributes) - end - - def changed_attributes - work_package.changed.map(&:to_sym) + .call(work_package.saved_changes.keys.map(&:to_sym)) end # When multiple services change a work package, we still only want one update to the database due to: @@ -195,19 +130,15 @@ class WorkPackages::UpdateService < ::BaseServices::BaseCallable # * having only one journal entry # * stale object errors # we thus consolidate the results so that one instance contains the changes made by all the services. - def consolidated_results(result) - result.all_results.group_by(&:id).inject([]) do |a, (_, instances)| - master = instances.pop - - instances.each do |instance| - master.attributes = instance.changes.transform_values(&:last) + def consolidated_calls(service_calls) + service_calls + .group_by { |sc| sc.result.id } + .map do |(_, same_work_package_calls)| + same_work_package_calls.pop.tap do |master| + same_work_package_calls.each do |sc| + master.result.attributes = sc.result.changes.transform_values(&:last) + end end - - a + [master] end end - - def work_package - model - end end diff --git a/modules/backlogs/lib/open_project/backlogs/list.rb b/modules/backlogs/lib/open_project/backlogs/list.rb index 12b2e8e298..9b07e1aa6d 100644 --- a/modules/backlogs/lib/open_project/backlogs/list.rb +++ b/modules/backlogs/lib/open_project/backlogs/list.rb @@ -81,8 +81,10 @@ module OpenProject::Backlogs::List protected - def assume_bottom_position - update_columns(position: bottom_position_in_list(self).to_i + 1) + # Overwriting acts/list to avoid it calling save. + # Calling save will remove the changes/saved_changes information. + def set_list_position(new_position, _raise_exception_if_save_fails = false) + update_columns(position: new_position) end def fix_other_work_package_positions 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 7740752f3e..291ac6e54c 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 @@ -32,36 +32,44 @@ module OpenProject::Backlogs::Patches::UpdateServicePatch end module InstanceMethods - def update_descendants + def update_descendants(work_package) super_result = super - if work_package.in_backlogs_type? && work_package.version_id_changed? - inherit_version_to_descendants(super_result) + if work_package.in_backlogs_type? && work_package.saved_change_to_version_id? + super_result += inherit_version_to_descendants(work_package) end super_result end - def inherit_version_to_descendants(result) - all_descendants = work_package - .descendants - .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_id) || !t.is_task? - stop_descendants_ids << t.id - end - end + def inherit_version_to_descendants(work_package) + all_descendants = sorted_descendants(work_package) + descendant_tasks = descendant_tasks_of(all_descendants) attributes = { version_id: work_package.version_id } - descendant_tasks.each do |task| + descendant_tasks.map do |task| # Ensure the parent is already moved to new version so that validation errors are avoided. task.parent = ([work_package] + all_descendants).detect { |d| d.id == task.parent_id } - result.add_dependent!(set_attributes(attributes, task)) + set_descendant_attributes(attributes, task) + end + end + + def sorted_descendants(work_package) + work_package + .descendants + .includes(project: :enabled_modules) + .order_by_ancestors('asc') + .select('work_packages.*') + end + + def descendant_tasks_of(descendants) + stop_descendants_ids = [] + + descendants.reject do |t| + if stop_descendants_ids.include?(t.parent_id) || !t.is_task? + stop_descendants_ids << t.id + end end end end diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index 54fe6cc670..74785b35a5 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -1096,6 +1096,33 @@ describe WorkPackages::SetAttributesService, type: :model do end end end + + context 'for parent' do + let(:parent_work_package) { build_stubbed(:work_package, project:) } + let(:work_package) do + build_stubbed(:work_package, project:, type: initial_type, parent: parent_work_package) + end + + context 'with cross project relations allowed', with_settings: { cross_project_work_package_relations: true } do + it 'keeps the parent' do + expect(subject) + .to be_success + + expect(work_package.parent) + .to eql(parent_work_package) + end + end + + context 'with cross project relations disabled', with_settings: { cross_project_work_package_relations: false } do + it 'deletes the parent' do + expect(subject) + .to be_success + + expect(work_package.parent) + .to be_nil + end + end + end end context 'when updating project before calling the service' do diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index 88012e00c7..6b09ef52bf 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -28,7 +28,6 @@ require 'spec_helper' -# rubocop:disable RSpec/MultipleMemoizedHelpers describe WorkPackages::UpdateService, 'integration tests', type: :model, with_mail: false do let(:user) do create(:user, @@ -106,1073 +105,1105 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma instance.call(**attributes.merge(send_notifications: false).symbolize_keys) end - describe '#call' do - describe 'updating subject' do - let(:attributes) { { subject: 'New subject' } } + describe 'updating subject' do + let(:attributes) { { subject: 'New subject' } } - it 'works' do - expect(subject) - .to be_success + it 'works' do + expect(subject) + .to be_success - expect(work_package.subject) - .to eql(attributes[:subject]) - end + expect(work_package.subject) + .to eql(attributes[:subject]) end + end - describe 'updating project' do - let(:target_project) do - p = create(:project, - types: target_types, - parent: target_parent) - - create(:member, - user:, - project: p, - roles: [create(:role, permissions: target_permissions)]) - - p - end - let(:attributes) { { project_id: target_project.id } } - let(:target_permissions) { [:move_work_packages] } - let(:target_parent) { nil } - let(:target_types) { [type] } - - describe 'with missing permissions' do - let(:target_permissions) { [] } + context 'when updating the project' do + let(:target_project) do + p = create(:project, + types: target_types, + parent: target_parent) - it 'is failure' do - expect(subject) - .to be_failure - end - end + create(:member, + user:, + project: p, + roles: [create(:role, permissions: target_permissions)]) - describe 'time_entries' do - let!(:time_entries) do - [create(:time_entry, - project:, - work_package:), - create(:time_entry, - project:, - work_package:)] - end + p + end + let(:attributes) { { project_id: target_project.id } } + let(:target_permissions) { [:move_work_packages] } + let(:target_parent) { nil } + let(:target_types) { [type] } - it 'moves the time entries along' do - expect(subject) - .to be_success + it 'is is success and updates the project' do + expect(subject) + .to be_success - expect(TimeEntry.where(id: time_entries.map(&:id)).pluck(:project_id).uniq) - .to match_array [target_project.id] - end + expect(work_package.reload.project) + .to eql target_project + end - describe 'categories' do - let(:category) do - create(:category, - project:) - end + context 'with missing permissions' do + let(:target_permissions) { [] } - before do - work_package.category = category - work_package.save! - end + it 'is failure' do + expect(subject) + .to be_failure + end + end - context 'with equally named category' do - let!(:target_category) do - create(:category, - name: category.name, - project: target_project) - end + describe 'time_entries' do + let!(:time_entries) do + [create(:time_entry, + project:, + work_package:), + create(:time_entry, + project:, + work_package:)] + end - it 'replaces the current category by the equally named one' do - expect(subject) - .to be_success + it 'moves the time entries along' do + expect(subject) + .to be_success - expect(subject.result.category) - .to eql target_category - end - end + expect(TimeEntry.where(id: time_entries.map(&:id)).pluck(:project_id).uniq) + .to match_array [target_project.id] + end + end - context 'w/o target category' do - let!(:other_category) do - create(:category, - project: target_project) - end + describe 'categories' do + let(:category) do + create(:category, + project:) + end - it 'removes the category' do - expect(subject) - .to be_success + before do + work_package.category = category + work_package.save! + end - expect(subject.result.category) - .to be_nil - end - end + context 'with equally named category' do + let!(:target_category) do + create(:category, + name: category.name, + project: target_project) end - describe 'version' do - let(:sharing) { 'none' } - let(:version) do - create(:version, - status: 'open', - project:, - sharing:) - end - let(:work_package) do - create(:work_package, - version:, - project:) - end - - context 'unshared version' do - it 'removes the version' do - expect(subject) - .to be_success - - expect(subject.result.version) - .to be_nil - end - end - - context 'system wide shared version' do - let(:sharing) { 'system' } - - it 'keeps the version' do - expect(subject) - .to be_success - - expect(subject.result.version) - .to eql version - end - end - - context 'move work package in project hierarchy' do - let(:target_parent) do - project - end - - context 'unshared version' do - it 'removes the version' do - expect(subject) - .to be_success + it 'replaces the current category by the equally named one' do + expect(subject) + .to be_success - expect(subject.result.version) - .to be_nil - end - end + expect(subject.result.category) + .to eql target_category + end + end - context 'shared version' do - let(:sharing) { 'tree' } + context 'without a target category' do + let!(:other_category) do + create(:category, + project: target_project) + end - it 'keeps the version' do - expect(subject) - .to be_success + it 'removes the category' do + expect(subject) + .to be_success - expect(subject.result.version) - .to eql version - end - end - end + expect(subject.result.category) + .to be_nil end + end + end - describe 'type' do - let(:target_types) { [type, other_type] } - let(:other_type) { create(:type) } - let(:default_type) { type } - let(:project_types) { [type, other_type] } - let!(:workflow_type) do - create(:workflow, type: default_type, role:, old_status_id: status.id) - end - let!(:workflow_other_type) do - create(:workflow, type: other_type, role:, old_status_id: status.id) - end + describe 'version' do + let(:sharing) { 'none' } + let(:version) do + create(:version, + status: 'open', + project:, + sharing:) + end + let(:work_package) do + create(:work_package, + version:, + project:) + end - context 'with the type existing in the target project' do - it 'keeps the type' do - expect(subject) - .to be_success + context 'with an unshared version' do + it 'removes the version' do + expect(subject) + .to be_success - expect(subject.result.type) - .to eql type - end - end + expect(subject.result.version) + .to be_nil + end + end - context 'with a default type existing in the target project' do - let(:target_types) { [other_type, default_type] } + context 'with a system wide shared version' do + let(:sharing) { 'system' } - it 'uses the default type' do - expect(subject) - .to be_success + it 'keeps the version' do + expect(subject) + .to be_success - expect(subject.result.type) - .to eql default_type - end - end + expect(subject.result.version) + .to eql version + end + end - context 'with only non default types' do - let(:target_types) { [other_type] } + context 'when moving the work package in project hierarchy' do + let(:target_parent) do + project + end - it 'uses the first type' do - expect(subject) - .to be_success + context 'with an unshared version' do + it 'removes the version' do + expect(subject) + .to be_success - expect(subject.result.type) - .to eql other_type - end + expect(subject.result.version) + .to be_nil end + end - context 'with an invalid type being provided' do - let(:target_types) { [type] } + context 'with a shared version' do + let(:sharing) { 'tree' } - let(:attributes) do - { project: target_project, - type: other_type } - end + it 'keeps the version' do + expect(subject) + .to be_success - it 'is unsuccessful' do - expect(subject) - .to be_failure - end + expect(subject.result.version) + .to eql version end end end end - describe 'inheriting dates' do - let(:attributes) { { start_date: Date.today - 8.days, due_date: Date.today + 12.days } } - let(:sibling1_attributes) do - work_package_attributes.merge(start_date: Date.today - 5.days, - due_date: Date.today + 10.days, - parent: parent_work_package) - end - let(:sibling2_attributes) do - work_package_attributes.merge(due_date: Date.today + 16.days, - parent: parent_work_package) + describe 'type' do + let(:target_types) { [type, other_type] } + let(:other_type) { create(:type) } + let(:default_type) { type } + let(:project_types) { [type, other_type] } + let!(:workflow_type) do + create(:workflow, type: default_type, role:, old_status_id: status.id) end - - before do - parent_work_package - grandparent_work_package - sibling1_work_package - sibling2_work_package + let!(:workflow_other_type) do + create(:workflow, type: other_type, role:, old_status_id: status.id) end - it 'works and inherits' do - expect(subject) - .to be_success + context 'with the type existing in the target project' do + it 'keeps the type' do + expect(subject) + .to be_success - # receives the provided start/finish date - expect(work_package.start_date) - .to eql(attributes[:start_date]) - expect(work_package.due_date) - .to eql(attributes[:due_date]) - - # receives the min/max of the children's start/finish date - [parent_work_package, - grandparent_work_package].each do |wp| - wp.reload - - expect(wp.start_date) - .to eql(attributes[:start_date]) - expect(wp.due_date) - .to eql(sibling2_work_package.due_date) + expect(subject.result.type) + .to eql type end - - # sibling dates are unchanged - sibling1_work_package.reload - expect(sibling1_work_package.start_date) - .to eql(sibling1_attributes[:start_date]) - expect(sibling1_work_package.due_date) - .to eql(sibling1_attributes[:due_date]) - - sibling2_work_package.reload - expect(sibling2_work_package.start_date) - .to eql(sibling2_attributes[:start_date]) - expect(sibling2_work_package.due_date) - .to eql(sibling2_attributes[:due_date]) end - end - describe 'inheriting done_ratio' do - let(:attributes) { { done_ratio: 50 } } - let(:work_package_attributes) do - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - estimated_hours: 10 } - end + context 'with a default type existing in the target project' do + let(:target_types) { [other_type, default_type] } - let(:sibling1_attributes) do - work_package_attributes.merge(estimated_hours: nil, - done_ratio: 20, - parent: parent_work_package) - end - let(:sibling2_attributes) do - work_package_attributes.merge(done_ratio: 0, - estimated_hours: 100, - parent: parent_work_package) - end + it 'uses the default type' do + expect(subject) + .to be_success - before do - parent_work_package - grandparent_work_package - sibling1_work_package - sibling2_work_package + expect(subject.result.type) + .to eql default_type + end end - it 'works and inherits average done ratio of leaves weighted by estimated times' do - expect(subject) - .to be_success + context 'with only non default types' do + let(:target_types) { [other_type] } - # set to the provided values - expect(work_package.done_ratio) - .to eql(attributes[:done_ratio]) + it 'uses the first type' do + expect(subject) + .to be_success - # calculated - # sibling1 not factored in as its estimated_hours are nil - calculated_ratio = ((work_package.done_ratio * work_package.estimated_hours) + - (sibling2_work_package.done_ratio * sibling2_work_package.estimated_hours)) / - (work_package.done_ratio + - sibling2_work_package.done_ratio) + expect(subject.result.type) + .to eql other_type + end + end - [parent_work_package, - grandparent_work_package].each do |wp| - wp.reload + context 'with an invalid type being provided' do + let(:target_types) { [type] } - expect(wp.done_ratio) - .to eql(calculated_ratio.to_i) + let(:attributes) do + { project: target_project, + type: other_type } end - # unchanged - sibling1_work_package.reload - expect(sibling1_work_package.done_ratio) - .to eql(sibling1_attributes[:done_ratio]) - - sibling2_work_package.reload - expect(sibling2_work_package.done_ratio) - .to eql(sibling2_attributes[:done_ratio]) + it 'is unsuccessful' do + expect(subject) + .to be_failure + end end end - describe 'inheriting estimated_hours' do - let(:attributes) { { estimated_hours: 7 } } - let(:sibling1_attributes) do - # no estimated hours - work_package_attributes.merge(parent: parent_work_package) - end - let(:sibling2_attributes) do - work_package_attributes.merge(estimated_hours: 5, - parent: parent_work_package) - end - let(:child_attributes) do - work_package_attributes.merge(estimated_hours: 10, - parent: work_package) - end - - before do - parent_work_package - grandparent_work_package - sibling1_work_package - sibling2_work_package - child_work_package + describe 'relations' do + let!(:relation) do + create(:follows_relation, + from: work_package, + to: create(:work_package, + project:)) end - it 'works and inherits' do - expect(subject) - .to be_success - - # receives the provided value - expect(work_package.estimated_hours) - .to eql(attributes[:estimated_hours].to_f) + context 'with cross project relations allowed', with_settings: { cross_project_work_package_relations: true } do + it 'keeps the relation' do + expect(subject) + .to be_success - # receive the sum of the children's estimated hours - [parent_work_package, - grandparent_work_package].each do |wp| - sum = sibling1_attributes[:estimated_hours].to_f + - sibling2_attributes[:estimated_hours].to_f + - attributes[:estimated_hours].to_f + - child_attributes[:estimated_hours].to_f + expect(Relation.find_by(id: relation.id)) + .to eql(relation) + end + end - wp.reload + context 'with cross project relations disabled', with_settings: { cross_project_work_package_relations: false } do + it 'deletes the relation' do + expect(subject) + .to be_success - expect(wp.estimated_hours).to be_nil - expect(wp.derived_estimated_hours).to eql(sum) + expect(Relation.find_by(id: relation.id)) + .to be_nil end + end + end + end - # sibling hours are unchanged - sibling1_work_package.reload - expect(sibling1_work_package.estimated_hours) - .to be_nil - - sibling2_work_package.reload - expect(sibling2_work_package.estimated_hours) - .to eql(sibling2_attributes[:estimated_hours].to_f) + describe 'inheriting dates' do + let(:attributes) { { start_date: Time.zone.today - 8.days, due_date: Time.zone.today + 12.days } } + let(:sibling1_attributes) do + work_package_attributes.merge(start_date: Time.zone.today - 5.days, + due_date: Time.zone.today + 10.days, + parent: parent_work_package) + end + let(:sibling2_attributes) do + work_package_attributes.merge(due_date: Time.zone.today + 16.days, + parent: parent_work_package) + end - # child hours are unchanged - child_work_package.reload - expect(child_work_package.estimated_hours) - .to eql(child_attributes[:estimated_hours].to_f) - end + before do + parent_work_package + grandparent_work_package + sibling1_work_package + sibling2_work_package end - describe 'closing duplicates on closing status' do - let(:status_closed) do - create(:status, - is_closed: true).tap do |status_closed| - create(:workflow, - old_status: status, - new_status: status_closed, - type:, - role:) - end - end - let!(:duplicate_work_package) do - create(:work_package, - work_package_attributes).tap do |wp| - create(:relation, relation_type: Relation::TYPE_DUPLICATES, from: wp, to: work_package) - end - end + it 'works and inherits' do + expect(subject) + .to be_success - let(:attributes) { { status: status_closed } } + # receives the provided start/finish date + expect(work_package.start_date) + .to eql(attributes[:start_date]) + expect(work_package.due_date) + .to eql(attributes[:due_date]) - it 'works and closes duplicates' do - expect(subject) - .to be_success + # receives the min/max of the children's start/finish date + [parent_work_package, + grandparent_work_package].each do |wp| + wp.reload - duplicate_work_package.reload + expect(wp.start_date) + .to eql(attributes[:start_date]) + expect(wp.due_date) + .to eql(sibling2_work_package.due_date) + end + + # sibling dates are unchanged + sibling1_work_package.reload + expect(sibling1_work_package.start_date) + .to eql(sibling1_attributes[:start_date]) + expect(sibling1_work_package.due_date) + .to eql(sibling1_attributes[:due_date]) + + sibling2_work_package.reload + expect(sibling2_work_package.start_date) + .to eql(sibling2_attributes[:start_date]) + expect(sibling2_work_package.due_date) + .to eql(sibling2_attributes[:due_date]) + end + end - expect(work_package.status) - .to eql(attributes[:status]) - expect(duplicate_work_package.status) - .to eql(attributes[:status]) - end + describe 'inheriting done_ratio' do + let(:attributes) { { done_ratio: 50 } } + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + estimated_hours: 10 } end - describe 'rescheduling work packages along follows/hierarchy relations' do - # layout - # following_parent_work_package +-follows- following2_parent_work_package following3_parent_work_package - # | | / | - # hierarchy hierarchy hierarchy hierarchy - # | | / | - # + + + | - # work_package +-follows- following_work_package following2_work_package +-follows- following3_work_package + - # following3_sibling_work_package - let(:work_package_attributes) do - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - start_date: Date.today, - due_date: Date.today + 5.days } - end - let(:attributes) do - { start_date: Date.today + 5.days, - due_date: Date.today + 10.days } - end - let(:following_attributes) do - work_package_attributes.merge(parent: following_parent_work_package, - subject: 'following', - start_date: Date.today + 6.days, - due_date: Date.today + 20.days) - end - let(:following_work_package) do - create(:work_package, - following_attributes).tap do |wp| - create(:follows_relation, from: wp, to: work_package) - end - end - let(:following_parent_attributes) do - work_package_attributes.merge(subject: 'following_parent', - start_date: Date.today + 6.days, - due_date: Date.today + 20.days) - end - let(:following_parent_work_package) do - create(:work_package, - following_parent_attributes) - end - let(:following2_attributes) do - work_package_attributes.merge(parent: following2_parent_work_package, - subject: 'following2', - start_date: Date.today + 21.days, - due_date: Date.today + 25.days) - end - let(:following2_work_package) do - create(:work_package, - following2_attributes) - end - let(:following2_parent_attributes) do - work_package_attributes.merge(subject: 'following2_parent', - start_date: Date.today + 21.days, - due_date: Date.today + 25.days) - end - let(:following2_parent_work_package) do - create(:work_package, - following2_parent_attributes).tap do |wp| - create(:follows_relation, from: wp, to: following_parent_work_package) - end - end - let(:following3_attributes) do - work_package_attributes.merge(subject: 'following3', - parent: following3_parent_work_package, - start_date: Date.today + 26.days, - due_date: Date.today + 30.days) - end - let(:following3_work_package) do - create(:work_package, - following3_attributes).tap do |wp| - create(:follows_relation, from: wp, to: following2_work_package) - end - end - let(:following3_parent_attributes) do - work_package_attributes.merge(subject: 'following3_parent', - start_date: Date.today + 26.days, - due_date: Date.today + 36.days) - end - let(:following3_parent_work_package) do - create(:work_package, - following3_parent_attributes) - end - let(:following3_sibling_attributes) do - work_package_attributes.merge(parent: following3_parent_work_package, - subject: 'following3_sibling', - start_date: Date.today + 32.days, - due_date: Date.today + 36.days) - end - let(:following3_sibling_work_package) do - create(:work_package, - following3_sibling_attributes) - end + let(:sibling1_attributes) do + work_package_attributes.merge(estimated_hours: nil, + done_ratio: 20, + parent: parent_work_package) + end + let(:sibling2_attributes) do + work_package_attributes.merge(done_ratio: 0, + estimated_hours: 100, + parent: parent_work_package) + end - before do - work_package - following_parent_work_package - following_work_package - following2_parent_work_package - following2_work_package - following3_parent_work_package - following3_work_package - following3_sibling_work_package - end + before do + parent_work_package + grandparent_work_package + sibling1_work_package + sibling2_work_package + end - it 'propagates the changes to start/finish date along' do - expect(subject) - .to be_success + it 'works and inherits average done ratio of leaves weighted by estimated times' do + expect(subject) + .to be_success - work_package.reload(select: %i(start_date due_date)) - expect(work_package.start_date) - .to eql Date.today + 5.days + # set to the provided values + expect(work_package.done_ratio) + .to eql(attributes[:done_ratio]) - expect(work_package.due_date) - .to eql Date.today + 10.days + # calculated + # sibling1 not factored in as its estimated_hours are nil + calculated_ratio = ((work_package.done_ratio * work_package.estimated_hours) + + (sibling2_work_package.done_ratio * sibling2_work_package.estimated_hours)) / + (work_package.done_ratio + + sibling2_work_package.done_ratio) - following_work_package.reload(select: %i(start_date due_date)) - expect(following_work_package.start_date) - .to eql Date.today + 11.days + [parent_work_package, + grandparent_work_package].each do |wp| + wp.reload - expect(following_work_package.due_date) - .to eql Date.today + 25.days + expect(wp.done_ratio) + .to eql(calculated_ratio.to_i) + end - following_parent_work_package.reload(select: %i(start_date due_date)) - expect(following_parent_work_package.start_date) - .to eql Date.today + 11.days + # unchanged + sibling1_work_package.reload + expect(sibling1_work_package.done_ratio) + .to eql(sibling1_attributes[:done_ratio]) - expect(following_parent_work_package.due_date) - .to eql Date.today + 25.days + sibling2_work_package.reload + expect(sibling2_work_package.done_ratio) + .to eql(sibling2_attributes[:done_ratio]) + end + end - following2_parent_work_package.reload(select: %i(start_date due_date)) - expect(following2_parent_work_package.start_date) - .to eql Date.today + 26.days + describe 'inheriting estimated_hours' do + let(:attributes) { { estimated_hours: 7 } } + let(:sibling1_attributes) do + # no estimated hours + work_package_attributes.merge(parent: parent_work_package) + end + let(:sibling2_attributes) do + work_package_attributes.merge(estimated_hours: 5, + parent: parent_work_package) + end + let(:child_attributes) do + work_package_attributes.merge(estimated_hours: 10, + parent: work_package) + end - expect(following2_parent_work_package.due_date) - .to eql Date.today + 30.days + before do + parent_work_package + grandparent_work_package + sibling1_work_package + sibling2_work_package + child_work_package + end - following2_work_package.reload(select: %i(start_date due_date)) - expect(following2_work_package.start_date) - .to eql Date.today + 26.days + it 'works and inherits' do + expect(subject) + .to be_success - expect(following2_work_package.due_date) - .to eql Date.today + 30.days + # receives the provided value + expect(work_package.estimated_hours) + .to eql(attributes[:estimated_hours].to_f) - following3_work_package.reload(select: %i(start_date due_date)) - expect(following3_work_package.start_date) - .to eql Date.today + 31.days + # receive the sum of the children's estimated hours + [parent_work_package, + grandparent_work_package].each do |wp| + sum = sibling1_attributes[:estimated_hours].to_f + + sibling2_attributes[:estimated_hours].to_f + + attributes[:estimated_hours].to_f + + child_attributes[:estimated_hours].to_f - expect(following3_work_package.due_date) - .to eql Date.today + 35.days + wp.reload - following3_parent_work_package.reload(select: %i(start_date due_date)) - expect(following3_parent_work_package.start_date) - .to eql Date.today + 31.days + expect(wp.estimated_hours).to be_nil + expect(wp.derived_estimated_hours).to eql(sum) + end - expect(following3_parent_work_package.due_date) - .to eql Date.today + 36.days + # sibling hours are unchanged + sibling1_work_package.reload + expect(sibling1_work_package.estimated_hours) + .to be_nil - following3_sibling_work_package.reload(select: %i(start_date due_date)) - expect(following3_sibling_work_package.start_date) - .to eql Date.today + 32.days + sibling2_work_package.reload + expect(sibling2_work_package.estimated_hours) + .to eql(sibling2_attributes[:estimated_hours].to_f) - expect(following3_sibling_work_package.due_date) - .to eql Date.today + 36.days - end + # child hours are unchanged + child_work_package.reload + expect(child_work_package.estimated_hours) + .to eql(child_attributes[:estimated_hours].to_f) end + end - describe 'rescheduling work packages forward follows/hierarchy relations' do - # layout - # other_work_package - # + - # | - # follows (delay: 3 days) - # | - # following_parent_work_package +-follows- following2_parent_work_package - # | | - # hierarchy hierarchy - # | | - # + + - # work_package +-follows- following_work_package following2_work_package +-follows- following3_work_package - let(:work_package_attributes) do - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - start_date: Date.today, - due_date: Date.today + 5.days } - end - let(:attributes) do - { start_date: Date.today - 5.days, - due_date: Date.today } - end - let(:following_attributes) do - work_package_attributes.merge(parent: following_parent_work_package, - subject: 'following', - start_date: Date.today + 6.days, - due_date: Date.today + 20.days) - end - let(:following_work_package) do - create(:work_package, - following_attributes).tap do |wp| - create(:follows_relation, from: wp, to: work_package) - end - end - let(:following_parent_attributes) do - work_package_attributes.merge(subject: 'following_parent', - start_date: Date.today + 6.days, - due_date: Date.today + 20.days) - end - let(:following_parent_work_package) do - create(:work_package, - following_parent_attributes) - end - let(:other_attributes) do - work_package_attributes.merge(subject: 'other', - start_date: Date.today + 10.days, - due_date: Date.today + 18.days) - end - let(:other_work_package) do - create(:work_package, - other_attributes) - end - let(:following2_attributes) do - work_package_attributes.merge(parent: following2_parent_work_package, - subject: 'following2', - start_date: Date.today + 24.days, - due_date: Date.today + 28.days) - end - let(:following2_work_package) do - create(:work_package, - following2_attributes) - end - let(:following2_parent_attributes) do - work_package_attributes.merge(subject: 'following2_parent', - start_date: Date.today + 24.days, - due_date: Date.today + 28.days) - end - let(:following2_parent_work_package) do - following2 = create(:work_package, - following2_parent_attributes).tap do |wp| - create(:follows_relation, from: wp, to: following_parent_work_package) - end - - create(:relation, - relation_type: Relation::TYPE_FOLLOWS, - from: following2, - to: other_work_package, - delay: 3) - - following2 - end - let(:following3_attributes) do - work_package_attributes.merge(subject: 'following3', - start_date: Date.today + 29.days, - due_date: Date.today + 33.days) - end - let(:following3_work_package) do - create(:work_package, - following3_attributes).tap do |wp| - create(:follows_relation, from: wp, to: following2_work_package) - end + describe 'closing duplicates on closing status' do + let(:status_closed) do + create(:status, + is_closed: true).tap do |status_closed| + create(:workflow, + old_status: status, + new_status: status_closed, + type:, + role:) end - - before do - work_package - other_work_package - following_parent_work_package - following_work_package - following2_parent_work_package - following2_work_package - following3_work_package + end + let!(:duplicate_work_package) do + create(:work_package, + work_package_attributes).tap do |wp| + create(:relation, relation_type: Relation::TYPE_DUPLICATES, from: wp, to: work_package) end + end - it 'propagates the changes to start/finish date along' do - expect(subject) - .to be_success - - work_package.reload(select: %i(start_date due_date)) - expect(work_package.start_date) - .to eql Date.today - 5.days + let(:attributes) { { status: status_closed } } - expect(work_package.due_date) - .to eql Date.today + it 'works and closes duplicates' do + expect(subject) + .to be_success - following_work_package.reload(select: %i(start_date due_date)) - expect(following_work_package.start_date) - .to eql Date.today + 1.day + duplicate_work_package.reload - expect(following_work_package.due_date) - .to eql Date.today + 15.days + expect(work_package.status) + .to eql(attributes[:status]) + expect(duplicate_work_package.status) + .to eql(attributes[:status]) + end + end - following_parent_work_package.reload(select: %i(start_date due_date)) - expect(following_parent_work_package.start_date) - .to eql Date.today + 1.day + describe 'rescheduling work packages along follows/hierarchy relations' do + # layout + # following_parent_work_package +-follows- following2_parent_work_package following3_parent_work_package + # | | / | + # hierarchy hierarchy hierarchy hierarchy + # | | / | + # + + + | + # work_package +-follows- following_work_package following2_work_package +-follows- following3_work_package + + # following3_sibling_work_package + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + start_date: Time.zone.today, + due_date: Time.zone.today + 5.days } + end + let(:attributes) do + { start_date: Time.zone.today + 5.days, + due_date: Time.zone.today + 10.days } + end + let(:following_attributes) do + work_package_attributes.merge(parent: following_parent_work_package, + subject: 'following', + start_date: Time.zone.today + 6.days, + due_date: Time.zone.today + 20.days) + end + let(:following_work_package) do + create(:work_package, + following_attributes).tap do |wp| + create(:follows_relation, from: wp, to: work_package) + end + end + let(:following_parent_attributes) do + work_package_attributes.merge(subject: 'following_parent', + start_date: Time.zone.today + 6.days, + due_date: Time.zone.today + 20.days) + end + let(:following_parent_work_package) do + create(:work_package, + following_parent_attributes) + end + let(:following2_attributes) do + work_package_attributes.merge(parent: following2_parent_work_package, + subject: 'following2', + start_date: Time.zone.today + 21.days, + due_date: Time.zone.today + 25.days) + end + let(:following2_work_package) do + create(:work_package, + following2_attributes) + end + let(:following2_parent_attributes) do + work_package_attributes.merge(subject: 'following2_parent', + start_date: Time.zone.today + 21.days, + due_date: Time.zone.today + 25.days) + end + let(:following2_parent_work_package) do + create(:work_package, + following2_parent_attributes).tap do |wp| + create(:follows_relation, from: wp, to: following_parent_work_package) + end + end + let(:following3_attributes) do + work_package_attributes.merge(subject: 'following3', + parent: following3_parent_work_package, + start_date: Time.zone.today + 26.days, + due_date: Time.zone.today + 30.days) + end + let(:following3_work_package) do + create(:work_package, + following3_attributes).tap do |wp| + create(:follows_relation, from: wp, to: following2_work_package) + end + end + let(:following3_parent_attributes) do + work_package_attributes.merge(subject: 'following3_parent', + start_date: Time.zone.today + 26.days, + due_date: Time.zone.today + 36.days) + end + let(:following3_parent_work_package) do + create(:work_package, + following3_parent_attributes) + end + let(:following3_sibling_attributes) do + work_package_attributes.merge(parent: following3_parent_work_package, + subject: 'following3_sibling', + start_date: Time.zone.today + 32.days, + due_date: Time.zone.today + 36.days) + end + let(:following3_sibling_work_package) do + create(:work_package, + following3_sibling_attributes) + end - expect(following_parent_work_package.due_date) - .to eql Date.today + 15.days + before do + work_package + following_parent_work_package + following_work_package + following2_parent_work_package + following2_work_package + following3_parent_work_package + following3_work_package + following3_sibling_work_package + end - following2_parent_work_package.reload(select: %i(start_date due_date)) - expect(following2_parent_work_package.start_date) - .to eql Date.today + 22.days + # rubocop:disable RSpec/ExampleLength + # rubocop:disable RSpec/MultipleExpectations + it 'propagates the changes to start/finish date along' do + expect(subject) + .to be_success + + work_package.reload(select: %i(start_date due_date)) + expect(work_package.start_date) + .to eql Time.zone.today + 5.days + + expect(work_package.due_date) + .to eql Time.zone.today + 10.days + + following_work_package.reload(select: %i(start_date due_date)) + expect(following_work_package.start_date) + .to eql Time.zone.today + 11.days + expect(following_work_package.due_date) + .to eql Time.zone.today + 25.days + + following_parent_work_package.reload(select: %i(start_date due_date)) + expect(following_parent_work_package.start_date) + .to eql Time.zone.today + 11.days + expect(following_parent_work_package.due_date) + .to eql Time.zone.today + 25.days + + following2_parent_work_package.reload(select: %i(start_date due_date)) + expect(following2_parent_work_package.start_date) + .to eql Time.zone.today + 26.days + expect(following2_parent_work_package.due_date) + .to eql Time.zone.today + 30.days + + following2_work_package.reload(select: %i(start_date due_date)) + expect(following2_work_package.start_date) + .to eql Time.zone.today + 26.days + expect(following2_work_package.due_date) + .to eql Time.zone.today + 30.days + + following3_work_package.reload(select: %i(start_date due_date)) + expect(following3_work_package.start_date) + .to eql Time.zone.today + 31.days + expect(following3_work_package.due_date) + .to eql Time.zone.today + 35.days + + following3_parent_work_package.reload(select: %i(start_date due_date)) + expect(following3_parent_work_package.start_date) + .to eql Time.zone.today + 31.days + expect(following3_parent_work_package.due_date) + .to eql Time.zone.today + 36.days + + following3_sibling_work_package.reload(select: %i(start_date due_date)) + expect(following3_sibling_work_package.start_date) + .to eql Time.zone.today + 32.days + expect(following3_sibling_work_package.due_date) + .to eql Time.zone.today + 36.days + end + # rubocop:enable RSpec/ExampleLength + # rubocop:enable RSpec/MultipleExpectations + end - expect(following2_parent_work_package.due_date) - .to eql Date.today + 26.days + describe 'rescheduling work packages forward follows/hierarchy relations' do + # layout + # other_work_package + # + + # | + # follows (delay: 3 days) + # | + # following_parent_work_package +-follows- following2_parent_work_package + # | | + # hierarchy hierarchy + # | | + # + + + # work_package +-follows- following_work_package following2_work_package +-follows- following3_work_package + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + start_date: Time.zone.today, + due_date: Time.zone.today + 5.days } + end + let(:attributes) do + { + start_date: Time.zone.today - 5.days, + due_date: Time.zone.today + } + end + let(:following_attributes) do + work_package_attributes.merge(parent: following_parent_work_package, + subject: 'following', + start_date: Time.zone.today + 6.days, + due_date: Time.zone.today + 20.days) + end + let(:following_work_package) do + create(:work_package, + following_attributes).tap do |wp| + create(:follows_relation, from: wp, to: work_package) + end + end + let(:following_parent_attributes) do + work_package_attributes.merge(subject: 'following_parent', + start_date: Time.zone.today + 6.days, + due_date: Time.zone.today + 20.days) + end + let(:following_parent_work_package) do + create(:work_package, + following_parent_attributes) + end + let(:other_attributes) do + work_package_attributes.merge(subject: 'other', + start_date: Time.zone.today + 10.days, + due_date: Time.zone.today + 18.days) + end + let(:other_work_package) do + create(:work_package, + other_attributes) + end + let(:following2_attributes) do + work_package_attributes.merge(parent: following2_parent_work_package, + subject: 'following2', + start_date: Time.zone.today + 24.days, + due_date: Time.zone.today + 28.days) + end + let(:following2_work_package) do + create(:work_package, + following2_attributes) + end + let(:following2_parent_attributes) do + work_package_attributes.merge(subject: 'following2_parent', + start_date: Time.zone.today + 24.days, + due_date: Time.zone.today + 28.days) + end + let(:following2_parent_work_package) do + following2 = create(:work_package, + following2_parent_attributes).tap do |wp| + create(:follows_relation, from: wp, to: following_parent_work_package) + end - following2_work_package.reload(select: %i(start_date due_date)) - expect(following2_work_package.start_date) - .to eql Date.today + 22.days + create(:relation, + relation_type: Relation::TYPE_FOLLOWS, + from: following2, + to: other_work_package, + delay: 3) - expect(following2_work_package.due_date) - .to eql Date.today + 26.days + following2 + end + let(:following3_attributes) do + work_package_attributes.merge(subject: 'following3', + start_date: Time.zone.today + 29.days, + due_date: Time.zone.today + 33.days) + end + let(:following3_work_package) do + create(:work_package, + following3_attributes).tap do |wp| + create(:follows_relation, from: wp, to: following2_work_package) + end + end - following3_work_package.reload(select: %i(start_date due_date)) - expect(following3_work_package.start_date) - .to eql Date.today + 27.days + before do + work_package + other_work_package + following_parent_work_package + following_work_package + following2_parent_work_package + following2_work_package + following3_work_package + end - expect(following3_work_package.due_date) - .to eql Date.today + 31.days - end + # rubocop:disable RSpec/ExampleLength + it 'propagates the changes to start/finish date along' do + expect(subject) + .to be_success + + work_package.reload(select: %i(start_date due_date)) + expect(work_package.start_date) + .to eql Time.zone.today - 5.days + expect(work_package.due_date) + .to eql Time.zone.today + + following_work_package.reload(select: %i(start_date due_date)) + expect(following_work_package.start_date) + .to eql Time.zone.today + 1.day + expect(following_work_package.due_date) + .to eql Time.zone.today + 15.days + + following_parent_work_package.reload(select: %i(start_date due_date)) + expect(following_parent_work_package.start_date) + .to eql Time.zone.today + 1.day + expect(following_parent_work_package.due_date) + .to eql Time.zone.today + 15.days + + following2_parent_work_package.reload(select: %i(start_date due_date)) + expect(following2_parent_work_package.start_date) + .to eql Time.zone.today + 22.days + expect(following2_parent_work_package.due_date) + .to eql Time.zone.today + 26.days + + following2_work_package.reload(select: %i(start_date due_date)) + expect(following2_work_package.start_date) + .to eql Time.zone.today + 22.days + expect(following2_work_package.due_date) + .to eql Time.zone.today + 26.days + + following3_work_package.reload(select: %i(start_date due_date)) + expect(following3_work_package.start_date) + .to eql Time.zone.today + 27.days + expect(following3_work_package.due_date) + .to eql Time.zone.today + 31.days end + # rubocop:enable RSpec/ExampleLength + end - describe 'changing the parent' do - let(:former_parent_attributes) do - { - subject: 'former parent', - project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - start_date: Date.today + 3.days, - due_date: Date.today + 9.days - } - end - let(:attributes) { { parent: new_parent_work_package } } - let(:former_parent_work_package) do - create(:work_package, former_parent_attributes) - end + describe 'changing the parent' do + let(:former_parent_attributes) do + { + subject: 'former parent', + project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + start_date: Time.zone.today + 3.days, + due_date: Time.zone.today + 9.days + } + end + let(:attributes) { { parent: new_parent_work_package } } + let(:former_parent_work_package) do + create(:work_package, former_parent_attributes) + end - let(:former_sibling_attributes) do - work_package_attributes.merge( - subject: 'former sibling', - parent: former_parent_work_package, - start_date: Date.today + 3.days, - due_date: Date.today + 6.days - ) - end - let(:former_sibling_work_package) do - create(:work_package, former_sibling_attributes) - end + let(:former_sibling_attributes) do + work_package_attributes.merge( + subject: 'former sibling', + parent: former_parent_work_package, + start_date: Time.zone.today + 3.days, + due_date: Time.zone.today + 6.days + ) + end + let(:former_sibling_work_package) do + create(:work_package, former_sibling_attributes) + end - let(:work_package_attributes) do - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - parent: former_parent_work_package, - start_date: Date.today + 7.days, - due_date: Date.today + 9.days } - end + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + parent: former_parent_work_package, + start_date: Time.zone.today + 7.days, + due_date: Time.zone.today + 9.days } + end - let(:new_parent_attributes) do - work_package_attributes.merge( - subject: 'new parent', - parent: nil, - start_date: Date.today + 10.days, - due_date: Date.today + 12.days - ) - end - let(:new_parent_work_package) do - create(:work_package, new_parent_attributes) - end + let(:new_parent_attributes) do + work_package_attributes.merge( + subject: 'new parent', + parent: nil, + start_date: Time.zone.today + 10.days, + due_date: Time.zone.today + 12.days + ) + end + let(:new_parent_work_package) do + create(:work_package, new_parent_attributes) + end - let(:new_sibling_attributes) do - work_package_attributes.merge( - subject: 'new sibling', - parent: new_parent_work_package, - start_date: Date.today + 10.days, - due_date: Date.today + 12.days - ) - end - let(:new_sibling_work_package) do - create(:work_package, new_sibling_attributes) - end + let(:new_sibling_attributes) do + work_package_attributes.merge( + subject: 'new sibling', + parent: new_parent_work_package, + start_date: Time.zone.today + 10.days, + due_date: Time.zone.today + 12.days + ) + end + let(:new_sibling_work_package) do + create(:work_package, new_sibling_attributes) + end - before do - work_package.reload - former_parent_work_package.reload - former_sibling_work_package.reload - new_parent_work_package.reload - new_sibling_work_package.reload - end + before do + work_package.reload + former_parent_work_package.reload + former_sibling_work_package.reload + new_parent_work_package.reload + new_sibling_work_package.reload + end - it 'changes the parent reference and reschedules former and new parent' do - expect(subject) - .to be_success + it 'changes the parent reference and reschedules former and new parent' do + expect(subject) + .to be_success - # sets the parent and leaves the dates unchanged - work_package.reload - expect(work_package.parent) - .to eql new_parent_work_package - expect(work_package.start_date) - .to eql work_package_attributes[:start_date] - expect(work_package.due_date) - .to eql work_package_attributes[:due_date] - - # 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 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] - expect(new_parent_work_package.due_date) - .to eql new_sibling_attributes[:due_date] - end + # sets the parent and leaves the dates unchanged + work_package.reload + expect(work_package.parent) + .to eql new_parent_work_package + expect(work_package.start_date) + .to eql work_package_attributes[:start_date] + expect(work_package.due_date) + .to eql work_package_attributes[:due_date] + + # 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 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] + expect(new_parent_work_package.due_date) + .to eql new_sibling_attributes[:due_date] end + end - describe 'changing the parent with the parent being restricted in moving to an earlier date' do - # there is actually some time between the new parent and its predecessor - let(:new_parent_attributes) do - work_package_attributes.merge( - subject: 'new parent', - parent: nil, - start_date: Date.today + 8.days, - due_date: Date.today + 14.days - ) - end - let(:attributes) { { parent: new_parent_work_package } } - let(:new_parent_work_package) do - create(:work_package, new_parent_attributes) - end + describe 'changing the parent with the parent being restricted in moving to an earlier date' do + # there is actually some time between the new parent and its predecessor + let(:new_parent_attributes) do + work_package_attributes.merge( + subject: 'new parent', + parent: nil, + start_date: Time.zone.today + 8.days, + due_date: Time.zone.today + 14.days + ) + end + let(:attributes) { { parent: new_parent_work_package } } + let(:new_parent_work_package) do + create(:work_package, new_parent_attributes) + end - let(:new_parent_predecessor_attributes) do - work_package_attributes.merge( - subject: 'new parent predecessor', - parent: nil, - start_date: Date.today + 1.day, - due_date: Date.today + 4.days - ) - end - let(:new_parent_predecessor_work_package) do - create(:work_package, new_parent_predecessor_attributes).tap do |wp| - create(:follows_relation, from: new_parent_work_package, to: wp) - end + let(:new_parent_predecessor_attributes) do + work_package_attributes.merge( + subject: 'new parent predecessor', + parent: nil, + start_date: Time.zone.today + 1.day, + due_date: Time.zone.today + 4.days + ) + end + let(:new_parent_predecessor_work_package) do + 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 - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - start_date: Date.today, - due_date: Date.today + 3.days } - end + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + start_date: Time.zone.today, + due_date: Time.zone.today + 3.days } + end - before do - work_package.reload - new_parent_work_package.reload - new_parent_predecessor_work_package.reload - end + before do + work_package.reload + new_parent_work_package.reload + new_parent_predecessor_work_package.reload + end - it 'reschedules the parent and the work package while adhering to the limitation imposed by the predecessor' do - expect(subject) - .to be_success + it 'reschedules the parent and the work package while adhering to the limitation imposed by the predecessor' do + expect(subject) + .to be_success - # sets the parent and adapts the dates - # The dates are overwritten as the new parent is unable - # to move to the dates of its new child because of the follows relation. - work_package.reload - expect(work_package.parent) - .to eql new_parent_work_package - expect(work_package.start_date) - .to eql new_parent_predecessor_attributes[:due_date] + 1.day - expect(work_package.due_date) - .to eql new_parent_predecessor_attributes[:due_date] + 4.days - - # adapts the parent's dates but adheres to its limitations - # due to the follows relationship - new_parent_work_package.reload - expect(new_parent_work_package.start_date) - .to eql new_parent_predecessor_attributes[:due_date] + 1.day - expect(new_parent_work_package.due_date) - .to eql new_parent_predecessor_attributes[:due_date] + 4.days - - # leaves the parent's predecessor unchanged - new_parent_work_package.reload - expect(new_parent_work_package.start_date) - .to eql new_parent_predecessor_attributes[:due_date] + 1.day - expect(new_parent_work_package.due_date) - .to eql new_parent_predecessor_attributes[:due_date] + 4.days - end + # sets the parent and adapts the dates + # The dates are overwritten as the new parent is unable + # to move to the dates of its new child because of the follows relation. + work_package.reload + expect(work_package.parent) + .to eql new_parent_work_package + expect(work_package.start_date) + .to eql new_parent_predecessor_attributes[:due_date] + 1.day + expect(work_package.due_date) + .to eql new_parent_predecessor_attributes[:due_date] + 4.days + + # adapts the parent's dates but adheres to its limitations + # due to the follows relationship + new_parent_work_package.reload + expect(new_parent_work_package.start_date) + .to eql new_parent_predecessor_attributes[:due_date] + 1.day + expect(new_parent_work_package.due_date) + .to eql new_parent_predecessor_attributes[:due_date] + 4.days + + # leaves the parent's predecessor unchanged + new_parent_work_package.reload + expect(new_parent_work_package.start_date) + .to eql new_parent_predecessor_attributes[:due_date] + 1.day + expect(new_parent_work_package.due_date) + .to eql new_parent_predecessor_attributes[:due_date] + 4.days end + end - describe 'removing the parent on a work package which precedes its sibling' do - let(:work_package_attributes) do - { project_id: project.id, - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - parent: parent_work_package, - start_date: Date.today, - due_date: Date.today + 3.days } - end - let(:attributes) { { parent: nil } } - - let(:parent_attributes) do - { project_id: project.id, - subject: 'parent', - type_id: type.id, - author_id: user.id, - status_id: status.id, - priority:, - start_date: Date.today, - due_date: Date.today + 10.days } - end + describe 'removing the parent on a work package which precedes its sibling' do + let(:work_package_attributes) do + { project_id: project.id, + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + parent: parent_work_package, + start_date: Time.zone.today, + due_date: Time.zone.today + 3.days } + end + let(:attributes) { { parent: nil } } + + let(:parent_attributes) do + { project_id: project.id, + subject: 'parent', + type_id: type.id, + author_id: user.id, + status_id: status.id, + priority:, + start_date: Time.zone.today, + due_date: Time.zone.today + 10.days } + end - let(:parent_work_package) do - create(:work_package, parent_attributes) - end + let(:parent_work_package) do + create(:work_package, parent_attributes) + end - let(:sibling_attributes) do - work_package_attributes.merge( - subject: 'sibling', - start_date: Date.today + 4.days, - due_date: Date.today + 10.days - ) - end + let(:sibling_attributes) do + work_package_attributes.merge( + subject: 'sibling', + start_date: Time.zone.today + 4.days, + due_date: Time.zone.today + 10.days + ) + end - let(:sibling_work_package) do - create(:work_package, sibling_attributes).tap do |wp| - create(:follows_relation, from: wp, to: work_package) - end + let(:sibling_work_package) do + create(:work_package, sibling_attributes).tap do |wp| + create(:follows_relation, from: wp, to: work_package) end + end - before do - work_package.reload - parent_work_package.reload - sibling_work_package.reload - end + before do + work_package.reload + parent_work_package.reload + sibling_work_package.reload + end - it 'removes the parent and reschedules it' do - expect(subject) - .to be_success + it 'removes the parent and reschedules it' do + expect(subject) + .to be_success - # work package itself is unchanged (except for the parent) - work_package.reload - expect(work_package.parent) - .to be_nil - expect(work_package.start_date) - .to eql work_package_attributes[:start_date] - expect(work_package.due_date) - .to eql work_package_attributes[:due_date] - - parent_work_package.reload - expect(parent_work_package.start_date) - .to eql sibling_attributes[:start_date] - expect(parent_work_package.due_date) - .to eql sibling_attributes[:due_date] - end + # work package itself is unchanged (except for the parent) + work_package.reload + expect(work_package.parent) + .to be_nil + expect(work_package.start_date) + .to eql work_package_attributes[:start_date] + expect(work_package.due_date) + .to eql work_package_attributes[:due_date] + + parent_work_package.reload + expect(parent_work_package.start_date) + .to eql sibling_attributes[:start_date] + expect(parent_work_package.due_date) + .to eql sibling_attributes[:due_date] end + end - describe 'replacing the attachments' do - let!(:old_attachment) do - create(:attachment, container: work_package) - end - let!(:other_users_attachment) do - create(:attachment, container: nil, author: create(:user)) - end - let!(:new_attachment) do - create(:attachment, container: nil, author: user) - end + describe 'replacing the attachments' do + let!(:old_attachment) do + create(:attachment, container: work_package) + end + let!(:other_users_attachment) do + create(:attachment, container: nil, author: create(:user)) + end + let!(:new_attachment) do + create(:attachment, container: nil, author: user) + end - it 'reports on invalid attachments and replaces the existent with the new if everything is valid' do - work_package.attachments.reload + # rubocop:disable RSpec/ExampleLength + it 'reports on invalid attachments and replaces the existent with the new if everything is valid' do + work_package.attachments.reload - result = instance.call(attachment_ids: [other_users_attachment.id]) + result = instance.call(attachment_ids: [other_users_attachment.id]) - expect(result) - .to be_failure + expect(result) + .to be_failure - expect(result.errors.symbols_for(:attachments)) - .to match_array [:does_not_exist] + expect(result.errors.symbols_for(:attachments)) + .to match_array [:does_not_exist] - expect(work_package.attachments.reload) - .to match_array [old_attachment] + expect(work_package.attachments.reload) + .to match_array [old_attachment] - expect(other_users_attachment.reload.container) - .to be_nil + expect(other_users_attachment.reload.container) + .to be_nil - result = instance.call(attachment_ids: [new_attachment.id]) + result = instance.call(attachment_ids: [new_attachment.id]) - expect(result) - .to be_success + expect(result) + .to be_success - expect(work_package.attachments.reload) - .to match_array [new_attachment] + expect(work_package.attachments.reload) + .to match_array [new_attachment] - expect(new_attachment.reload.container) - .to eql work_package + expect(new_attachment.reload.container) + .to eql work_package - expect(Attachment.find_by(id: old_attachment.id)) - .to be_nil + expect(Attachment.find_by(id: old_attachment.id)) + .to be_nil - result = instance.call(attachment_ids: []) + result = instance.call(attachment_ids: []) - expect(result) - .to be_success + expect(result) + .to be_success - expect(work_package.attachments.reload) - .to be_empty + expect(work_package.attachments.reload) + .to be_empty - expect(Attachment.all) - .to match_array [other_users_attachment] - end + expect(Attachment.all) + .to match_array [other_users_attachment] end + # rubocop:enable RSpec/ExampleLength end ## @@ -1206,26 +1237,27 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma let(:project_types) { [type, new_type] } let(:attributes) { { type: new_type } } - context 'work package does NOT have default status' do + context 'when the work package does NOT have default status' do let(:status) { create(:status) } it 'assigns the default status' do - expect(work_package).to receive(:status=).and_call_original expect(subject).to be_success expect(work_package.status).to eq(Status.default) end end - context 'work package does have default status' do + context 'when the work package does have default status' do let(:status) { create :default_status } let!(:workflow_type) do create(:workflow, type: new_type, role:, old_status_id: status.id) end it 'does not set the status' do - expect(work_package).not_to receive(:status=) expect(subject).to be_success + + expect(work_package) + .not_to be_saved_change_to_status_id end end end @@ -1236,7 +1268,7 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma let!(:parent) do create(:work_package, type: project.types.first, - project: project, + project:, start_date: Time.zone.today - 1.day, due_date: Time.zone.today + 5.days) end @@ -1249,8 +1281,8 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma let!(:sibling) do create(:work_package, type: project.types.first, - project: project, - parent: parent, + project:, + parent:, start_date: Time.zone.today + 1.day, due_date: Time.zone.today + 5.days, "custom_field_#{custom_field.id}": 5) @@ -1261,9 +1293,9 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma { start_date: Time.zone.today - 1.day, due_date: Time.zone.today + 1.day, - project: project, + project:, type: project.types.first, - parent: parent, + parent:, "custom_field_#{custom_field.id}": 8 } end @@ -1279,5 +1311,71 @@ describe WorkPackages::UpdateService, 'integration tests', type: :model, with_ma .to eql(sibling.due_date) end end + + describe 'updating an invalid work package' do + # The work package does not have a required custom field set. + let(:custom_field) do + create(:int_wp_custom_field, is_required: true, is_for_all: true, default_value: nil).tap do |cf| + project.types.first.custom_fields << cf + project.work_package_custom_fields << cf + end + end + let(:attributes) { { subject: 'A new subject' } } + + let(:work_package_attributes) do + { + subject: 'The old subject', + project:, + type: project.types.first + } + end + + before do + # Creating the custom field after the work package is already saved. + work_package + custom_field + end + + it 'is a failure and does not save the change' do + expect(subject).to be_failure + + expect(work_package.reload.subject) + .to eql work_package_attributes[:subject] + end + end + + describe 'updating the type (custom field resetting)' do + let(:project_types) { [type, new_type] } + let(:new_type) { create(:type) } + let!(:custom_field_of_current_type) do + create(:int_wp_custom_field, default_value: nil).tap do |cf| + type.custom_fields << cf + project.work_package_custom_fields << cf + end + end + let!(:custom_field_of_new_type) do + create(:int_wp_custom_field, default_value: 8).tap do |cf| + new_type.custom_fields << cf + project.work_package_custom_fields << cf + end + end + let(:attributes) do + { type: new_type } + end + + let(:work_package_attributes) do + { + type:, + project:, + "custom_field_#{custom_field_of_current_type.id}": 5 + } + end + + it 'is success, removes the existing custom field value and sets the default for the new one' do + expect(subject).to be_success + + expect(work_package.reload.custom_values.pluck(:custom_field_id, :value)) + .to eq [[custom_field_of_new_type.id, "8"]] + 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 2102c69b44..f15cc24af7 100644 --- a/spec/services/work_packages/update_service_spec.rb +++ b/spec/services/work_packages/update_service_spec.rb @@ -27,238 +27,15 @@ #++ require 'spec_helper' +require 'services/base_services/behaves_like_update_service' describe WorkPackages::UpdateService, type: :model do - let(:user) { build_stubbed(:user) } - let(:project) do - p = build_stubbed(:project) - allow(p).to receive(:shared_versions).and_return([]) - - p - end - let(:work_package) do - wp = build_stubbed(:work_package, project:) - wp.type = build_stubbed(:type) - wp.send(:clear_changes_information) - - wp - end - let(:instance) do - described_class.new(user:, - model: work_package) - end - - before do - # Stub update_ancestors because it messes with the jouralizing expectations - allow(instance).to receive(:update_ancestors).and_return [] - end - - describe 'call' do - let(:set_attributes_service) do - service = double("WorkPackages::SetAttributesService", - new: set_attributes_service_instance) - - stub_const('WorkPackages::SetAttributesService', service) - - service - end - let(:send_notifications) { true } - let(:set_attributes_service_instance) do - instance = double("WorkPackages::SetAttributesServiceInstance") - - allow(instance) - .to receive(:call) do |attributes| - work_package.attributes = attributes - - set_service_results - end - - instance - end - let(:errors) { [] } - let(:set_service_results) { ServiceResult.new success: true, result: work_package } - let(:work_package_save_result) { true } - + # This is now only a very basic test testing the structure of the service. + # The domain tests are in the update_service_integration_spec.rb + it_behaves_like 'BaseServices update service' do before do - set_attributes_service - end - - before do - expect(Journal::NotificationConfiguration) - .to receive(:with) - .with(send_notifications) - .and_yield - - allow(work_package) - .to receive(:save) - .and_return work_package_save_result - end - - shared_examples_for 'service call' do - subject { instance.call(**call_attributes.merge(send_notifications:).symbolize_keys) } - - it 'is successful' do - expect(subject.success?).to be_truthy - end - - it 'sets the value' do - subject - - attributes.each do |attribute, key| - expect(work_package.send(attribute)).to eql key - end - end - - it 'has no errors' do - expect(subject.errors.all?(&:empty?)).to be_truthy - end - - context 'when setting the attributes is unsuccessful (invalid)' do - let(:errors) { ActiveModel::Errors.new(work_package) } - let(:set_service_results) { ServiceResult.new success: false, errors:, result: work_package } - - it 'is unsuccessful' do - expect(subject.success?).to be_falsey - end - - it 'does not persist the changes' do - subject - - expect(work_package).not_to receive(:save) - end - - it 'exposes the errors' do - errors.add(:base, 'This is a custom error!') - - subject - - expect(subject.errors).to eql errors - expect(subject.errors[:base]).to include 'This is a custom error!' - end - end - - context 'when the saving is unsuccessful' do - let(:work_package_save_result) { false } - let(:saving_errors) { ActiveModel::Errors.new(work_package) } - - before do - allow(work_package) - .to receive(:errors) - .and_return(saving_errors) - end - - it 'is unsuccessful' do - expect(subject.success?).to be_falsey - end - - it 'leaves the value unchanged' do - subject - - expect(work_package.changed?).to be_truthy - end - - it 'exposes the errors, but is a new instance' do - saving_errors.add(:base, 'This is a custom error!') - - subject - - expect(subject.errors).not_to eql saving_errors - expect(subject.errors[:base]).to include 'This is a custom error!' - end - end - end - - context 'update subject before calling the service' do - let(:call_attributes) { {} } - let(:attributes) { { subject: 'blubs blubs' } } - - before do - work_package.attributes = attributes - end - - it_behaves_like 'service call' - end - - context 'updating subject via attributes' do - let(:call_attributes) { attributes } - let(:attributes) { { subject: 'blubs blubs' } } - - it_behaves_like 'service call' - end - - context 'when switching the project' do - let(:target_project) do - build_stubbed(:project) - end - let(:call_attributes) { attributes } - let(:attributes) { { project: target_project } } - - it_behaves_like 'service call' - - context 'relations' do - let!(:scope) do - instance_double(ActiveRecord::Relation).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 - - 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 - - instance.call(project: target_project) - - expect(scope) - .not_to have_received(:destroy_all) - end - end - - context 'time_entries' do - it 'moves the time entries' do - scope = double('scope') - - expect(TimeEntry) - .to receive(:on_work_packages) - .with([work_package]) - .and_return(scope) - - expect(scope) - .to receive(:update_all) - .with(project_id: target_project.id) - - instance.call(project: target_project) - end - end - end - - context 'when switching the type' do - let(:target_type) { build_stubbed(:type) } - - context 'custom_values' do - it 'resets the custom values' do - expect(work_package) - .to receive(:reset_custom_values!) - - instance.call(type: target_type) - end - end + allow(set_attributes_errors) + .to receive(:merge!) end end end From 44454b94668308216d92cafaab80fbb8112cea6c Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 10 Jun 2022 13:21:04 +0200 Subject: [PATCH 03/21] eager load follows relations --- app/services/work_packages/schedule_dependency.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/work_packages/schedule_dependency.rb b/app/services/work_packages/schedule_dependency.rb index 9283c8d8ba..e1fd9b0cd0 100644 --- a/app/services/work_packages/schedule_dependency.rb +++ b/app/services/work_packages/schedule_dependency.rb @@ -101,6 +101,7 @@ class WorkPackages::ScheduleDependency descendants + WorkPackage .with_ancestor(descendants) + .includes(follows_relations: :to) .where.not(id: known_work_packages_by_id.keys) end From 1e6a96ba7ac8f87ca5df58d4e726eb51422b4e1d Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 10 Jun 2022 13:23:38 +0200 Subject: [PATCH 04/21] only save, and for that load, custom field values if changes have been made --- .../plugins/acts_as_customizable/lib/acts_as_customizable.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb b/lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb index 1059a3c953..512891874f 100644 --- a/lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb +++ b/lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb @@ -179,6 +179,8 @@ module Redmine end def ensure_custom_values_complete + return unless custom_values.loaded? && (custom_values.any?(&:changed?) || custom_value_destroyed) + self.custom_values = custom_field_values end From 8868186415ae22b613f8f9b978c5111f8d1e43a1 Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 10 Jun 2022 13:49:26 +0200 Subject: [PATCH 05/21] skip validating automatically calculated values --- .../update_dependent_contract.rb | 36 ++++ app/services/base_services/base_contracted.rb | 6 +- app/services/base_services/copy.rb | 2 +- app/services/base_services/create.rb | 4 +- app/services/journals/create_service.rb | 3 - app/services/relations/base_service.rb | 2 +- app/services/work_packages/update_service.rb | 2 +- .../lib/open_project/backlogs/list.rb | 2 + .../v3/work_packages/dependent_errors_spec.rb | 198 ------------------ 9 files changed, 46 insertions(+), 209 deletions(-) create mode 100644 app/contracts/work_packages/update_dependent_contract.rb delete mode 100644 spec/requests/api/v3/work_packages/dependent_errors_spec.rb diff --git a/app/contracts/work_packages/update_dependent_contract.rb b/app/contracts/work_packages/update_dependent_contract.rb new file mode 100644 index 0000000000..f18fbf18ec --- /dev/null +++ b/app/contracts/work_packages/update_dependent_contract.rb @@ -0,0 +1,36 @@ +# 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. + +# Disables all validations but still allows to be queried for assignable_* (e.g. statuses) +# that are necessary whenever a dependent (e.g. descendant of a work package) is moved to a project +# where the type is switched. +module WorkPackages + class UpdateDependentContract < UpdateContract + def validate + true + end + end +end diff --git a/app/services/base_services/base_contracted.rb b/app/services/base_services/base_contracted.rb index f671b42ecb..390df28969 100644 --- a/app/services/base_services/base_contracted.rb +++ b/app/services/base_services/base_contracted.rb @@ -50,12 +50,12 @@ module BaseServices # Determine the type of context # this service is running in # e.g., within a resource lock or just executing as the given user - def service_context(&) - in_context(model, true, &) + def service_context(send_notifications: true, &block) + in_context(model, send_notifications, &block) end def perform(params = {}) - service_context do + service_context(send_notifications: (params || {}).fetch(:send_notifications, true)) do service_call = validate_params(params) service_call = before_perform(params, service_call) if service_call.success? service_call = validate_contract(service_call) if service_call.success? diff --git a/app/services/base_services/copy.rb b/app/services/base_services/copy.rb index e98aa0b7fa..0cc3cf5b5e 100644 --- a/app/services/base_services/copy.rb +++ b/app/services/base_services/copy.rb @@ -94,7 +94,7 @@ module BaseServices ## # Disabling sending regular notifications - def service_context(&) + def service_context(*_args, &) in_context(model, false, &) end diff --git a/app/services/base_services/create.rb b/app/services/base_services/create.rb index 6b52e4d96b..5154536913 100644 --- a/app/services/base_services/create.rb +++ b/app/services/base_services/create.rb @@ -30,8 +30,8 @@ module BaseServices class Create < Write protected - def service_context(&) - in_user_context(true, &) + def service_context(send_notifications: true, &block) + in_user_context(send_notifications, &block) end def instance(_params) diff --git a/app/services/journals/create_service.rb b/app/services/journals/create_service.rb index 10c292de88..fd792c3bda 100644 --- a/app/services/journals/create_service.rb +++ b/app/services/journals/create_service.rb @@ -54,9 +54,6 @@ module Journals return ServiceResult.new success: true unless journal destroy_predecessor(journal) - - journal - reload_journals touch_journable(journal) diff --git a/app/services/relations/base_service.rb b/app/services/relations/base_service.rb index 23f1dcf080..e42f5db1a0 100644 --- a/app/services/relations/base_service.rb +++ b/app/services/relations/base_service.rb @@ -70,7 +70,7 @@ class Relations::BaseService < ::BaseServices::BaseCallable # The to-work_package will not be altered by the schedule service so # we do not have to save the result of the service. save_result = if schedule_result.success? - schedule_result.dependent_results.all? { |dr| !dr.result.changed? || dr.result.save } + schedule_result.dependent_results.all? { |dr| !dr.result.changed? || dr.result.save(validate: false) } end || false schedule_result.success = save_result diff --git a/app/services/work_packages/update_service.rb b/app/services/work_packages/update_service.rb index 33ef46869f..ce1a947b6b 100644 --- a/app/services/work_packages/update_service.rb +++ b/app/services/work_packages/update_service.rb @@ -71,7 +71,7 @@ class WorkPackages::UpdateService < ::BaseServices::Update WorkPackages::SetAttributesService .new(user:, model: descendant, - contract_class: EmptyContract) + contract_class: WorkPackages::UpdateDependentContract) .call(attributes) end diff --git a/modules/backlogs/lib/open_project/backlogs/list.rb b/modules/backlogs/lib/open_project/backlogs/list.rb index 9b07e1aa6d..c38edb717c 100644 --- a/modules/backlogs/lib/open_project/backlogs/list.rb +++ b/modules/backlogs/lib/open_project/backlogs/list.rb @@ -83,9 +83,11 @@ module OpenProject::Backlogs::List # Overwriting acts/list to avoid it calling save. # Calling save will remove the changes/saved_changes information. + # rubocop:disable Style/OptionalBooleanParameter def set_list_position(new_position, _raise_exception_if_save_fails = false) update_columns(position: new_position) end + # rubocop:enable Style/OptionalBooleanParameter def fix_other_work_package_positions if changes.slice('project_id', 'type_id', 'version_id').present? diff --git a/spec/requests/api/v3/work_packages/dependent_errors_spec.rb b/spec/requests/api/v3/work_packages/dependent_errors_spec.rb deleted file mode 100644 index 19cabf4492..0000000000 --- a/spec/requests/api/v3/work_packages/dependent_errors_spec.rb +++ /dev/null @@ -1,198 +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. -#++ - -require 'spec_helper' -require 'rack/test' - -describe 'API v3 Work package resource', type: :request, content_type: :json do - include Rack::Test::Methods - include Capybara::RSpecMatchers - include API::V3::Utilities::PathHelper - - let(:work_package) do - create( - :work_package, - project_id: project.id, - parent:, - subject: "Updated WorkPackage" - ) - end - - let!(:parent) do - create(:work_package, project_id: project.id, type:, subject: "Invalid Dependent WorkPackage").tap do |parent| - parent.custom_values.create custom_field: custom_field, value: custom_field.possible_values.first.id - - cv = parent.custom_values.last - cv.update_column :value, "0" - end - end - - let(:project) do - create(:project, identifier: 'deperr', public: false).tap do |project| - project.types << type - end - end - - let(:type) do - create(:type).tap do |type| - type.custom_fields << custom_field - end - end - - let(:status) { create :status } - - let(:custom_field) do - create( - :list_wp_custom_field, - name: "Gate", - possible_values: %w(A B C), - is_required: true - ) - end - - let(:role) { create(:role, permissions:) } - let(:permissions) { %i[view_work_packages edit_work_packages create_work_packages] } - - let(:current_user) do - create(:user, member_in_project: project, member_through_role: role) - end - - let(:dependent_error_result) do - proc do |instance, _attributes, work_package| - result = ServiceResult.new(success: true, result: (instance.respond_to?(:model) && instance.model) || work_package) - dep = parent - dep.errors.add :base, "invalid", message: "invalid" - - result.add_dependent!(ServiceResult.new(success: false, errors: dep.errors, result: dep)) - - result - end - end - - before do - login_as current_user - end - - describe '#patch' do - let(:path) { api_v3_paths.work_package work_package.id } - let(:valid_params) do - { - _type: 'WorkPackage', - lockVersion: work_package.lock_version - } - end - - subject(:response) { last_response } - - shared_context 'patch request' do - before do - patch path, params.to_json, 'CONTENT_TYPE' => 'application/json' - end - end - - before do - allow_any_instance_of(WorkPackages::UpdateService).to receive(:update_dependent, &dependent_error_result) - end - - context 'attribute' do - let(:params) { valid_params.merge(startDate: "2018-05-23") } - - include_context 'patch request' - - it { expect(response.status).to eq(422) } - - it 'responds with an error' do - expected_error = { - _type: "Error", - errorIdentifier: "urn:openproject-org:api:v3:errors:PropertyConstraintViolation", - message: "Error attempting to alter dependent object: Work package ##{parent.id} - #{parent.subject}: invalid", - _embedded: { - details: { - attribute: "base" - } - } - } - - expect(subject.body).to be_json_eql(expected_error.to_json) - end - end - end - - describe '#post' do - let(:current_user) { create :admin } - - let(:path) { api_v3_paths.work_packages } - let(:valid_params) do - { - _type: 'WorkPackage', - lockVersion: 0, - _links: { - author: { href: "/api/v3/users/#{current_user.id}" }, - project: { href: "/api/v3/projects/#{project.id}" }, - status: { href: "/api/v3/statuses/#{status.id}" }, - priority: { href: "/api/v3/priorities/#{work_package.priority.id}" } - } - } - end - - subject(:response) { last_response } - - shared_context 'post request' do - before do - post path, params.to_json, 'CONTENT_TYPE' => 'application/json' - end - end - - before do - allow_any_instance_of(WorkPackages::CreateService).to receive(:create, &dependent_error_result) - end - - context 'request' do - let(:params) { valid_params.merge(subject: "Test Subject") } - - include_context 'post request' - - it { expect(response.status).to eq(422) } - - it 'responds with an error' do - expected_error = { - _type: "Error", - errorIdentifier: "urn:openproject-org:api:v3:errors:PropertyConstraintViolation", - message: "Error attempting to alter dependent object: Work package ##{parent.id} - #{parent.subject}: invalid", - _embedded: { - details: { - attribute: "base" - } - } - } - - expect(subject.body).to be_json_eql(expected_error.to_json) - end - end - end -end From 61bd9e454bdb218ea3a10a2b70d2e685471b8b0d Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 13:23:11 +0200 Subject: [PATCH 06/21] describe ignoreNonWorkingDays in API spec --- .../components/schemas/work_package_model.yml | 11 ++++- docs/api/apiv3/tags/work_packages.yml | 40 ++++++++++--------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/docs/api/apiv3/components/schemas/work_package_model.yml b/docs/api/apiv3/components/schemas/work_package_model.yml index 3393766098..2a967f8220 100644 --- a/docs/api/apiv3/components/schemas/work_package_model.yml +++ b/docs/api/apiv3/components/schemas/work_package_model.yml @@ -45,14 +45,14 @@ properties: type: string format: date description: Similar to start date but is not set by a client but rather deduced - by the work packages's descendants. If manual scheduleManually is active, the + by the work packages' descendants. If manual scheduleManually is active, the two dates can deviate. readOnly: true derivedDueDate: type: string format: date description: Similar to due date but is not set by a client but rather deduced - by the work packages's descendants. If manual scheduleManually is active, the + by the work packages' descendants. If manual scheduleManually is active, the two dates can deviate. readOnly: true duration: @@ -71,6 +71,13 @@ properties: format: duration description: Time a work package likely needs to be completed including its descendants readOnly: true + ignoreNonWorkingDays: + type: boolean + description: |- + **(NOT IMPLEMENTED)** When scheduling, whether or not to ignore the non working days being defined. + A work package with the flag set to true will be allowed to be scheduled to a non working day. + Not available for milestone type of work packages. + readOnly: true spentTime: type: string format: duration diff --git a/docs/api/apiv3/tags/work_packages.yml b/docs/api/apiv3/tags/work_packages.yml index fca900ed6a..53d703f033 100644 --- a/docs/api/apiv3/tags/work_packages.yml +++ b/docs/api/apiv3/tags/work_packages.yml @@ -44,25 +44,27 @@ description: |- ## Local Properties - | Property | Description | Type | Constraints | Supported operations | Condition | - | :--------------: | ------------------------------------------------------ | ----------- | ------------------------------------------------------------------------------------------------------ | -------------------- | -------------------------------- | - | id | Work package id | Integer | x > 0 | READ | | - | lockVersion | The version of the item as used for optimistic locking | Integer | | READ | | - | subject | Work package subject | String | not null; 1 <= length <= 255 | READ / WRITE | | - | type | Name of the work package's type | String | not null | READ | | - | description | The work package description | Formattable | | READ / WRITE | | - | scheduleManually | If false (default) schedule automatically. | Boolean | | READ / WRITE | | - | startDate | Scheduled beginning of a work package | Date | Cannot be set for parent work packages; must be equal or greater than the earliest possible start date; Exists only on work packages of a non milestone type | READ / WRITE | | - | dueDate | Scheduled end of a work package | Date | Cannot be set for parent work packages; must be greater than or equal to the start date; Exists only on work packages of a non milestone type | READ / WRITE | | - | date | Date on which a milestone is achieved | Date | Exists only on work packages of a milestone type | READ / WRITE | | - | derivedStartDate | Similar to start date but is not set by a client but rather deduced by the work packages's descendants. If manual scheduleManually is active, the two dates can deviate. | Date | | READ | | - | derivedDueDate | Similar to due date but is not set by a client but rather deduced by the work packages's descendants. If manual scheduleManually is active, the two dates can deviate. | Date | | READ | | - | estimatedTime | Time a work package likely needs to be completed excluding its descendants | Duration | | READ / WRITE | | - | derivedEstimatedTime | Time a work package likely needs to be completed including its descendants | Duration | | READ | | - | spentTime | The time booked for this work package by users working on it | Duration | | READ | **Permission** view time entries | - | percentageDone | Amount of total completion for a work package | Integer | 0 <= x <= 100; Cannot be set for parent work packages | READ / WRITE | | - | createdAt | Time of creation | DateTime | | READ | | - | updatedAt | Time of the most recent change to the work package | DateTime | | READ | | + | Property | Description | Type | Constraints | Supported operations | Condition | + | :--------------: | ------------------------------------------------------ | ----------- | ------------------------------------------------------------------------------------------------------ | -------------------- | -------------------------------- | + | id | Work package id | Integer | x > 0 | READ | | + | lockVersion | The version of the item as used for optimistic locking | Integer | | READ | | + | subject | Work package subject | String | not null; 1 <= length <= 255 | READ / WRITE | | + | type | Name of the work package's type | String | not null | READ | | + | description | The work package description | Formattable | | READ / WRITE | | + | scheduleManually | If false (default) schedule automatically. | Boolean | | READ / WRITE | | + | startDate | Scheduled beginning of a work package | Date | Cannot be set for parent work packages; must be equal or greater than the earliest possible start date; Exists only on work packages of a non milestone type | READ / WRITE | | + | dueDate | Scheduled end of a work package | Date | Cannot be set for parent work packages; must be greater than or equal to the start date; Exists only on work packages of a non milestone type | READ / WRITE | | + | date | Date on which a milestone is achieved | Date | Exists only on work packages of a milestone type | READ / WRITE | | + | derivedStartDate | Similar to start date but is not set by a client but rather deduced by the work packages' descendants. If manual scheduleManually is active, the two dates can deviate. | Date | | READ | | + | derivedDueDate | Similar to due date but is not set by a client but rather deduced by the work packages' descendants. If manual scheduleManually is active, the two dates can deviate. | Date | | READ | | + | duration | The amount of time in hours the work package needs to be completed. | Duration | **(NOT IMPLEMENTED YET)** Not available for milestone type of work packages. | READ | | + | estimatedTime | Time a work package likely needs to be completed excluding its descendants | Duration | | READ / WRITE | | + | derivedEstimatedTime | Time a work package likely needs to be completed including its descendants | Duration | | READ | | + | ignoreNonWorkingDays | When scheduling, whether or not to ignore the non working days being defined. A work package with the flag set to true will be allowed to be scheduled to a non working day. | Boolean | **(NOT IMPLEMENTED YET)** Not available for milestone type of work packages. | READ | | + | spentTime | The time booked for this work package by users working on it | Duration | | READ | **Permission** view time entries | + | percentageDone | Amount of total completion for a work package | Integer | 0 <= x <= 100; Cannot be set for parent work packages | READ / WRITE | | + | createdAt | Time of creation | DateTime | | READ | | + | updatedAt | Time of the most recent change to the work package | DateTime | | READ | | Note that the properties listed here only cover the built-in properties of the OpenProject Core. Using plug-ins and custom fields a work package might contain various additional properties. From d64af76ea31e09b128dc9c551459450ec2af0ad5 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 13:23:50 +0200 Subject: [PATCH 07/21] remove file links from WorkPackages in API spec --- docs/api/apiv3/paths/file_link.yml | 2 -- docs/api/apiv3/paths/file_link_open.yml | 1 - 2 files changed, 3 deletions(-) diff --git a/docs/api/apiv3/paths/file_link.yml b/docs/api/apiv3/paths/file_link.yml index 4b114aa019..dc97e943ce 100644 --- a/docs/api/apiv3/paths/file_link.yml +++ b/docs/api/apiv3/paths/file_link.yml @@ -4,7 +4,6 @@ get: summary: Gets a file link. operationId: view_file_link tags: - - Work Packages - File links description: |- Gets a single file link resource of a work package. @@ -41,7 +40,6 @@ delete: summary: Removes a file link. operationId: delete_file_link tags: - - Work Packages - File links description: |- Removes a file link on a work package. diff --git a/docs/api/apiv3/paths/file_link_open.yml b/docs/api/apiv3/paths/file_link_open.yml index c05047ad8a..b863b1ea40 100644 --- a/docs/api/apiv3/paths/file_link_open.yml +++ b/docs/api/apiv3/paths/file_link_open.yml @@ -4,7 +4,6 @@ get: summary: Creates an opening uri of the linked file. operationId: open_file_link tags: - - Work Packages - File links description: |- Creates a uri to open the origin file linked by the given file link. This uri depends on the storage type and From a394fe5c15334342c97c5843098f32aa158593db Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 15:28:47 +0200 Subject: [PATCH 08/21] add ignore flag to work packages --- ...614132200_add_ignore_non_working_days_to_work_packages.rb | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 db/migrate/20220614132200_add_ignore_non_working_days_to_work_packages.rb diff --git a/db/migrate/20220614132200_add_ignore_non_working_days_to_work_packages.rb b/db/migrate/20220614132200_add_ignore_non_working_days_to_work_packages.rb new file mode 100644 index 0000000000..0dcd0331d0 --- /dev/null +++ b/db/migrate/20220614132200_add_ignore_non_working_days_to_work_packages.rb @@ -0,0 +1,5 @@ +class AddIgnoreNonWorkingDaysToWorkPackages < ActiveRecord::Migration[7.0] + def change + add_column :work_packages, :ignore_non_working_days, :boolean, default: true, null: false + end +end From 1aaf5ba57a89fd1de3aa00bcf32d4bb7fb8b55fe Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 15:36:46 +0200 Subject: [PATCH 09/21] add ignore flag to work package representer --- .../components/schemas/work_package_model.yml | 1 - docs/api/apiv3/tags/work_packages.yml | 2 +- .../work_packages/work_package_representer.rb | 5 +++ .../work_package_representer_spec.rb | 42 +++++++++++++++---- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/docs/api/apiv3/components/schemas/work_package_model.yml b/docs/api/apiv3/components/schemas/work_package_model.yml index 2a967f8220..4c821f2574 100644 --- a/docs/api/apiv3/components/schemas/work_package_model.yml +++ b/docs/api/apiv3/components/schemas/work_package_model.yml @@ -76,7 +76,6 @@ properties: description: |- **(NOT IMPLEMENTED)** When scheduling, whether or not to ignore the non working days being defined. A work package with the flag set to true will be allowed to be scheduled to a non working day. - Not available for milestone type of work packages. readOnly: true spentTime: type: string diff --git a/docs/api/apiv3/tags/work_packages.yml b/docs/api/apiv3/tags/work_packages.yml index 53d703f033..d0037b9886 100644 --- a/docs/api/apiv3/tags/work_packages.yml +++ b/docs/api/apiv3/tags/work_packages.yml @@ -60,7 +60,7 @@ description: |- | duration | The amount of time in hours the work package needs to be completed. | Duration | **(NOT IMPLEMENTED YET)** Not available for milestone type of work packages. | READ | | | estimatedTime | Time a work package likely needs to be completed excluding its descendants | Duration | | READ / WRITE | | | derivedEstimatedTime | Time a work package likely needs to be completed including its descendants | Duration | | READ | | - | ignoreNonWorkingDays | When scheduling, whether or not to ignore the non working days being defined. A work package with the flag set to true will be allowed to be scheduled to a non working day. | Boolean | **(NOT IMPLEMENTED YET)** Not available for milestone type of work packages. | READ | | + | ignoreNonWorkingDays | When scheduling, whether or not to ignore the non working days being defined. A work package with the flag set to true will be allowed to be scheduled to a non working day. | Boolean | **(NOT IMPLEMENTED YET)** | READ | | | spentTime | The time booked for this work package by users working on it | Duration | | READ | **Permission** view time entries | | percentageDone | Amount of total completion for a work package | Integer | 0 <= x <= 100; Cannot be set for parent work packages | READ / WRITE | | | createdAt | Time of creation | DateTime | | READ | | diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index e70c9c2ad5..97c3d30b91 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -402,6 +402,11 @@ module API end, render_nil: true + property :ignore_non_working_days, + if: ->(*) { + OpenProject::FeatureDecisions.work_packages_duration_field_active? + } + property :spent_time, exec_context: :decorator, getter: ->(*) do 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 df0ebb1a0d..3d907ee506 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 @@ -53,6 +53,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do let(:derived_due_date) { Time.zone.today - 5.days } let(:budget) { build_stubbed(:budget, project:) } let(:duration) { nil } + let(:ignore_non_working_days) { true } let(:work_package) do build_stubbed(:work_package, schedule_manually:, @@ -69,6 +70,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do estimated_hours:, derived_estimated_hours:, budget:, + ignore_non_working_days:, status:) do |wp| allow(wp) .to receive(:available_custom_fields) @@ -299,12 +301,13 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do describe 'duration' do let(:duration) { 6 } + let(:feature_active) { true } before do # TODO: remove feature flag once the implementation is complete allow(OpenProject::FeatureDecisions) .to receive(:work_packages_duration_field_active?) - .and_return(true) + .and_return(feature_active) end it { is_expected.to be_json_eql('P6D'.to_json).at_path('duration') } @@ -326,12 +329,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end context 'when the feature flag is off' do - before do - # TODO: remove feature flag once the implementation is complete - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(false) - end + let(:feature_active) { false } let(:duration) { 6 } it 'has no duration' do @@ -340,6 +338,36 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end end + describe 'ignoreNonWorkingDays' do + let(:ignore_non_working_days) { true } + let(:feature_active) { true } + + before do + # TODO: remove feature flag once the implementation is complete + allow(OpenProject::FeatureDecisions) + .to receive(:work_packages_duration_field_active?) + .and_return(feature_active) + end + + context 'with the value being `true`' do + it { is_expected.to be_json_eql(true.to_json).at_path('ignoreNonWorkingDays') } + end + + context 'with the value being `false`' do + let(:ignore_non_working_days) { false } + + it { is_expected.to be_json_eql(false.to_json).at_path('ignoreNonWorkingDays') } + end + + context 'when the feature flag is off' do + let(:feature_active) { false } + + it 'has no ignoreNonWorkingDays' do + is_expected.to_not have_json_path('ignoreNonWorkingDays') + end + end + end + describe 'createdAt' do it_behaves_like 'has UTC ISO 8601 date and time' do let(:date) { work_package.created_at } From b314f02c1b8c04c6384372c03eb1444b3afc1b24 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 16:42:01 +0200 Subject: [PATCH 10/21] add ignore flag to work package schema representer --- config/locales/en.yml | 3 +- .../schema/work_package_schema_representer.rb | 6 +++ .../work_package_schema_representer_spec.rb | 42 ++++++++++++++++--- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index d82b4ca79f..2c84ff8dfd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -581,7 +581,7 @@ en: duration: "Duration" end_insertion: "End of the insertion" end_deletion: "End of the deletion" - version: "Version" + ignore_non_working_days: "Ignore non working days" parent: "Parent" parent_issue: "Parent" parent_work_package: "Parent" @@ -593,6 +593,7 @@ en: subproject: "Subproject" time_entries: "Log time" type: "Type" + version: "Version" watcher: "Watcher" 'doorkeeper/application': uid: "Client ID" diff --git a/lib/api/v3/work_packages/schema/work_package_schema_representer.rb b/lib/api/v3/work_packages/schema/work_package_schema_representer.rb index b42dbe1baf..b0cfe02823 100644 --- a/lib/api/v3/work_packages/schema/work_package_schema_representer.rb +++ b/lib/api/v3/work_packages/schema/work_package_schema_representer.rb @@ -128,6 +128,12 @@ module API required: false, has_default: true + schema :ignore_non_working_days, + type: 'Boolean', + required: false, + writable: false, + show_if: ->(*) { OpenProject::FeatureDecisions.work_packages_duration_field_active? } + schema :start_date, type: 'Date', required: false, diff --git a/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb b/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb index 1e6382424c..bbfd2ae816 100644 --- a/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb @@ -348,6 +348,33 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do end end + describe 'ignoreNonWorkingDays' do + let(:feature_active) { true } + + before do + # TODO: remove feature flag once the implementation is complete + allow(OpenProject::FeatureDecisions) + .to receive(:work_packages_duration_field_active?) + .and_return(feature_active) + end + + it_behaves_like 'has basic schema properties' do + let(:path) { 'ignoreNonWorkingDays' } + let(:type) { 'Boolean' } + let(:name) { I18n.t('activerecord.attributes.work_package.ignore_non_working_days') } + let(:required) { false } + let(:writable) { false } + end + + context 'when the feature flag is off' do + let(:feature_active) { false } + + it 'has no ignoreNonWorkingDays attribute' do + expect(subject).not_to have_json_path('ignoreNonWorkingDays') + end + end + end + describe 'date' do before do allow(schema) @@ -1063,13 +1090,18 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do end it 'does not cache the attribute_groups' do - representer.to_json - - expect(work_package.type) - .to receive(:attribute_groups) - .and_return([]) + call_count = 0 + allow(work_package.type) + .to receive(:attribute_groups) do + call_count += 1 + [] + end + # Rendering two times, the Type#attribute_groups + # should still be called on the second rendering. representer.to_json + expect { representer.to_json } + .to change { call_count } end end From a82fa93a70c6f36e3e67108a9629dd6bba39dcac Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Jun 2022 17:30:32 +0200 Subject: [PATCH 11/21] add ignore flag to work package contracts but hide from form configuration --- app/contracts/work_packages/base_contract.rb | 1 + app/models/type/attributes.rb | 13 ++++---- .../work_packages/base_contract_spec.rb | 18 +++++++++++ spec/models/type_spec.rb | 32 +++++++++++++------ 4 files changed, 49 insertions(+), 15 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 62ea0acc5a..b03bda4f1c 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -80,6 +80,7 @@ module WorkPackages end attribute :schedule_manually + attribute :ignore_non_working_days attribute :start_date, writeable: ->(*) { diff --git a/app/models/type/attributes.rb b/app/models/type/attributes.rb index 4387cd7175..24eed20f4e 100644 --- a/app/models/type/attributes.rb +++ b/app/models/type/attributes.rb @@ -32,15 +32,16 @@ module Type::Attributes EXCLUDED = %w[_type _dependencies attribute_groups - links - parent_id - parent - description - schedule_manually derived_start_date derived_due_date derived_estimated_time - duration].freeze + ignore_non_working_days + duration + description + links + parent_id + parent + schedule_manually].freeze included do # Allow plugins to define constraints diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index d7d0e852bb..c9122ee307 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -550,6 +550,24 @@ describe WorkPackages::BaseContract do end end + describe 'ignore_non_working_days' do + context 'when setting the value to true' do + before do + work_package.ignore_non_working_days = true + end + + it_behaves_like 'contract is valid' + end + + context 'when setting the value to false' do + before do + work_package.ignore_non_working_days = false + end + + it_behaves_like 'contract is valid' + end + end + describe 'percentage done' do it_behaves_like 'a parent unwritable property', :done_ratio diff --git a/spec/models/type_spec.rb b/spec/models/type_spec.rb index 25957d8772..add1dc7b55 100644 --- a/spec/models/type_spec.rb +++ b/spec/models/type_spec.rb @@ -113,25 +113,39 @@ describe ::Type, type: :model do describe '#work_package_attributes' do subject { type.work_package_attributes } + let(:duration_field_active) { true } + before do allow(OpenProject::FeatureDecisions) .to receive(:work_packages_duration_field_active?) .and_return(true) end - it 'does not return the duration field' do - expect(subject).not_to have_key("duration") + context 'for the duration field' do + it 'does not return true field' do + expect(subject).not_to have_key("duration") + end + + context 'when the feature flag is off' do + let(:duration_field_active) { true } + + it 'does not return the field' do + expect(subject).not_to have_key("duration") + end + end end - context 'when the feature flag is off' do - before do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(false) + context 'for the ignore_non_working_days field' do + it 'does not return duration' do + expect(subject).not_to have_key("ignore_non_working_days") end - it 'does not return the duration field' do - expect(subject).not_to have_key("duration") + context 'when the feature flag is off' do + let(:duration_field_active) { true } + + it 'does not return the duration field' do + expect(subject).not_to have_key("ignore_non_working_days") + end end end end From 369ea5578b86001146f050f7ca35203203730ea6 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 09:23:09 +0200 Subject: [PATCH 12/21] introduce with_flag spec helper --- .../work_packages/work_package_representer.rb | 4 +- .../work_package_representer_spec.rb | 29 ++--------- spec/models/type_spec.rb | 20 ++------ spec/support/shared/with_flag.rb | 51 +++++++++++++++++++ 4 files changed, 61 insertions(+), 43 deletions(-) create mode 100644 spec/support/shared/with_flag.rb diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index 97c3d30b91..0ec5dbb821 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -393,8 +393,8 @@ module API property :duration, exec_context: :decorator, - skip_render: ->(represented:, **) { - represented.milestone? || !OpenProject::FeatureDecisions.work_packages_duration_field_active? + if: ->(represented:, **) { + !represented.milestone? && OpenProject::FeatureDecisions.work_packages_duration_field_active? }, getter: ->(*) do datetime_formatter.format_duration_from_hours(represented.duration_in_hours, 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 3d907ee506..5252b265f6 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 @@ -299,16 +299,8 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end end - describe 'duration' do + describe 'duration', with_flag: { work_packages_duration_field_active: true } do let(:duration) { 6 } - let(:feature_active) { true } - - before do - # TODO: remove feature flag once the implementation is complete - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(feature_active) - end it { is_expected.to be_json_eql('P6D'.to_json).at_path('duration') } @@ -328,26 +320,15 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end end - context 'when the feature flag is off' do - let(:feature_active) { false } - let(:duration) { 6 } - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'has no duration' do is_expected.to_not have_json_path('duration') end end end - describe 'ignoreNonWorkingDays' do + describe 'ignoreNonWorkingDays', with_flag: { work_packages_duration_field_active: true } do let(:ignore_non_working_days) { true } - let(:feature_active) { true } - - before do - # TODO: remove feature flag once the implementation is complete - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(feature_active) - end context 'with the value being `true`' do it { is_expected.to be_json_eql(true.to_json).at_path('ignoreNonWorkingDays') } @@ -359,9 +340,7 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do it { is_expected.to be_json_eql(false.to_json).at_path('ignoreNonWorkingDays') } end - context 'when the feature flag is off' do - let(:feature_active) { false } - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'has no ignoreNonWorkingDays' do is_expected.to_not have_json_path('ignoreNonWorkingDays') end diff --git a/spec/models/type_spec.rb b/spec/models/type_spec.rb index add1dc7b55..ae45715e47 100644 --- a/spec/models/type_spec.rb +++ b/spec/models/type_spec.rb @@ -113,36 +113,24 @@ describe ::Type, type: :model do describe '#work_package_attributes' do subject { type.work_package_attributes } - let(:duration_field_active) { true } - - before do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(true) - end - - context 'for the duration field' do + context 'for the duration field', with_flag: { work_packages_duration_field_active: true } do it 'does not return true field' do expect(subject).not_to have_key("duration") end - context 'when the feature flag is off' do - let(:duration_field_active) { true } - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'does not return the field' do expect(subject).not_to have_key("duration") end end end - context 'for the ignore_non_working_days field' do + context 'for the ignore_non_working_days field', with_flag: { work_packages_duration_field_active: true } do it 'does not return duration' do expect(subject).not_to have_key("ignore_non_working_days") end - context 'when the feature flag is off' do - let(:duration_field_active) { true } - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'does not return the duration field' do expect(subject).not_to have_key("ignore_non_working_days") end diff --git a/spec/support/shared/with_flag.rb b/spec/support/shared/with_flag.rb new file mode 100644 index 0000000000..24ad8534e8 --- /dev/null +++ b/spec/support/shared/with_flag.rb @@ -0,0 +1,51 @@ +# 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 WithFlagMixin + module_function + + def with_flags(flags) + flags.each do |k, value| + name = k.end_with?('?') ? k : "#{k}?" + + raise "#{k} is not a valid flag" unless OpenProject::FeatureDecisions.respond_to?(name) + + allow(OpenProject::FeatureDecisions).to receive(name).and_return value + end + end +end + +RSpec.configure do |config| + config.include WithFlagMixin + + config.before do |example| + flags = example.metadata[:with_flag] + + if flags.present? + with_flags(flags) + end + end +end From 5e0f1b5a5b95f30cf289e3b85c76c39646866aa6 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Tue, 14 Jun 2022 13:26:36 +0200 Subject: [PATCH 13/21] fix flickering storages feature spec - removed race condition, that caused database to be updated AFTER view was initialized sometimes --- .../storages/spec/features/show_file_links_spec.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/modules/storages/spec/features/show_file_links_spec.rb b/modules/storages/spec/features/show_file_links_spec.rb index 99e267982f..2da5d135f0 100644 --- a/modules/storages/spec/features/show_file_links_spec.rb +++ b/modules/storages/spec/features/show_file_links_spec.rb @@ -41,6 +41,7 @@ describe 'Showing of file links in work package', :enable_storages, type: :featu let(:wp_page) { ::Pages::FullWorkPackage.new(work_package, project) } before do + project_storage file_link login_as current_user @@ -48,10 +49,6 @@ describe 'Showing of file links in work package', :enable_storages, type: :featu end context 'if work package has associated file links' do - before do - project_storage - end - it "must show associated file links" do expect(page).to have_selector('[data-qa-selector="op-tab-content--tab-section"]', count: 2) expect(page.find('[data-qa-selector="op-files-tab--file-link-list"]')) @@ -62,16 +59,14 @@ describe 'Showing of file links in work package', :enable_storages, type: :featu context 'if user has no permission to see file links' do let(:permissions) { %i(view_work_packages edit_work_packages) } - before do - project_storage - end - it 'must not show a file links section' do expect(page).to have_selector('[data-qa-selector="op-tab-content--tab-section"]', count: 1) end end context 'if project has no storage' do + let(:project_storage) { {} } + it 'must not show a file links section' do expect(page).to have_selector('[data-qa-selector="op-tab-content--tab-section"]', count: 1) end From 122b56332e76c181bc414bc05eb466ecdcf96dd0 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 09:24:32 +0200 Subject: [PATCH 14/21] let writability of duration and ignore_non_working_days be govered by feature flag --- app/contracts/work_packages/base_contract.rb | 10 ++++++-- .../work_package_payload_representer.rb | 8 +------ .../work_packages/base_contract_spec.rb | 23 ++++++++++++++++--- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index b03bda4f1c..2dd08b1c57 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -80,7 +80,10 @@ module WorkPackages end attribute :schedule_manually - attribute :ignore_non_working_days + attribute :ignore_non_working_days, + writeable: ->(*) { + OpenProject::FeatureDecisions.work_packages_duration_field_active? + } attribute :start_date, writeable: ->(*) { @@ -99,7 +102,10 @@ module WorkPackages model.leaf? || model.schedule_manually? } - attribute :duration + attribute :duration, + writeable: ->(*) { + OpenProject::FeatureDecisions.work_packages_duration_field_active? + } attribute :budget diff --git a/lib/api/v3/work_packages/work_package_payload_representer.rb b/lib/api/v3/work_packages/work_package_payload_representer.rb index 8293edf578..df9edd8149 100644 --- a/lib/api/v3/work_packages/work_package_payload_representer.rb +++ b/lib/api/v3/work_packages/work_package_payload_representer.rb @@ -36,13 +36,7 @@ module API cached_representer disabled: true def writeable_attributes - attributes = super - # TODO: Remove this, once the duration feature is complete - # We have to remove the duration field as it is not being accepted yet. - if OpenProject::FeatureDecisions.work_packages_duration_field_active? - attributes -= %w[duration] - end - attributes + %w[date] + super + %w[date] end def load_complete_model(model) diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index c9122ee307..4b765aa4c3 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -476,7 +476,7 @@ describe WorkPackages::BaseContract do end end - describe 'duration' do + describe 'duration', with_flag: { work_packages_duration_field_active: true } do context 'when setting the duration' do before do work_package.duration = 5 @@ -485,6 +485,14 @@ describe WorkPackages::BaseContract do it_behaves_like 'contract is valid' end + context 'when setting the duration with the feature disabled', with_flag: { work_packages_duration_field_active: false } do + before do + work_package.duration = 5 + end + + it_behaves_like 'contract is invalid', duration: :error_readonly + end + context 'when setting the duration to 0' do before do work_package.duration = 0 @@ -551,7 +559,7 @@ describe WorkPackages::BaseContract do end describe 'ignore_non_working_days' do - context 'when setting the value to true' do + context 'when setting the value to true', with_flag: { work_packages_duration_field_active: true } do before do work_package.ignore_non_working_days = true end @@ -559,13 +567,22 @@ describe WorkPackages::BaseContract do it_behaves_like 'contract is valid' end - context 'when setting the value to false' do + context 'when setting the value to false', with_flag: { work_packages_duration_field_active: true } do before do work_package.ignore_non_working_days = false end it_behaves_like 'contract is valid' end + + context 'when setting the value to false and with the feature disabled', + with_flag: { work_packages_duration_field_active: false } do + before do + work_package.ignore_non_working_days = false + end + + it_behaves_like 'contract is invalid', ignore_non_working_days: :error_readonly + end end describe 'percentage done' do From ec01d40440c90dab080f2097fb04e0025eed3169 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 09:30:28 +0200 Subject: [PATCH 15/21] use feature flag spec helper for all duration specs --- .../work_package_schema_representer_spec.rb | 39 ++++--------------- .../columns/property_column_spec.rb | 12 +----- 2 files changed, 10 insertions(+), 41 deletions(-) diff --git a/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb b/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb index bbfd2ae816..dfb82fe4c5 100644 --- a/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/schema/work_package_schema_representer_spec.rb @@ -293,15 +293,13 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do end end - describe 'duration' do + describe 'duration', with_flag: { work_packages_duration_field_active: true } do + let(:milestone?) { false } + before do - # TODO: remove feature flag once the implementation is complete - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(true) allow(schema) .to receive(:milestone?) - .and_return(false) + .and_return(milestone?) end it_behaves_like 'has basic schema properties' do @@ -313,24 +311,14 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do end context 'when the work package is a milestone' do - before do - allow(schema) - .to receive(:milestone?) - .and_return(true) - end + let(:milestone?) { true } it 'has no duration attribute' do expect(subject).not_to have_json_path('duration') end end - context 'when the feature flag is off' do - before do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(false) - end - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'has no duration attribute' do expect(subject).not_to have_json_path('duration') end @@ -348,16 +336,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do end end - describe 'ignoreNonWorkingDays' do - let(:feature_active) { true } - - before do - # TODO: remove feature flag once the implementation is complete - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(feature_active) - end - + describe 'ignoreNonWorkingDays', with_flag: { work_packages_duration_field_active: true } do it_behaves_like 'has basic schema properties' do let(:path) { 'ignoreNonWorkingDays' } let(:type) { 'Boolean' } @@ -366,9 +345,7 @@ describe ::API::V3::WorkPackages::Schema::WorkPackageSchemaRepresenter do let(:writable) { false } end - context 'when the feature flag is off' do - let(:feature_active) { false } - + context 'when the feature flag is off', with_flag: { work_packages_duration_field_active: false } do it 'has no ignoreNonWorkingDays attribute' do expect(subject).not_to have_json_path('ignoreNonWorkingDays') end diff --git a/spec/models/queries/work_packages/columns/property_column_spec.rb b/spec/models/queries/work_packages/columns/property_column_spec.rb index 035d075091..5465336bc7 100644 --- a/spec/models/queries/work_packages/columns/property_column_spec.rb +++ b/spec/models/queries/work_packages/columns/property_column_spec.rb @@ -55,22 +55,14 @@ describe Queries::WorkPackages::Columns::PropertyColumn, type: :model do end end - context 'when duration feature flag disabled' do + context 'when duration feature flag disabled', with_flag: { work_packages_duration_field_active: false } do it 'column does not exist' do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(false) - expect(described_class.instances.map(&:name)).not_to include :duration end end - context 'when duration feature flag enabled' do + context 'when duration feature flag enabled', with_flag: { work_packages_duration_field_active: true } do it 'column exists' do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(true) - expect(described_class.instances.map(&:name)).to include :duration end end From fd3fb7b46c7b48dc5a581afb91d3a3918d78ec57 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 09:34:01 +0200 Subject: [PATCH 16/21] spec default value for ignore_non_working_days --- spec/models/work_package_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index 0ef63a1e1c..6875a70762 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -690,4 +690,13 @@ describe WorkPackage, type: :model do it { expect(subject).to match_array([work_package]) } end end + + describe '#ignore_non_working_days' do + context 'for a new record' do + it 'is true' do + expect(described_class.new.ignore_non_working_days) + .to be true + end + end + end end From c33890eb2c5f2f29a50d66fbc631e2b9945be4ed Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 09:36:26 +0200 Subject: [PATCH 17/21] autolint work_package spec --- spec/models/work_package_spec.rb | 46 ++++++++++++++++---------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index 6875a70762..9b550dd149 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -39,7 +39,7 @@ describe WorkPackage, type: :model do let(:status) { create(:status) } let(:priority) { create(:priority) } let(:work_package) do - WorkPackage.new.tap do |w| + described_class.new.tap do |w| w.attributes = { project_id: project.id, type_id: type.id, author_id: user.id, @@ -62,14 +62,14 @@ describe WorkPackage, type: :model do context 'no project chosen' do it 'has no type set if no project was chosen' do - expect(WorkPackage.new.type) + expect(described_class.new.type) .to be_nil end end context 'project chosen' do it 'has the provided type if one is provided' do - expect(WorkPackage.new(project:, type: type2).type) + expect(described_class.new(project:, type: type2).type) .to eql type2 end end @@ -96,7 +96,7 @@ describe WorkPackage, type: :model do describe 'minimal' do let(:work_package_minimal) do - WorkPackage.new.tap do |w| + described_class.new.tap do |w| w.attributes = { project_id: project.id, type_id: type.id, author_id: user.id, @@ -277,7 +277,7 @@ describe WorkPackage, type: :model do end context 'work package' do - subject { WorkPackage.find_by(id: work_package.id) } + subject { described_class.find_by(id: work_package.id) } it { is_expected.to be_nil } end @@ -450,43 +450,43 @@ describe WorkPackage, type: :model do end context 'by type' do - let(:groups) { WorkPackage.by_type(project) } + let(:groups) { described_class.by_type(project) } it_behaves_like 'group by' end context 'by version' do - let(:groups) { WorkPackage.by_version(project) } + let(:groups) { described_class.by_version(project) } it_behaves_like 'group by' end context 'by priority' do - let(:groups) { WorkPackage.by_priority(project) } + let(:groups) { described_class.by_priority(project) } it_behaves_like 'group by' end context 'by category' do - let(:groups) { WorkPackage.by_category(project) } + let(:groups) { described_class.by_category(project) } it_behaves_like 'group by' end context 'by assigned to' do - let(:groups) { WorkPackage.by_assigned_to(project) } + let(:groups) { described_class.by_assigned_to(project) } it_behaves_like 'group by' end context 'by responsible' do - let(:groups) { WorkPackage.by_responsible(project) } + let(:groups) { described_class.by_responsible(project) } it_behaves_like 'group by' end context 'by author' do - let(:groups) { WorkPackage.by_author(project) } + let(:groups) { described_class.by_author(project) } it_behaves_like 'group by' end @@ -496,7 +496,7 @@ describe WorkPackage, type: :model do create(:project, parent: project) end - let(:groups) { WorkPackage.by_author(project) } + let(:groups) { described_class.by_author(project) } let(:work_package_3) do create(:work_package, project: project_2) @@ -523,7 +523,7 @@ describe WorkPackage, type: :model do end context 'limit' do - subject { WorkPackage.recently_updated.limit(1).first } + subject { described_class.recently_updated.limit(1).first } it { is_expected.to eq(work_package_2) } end @@ -540,7 +540,7 @@ describe WorkPackage, type: :model do project: project_archived) end - subject { WorkPackage.on_active_project.length } + subject { described_class.on_active_project.length } context 'one work package in active projects' do it { is_expected.to eq(1) } @@ -566,7 +566,7 @@ describe WorkPackage, type: :model do author: user) end - subject { WorkPackage.with_author(user).length } + subject { described_class.with_author(user).length } context 'one work package in active projects' do it { is_expected.to eq(1) } @@ -608,7 +608,7 @@ describe WorkPackage, type: :model do context 'when having the move_work_packages permission' do it 'returns the project' do - expect(WorkPackage.allowed_target_projects_on_move(user)) + expect(described_class.allowed_target_projects_on_move(user)) .to match_array [project] end end @@ -617,7 +617,7 @@ describe WorkPackage, type: :model do let(:role) { create(:role, permissions: []) } it 'does not return the project' do - expect(WorkPackage.allowed_target_projects_on_move(user)) + expect(described_class.allowed_target_projects_on_move(user)) .to be_empty end end @@ -632,7 +632,7 @@ describe WorkPackage, type: :model do context 'when having the add_work_packages permission' do it 'returns the project' do - expect(WorkPackage.allowed_target_projects_on_create(user)) + expect(described_class.allowed_target_projects_on_create(user)) .to match_array [project] end end @@ -641,7 +641,7 @@ describe WorkPackage, type: :model do let(:role) { create(:role, permissions: []) } it 'does not return the project' do - expect(WorkPackage.allowed_target_projects_on_create(user)) + expect(described_class.allowed_target_projects_on_create(user)) .to be_empty end end @@ -673,19 +673,19 @@ describe WorkPackage, type: :model do end describe 'null' do - subject { WorkPackage.changed_since(nil) } + subject { described_class.changed_since(nil) } it { expect(subject).to match_array([work_package]) } end describe 'now' do - subject { WorkPackage.changed_since(DateTime.now) } + subject { described_class.changed_since(DateTime.now) } it { expect(subject).to be_empty } end describe 'work package update' do - subject { WorkPackage.changed_since(work_package.reload.updated_at) } + subject { described_class.changed_since(work_package.reload.updated_at) } it { expect(subject).to match_array([work_package]) } end From 44996d2ec37d52d8c7974002aeb57b4058ba023e Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 10:12:54 +0200 Subject: [PATCH 18/21] allow having duration in payload representer It will only show up if the feature flag is enabled. As this will not yet be used in production it is ok to behave like that. --- .../work_package_payload_representer_spec.rb | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb index afacb57ddd..f282b3dea1 100644 --- a/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb @@ -269,32 +269,6 @@ describe ::API::V3::WorkPackages::WorkPackagePayloadRepresenter do end end end - - describe 'duration' do - before do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(true) - end - - it 'has no duration attribute' do - # TODO: Remove this, once the duration feature is complete - expect(subject).not_to have_json_path('duration') - end - - context 'when duration feature flag disabled' do - before do - allow(OpenProject::FeatureDecisions) - .to receive(:work_packages_duration_field_active?) - .and_return(false) - end - - it 'has no duration attribute' do - # TODO: Remove this, once the duration feature is complete - expect(subject).not_to have_json_path('duration') - end - end - end end describe '_links' do From b3839d420bec546a06878a5b1addc52f184cf532 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 15 Jun 2022 11:05:45 +0200 Subject: [PATCH 19/21] Make comment more explicit --- modules/backlogs/lib/open_project/backlogs/list.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/modules/backlogs/lib/open_project/backlogs/list.rb b/modules/backlogs/lib/open_project/backlogs/list.rb index c38edb717c..27cfa83b70 100644 --- a/modules/backlogs/lib/open_project/backlogs/list.rb +++ b/modules/backlogs/lib/open_project/backlogs/list.rb @@ -81,13 +81,11 @@ module OpenProject::Backlogs::List protected - # Overwriting acts/list to avoid it calling save. - # Calling save will remove the changes/saved_changes information. - # rubocop:disable Style/OptionalBooleanParameter - def set_list_position(new_position, _raise_exception_if_save_fails = false) + # Override acts_as_list implementation to avoid it calling save. + # Calling save would remove the changes/saved_changes information. + def set_list_position(new_position, _raise_exception_if_save_fails = false) # rubocop:disable Style/OptionalBooleanParameter update_columns(position: new_position) end - # rubocop:enable Style/OptionalBooleanParameter def fix_other_work_package_positions if changes.slice('project_id', 'type_id', 'version_id').present? From eb5fd5414277221b6ab325a8276ac73a1b86bd60 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 12:34:08 +0200 Subject: [PATCH 20/21] handle no duration set on copying a work package --- app/services/work_packages/set_attributes_service.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 77afb91e0b..72b35487cc 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -303,6 +303,7 @@ class WorkPackages::SetAttributesService < ::BaseServices::SetAttributes end def date_changed_but_not_duration? - (work_package.start_date_changed? || work_package.due_date_changed?) && !work_package.duration_changed? + (work_package.start_date_changed? || work_package.due_date_changed? || work_package.duration.nil?) && + !work_package.duration_changed? end end From 63bd176a8c03db3de1593244feb8c748fe896e48 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Jun 2022 13:28:18 +0200 Subject: [PATCH 21/21] replace enable_storages by more generic with_flag --- .../file_links/create_contract_spec.rb | 2 +- .../file_links/delete_contract_spec.rb | 2 +- .../project_storages/create_contract_spec.rb | 2 +- .../project_storages/delete_contract_spec.rb | 2 +- .../storages/storages/base_contract_spec.rb | 2 +- .../storages/storages/create_contract_spec.rb | 2 +- .../storages/storages/delete_contract_spec.rb | 2 +- .../storages/storages/update_contract_spec.rb | 2 +- .../spec/features/admin_storages_spec.rb | 2 +- .../create_and_delete_project_storage_spec.rb | 2 +- ...ete_project_storage_and_file_links_spec.rb | 2 +- .../spec/features/show_file_links_spec.rb | 2 +- .../manage_storage_in_project_spec.rb | 2 +- .../api/v3/file_links/file_links_spec.rb | 2 +- .../requests/api/v3/storages/storages_spec.rb | 2 +- .../work_packages_linkable_filter_spec.rb | 2 +- .../work_packages_linked_filter_spec.rb | 2 +- .../project_storages/delete_service_spec.rb | 2 +- .../spec/support/enable_storages_module.rb | 48 ------------------- .../oauth_clients/callback_flow_spec.rb | 2 +- spec/support/shared/with_flag.rb | 8 +--- 21 files changed, 21 insertions(+), 73 deletions(-) delete mode 100644 modules/storages/spec/support/enable_storages_module.rb diff --git a/modules/storages/spec/contracts/storages/file_links/create_contract_spec.rb b/modules/storages/spec/contracts/storages/file_links/create_contract_spec.rb index c43e1cbb13..9042fa4b91 100644 --- a/modules/storages/spec/contracts/storages/file_links/create_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/file_links/create_contract_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require 'contracts/shared/model_contract_shared_context' require_relative 'shared_contract_examples' -describe Storages::FileLinks::CreateContract, :enable_storages do +describe Storages::FileLinks::CreateContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' it_behaves_like 'file_link contract' do diff --git a/modules/storages/spec/contracts/storages/file_links/delete_contract_spec.rb b/modules/storages/spec/contracts/storages/file_links/delete_contract_spec.rb index 66d15f3dd1..08ad5ced0e 100644 --- a/modules/storages/spec/contracts/storages/file_links/delete_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/file_links/delete_contract_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' require_module_spec_helper require 'contracts/shared/model_contract_shared_context' -describe ::Storages::FileLinks::DeleteContract, :enable_storages do +describe ::Storages::FileLinks::DeleteContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' let(:current_user) { create(:user) } diff --git a/modules/storages/spec/contracts/storages/project_storages/create_contract_spec.rb b/modules/storages/spec/contracts/storages/project_storages/create_contract_spec.rb index a95c6ba880..58be94e376 100644 --- a/modules/storages/spec/contracts/storages/project_storages/create_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/project_storages/create_contract_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require 'contracts/shared/model_contract_shared_context' require_relative 'shared_contract_examples' -describe Storages::ProjectStorages::CreateContract, :enable_storages do +describe Storages::ProjectStorages::CreateContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' it_behaves_like 'ProjectStorages contract' do diff --git a/modules/storages/spec/contracts/storages/project_storages/delete_contract_spec.rb b/modules/storages/spec/contracts/storages/project_storages/delete_contract_spec.rb index f554ab8ea0..9cbbcba8c5 100644 --- a/modules/storages/spec/contracts/storages/project_storages/delete_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/project_storages/delete_contract_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' require_module_spec_helper require 'contracts/shared/model_contract_shared_context' -describe ::Storages::ProjectStorages::DeleteContract, :enable_storages do +describe ::Storages::ProjectStorages::DeleteContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' let(:current_user) { create(:user) } diff --git a/modules/storages/spec/contracts/storages/storages/base_contract_spec.rb b/modules/storages/spec/contracts/storages/storages/base_contract_spec.rb index 7a939606d2..487d501a54 100644 --- a/modules/storages/spec/contracts/storages/storages/base_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/base_contract_spec.rb @@ -28,7 +28,7 @@ require_relative '../../../spec_helper' -describe Storages::Storages::BaseContract, :enable_storages, :storage_server_helpers, webmock: true do +describe Storages::Storages::BaseContract, :storage_server_helpers, with_flag: { storages_module_active: true }, webmock: true do let(:current_user) { create(:admin) } let(:storage_host) { 'https://host1.example.com' } let(:storage) { build(:storage, host: storage_host) } diff --git a/modules/storages/spec/contracts/storages/storages/create_contract_spec.rb b/modules/storages/spec/contracts/storages/storages/create_contract_spec.rb index a4eee44bee..84d1b6aef5 100644 --- a/modules/storages/spec/contracts/storages/storages/create_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/create_contract_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require 'contracts/shared/model_contract_shared_context' require_relative 'shared_contract_examples' -describe Storages::Storages::CreateContract, :enable_storages do +describe Storages::Storages::CreateContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' it_behaves_like 'storage contract' do diff --git a/modules/storages/spec/contracts/storages/storages/delete_contract_spec.rb b/modules/storages/spec/contracts/storages/storages/delete_contract_spec.rb index 0453a04120..197a2ccc2b 100644 --- a/modules/storages/spec/contracts/storages/storages/delete_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/delete_contract_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' require_module_spec_helper require 'contracts/shared/model_contract_shared_context' -describe ::Storages::Storages::DeleteContract, :enable_storages do +describe ::Storages::Storages::DeleteContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' let(:storage) { create(:storage) } diff --git a/modules/storages/spec/contracts/storages/storages/update_contract_spec.rb b/modules/storages/spec/contracts/storages/storages/update_contract_spec.rb index 2a86c22de4..cf794325bf 100644 --- a/modules/storages/spec/contracts/storages/storages/update_contract_spec.rb +++ b/modules/storages/spec/contracts/storages/storages/update_contract_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require 'contracts/shared/model_contract_shared_context' require_relative 'shared_contract_examples' -describe ::Storages::Storages::UpdateContract, :enable_storages do +describe ::Storages::Storages::UpdateContract, with_flag: { storages_module_active: true } do include_context 'ModelContract shared context' it_behaves_like 'storage contract' do diff --git a/modules/storages/spec/features/admin_storages_spec.rb b/modules/storages/spec/features/admin_storages_spec.rb index ef33a04224..a63dd2bea9 100644 --- a/modules/storages/spec/features/admin_storages_spec.rb +++ b/modules/storages/spec/features/admin_storages_spec.rb @@ -28,7 +28,7 @@ require_relative '../spec_helper' -describe 'Admin storages', :enable_storages, :storage_server_helpers, type: :feature, js: true do +describe 'Admin storages', :storage_server_helpers, with_flag: { storages_module_active: true }, type: :feature, js: true do let(:admin) { create(:admin) } before do diff --git a/modules/storages/spec/features/create_and_delete_project_storage_spec.rb b/modules/storages/spec/features/create_and_delete_project_storage_spec.rb index 4425abc5e4..c40c4fca6c 100644 --- a/modules/storages/spec/features/create_and_delete_project_storage_spec.rb +++ b/modules/storages/spec/features/create_and_delete_project_storage_spec.rb @@ -31,7 +31,7 @@ require_relative '../spec_helper' # Setup storages in Project -> Settings -> File Storages # This tests assumes that a Storage has already been setup # in the Admin section, tested by admin_storage_spec.rb. -describe 'Activation of storages in projects', :enable_storages, type: :feature, js: true do +describe 'Activation of storages in projects', with_flag: { storages_module_active: true }, type: :feature, js: true do let(:user) { create(:user) } # The first page is the Project -> Settings -> General page, so we need # to provide the user with the edit_project permission in the role. diff --git a/modules/storages/spec/features/delete_project_storage_and_file_links_spec.rb b/modules/storages/spec/features/delete_project_storage_and_file_links_spec.rb index 3526acafcb..2046915afc 100644 --- a/modules/storages/spec/features/delete_project_storage_and_file_links_spec.rb +++ b/modules/storages/spec/features/delete_project_storage_and_file_links_spec.rb @@ -30,7 +30,7 @@ require_relative '../spec_helper' # Test if the deletion of a ProjectStorage actually deletes related FileLink # objects. -describe 'Delete ProjectStorage with FileLinks', :enable_storages, type: :feature, js: true do +describe 'Delete ProjectStorage with FileLinks', with_flag: { storages_module_active: true }, type: :feature, js: true do let(:user) { create(:user) } let(:role) { create(:existing_role, permissions: [:manage_storages_in_project]) } let(:project) do diff --git a/modules/storages/spec/features/show_file_links_spec.rb b/modules/storages/spec/features/show_file_links_spec.rb index 99e267982f..9fd27a0536 100644 --- a/modules/storages/spec/features/show_file_links_spec.rb +++ b/modules/storages/spec/features/show_file_links_spec.rb @@ -28,7 +28,7 @@ require_relative '../spec_helper' -describe 'Showing of file links in work package', :enable_storages, type: :feature, js: true do +describe 'Showing of file links in work package', with_flag: { storages_module_active: true }, type: :feature, js: true do let(:permissions) { %i(view_work_packages edit_work_packages view_file_links) } let(:project) { create(:project) } let(:current_user) do diff --git a/modules/storages/spec/permissions/manage_storage_in_project_spec.rb b/modules/storages/spec/permissions/manage_storage_in_project_spec.rb index f3253be905..45f3812863 100644 --- a/modules/storages/spec/permissions/manage_storage_in_project_spec.rb +++ b/modules/storages/spec/permissions/manage_storage_in_project_spec.rb @@ -33,7 +33,7 @@ require_module_spec_helper # rubocop:disable RSpec/EmptyExampleGroup describe Storages::Admin::ProjectsStoragesController, 'manage_storage_in_project permission', - :enable_storages, + with_flag: { storages_module_active: true }, type: :controller do include PermissionSpecs diff --git a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb index 40eaaffbaf..c5bc784007 100644 --- a/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb +++ b/modules/storages/spec/requests/api/v3/file_links/file_links_spec.rb @@ -29,7 +29,7 @@ require 'spec_helper' require_module_spec_helper -describe 'API v3 file links resource', :enable_storages, type: :request do +describe 'API v3 file links resource', with_flag: { storages_module_active: true }, type: :request do include API::V3::Utilities::PathHelper let(:permissions) { %i(view_work_packages view_file_links) } diff --git a/modules/storages/spec/requests/api/v3/storages/storages_spec.rb b/modules/storages/spec/requests/api/v3/storages/storages_spec.rb index d91a785d4a..d9d12f359a 100644 --- a/modules/storages/spec/requests/api/v3/storages/storages_spec.rb +++ b/modules/storages/spec/requests/api/v3/storages/storages_spec.rb @@ -29,7 +29,7 @@ require 'spec_helper' require_module_spec_helper -describe 'API v3 storages resource', :enable_storages, type: :request, content_type: :json do +describe 'API v3 storages resource', with_flag: { storages_module_active: true }, type: :request, content_type: :json do include API::V3::Utilities::PathHelper let(:permissions) { %i(view_file_links) } diff --git a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb index 6342a99d51..8e41612a04 100644 --- a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb +++ b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linkable_filter_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require_relative 'shared_filter_examples' describe 'API v3 work packages resource with filters for the linkable to storage attribute', - :enable_storages, + with_flag: { storages_module_active: true }, type: :request, content_type: :json do include API::V3::Utilities::PathHelper diff --git a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb index f70e503932..0561601326 100644 --- a/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb +++ b/modules/storages/spec/requests/api/v3/work_packages/work_packages_linked_filter_spec.rb @@ -31,7 +31,7 @@ require_module_spec_helper require_relative 'shared_filter_examples' describe 'API v3 work packages resource with filters for linked storage file', - :enable_storages, + with_flag: { storages_module_active: true }, type: :request, content_type: :json do include API::V3::Utilities::PathHelper diff --git a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb b/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb index 87db3c0ea2..e72f2ca7ad 100644 --- a/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb +++ b/modules/storages/spec/services/storages/project_storages/delete_service_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' require_module_spec_helper require 'services/base_services/behaves_like_delete_service' -describe ::Storages::ProjectStorages::DeleteService, :enable_storages, type: :model do +describe ::Storages::ProjectStorages::DeleteService, with_flag: { storages_module_active: true }, type: :model do context 'with records written to DB' do let(:user) { create(:user) } let(:role) { create(:existing_role, permissions: [:manage_storages_in_project]) } diff --git a/modules/storages/spec/support/enable_storages_module.rb b/modules/storages/spec/support/enable_storages_module.rb deleted file mode 100644 index dcc4cc4b3b..0000000000 --- a/modules/storages/spec/support/enable_storages_module.rb +++ /dev/null @@ -1,48 +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. -#++ - -RSpec.shared_context "with storages module enabled" do - let(:storages_module_active) { true } - - before do - allow(OpenProject::FeatureDecisions).to receive(:storages_module_active?).and_return(storages_module_active) - end -end - -RSpec.shared_context "with storages module disabled" do - let(:storages_module_active) { false } -end - -RSpec.configure do |rspec| - # examples tagged with `:enable_storages` will automatically have context - # included and storages module enabled - rspec.include_context "with storages module enabled", :enable_storages - # examples tagged with `disable_storages` will automatically have context - # included and storages module disabled - rspec.include_context "with storages module disabled", :disable_storages -end diff --git a/spec/requests/oauth_clients/callback_flow_spec.rb b/spec/requests/oauth_clients/callback_flow_spec.rb index a5f6d8ae62..ed357ccf26 100644 --- a/spec/requests/oauth_clients/callback_flow_spec.rb +++ b/spec/requests/oauth_clients/callback_flow_spec.rb @@ -29,7 +29,7 @@ require 'spec_helper' require 'rack/test' -describe 'OAuthClient callback endpoint', :enable_storages, type: :request do +describe 'OAuthClient callback endpoint', with_flag: { storages_module_active: true }, type: :request do include Rack::Test::Methods include API::V3::Utilities::PathHelper diff --git a/spec/support/shared/with_flag.rb b/spec/support/shared/with_flag.rb index 24ad8534e8..c2dc7eaf44 100644 --- a/spec/support/shared/with_flag.rb +++ b/spec/support/shared/with_flag.rb @@ -41,11 +41,7 @@ end RSpec.configure do |config| config.include WithFlagMixin - config.before do |example| - flags = example.metadata[:with_flag] - - if flags.present? - with_flags(flags) - end + config.before :example, :with_flag do |example| + with_flags(example.metadata[:with_flag]) end end