Merge pull request #16568 from opf/feature/57258-fix-issues-found-in-…

…review

[57258] Fix issues found during review

app/forms/work_packages/progress_form.rb

@@ -151,6 +151,7 @@ def readonly_text_field(group,
       name:,
       value: field_value(name),
       label:,
+      caption: field_hint_message(name),
       readonly: true,
       classes: "input--readonly",
       placeholder: ("-" if placeholder)
@@ -182,10 +183,7 @@ def field_value(name)
   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}")
+    work_package.derived_progress_hint(field_name)&.message
   end
 
   def validation_message(name)
@@ -201,7 +199,7 @@ def as_percent(value)
   def default_field_options(name)
     data = { "work-packages--progress--preview-progress-target": "progressInput",
              "work-packages--progress--touched-field-marker-target": "progressInput",
-             action: "input->work-packages--progress--touched-field-marker#markFieldAsTouched" }
+             action: "work-packages--progress--touched-field-marker#markFieldAsTouched" }
 
     if @focused_field == name
       data[:"work-packages--progress--focus-field-target"] = "fieldToFocus"

app/models/work_package.rb

@@ -332,12 +332,12 @@ def done_ratio=(value)
     write_attribute :done_ratio, convert_value_to_percentage(value)
   end
 
-  def derived_progress_hints=(hints)
-    @derived_progress_hints = hints
+  def set_derived_progress_hint(field_name, hint, **params)
+    derived_progress_hints[field_name] = ProgressHint.new("#{field_name}.#{hint}", params)
   end
 
-  def derived_progress_hints
-    @derived_progress_hints ||= {}
+  def derived_progress_hint(field_name)
+    derived_progress_hints[field_name]
   end
 
   def duration_in_hours
@@ -553,6 +553,10 @@ def <=>(other)
 
   private
 
+  def derived_progress_hints
+    @derived_progress_hints ||= {}
+  end
+
   def add_time_entry_for(user, attributes)
     return if time_entry_blank?(attributes)
 

app/models/work_package/progress_hint.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+# Hint to be displayed in the progress popover under a progress value.
+#
+# `key` is like `field_name`.`hint` and is used to build up the translation
+# key. `params` is the translation parameters as some translation are
+# parameterized.
+class WorkPackage
+  ProgressHint = Data.define(:key, :params) do
+    def initialize(key:, params: {})
+      super
+    end
+
+    def message
+      I18n.t("work_package.progress.derivation_hints.#{key}", **to_hours(params))
+    end
+
+    def reason
+      key.split(".", 2).last
+    end
+
+    def to_hours(params)
+      params.transform_values { |value| DurationConverter.output(value.abs) }
+    end
+  end
+end

app/services/work_packages/set_attributes_service/derive_progress_values_base.rb

@@ -79,6 +79,10 @@ def work_valid?
       DurationConverter.valid?(work_package.estimated_hours_before_type_cast)
     end
 
+    def work_invalid?
+      !work_valid?
+    end
+
     def remaining_work
       work_package.remaining_hours
     end
@@ -115,6 +119,10 @@ def remaining_work_valid?
       DurationConverter.valid?(work_package.remaining_hours_before_type_cast)
     end
 
+    def remaining_work_invalid?
+      !remaining_work_valid?
+    end
+
     def percent_complete
       work_package.done_ratio
     end
@@ -149,8 +157,8 @@ def percent_complete_not_provided_by_user?
 
     private
 
-    def set_hint(field, hint)
-      work_package.derived_progress_hints[field] = hint
+    def set_hint(field, hint, **params)
+      work_package.set_derived_progress_hint(field, hint, **params)
     end
 
     def round_progress_values

app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb

@@ -33,24 +33,39 @@ class DeriveProgressValuesStatusBased < DeriveProgressValuesBase
     def derive_progress_attributes
       raise ArgumentError, "Cannot use #{self.class.name} in work-based mode" if WorkPackage.work_based_mode?
 
-      update_percent_complete
-      update_remaining_work_from_percent_complete
+      # do not change anything if some values are invalid: this will be detected
+      # by the contract and errors will be set.
+      return if invalid_progress_values?
+
+      update_percent_complete if derive_percent_complete?
+      update_remaining_work if derive_remaining_work?
+    end
+
+    def invalid_progress_values?
+      work_invalid?
+    end
+
+    def derive_percent_complete?
+      status_percent_complete_changed?
+    end
+
+    def derive_remaining_work?
+      status_percent_complete_changed? || work_changed?
+    end
+
+    def status_percent_complete_changed?
+      work_package.status_id_changed? && work_package.status.default_done_ratio != work_package.done_ratio_was
     end
 
     # Update +% complete+ from the status if the status changed.
     def update_percent_complete
