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 } }