Fix lost progress popover state on submit with invalid values

When submitting with invalid values, the progress popover was updated
via turbo-stream with a modal which did not include the touched fields.
This caused the fields to be ignored on the second submit, even if they
appeared to be invalid. This was also causing the preview to not work as
expected.

This has been changed:
- the update is done through a turbo-stream morph instead of an update.
  It avoids disconnecting and reconnecting the stimulus controllers.
- the touched fields are now correctly sent to the component for
  rendering. The state is maintained in the stimulus controller instead
  of the hidden fields. Fields are still needed for form submission.
- the fields sent to the server when previewing are exactly the same as
  the ones being sent on submit.
- the turbo_stream commands are sent directly from the controller. None
  are sent if the update completed successfully as the modal will be
  closed by client side javascript anyway.
- the referrer-field is now just the field name instead of the full
  work package key, to reduce noise.

app/components/work_packages/progress/work_based/modal_body_component.html.erb

@@ -8,7 +8,7 @@
         data: { "application-target": "dynamic",
                 "work-packages--progress--preview-target": "form",
                 controller: "work-packages--progress--focus-field " \
-                          "work-packages--progress--preview" }
+                            "work-packages--progress--preview" }
       ) do |f| %>
     <%= flex_layout do |modal_body| %>
       <% modal_body.with_row(classes: "FormControl-horizontalGroup--sm-vertical") do |_fields| %>

app/controllers/work_packages/progress_controller.rb

@@ -37,24 +37,18 @@ class WorkPackages::ProgressController < ApplicationController
   layout false
   authorization_checked! :new, :edit, :create, :update
 
-  helper_method :modal_class
-
   def new
     make_fake_initial_work_package
     set_progress_attributes_to_work_package
 
-    render modal_class.new(@work_package,
-                           focused_field: params[:field],
-                           touched_field_map:)
+    render progress_modal_component
   end
 
   def edit
     find_work_package
     set_progress_attributes_to_work_package
 
-    render modal_class.new(@work_package,
-                           focused_field: params[:field],
-                           touched_field_map:)
+    render progress_modal_component
   end
 
   # rubocop:disable Metrics/AbcSize
@@ -71,7 +65,9 @@ def create
           # Angular has context as to the success or failure of
           # the request in order to fetch the new set of Work Package
           # attributes in the ancestry solely on success.
-          render :update, status: :unprocessable_entity
+          render turbo_stream: [
+            turbo_stream.morph("work_package_progress_modal", progress_modal_component)
+          ], status: :unprocessable_entity
         end
       end
     # following 3 lines to be removed in 15.0 with :percent_complete_edition feature flag removal
@@ -95,7 +91,9 @@ def update
 
     if service_call.success?
       respond_to do |format|
-        format.turbo_stream
+        format.turbo_stream do
+          render turbo_stream: []
+        end
       end
     else
       respond_to do |format|
@@ -104,14 +102,20 @@ def update
           # Angular has context as to the success or failure of
           # the request in order to fetch the new set of Work Package
           # attributes in the ancestry solely on success.
-          render :update, status: :unprocessable_entity
+          render turbo_stream: [
+            turbo_stream.morph("work_package_progress_modal", progress_modal_component)
+          ], status: :unprocessable_entity
         end
       end
     end
   end
 
   private
 
+  def progress_modal_component
+    modal_class.new(@work_package, focused_field:, touched_field_map:)
+  end
+
   def modal_class
     if WorkPackage.use_status_for_done_ratio?
       WorkPackages::Progress::StatusBased::ModalBodyComponent
@@ -120,6 +124,10 @@ def modal_class
     end
   end
 
+  def focused_field
+    params[:field]
+  end
+
   def find_work_package
     @work_package = WorkPackage.visible.find(params[:work_package_id])
   end

app/forms/work_packages/pre_14_4_progress_form.rb

@@ -92,11 +92,11 @@ def initialize(work_package:,
         group.hidden(name: :status_id_touched,
                      value: @touched_field_map["status_id_touched"] || false,
                      data: { "work-packages--progress--preview-target": "touchedFieldInput",
-                             "referrer-field": "work_package[status_id]" })
+                             "referrer-field": "status_id" })
         group.hidden(name: :estimated_hours_touched,
                      value: @touched_field_map["estimated_hours_touched"] || false,
                      data: { "work-packages--progress--preview-target": "touchedFieldInput",
-                             "referrer-field": "work_package[estimated_hours]" })
+                             "referrer-field": "estimated_hours" })
       else
         render_text_field(group, name: :estimated_hours, label: I18n.t(:label_work))
         render_text_field(group, name: :remaining_hours, label: I18n.t(:label_remaining_work),
@@ -106,11 +106,11 @@ def initialize(work_package:,
         group.hidden(name: :estimated_hours_touched,
                      value: @touched_field_map["estimated_hours_touched"] || false,
                      data: { "work-packages--progress--preview-target": "touchedFieldInput",
-                             "referrer-field": "work_package[estimated_hours]" })
+                             "referrer-field": "estimated_hours" })
         group.hidden(name: :remaining_hours_touched,
                      value: @touched_field_map["remaining_hours_touched"] || false,
                      data: { "work-packages--progress--preview-target": "touchedFieldInput",
-                             "referrer-field": "work_package[remaining_hours]" })
+                             "referrer-field": "remaining_hours" })
       end
       group.fields_for(:initial) do |builder|
         ::WorkPackages::ProgressForm::InitialValuesForm.new(builder, work_package:, mode:)

