TheDude
/
openproject
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #10815 from opf/fix/fix_follower_of_parent_scheduling_bug

Browse Source 
Fix scheduling dependency bug
pull/10816/head
ulferts 2 years ago committed by GitHub
parent 4df6feab07 46d338c4fb
commit 02b6d206bd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 442 additions and 120 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 117
      app/services/work_packages/schedule_dependency.rb
  2. 130
      app/services/work_packages/schedule_dependency/dependency.rb
  3. 69
      app/services/work_packages/schedule_dependency/dependency_graph.rb
  4. 160
      spec/services/work_packages/schedule_dependency/dependency_spec.rb
  5. 86
      spec/services/work_packages/set_schedule_service_spec.rb

117
app/services/work_packages/schedule_dependency.rb
Unescape View File

@ -45,23 +45,7 @@ class WorkPackages::ScheduleDependency
# * successors after predecessors
# * ancestors after descendants
def in_schedule_order
schedule_order = []
dependencies
.each_value do |dependency|
# Find the index of the last dependency the dependency needs to come after.
index = schedule_order.rindex do |inserted_dependency|
dependency.dependent_ids.include?(inserted_dependency.work_package.id)
end
if index
schedule_order.insert(index + 1, dependency)
else
schedule_order.unshift(dependency)
end
end
schedule_order.each do |dependency|
DependencyGraph.new(dependencies.values).schedule_order.each do |dependency|
yield dependency.work_package, dependency
end
end
@ -104,103 +88,4 @@ class WorkPackages::ScheduleDependency
.includes(follows_relations: :to)
.where.not(id: known_work_packages_by_id.keys)
end
class Dependency
def initialize(work_package, schedule_dependency)
self.schedule_dependency = schedule_dependency
self.work_package = work_package
end
def ancestors
@ancestors ||= ancestors_from_preloaded(work_package)
end
def descendants
@descendants ||= descendants_from_preloaded(work_package)
end
def follows_moved
@follows_moved ||= moved_predecessors_from_preloaded(work_package)
end
def follows_unmoved
@follows_unmoved ||= unmoved_predecessors_from_preloaded(work_package)
end
attr_accessor :work_package,
:schedule_dependency
def dependent_ids
@dependent_ids ||= (descendants + follows_moved.map(&:to)).map(&:id)
end
def max_date_of_followed
(follows_moved + follows_unmoved)
.map(&:successor_soonest_start)
.compact
.max
end
def start_date
descendants_dates.min
end
def due_date
descendants_dates.max
end
private
def descendants_dates
(descendants.map(&:due_date) + descendants.map(&:start_date)).compact
end
def ancestors_from_preloaded(work_package)
parent = known_work_packages_by_id[work_package.parent_id]
if parent
[parent] + ancestors_from_preloaded(parent)
else
[]
end
end
def descendants_from_preloaded(work_package)
children = known_work_packages_by_parent_id[work_package.id] || []
children + children.map { |child| descendants_from_preloaded(child) }.flatten
end
delegate :known_work_packages_by_id,
:known_work_packages_by_parent_id,
:scheduled_work_packages_by_id, to: :schedule_dependency
def scheduled_work_packages
schedule_dependency.work_packages + schedule_dependency.dependencies.keys
end
def moved_predecessors_from_preloaded(work_package)
([work_package] + ancestors + descendants)
.map(&:follows_relations)
.flatten
.map do |relation|
scheduled = scheduled_work_packages_by_id[relation.to_id]
if scheduled
relation.to = scheduled
relation
end
end
.compact
end
def unmoved_predecessors_from_preloaded(work_package)
([work_package] + ancestors + descendants)
.map(&:follows_relations)
.flatten
.reject do |relation|
scheduled_work_packages_by_id[relation.to_id].present?
end
end
end
end

130
app/services/work_packages/schedule_dependency/dependency.rb
Unescape View File