-      return unless work_package.status_id_changed?
-
       self.percent_complete = work_package.status.default_done_ratio
     end
 
     # When in "Status-based" mode for progress calculation, remaining work is
     # always derived from % complete and work. If work is unset, then remaining
     # work must be unset too.
-    def update_remaining_work_from_percent_complete
-      return if remaining_work_came_from_user?
-      return if work&.negative?
-
+    def update_remaining_work
       if work_empty?
         return unless work_changed?
 

app/services/work_packages/set_attributes_service/derive_progress_values_work_based.rb

@@ -100,9 +100,9 @@ def update_remaining_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)
+          set_hint(:remaining_hours, :increased_by_delta_like_work, delta:)
         elsif delta.negative?
-          set_hint(:remaining_hours, :decreased_like_work)
+          set_hint(:remaining_hours, :decreased_by_delta_like_work, delta:)
         end
         self.remaining_work = (remaining_work + delta).clamp(0.0, work)
       elsif work_empty?
@@ -161,14 +161,6 @@ def work_from_percent_complete_and_remaining_work
       remaining_work / remaining_percent_complete
     end
 
-    def work_invalid?
-      !work_valid?
-    end
-
-    def remaining_work_invalid?
-      !remaining_work_valid?
-    end
-
     def percent_complete_unparsable?
       !PercentageConverter.valid?(work_package.done_ratio_before_type_cast)
     end

config/locales/en.yml

@@ -3773,9 +3773,9 @@ en:
         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."
+          decreased_by_delta_like_work: "Decreased by %{delta}, matching the reduction in Work."
           derived: "Derived from Work and % Complete."
-          increased_like_work: "Increased by the same amount as Work."
+          increased_by_delta_like_work: "Increased by %{delta}, matching the increase in Work."
           same_as_work: "Set to same value as Work."
     permissions:
       comment: "Comment"

frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts

@@ -61,7 +61,11 @@ export default class PreviewProgressController extends Controller {
     };
 
     this.progressInputTargets.forEach((target) => {
-      target.addEventListener('input', this.debouncedPreview);
+      if (target.tagName.toLowerCase() === 'select') {
+        target.addEventListener('change', this.debouncedPreview);
+      } else {
+        target.addEventListener('input', this.debouncedPreview);
+      }
       target.addEventListener('blur', this.debouncedPreview);
     });
 

spec/features/work_packages/progress_modal_pre_14_4_without_percent_complete_edition_spec.rb

@@ -208,7 +208,7 @@ def update_work_package_with(work_package, attributes)
         remaining_work_field = ProgressEditField.new(work_package_row, :remainingTime)
         work_field.activate!
 
-        remaining_work_field.expect_read_only_modal_field
+        remaining_work_field.expect_modal_field_read_only
       end
     end
 
@@ -356,7 +356,8 @@ def update_work_package_with(work_package, attributes)
 
           work_edit_field.expect_modal_field_value("10h")
           remaining_work_edit_field.expect_modal_field_value("2.12h") # 2h 7m
-          percent_complete_edit_field.expect_modal_field_value("78", readonly: true)
+          percent_complete_edit_field.expect_modal_field_read_only
+          percent_complete_edit_field.expect_modal_field_value("78")
         end
       end
 
@@ -408,7 +409,8 @@ def update_work_package_with(work_package, attributes)
 
           work_edit_field.expect_modal_field_value("")
           remaining_work_edit_field.expect_modal_field_value("", disabled: true)
-          percent_complete_edit_field.expect_modal_field_value("-", readonly: true)
+          percent_complete_edit_field.expect_modal_field_read_only
+          percent_complete_edit_field.expect_modal_field_value("-")
         end
       end
 
@@ -468,7 +470,7 @@ def update_work_package_with(work_package, attributes)
 
       work_edit_field.activate!
 
-      percent_complete_edit_field.expect_read_only_modal_field
+      percent_complete_edit_field.expect_modal_field_read_only
     end
   end