From 0de6171c3196a49271df565fa2ce8c9663aa2b90 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 27 Feb 2024 11:51:44 -0500 Subject: [PATCH] Compute remaining_hours from other fields when in Status-based mode Rules when using a status-based mode for keeping % Complete in sync with the other two work fields apply a bit differently. To ensure the correct order of operations and in-time updates, this commit moves the necessary updates to the service layer along with the computation formulas for correctly deriving fields from one another. --- .../work_packages/set_attributes_service.rb | 42 ++++++++++--- .../work_packages/update_ancestors_service.rb | 12 ++-- .../set_attributes_service_spec.rb | 60 +++++++++++++++---- 3 files changed, 90 insertions(+), 24 deletions(-) diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index cb5787b2421a..22170c96c13b 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -63,6 +63,7 @@ def set_calculated_attributes(attributes) shift_dates_to_soonest_working_days update_duration update_done_ratio + update_remaining_hours update_derivable update_project_dependent_attributes reassign_invalid_status_if_type_changed @@ -290,14 +291,21 @@ def update_duration # Unless both +remaining_hours+ and +estimated_hours+ are set, +done_ratio+ will be # considered 0. def update_done_ratio - return if WorkPackage.use_status_for_done_ratio? - return unless work_package.remaining_hours_changed? || work_package.estimated_hours_changed? + if WorkPackage.use_status_for_done_ratio? + return unless model.status_id_changed? - work_package.done_ratio = if done_ratio_dependent_attribute_unset? - 0 - else - compute_done_ratio - end + if model.status&.default_done_ratio + model.done_ratio = model.status.default_done_ratio + end + else + return unless work_package.remaining_hours_changed? || work_package.estimated_hours_changed? + + work_package.done_ratio = if done_ratio_dependent_attribute_unset? + 0 + else + compute_done_ratio + end + end end def done_ratio_dependent_attribute_unset? @@ -311,6 +319,26 @@ def compute_done_ratio (completion_ratio * 100).round(2) end + # When in "Status-based" mode for % Complete, remaining hours are based + # on the computation of it derived from the status's default done ratio + # and the estimated hours. If the estimated hours are unset, then also + # unset the remaining hours. + def update_remaining_hours + if WorkPackage.use_status_for_done_ratio? && + model.status && + model.status.default_done_ratio + model.remaining_hours = if model.estimated_hours + remaining_hours_from_done_ratio_and_estimated_hours + end + end + end + + def remaining_hours_from_done_ratio_and_estimated_hours + ((((model.done_ratio.to_f / 100) * model.estimated_hours) \ + - model.estimated_hours) \ + * -1).abs + end + def set_version_to_nil if work_package.version && work_package.project && diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 02b8b0532f38..80b6ac43424a 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -90,12 +90,18 @@ def derive_attributes(work_package, loader, attributes) # Derived estimated hours and Derived remaining hours need to be # calculated before the Derived done ratio below since the # aggregation depends on both derived fields. - # # Changes in any of these, also warrant a recalculation of # the Derived done ratio. + # + # Changes to estimated hours also warrant a recalculation of + # derived done ratios in the work package's ancestry as the + # derived estimated hours would affect the derived done ratio + # or the derived remaining hours, depending on the % Complete mode + # currently active. + # %i[estimated_hours] => :derive_estimated_hours, %i[remaining_hours] => :derive_remaining_hours, - %i[done_ratio status status_id] => :derive_done_ratio, + %i[estimated_hours done_ratio status status_id] => :derive_done_ratio, %i[ignore_non_working_days] => :derive_ignore_non_working_days }.each do |derivative_attributes, method| if attributes.intersect?(derivative_attributes + %i[parent parent_id]) @@ -114,8 +120,6 @@ def derive_done_ratio(ancestor, loader) return if initiator?(ancestor) return if WorkPackage.done_ratio_disabled? - return if WorkPackage.use_status_for_done_ratio? && ancestor.status && ancestor.status.default_done_ratio - ancestor.derived_done_ratio = compute_derived_done_ratio(ancestor, loader) || 0 end diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index db5ae2f17426..233fb9a7e03b 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -145,27 +145,61 @@ it_behaves_like 'service call' end - context 'for done_ratio' do + context 'for estimated_hours' do context 'with the status being used for done_ratio computations', with_settings: { work_package_done_ratio: 'status' } do - let!(:status) { create(:status, default_done_ratio: 99) } - let(:call_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0 } } - let(:expected_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0, done_ratio: 99 } } + context 'when the work package has estimated hours and remaining hours set along with a done ratio' do + let!(:status) { create(:status, default_done_ratio: 50) } - before do - allow(work_package).to receive(:done_ratio=) # Spying-purposes + before do + work_package.status = status + work_package.estimated_hours = 10.0 + work_package.remaining_hours = 5.0 + work_package.send(:clear_changes_information) + end - work_package.status = status - end + context 'and estimated_hours are subsequently unset' do + let(:call_attributes) { { estimated_hours: nil } } + let(:expected_attributes) { { estimated_hours: nil, remaining_hours: nil, done_ratio: 50 } } - it_behaves_like 'service call' do - it 'does not try to compute and assign done_ratio' do - subject + it_behaves_like 'service call' + end - expect(work_package) - .not_to have_received(:done_ratio=) + context 'and estimated_hours are subsequently modified' do + let(:call_attributes) { { estimated_hours: 5.0 } } + let(:expected_attributes) { { estimated_hours: 5.0, remaining_hours: 2.5, done_ratio: 50 } } + + it_behaves_like 'service call' end + + context 'and the status is subsequently changed' do + let!(:other_status) { create(:status, default_done_ratio: 70) } + let(:call_attributes) { {} } + let(:expected_attributes) { { estimated_hours: 10.0, remaining_hours: 3.0 } } + + before do + work_package.status = other_status + end + + it_behaves_like 'service call' + end + end + end + end + + context 'for done_ratio' do + context 'with the status being used for done_ratio computations', + with_settings: { work_package_done_ratio: 'status' } do + let!(:status) { create(:status, default_done_ratio: 50) } + let(:call_attributes) { { estimated_hours: 10.0 } } + # remaining hours computed from estimated hours and the status's default done ratio + let(:expected_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0, done_ratio: 50 } } + + before do + work_package.status = status end + + it_behaves_like 'service call' end describe 'is "unset"' do