Skip to content

Commit

Permalink
Move some dates handling in SetAttributesService for automatic mode
Browse files Browse the repository at this point in the history 
Instead of doing it in both the `SetScheduleService` and
`SetAttributesService`, and have the risk of having different business
logic (and there is a different handling currently), the logic is slowly
removed from the `SetScheduleService` and moved to the
`SetAttributesService` instead.

This only concerns the work package being updated. For dependent work
packages needing to be rescheduled (ancestors and followers), the
`SetScheduleService` is still used.

Relevant tests have been moved from `spec/services/work_packages/set_schedule_service_spec.rb`
to `spec/services/work_packages/update_service_integration_spec.rb`.
  • Loading branch information
@cbliard
cbliard committed Jan 7, 2025
1 parent 0464d6c commit c0e00ed
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 104 deletions.
32 changes: 22 additions & 10 deletions app/services/work_packages/set_attributes_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
24 changes: 0 additions & 24 deletions app/services/work_packages/set_schedule_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
#
Expand Down Expand Up @@ -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
Expand Down
67 changes: 0 additions & 67 deletions spec/services/work_packages/set_schedule_service_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
122 changes: 119 additions & 3 deletions spec/services/work_packages/update_service_integration_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 |
Expand All @@ -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
Expand Down

0 comments on commit c0e00ed

Please sign in to comment.