@ -0,0 +1,130 @@
#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) 2012-2022 the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++
class WorkPackages::ScheduleDependency::Dependency
def initialize(work_package, schedule_dependency)
self.schedule_dependency = schedule_dependency
self.work_package = work_package
end
def ancestors
@ancestors ||= ancestors_from_preloaded(work_package)
end
def descendants
@descendants ||= descendants_from_preloaded(work_package)
end
def follows_moved
@follows_moved ||= moved_predecessors_from_preloaded(work_package)
end
def follows_unmoved
@follows_unmoved ||= unmoved_predecessors_from_preloaded(work_package)
end
attr_accessor :work_package,
:schedule_dependency
# Returns the work package ids that the work package directly depends on.
#
# The dates of a work package depend on its descendants and predecessors
# dates.
def dependent_ids
@dependent_ids ||= (descendants + follows_moved.map(&:to)).map(&:id)
end
def max_date_of_followed
(follows_moved + follows_unmoved)
.map(&:successor_soonest_start)
.compact
.max
end
def start_date
descendants_dates.min
end
def due_date
descendants_dates.max
end
private
def descendants_dates
(descendants.map(&:due_date) + descendants.map(&:start_date)).compact
end
def ancestors_from_preloaded(work_package)
parent = known_work_packages_by_id[work_package.parent_id]
if parent
[parent] + ancestors_from_preloaded(parent)
else
[]
end
end
def descendants_from_preloaded(work_package)
children = known_work_packages_by_parent_id[work_package.id] || []
children + children.map { |child| descendants_from_preloaded(child) }.flatten
end
delegate :known_work_packages_by_id,
:known_work_packages_by_parent_id,
:scheduled_work_packages_by_id, to: :schedule_dependency
def scheduled_work_packages
schedule_dependency.work_packages + schedule_dependency.dependencies.keys
end
def moved_predecessors_from_preloaded(work_package)
([work_package] + ancestors + descendants)
.map(&:follows_relations)
.flatten
.map do |relation|
scheduled = scheduled_work_packages_by_id[relation.to_id]
if scheduled
relation.to = scheduled
relation
end
end
.compact
end
def unmoved_predecessors_from_preloaded(work_package)
([work_package] + ancestors + descendants)
.map(&:follows_relations)
.flatten
.reject do |relation|
scheduled_work_packages_by_id[relation.to_id].present?
end
end
end

69
app/services/work_packages/schedule_dependency/dependency_graph.rb
Unescape View File

@ -0,0 +1,69 @@
#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) 2012-2022 the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++
class WorkPackages::ScheduleDependency::DependencyGraph
attr_reader :dependencies
def initialize(dependencies)
@dependencies = dependencies
@dependent_ids = dependencies.to_h { |dep| [dep.work_package.id, dep.dependent_ids.uniq] }
end
def schedule_order
schedule_order = []
dependencies.each do |dependency|
# Find the index of the last dependency the dependency needs to come after.
index = schedule_order.rindex do |inserted_dependency|
depends_on?(dependency.work_package, inserted_dependency)
end
if index
schedule_order.insert(index + 1, dependency)
else
schedule_order.unshift(dependency)
end
end
schedule_order
end
# Returns true if the given work package depends on the work package of the
# dependency, either directly or transitively.
def depends_on?(work_package, dependency)
to_process_ids = [work_package.id]
processed_ids = Set.new
while id = to_process_ids.shift
processed_ids.add(id)
dependent_ids = @dependent_ids[id]
next if dependent_ids.nil?
return true if dependent_ids.include?(dependency.work_package.id)
to_process_ids.concat(dependent_ids.without(processed_ids))
end
false
end
end

160
spec/services/work_packages/schedule_dependency/dependency_spec.rb
Unescape View File

