diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index c6209706a197..a978eb797dbc 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -354,17 +354,19 @@ def initialize_unset_custom_values end def new_start_date - return if work_package.schedule_manually? + if work_package.schedule_manually? + # Weird rule from SetScheduleService: if the work package does not have a + # start date, it inherits it from the parent soonest start, regardless of + # scheduling mode. + return if work_package.start_date - current_start_date = work_package.start_date || work_package.due_date - - return if current_start_date.nil? - - min_start = new_start_date_from_parent || new_start_date_from_self - min_start = days.soonest_working_day(min_start) + days.soonest_working_day(new_start_date_from_parent) + else + # current_start_date = work_package.start_date || work_package.due_date + # return if current_start_date.nil? - if min_start && (min_start > current_start_date || work_package.schedule_manually_changed?) - min_start + min_start = new_start_date_from_parent || new_start_date_from_self + days.soonest_working_day(min_start) end end @@ -383,7 +385,17 @@ def new_start_date_from_self def new_due_date(min_start) duration = children_duration || work_package.duration - days.due_date(min_start, duration) + return unless work_package.due_date || duration + + due_date = + if duration + days.due_date(min_start, duration) + else + work_package.due_date + end + + # if due date is before start date, then start is used as due date. + [min_start, due_date].max end def work_package diff --git a/app/services/work_packages/set_schedule_service.rb b/app/services/work_packages/set_schedule_service.rb index a6b2108c4900..601df69e5096 100644 --- a/app/services/work_packages/set_schedule_service.rb +++ b/app/services/work_packages/set_schedule_service.rb @@ -39,10 +39,6 @@ def initialize(user:, work_package:, initiated_by: nil, switching_to_automatic_m def call(changed_attributes = %i(start_date due_date)) altered = [] - if %i(parent parent_id).intersect?(changed_attributes) - altered += schedule_by_parent - end - if %i(start_date due_date parent parent_id).intersect?(changed_attributes) altered += schedule_following end @@ -58,25 +54,6 @@ def call(changed_attributes = %i(start_date due_date)) private - # rubocop:disable Metrics/AbcSize - def schedule_by_parent - work_packages - .select { |wp| wp.start_date.nil? && wp.parent } - .each do |wp| - days = WorkPackages::Shared::Days.for(wp) - wp.start_date = days.soonest_working_day(wp.parent.soonest_start) - if wp.due_date || wp.duration - wp.due_date = [ - wp.start_date, - days.due_date(wp.start_date, wp.duration), - wp.due_date - ].compact.max - assign_cause_for_journaling(wp, :parent) - end - end - end - # rubocop:enable Metrics/AbcSize - # Finds all work packages that need to be rescheduled because of a # rescheduling of the service's work package and reschedules them. # @@ -194,7 +171,6 @@ def assign_cause_for_journaling(work_package, relation) def assign_cause_initiated_by_work_package(work_package, _relation) # For now we only track a generic cause, and not a specialized reason depending on the relation # work_package.journal_cause = case relation - # when :parent then Journal::CausedByWorkPackageParentChange.new(initiated_by) # when :children then Journal::CausedByWorkPackageChildChange.new(initiated_by) # when :predecessor then Journal::CausedByWorkPackagePredecessorChange.new(initiated_by) # end diff --git a/spec/services/work_packages/set_schedule_service_spec.rb b/spec/services/work_packages/set_schedule_service_spec.rb index 453f804db727..6e175079dbd6 100644 --- a/spec/services/work_packages/set_schedule_service_spec.rb +++ b/spec/services/work_packages/set_schedule_service_spec.rb @@ -873,73 +873,6 @@ def create_child(parent, start_date, due_date, **attributes) end end - context "when setting the parent" do - let(:new_parent_work_package) { create(:work_package) } - let(:attributes) { [:parent] } - - before do - allow(new_parent_work_package) - .to receive(:soonest_start) - .and_return(soonest_date) - allow(work_package) - .to receive(:parent) - .and_return(new_parent_work_package) - end - - context "with the parent being restricted in its ability to be moved" do - let(:soonest_date) { Time.zone.today + 3.days } - - it "sets the start date and due date to the earliest possible date" do - subject - - expect(work_package.start_date).to eql(Time.zone.today + 3.days) - expect(work_package.due_date).to eql(Time.zone.today + 3.days) - end - - it "does not change the due date if after the newly set start date" do - work_package.due_date = Time.zone.today + 5.days - subject - - expect(work_package.start_date).to eql(Time.zone.today + 3.days) - expect(work_package.due_date).to eql(Time.zone.today + 5.days) - end - end - - context "with the parent being restricted but work package already having dates set" do - let(:soonest_date) { Time.zone.today + 3.days } - - before do - work_package.start_date = Time.zone.today + 4.days - work_package.due_date = Time.zone.today + 5.days - end - - it "sets the dates to provided dates" do - subject - - expect(work_package.start_date).to eql(Time.zone.today + 4.days) - expect(work_package.due_date).to eql(Time.zone.today + 5.days) - end - end - - context "with the parent being restricted but the attributes define an earlier date" do - let(:soonest_date) { Time.zone.today + 3.days } - - before do - work_package.start_date = Time.zone.today + 1.day - work_package.due_date = Time.zone.today + 2.days - end - - # This would be invalid but the dates should be set nevertheless - # so we can have a correct error handling. - it "sets the dates to provided dates" do - subject - - expect(work_package.start_date).to eql(Time.zone.today + 1.day) - expect(work_package.due_date).to eql(Time.zone.today + 2.days) - end - end - end - context "with deep hierarchy of work packages" do before do work_package.due_date = Time.zone.today - 5.days diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index 071d8b6bde57..dc229c1f09bf 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -848,7 +848,8 @@ } end - let(:attributes) { { parent: parent_work_package } } + # `schedule_manually: true` is the default value. Adding it here anyway for explicitness. + let(:attributes) { { parent: parent_work_package, schedule_manually: true } } it "sets the parent and child dates correctly" do expect(subject) @@ -972,7 +973,7 @@ end end - context "when setting the parent" do + context "when being manually scheduled and setting the parent" do let(:attributes) { { parent: new_parent } } before do @@ -1081,7 +1082,8 @@ set_factory_default(:user, user) end - context "when the work package is automatically scheduled with dates set" do + context "when the work package is automatically scheduled with both dates set " \ + "and start date is before predecessor's due date" do let_work_packages(<<~TABLE) subject | MTWTFSS | scheduling mode | predecessors new_parent_predecessor | XX | manual | @@ -1106,6 +1108,120 @@ end end + context "when the work package is automatically scheduled with both dates set after predecessor's due date" do + let_work_packages(<<~TABLE) + subject | MTWTFSS | scheduling mode | predecessors + new_parent_predecessor | XX | manual | + new_parent | XXXXXX | automatic | new_parent_predecessor + work_package | XXX | automatic | + TABLE + let(:attributes) { { parent: new_parent } } + + it "reschedules the work package to start ASAP and keeps the duration; " \ + "the parent is rescheduled like its child" do + expect(subject).to be_success + expect(work_package.reload.parent).to eq new_parent + # TODO: change to this line instead: + # expect(subject.all_results.map(&:subject)).to contain_exactly("work_package", "new_parent") + expect(subject.all_results.map(&:subject).uniq).to contain_exactly("work_package", "new_parent") + + # The work_package dates are moved to the parent's soonest start date. + # The parent dates are the same as its child. + expect_work_packages(subject.all_results + [new_parent_predecessor], <<~TABLE) + subject | MTWTFSS | scheduling mode + new_parent_predecessor | XX | manual + new_parent | XXX | automatic + work_package | XXX | automatic + TABLE + end + end + + context "when the work package is automatically scheduled without any dates set" do + let_work_packages(<<~TABLE) + subject | MTWTFSS | scheduling mode | predecessors + new_parent_predecessor | XX | manual | + new_parent | XXXXXX | automatic | new_parent_predecessor + work_package | | automatic | + TABLE + let(:attributes) { { parent: new_parent } } + + it "reschedules the work package to start ASAP and leaves its due date unset; " \ + "the parent is rescheduled to start ASAP too and end on the same day (use child start date as due date)" do + expect(subject).to be_success + expect(work_package.reload.parent).to eq new_parent + # TODO: change to this line instead: + # expect(subject.all_results.map(&:subject)).to contain_exactly("work_package", "new_parent") + expect(subject.all_results.map(&:subject).uniq).to contain_exactly("work_package", "new_parent") + + # The work_package start date is set to the parent's soonest start date. + # Both parent dates are the same as its child start date. + expect_work_packages(subject.all_results + [new_parent_predecessor], <<~TABLE) + subject | MTWTFSS | scheduling mode + new_parent_predecessor | XX | manual + new_parent | X | automatic + work_package | [ | automatic + TABLE + end + end + + context "when the work package is automatically scheduled with only a due date being set before predecessor's due date" do + let_work_packages(<<~TABLE) + subject | MTWTFSS | scheduling mode | predecessors + new_parent_predecessor | XX | manual | + new_parent | XXXXXX | automatic | new_parent_predecessor + work_package | ] | automatic | + TABLE + let(:attributes) { { parent: new_parent } } + + it "reschedules the work package to start ASAP and changes the due date to be the same as start date; " \ + "the parent is rescheduled like its child" do + expect(subject).to be_success + expect(work_package.reload.parent).to eq new_parent + # TODO: change to this line instead: + # expect(subject.all_results.map(&:subject)).to contain_exactly("work_package", "new_parent") + expect(subject.all_results.map(&:subject).uniq).to contain_exactly("work_package", "new_parent") + + # The work_package start date is set to the parent's soonest start date. + # The work_package due date is moved to the same as start date (can't start earlier). + # The parent dates are the same as its child. + expect_work_packages(subject.all_results + [new_parent_predecessor], <<~TABLE) + subject | MTWTFSS | scheduling mode + new_parent_predecessor | XX | manual + new_parent | X | automatic + work_package | X | automatic + TABLE + end + end + + context "when the work package is automatically scheduled with only a due date being set after predecessor's due date" do + let_work_packages(<<~TABLE) + subject | MTWTFSS | scheduling mode | predecessors + new_parent_predecessor | XX | manual | + new_parent | XXXXXX | automatic | new_parent_predecessor + work_package | ] | automatic | + TABLE + let(:attributes) { { parent: new_parent } } + + it "reschedules the work package to start ASAP and keeps the due date; " \ + "the parent is rescheduled like its child" do + expect(subject).to be_success + expect(work_package.reload.parent).to eq new_parent + # TODO: change to this line instead: + # expect(subject.all_results.map(&:subject)).to contain_exactly("work_package", "new_parent") + expect(subject.all_results.map(&:subject).uniq).to contain_exactly("work_package", "new_parent") + + # The work_package start date is set to the parent's soonest start date. + # The work_package due date is kept. + # The parent dates are the same as its child. + expect_work_packages(subject.all_results + [new_parent_predecessor], <<~TABLE) + subject | MTWTFSS | scheduling mode + new_parent_predecessor | XX | manual + new_parent | XXXXX | automatic + work_package | XXXXX | automatic + TABLE + end + end + context "when the work package is manually scheduled with dates set" do let_work_packages(<<~TABLE) subject | MTWTFSS | scheduling mode | predecessors