From 2227ec99c98fbe367c9a9401bad221ec50a27768 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 27 Aug 2024 10:20:04 +0200 Subject: [PATCH 1/3] [57258] Indicate progress value derivation reason It gives a hint to the user about which fields had their values automatically calculated and how this calculation has been done. --- .../work_based/modal_body_component.html.erb | 12 +- app/forms/work_packages/progress_form.rb | 8 + app/models/work_package.rb | 8 + .../derive_progress_values_base.rb | 6 +- .../derive_progress_values_status_based.rb | 10 +- .../derive_progress_values_work_based.rb | 63 +++- config/locales/en.yml | 16 + .../work_based/modal_body_component_spec.rb | 1 - ...erive_progress_values_status_based_spec.rb | 86 ++--- .../derive_progress_values_work_based_spec.rb | 351 ++++++++++++------ .../set_attributes_service/shared_examples.rb | 91 +++++ 11 files changed, 480 insertions(+), 172 deletions(-) create mode 100644 spec/services/work_packages/set_attributes_service/shared_examples.rb diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb index 3451bc3d3b71..d29236da0e9e 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb @@ -38,15 +38,13 @@ <% end %> <% end %> - <% modal_body.with_row(mt: 3) do |_tooltip| %> - <%= render(Primer::Beta::Text.new(font_weight: :semibold)) { t("work_package.progress.label_note") } %> - <% if OpenProject::FeatureDecisions.percent_complete_edition_active? %> - <%= render(Primer::Beta::Text.new) { t("work_package.progress.modal.work_based_help_text") } %> - <% else %> - <%# This condition branch to be removed in 15.0 with :percent_complete_edition feature flag removal %> + <%# This condition branch to be removed in 15.0 with :percent_complete_edition feature flag removal %> + <% unless OpenProject::FeatureDecisions.percent_complete_edition_active? %> + <% modal_body.with_row(mt: 3) do |_tooltip| %> + <%= render(Primer::Beta::Text.new(font_weight: :semibold)) { t("work_package.progress.label_note") } %> <%= render(Primer::Beta::Text.new) { t("work_package.progress.modal.work_based_help_text_pre_14_4_without_percent_complete_edition") } %> + <%= render(Primer::Beta::Link.new(href: learn_more_href)) { t(:label_learn_more) } %> <% end %> - <%= render(Primer::Beta::Link.new(href: learn_more_href)) { t(:label_learn_more) } %> <% end %> <% modal_body.with_row(mt: 3) do |_actions_row| %> diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb index a2e32c8a0004..61eecfa284b5 100644 --- a/app/forms/work_packages/progress_form.rb +++ b/app/forms/work_packages/progress_form.rb @@ -136,6 +136,7 @@ def text_field(group, name:, value: field_value(name), label:, + caption: field_hint_message(name), validation_message: validation_message(name) ) @@ -180,6 +181,13 @@ def field_value(name) end end + def field_hint_message(field_name) + hint = work_package.derived_progress_hints[field_name] + return if hint.nil? + + I18n.t("work_package.progress.derivation_hints.#{field_name}.#{hint}") + end + def validation_message(name) # it's ok to take the first error only, that's how primer_view_component does it anyway. message = @work_package.errors.messages_for(name).first diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 8ab973b435e6..de48fc2e390a 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -332,6 +332,14 @@ def done_ratio=(value) write_attribute :done_ratio, convert_value_to_percentage(value) end + def derived_progress_hints=(hints) + @derived_progress_hints = hints + end + + def derived_progress_hints + @derived_progress_hints ||= {} + end + def duration_in_hours duration ? duration * 24 : nil end diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb index 229d7ea05188..87f5d610eda9 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb @@ -141,6 +141,10 @@ def percent_complete_not_provided_by_user? private + def set_hint(field, hint) + work_package.derived_progress_hints[field] = hint + end + def round_progress_values # The values are set only when rounding returns a different value. Doing # otherwise would modify the values returned by `xxx_before_type_cast` and @@ -157,8 +161,6 @@ def round_progress_values end def remaining_work_from_percent_complete_and_work - return nil if work_empty? || percent_complete_empty? - completed_work = work * percent_complete / 100.0 remaining_work = (work - completed_work).round(2) remaining_work.clamp(0.0, work) diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb index 5fdc67429e48..e2a3564739fe 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb @@ -51,7 +51,15 @@ def update_remaining_work_from_percent_complete return if remaining_work_came_from_user? return if work&.negative? - self.remaining_work = remaining_work_from_percent_complete_and_work + if work_empty? + return unless work_changed? + + set_hint(:remaining_hours, :cleared_because_work_is_empty) + self.remaining_work = nil + else + set_hint(:remaining_hours, :derived) + self.remaining_work = remaining_work_from_percent_complete_and_work + end end end end diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb index 76ef3cf9a185..9921ee1cb243 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb @@ -30,6 +30,8 @@ class WorkPackages::SetAttributesService class DeriveProgressValuesWorkBased < DeriveProgressValuesBase private + attr_accessor :skip_percent_complete_derivation + def derive_progress_attributes raise ArgumentError, "Cannot use #{self.class.name} in status-based mode" if WorkPackage.status_based_mode? @@ -63,36 +65,58 @@ def derive_remaining_work? end def derive_percent_complete? - percent_complete_not_provided_by_user? && (work_changed? || remaining_work_changed?) + percent_complete_not_provided_by_user? && (work_changed? || remaining_work_changed?) \ + && !skip_percent_complete_derivation end + # rubocop:disable Metrics/AbcSize,Metrics/PerceivedComplexity def update_work return if remaining_work_empty? && percent_complete_empty? return if percent_complete == 100 # would be Infinity if computed when % complete is 100% return unless work_can_be_derived? - self.work = - if remaining_work_empty? - nil - else - work_from_percent_complete_and_remaining_work - end + if remaining_work_empty? + return unless remaining_work_changed? + + set_hint(:estimated_hours, :cleared_because_remaining_work_is_empty) + self.work = nil + elsif percent_complete_empty? + set_hint(:estimated_hours, :same_as_remaining_work) + self.work = remaining_work + else + set_hint(:estimated_hours, :derived) + self.work = work_from_percent_complete_and_remaining_work + skip_percent_complete_derivation! + end end - # rubocop:disable Metrics/AbcSize,Metrics/PerceivedComplexity def update_remaining_work return if work_empty? && percent_complete_empty? - return if work_was_empty? && remaining_work_set? # remaining work is kept and % complete will be set + return if work_was_empty? && remaining_work_set? # remaining work is kept and % complete will be unset if work_set? && remaining_work_empty? && percent_complete_empty? + set_hint(:remaining_hours, :same_as_work) self.remaining_work = work elsif work_changed? && work_set? && remaining_work_set? && percent_complete_not_provided_by_user? delta = work - work_was + if delta.positive? + set_hint(:remaining_hours, :increased_like_work) + elsif delta.negative? + set_hint(:remaining_hours, :decreased_like_work) + end self.remaining_work = (remaining_work + delta).clamp(0.0, work) - elsif work_empty? || percent_complete_empty? + elsif work_empty? + return unless work_changed? + + set_hint(:remaining_hours, :cleared_because_work_is_empty) + self.remaining_work = nil + elsif percent_complete_empty? + set_hint(:remaining_hours, :cleared_because_percent_complete_is_empty) self.remaining_work = nil else + set_hint(:remaining_hours, :derived) self.remaining_work = remaining_work_from_percent_complete_and_work + skip_percent_complete_derivation! end end # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity @@ -100,12 +124,23 @@ def update_remaining_work def update_percent_complete return if work_empty? - self.percent_complete = percent_complete_from_work_and_remaining_work + if work.zero? + set_hint(:done_ratio, :cleared_because_work_is_0h) + self.percent_complete = nil + elsif remaining_work_empty? + set_hint(:done_ratio, :cleared_because_remaining_work_is_empty) + self.percent_complete = nil + else + set_hint(:done_ratio, :derived) + self.percent_complete = percent_complete_from_work_and_remaining_work + end end - def percent_complete_from_work_and_remaining_work - return nil if work.zero? || remaining_work_empty? + def skip_percent_complete_derivation! + self.skip_percent_complete_derivation = true + end + def percent_complete_from_work_and_remaining_work completed_work = work - remaining_work completion_ratio = completed_work.to_f / work @@ -113,7 +148,7 @@ def percent_complete_from_work_and_remaining_work end def work_from_percent_complete_and_remaining_work - remaining_percent_complete = 1.0 - ((percent_complete || 0) / 100.0) + remaining_percent_complete = 1.0 - (percent_complete / 100.0) remaining_work / remaining_percent_complete end diff --git a/config/locales/en.yml b/config/locales/en.yml index 46384005d4a4..31a0dfe4b7ea 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3696,6 +3696,22 @@ en: work_based_help_text_pre_14_4_without_percent_complete_edition: "% Complete is automatically derived from Work and Remaining work." status_based_help_text: "% Complete is set by work package status." migration_warning_text: "In work-based progress calculation mode, % Complete cannot be manually set and is tied to Work. The existing value has been kept but cannot be edited. Please input Work first." + derivation_hints: + done_ratio: + cleared_because_remaining_work_is_empty: "Cleared because Remaining work is empty." + cleared_because_work_is_0h: "Cleared because Work is 0h." + derived: "Derived from Work and Remaining work." + estimated_hours: + cleared_because_remaining_work_is_empty: "Cleared because Remaining work is empty." + derived: "Derived from Remaining work and % Complete." + same_as_remaining_work: "Set to same value as Remaining work." + remaining_hours: + cleared_because_work_is_empty: "Cleared because Work is empty." + cleared_because_percent_complete_is_empty: "Cleared because % Complete is empty." + decreased_like_work: "Decreased by the same amount as Work." + derived: "Derived from Work and % Complete." + increased_like_work: "Increased by the same amount as Work." + same_as_work: "Set to same value as Work." permissions: comment: "Comment" comment_description: "Can view and comment this work package." diff --git a/spec/components/work_packages/progress/work_based/modal_body_component_spec.rb b/spec/components/work_packages/progress/work_based/modal_body_component_spec.rb index 0fc2617472f6..5ac7c72b7731 100644 --- a/spec/components/work_packages/progress/work_based/modal_body_component_spec.rb +++ b/spec/components/work_packages/progress/work_based/modal_body_component_spec.rb @@ -36,7 +36,6 @@ include_examples "progress modal validations" include_examples "progress modal submit path" - include_examples "progress modal help links" describe "#mode" do subject(:component) { described_class.new(WorkPackage.new) } diff --git a/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb b/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb index 50ba599878bb..c59d0757c41b 100644 --- a/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb +++ b/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb @@ -27,6 +27,7 @@ #++ require "spec_helper" +require_relative "shared_examples" # Scenarios specified in https://community.openproject.org/wp/40749 RSpec.describe WorkPackages::SetAttributesService::DeriveProgressValuesStatusBased, @@ -41,39 +42,6 @@ let(:work_package) { build_stubbed(:work_package, project:, status: status_0_pct_complete) } let(:instance) { described_class.new(work_package) } - shared_examples_for "update progress values" do |description:| - subject do - allow(work_package) - .to receive(:save) - - instance.call - end - - it description do - work_package.attributes = set_attributes - all_expected_attributes = {} - all_expected_attributes.merge!(expected_derived_attributes) if defined?(expected_derived_attributes) - if defined?(expected_kept_attributes) - kept = work_package.attributes.slice(*expected_kept_attributes) - if kept.size != expected_kept_attributes.size - raise ArgumentError, "expected_kept_attributes contains attributes that are not present in the work_package: " \ - "#{expected_kept_attributes - kept.keys} not present in #{work_package.attributes}" - end - all_expected_attributes.merge!(kept) - end - next if all_expected_attributes.blank? - - subject - - aggregate_failures do - expect(work_package).to have_attributes(all_expected_attributes) - # work package is not saved and no errors are created - expect(work_package).not_to have_received(:save) - expect(work_package.errors).to be_empty - end - end - end - context "given a work package with work, remaining work, and status with % complete being set" do before do work_package.status = status_50_pct_complete @@ -87,14 +55,20 @@ let(:set_attributes) { { estimated_hours: nil } } let(:expected_derived_attributes) { { remaining_hours: nil } } - include_examples "update progress values", description: "clearss remaining work" + include_examples "update progress values", description: "clears remaining work", + expected_hints: { + remaining_work: :cleared_because_work_is_empty + } end context "when work is changed" do let(:set_attributes) { { estimated_hours: 5.0 } } let(:expected_derived_attributes) { { remaining_hours: 2.5 } } - include_examples "update progress values", description: "recomputes remaining work accordingly" + include_examples "update progress values", description: "derives remaining work", + expected_hints: { + remaining_work: :derived + } end context "when work is changed to a negative value" do @@ -102,7 +76,8 @@ let(:expected_kept_attributes) { %w[remaining_hours] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and remaining work is kept" + description: "is an error state (to be detected by contract), and remaining work is kept", + expected_hints: {} end context "when another status is set" do @@ -110,14 +85,20 @@ let(:expected_derived_attributes) { { remaining_hours: 3.0 } } include_examples "update progress values", - description: "recomputes remaining work according to the % complete value of the new status" + description: "derives remaining work according to the % complete value of the new status", + expected_hints: { + remaining_work: :derived + } end context "when floating point operations are inaccurate (2.4000000000000004h)" do let(:set_attributes) { { estimated_hours: 8.0, status: status_70_pct_complete } } let(:expected_derived_attributes) { { remaining_hours: 2.4 } } # would be 2.4000000000000004 without rounding - include_examples "update progress values", description: "remaining work is computed and rounded (2.4)" + include_examples "update progress values", description: "remaining work is derived and rounded (2.4)", + expected_hints: { + remaining_work: :derived + } end end @@ -135,7 +116,8 @@ let(:expected_derived_attributes) { { remaining_hours: nil } } include_examples "update progress values", - description: "remaining work remains empty" + description: "remaining work remains empty", + expected_hints: {} end context "when work is set" do @@ -143,7 +125,10 @@ let(:expected_derived_attributes) { { remaining_hours: 10.0 } } include_examples "update progress values", - description: "remaining work is derived from work and % complete value of the status" + description: "remaining work is derived from work and % complete value of the status", + expected_hints: { + remaining_work: :derived + } end context "when work is set to a negative value" do @@ -151,7 +136,8 @@ let(:expected_kept_attributes) { %w[remaining_hours] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and remaining work is kept" + description: "is an error state (to be detected by contract), and remaining work is kept", + expected_hints: {} end context "when work is set with 2nd decimal rounding up" do @@ -159,7 +145,10 @@ let(:expected_derived_attributes) { { estimated_hours: 3.57, remaining_hours: 3.57 } } include_examples "update progress values", - description: "values are rounded up to 2 decimals and set to the same value" + description: "values are rounded up to 2 decimals and set to the same value", + expected_hints: { + remaining_work: :derived + } end end @@ -175,7 +164,18 @@ let(:expected_derived_attributes) { { done_ratio: 70 } } include_examples "update progress values", - description: "sets the % complete value to the status default % complete value" + description: "sets the % complete value to the status default % complete value", + expected_hints: {} + end + + # can happen if work is set and then is unset + context "when another status with another % complete value is set and work is cleared" do + let(:set_attributes) { { status: status_70_pct_complete, estimated_hours: nil } } + let(:expected_derived_attributes) { { done_ratio: 70 } } + + include_examples "update progress values", + description: "sets the % complete value to the status default % complete value", + expected_hints: {} end end end diff --git a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb index 0b16320a2748..f76e476fab7c 100644 --- a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb +++ b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb @@ -27,6 +27,7 @@ #++ require "spec_helper" +require_relative "shared_examples" # Scenarios specified in https://community.openproject.org/wp/40749 RSpec.describe WorkPackages::SetAttributesService::DeriveProgressValuesWorkBased, @@ -37,40 +38,6 @@ let(:work_package) { build_stubbed(:work_package, project:) } let(:instance) { described_class.new(work_package) } - shared_examples_for "update progress values" do |description:| - subject do - allow(work_package) - .to receive(:save) - - instance.call - end - - it description do - work_package.attributes = set_attributes - all_expected_attributes = {} - all_expected_attributes.merge!(expected_derived_attributes) if defined?(expected_derived_attributes) - if defined?(expected_kept_attributes) - kept = work_package.attributes.slice(*expected_kept_attributes) - if kept.size != expected_kept_attributes.size - raise ArgumentError, "expected_kept_attributes contains attributes that are not present in the work_package: " \ - "#{expected_kept_attributes - kept.keys} not present in #{work_package.attributes}" - end - all_expected_attributes.merge!(kept) - end - next if all_expected_attributes.blank? - - subject - - aggregate_failures do - expect(work_package).to have_attributes(all_expected_attributes) - expect(work_package).to have_attributes(set_attributes.except(*all_expected_attributes.keys)) - # work package is not saved and no errors are created - expect(work_package).not_to have_received(:save) - expect(work_package.errors).to be_empty - end - end - end - context "given a work package with work, remaining work, and % complete being set" do before do work_package.estimated_hours = 10.0 @@ -84,7 +51,10 @@ let(:expected_derived_attributes) { { remaining_hours: nil } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "keeps % complete, and clears remaining work" + include_examples "update progress values", description: "keeps % complete, and clears remaining work", + expected_hints: { + remaining_work: :cleared_because_work_is_empty + } end context "when remaining work is cleared" do @@ -92,7 +62,10 @@ let(:expected_derived_attributes) { { estimated_hours: nil } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "keeps % complete, and clears work" + include_examples "update progress values", description: "keeps % complete, and clears work", + expected_hints: { + work: :cleared_because_remaining_work_is_empty + } end context "when % complete is cleared" do @@ -100,35 +73,44 @@ let(:expected_derived_attributes) { { remaining_hours: nil } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "keeps work, and clears remaining work" + include_examples "update progress values", description: "keeps work, and clears remaining work", + expected_hints: { + remaining_work: :cleared_because_percent_complete_is_empty + } end context "when both work and remaining work are cleared" do let(:set_attributes) { { estimated_hours: nil, remaining_hours: nil } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "keeps % complete" + include_examples "update progress values", description: "keeps % complete", + expected_hints: {} end context "when work is changed and percent complete is set to the same value as before" do let(:set_attributes) { { estimated_hours: 20.0, done_ratio: 70 } } let(:expected_derived_attributes) { { remaining_hours: 6.0 } } - include_examples "update progress values", description: "derives remaining work from work and % complete" + include_examples "update progress values", description: "derives remaining work from work and % complete", + expected_hints: { + remaining_work: :derived + } end context "when both work and percent complete are cleared" do let(:set_attributes) { { estimated_hours: nil, done_ratio: nil } } let(:expected_kept_attributes) { %w[remaining_hours] } - include_examples "update progress values", description: "keeps remaining work" + include_examples "update progress values", description: "keeps remaining work", + expected_hints: {} end context "when both remaining work and percent complete are cleared" do let(:set_attributes) { { remaining_hours: nil, done_ratio: nil } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "keeps work" + include_examples "update progress values", description: "keeps work", + expected_hints: {} end context "when work is increased" do @@ -139,7 +121,11 @@ end include_examples "update progress values", - description: "remaining work is increased by the same amount, and % complete is derived" + description: "remaining work is increased by the same amount, and % complete is derived", + expected_hints: { + remaining_work: :increased_like_work, + percent_complete: :derived + } end context "when work is set to 0h" do @@ -149,7 +135,11 @@ end include_examples "update progress values", - description: "remaining work is set to 0h and % Complete is cleared" + description: "remaining work is set to 0h and % Complete is cleared", + expected_hints: { + remaining_work: :decreased_like_work, + percent_complete: :cleared_because_work_is_0h + } end context "when work is decreased" do @@ -160,7 +150,11 @@ end include_examples "update progress values", - description: "remaining work is decreased by the same amount, and % complete is derived" + description: "remaining work is decreased by the same amount, and % complete is derived", + expected_hints: { + remaining_work: :decreased_like_work, + percent_complete: :derived + } end context "when work is decreased below remaining work value" do @@ -171,7 +165,11 @@ end include_examples "update progress values", - description: "remaining work becomes 0h, and % complete becomes 100%" + description: "remaining work becomes 0h, and % complete becomes 100%", + expected_hints: { + remaining_work: :decreased_like_work, + percent_complete: :derived + } end context "when work is changed to a negative value" do @@ -180,7 +178,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when remaining work is changed" do @@ -188,14 +187,20 @@ let(:expected_derived_attributes) { { done_ratio: 80 } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "updates % complete accordingly" + include_examples "update progress values", description: "updates % complete accordingly", + expected_hints: { + percent_complete: :derived + } end context "when remaining work and % complete are both changed" do let(:set_attributes) { { remaining_hours: 12.0, done_ratio: 40 } } let(:expected_derived_attributes) { { estimated_hours: 20.0 } } - include_examples "update progress values", description: "work is derived" + include_examples "update progress values", description: "work is derived", + expected_hints: { + work: :derived + } end context "when remaining work is changed and % complete is changed to 100%" do @@ -203,7 +208,8 @@ let(:expected_kept_attributes) { %w[estimated_hours] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and work is kept" + description: "is an error state (to be detected by contract), and work is kept", + expected_hints: {} end context "when work and remaining work are both changed to negative values" do @@ -211,7 +217,8 @@ let(:expected_kept_attributes) { %w[done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % Complete is kept" + description: "is an error state (to be detected by contract), and % Complete is kept", + expected_hints: {} end context "when work and remaining work are both changed so that work is lower than remaining work" do @@ -219,7 +226,8 @@ let(:expected_kept_attributes) { %w[done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % Complete is kept" + description: "is an error state (to be detected by contract), and % Complete is kept", + expected_hints: {} end context "when work and remaining work are both changed to values with more than 2 decimals" do @@ -227,7 +235,10 @@ let(:expected_derived_attributes) { { estimated_hours: 10.12, remaining_hours: 5.68, done_ratio: 44 } } include_examples "update progress values", description: "rounds work and remaining work to 2 decimals " \ - "and updates % complete accordingly" + "and updates % complete accordingly", + expected_hints: { + percent_complete: :derived + } end context "when remaining work is changed to a value greater than work" do @@ -235,7 +246,8 @@ let(:expected_kept_attributes) { %w[done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % Complete is kept" + description: "is an error state (to be detected by contract), and % Complete is kept", + expected_hints: {} end context "when remaining work is changed to a negative value" do @@ -243,42 +255,58 @@ let(:expected_kept_attributes) { %w[done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % Complete is kept" + description: "is an error state (to be detected by contract), and % Complete is kept", + expected_hints: {} end context "when both work and remaining work are changed" do let(:set_attributes) { { estimated_hours: 20, remaining_hours: 2 } } let(:expected_derived_attributes) { { done_ratio: 90 } } - include_examples "update progress values", description: "updates % complete accordingly" + include_examples "update progress values", description: "updates % complete accordingly", + expected_hints: { + percent_complete: :derived + } end context "when work is changed and remaining work is cleared" do let(:set_attributes) { { estimated_hours: 8.0, remaining_hours: nil } } let(:expected_derived_attributes) { { done_ratio: nil } } - include_examples "update progress values", description: "% complete is cleared" + include_examples "update progress values", description: "% complete is cleared", + expected_hints: { + percent_complete: :cleared_because_remaining_work_is_empty + } end context "when percent complete is changed and work is cleared" do let(:set_attributes) { { done_ratio: 40, estimated_hours: nil } } let(:expected_derived_attributes) { { remaining_hours: nil } } - include_examples "update progress values", description: "remaining work is cleared" + include_examples "update progress values", description: "remaining work is cleared", + expected_hints: { + remaining_work: :cleared_because_work_is_empty + } end context "when percent complete is changed and remaining work is cleared" do let(:set_attributes) { { done_ratio: 40, remaining_hours: nil } } let(:expected_derived_attributes) { { estimated_hours: nil } } - include_examples "update progress values", description: "work is cleared" + include_examples "update progress values", description: "work is cleared", + expected_hints: { + work: :cleared_because_remaining_work_is_empty + } end context "when % complete is changed and remaining work is set to same value" do let(:set_attributes) { { done_ratio: 90, remaining_hours: 3 } } let(:expected_derived_attributes) { { estimated_hours: 30 } } - include_examples "update progress values", description: "work is derived" + include_examples "update progress values", description: "work is derived", + expected_hints: { + work: :derived + } end context "when work is set to the same value and remaining work is changed" do @@ -286,16 +314,23 @@ let(:expected_derived_attributes) { { done_ratio: 90 } } include_examples "update progress values", - description: "% complete is derived" + description: "% complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when work is increased and remaining work is set to its current value (to prevent it from being increased)" do # work changed by +10h let(:set_attributes) { { estimated_hours: 10.0 + 10.0, remaining_hours: 3 } } - let(:expected_derived_attributes) { { remaining_hours: 3.0, done_ratio: 85 } } + let(:expected_derived_attributes) { { done_ratio: 85 } } + let(:expected_kept_attributes) { %w[remaining_hours] } include_examples "update progress values", - description: "remaining work is kept (not increased), and % complete is derived" + description: "remaining work is kept (not increased), and % complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when % complete is changed" do @@ -303,7 +338,10 @@ let(:expected_derived_attributes) { { remaining_hours: 6.0 } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "work is kept, and remaining work is derived" + include_examples "update progress values", description: "work is kept, and remaining work is derived", + expected_hints: { + remaining_work: :derived + } end context "when % complete is changed to a negative value" do @@ -311,7 +349,8 @@ let(:expected_kept_attributes) { %w[estimated_hours remaining_hours] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and work and remaining work are kept" + description: "is an error state (to be detected by contract), and work and remaining work are kept", + expected_hints: {} end context "when % complete is more than 100%" do @@ -319,7 +358,8 @@ let(:expected_kept_attributes) { %w[estimated_hours remaining_hours] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and work and remaining work are kept" + description: "is an error state (to be detected by contract), and work and remaining work are kept", + expected_hints: {} end context "when % complete is a string not representing a valid percentage" do @@ -336,14 +376,16 @@ end include_examples "update progress values", - description: "is an error state (to be detected by contract), and work and remaining work are kept" + description: "is an error state (to be detected by contract), and work and remaining work are kept", + expected_hints: {} end context "when work, remaining work, and % complete are all changed to consistent values" do let(:set_attributes) { { estimated_hours: 20, remaining_hours: 12.0, done_ratio: 40 } } let(:expected_kept_attributes) { %w[estimated_hours remaining_hours done_ratio] } - include_examples "update progress values", description: "they are all kept" + include_examples "update progress values", description: "they are all kept", + expected_hints: {} end context "when work, remaining work, and % complete are all changed to inconsistent values" do @@ -351,7 +393,8 @@ let(:expected_kept_attributes) { %w[estimated_hours remaining_hours done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and all values are kept" + description: "is an error state (to be detected by contract), and all values are kept", + expected_hints: {} end end @@ -368,7 +411,10 @@ let(:expected_derived_attributes) { { remaining_hours: 14.0 } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "% complete is kept and remaining work is derived" + include_examples "update progress values", description: "% complete is kept and remaining work is derived", + expected_hints: { + remaining_work: :derived + } end context "when work is changed to a negative value" do @@ -377,7 +423,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when remaining work is set" do @@ -385,7 +432,10 @@ let(:expected_derived_attributes) { { done_ratio: 90.0 } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "work is kept and % complete is derived" + include_examples "update progress values", description: "work is kept and % complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when % complete is set" do @@ -393,7 +443,10 @@ let(:expected_derived_attributes) { { remaining_hours: 1.0 } } let(:expected_kept_attributes) { %w[estimated_hours] } - include_examples "update progress values", description: "work is kept and remaining work is derived" + include_examples "update progress values", description: "work is kept and remaining work is derived", + expected_hints: { + remaining_work: :derived + } end end @@ -410,7 +463,10 @@ let(:expected_derived_attributes) { { estimated_hours: 20.0 } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "% complete is kept and work is derived" + include_examples "update progress values", description: "% complete is kept and work is derived", + expected_hints: { + work: :derived + } end context "when % complete is 0% and remaining work is changed to a decimal rounded up" do @@ -424,7 +480,10 @@ end include_examples "update progress values", - description: "% complete is kept, values are rounded, and work is derived" + description: "% complete is kept, values are rounded, and work is derived", + expected_hints: { + work: :derived + } end context "when work is set" do @@ -432,7 +491,10 @@ let(:expected_derived_attributes) { { done_ratio: 80.0 } } let(:expected_kept_attributes) { %w[remaining_hours] } - include_examples "update progress values", description: "remaining work is kept and % complete is derived" + include_examples "update progress values", description: "remaining work is kept and % complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when % complete is changed" do @@ -440,7 +502,10 @@ let(:expected_derived_attributes) { { estimated_hours: 10.0 } } let(:expected_kept_attributes) { %w[remaining_hours] } - include_examples "update progress values", description: "remaining work is kept and work is derived" + include_examples "update progress values", description: "remaining work is kept and work is derived", + expected_hints: { + work: :derived + } end context "when % complete is changed to 100%" do @@ -449,7 +514,8 @@ include_examples "update progress values", description: "work and remaining work are kept because work cannot be derived " \ - "(error state to be detected by contract)" + "(error state to be detected by contract)", + expected_hints: {} end end @@ -466,7 +532,11 @@ let(:expected_derived_attributes) { { remaining_hours: 20.0, done_ratio: 0 } } include_examples "update progress values", - description: "remaining work is set to the same value and % complete is set to 0%" + description: "remaining work is set to the same value and % complete is set to 0%", + expected_hints: { + remaining_work: :same_as_work, + percent_complete: :derived + } end context "when work is changed and remaining work is cleared" do @@ -475,7 +545,8 @@ include_examples "update progress values", description: "% complete is kept and remaining work is kept empty and not recomputed " \ - "(error state to be detected by contract)" + "(error state to be detected by contract)", + expected_hints: {} end context "when work is changed to a negative value" do @@ -484,7 +555,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when % complete is set" do @@ -493,7 +565,10 @@ let(:expected_kept_attributes) { %w[estimated_hours] } include_examples "update progress values", - description: "work is kept and remaining work is derived" + description: "work is kept and remaining work is derived", + expected_hints: { + remaining_work: :derived + } end end @@ -511,7 +586,10 @@ let(:expected_kept_attributes) { %w[remaining_hours] } include_examples "update progress values", - description: "remaining work is kept to the same value and % complete is derived" + description: "remaining work is kept to the same value and % complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when work is set lower than remaining work" do @@ -520,7 +598,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when work is changed to a negative value" do @@ -529,7 +608,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when remaining work is changed" do @@ -537,7 +617,11 @@ let(:expected_derived_attributes) { { estimated_hours: 12.0, done_ratio: 0 } } include_examples "update progress values", - description: "work is set to the same value and % complete is set to 0%" + description: "work is set to the same value and % complete is set to 0%", + expected_hints: { + work: :same_as_remaining_work, + percent_complete: :derived + } end context "when remaining work is changed to a negative value" do @@ -546,7 +630,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and work are kept" + "and % complete and work are kept", + expected_hints: {} end context "when % complete is set" do @@ -554,7 +639,10 @@ let(:expected_derived_attributes) { { estimated_hours: 10.0 } } let(:expected_kept_attributes) { %w[remaining_hours] } - include_examples "update progress values", description: "work is derived" + include_examples "update progress values", description: "work is derived", + expected_hints: { + work: :derived + } end end @@ -571,7 +659,11 @@ let(:expected_derived_attributes) { { remaining_hours: 5.0, done_ratio: 0 } } include_examples "update progress values", - description: "remaining work is set to same value as work, and % complete is set to 0%" + description: "remaining work is increased like work, and % complete is set to 0%", + expected_hints: { + remaining_hours: :increased_like_work, + percent_complete: :derived + } end end @@ -588,7 +680,10 @@ let(:expected_derived_attributes) { { remaining_hours: 4.0 } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "% complete is kept and remaining work is derived" + include_examples "update progress values", description: "% complete is kept and remaining work is derived", + expected_hints: { + remaining_work: :derived + } end context "when work is set to a number with with 4 decimals" do @@ -598,7 +693,10 @@ include_examples "update progress values", description: "% complete is kept, work is rounded to 2 decimals, " \ - "and remaining work is derived and rounded to 2 decimals" + "and remaining work is derived and rounded to 2 decimals", + expected_hints: { + remaining_work: :derived + } end context "when work is set to a string" do @@ -618,14 +716,20 @@ let(:set_attributes) { { estimated_hours: 10.0, remaining_hours: 0 } } let(:expected_derived_attributes) { { done_ratio: 100 } } - include_examples "update progress values", description: "% complete is derived" + include_examples "update progress values", description: "% complete is derived", + expected_hints: { + percent_complete: :derived + } end - context "when work is set and remaining work is clered" do + context "when work is set and remaining work is cleared" do let(:set_attributes) { { estimated_hours: 10.0, remaining_hours: nil } } let(:expected_derived_attributes) { { done_ratio: nil } } - include_examples "update progress values", description: "% complete is cleared" + include_examples "update progress values", description: "% complete is cleared", + expected_hints: { + percent_complete: :cleared_because_remaining_work_is_empty + } end context "when work and remaining work are both set to negative values" do @@ -633,7 +737,8 @@ let(:expected_kept_attributes) { %w[done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % complete is kept" + description: "is an error state (to be detected by contract), and % complete is kept", + expected_hints: {} end context "when % complete is changed" do @@ -641,7 +746,8 @@ let(:expected_kept_attributes) { %w[estimated_hours remaining_hours] } include_examples "update progress values", - description: "work and remaining work are kept empty" + description: "work and remaining work are kept empty", + expected_hints: {} end end @@ -658,7 +764,10 @@ let(:expected_derived_attributes) { { remaining_hours: 0.0 } } let(:expected_kept_attributes) { %w[done_ratio] } - include_examples "update progress values", description: "% complete is kept and remaining work is set to 0h" + include_examples "update progress values", description: "% complete is kept and remaining work is set to 0h", + expected_hints: { + remaining_work: :derived + } end context "when work is set to a string" do @@ -678,7 +787,10 @@ let(:set_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0 } } let(:expected_derived_attributes) { { done_ratio: 50 } } - include_examples "update progress values", description: "% complete is derived" + include_examples "update progress values", description: "% complete is derived", + expected_hints: { + percent_complete: :derived + } end context "when remaining work is set to 0h" do @@ -686,7 +798,8 @@ let(:expected_kept_attributes) { %w[estimated_hours done_ratio] } include_examples "update progress values", - description: "is an error state (to be detected by contract), and % complete is kept" + description: "is an error state (to be detected by contract), and % complete is kept", + expected_hints: {} end end @@ -705,7 +818,11 @@ end include_examples "update progress values", - description: "remaining work is set to the same value and % complete is set to 0%" + description: "remaining work is set to the same value and % complete is set to 0%", + expected_hints: { + remaining_work: :same_as_work, + percent_complete: :derived + } end context "when work is set to a negative value" do @@ -714,14 +831,19 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and remaining work are kept" + "and % complete and remaining work are kept", + expected_hints: {} end context "when remaining work is set" do let(:set_attributes) { { remaining_hours: 10.0 } } let(:expected_derived_attributes) { { estimated_hours: 10.0, done_ratio: 0 } } - include_examples "update progress values", description: "work is set to the same value and % complete is set to 0%" + include_examples "update progress values", description: "work is set to the same value and % complete is set to 0%", + expected_hints: { + work: :same_as_remaining_work, + percent_complete: :derived + } end context "when remaining work is set to a negative value" do @@ -730,7 +852,8 @@ include_examples "update progress values", description: "is an error state (to be detected by contract), " \ - "and % complete and work are kept" + "and % complete and work are kept", + expected_hints: {} end context "when remaining work is set and work is cleared" do @@ -739,7 +862,8 @@ include_examples "update progress values", description: "% complete is kept and work is kept empty and not recomputed" \ - "(error state to be detected by contract)" + "(error state to be detected by contract)", + expected_hints: {} end context "when % complete is set" do @@ -747,7 +871,26 @@ let(:expected_kept_attributes) { %w[estimated_hours remaining_hours] } include_examples "update progress values", - description: "work and remaining work are kept empty" + description: "work and remaining work are kept empty", + expected_hints: {} + end + + context "when % complete is set and remaining work is cleared" do + let(:set_attributes) { { done_ratio: 80, remaining_hours: nil } } + let(:expected_kept_attributes) { %w[estimated_hours] } + + include_examples "update progress values", + description: "work and remaining work are kept empty", + expected_hints: {} + end + + context "when % complete is set and work is cleared" do + let(:set_attributes) { { done_ratio: 80, estimated_hours: nil } } + let(:expected_kept_attributes) { %w[remaining_hours] } + + include_examples "update progress values", + description: "work and remaining work are kept empty", + expected_hints: {} end end end diff --git a/spec/services/work_packages/set_attributes_service/shared_examples.rb b/spec/services/work_packages/set_attributes_service/shared_examples.rb new file mode 100644 index 000000000000..7598a3605aad --- /dev/null +++ b/spec/services/work_packages/set_attributes_service/shared_examples.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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. +#++ + +def to_work_package_field(name) + { + work: :estimated_hours, + remaining_work: :remaining_hours, + percent_complete: :done_ratio + }.fetch(name, name) +end + +# Sets values on a work package, calls a `DeriveProgressValuesBase` instance and +# check if the work_package as been updated as expected. +# +# * Use `let(:set_attributes)` to define the values to set on the work package: +# +# let(:set_attributes) { { estimated_hours: 10.0, remaining_hours: 1.0 } } +# +# * Use `let(:expected_derived_attributes)` to define the expected attribute +# values after derivation: +# +# let(:expected_derived_attributes) { { done_ratio: 90 } } +# +# * Use `let(:expected_kept_attributes)` to define the attributes which are +# expected to not change: +# +# let(:expected_kept_attributes) { %w[estimated_hours] } +RSpec.shared_examples_for "update progress values" do |description:, expected_hints:| + subject do + allow(work_package) + .to receive(:save) + + described_class.new(work_package).call + end + + it description do + work_package.attributes = set_attributes + all_expected_attributes = {} + all_expected_attributes.merge!(expected_derived_attributes) if defined?(expected_derived_attributes) + if defined?(expected_kept_attributes) + kept = work_package.attributes.slice(*expected_kept_attributes) + if kept.size != expected_kept_attributes.size + raise ArgumentError, "expected_kept_attributes contains attributes that are not present in the work_package: " \ + "#{expected_kept_attributes - kept.keys} not present in #{work_package.attributes}" + end + all_expected_attributes.merge!(kept) + end + next if all_expected_attributes.blank? + + subject + + aggregate_failures do + expect(work_package).to have_attributes(all_expected_attributes) + expect(work_package).to have_attributes(set_attributes.except(*all_expected_attributes.keys)) + if expected_hints + expected_hints = expected_hints.transform_keys { to_work_package_field(_1) } + expect(work_package.derived_progress_hints).to eq(expected_hints) + end + # work package is not saved and no errors are created + expect(work_package).not_to have_received(:save) + expect(work_package.errors).to be_empty + end + end +end From 6851972eb207a0d27bcf56e4734df8656c793cb2 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 27 Aug 2024 12:12:51 +0200 Subject: [PATCH 2/3] Handle invalid work and remaining work values In case work or remaining work are set to unparsable strings, no values should be derived and no hints should be displayed. --- app/contracts/work_packages/base_contract.rb | 9 +-- app/models/work_package.rb | 2 +- app/services/duration_converter.rb | 50 ++++++++++++++ .../derive_progress_values_work_based.rb | 12 +++- .../work_packages/base_contract_spec.rb | 24 +++++++ spec/services/duration_converter_spec.rb | 65 +++++++++++++++++++ .../derive_progress_values_work_based_spec.rb | 54 ++++++++++++++- 7 files changed, 207 insertions(+), 9 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 87c77f500319..239586b1efef 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -405,7 +405,7 @@ def validate_percent_complete_matches_work_and_remaining_work def validate_percent_complete_is_empty_when_work_is_zero return if WorkPackage.status_based_mode? - if work == 0 && percent_complete_set? + if work_set_and_valid? && work == 0 && percent_complete_set? errors.add(:done_ratio, :cannot_be_set_when_work_is_zero) end end @@ -419,7 +419,7 @@ def work_set? end def work_set_and_valid? - work_set? && work >= 0 + work_set? && work >= 0 && !model.errors.has_key?(:estimated_hours) end def work_empty? @@ -435,7 +435,7 @@ def remaining_work_set? end def remaining_work_set_and_valid? - remaining_work_set? && remaining_work >= 0 + remaining_work_set? && remaining_work >= 0 && !model.errors.has_key?(:remaining_hours) end def remaining_work_empty? @@ -445,7 +445,8 @@ def remaining_work_empty? def invalid_work_or_remaining_work_values? (work_set? && work.negative?) || (remaining_work_set? && remaining_work.negative?) || - remaining_work_exceeds_work? + (model.errors.has_key?(:estimated_hours) || model.errors.has_key?(:remaining_hours)) || + remaining_work_exceeds_work? end def remaining_work_exceeds_work? diff --git a/app/models/work_package.rb b/app/models/work_package.rb index de48fc2e390a..859a093eb4d6 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -565,7 +565,7 @@ def add_time_entry_for(user, attributes) def convert_duration_to_hours(value) if value.is_a?(String) begin - value = value.blank? ? nil : DurationConverter.parse(value) + value = DurationConverter.parse(value) rescue ChronicDuration::DurationParseError # keep invalid value, error shall be caught by numericality validator end diff --git a/app/services/duration_converter.rb b/app/services/duration_converter.rb index f267b5b05c24..1df40065e91f 100644 --- a/app/services/duration_converter.rb +++ b/app/services/duration_converter.rb @@ -76,6 +76,8 @@ class DurationConverter class << self def parse(duration_string) + return nil if duration_string.blank? + # Assume the next logical unit to allow users to write # durations such as "2h 1" assuming "1" is "1 minute" last_unit_in_string = duration_string.scan(/[a-zA-Z]+/) @@ -95,6 +97,21 @@ def parse(duration_string) **duration_length_options) / 3600.to_f end + def valid?(duration) + case duration + when String + duration.blank? || parseable?(duration) + when Numeric + duration >= 0 + when nil + true + else + false + end + rescue ChronicDuration::DurationParseError + false + end + def output(duration_in_hours) return duration_in_hours if duration_in_hours.nil? @@ -108,6 +125,39 @@ def output(duration_in_hours) private + def parseable?(duration_string) + if number = Integer(duration_string, 10, exception: false) || Float(duration_string, exception: false) + number >= 0 + else + begin + internal_parse(duration_string) + true + rescue ChronicDuration::DurationParseError + false + end + end + end + + def internal_parse(duration_string) + # Assume the next logical unit to allow users to write + # durations such as "2h 1" assuming "1" is "1 minute" + last_unit_in_string = duration_string.scan(/[a-zA-Z]+/) + .last + default_unit = if last_unit_in_string + last_unit_in_string + .then { |last_unit| UNIT_ABBREVIATION_MAP[last_unit.downcase] } + .then { |last_unit| NEXT_UNIT_MAP[last_unit] } + else + "hours" + end + + ChronicDuration.parse(duration_string, + keep_zero: true, + default_unit:, + raise_exceptions: true, + **duration_length_options) / 3600.to_f + end + def format Setting.duration_format == "days_and_hours" ? :days_and_hours : :hours_only end diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb index 9921ee1cb243..87d96d6db8de 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb @@ -45,8 +45,8 @@ def derive_progress_attributes end def invalid_progress_values? - work&.negative? \ - || remaining_work&.negative? \ + work_invalid? \ + || remaining_work_invalid? \ || percent_complete_out_of_range? \ || percent_complete_unparsable? \ || remaining_work_set_greater_than_work? @@ -152,6 +152,14 @@ def work_from_percent_complete_and_remaining_work remaining_work / remaining_percent_complete end + def work_invalid? + !DurationConverter.valid?(work_package.estimated_hours_before_type_cast) + end + + def remaining_work_invalid? + !DurationConverter.valid?(work_package.remaining_hours_before_type_cast) + end + def percent_complete_unparsable? !PercentageConverter.valid?(work_package.done_ratio_before_type_cast) end diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index c89622a048da..971317d151ad 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -590,6 +590,30 @@ done_ratio: nil end + context "when work is not a valid duration (an invalid string for instance)" do + let(:estimated_hours) { "invalid" } + let(:remaining_hours) { 6.0 } + let(:done_ratio) { 50 } + + # no errors should be reported for % complete in this case to not overload + # the progress modal with error messages. + include_examples "contract is invalid", estimated_hours: :not_a_number, + remaining_hours: nil, + done_ratio: nil + end + + context "when remaining work is not a valid duration (an invalid string for instance)" do + let(:estimated_hours) { 10.0 } + let(:remaining_hours) { "invalid" } + let(:done_ratio) { 50 } + + # no errors should be reported for % complete in this case to not overload + # the progress modal with error messages. + include_examples "contract is invalid", estimated_hours: nil, + remaining_hours: :not_a_number, + done_ratio: nil + end + context "when all three empty" do let(:estimated_hours) { nil } let(:remaining_hours) { nil } diff --git a/spec/services/duration_converter_spec.rb b/spec/services/duration_converter_spec.rb index 622ed4327c08..95c082fd15b9 100644 --- a/spec/services/duration_converter_spec.rb +++ b/spec/services/duration_converter_spec.rb @@ -32,6 +32,13 @@ RSpec.describe DurationConverter do describe ".parse" do + it "returns nil when given blank strings or nil" do + expect(described_class.parse("")).to be_nil + expect(described_class.parse(" ")).to be_nil + expect(described_class.parse(" \t ")).to be_nil + expect(described_class.parse(nil)).to be_nil + end + it "returns 0 when given 0 duration" do expect(described_class.parse("0 hrs")).to eq(0) end @@ -53,6 +60,64 @@ end end + describe ".valid?", :aggregate_failures do + it "returns true for positive numbers" do + expect(described_class.valid?(0)).to be(true) + expect(described_class.valid?(0.0)).to be(true) + expect(described_class.valid?(100)).to be(true) + expect(described_class.valid?(100.0)).to be(true) + expect(described_class.valid?(789)).to be(true) + expect(described_class.valid?(789.123)).to be(true) + end + + it "returns false for negative numbers" do + expect(described_class.valid?(-0.01)).to be(false) + expect(described_class.valid?(-1)).to be(false) + expect(described_class.valid?(-1.5)).to be(false) + expect(described_class.valid?(-456)).to be(false) + expect(described_class.valid?(-456.789)).to be(false) + end + + it "returns true for blank values" do + expect(described_class.valid?(nil)).to be(true) + expect(described_class.valid?("")).to be(true) + expect(described_class.valid?(" ")).to be(true) + expect(described_class.valid?(" \t ")).to be(true) + end + + it "returns true for strings representing a positive number or a valid duration" do + expect(described_class.valid?("50")).to be(true) + expect(described_class.valid?(" 50 ")).to be(true) + expect(described_class.valid?(" +1278 ")).to be(true) + expect(described_class.valid?(" -0 ")).to be(true) + expect(described_class.valid?(" 1234.0 h ")).to be(true) + expect(described_class.valid?("12h.4")).to be(true) # 12h 0.4m + expect(described_class.valid?("1 week 2 days 3 hours 5 minutes")).to be(true) + end + + it "returns false for strings not representing a positive number nor a valid duration" do + expect(described_class.valid?("invalid")).to be(false) + expect(described_class.valid?("dsg")).to be(false) + expect(described_class.valid?(" +0h ")).to be(false) + expect(described_class.valid?("-5")).to be(false) + expect(described_class.valid?("-5.6")).to be(false) + expect(described_class.valid?("5.")).to be(false) + expect(described_class.valid?("5.h")).to be(false) + expect(described_class.valid?("-5.75h")).to be(false) + expect(described_class.valid?("invalid")).to be(false) + expect(described_class.valid?("-")).to be(false) + expect(described_class.valid?("+")).to be(false) + expect(described_class.valid?("日")).to be(false) + expect(described_class.valid?("invalid 123")).to be(false) + expect(described_class.valid?("123 invalid")).to be(false) + expect(described_class.valid?("- 23")).to be(false) + expect(described_class.valid?("123..5")).to be(false) + expect(described_class.valid?("1'234.5")).to be(false) + expect(described_class.valid?("1,234.5")).to be(false) + expect(described_class.valid?("12mm")).to be(false) + end + end + describe ".output" do it "returns nil when given nil" do expect(described_class.output(nil)).to be_nil diff --git a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb index f76e476fab7c..631fc108651b 100644 --- a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb +++ b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb @@ -701,7 +701,8 @@ context "when work is set to a string" do let(:set_attributes) { { estimated_hours: "I am a string" } } - let(:expected_derived_attributes) { { estimated_hours: 0.0, remaining_hours: 0.0 } } + let(:expected_derived_attributes) { { estimated_hours: 0.0 } } + let(:expected_kept_attributes) { %w[remaining_hours done_ratio] } it "keeps the original string value in the _before_type_cast method " \ "so that validation can detect it is invalid" do @@ -710,6 +711,11 @@ expect(work_package.estimated_hours_before_type_cast).to eq("I am a string") end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and remaining work and % complete are kept", + expected_hints: {} end context "when work and remaining work are set" do @@ -772,7 +778,8 @@ context "when work is set to a string" do let(:set_attributes) { { estimated_hours: "I am a string" } } - let(:expected_derived_attributes) { { estimated_hours: 0.0, remaining_hours: 0.0 } } + let(:expected_derived_attributes) { { estimated_hours: 0.0 } } + let(:expected_kept_attributes) { %w[remaining_hours done_ratio] } it "keeps the original string value in the _before_type_cast method " \ "so that validation can detect it is invalid" do @@ -781,6 +788,30 @@ expect(work_package.estimated_hours_before_type_cast).to eq("I am a string") end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and remaining work and % complete are kept", + expected_hints: {} + end + + context "when remaining work is set to a string" do + let(:set_attributes) { { remaining_hours: "I am a string" } } + let(:expected_derived_attributes) { { remaining_hours: 0.0 } } + let(:expected_kept_attributes) { %w[estimated_hours done_ratio] } + + it "keeps the original string value in the _before_type_cast method " \ + "so that validation can detect it is invalid" do + work_package.attributes = set_attributes + instance.call + + expect(work_package.remaining_hours_before_type_cast).to eq("I am a string") + end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and work and % complete are kept", + expected_hints: {} end context "when work and remaining work are set" do @@ -856,6 +887,25 @@ expected_hints: {} end + context "when remaining work is set to a string" do + let(:set_attributes) { { remaining_hours: "I am a string" } } + let(:expected_derived_attributes) { { remaining_hours: 0.0 } } + let(:expected_kept_attributes) { %w[estimated_hours done_ratio] } + + it "keeps the original string value in the _before_type_cast method " \ + "so that validation can detect it is invalid" do + work_package.attributes = set_attributes + instance.call + + expect(work_package.remaining_hours_before_type_cast).to eq("I am a string") + end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and work and % complete are kept", + expected_hints: {} + end + context "when remaining work is set and work is cleared" do let(:set_attributes) { { estimated_hours: nil, remaining_hours: 6.7 } } let(:expected_kept_attributes) { %w[done_ratio] } From 5fc40d4af102f58a618c882314ebbdf797afcecb Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 27 Aug 2024 15:56:24 +0200 Subject: [PATCH 3/3] Handle cases with imprecise progress value derivations As the work and remaining work values are rounded to two decimals, it can lead to strange result of % complete computation and validation. Two examples: - A derived % complete of 99.9% should not be rounded to 100% because there is still some work remaining. It should be 99% in this case. - Setting work to 2h and remaining work to 1.999h should not lead to 99% complete because remaining work will be rounded to 2h, so % complete should be 100% in this case. These really are edge cases. --- app/contracts/work_packages/base_contract.rb | 21 +++-- .../derive_progress_values_base.rb | 12 ++- .../derive_progress_values_work_based.rb | 23 +++-- .../work_packages/base_contract_spec.rb | 20 +++++ .../derive_progress_values_work_based_spec.rb | 89 +++++++++++++++++++ 5 files changed, 147 insertions(+), 18 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 239586b1efef..dc62c589bf41 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -58,6 +58,8 @@ class BaseContract < ::ModelContract && WorkPackage.work_based_mode? } do if OpenProject::FeatureDecisions.percent_complete_edition_active? + next if invalid_work_or_remaining_work_values? # avoid too many error messages at the same time + validate_percent_complete_matches_work_and_remaining_work validate_percent_complete_is_empty_when_work_is_zero validate_percent_complete_is_set_when_work_and_remaining_work_are_set @@ -387,9 +389,7 @@ def validate_remaining_work_is_set_when_work_and_percent_complete_are_set end def validate_percent_complete_is_set_when_work_and_remaining_work_are_set - return if remaining_work_exceeds_work? # avoid too many error messages at the same time - - if work_set_and_valid? && remaining_work_set_and_valid? && work != 0 && percent_complete_empty? + if work_set? && remaining_work_set? && work != 0 && percent_complete_empty? errors.add(:done_ratio, :must_be_set_when_work_and_remaining_work_are_set) end end @@ -397,7 +397,7 @@ def validate_percent_complete_is_set_when_work_and_remaining_work_are_set def validate_percent_complete_matches_work_and_remaining_work return if percent_complete_derivation_unapplicable? - if percent_complete != percent_complete_derived_from_work_and_remaining_work + if !percent_complete_range_derived_from_work_and_remaining_work.cover?(percent_complete) errors.add(:done_ratio, :does_not_match_work_and_remaining_work) end end @@ -405,7 +405,7 @@ def validate_percent_complete_matches_work_and_remaining_work def validate_percent_complete_is_empty_when_work_is_zero return if WorkPackage.status_based_mode? - if work_set_and_valid? && work == 0 && percent_complete_set? + if work == 0 && percent_complete_set? errors.add(:done_ratio, :cannot_be_set_when_work_is_zero) end end @@ -476,13 +476,16 @@ def percent_complete_empty? def percent_complete_derivation_unapplicable? WorkPackage.status_based_mode? || # only applicable in work-based mode work_empty? || remaining_work_empty? || percent_complete_empty? || # only applicable if all 3 values are set - work == 0 || percent_complete == 100 || # only applicable if not in special cases leading to divisions by zero - invalid_work_or_remaining_work_values? # only applicable if work and remaining work values are valid + work == 0 || percent_complete == 100 # only applicable if not in special cases leading to divisions by zero end - def percent_complete_derived_from_work_and_remaining_work + def percent_complete_range_derived_from_work_and_remaining_work work_done = work - remaining_work - (100 * work_done.to_f / work).round + percentage = (100 * work_done.to_f / work) + + lower_bound = percentage.truncate + upper_bound = lower_bound + 1 + lower_bound..upper_bound end def validate_no_reopen_on_closed_version diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb index 87f5d610eda9..3d8389408435 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_base.rb @@ -75,6 +75,10 @@ def work_not_provided_by_user? !work_came_from_user? end + def work_valid? + DurationConverter.valid?(work_package.estimated_hours_before_type_cast) + end + def remaining_work work_package.remaining_hours end @@ -107,6 +111,10 @@ def remaining_work_not_provided_by_user? !remaining_work_came_from_user? end + def remaining_work_valid? + DurationConverter.valid?(work_package.remaining_hours_before_type_cast) + end + def percent_complete work_package.done_ratio end @@ -151,11 +159,11 @@ def round_progress_values # prevent the numericality validator from working when setting the field # to a string value. rounded = work&.round(2) - if rounded != work + if rounded != work && work_valid? self.work = rounded end rounded = remaining_work&.round(2) - if rounded != remaining_work + if rounded != remaining_work && remaining_work_valid? self.remaining_work = rounded end end diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb index 87d96d6db8de..fa9ee80e4464 100644 --- a/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb +++ b/app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb @@ -124,7 +124,7 @@ def update_remaining_work def update_percent_complete return if work_empty? - if work.zero? + if work < 0.005 set_hint(:done_ratio, :cleared_because_work_is_0h) self.percent_complete = nil elsif remaining_work_empty? @@ -141,10 +141,19 @@ def skip_percent_complete_derivation! end def percent_complete_from_work_and_remaining_work - completed_work = work - remaining_work - completion_ratio = completed_work.to_f / work - - (completion_ratio * 100).round + rounded_work = work.round(2) + rounded_remaining_work = remaining_work.round(2) + completed_work = rounded_work - rounded_remaining_work + completion_ratio = completed_work.to_f / rounded_work + + percentage = (completion_ratio * 100) + case percentage + in 0 then 0 + in 0..1 then 1 + in 99...100 then 99 + else + percentage.round + end end def work_from_percent_complete_and_remaining_work @@ -153,11 +162,11 @@ def work_from_percent_complete_and_remaining_work end def work_invalid? - !DurationConverter.valid?(work_package.estimated_hours_before_type_cast) + !work_valid? end def remaining_work_invalid? - !DurationConverter.valid?(work_package.remaining_hours_before_type_cast) + !remaining_work_valid? end def percent_complete_unparsable? diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index 971317d151ad..90845e55669b 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -614,6 +614,18 @@ done_ratio: nil end + context "when work is 0h and remaining work is not a valid duration (an invalid string for instance)" do + let(:estimated_hours) { 0.0 } + let(:remaining_hours) { "invalid" } + let(:done_ratio) { 50 } + + # no errors should be reported for % complete in this case to not overload + # the progress modal with error messages. + include_examples "contract is invalid", estimated_hours: nil, + remaining_hours: :not_a_number, + done_ratio: nil + end + context "when all three empty" do let(:estimated_hours) { nil } let(:remaining_hours) { nil } @@ -630,6 +642,14 @@ include_examples "contract is valid" end + context "with inexact calculated % complete value (rounded to 1% VS 0.05% actual)" do + let(:estimated_hours) { 2000 } + let(:remaining_hours) { 1999 } + let(:done_ratio) { 1 } + + include_examples "contract is valid" + end + context "when all three set with inconsistent values" do let(:estimated_hours) { 10 } let(:remaining_hours) { 0 } diff --git a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb index 631fc108651b..e5a5941edf59 100644 --- a/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb +++ b/spec/services/work_packages/set_attributes_service/derive_progress_values_work_based_spec.rb @@ -142,6 +142,44 @@ } end + context "when work is set to 0.0049h" do + let(:set_attributes) { { estimated_hours: 0.0049 } } + let(:expected_derived_attributes) do + { estimated_hours: 0, remaining_hours: 0, done_ratio: nil } + end + + include_examples "update progress values", + description: "work and remaining work are set to 0h (rounded) and % Complete is cleared", + expected_hints: { + remaining_work: :decreased_like_work, + percent_complete: :cleared_because_work_is_0h + } + end + + context "when % complete is derived to 99.5% or more" do + let(:set_attributes) { { estimated_hours: 20000, remaining_hours: 1 } } + let(:expected_derived_attributes) { { done_ratio: 99 } } + let(:expected_kept_attributes) { %w[remaining_hours] } + + include_examples "update progress values", + description: "% complete is set to 99% instead of rounding it to 100%", + expected_hints: { + percent_complete: :derived + } + end + + context "when % complete is derived to 0.5% or less" do + let(:set_attributes) { { estimated_hours: 20000, remaining_hours: 19999 } } + let(:expected_derived_attributes) { { done_ratio: 1 } } + let(:expected_kept_attributes) { %w[remaining_hours] } + + include_examples "update progress values", + description: "% complete is set to 1% instead of rounding it to 0%", + expected_hints: { + percent_complete: :derived + } + end + context "when work is decreased" do # work changed by -2h let(:set_attributes) { { estimated_hours: 10.0 - 2.0 } } @@ -486,6 +524,17 @@ } end + context "when work and remaining work are different but would be identical if rounded" do + let(:set_attributes) { { estimated_hours: 1.997, remaining_hours: 1.995 } } + let(:expected_derived_attributes) { { estimated_hours: 2.0, remaining_hours: 2.0, done_ratio: 0 } } + + include_examples "update progress values", + description: "% complete is calculated with rounded values", + expected_hints: { + done_ratio: :derived + } + end + context "when work is set" do let(:set_attributes) { { estimated_hours: 10.0 } } let(:expected_derived_attributes) { { done_ratio: 80.0 } } @@ -795,6 +844,26 @@ expected_hints: {} end + context "when work is set to an invalid string with a leading number " \ + "which could be rounded ('3.999 invalid' for instance)" do + let(:set_attributes) { { estimated_hours: "3.999 invalid" } } + let(:expected_derived_attributes) { { estimated_hours: 3.999 } } + let(:expected_kept_attributes) { %w[remaining_hours done_ratio] } + + it "keeps the original string value in the _before_type_cast method " \ + "so that validation can detect it is invalid" do + work_package.attributes = set_attributes + instance.call + + expect(work_package.estimated_hours_before_type_cast).to eq("3.999 invalid") + end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and remaining work and % complete are kept", + expected_hints: {} + end + context "when remaining work is set to a string" do let(:set_attributes) { { remaining_hours: "I am a string" } } let(:expected_derived_attributes) { { remaining_hours: 0.0 } } @@ -814,6 +883,26 @@ expected_hints: {} end + context "when remaining work is set to an invalid string with a leading " \ + "number which could be rounded ('3.999 invalid' for instance)" do + let(:set_attributes) { { remaining_hours: "3.999 invalid" } } + let(:expected_derived_attributes) { { remaining_hours: 3.999 } } + let(:expected_kept_attributes) { %w[estimated_hours done_ratio] } + + it "keeps the original string value in the _before_type_cast method " \ + "so that validation can detect it is invalid" do + work_package.attributes = set_attributes + instance.call + + expect(work_package.remaining_hours_before_type_cast).to eq("3.999 invalid") + end + + include_examples "update progress values", + description: "is an error state (to be detected by contract), " \ + "and work and % complete are kept", + expected_hints: {} + end + context "when work and remaining work are set" do let(:set_attributes) { { estimated_hours: 10.0, remaining_hours: 5.0 } } let(:expected_derived_attributes) { { done_ratio: 50 } }