app/forms/work_packages/progress_form.rb

@@ -164,7 +164,7 @@ def hidden_touched_field(group, name:)
     group.hidden(name: :"#{name}_touched",
                  value: touched(name),
                  data: { "work-packages--progress--preview-target": "touchedFieldInput",
-                         "referrer-field": "work_package[#{name}]" })
+                         "referrer-field": name })
   end
 
   def touched(name)

app/forms/work_packages/progress_form/initial_values_form.rb

@@ -58,7 +58,7 @@ def hidden_initial_field(form, name:)
       form.hidden(name:,
                   value: work_package.public_send(:"#{name}_was"),
                   data: { "work-packages--progress--preview-target": "initialValueInput",
-                          "referrer-field": "work_package[#{name}]" })
+                          "referrer-field": name })
     end
   end
 end

frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts

@@ -260,6 +260,7 @@ export class ProgressPopoverEditFieldComponent extends ProgressEditFieldComponen
 
   public onModalClosed():void {
     this.opened = false;
+    this.frameSrc = '';
 
     if (!this.handler.inEditMode) {
       this.handler.deactivate(false);

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

@@ -52,8 +52,17 @@ export default class PreviewController extends Controller {
   private debouncedPreview:DebouncedFunc<(event:Event) => void>;
   private frameMorphRenderer:(event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => void;
   private targetFieldName:string;
+  private touchedFields:Set<string>;
 
   connect() {
+    this.touchedFields = new Set();
+    this.touchedFieldInputTargets.forEach((input) => {
+      const fieldName = input.dataset.referrerField;
+      if (fieldName && input.value === 'true') {
+        this.touchedFields.add(fieldName);
+      }
+    });
+
     this.debouncedPreview = debounce((event:Event) => { void this.preview(event); }, 100);
     // TODO: Ideally morphing in this single controller should not be necessary.
     // Turbo supports morphing, by adding the <turbo-frame refresh="morph"> attribute.
@@ -116,22 +125,10 @@ export default class PreviewController extends Controller {
     const form = this.formTarget;
     const formData = new FormData(form) as unknown as undefined;
     const formParams = new URLSearchParams(formData);
-    const wpParams = [
-      ['work_package[initial][estimated_hours]', formParams.get('work_package[initial][estimated_hours]') || ''],
-      ['work_package[initial][remaining_hours]', formParams.get('work_package[initial][remaining_hours]') || ''],
-      ['work_package[initial][done_ratio]', formParams.get('work_package[initial][done_ratio]') || ''],
-      ['work_package[estimated_hours]', formParams.get('work_package[estimated_hours]') || ''],
-      ['work_package[remaining_hours]', formParams.get('work_package[remaining_hours]') || ''],
-      ['work_package[done_ratio]', formParams.get('work_package[done_ratio]') || ''],
-      ['work_package[status_id]', formParams.get('work_package[status_id]') || ''],
-      ['field', field?.name ?? ''],
-    ];
-
-    this.progressInputTargets.forEach((progressInput) => {
-      const touchedInputName = progressInput.name.replace(']', '_touched]');
-      const touchedValue = formParams.get(touchedInputName) || '';
-      wpParams.push([touchedInputName, touchedValue]);
-    });
+
+    const wpParams = Array.from(formParams.entries())
+      .filter(([key, _]) => key.startsWith('work_package'));
+    wpParams.push(['field', field?.name ?? '']);
 
     const wpPath = this.ensureValidPathname(form.action);
     const wpAction = wpPath.endsWith('/work_packages/new/progress') ? 'new' : 'edit';
@@ -227,16 +224,7 @@ export default class PreviewController extends Controller {
   // before being set by the user or derived.
   private findInitialValueInput(fieldName:string):HTMLInputElement|undefined {
     return this.initialValueInputTargets.find((input) =>
-      (input.dataset.referrerField === fieldName) || (input.dataset.referrerField === `work_package[${fieldName}]`));
-  }
-
-  // Finds the touched field input based on a field name.
-  //
-  // The touched input field is used to mark a field as touched by the user so
-  // that the backend keeps the value instead of deriving it.
-  private findTouchedInput(fieldName:string):HTMLInputElement|undefined {
-    return this.touchedFieldInputTargets.find((input) =>
-      (input.dataset.referrerField === fieldName) || (input.dataset.referrerField === `work_package[${fieldName}]`));
+      (input.dataset.referrerField === fieldName));
   }
 
   // Finds the value field input based on a field name.
@@ -252,8 +240,7 @@ export default class PreviewController extends Controller {
   }
 
   private isTouched(fieldName:string) {
-    const touchedInput = this.findTouchedInput(fieldName);
-    return touchedInput?.value === 'true';
+    return this.touchedFields.has(fieldName);
   }
 
   private isInitialValueEmpty(fieldName:string) {
@@ -272,16 +259,21 @@ export default class PreviewController extends Controller {
   }
 
   private markTouched(fieldName:string) {
-    const touchedInput = this.findTouchedInput(fieldName);
-    if (touchedInput) {
-      touchedInput.value = 'true';
-    }
+    this.touchedFields.add(fieldName);
+    this.updateTouchedFieldHiddenInputs();
   }
 
   private markUntouched(fieldName:string) {
-    const touchedInput = this.findTouchedInput(fieldName);
-    if (touchedInput) {
-      touchedInput.value = 'false';
-    }
+    this.touchedFields.delete(fieldName);
+    this.updateTouchedFieldHiddenInputs();
+  }
+
+  private updateTouchedFieldHiddenInputs() {
+    this.touchedFieldInputTargets.forEach((input) => {
+      const fieldName = input.dataset.referrerField;
+      if (fieldName) {
+        input.value = this.isTouched(fieldName) ? 'true' : 'false';
+      }
+    });
   }
 }

spec/features/work_packages/progress_modal_spec.rb

@@ -223,9 +223,23 @@ def visit_progress_query_displaying_work_package
 
         it "can create work package after setting work" do
           work_package_create_page.visit!
+          work_package_create_page.expect_fully_loaded
+
+          progress_popover.open
+          progress_popover.set_values(work: "invalid")
+          progress_popover.expect_values(remaining_work: "")
+          progress_popover.expect_errors(work: "Is not a valid duration.")
+
+          # The modal does not go away when clicking Save until all fields are valid
+          3.times do
+            progress_popover.save
+            sleep 0.2
+            progress_popover.expect_errors(work: "Is not a valid duration.")
+          end
+          progress_popover.set_values(work: "10h")
+          progress_popover.save
 
           work_package_create_page.set_attributes({ subject: "hello" })
-          work_package_create_page.set_progress_attributes({ estimatedTime: "10h" })
           work_package_create_page.save!
           work_package_table.expect_and_dismiss_toaster(message: "Successful creation.", wait: 5)
 
@@ -405,6 +419,28 @@ def visit_progress_query_displaying_work_package
         end
       end
 
+      context "with invalid values" do
+        before do
+          update_work_package_with(work_package, estimated_hours: nil, remaining_hours: nil, done_ratio: nil)
+        end
+
+        it "does not close the modal when clicking save multiple times (Bug #57423)" do
+          visit_progress_query_displaying_work_package
+
+          progress_popover.open
+          progress_popover.set_values(work: "invalid")
+          progress_popover.expect_values(remaining_work: "")
+          progress_popover.expect_errors(work: "Is not a valid duration.")
+
+          # The modal does not go away when clicking save
+          3.times do
+            progress_popover.save
+            sleep 0.2
+            progress_popover.expect_errors(work: "Is not a valid duration.")
+          end
+        end
+      end
+
       describe "status field", with_settings: { work_package_done_ratio: "status" } do
         it "renders the status options as the << status_name (percent_complete_value %) >>" do
           visit_progress_query_displaying_work_package

spec/support/components/work_packages/progress_popover.rb

@@ -156,6 +156,18 @@ def expect_hints(**field_hint_pairs)
         end
       end
 
+      def expect_error(field_name, error_message)
+        field(field_name).expect_error(error_message)
+      end
+
+      def expect_errors(**field_error_pairs)
+        aggregate_failures("progress popover errors expectations") do
+          field_error_pairs.each do |field_name, error_message|
+            expect_error(field_name, error_message)
+          end
+        end
+      end
+
       private
 
       def field(field_name)

spec/support/edit_fields/progress_edit_field.rb

@@ -129,10 +129,11 @@ def input_element
   end
 
   def input_caption_element
-    input_element["aria-describedby"]
-      .split
-      .find { _1.start_with?("caption-") }
-      &.then { |caption_id| find(id: caption_id) }
+    input_aria_related_element(describedby: "caption")
+  end
+
+  def input_validation_element
+    input_aria_related_element(describedby: "validation")
   end
 
   def trigger_element
@@ -239,6 +240,15 @@ def expect_caption(expected_caption)
     end
   end
 
+  def expect_error(expected_error)
+    if expected_error.nil?
+      expect(input_validation_element).to be_nil, "Expected no error message for #{@human_field_name} field, " \
+                                                  "got \"#{input_validation_element&.text}\""
+    else
+      expect(input_validation_element).to have_text(expected_error)
+    end
+  end
+
   def expect_select_field_with_options(*expected_options)
     within modal_element do
       expect(page).to have_select(field_name, with_options: expected_options)
@@ -273,4 +283,11 @@ def expect_migration_warning_banner(should_render: true)
   def modal_element
     page.find(MODAL_SELECTOR)
   end
+
+  def input_aria_related_element(describedby:)
+    input_element["aria-describedby"]
+      .split
+      .find { _1.start_with?("#{describedby}-") }
+      &.then { |id| find(id:) }
+  end
 end