@ -0,0 +1,160 @@
#-- 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 'rails_helper'
RSpec.describe WorkPackages::ScheduleDependency::Dependency do
subject { described_class.new(work_package, schedule_dependency) }
let(:work_package) { create(:work_package, subject: 'subject') }
let(:schedule_dependency) { instance_double(WorkPackages::ScheduleDependency) }
let(:known_work_packages_by_parent_id) { Hash.new { |h, k| h[k] = [] } }
let(:known_work_packages_by_id) { { work_package.id => work_package } }
before do
allow(schedule_dependency)
.to receive(:known_work_packages_by_parent_id)
.and_return(known_work_packages_by_parent_id)
allow(schedule_dependency)
.to receive(:known_work_packages_by_id)
.and_return(known_work_packages_by_id)
allow(schedule_dependency)
.to receive(:scheduled_work_packages_by_id)
.and_return(known_work_packages_by_id)
end
def create_predecessor_of(work_package)
create(:work_package, subject: "predecessor of #{work_package.subject}").tap do |predecessor|
create(:follows_relation, from: work_package, to: predecessor)
known_work_packages_by_id[predecessor.id] = predecessor
end
end
def create_follower_of(work_package)
create(:work_package, subject: "follower of #{work_package.subject}").tap do |follower|
create(:follows_relation, from: follower, to: work_package)
known_work_packages_by_id[follower.id] = follower
end
end
def create_parent_of(work_package)
create(:work_package, subject: "parent of #{work_package.subject}", parent: work_package).tap do |parent|
known_work_packages_by_id[parent.id] = parent
known_work_packages_by_parent_id[work_package.parent_id] << parent
end
end
def create_child_of(work_package)
create(:work_package, subject: "child of #{work_package.subject}", parent: work_package).tap do |child|
known_work_packages_by_id[child.id] = child
known_work_packages_by_parent_id[child.parent_id] << child
end
end
describe '#dependent_ids' do
context 'when the work_package is not related to anything' do
it 'returns empty array' do
expect(subject.dependent_ids).to eq([])
end
end
context 'when the work_package has a predecessor' do
let!(:predecessor) { create_predecessor_of(work_package) }
it 'returns an array with the predecessor id' do
expect(subject.dependent_ids).to eq([predecessor.id])
end
end
context 'when the work_package has a follower' do
let!(:follower) { create_follower_of(work_package) }
it 'returns empty array' do
expect(subject.dependent_ids).to eq([])
end
end
context 'when the work_package has a parent' do
let!(:parent) { create_parent_of(work_package) }
it 'returns empty array' do
expect(subject.dependent_ids).to eq([])
end
end
context 'when the work_package has a child' do
let!(:child) { create_child_of(work_package) }
it 'returns an array with the child id' do
expect(subject.dependent_ids).to eq([child.id])
end
end
context 'when the work_package has multiple children and predecessors' do
let!(:child1) { create_child_of(work_package) }
let!(:child2) { create_child_of(work_package) }
let!(:predecessor1) { create_predecessor_of(work_package) }
let!(:predecessor2) { create_predecessor_of(work_package) }
it 'returns an array with the children and the predecessors ids' do
expect(subject.dependent_ids).to contain_exactly(child1.id, child2.id, predecessor1.id, predecessor2.id)
end
end
context 'with more complex relations' do
context 'when has a child which has a child' do
let!(:child) { create_child_of(work_package) }
let!(:child_child) { create_child_of(child) }
it 'returns an array with both children ids' do
expect(subject.dependent_ids).to contain_exactly(child.id, child_child.id)
end
end
context 'when has a predecessor which has a predecessor and a follower' do
let!(:predecessor) { create_predecessor_of(work_package) }
let!(:predecessor_predecessor) { create_predecessor_of(predecessor) }
let!(:predecessor_follower) { create_follower_of(predecessor) }
it 'returns an array with the first predecessor only (not transient)' do
expect(subject.dependent_ids).to contain_exactly(predecessor.id)
end
end
context 'when has a predecessor which has a parent and a child' do
let!(:predecessor) { create_predecessor_of(work_package) }
let!(:predecessor_parent) { create_parent_of(predecessor) }
let!(:predecessor_child) { create_child_of(predecessor) }
it 'returns an array with the predecessor only (not transient)' do
expect(subject.dependent_ids).to contain_exactly(predecessor.id)
end
end
end
end
end

86
spec/services/work_packages/set_schedule_service_spec.rb
Unescape View File

