Merge pull request #16539 from opf/feature/57258-progress-values-deri…

…vation-hints

[57258] Indicate which fields are automatically derived and why in progress pop over

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

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,17 +389,15 @@ 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
@@ -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?
@@ -475,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/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

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
@@ -557,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

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

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
@@ -141,24 +149,26 @@ 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
       # 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
 
     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)

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