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

@@ -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,25 +389,23 @@ 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
 
     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
 
     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

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

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?

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 }

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