Skip to content

Commit

Permalink
Move dates handling to SetAttributesService
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 e256a4f
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 e256a4f

Please sign in to comment.