From 968fbfa6f3ed4dc8415a3e4bb3928d0c1e4e4f49 Mon Sep 17 00:00:00 2001 From: Frank Bergmann Date: Thu, 21 Apr 2022 17:40:22 +0200 Subject: [PATCH 1/2] Simplified spec --- .../update_ancestors_est_spec.rb | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 spec/services/work_packages/update_ancestors_est_spec.rb diff --git a/spec/services/work_packages/update_ancestors_est_spec.rb b/spec/services/work_packages/update_ancestors_est_spec.rb new file mode 100644 index 0000000000..3acbd3322d --- /dev/null +++ b/spec/services/work_packages/update_ancestors_est_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' + +describe WorkPackages::UpdateAncestorsService, type: :model, with_mail: false do + let(:user) { create :user } + let(:estimated_hours) { [nil, nil, nil] } + let(:done_ratios) { [0, 0, 0] } + let(:statuses) { %i(open open open) } + let(:open_status) { create :status } + let(:closed_status) { create :closed_status } + let(:aggregate_done_ratio) { 0.0 } + + context 'with a common ancestor' do + let(:status) { open_status } + let(:done_ratio) { 50 } + let(:estimated_hours) { 7.0 } + + let!(:grandparent) do + create :work_package, + derived_estimated_hours: estimated_hours, + done_ratio: done_ratio + end + let!(:old_parent) do + create :work_package, + parent: grandparent, + derived_estimated_hours: estimated_hours, + done_ratio: done_ratio + end + let!(:new_parent) do + create :work_package, + parent: grandparent + end + let!(:work_package) do + create :work_package, + parent: old_parent, + status: status, + estimated_hours: estimated_hours, + done_ratio: done_ratio + end + + subject do + # binding.pry + work_package.parent = new_parent + # In this test case, derived_estimated_hours and done_ratio will not + # inherently change on grandparent. However, if work_package has siblings + # then changing its parent could cause derived_estimated_hours and/or + # done_ratio on grandparent to inherently change. To verify that + # grandparent can be properly updated in that case without making this + # test dependent on the implementation details of the + # derived_estimated_hours and done_ratio calculations, force + # derived_estimated_hours and done_ratio to change at the same time as the + # parent. + work_package.estimated_hours = (estimated_hours + 1) + work_package.done_ratio = (done_ratio + 1) + work_package.save! + + described_class + .new(user: user, + work_package: work_package) + .call(%i(parent)) + end + + before do + subject + end + + it 'is successful' do + expect(subject) + .to be_success + end + + it 'returns both the former and new ancestors in the dependent results without duplicates' do + expect(subject.dependent_results.map(&:result)) + .to match_array [new_parent, grandparent, old_parent] + end + + it 'updates the done_ratio of the former parent' do + expect(old_parent.reload(select: :done_ratio).done_ratio) + .to be 0 + end + + it 'updates the estimated_hours of the former parent' do + expect(old_parent.reload(select: :derived_estimated_hours).derived_estimated_hours) + .to be_nil + end + end +end From d632ba6bb6f554da4450da354a0bc0bf4533683f Mon Sep 17 00:00:00 2001 From: Frank Bergmann Date: Fri, 22 Apr 2022 11:07:27 +0200 Subject: [PATCH 2/2] [#40867] Wrong progress value in parent task - Estimated hours https://community.openproject.org/work_packages/40867 This is a second commit/pull request related to this bug. It deals with the dependency of done_ratio on estimated_hours. We need to recalculate done_ration along the hierarchy with changes in estimated_hours. --- .../work_packages/update_ancestors_service.rb | 10 ++- .../update_ancestors_est_spec.rb | 86 ------------------- .../update_ancestors_service_spec.rb | 6 +- 3 files changed, 14 insertions(+), 88 deletions(-) delete mode 100644 spec/services/work_packages/update_ancestors_est_spec.rb diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 2786deadfe..5ba290a73c 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -91,8 +91,16 @@ class WorkPackages::UpdateAncestorsService def inherit_attributes(ancestor, attributes) return unless attributes_justify_inheritance?(attributes) + # Estimated hours need to be calculated before the done_ratio below. + # The aggregation only depends on estimated hours. derive_estimated_hours(ancestor) if inherit?(attributes, :estimated_hours) - inherit_done_ratio(ancestor) if inherit?(attributes, :done_ratio) + + # Progress (done_ratio or also: percentDone) depends on both + # the completion of sub-WPs, as well as the estimated hours + # as a weight factor. So changes in estimated hours also have + # to trigger a recalculation of done_ratio. + # + inherit_done_ratio(ancestor) if inherit?(attributes, :done_ratio) || inherit?(attributes, :estimated_hours) end def inherit?(attributes, attribute) diff --git a/spec/services/work_packages/update_ancestors_est_spec.rb b/spec/services/work_packages/update_ancestors_est_spec.rb deleted file mode 100644 index 3acbd3322d..0000000000 --- a/spec/services/work_packages/update_ancestors_est_spec.rb +++ /dev/null @@ -1,86 +0,0 @@ -require 'spec_helper' - -describe WorkPackages::UpdateAncestorsService, type: :model, with_mail: false do - let(:user) { create :user } - let(:estimated_hours) { [nil, nil, nil] } - let(:done_ratios) { [0, 0, 0] } - let(:statuses) { %i(open open open) } - let(:open_status) { create :status } - let(:closed_status) { create :closed_status } - let(:aggregate_done_ratio) { 0.0 } - - context 'with a common ancestor' do - let(:status) { open_status } - let(:done_ratio) { 50 } - let(:estimated_hours) { 7.0 } - - let!(:grandparent) do - create :work_package, - derived_estimated_hours: estimated_hours, - done_ratio: done_ratio - end - let!(:old_parent) do - create :work_package, - parent: grandparent, - derived_estimated_hours: estimated_hours, - done_ratio: done_ratio - end - let!(:new_parent) do - create :work_package, - parent: grandparent - end - let!(:work_package) do - create :work_package, - parent: old_parent, - status: status, - estimated_hours: estimated_hours, - done_ratio: done_ratio - end - - subject do - # binding.pry - work_package.parent = new_parent - # In this test case, derived_estimated_hours and done_ratio will not - # inherently change on grandparent. However, if work_package has siblings - # then changing its parent could cause derived_estimated_hours and/or - # done_ratio on grandparent to inherently change. To verify that - # grandparent can be properly updated in that case without making this - # test dependent on the implementation details of the - # derived_estimated_hours and done_ratio calculations, force - # derived_estimated_hours and done_ratio to change at the same time as the - # parent. - work_package.estimated_hours = (estimated_hours + 1) - work_package.done_ratio = (done_ratio + 1) - work_package.save! - - described_class - .new(user: user, - work_package: work_package) - .call(%i(parent)) - end - - before do - subject - end - - it 'is successful' do - expect(subject) - .to be_success - end - - it 'returns both the former and new ancestors in the dependent results without duplicates' do - expect(subject.dependent_results.map(&:result)) - .to match_array [new_parent, grandparent, old_parent] - end - - it 'updates the done_ratio of the former parent' do - expect(old_parent.reload(select: :done_ratio).done_ratio) - .to be 0 - end - - it 'updates the estimated_hours of the former parent' do - expect(old_parent.reload(select: :derived_estimated_hours).derived_estimated_hours) - .to be_nil - end - end -end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 340beb1613..432387b8b8 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -76,10 +76,14 @@ describe WorkPackages::UpdateAncestorsService, type: :model, with_mail: false do let(:parent) { create :work_package, status: open_status } subject do + # In the call we only use estimated_hours (instead of also adding + # done_ratio) in order to test that changes in estimated hours + # trigger a recalculation of done_ration, because estimated hours + # act as weights in this calculation. described_class .new(user: user, work_package: children.first) - .call(%i(done_ratio estimated_hours)) + .call(%i(estimated_hours)) end context 'with no estimated hours and no progress' do