@ -35,6 +35,7 @@ describe WorkPackages::SetScheduleService do
let(:work_package) do
create(:work_package,
subject: 'subject',
project:,
start_date: work_package_start_date,
due_date: work_package_due_date)
@ -96,6 +97,7 @@ describe WorkPackages::SetScheduleService do
def create_follower(start_date, due_date, predecessors, parent: nil)
work_package = create(:work_package,
subject: "follower of #{predecessors.keys.map(&:subject).to_sentence}",
type:,
start_date:,
due_date:,
@ -136,12 +138,15 @@ describe WorkPackages::SetScheduleService do
expected.each do |wp, (start_date, due_date)|
result = subject.all_results.find { |result_wp| result_wp.id == wp.id }
expect(result)
.to be_present
.to be_present,
"Expected work package ##{wp.id} '#{wp.subject}' to be rescheduled"
expect(result.start_date)
.to eql start_date
.to eql(start_date),
"Expected work package ##{wp.id} '#{wp.subject}' to have start date #{start_date}, got #{result.start_date}"
expect(result.due_date)
.to eql due_date
.to eql(due_date),
"Expected work package ##{wp.id} '#{wp.subject}' to have due date #{due_date}, got #{result.due_date}"
duration = if start_date && due_date
(due_date - start_date + 1).to_i
@ -151,7 +156,8 @@ describe WorkPackages::SetScheduleService do
end
expect(result.duration)
.to eql duration
.to eql(duration),
"Expected work package ##{wp.id} '#{wp.subject}' to have duration #{duration}, got #{result.duration}"
end
end
@ -492,6 +498,78 @@ describe WorkPackages::SetScheduleService do
end
end
context 'with a parent having a follower' do
let(:work_package_start_date) { Time.zone.today }
let(:work_package_due_date) { Time.zone.today + 5.days }
let!(:parent_work_package) do
create(:work_package,
subject: "parent of #{work_package.subject}",
start_date: Time.zone.today,
due_date: Time.zone.today + 1.day).tap do |parent|
work_package.parent = parent
work_package.save
end
end
let!(:follower_of_parent_work_package) do
create_follower(Time.zone.today + 4.days,
Time.zone.today + 6.days,
{ parent_work_package => 0 })
end
it_behaves_like 'reschedules' do
let(:expected) do
{ parent_work_package => [work_package_start_date, work_package_due_date],
follower_of_parent_work_package => [work_package_due_date + 1.day, work_package_due_date + 3.days] }
end
end
# There is a bug in the scheduling that happens if the dependencies
# array order is: [sibling child, follower of parent, parent]
#
# In this case, as the follower of parent only knows about direct
# dependencies (and not about the transitive dependencies of children of
# predecessor), it will be made the first in the order, based on the
# current algorithm. And as the parent depends on its child, it will
# come after it.
#
# Based on the algorithm when this test was written, the resulting
# scheduling order will be [follower of parent, sibling child, parent],
# which is wrong: if follower of parent is rescheduled first, then it
# will not change because its predecessor, the parent, has not been
# scheduled yet.
#
# The expected and right order is [sibling child, parent, follower of
# parent].
#
# That's why the WorkPackage.for_scheduling call is mocked to customize
# the order of the returned work_packages to reproduce this bug.
context 'with also a sibling follower with same parent' do
let!(:sibling_follower_of_work_package) do
create_follower(Time.zone.today + 2.days,
Time.zone.today + 3.days,
{ work_package => 0 },
parent: parent_work_package)
end
before do
allow(WorkPackage)
.to receive(:for_scheduling)
.and_wrap_original do |method, *args|
wanted_order = [sibling_follower_of_work_package, follower_of_parent_work_package, parent_work_package]
method.call(*args).in_order_of(:id, wanted_order.map(&:id))
end
end
it_behaves_like 'reschedules' do
let(:expected) do
{ sibling_follower_of_work_package => [work_package_due_date + 1.day, work_package_due_date + 2.days],
parent_work_package => [work_package_start_date, work_package_due_date + 2.days],
follower_of_parent_work_package => [work_package_due_date + 3.days, work_package_due_date + 5.days] }
end
end
end
end
context 'with a single successor having a parent' do
let!(:following) do
[following_work_package1,

Write Preview
Loading…
Cancel
Save