From 291b87b78c951b67d7cf70bc8da9d00947c48bf1 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Fri, 6 Sep 2024 07:50:18 -0500 Subject: [PATCH 01/22] Create setting for Calculation of % Complete hierarchy totals * Adds the settings to the definitions file. * Sets the default and allowed values. --- config/constants/settings/definition.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/constants/settings/definition.rb b/config/constants/settings/definition.rb index 1f403b76cae8..e2bf418d9669 100644 --- a/config/constants/settings/definition.rb +++ b/config/constants/settings/definition.rb @@ -231,6 +231,11 @@ class Definition default: nil, writable: false }, + total_percent_complete_mode: { + description: "Mode in which the total % Complete for work packages in a hierarchy is calculated", + default: "work_weighted_average", + allowed: %w[work_weighted_average simple_average] + }, commit_fix_keywords: { description: "Keywords to look for in commit for fixing work packages", default: "fixes,closes" From b92e9d12a421a6e3fa20164594abc4d30f7c7fac Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 10 Sep 2024 09:39:14 -0500 Subject: [PATCH 02/22] Incorporate simple average mode into total % complete calculations * Adds setting into derivation code. * Adds specs for the simple average mode calculation. --- .../work_packages/update_ancestors/loader.rb | 2 + .../work_packages/update_ancestors_service.rb | 24 ++- .../update_ancestors_service_spec.rb | 187 +++++++++++++++++- 3 files changed, 207 insertions(+), 6 deletions(-) diff --git a/app/services/work_packages/update_ancestors/loader.rb b/app/services/work_packages/update_ancestors/loader.rb index 966a04fd4620..dcf4ae002740 100644 --- a/app/services/work_packages/update_ancestors/loader.rb +++ b/app/services/work_packages/update_ancestors/loader.rb @@ -30,6 +30,8 @@ class WorkPackages::UpdateAncestors::Loader parent_id: "parent_id", estimated_hours: "estimated_hours", remaining_hours: "remaining_hours", + done_ratio: "done_ratio", + derived_done_ratio: "derived_done_ratio", status_excluded_from_totals: "statuses.excluded_from_totals", schedule_manually: "schedule_manually", ignore_non_working_days: "ignore_non_working_days" diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index b6f0c3e0d78f..b08e756a90f5 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -120,13 +120,27 @@ def derive_done_ratio(ancestor, loader) end def compute_derived_done_ratio(work_package, loader) - return if work_package.derived_estimated_hours.nil? || work_package.derived_remaining_hours.nil? - return if work_package.derived_estimated_hours.zero? return if no_children?(work_package, loader) - work_done = (work_package.derived_estimated_hours - work_package.derived_remaining_hours) - progress = (work_done.to_f / work_package.derived_estimated_hours) * 100 - progress.round + if Setting.total_percent_complete_mode == "work_weighted_average" + return if work_package.derived_estimated_hours.nil? || work_package.derived_remaining_hours.nil? + return if work_package.derived_estimated_hours.zero? + + work_done = (work_package.derived_estimated_hours - work_package.derived_remaining_hours) + progress = (work_done.to_f / work_package.derived_estimated_hours) * 100 + progress.round + elsif Setting.total_percent_complete_mode == "simple_average" + all_done_ratios = loader + .children_of(work_package) + .map { |child| child.done_ratio || 0 } + + if work_package.done_ratio.present? + all_done_ratios << work_package.done_ratio + end + + progress = all_done_ratios.sum.to_f / all_done_ratios.count + progress.round + end end # Sets the ignore_non_working_days to true if any descendant has its value set to true. diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 98f6fb1ec3fc..dbf020659f53 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -28,7 +28,8 @@ require "spec_helper" -RSpec.describe WorkPackages::UpdateAncestorsService, type: :model do +RSpec.describe WorkPackages::UpdateAncestorsService, + type: :model do shared_association_default(:author, factory_name: :user) { create(:user) } shared_association_default(:project_with_types) { create(:project_with_types) } shared_association_default(:priority) { create(:priority) } @@ -795,6 +796,190 @@ def call_update_ancestors_service(work_package) end end + describe "simple average mode for total % complete calculation", + with_settings: { total_percent_complete_mode: "simple_average" } do + subject(:call_result) do + described_class.new(user:, work_package: parent) + .call(%i(remaining_hours)) + end + + context "with parent and all children all values set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 40h | 7.5h | 22.5h | 25% | + child1 | 15h | | 10h | | 33% | + child2 | 5h | | 2.5h | | 50% | + child3 | 10h | | 2.5h | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of children and parent" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 46% + TABLE + end + end + + context "with parent having no values and children having all values set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | 30h | | 15h | | + child1 | 15h | | 10h | | 33% | + child2 | 5h | | 2.5h | | 50% | + child3 | 10h | | 2.5h | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of children" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 53% + TABLE + end + end + + context "with parent having no values set " \ + "and some children having work and remaining work set " \ + "and all children having % complete set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | 25h | | 12.5h | | + child1 | 15h | | 10h | | 33% | + child2 | | | | | 75% | + child3 | 10h | | 2.5h | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of children" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 61% + TABLE + end + end + + context "with parent having no values set " \ + "and no children having work and remaining work set " \ + "and all children having % complete set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | | + child1 | | | | | 100% | + child2 | | | | | 100% | + child3 | | | | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of children" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 92% + TABLE + end + end + + context "with parent having no values set " \ + "and no children having work and remaining work set " \ + "and some children having % complete set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | | + child1 | | | | | 100% | + child2 | | | | | | + child3 | | | | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of children " \ + "and accounts unset values in children as 0" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 58% + TABLE + end + end + + context "with parent having % complete set " \ + "and no children having work and remaining work set " \ + "and some children having % complete set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 10% | + child1 | | | | | 100% | + child2 | | | | | | + child3 | | | | | 75% | + TABLE + + it "sets the total % complete based on % complete values of children and parent " \ + "and accounts unset values in children as 0" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 46% + TABLE + end + end + + context "with parent having no values set " \ + "and a multi-level children hierarchy with all values set" do + let_work_packages(<<~TABLE) + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | 44h | | 21h | | 52% + child1 | 15h | 23h | 10h | 14h | 33% | 43% + grandchild1 | 5h | | 3h | | 40% | + grandchild2 | 3h | | 1h | | 67% | + child2 | 5h | 7h | 2.5h | 3.5h | 50% | 50% + grandchild3 | 2h | | 1h | | 50% | + child3 | 10h | 14h | 2.5h | 3.5h | 75% | 75% + grandchild4 | 4h | | 1h | | 75% | + TABLE + + it "sets the total % complete solely based on % complete values of direct children" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 53% + TABLE + end + end + end + + describe "work weighted average mode for total % complete calculation", + with_settings: { total_percent_complete_mode: "work_weighted_average" } do + subject(:call_result) do + described_class.new(user:, work_package: parent) + .call(%i(remaining_hours)) + end + + context "with parent and all children all values set" do + let_work_packages(<<~TABLE) + hierarchy | work | remaining work | % complete + parent | 10h | 7.5h | 25% + child1 | 15h | 10h | 33% + child2 | 5h | 2.5h | 50% + child3 | 10h | 2.5h | 75% + TABLE + + it "sets the total % complete accounting for work values as weights" do + pending "TODO" + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 46% + TABLE + end + end + end + describe "ignore_non_working_days propagation" do shared_let(:grandgrandparent) do create(:work_package, From 53a3a7266cab2eb9ae0e6dd5ea30ab60e311a187 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Mon, 16 Sep 2024 11:45:38 -0500 Subject: [PATCH 03/22] Adjust selection of done ratio based on new rules for children --- .../work_packages/update_ancestors_service.rb | 41 +++++++++++-------- .../update_ancestors_service_spec.rb | 33 +++++++-------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index b08e756a90f5..7e44b2387408 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -122,25 +122,34 @@ def derive_done_ratio(ancestor, loader) def compute_derived_done_ratio(work_package, loader) return if no_children?(work_package, loader) - if Setting.total_percent_complete_mode == "work_weighted_average" - return if work_package.derived_estimated_hours.nil? || work_package.derived_remaining_hours.nil? - return if work_package.derived_estimated_hours.zero? - - work_done = (work_package.derived_estimated_hours - work_package.derived_remaining_hours) - progress = (work_done.to_f / work_package.derived_estimated_hours) * 100 - progress.round - elsif Setting.total_percent_complete_mode == "simple_average" - all_done_ratios = loader - .children_of(work_package) - .map { |child| child.done_ratio || 0 } + case Setting.total_percent_complete_mode + when "work_weighted_average" + calculate_work_weighted_average_percent_complete(work_package) + when "simple_average" + calculate_simple_average_percent_complete(work_package, loader) + end + end - if work_package.done_ratio.present? - all_done_ratios << work_package.done_ratio - end + def calculate_work_weighted_average_percent_complete(work_package) + return if work_package.derived_estimated_hours.nil? || work_package.derived_remaining_hours.nil? + return if work_package.derived_estimated_hours.zero? - progress = all_done_ratios.sum.to_f / all_done_ratios.count - progress.round + work_done = (work_package.derived_estimated_hours - work_package.derived_remaining_hours) + progress = (work_done.to_f / work_package.derived_estimated_hours) * 100 + progress.round + end + + def calculate_simple_average_percent_complete(work_package, loader) + all_done_ratios = loader + .children_of(work_package) + .map { |child| child.derived_done_ratio || child.done_ratio || 0 } + + if work_package.done_ratio.present? + all_done_ratios << work_package.done_ratio end + + progress = all_done_ratios.sum.to_f / all_done_ratios.count + progress.round end # Sets the ignore_non_working_days to true if any descendant has its value set to true. diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index dbf020659f53..57e5cfeddeae 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -946,35 +946,32 @@ def call_update_ancestors_service(work_package) updated_work_packages = call_result.all_results expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete - parent | 53% + parent | 56% TABLE end end - end - - describe "work weighted average mode for total % complete calculation", - with_settings: { total_percent_complete_mode: "work_weighted_average" } do - subject(:call_result) do - described_class.new(user:, work_package: parent) - .call(%i(remaining_hours)) - end - context "with parent and all children all values set" do + context "with parent having % complete set " \ + "and a multi-level children hierarchy with some % complete set" do let_work_packages(<<~TABLE) - hierarchy | work | remaining work | % complete - parent | 10h | 7.5h | 25% - child1 | 15h | 10h | 33% - child2 | 5h | 2.5h | 50% - child3 | 10h | 2.5h | 75% + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | 44h | | 21h | 10% | + child1 | 15h | 23h | 10h | 14h | 33% | 43% + grandchild1 | 5h | | 3h | | 40% | + grandchild2 | 3h | | 1h | | 67% | + child2 | 5h | 7h | 2.5h | 3.5h | 50% | 75% + grandchild3 | 2h | | 1h | | 100% | + child3 | 10h | 14h | 2.5h | 3.5h | 75% | 75% + grandchild4 | 4h | | 1h | | 75% | + child4 | | | | | 60% | TABLE - it "sets the total % complete accounting for work values as weights" do - pending "TODO" + it "sets the total % complete solely based on % complete values of direct children" do expect(call_result).to be_success updated_work_packages = call_result.all_results expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete - parent | 46% + parent | 53% TABLE end end From 793d7581f352cb8922017336b9bd271e2a0e5e0e Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Mon, 16 Sep 2024 12:00:55 -0500 Subject: [PATCH 04/22] Add radio group to progress tracking form Now the settings can be changed from the UI. --- app/models/work_package.rb | 1 + app/views/admin/settings/progress_tracking/show.html.erb | 7 ++++--- config/locales/en.yml | 8 ++++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 100b19aac31a..51d387b21e6a 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -45,6 +45,7 @@ class WorkPackage < ApplicationRecord include OpenProject::Journal::AttachmentHelper DONE_RATIO_OPTIONS = %w[field status].freeze + TOTAL_PERCENT_COMPLETE_MODE_OPTIONS = %w[work_weighted_average simple_average].freeze belongs_to :project belongs_to :type diff --git a/app/views/admin/settings/progress_tracking/show.html.erb b/app/views/admin/settings/progress_tracking/show.html.erb index 391de8f1a506..1e32f237b000 100644 --- a/app/views/admin/settings/progress_tracking/show.html.erb +++ b/app/views/admin/settings/progress_tracking/show.html.erb @@ -26,9 +26,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. See COPYRIGHT and LICENSE files for more details. ++#%> - <% html_title t(:label_administration), t(:label_work_package_plural), t(:label_progress_tracking) -%> - <%= render Primer::OpenProject::PageHeader.new do |header| header.with_title { t(:label_progress_tracking) } @@ -37,7 +35,6 @@ See COPYRIGHT and LICENSE files for more details. t(:label_progress_tracking)]) end %> - <%= primer_form_with( scope: :settings, action: :update, method: :patch, @@ -70,6 +67,10 @@ primer_form_with( disabled: WorkPackage.status_based_mode?, data: { admin__progress_tracking_target: "statusClosedRadioGroup" } ) + form.radio_button_group( + name: "total_percent_complete_mode", + values: WorkPackage::TOTAL_PERCENT_COMPLETE_MODE_OPTIONS + ) form.submit end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 2194c9f433e5..fb7c765bc2b6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3370,6 +3370,14 @@ en: setting_sys_api_enabled: "Enable repository management web service" setting_sys_api_description: "The repository management web service provides integration and user authorization for accessing repositories." setting_time_format: "Time" + setting_total_percent_complete_mode: "Calculation of % Complete hierarchy totals" + setting_total_percent_complete_mode_work_weighted_average: "Weighted by work" + setting_total_percent_complete_mode_work_weighted_average_caption_html: >- + The total % Complete will be weighted against the Work of each work package in the hierarchy. + Work packages without Work will be ignored (current behaviour). + setting_total_percent_complete_mode_simple_average: "Simple average" + setting_total_percent_complete_mode_simple_average_caption_html: >- + Work is ignored and the total % Complete will be a simple average of % Complete values of work packages in the hierarchy. setting_accessibility_mode_for_anonymous: "Enable accessibility mode for anonymous users" setting_user_format: "Users name format" setting_user_default_timezone: "Users default time zone" From 06bd83f8f8bbb2013ae004462f79b6ff52b854e1 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 17 Sep 2024 17:14:59 -0500 Subject: [PATCH 05/22] Add feature spec for setting toggling * Fixes cuprite flag to enable cuprite in this spec file. --- spec/features/admin/progress_tracking_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/features/admin/progress_tracking_spec.rb b/spec/features/admin/progress_tracking_spec.rb index c103592437f8..480014d6e18c 100644 --- a/spec/features/admin/progress_tracking_spec.rb +++ b/spec/features/admin/progress_tracking_spec.rb @@ -28,7 +28,9 @@ require "spec_helper" -RSpec.describe "Progress tracking admin page", :cuprite, :js, +RSpec.describe "Progress tracking admin page", + :js, + :with_cuprite, with_flag: { percent_complete_edition: true } do include ActionView::Helpers::SanitizeHelper include Toasts::Expectations From 503ad154cabd6a5c83de9c7f9b47a47a414e6752 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Thu, 19 Sep 2024 12:29:37 -0500 Subject: [PATCH 06/22] Add recalculation in job when switching to work weighted mode * Missing to add journal entries --- ...e_mode_changed_to_work_weighted_average.rb | 33 +++ ..._total_percent_complete_mode_change_job.rb | 143 +++++++++++ .../work_packages/progress/sql_commands.rb | 56 +++++ ...l_percent_complete_mode_change_job_spec.rb | 235 ++++++++++++++++++ 4 files changed, 467 insertions(+) create mode 100644 app/models/journal/caused_by_total_percent_complete_mode_changed_to_work_weighted_average.rb create mode 100644 app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb create mode 100644 spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb diff --git a/app/models/journal/caused_by_total_percent_complete_mode_changed_to_work_weighted_average.rb b/app/models/journal/caused_by_total_percent_complete_mode_changed_to_work_weighted_average.rb new file mode 100644 index 000000000000..b1d2ce41a29c --- /dev/null +++ b/app/models/journal/caused_by_total_percent_complete_mode_changed_to_work_weighted_average.rb @@ -0,0 +1,33 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 Journal::CausedByTotalPercentCompleteModeChangedToWorkWeightedAverage < CauseOfChange::Base + def initialize + super("total_percent_complete_mode_changed_to_work_weighted_average") + end +end diff --git a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb new file mode 100644 index 000000000000..518d1004959a --- /dev/null +++ b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb @@ -0,0 +1,143 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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::Progress::ApplyTotalPercentCompleteModeChangeJob < WorkPackages::Progress::Job + VALID_CAUSE_TYPES = %w[ + total_percent_complete_mode_changed_to_work_weighted_average + total_percent_complete_mode_changed_to_simple_average + ].freeze + + attr_reader :cause_type, :old_mode, :new_mode + + # Updates the total % complete of all work packages after the total + # percent complete mode has been changed. + # + # It creates a journal entry with the System user describing the changes. + # + # + # Updates the total % complete of all work packages after the total + # percent complete mode has been changed. + # + # It creates a journal entry with the System user describing the changes. + # + # @param [String] old_mode The previous total percent complete mode + # @param [String] new_mode The new total percent complete mode + # @return [void] + def perform(cause_type:, old_mode:, new_mode:) + @cause_type = cause_type + @old_mode = old_mode + @new_mode = new_mode + + with_temporary_total_percent_complete_table do + update_total_percent_complete + copy_total_percent_complete_values_to_work_packages_and_update_journals(journal_cause) + end + end + + private + + def update_total_percent_complete + case new_mode + when "work_weighted_average" + update_work_weighted_average + when "simple_average" + update_simple_average + else + raise ArgumentError, "Invalid total percent complete mode: #{new_mode}" + end + end + + def update_work_weighted_average + execute(<<~SQL.squish) + UPDATE temp_wp_progress_values + SET total_p_complete = CASE + WHEN total_work IS NULL OR total_remaining_work IS NULL THEN NULL + WHEN total_work = 0 THEN NULL + ELSE ROUND( + ((total_work - total_remaining_work)::float / total_work) * 100 + ) + END + WHERE id IN ( + SELECT ancestor_id + FROM work_package_hierarchies + GROUP BY ancestor_id + HAVING MAX(generations) > 0 + ) + SQL + end + + def update_simple_average + execute(<<~SQL.squish) + UPDATE temp_wp_progress_values + SET derived_done_ratio = CASE + WHEN avg_ratios.avg_done_ratio IS NOT NULL THEN ROUND(avg_ratios.avg_done_ratio) + ELSE done_ratio + END + FROM ( + SELECT wp_tree.ancestor_id AS id, + AVG(CASE + WHEN wp_tree.generations = 1 THEN COALESCE(wp_progress.done_ratio, 0) + ELSE NULL + END) AS avg_done_ratio + FROM work_package_hierarchies wp_tree + LEFT JOIN temp_wp_progress_values wp_progress ON wp_tree.descendant_id = wp_progress.id + LEFT JOIN statuses ON wp_progress.status_id = statuses.id + WHERE statuses.excluded_from_totals = FALSE + GROUP BY wp_tree.ancestor_id + ) avg_ratios + WHERE temp_wp_progress_values.id = avg_ratios.id + AND temp_wp_progress_values.id IN ( + SELECT ancestor_id AS id + FROM work_package_hierarchies + GROUP BY id + HAVING MAX(generations) > 0 + ) + SQL + end + + def journal_cause + assert_valid_cause_type! + + @journal_cause ||= + case cause_type + when "total_percent_complete_mode_changed_to_work_weighted_average" + Journal::CausedByTotalPercentCompleteModeChangedToWorkWeightedAverage.new + when "total_percent_complete_mode_changed_to_simple_average" + Journal::CausedByTotalPercentCompleteModeChangedToSimpleAverage.new + else + raise "Unable to handle cause type #{cause_type.inspect}" + end + end + + def assert_valid_cause_type! + unless VALID_CAUSE_TYPES.include?(cause_type) + raise ArgumentError, "Invalid cause type #{cause_type.inspect}. " \ + "Valid values are #{VALID_CAUSE_TYPES.inspect}" + end + end +end diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index e798b21f3eda..671a2959dfc8 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -60,6 +60,41 @@ def drop_temporary_progress_table SQL end + def with_temporary_total_percent_complete_table + WorkPackage.transaction do + case new_mode + when "work_weighted_average" + create_temporary_total_percent_complete_table_for_work_weighted_average_mode + when "simple_average" + create_temporary_total_percent_complete_table_for_simple_average_mode + else + raise ArgumentError, "Invalid total percent complete mode: #{new_mode}" + end + + yield + ensure + drop_temporary_total_percent_complete_table + end + end + + def create_temporary_total_percent_complete_table_for_work_weighted_average_mode + execute(<<~SQL.squish) + CREATE UNLOGGED TABLE temp_wp_progress_values + AS SELECT + id, + derived_estimated_hours as total_work, + derived_remaining_hours as total_remaining_work, + derived_done_ratio as total_p_complete + FROM work_packages + SQL + end + + def drop_temporary_total_percent_complete_table + execute(<<~SQL.squish) + DROP TABLE temp_wp_progress_values + SQL + end + def derive_remaining_work_from_work_and_percent_complete execute(<<~SQL.squish) UPDATE temp_wp_progress_values @@ -170,6 +205,27 @@ def copy_progress_values_to_work_packages results.column_values(0) end + def copy_total_percent_complete_values_to_work_packages_and_update_journals(cause) + updated_work_package_ids = copy_total_percent_complete_values_to_work_packages + create_journals_for_updated_work_packages(updated_work_package_ids, cause:) + end + + def copy_total_percent_complete_values_to_work_packages + results = execute(<<~SQL.squish) + UPDATE work_packages + SET derived_done_ratio = temp_wp_progress_values.total_p_complete, + lock_version = lock_version + 1, + updated_at = NOW() + FROM temp_wp_progress_values + WHERE work_packages.id = temp_wp_progress_values.id + AND ( + work_packages.derived_done_ratio IS DISTINCT FROM temp_wp_progress_values.total_p_complete + ) + RETURNING work_packages.id + SQL + results.column_values(0) + end + def create_journals_for_updated_work_packages(updated_work_package_ids, cause:) WorkPackage.where(id: updated_work_package_ids).find_each do |work_package| Journals::CreateService diff --git a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb new file mode 100644 index 000000000000..51e0cc38e06c --- /dev/null +++ b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb @@ -0,0 +1,235 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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::Progress::ApplyTotalPercentCompleteModeChangeJob do + shared_let(:author) { create(:user) } + shared_let(:priority) { create(:priority, name: "Normal") } + shared_let(:project) { create(:project, name: "Main project") } + + # statuses for work-based mode + shared_let(:status_new) { create(:status, name: "New") } + shared_let(:status_wip) { create(:status, name: "In progress") } + shared_let(:status_closed) { create(:status, name: "Closed") } + + # statuses for status-based mode + shared_let(:status_0p_todo) { create(:status, name: "To do (0%)", default_done_ratio: 0) } + shared_let(:status_40p_doing) { create(:status, name: "Doing (40%)", default_done_ratio: 40) } + shared_let(:status_100p_done) { create(:status, name: "Done (100%)", default_done_ratio: 100) } + + # statuses for both work-based and status-based modes + shared_let(:status_excluded) { create(:status, :excluded_from_totals, name: "Excluded") } + + before_all do + set_factory_default(:user, author) + set_factory_default(:priority, priority) + set_factory_default(:project, project) + set_factory_default(:project_with_types, project) + set_factory_default(:status, status_new) + end + + subject(:job) { described_class } + + def expect_performing_job_changes(from:, to:, + cause_type: "total_percent_complete_mode_changed_to_work_weighted_average", + old_mode: "simple_average", + new_mode: "work_weighted_average") + table = create_table(from) + + job.perform_now(cause_type:, old_mode:, new_mode:) + + table.work_packages.map(&:reload) + expect_work_packages(table.work_packages, to) + + table.work_packages + end + + context "when changing from simple average to work weighted average mode", + with_settings: { total_percent_complete_mode: "work_weighted_average" } do + context "on a single-level hierarchy" do + it "updates the total % complete of the work packages" do + expect_performing_job_changes( + old_mode: "simple_average", + new_mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + flat_wp_1 | 10h | | 6h | | 40% | + flat_wp_2 | 5h | | 3h | | 60% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + flat_wp_1 | 10h | | 6h | | 40% | + flat_wp_2 | 5h | | 3h | | 60% | + TABLE + ) + end + end + + context "on a two-level hierarchy with parents having total values" do + it "updates the total % complete of parent work packages" do + expect_performing_job_changes( + old_mode: "simple_average", + new_mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 30h | 6h | 6h | 40% | 70% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 30h | 6h | 6h | 40% | 80% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | | 0h | | 100% | + TABLE + ) + end + end + + context "on a two-level hierarchy with only % complete values set" do + it "unsets the % complete value from parents" do + expect_performing_job_changes( + old_mode: "simple_average", + new_mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | 70% + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + TABLE + ) + end + end + + context "on a multi-level hierarchy with only % complete values set" do + it "unsets the % complete value from parents" do + expect_performing_job_changes( + old_mode: "simple_average", + new_mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | 63% + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | 70% + grandchild1 | | | | | 40% | + grandchild2 | | | | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + grandchild1 | | | | | 40% | + grandchild2 | | | | | 100% | + TABLE + ) + end + end + + context "on a multi-level hierarchy with work and remaining work values set" do + it "updates the total % complete of parent work packages" do + expect_performing_job_changes( + old_mode: "simple_average", + new_mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 63% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 70% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 88% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 100% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + ) + end + end + + describe "journal entries" do + it "is still not done" do + pending "TODO: Add specs for the journal entries created" + raise StandardError, "Not implemented" + end + end + end + + context "with errors during job execution" do + let_work_packages(<<~TABLE) + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + wp | 10h | 10h | 6h | 6h | 40% | 40% + wp 0% | 10h | 10h | 10h | 10h | 0% | 0% + wp 40% | 10h | 10h | 6h | 6h | 40% | 40% + wp 100% | 10h | 10h | 0h | 0h | 100% | 100% + TABLE + + before do + job.perform_now(cause_type: "should make it blow up!", + old_mode: "simple_average", + new_mode: "work_weighted_average") + rescue StandardError + # Catch the error to continue the test + end + + it "does not update any work package" do + expect_work_packages(WorkPackage.all, <<~TABLE) + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + wp | 10h | 10h | 6h | 6h | 40% | 40% + wp 0% | 10h | 10h | 10h | 10h | 0% | 0% + wp 40% | 10h | 10h | 6h | 6h | 40% | 40% + wp 100% | 10h | 10h | 0h | 0h | 100% | 100% + TABLE + end + + it "cleans up temporary database artifacts used throughout the job" do + expect( + ActiveRecord::Base.connection.table_exists?("temp_wp_progress_values") + ).to be(false) + end + end +end From 5e36ce2580454528c24555a7260a11f2c500b871 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Fri, 20 Sep 2024 12:40:42 -0500 Subject: [PATCH 07/22] Re-order radio group options according to design document Calculation of % Complete hierarchy totals should be the second setting on the form --- app/views/admin/settings/progress_tracking/show.html.erb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/admin/settings/progress_tracking/show.html.erb b/app/views/admin/settings/progress_tracking/show.html.erb index 1e32f237b000..cb81766f37b9 100644 --- a/app/views/admin/settings/progress_tracking/show.html.erb +++ b/app/views/admin/settings/progress_tracking/show.html.erb @@ -62,15 +62,15 @@ primer_form_with( end end end + form.radio_button_group( + name: "total_percent_complete_mode", + values: WorkPackage::TOTAL_PERCENT_COMPLETE_MODE_OPTIONS + ) form.radio_button_group( name: "percent_complete_on_status_closed", disabled: WorkPackage.status_based_mode?, data: { admin__progress_tracking_target: "statusClosedRadioGroup" } ) - form.radio_button_group( - name: "total_percent_complete_mode", - values: WorkPackage::TOTAL_PERCENT_COMPLETE_MODE_OPTIONS - ) form.submit end end From 6cbfdc8e784fbfa2c505ad3eac8b3ef3b3fa5390 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Fri, 20 Sep 2024 12:41:22 -0500 Subject: [PATCH 08/22] Trigger the recalculation job when changing to work weighted mode * Triggers job * Adds journal entries with specs and translations --- app/models/journal.rb | 1 + app/services/settings/update_service.rb | 4 ++ ..._total_percent_complete_mode_change_job.rb | 23 ++++--- .../work_packages/progress/sql_commands.rb | 4 +- config/locales/en.yml | 3 + lib/open_project/journal_formatter/cause.rb | 6 ++ .../progress_tracking_controller_spec.rb | 25 +++++++- spec/lib/journal_formatter/cause_spec.rb | 16 +++++ ...l_percent_complete_mode_change_job_spec.rb | 61 +++++++++++++------ 9 files changed, 110 insertions(+), 33 deletions(-) diff --git a/app/models/journal.rb b/app/models/journal.rb index d393198b9132..6a004db89db5 100644 --- a/app/models/journal.rb +++ b/app/models/journal.rb @@ -77,6 +77,7 @@ class Journal < ApplicationRecord progress_mode_changed_to_status_based status_changed system_update + total_percent_complete_mode_changed_to_work_weighted_average work_package_children_changed_times work_package_parent_changed_times work_package_predecessor_changed_times diff --git a/app/services/settings/update_service.rb b/app/services/settings/update_service.rb index a8eda6a5e1dc..0cab0e99ceb8 100644 --- a/app/services/settings/update_service.rb +++ b/app/services/settings/update_service.rb @@ -47,6 +47,10 @@ def set_setting_value(name, value) Setting[name] = new_value if name == :work_package_done_ratio && old_value != "status" && new_value == "status" WorkPackages::Progress::ApplyStatusesChangeJob.perform_later(cause_type: "progress_mode_changed_to_status_based") + elsif name == :total_percent_complete_mode && old_value != "work_weighted_average" && new_value == "work_weighted_average" + WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob + .perform_later(mode: new_value, + cause_type: "total_percent_complete_mode_changed_to_work_weighted_average") end end diff --git a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb index 518d1004959a..b97b1ed1701a 100644 --- a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb +++ b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb @@ -32,7 +32,7 @@ class WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob < WorkPacka total_percent_complete_mode_changed_to_simple_average ].freeze - attr_reader :cause_type, :old_mode, :new_mode + attr_reader :cause_type, :mode # Updates the total % complete of all work packages after the total # percent complete mode has been changed. @@ -45,13 +45,12 @@ class WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob < WorkPacka # # It creates a journal entry with the System user describing the changes. # - # @param [String] old_mode The previous total percent complete mode - # @param [String] new_mode The new total percent complete mode + # @param [String] cause_type The cause type of the change + # @param [String] mode The new total percent complete mode # @return [void] - def perform(cause_type:, old_mode:, new_mode:) + def perform(cause_type:, mode:) @cause_type = cause_type - @old_mode = old_mode - @new_mode = new_mode + @mode = mode with_temporary_total_percent_complete_table do update_total_percent_complete @@ -62,17 +61,17 @@ def perform(cause_type:, old_mode:, new_mode:) private def update_total_percent_complete - case new_mode + case mode when "work_weighted_average" - update_work_weighted_average + update_to_work_weighted_average when "simple_average" - update_simple_average + update_to_simple_average else - raise ArgumentError, "Invalid total percent complete mode: #{new_mode}" + raise ArgumentError, "Invalid total percent complete mode: #{mode}" end end - def update_work_weighted_average + def update_to_work_weighted_average execute(<<~SQL.squish) UPDATE temp_wp_progress_values SET total_p_complete = CASE @@ -91,7 +90,7 @@ def update_work_weighted_average SQL end - def update_simple_average + def update_to_simple_average execute(<<~SQL.squish) UPDATE temp_wp_progress_values SET derived_done_ratio = CASE diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index 671a2959dfc8..c142597637df 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -62,13 +62,13 @@ def drop_temporary_progress_table def with_temporary_total_percent_complete_table WorkPackage.transaction do - case new_mode + case mode when "work_weighted_average" create_temporary_total_percent_complete_table_for_work_weighted_average_mode when "simple_average" create_temporary_total_percent_complete_table_for_simple_average_mode else - raise ArgumentError, "Invalid total percent complete mode: #{new_mode}" + raise ArgumentError, "Invalid total percent complete mode: #{mode}" end yield diff --git a/config/locales/en.yml b/config/locales/en.yml index fb7c765bc2b6..c8f52f7b5079 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1915,6 +1915,7 @@ en: progress_mode_changed_to_status_based: "Progress calculation updated" status_changed: "Status '%{status_name}'" system_update: "OpenProject system update:" + total_percent_complete_mode_changed_to_work_weighted_average: "Calculation of % Complete totals now weighted by Work." cause_descriptions: work_package_predecessor_changed_times: by changes to predecessor %{link} @@ -1945,6 +1946,8 @@ en: totals_removed_from_childless_work_packages: >- Work and progress totals automatically removed for non-parent work packages with version update. This is a maintenance task and can be safely ignored. + total_percent_complete_mode_changed_to_work_weighted_average: >- + Child work packages without Work are ignored. links: configuration_guide: "Configuration guide" diff --git a/lib/open_project/journal_formatter/cause.rb b/lib/open_project/journal_formatter/cause.rb index 08107c316877..1674bf78ef06 100644 --- a/lib/open_project/journal_formatter/cause.rb +++ b/lib/open_project/journal_formatter/cause.rb @@ -77,6 +77,8 @@ def cause_description status_changed_message when "progress_mode_changed_to_status_based" progress_mode_changed_to_status_based_message + when "total_percent_complete_mode_changed_to_work_weighted_average" + total_percent_complete_mode_changed_to_work_weighted_average_message else related_work_package_changed_message end @@ -143,6 +145,10 @@ def progress_mode_changed_to_status_based_message I18n.t("journals.cause_descriptions.progress_mode_changed_to_status_based") end + def total_percent_complete_mode_changed_to_work_weighted_average_message + I18n.t("journals.cause_descriptions.total_percent_complete_mode_changed_to_work_weighted_average") + end + def related_work_package_changed_message related_work_package = WorkPackage.includes(:project).visible(User.current).find_by(id: cause["work_package_id"]) diff --git a/spec/controllers/admin/settings/progress_tracking_controller_spec.rb b/spec/controllers/admin/settings/progress_tracking_controller_spec.rb index 20485f8328b3..b65bcae1f276 100644 --- a/spec/controllers/admin/settings/progress_tracking_controller_spec.rb +++ b/spec/controllers/admin/settings/progress_tracking_controller_spec.rb @@ -64,7 +64,7 @@ end end - context "when sending path request to change progress calculation from status-based to status-based" do + context "when sending patch request to change progress calculation from status-based to status-based" do before do Setting.work_package_done_ratio = "status" end @@ -97,4 +97,27 @@ .not_to have_been_enqueued end end + + context "when changing total percent complete mode from simple average to work-weighted average" do + before do + Setting.total_percent_complete_mode = "simple_average" + end + + it "starts a job to update work packages' total % complete values" do + patch "update", + params: { + settings: { + total_percent_complete_mode: "work_weighted_average" + } + } + expect(WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob) + .to have_been_enqueued.with(old_mode: "simple_average", + new_mode: "work_weighted_average", + cause_type: "total_percent_complete_mode_changed_to_work_weighted_average") + + perform_enqueued_jobs + + expect(Setting.total_percent_complete_mode).to eq("work_weighted_average") + end + end end diff --git a/spec/lib/journal_formatter/cause_spec.rb b/spec/lib/journal_formatter/cause_spec.rb index 649e74df9c1e..565bfbaeb6f2 100644 --- a/spec/lib/journal_formatter/cause_spec.rb +++ b/spec/lib/journal_formatter/cause_spec.rb @@ -398,6 +398,22 @@ def render(cause, html:) end end + context "when a change of total percent complete mode from " \ + "simple average to work-weighted average is the cause" do + subject(:cause) do + { + "type" => "total_percent_complete_mode_changed_to_work_weighted_average" + } + end + + it do + expect(cause).to render_html_variant( + "Calculation of % Complete totals now weighted by Work. " \ + "Child work packages without Work are ignored." + ) + end + end + context "when both a change of status % complete and excluded from totals is the cause" do shared_let(:status) { create(:status, name: "In progress", default_done_ratio: 40) } subject(:cause) do diff --git a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb index 51e0cc38e06c..03caf455e814 100644 --- a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb @@ -58,11 +58,10 @@ def expect_performing_job_changes(from:, to:, cause_type: "total_percent_complete_mode_changed_to_work_weighted_average", - old_mode: "simple_average", - new_mode: "work_weighted_average") + mode: "work_weighted_average") table = create_table(from) - job.perform_now(cause_type:, old_mode:, new_mode:) + job.perform_now(cause_type:, mode:) table.work_packages.map(&:reload) expect_work_packages(table.work_packages, to) @@ -75,8 +74,7 @@ def expect_performing_job_changes(from:, to:, context "on a single-level hierarchy" do it "updates the total % complete of the work packages" do expect_performing_job_changes( - old_mode: "simple_average", - new_mode: "work_weighted_average", + mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete flat_wp_1 | 10h | | 6h | | 40% | @@ -94,8 +92,7 @@ def expect_performing_job_changes(from:, to:, context "on a two-level hierarchy with parents having total values" do it "updates the total % complete of parent work packages" do expect_performing_job_changes( - old_mode: "simple_average", - new_mode: "work_weighted_average", + mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete parent | 10h | 30h | 6h | 6h | 40% | 70% @@ -117,8 +114,7 @@ def expect_performing_job_changes(from:, to:, context "on a two-level hierarchy with only % complete values set" do it "unsets the % complete value from parents" do expect_performing_job_changes( - old_mode: "simple_average", - new_mode: "work_weighted_average", + mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete parent | | | | | 40% | 70% @@ -140,8 +136,7 @@ def expect_performing_job_changes(from:, to:, context "on a multi-level hierarchy with only % complete values set" do it "unsets the % complete value from parents" do expect_performing_job_changes( - old_mode: "simple_average", - new_mode: "work_weighted_average", + mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete parent | | | | | 40% | 63% @@ -167,8 +162,7 @@ def expect_performing_job_changes(from:, to:, context "on a multi-level hierarchy with work and remaining work values set" do it "updates the total % complete of parent work packages" do expect_performing_job_changes( - old_mode: "simple_average", - new_mode: "work_weighted_average", + mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete parent | 10h | 50h | 6h | 6h | 40% | 63% @@ -192,10 +186,42 @@ def expect_performing_job_changes(from:, to:, end describe "journal entries" do - it "is still not done" do - pending "TODO: Add specs for the journal entries created" - raise StandardError, "Not implemented" + # rubocop:disable RSpec/ExampleLength + it "creates journal entries for the modified work packages" do + parent, child1, child2, child3, grandchild1, grandchild2 = expect_performing_job_changes( + mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 63% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 70% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 88% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 100% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + ) + [parent, child3].each do |work_package| + expect(work_package.journals.count).to eq 2 + last_journal = work_package.last_journal + expect(last_journal.user).to eq(User.system) + expect(last_journal.cause_type).to eq("total_percent_complete_mode_changed_to_work_weighted_average") + end + + # unchanged => no new journals + [child1, child2, grandchild1, grandchild2].each do |work_package| + expect(work_package.journals.count).to eq 1 + end end + # rubocop:enable RSpec/ExampleLength end end @@ -210,8 +236,7 @@ def expect_performing_job_changes(from:, to:, before do job.perform_now(cause_type: "should make it blow up!", - old_mode: "simple_average", - new_mode: "work_weighted_average") + mode: "work_weighted_average") rescue StandardError # Catch the error to continue the test end From 2f2561c08ff2587bbe6cb72ec497e2dc9e7803d3 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Mon, 23 Sep 2024 17:54:23 -0500 Subject: [PATCH 09/22] Add in simple average calculation mode job * Adds temporary depth table - Could be cleaned up down thie line with the `work_package_hierarchies` table. --- ...complete_mode_changed_to_simple_average.rb | 33 ++++ app/services/settings/update_service.rb | 4 + ..._total_percent_complete_mode_change_job.rb | 79 ++++++--- .../work_packages/progress/sql_commands.rb | 72 +++++++- config/locales/en.yml | 5 +- lib/open_project/journal_formatter/cause.rb | 6 + .../progress_tracking_controller_spec.rb | 25 ++- ...l_percent_complete_mode_change_job_spec.rb | 163 +++++++++++++++++- 8 files changed, 352 insertions(+), 35 deletions(-) create mode 100644 app/models/journal/caused_by_total_percent_complete_mode_changed_to_simple_average.rb diff --git a/app/models/journal/caused_by_total_percent_complete_mode_changed_to_simple_average.rb b/app/models/journal/caused_by_total_percent_complete_mode_changed_to_simple_average.rb new file mode 100644 index 000000000000..2adcf77163df --- /dev/null +++ b/app/models/journal/caused_by_total_percent_complete_mode_changed_to_simple_average.rb @@ -0,0 +1,33 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 Journal::CausedByTotalPercentCompleteModeChangedToSimpleAverage < CauseOfChange::Base + def initialize + super("total_percent_complete_mode_changed_to_simple_average") + end +end diff --git a/app/services/settings/update_service.rb b/app/services/settings/update_service.rb index 0cab0e99ceb8..0744628f29fa 100644 --- a/app/services/settings/update_service.rb +++ b/app/services/settings/update_service.rb @@ -51,6 +51,10 @@ def set_setting_value(name, value) WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob .perform_later(mode: new_value, cause_type: "total_percent_complete_mode_changed_to_work_weighted_average") + elsif name == :total_percent_complete_mode && old_value != "simple_average" && new_value == "simple_average" + WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob + .perform_later(mode: new_value, + cause_type: "total_percent_complete_mode_changed_to_simple_average") end end diff --git a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb index b97b1ed1701a..7feb4fc7e2c8 100644 --- a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb +++ b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb @@ -92,30 +92,61 @@ def update_to_work_weighted_average def update_to_simple_average execute(<<~SQL.squish) - UPDATE temp_wp_progress_values - SET derived_done_ratio = CASE - WHEN avg_ratios.avg_done_ratio IS NOT NULL THEN ROUND(avg_ratios.avg_done_ratio) - ELSE done_ratio - END - FROM ( - SELECT wp_tree.ancestor_id AS id, - AVG(CASE - WHEN wp_tree.generations = 1 THEN COALESCE(wp_progress.done_ratio, 0) - ELSE NULL - END) AS avg_done_ratio - FROM work_package_hierarchies wp_tree - LEFT JOIN temp_wp_progress_values wp_progress ON wp_tree.descendant_id = wp_progress.id - LEFT JOIN statuses ON wp_progress.status_id = statuses.id - WHERE statuses.excluded_from_totals = FALSE - GROUP BY wp_tree.ancestor_id - ) avg_ratios - WHERE temp_wp_progress_values.id = avg_ratios.id - AND temp_wp_progress_values.id IN ( - SELECT ancestor_id AS id - FROM work_package_hierarchies - GROUP BY id - HAVING MAX(generations) > 0 - ) + DO $$ + DECLARE + min_depth INTEGER := 0; + max_depth INTEGER := (SELECT MAX(depth) FROM temp_work_package_depth); + current_depth INTEGER := min_depth; + BEGIN + /* Navigate work packages and perform updates bottom-up */ + while current_depth <= max_depth loop + UPDATE temp_wp_progress_values wp + SET + total_p_complete = CASE + WHEN current_depth = min_depth THEN NULL + ELSE ROUND( + ( + COALESCE(wp.p_complete, 0) + ( + SELECT + SUM( + COALESCE(child_wp.total_p_complete, child_wp.p_complete, 0) + ) + FROM + temp_wp_progress_values child_wp + WHERE + child_wp.parent_id = wp.id + ) + ) / ( + CASE + WHEN wp.p_complete IS NOT NULL THEN 1 + ELSE 0 + END + ( + SELECT + COUNT(1) + FROM + temp_wp_progress_values child_wp + WHERE + child_wp.parent_id = wp.id + ) + ) + ) + END + /* Select only work packages at the curren depth */ + WHERE + wp.id IN ( + SELECT + id + FROM + temp_work_package_depth + WHERE + depth = current_depth + ); + + /* Go up a level from a child to a parent*/ + current_depth := current_depth + 1; + + END loop; + END $$; SQL end diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index c142597637df..ce4ff402aea0 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -67,6 +67,7 @@ def with_temporary_total_percent_complete_table create_temporary_total_percent_complete_table_for_work_weighted_average_mode when "simple_average" create_temporary_total_percent_complete_table_for_simple_average_mode + create_temporary_depth_table else raise ArgumentError, "Invalid total percent complete mode: #{mode}" end @@ -74,24 +75,86 @@ def with_temporary_total_percent_complete_table yield ensure drop_temporary_total_percent_complete_table + drop_temporary_depth_table end end def create_temporary_total_percent_complete_table_for_work_weighted_average_mode execute(<<~SQL.squish) - CREATE UNLOGGED TABLE temp_wp_progress_values - AS SELECT + CREATE UNLOGGED TABLE temp_wp_progress_values AS + SELECT id, derived_estimated_hours as total_work, derived_remaining_hours as total_remaining_work, - derived_done_ratio as total_p_complete + derived_done_ratio as total_p_complete + FROM work_packages + SQL + end + + def create_temporary_total_percent_complete_table_for_simple_average_mode + execute(<<~SQL.squish) + CREATE UNLOGGED TABLE temp_wp_progress_values AS + SELECT + id, + parent_id, + status_id, + done_ratio AS p_complete, + NULL::INTEGER AS total_p_complete FROM work_packages SQL end + def create_temporary_depth_table + execute(<<~SQL.squish) + CREATE UNLOGGED TABLE temp_work_package_depth AS + WITH RECURSIVE + work_package_depth AS ( + /* Base case: Leaves (work packages with no children) */ + SELECT + wp.id, + wp.parent_id, + 0 AS depth + FROM + temp_wp_progress_values wp + WHERE + NOT EXISTS ( + SELECT + 1 + FROM + temp_wp_progress_values c + WHERE + c.parent_id = wp.id + ) + UNION ALL + /* Recursive case: Parents */ + SELECT + wp.parent_id AS id, + wp2.parent_id, + wpd.depth + 1 AS depth + FROM + work_packages wp + INNER JOIN work_package_depth wpd ON wp.id = wpd.id + INNER JOIN temp_wp_progress_values wp2 ON wp.parent_id = wp2.id + WHERE + wp.parent_id IS NOT NULL + ) + SELECT + id, + depth + FROM + work_package_depth + SQL + end + def drop_temporary_total_percent_complete_table execute(<<~SQL.squish) - DROP TABLE temp_wp_progress_values + DROP TABLE IF EXISTS temp_wp_progress_values + SQL + end + + def drop_temporary_depth_table + execute(<<~SQL.squish) + DROP TABLE IF EXISTS temp_work_package_depth SQL end @@ -223,6 +286,7 @@ def copy_total_percent_complete_values_to_work_packages ) RETURNING work_packages.id SQL + results.column_values(0) end diff --git a/config/locales/en.yml b/config/locales/en.yml index c8f52f7b5079..f9a9845c5397 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1916,7 +1916,7 @@ en: status_changed: "Status '%{status_name}'" system_update: "OpenProject system update:" total_percent_complete_mode_changed_to_work_weighted_average: "Calculation of % Complete totals now weighted by Work." - + total_percent_complete_mode_changed_to_simple_average: "Calculation of % Complete totals now based on a simple average of only % Complete values." cause_descriptions: work_package_predecessor_changed_times: by changes to predecessor %{link} work_package_parent_changed_times: by changes to parent %{link} @@ -1948,7 +1948,8 @@ en: This is a maintenance task and can be safely ignored. total_percent_complete_mode_changed_to_work_weighted_average: >- Child work packages without Work are ignored. - + total_percent_complete_mode_changed_to_simple_average: >- + Work values of child work packages are ignored. links: configuration_guide: "Configuration guide" get_in_touch: "You have questions? Get in touch with us." diff --git a/lib/open_project/journal_formatter/cause.rb b/lib/open_project/journal_formatter/cause.rb index 1674bf78ef06..6148cbf82f9b 100644 --- a/lib/open_project/journal_formatter/cause.rb +++ b/lib/open_project/journal_formatter/cause.rb @@ -79,6 +79,8 @@ def cause_description progress_mode_changed_to_status_based_message when "total_percent_complete_mode_changed_to_work_weighted_average" total_percent_complete_mode_changed_to_work_weighted_average_message + when "total_percent_complete_mode_changed_to_simple_average" + total_percent_complete_mode_changed_to_simple_average_message else related_work_package_changed_message end @@ -149,6 +151,10 @@ def total_percent_complete_mode_changed_to_work_weighted_average_message I18n.t("journals.cause_descriptions.total_percent_complete_mode_changed_to_work_weighted_average") end + def total_percent_complete_mode_changed_to_simple_average_message + I18n.t("journals.cause_descriptions.total_percent_complete_mode_changed_to_simple_average") + end + def related_work_package_changed_message related_work_package = WorkPackage.includes(:project).visible(User.current).find_by(id: cause["work_package_id"]) diff --git a/spec/controllers/admin/settings/progress_tracking_controller_spec.rb b/spec/controllers/admin/settings/progress_tracking_controller_spec.rb index b65bcae1f276..0b6f89bbbee3 100644 --- a/spec/controllers/admin/settings/progress_tracking_controller_spec.rb +++ b/spec/controllers/admin/settings/progress_tracking_controller_spec.rb @@ -111,8 +111,7 @@ } } expect(WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob) - .to have_been_enqueued.with(old_mode: "simple_average", - new_mode: "work_weighted_average", + .to have_been_enqueued.with(mode: "work_weighted_average", cause_type: "total_percent_complete_mode_changed_to_work_weighted_average") perform_enqueued_jobs @@ -120,4 +119,26 @@ expect(Setting.total_percent_complete_mode).to eq("work_weighted_average") end end + + context "when changing total percent complete mode from work-weighted average to simple average" do + before do + Setting.total_percent_complete_mode = "work_weighted_average" + end + + it "starts a job to update work packages' total % complete values" do + patch "update", + params: { + settings: { + total_percent_complete_mode: "simple_average" + } + } + expect(WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob) + .to have_been_enqueued.with(mode: "simple_average", + cause_type: "total_percent_complete_mode_changed_to_simple_average") + + perform_enqueued_jobs + + expect(Setting.total_percent_complete_mode).to eq("simple_average") + end + end end diff --git a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb index 03caf455e814..4ad6cae47099 100644 --- a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb @@ -72,7 +72,7 @@ def expect_performing_job_changes(from:, to:, context "when changing from simple average to work weighted average mode", with_settings: { total_percent_complete_mode: "work_weighted_average" } do context "on a single-level hierarchy" do - it "updates the total % complete of the work packages" do + it "does not update the total % complete of the work packages" do expect_performing_job_changes( mode: "work_weighted_average", from: <<~TABLE, @@ -139,10 +139,10 @@ def expect_performing_job_changes(from:, to:, mode: "work_weighted_average", from: <<~TABLE, hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete - parent | | | | | 40% | 63% + parent | | | | | 40% | 65% child1 | | | | | 100% | child2 | | | | | 40% | - child3 | | | | | 100% | 70% + child3 | | | | | 100% | 80% grandchild1 | | | | | 40% | grandchild2 | | | | | 100% | TABLE @@ -225,6 +225,163 @@ def expect_performing_job_changes(from:, to:, end end + context "when changing from work weighted average to simple average mode", + with_settings: { total_percent_complete_mode: "simple_average" } do + context "on a single-level hierarchy" do + it "does not update the total % complete of the work packages" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + flat_wp_1 | 10h | | 6h | | 40% | + flat_wp_2 | 5h | | 3h | | 60% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + flat_wp_1 | 10h | | 6h | | 40% | + flat_wp_2 | 5h | | 3h | | 60% | + TABLE + ) + end + end + + context "on a two-level hierarchy with parents having total values" do + it "updates the total % complete of parent work packages" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 30h | 6h | 6h | 40% | 80% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 30h | 6h | 6h | 40% | 70% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | | 0h | | 100% | + TABLE + ) + end + end + + context "on a two-level hierarchy with only % complete values set" do + it "sets the % complete value from parents" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | 70% + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + TABLE + ) + end + end + + context "on a multi-level hierarchy with only % complete values set" do + it "unsets the % complete value from parents" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | + grandchild1 | | | | | 40% | + grandchild2 | | | | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | | | | | 40% | 65% + child1 | | | | | 100% | + child2 | | | | | 40% | + child3 | | | | | 100% | 80% + grandchild1 | | | | | 40% | + grandchild2 | | | | | 100% | + TABLE + ) + end + end + + context "on a multi-level hierarchy with work and remaining work values set" do + it "updates the total % complete of parent work packages irrelevant of work values" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 88% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 100% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 65% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 80% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + ) + end + end + + describe "journal entries" do + # rubocop:disable RSpec/ExampleLength + it "creates journal entries for the modified work packages" do + parent, child1, child2, child3, grandchild1, grandchild2 = expect_performing_job_changes( + cause_type: "total_percent_complete_mode_changed_to_simple_average", + mode: "simple_average", + from: <<~TABLE, + hierarchy | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 88% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 100% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | 10h | 50h | 6h | 6h | 40% | 65% + child1 | 15h | | 0h | | 100% | + child2 | | | | | 40% | + child3 | 5h | 25h | 0h | 0h | 100% | 80% + grandchild1 | | | | | 40% | + grandchild2 | 20h | | 0h | | 100% | + TABLE + ) + [parent, child3].each do |work_package| + expect(work_package.journals.count).to eq 2 + last_journal = work_package.last_journal + expect(last_journal.user).to eq(User.system) + expect(last_journal.cause_type).to eq("total_percent_complete_mode_changed_to_simple_average") + end + + # unchanged => no new journals + [child1, child2, grandchild1, grandchild2].each do |work_package| + expect(work_package.journals.count).to eq 1 + end + end + # rubocop:enable RSpec/ExampleLength + end + end + context "with errors during job execution" do let_work_packages(<<~TABLE) subject | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete From 1f12393880265481bb453565562c7bf11d5be9fb Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 24 Sep 2024 16:56:36 -0500 Subject: [PATCH 10/22] Account for status exclusion from total calculations in simple average mode --- .../work_packages/update_ancestors_service.rb | 13 +++-- ..._total_percent_complete_mode_change_job.rb | 23 ++++++-- .../work_packages/progress/sql_commands.rb | 7 ++- .../update_ancestors_service_spec.rb | 26 +++++++++ ...l_percent_complete_mode_change_job_spec.rb | 55 +++++++++++++++++++ 5 files changed, 111 insertions(+), 13 deletions(-) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 7e44b2387408..e70189edeafe 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -140,11 +140,9 @@ def calculate_work_weighted_average_percent_complete(work_package) end def calculate_simple_average_percent_complete(work_package, loader) - all_done_ratios = loader - .children_of(work_package) - .map { |child| child.derived_done_ratio || child.done_ratio || 0 } + all_done_ratios = children_done_ratio_values(work_package, loader) - if work_package.done_ratio.present? + if work_package.done_ratio.present? && !work_package.status.excluded_from_totals all_done_ratios << work_package.done_ratio end @@ -152,6 +150,13 @@ def calculate_simple_average_percent_complete(work_package, loader) progress.round end + def children_done_ratio_values(work_package, loader) + loader + .children_of(work_package) + .reject(&:status_excluded_from_totals) + .map { |child| child.derived_done_ratio || child.done_ratio || 0 } + end + # Sets the ignore_non_working_days to true if any descendant has its value set to true. # If there is no value returned from the descendants, that means that the work package in # question no longer has a descendant. But since we are in the service going up the ancestor chain, diff --git a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb index 7feb4fc7e2c8..a2c614497d6c 100644 --- a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb +++ b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb @@ -106,7 +106,12 @@ def update_to_simple_average WHEN current_depth = min_depth THEN NULL ELSE ROUND( ( - COALESCE(wp.p_complete, 0) + ( + /* Exclude the current work package if it has a status excluded from totals */ + CASE WHEN wp.status_excluded_from_totals + THEN 0 + /* Otherwise, use the current work package's % complete value or 0 if unset */ + ELSE COALESCE(wp.p_complete, 0) + END + ( SELECT SUM( COALESCE(child_wp.total_p_complete, child_wp.p_complete, 0) @@ -115,18 +120,24 @@ def update_to_simple_average temp_wp_progress_values child_wp WHERE child_wp.parent_id = wp.id + /* Exclude children with a status excluded from totals */ + AND NOT child_wp.status_excluded_from_totals ) - ) / ( - CASE - WHEN wp.p_complete IS NOT NULL THEN 1 - ELSE 0 - END + ( + ) / ( + /* Exclude the current work package if it has a status excluded from totals */ + CASE WHEN wp.status_excluded_from_totals + THEN 0 + /* Otherwise, count the current work package if it has a % complete value set */ + ELSE(CASE WHEN wp.p_complete IS NOT NULL THEN 1 ELSE 0 END) + END + ( SELECT COUNT(1) FROM temp_wp_progress_values child_wp WHERE child_wp.parent_id = wp.id + /* Exclude children with a status excluded from totals */ + AND NOT child_wp.status_excluded_from_totals ) ) ) diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index ce4ff402aea0..4f2c77771202 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -95,12 +95,13 @@ def create_temporary_total_percent_complete_table_for_simple_average_mode execute(<<~SQL.squish) CREATE UNLOGGED TABLE temp_wp_progress_values AS SELECT - id, - parent_id, - status_id, + work_packages.id as id, + work_packages.parent_id as parent_id, + statuses.excluded_from_totals AS status_excluded_from_totals, done_ratio AS p_complete, NULL::INTEGER AS total_p_complete FROM work_packages + LEFT JOIN statuses ON work_packages.status_id = statuses.id SQL end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index 57e5cfeddeae..dac34b0aabcc 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -975,6 +975,32 @@ def call_update_ancestors_service(work_package) TABLE end end + + context "with parent having % complete set " \ + "and a multi-level children hierarchy with some % complete set " \ + "and some children having status excluded from totals" do + let_work_packages(<<~TABLE) + hierarchy | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | Open | | 44h | | 21h | 10% | + child1 | Open | 15h | 23h | 10h | 14h | 33% | 43% + grandchild1 | Open | 5h | | 3h | | 40% | + grandchild2 | Open | 3h | | 1h | | 67% | + child2 | Rejected | 5h | 7h | 2.5h | 3.5h | 50% | 75% + grandchild3 | Open | 2h | | 1h | | 100% | + child3 | Open | 10h | 14h | 2.5h | 3.5h | 75% | + grandchild4 | Rejected | 4h | | 1h | | 75% | + child4 | Open | | | | | 60% | + TABLE + + it "sets the total % complete solely based on % complete values of direct children" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | 47% + TABLE + end + end end describe "ignore_non_working_days propagation" do diff --git a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb index 4ad6cae47099..5e393754f545 100644 --- a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb @@ -185,6 +185,33 @@ def expect_performing_job_changes(from:, to:, end end + context "on a multi-level hierarchy with work and remaining work values set " \ + "and a child with a status excluded from totals" do + it "updates the total % complete of parent work packages excluding the child" do + expect_performing_job_changes( + mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | New | 10h | 30h | 6h | 6h | 40% | 63% + child1 | New | 15h | | 0h | | 100% | + child2 | New | | | | | 40% | + child3 | New | 5h | 5h | 0h | 0h | 100% | 70% + grandchild1 | New | | | | | 40% | + grandchild2 | Excluded | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | New | 10h | 30h | 6h | 6h | 40% | 80% + child1 | New | 15h | | 0h | | 100% | + child2 | New | | | | | 40% | + child3 | New | 5h | 5h | 0h | 0h | 100% | 100% + grandchild1 | New | | | | | 40% | + grandchild2 | Excluded | 20h | | 0h | | 100% | + TABLE + ) + end + end + describe "journal entries" do # rubocop:disable RSpec/ExampleLength it "creates journal entries for the modified work packages" do @@ -341,6 +368,34 @@ def expect_performing_job_changes(from:, to:, end end + context "on a multi-level hierarchy with work and remaining work values set " \ + "and a child having a status excluded from totals" do + it "updates the total % complete of parent work packages irrelevant of work values " \ + "excluding the child with the excluded status" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | New | 10h | 50h | 6h | 6h | 40% | 88% + child1 | New | 15h | | 0h | | 100% | + child2 | New | | | | | 40% | + child3 | Excluded | 5h | 25h | 0h | 0h | 100% | 100% + grandchild1 | New | | | | | 40% | + grandchild2 | New | 20h | | 0h | | 100% | + TABLE + to: <<~TABLE + subject | status | work | ∑ work | remaining work | ∑ remaining work | % complete | ∑ % complete + parent | New | 10h | 50h | 6h | 6h | 40% | 60% + child1 | New | 15h | | 0h | | 100% | + child2 | New | | | | | 40% | + child3 | Excluded | 5h | 25h | 0h | 0h | 100% | 70% + grandchild1 | New | | | | | 40% | + grandchild2 | New | 20h | | 0h | | 100% | + TABLE + ) + end + end + describe "journal entries" do # rubocop:disable RSpec/ExampleLength it "creates journal entries for the modified work packages" do From 1874fb3003ecbfc472bb68b3b9049cbb044a2364 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 11:55:56 +0200 Subject: [PATCH 11/22] Fix `status_excluded_from_totals` not existing for `WorkPackage` In `WorkPackages::UpdateAncestors::Loader`, the returned elements are not always `WorkPackageLikeStruct` instances: whenever possible the loader reuses knows instances of `WorkPackage` when looking for ancestors or descendants. --- .../work_packages/update_ancestors_service.rb | 2 +- .../update_ancestors_service_spec.rb | 41 ++++++++++++++----- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index e70189edeafe..3c7ad47ab8cd 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -153,7 +153,7 @@ def calculate_simple_average_percent_complete(work_package, loader) def children_done_ratio_values(work_package, loader) loader .children_of(work_package) - .reject(&:status_excluded_from_totals) + .filter(&:included_in_totals_calculation?) .map { |child| child.derived_done_ratio || child.done_ratio || 0 } end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index dac34b0aabcc..c0ee1665f191 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -799,7 +799,7 @@ def call_update_ancestors_service(work_package) describe "simple average mode for total % complete calculation", with_settings: { total_percent_complete_mode: "simple_average" } do subject(:call_result) do - described_class.new(user:, work_package: parent) + described_class.new(user:, work_package: initiator_work_package) .call(%i(remaining_hours)) end @@ -811,6 +811,7 @@ def call_update_ancestors_service(work_package) child2 | 5h | | 2.5h | | 50% | child3 | 10h | | 2.5h | | 75% | TABLE + let(:initiator_work_package) { child3 } it "sets the total % complete solely based on % complete values of children and parent" do expect(call_result).to be_success @@ -818,6 +819,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 46% + child3 | TABLE end end @@ -830,6 +832,7 @@ def call_update_ancestors_service(work_package) child2 | 5h | | 2.5h | | 50% | child3 | 10h | | 2.5h | | 75% | TABLE + let(:initiator_work_package) { child3 } it "sets the total % complete solely based on % complete values of children" do expect(call_result).to be_success @@ -837,6 +840,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 53% + child3 | TABLE end end @@ -851,6 +855,7 @@ def call_update_ancestors_service(work_package) child2 | | | | | 75% | child3 | 10h | | 2.5h | | 75% | TABLE + let(:initiator_work_package) { child2 } it "sets the total % complete solely based on % complete values of children" do expect(call_result).to be_success @@ -858,6 +863,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 61% + child2 | TABLE end end @@ -872,6 +878,7 @@ def call_update_ancestors_service(work_package) child2 | | | | | 100% | child3 | | | | | 75% | TABLE + let(:initiator_work_package) { child1 } it "sets the total % complete solely based on % complete values of children" do expect(call_result).to be_success @@ -879,6 +886,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 92% + child1 | TABLE end end @@ -893,6 +901,7 @@ def call_update_ancestors_service(work_package) child2 | | | | | | child3 | | | | | 75% | TABLE + let(:initiator_work_package) { child1 } it "sets the total % complete solely based on % complete values of children " \ "and accounts unset values in children as 0" do @@ -901,6 +910,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 58% + child1 | TABLE end end @@ -915,6 +925,7 @@ def call_update_ancestors_service(work_package) child2 | | | | | | child3 | | | | | 75% | TABLE + let(:initiator_work_package) { child1 } it "sets the total % complete based on % complete values of children and parent " \ "and accounts unset values in children as 0" do @@ -923,6 +934,7 @@ def call_update_ancestors_service(work_package) expect_work_packages(updated_work_packages, <<~TABLE) subject | total % complete parent | 46% + child1 | TABLE end end @@ -937,16 +949,19 @@ def call_update_ancestors_service(work_package) grandchild2 | 3h | | 1h | | 67% | child2 | 5h | 7h | 2.5h | 3.5h | 50% | 50% grandchild3 | 2h | | 1h | | 50% | - child3 | 10h | 14h | 2.5h | 3.5h | 75% | 75% + child3 | 10h | 14h | 2.5h | 3.5h | 75% | 11% grandchild4 | 4h | | 1h | | 75% | TABLE + let(:initiator_work_package) { grandchild4 } it "sets the total % complete solely based on % complete values of direct children" do expect(call_result).to be_success updated_work_packages = call_result.all_results expect_work_packages(updated_work_packages, <<~TABLE) - subject | total % complete - parent | 56% + subject | total % complete + parent | 56% + child3 | 75% + grandchild4 | TABLE end end @@ -961,17 +976,20 @@ def call_update_ancestors_service(work_package) grandchild2 | 3h | | 1h | | 67% | child2 | 5h | 7h | 2.5h | 3.5h | 50% | 75% grandchild3 | 2h | | 1h | | 100% | - child3 | 10h | 14h | 2.5h | 3.5h | 75% | 75% + child3 | 10h | 14h | 2.5h | 3.5h | 75% | 0% grandchild4 | 4h | | 1h | | 75% | child4 | | | | | 60% | TABLE + let(:initiator_work_package) { grandchild4 } it "sets the total % complete solely based on % complete values of direct children" do expect(call_result).to be_success updated_work_packages = call_result.all_results expect_work_packages(updated_work_packages, <<~TABLE) - subject | total % complete - parent | 53% + subject | total % complete + parent | 53% + child3 | 75% + grandchild4 | TABLE end end @@ -988,16 +1006,19 @@ def call_update_ancestors_service(work_package) child2 | Rejected | 5h | 7h | 2.5h | 3.5h | 50% | 75% grandchild3 | Open | 2h | | 1h | | 100% | child3 | Open | 10h | 14h | 2.5h | 3.5h | 75% | - grandchild4 | Rejected | 4h | | 1h | | 75% | + grandchild4 | Rejected | 4h | | 1h | | 100% | child4 | Open | | | | | 60% | TABLE + let(:initiator_work_package) { grandchild4 } it "sets the total % complete solely based on % complete values of direct children" do expect(call_result).to be_success updated_work_packages = call_result.all_results expect_work_packages(updated_work_packages, <<~TABLE) - subject | total % complete - parent | 47% + subject | total % complete + parent | 47% + child3 | 75% + grandchild4 | TABLE end end From f0408482463f039c9df0d292aedd51f405cb5942 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 12:28:28 +0200 Subject: [PATCH 12/22] Fix when all work packages of a hierarchy are excluded from totals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this case, there is no calculated simple average: the ∑ % Complete should be cleared. --- .../work_packages/update_ancestors_service.rb | 2 ++ .../update_ancestors_service_spec.rb | 23 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 3c7ad47ab8cd..877833bd8582 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -146,6 +146,8 @@ def calculate_simple_average_percent_complete(work_package, loader) all_done_ratios << work_package.done_ratio end + return if all_done_ratios.empty? + progress = all_done_ratios.sum.to_f / all_done_ratios.count progress.round end diff --git a/spec/services/work_packages/update_ancestors_service_spec.rb b/spec/services/work_packages/update_ancestors_service_spec.rb index c0ee1665f191..12ff0bc8f5ee 100644 --- a/spec/services/work_packages/update_ancestors_service_spec.rb +++ b/spec/services/work_packages/update_ancestors_service_spec.rb @@ -1022,6 +1022,29 @@ def call_update_ancestors_service(work_package) TABLE end end + + context "with all hierarchy being excluded from totals (parent excluded and child updated to be excluded)" do + let_work_packages(<<~TABLE) + hierarchy | status | % complete | ∑ % complete + parent | Rejected | 10% | 10% + child | Open | 33% | 43% + TABLE + let(:initiator_work_package) { child } + + before do + set_attributes_on(child, status: rejected_status) + end + + it "clears the total % complete of the parent" do + expect(call_result).to be_success + updated_work_packages = call_result.all_results + expect_work_packages(updated_work_packages, <<~TABLE) + subject | total % complete + parent | + child | + TABLE + end + end end describe "ignore_non_working_days propagation" do From 6d9a9c80ddb748f09b1668d37e4c53b21994bd91 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 12:30:33 +0200 Subject: [PATCH 13/22] Removed flaky spec I'm not sure why it fails now while it did not before. Maybe it's when switching to cuprite. Anyway this test is not worth enough to bother fixing it. --- spec/features/admin/progress_tracking_spec.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/features/admin/progress_tracking_spec.rb b/spec/features/admin/progress_tracking_spec.rb index 480014d6e18c..4a4867a7ed9a 100644 --- a/spec/features/admin/progress_tracking_spec.rb +++ b/spec/features/admin/progress_tracking_spec.rb @@ -91,19 +91,4 @@ expect(page).to have_field("No change", disabled: true) expect(page).to have_field("Automatically set to 100%", disabled: true) end - - it "does not keep radio button state when navigating to another page and back" do - Setting.work_package_done_ratio = "field" - visit admin_settings_progress_tracking_path - - find(:radio_button, "Status-based").click - - # navigate to another page, then back - click_on "General" - wait_for { page.current_path }.to include(admin_settings_work_packages_general_path) - page.go_back - - # browser should not keep the radio button state (autocomplete="off") - expect(page).to have_field("Work-based", checked: true) - end end From 447a6deafec900b034370d6dd665a31ebbd711c2 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 14:00:04 +0200 Subject: [PATCH 14/22] Fix rubocop warnings Metrics/AbcSize and Metrics/PerceivedComplexity --- app/services/settings/update_service.rb | 30 ++++++++++++++++--------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/app/services/settings/update_service.rb b/app/services/settings/update_service.rb index 0744628f29fa..591114db7138 100644 --- a/app/services/settings/update_service.rb +++ b/app/services/settings/update_service.rb @@ -45,19 +45,29 @@ def set_setting_value(name, value) old_value = Setting[name] new_value = derive_value(value) Setting[name] = new_value - if name == :work_package_done_ratio && old_value != "status" && new_value == "status" - WorkPackages::Progress::ApplyStatusesChangeJob.perform_later(cause_type: "progress_mode_changed_to_status_based") - elsif name == :total_percent_complete_mode && old_value != "work_weighted_average" && new_value == "work_weighted_average" - WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob - .perform_later(mode: new_value, - cause_type: "total_percent_complete_mode_changed_to_work_weighted_average") - elsif name == :total_percent_complete_mode && old_value != "simple_average" && new_value == "simple_average" - WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob - .perform_later(mode: new_value, - cause_type: "total_percent_complete_mode_changed_to_simple_average") + + if name == :work_package_done_ratio + trigger_update_job_for_progress_mode_change(old_value, new_value) + elsif name == :total_percent_complete_mode + trigger_update_job_for_total_percent_complete_mode_change(old_value, new_value) end end + def trigger_update_job_for_progress_mode_change(old_value, new_value) + return if old_value == new_value + return if new_value != "status" # only trigger if changing to status-based + + WorkPackages::Progress::ApplyStatusesChangeJob.perform_later(cause_type: "progress_mode_changed_to_status_based") + end + + def trigger_update_job_for_total_percent_complete_mode_change(old_value, new_value) + return if old_value == new_value + + WorkPackages::Progress::ApplyTotalPercentCompleteModeChangeJob + .perform_later(mode: new_value, + cause_type: "total_percent_complete_mode_changed_to_#{new_value}") + end + def derive_value(value) case value when Array, Hash From 0f9be1ba41d6bd49ef67d4a569a132bf18f7931c Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 14:46:40 +0200 Subject: [PATCH 15/22] Add a test when all hierarchy is excluded from totals Just to be extra-sure this case is handled correctly in jobs as well. --- .../apply_statuses_change_job_spec.rb | 15 ++++++++ ...l_percent_complete_mode_change_job_spec.rb | 36 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb index dbcf01d1403e..729b347784fe 100644 --- a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb @@ -132,6 +132,21 @@ def expect_performing_job_changes(from:, to:, ) end + it "removes all totals when all work packages of the hierarchy are excluded" do + expect_performing_job_changes( + from: <<~TABLE, + hierarchy | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + parent | Excluded | 10h | 3h | 70% | 20h | 5h | 75% + child | Excluded | 10h | 2h | 50% | | | + TABLE + to: <<~TABLE + subject | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + parent | Excluded | 10h | 3h | 70% | | | + child | Excluded | 10h | 2h | 50% | | | + TABLE + ) + end + it "keeps the totals unset if work, remaining work, and % complete are all nil" do expect_performing_job_changes( from: <<~TABLE, diff --git a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb index 5e393754f545..63527a3a1a23 100644 --- a/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_total_percent_complete_mode_change_job_spec.rb @@ -212,6 +212,24 @@ def expect_performing_job_changes(from:, to:, end end + context "when all work packages of the hierarchy are excluded" do + it "removes all totals" do + expect_performing_job_changes( + mode: "work_weighted_average", + from: <<~TABLE, + hierarchy | status | work | remaining work | % complete | ∑ % complete + parent | Excluded | 40h | 12h | 70% | 72% + child | Excluded | 10h | 2h | 80% | + TABLE + to: <<~TABLE + subject | status | work | remaining work | % complete | ∑ % complete + parent | Excluded | 40h | 12h | 70% | + child | Excluded | 10h | 2h | 80% | + TABLE + ) + end + end + describe "journal entries" do # rubocop:disable RSpec/ExampleLength it "creates journal entries for the modified work packages" do @@ -396,6 +414,24 @@ def expect_performing_job_changes(from:, to:, end end + context "when all work packages of the hierarchy are excluded" do + it "removes all totals" do + expect_performing_job_changes( + mode: "simple_average", + from: <<~TABLE, + hierarchy | status | % complete | ∑ % complete + parent | Excluded | 0% | 0% + child | Excluded | 0% | + TABLE + to: <<~TABLE + subject | status | % complete | ∑ % complete + parent | Excluded | 0% | + child | Excluded | 0% | + TABLE + ) + end + end + describe "journal entries" do # rubocop:disable RSpec/ExampleLength it "creates journal entries for the modified work packages" do From a574b2f7f6bdf281c2d8626f986abccb1d58e024 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 15:06:33 +0200 Subject: [PATCH 16/22] Add failing spec about handling simple average mode in a job --- .../apply_statuses_change_job_spec.rb | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb index 729b347784fe..de211674a153 100644 --- a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb @@ -284,7 +284,8 @@ def expect_performing_job_changes(from:, to:, end end - context "when in hierarchy" do + context "when in hierarchy in work-weighted average mode", + with_settings: { total_percent_complete_mode: "work_weighted_average" } do it "the total remaining work and total % complete values are recomputed" do # Simulating changing "Doing" status default % progress from 20% to 40% expect_performing_job_changes( @@ -308,6 +309,33 @@ def expect_performing_job_changes(from:, to:, end end + context "when in hierarchy in simple average mode", + with_settings: { total_percent_complete_mode: "simple_average" } do + it "the total remaining work and total % complete values are recomputed" do + # Simulating changing "Doing" status default % progress from 20% to 40% + expect_performing_job_changes( + from: <<~TABLE, + hierarchy | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + grandparent | Doing (40%) | 1h | 0.8h | 20% | 20h | 9.8h | 51% + parent | Doing (40%) | | | 20% | 19h | 9h | 53% + child 1 | Done (100%) | 9h | 0h | 100% | | | + child 2 | Doing (40%) | 5h | 4h | 20% | | | + child 3 | To do (0%) | 5h | 5h | 0% | | | + excluded | Excluded | 5h | 5h | 0% | | | + TABLE + to: <<~TABLE + subject | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + grandparent | Doing (40%) | 1h | 0.6h | 40% | 20h | 8.6h | 38% + parent | Doing (40%) | | | 40% | 19h | 8h | 35% + child 1 | Done (100%) | 9h | 0h | 100% | | | + child 2 | Doing (40%) | 5h | 3h | 40% | | | + child 3 | To do (0%) | 5h | 5h | 0% | | | + excluded | Excluded | 5h | 5h | 0% | | | + TABLE + ) + end + end + context "when a status is being excluded from progress calculation" do # The work packages are created like if the status is not excluded yet shared_let_work_packages(<<~TABLE) From 3f97171b796f289414e6fa9579fe78e1ce3c9a9a Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 17:12:34 +0200 Subject: [PATCH 17/22] Use work_package_hierarchies to create depth table Simpler. --- .../work_packages/progress/sql_commands.rb | 39 ++----------------- 1 file changed, 4 insertions(+), 35 deletions(-) diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index 4f2c77771202..8e1d2d8a3378 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -108,42 +108,11 @@ def create_temporary_total_percent_complete_table_for_simple_average_mode def create_temporary_depth_table execute(<<~SQL.squish) CREATE UNLOGGED TABLE temp_work_package_depth AS - WITH RECURSIVE - work_package_depth AS ( - /* Base case: Leaves (work packages with no children) */ - SELECT - wp.id, - wp.parent_id, - 0 AS depth - FROM - temp_wp_progress_values wp - WHERE - NOT EXISTS ( - SELECT - 1 - FROM - temp_wp_progress_values c - WHERE - c.parent_id = wp.id - ) - UNION ALL - /* Recursive case: Parents */ - SELECT - wp.parent_id AS id, - wp2.parent_id, - wpd.depth + 1 AS depth - FROM - work_packages wp - INNER JOIN work_package_depth wpd ON wp.id = wpd.id - INNER JOIN temp_wp_progress_values wp2 ON wp.parent_id = wp2.id - WHERE - wp.parent_id IS NOT NULL - ) SELECT - id, - depth - FROM - work_package_depth + ancestor_id as id, + max(generations) as depth + FROM work_package_hierarchies + GROUP BY ancestor_id SQL end From 3250f5104aac2910fc433954f0f4698915269926 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 25 Sep 2024 17:13:35 +0200 Subject: [PATCH 18/22] Compute total % complete according to mode in update status job Reuse and duplicate a lot of logic from the other job. It needs to be reworked, but it works --- .../work_packages/progress/sql_commands.rb | 120 ++++++++++++++++-- .../apply_statuses_change_job_spec.rb | 4 +- 2 files changed, 111 insertions(+), 13 deletions(-) diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index 8e1d2d8a3378..4362d9905990 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -32,8 +32,10 @@ module WorkPackages::Progress::SqlCommands def with_temporary_progress_table WorkPackage.transaction do create_temporary_progress_table + create_temporary_depth_table yield ensure + drop_temporary_depth_table drop_temporary_progress_table end end @@ -42,15 +44,18 @@ def create_temporary_progress_table execute(<<~SQL.squish) CREATE UNLOGGED TABLE temp_wp_progress_values AS SELECT - id, + work_packages.id, + parent_id as parent_id, status_id, estimated_hours, remaining_hours, done_ratio, + statuses.excluded_from_totals AS status_excluded_from_totals, NULL::double precision AS total_work, NULL::double precision AS total_remaining_work, NULL::integer AS total_p_complete FROM work_packages + LEFT JOIN statuses ON work_packages.status_id = statuses.id SQL end @@ -95,11 +100,11 @@ def create_temporary_total_percent_complete_table_for_simple_average_mode execute(<<~SQL.squish) CREATE UNLOGGED TABLE temp_wp_progress_values AS SELECT - work_packages.id as id, - work_packages.parent_id as parent_id, - statuses.excluded_from_totals AS status_excluded_from_totals, - done_ratio AS p_complete, - NULL::INTEGER AS total_p_complete + work_packages.id as id, + work_packages.parent_id as parent_id, + statuses.excluded_from_totals AS status_excluded_from_totals, + done_ratio AS p_complete, + NULL::INTEGER AS total_p_complete FROM work_packages LEFT JOIN statuses ON work_packages.status_id = statuses.id SQL @@ -179,14 +184,20 @@ def derive_unset_work_from_remaining_work_and_percent_complete # Computes total work, total remaining work and total % complete for all work # packages having children. def update_totals + update_work_and_remaining_work_totals + if Setting.total_percent_complete_mode == "work_weighted_average" + update_total_percent_complete_in_work_weighted_average_mode + elsif Setting.total_percent_complete_mode == "simple_average" + update_total_percent_complete_in_work_weighted_average_mode + update_total_percent_complete_in_simple_average_mode + end + end + + def update_work_and_remaining_work_totals execute(<<~SQL.squish) UPDATE temp_wp_progress_values SET total_work = totals.total_work, - total_remaining_work = totals.total_remaining_work, - total_p_complete = CASE - WHEN totals.total_work = 0 THEN NULL - ELSE (1 - (totals.total_remaining_work / totals.total_work)) * 100 - END + total_remaining_work = totals.total_remaining_work FROM ( SELECT wp_tree.ancestor_id AS id, SUM(estimated_hours) AS total_work, @@ -207,6 +218,93 @@ def update_totals SQL end + def update_total_percent_complete_in_work_weighted_average_mode + execute(<<~SQL.squish) + UPDATE temp_wp_progress_values + SET total_p_complete = CASE + WHEN total_work = 0 THEN NULL + ELSE (1 - (total_remaining_work / total_work)) * 100 + END + WHERE id IN ( + SELECT ancestor_id AS id + FROM work_package_hierarchies + GROUP BY id + HAVING MAX(generations) > 0 + ) + SQL + end + + def update_total_percent_complete_in_simple_average_mode + execute(<<~SQL.squish) + DO $$ + DECLARE + min_depth INTEGER := 0; + max_depth INTEGER := (SELECT MAX(depth) FROM temp_work_package_depth); + current_depth INTEGER := min_depth; + BEGIN + /* Navigate work packages and perform updates bottom-up */ + while current_depth <= max_depth loop + UPDATE temp_wp_progress_values wp + SET + total_p_complete = CASE + WHEN current_depth = min_depth THEN NULL + ELSE ROUND( + ( + /* Exclude the current work package if it has a status excluded from totals */ + CASE WHEN wp.status_excluded_from_totals + THEN 0 + /* Otherwise, use the current work package's % complete value or 0 if unset */ + ELSE COALESCE(wp.done_ratio, 0) + END + ( + SELECT + SUM( + COALESCE(child_wp.total_p_complete, child_wp.done_ratio, 0) + ) + FROM + temp_wp_progress_values child_wp + WHERE + child_wp.parent_id = wp.id + /* Exclude children with a status excluded from totals */ + AND NOT child_wp.status_excluded_from_totals + ) + ) / ( + /* Exclude the current work package if it has a status excluded from totals */ + CASE WHEN wp.status_excluded_from_totals + THEN 0 + /* Otherwise, count the current work package if it has a % complete value set */ + ELSE(CASE WHEN wp.done_ratio IS NOT NULL THEN 1 ELSE 0 END) + END + ( + SELECT + COUNT(1) + FROM + temp_wp_progress_values child_wp + WHERE + child_wp.parent_id = wp.id + /* Exclude children with a status excluded from totals */ + AND NOT child_wp.status_excluded_from_totals + ) + ) + ) + END + /* Select only work packages at the curren depth */ + WHERE + wp.id IN ( + SELECT + id + FROM + temp_work_package_depth + WHERE + depth = current_depth + ); + + /* Go up a level from a child to a parent*/ + current_depth := current_depth + 1; + + END loop; + END $$; + SQL + end + def copy_progress_values_to_work_packages_and_update_journals(cause) updated_work_package_ids = copy_progress_values_to_work_packages create_journals_for_updated_work_packages(updated_work_package_ids, cause:) diff --git a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb index de211674a153..7d34592d21ab 100644 --- a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb @@ -325,8 +325,8 @@ def expect_performing_job_changes(from:, to:, TABLE to: <<~TABLE subject | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete - grandparent | Doing (40%) | 1h | 0.6h | 40% | 20h | 8.6h | 38% - parent | Doing (40%) | | | 40% | 19h | 8h | 35% + grandparent | Doing (40%) | 1h | 0.6h | 40% | 20h | 8.6h | 42% + parent | Doing (40%) | | | 40% | 19h | 8h | 45% child 1 | Done (100%) | 9h | 0h | 100% | | | child 2 | Doing (40%) | 5h | 3h | 40% | | | child 3 | To do (0%) | 5h | 5h | 0% | | | From 0edb100994ad5f7d3bdc9652e38ebbeda8cc5095 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Wed, 25 Sep 2024 13:43:35 -0500 Subject: [PATCH 19/22] Add extra spec specifically accounting for excluded statuses --- .../apply_statuses_change_job_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb index 2dc221aad5f7..048a5ead0053 100644 --- a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb @@ -333,6 +333,27 @@ def expect_performing_job_changes(from:, to:, TABLE ) end + + context "when statuses are being excluded from progress calculation" do + it "recomputes totals without the values from work packages having the excluded status" do + expect_performing_job_changes( + from: <<~TABLE, + hierarchy | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + parent | Excluded | 4h | 4h | 0% | 90h | 30h | 67% + child 1 | Excluded | 9h | 9h | 0% | | | + child 2 | Doing (40%) | 5h | 3h | 40% | | | + child 3 | Done (100%) | 5h | 0h | 100% | | | + TABLE + to: <<~TABLE + subject | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + parent | Excluded | 4h | 4h | 0% | 10h | 3h | 70% + child 1 | Excluded | 9h | 9h | 0% | | | + child 2 | Doing (40%) | 5h | 3h | 40% | | | + child 3 | Done (100%) | 5h | 0h | 100% | | | + TABLE + ) + end + end end context "when a status is being excluded from progress calculation" do From 5b1dbb45c8a1c0046ecf6eef54e6b9a842389e13 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Wed, 25 Sep 2024 13:48:58 -0500 Subject: [PATCH 20/22] Test for unset total values --- .../progress/apply_statuses_change_job_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb index 048a5ead0053..a626c2d08e50 100644 --- a/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb +++ b/spec/workers/work_packages/progress/apply_statuses_change_job_spec.rb @@ -306,6 +306,23 @@ def expect_performing_job_changes(from:, to:, TABLE ) end + + context "when total work and remaining work values are null" do + it "recomputes the total % complete to null" do + expect_performing_job_changes( + from: <<~TABLE, + hierarchy | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + grandparent | Doing (40%) | | | 40% | | | 40% + parent | Doing (40%) | | | 40% | | | 40% + TABLE + to: <<~TABLE + hierarchy | status | work | remaining work | % complete | ∑ work | ∑ remaining work | ∑ % complete + grandparent | Doing (40%) | | | 40% | | | + parent | Doing (40%) | | | 40% | | | + TABLE + ) + end + end end context "when in hierarchy in simple average mode", From 95cda6de29d03ce01e16e5200e90c519fc821a16 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Wed, 25 Sep 2024 13:49:09 -0500 Subject: [PATCH 21/22] Remove unnecessary update --- app/workers/work_packages/progress/sql_commands.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index 4362d9905990..29f1a39948ae 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -188,7 +188,6 @@ def update_totals if Setting.total_percent_complete_mode == "work_weighted_average" update_total_percent_complete_in_work_weighted_average_mode elsif Setting.total_percent_complete_mode == "simple_average" - update_total_percent_complete_in_work_weighted_average_mode update_total_percent_complete_in_simple_average_mode end end From de862213616c184d774dde2cf6a1b3f8b247f44a Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Wed, 25 Sep 2024 13:54:31 -0500 Subject: [PATCH 22/22] Extract common operations to sql_commands --- ..._total_percent_complete_mode_change_job.rb | 94 +------------------ .../work_packages/progress/sql_commands.rb | 11 ++- 2 files changed, 9 insertions(+), 96 deletions(-) diff --git a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb index a2c614497d6c..d14fd19fcea7 100644 --- a/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb +++ b/app/workers/work_packages/progress/apply_total_percent_complete_mode_change_job.rb @@ -63,104 +63,14 @@ def perform(cause_type:, mode:) def update_total_percent_complete case mode when "work_weighted_average" - update_to_work_weighted_average + update_total_percent_complete_in_work_weighted_average_mode when "simple_average" - update_to_simple_average + update_total_percent_complete_in_simple_average_mode else raise ArgumentError, "Invalid total percent complete mode: #{mode}" end end - def update_to_work_weighted_average - execute(<<~SQL.squish) - UPDATE temp_wp_progress_values - SET total_p_complete = CASE - WHEN total_work IS NULL OR total_remaining_work IS NULL THEN NULL - WHEN total_work = 0 THEN NULL - ELSE ROUND( - ((total_work - total_remaining_work)::float / total_work) * 100 - ) - END - WHERE id IN ( - SELECT ancestor_id - FROM work_package_hierarchies - GROUP BY ancestor_id - HAVING MAX(generations) > 0 - ) - SQL - end - - def update_to_simple_average - execute(<<~SQL.squish) - DO $$ - DECLARE - min_depth INTEGER := 0; - max_depth INTEGER := (SELECT MAX(depth) FROM temp_work_package_depth); - current_depth INTEGER := min_depth; - BEGIN - /* Navigate work packages and perform updates bottom-up */ - while current_depth <= max_depth loop - UPDATE temp_wp_progress_values wp - SET - total_p_complete = CASE - WHEN current_depth = min_depth THEN NULL - ELSE ROUND( - ( - /* Exclude the current work package if it has a status excluded from totals */ - CASE WHEN wp.status_excluded_from_totals - THEN 0 - /* Otherwise, use the current work package's % complete value or 0 if unset */ - ELSE COALESCE(wp.p_complete, 0) - END + ( - SELECT - SUM( - COALESCE(child_wp.total_p_complete, child_wp.p_complete, 0) - ) - FROM - temp_wp_progress_values child_wp - WHERE - child_wp.parent_id = wp.id - /* Exclude children with a status excluded from totals */ - AND NOT child_wp.status_excluded_from_totals - ) - ) / ( - /* Exclude the current work package if it has a status excluded from totals */ - CASE WHEN wp.status_excluded_from_totals - THEN 0 - /* Otherwise, count the current work package if it has a % complete value set */ - ELSE(CASE WHEN wp.p_complete IS NOT NULL THEN 1 ELSE 0 END) - END + ( - SELECT - COUNT(1) - FROM - temp_wp_progress_values child_wp - WHERE - child_wp.parent_id = wp.id - /* Exclude children with a status excluded from totals */ - AND NOT child_wp.status_excluded_from_totals - ) - ) - ) - END - /* Select only work packages at the curren depth */ - WHERE - wp.id IN ( - SELECT - id - FROM - temp_work_package_depth - WHERE - depth = current_depth - ); - - /* Go up a level from a child to a parent*/ - current_depth := current_depth + 1; - - END loop; - END $$; - SQL - end - def journal_cause assert_valid_cause_type! diff --git a/app/workers/work_packages/progress/sql_commands.rb b/app/workers/work_packages/progress/sql_commands.rb index 29f1a39948ae..6d367d0f7f95 100644 --- a/app/workers/work_packages/progress/sql_commands.rb +++ b/app/workers/work_packages/progress/sql_commands.rb @@ -103,7 +103,7 @@ def create_temporary_total_percent_complete_table_for_simple_average_mode work_packages.id as id, work_packages.parent_id as parent_id, statuses.excluded_from_totals AS status_excluded_from_totals, - done_ratio AS p_complete, + done_ratio, NULL::INTEGER AS total_p_complete FROM work_packages LEFT JOIN statuses ON work_packages.status_id = statuses.id @@ -221,13 +221,16 @@ def update_total_percent_complete_in_work_weighted_average_mode execute(<<~SQL.squish) UPDATE temp_wp_progress_values SET total_p_complete = CASE + WHEN total_work IS NULL OR total_remaining_work IS NULL THEN NULL WHEN total_work = 0 THEN NULL - ELSE (1 - (total_remaining_work / total_work)) * 100 + ELSE ROUND( + ((total_work - total_remaining_work)::float / total_work) * 100 + ) END WHERE id IN ( - SELECT ancestor_id AS id + SELECT ancestor_id FROM work_package_hierarchies - GROUP BY id + GROUP BY ancestor_id HAVING MAX(generations) > 0 ) SQL