From bf8adf77b969b2e772aac0d75d7779c2e2649de2 Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Mon, 16 Sep 2024 15:11:48 +0200
Subject: [PATCH 1/5] Correctly remove event listeners

---
 .../progress/preview-progress.controller.ts           | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
index bf722fa38d63..c54f8a79e275 100644
--- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
+++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
@@ -74,12 +74,19 @@ export default class PreviewProgressController extends Controller {
   }
 
   disconnect() {
+    this.debouncedPreview.cancel();
     this.progressInputTargets.forEach((target) => {
-      target.removeEventListener('input', this.debouncedPreview);
+      if (target.tagName.toLowerCase() === 'select') {
+        target.removeEventListener('change', this.debouncedPreview);
+      } else {
+        target.removeEventListener('input', this.debouncedPreview);
+      }
       target.removeEventListener('blur', this.debouncedPreview);
     });
     const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
-    turboFrame.removeEventListener('turbo:before-frame-render', this.frameMorphRenderer);
+    if (turboFrame) {
+      turboFrame.removeEventListener('turbo:before-frame-render', this.frameMorphRenderer);
+    }
   }
 
   async preview(event:Event) {

From 5579eb4ef9944f92df5d963ef0d6f9b256768e8d Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Mon, 16 Sep 2024 15:19:54 +0200
Subject: [PATCH 2/5] Refactor: merge 2 stimulus controllers together

Merge `progress-preview` and `touched-field-marker` stimulus controllers
together into `preview` controller.

They are sharing some state, the touched status, showing that they need
to be grouped together. It will make it easier to deal with the touched
state later.
---
 .../modal_body_component.html.erb             |   5 +-
 .../work_based/modal_body_component.html.erb  |   5 +-
 .../work_packages/pre_14_4_progress_form.rb   |  13 +-
 app/forms/work_packages/progress_form.rb      |   7 +-
 .../progress_form/initial_values_form.rb      |   2 +-
 .../progress/preview-progress.controller.ts   | 144 ------------------
 ...er.controller.ts => preview.controller.ts} | 113 +++++++++++++-
 7 files changed, 123 insertions(+), 166 deletions(-)
 delete mode 100644 frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
 rename frontend/src/stimulus/controllers/dynamic/work-packages/progress/{touched-field-marker.controller.ts => preview.controller.ts} (56%)

diff --git a/app/components/work_packages/progress/status_based/modal_body_component.html.erb b/app/components/work_packages/progress/status_based/modal_body_component.html.erb
index d1a61701022f..d23796492650 100644
--- a/app/components/work_packages/progress/status_based/modal_body_component.html.erb
+++ b/app/components/work_packages/progress/status_based/modal_body_component.html.erb
@@ -6,10 +6,9 @@
         id: "progress-form",
         html: { autocomplete: "off" },
         data: { "application-target": "dynamic",
-                "work-packages--progress--preview-progress-target": "form",
+                "work-packages--progress--preview-target": "form",
                 controller: "work-packages--progress--focus-field " \
-                  "work-packages--progress--preview-progress " \
-                  "work-packages--progress--touched-field-marker" }
+                            "work-packages--progress--preview" }
       ) do |f| %>
     <%= flex_layout do |modal_body| %>
       <% modal_body.with_row(classes: "FormControl-horizontalGroup--sm-vertical") do |_fields| %>
diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb
index d29236da0e9e..7f93df22f6ea 100644
--- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb
+++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb
@@ -6,10 +6,9 @@
         id: "progress-form",
         html: { autocomplete: "off" },
         data: { "application-target": "dynamic",
-                "work-packages--progress--preview-progress-target": "form",
+                "work-packages--progress--preview-target": "form",
                 controller: "work-packages--progress--focus-field " \
-                  "work-packages--progress--preview-progress " \
-                  "work-packages--progress--touched-field-marker" }
+                          "work-packages--progress--preview" }
       ) do |f| %>
     <%= flex_layout do |modal_body| %>
       <% modal_body.with_row(classes: "FormControl-horizontalGroup--sm-vertical") do |_fields| %>
diff --git a/app/forms/work_packages/pre_14_4_progress_form.rb b/app/forms/work_packages/pre_14_4_progress_form.rb
index 2127f3fdb343..6dcb461a318b 100644
--- a/app/forms/work_packages/pre_14_4_progress_form.rb
+++ b/app/forms/work_packages/pre_14_4_progress_form.rb
@@ -91,11 +91,11 @@ def initialize(work_package:,
 
         group.hidden(name: :status_id_touched,
                      value: @touched_field_map["status_id_touched"] || false,
-                     data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
                              "referrer-field": "work_package[status_id]" })
         group.hidden(name: :estimated_hours_touched,
                      value: @touched_field_map["estimated_hours_touched"] || false,
-                     data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
                              "referrer-field": "work_package[estimated_hours]" })
       else
         render_text_field(group, name: :estimated_hours, label: I18n.t(:label_work))
@@ -105,11 +105,11 @@ def initialize(work_package:,
 
         group.hidden(name: :estimated_hours_touched,
                      value: @touched_field_map["estimated_hours_touched"] || false,
-                     data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
                              "referrer-field": "work_package[estimated_hours]" })
         group.hidden(name: :remaining_hours_touched,
                      value: @touched_field_map["remaining_hours_touched"] || false,
-                     data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
                              "referrer-field": "work_package[remaining_hours]" })
       end
       group.fields_for(:initial) do |builder|
@@ -195,9 +195,8 @@ def format_to_smallest_fractional_part(number)
   end
 
   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" }
+    data = { "work-packages--progress--preview-target": "progressInput",
+             action: "input->work-packages--progress--preview#markFieldAsTouched" }
 
     if @focused_field == name
       data[:"work-packages--progress--focus-field-target"] = "fieldToFocus"
diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb
index e61176221d27..23b122c6c423 100644
--- a/app/forms/work_packages/progress_form.rb
+++ b/app/forms/work_packages/progress_form.rb
@@ -163,7 +163,7 @@ def readonly_text_field(group,
   def hidden_touched_field(group, name:)
     group.hidden(name: :"#{name}_touched",
                  value: touched(name),
-                 data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
+                 data: { "work-packages--progress--preview-target": "touchedFieldInput",
                          "referrer-field": "work_package[#{name}]" })
   end
 
@@ -197,9 +197,8 @@ def as_percent(value)
   end
 
   def default_field_options(name)
-    data = { "work-packages--progress--preview-progress-target": "progressInput",
-             "work-packages--progress--touched-field-marker-target": "progressInput",
-             action: "work-packages--progress--touched-field-marker#markFieldAsTouched" }
+    data = { "work-packages--progress--preview-target": "progressInput",
+             action: "work-packages--progress--preview#markFieldAsTouched" }
 
     if @focused_field == name
       data[:"work-packages--progress--focus-field-target"] = "fieldToFocus"
diff --git a/app/forms/work_packages/progress_form/initial_values_form.rb b/app/forms/work_packages/progress_form/initial_values_form.rb
index 6ffe9618b536..8d8f3e10e329 100644
--- a/app/forms/work_packages/progress_form/initial_values_form.rb
+++ b/app/forms/work_packages/progress_form/initial_values_form.rb
@@ -57,7 +57,7 @@ def initialize(work_package:,
     def hidden_initial_field(form, name:)
       form.hidden(name:,
                   value: work_package.public_send(:"#{name}_was"),
-                  data: { "work-packages--progress--touched-field-marker-target": "initialValueInput",
+                  data: { "work-packages--progress--preview-target": "initialValueInput",
                           "referrer-field": "work_package[#{name}]" })
     end
   end
diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
deleted file mode 100644
index c54f8a79e275..000000000000
--- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts
+++ /dev/null
@@ -1,144 +0,0 @@
-/*
- * -- 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.
- * ++
- */
-
-import { Controller } from '@hotwired/stimulus';
-import { debounce, DebouncedFunc } from 'lodash';
-import Idiomorph from 'idiomorph/dist/idiomorph.cjs';
-
-interface TurboBeforeFrameRenderEventDetail {
-  render:(currentElement:HTMLElement, newElement:HTMLElement) => void;
-}
-
-export default class PreviewProgressController extends Controller {
-  static targets = [
-    'form', 'progressInput',
-  ];
-
-  declare readonly progressInputTargets:HTMLInputElement[];
-  declare readonly formTarget:HTMLFormElement;
-
-  private debouncedPreview:DebouncedFunc<(event:Event) => void>;
-  private frameMorphRenderer:(event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => void;
-
-  connect() {
-    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.
-    // However, it has a bug, and it doesn't morphs when reloading the frame via javascript.
-    // See https://github.com/hotwired/turbo/issues/1161 . Once the issue is solved, we can remove
-    // this code and just use <turbo-frame refresh="morph"> instead.
-    this.frameMorphRenderer = (event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => {
-      event.detail.render = (currentElement:HTMLElement, newElement:HTMLElement) => {
-        Idiomorph.morph(currentElement, newElement, { ignoreActiveValue: true });
-      };
-    };
-
-    this.progressInputTargets.forEach((target) => {
-      if (target.tagName.toLowerCase() === 'select') {
-        target.addEventListener('change', this.debouncedPreview);
-      } else {
-        target.addEventListener('input', this.debouncedPreview);
-      }
-      target.addEventListener('blur', this.debouncedPreview);
-    });
-
-    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
-    turboFrame.addEventListener('turbo:before-frame-render', this.frameMorphRenderer);
-  }
-
-  disconnect() {
-    this.debouncedPreview.cancel();
-    this.progressInputTargets.forEach((target) => {
-      if (target.tagName.toLowerCase() === 'select') {
-        target.removeEventListener('change', this.debouncedPreview);
-      } else {
-        target.removeEventListener('input', this.debouncedPreview);
-      }
-      target.removeEventListener('blur', this.debouncedPreview);
-    });
-    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
-    if (turboFrame) {
-      turboFrame.removeEventListener('turbo:before-frame-render', this.frameMorphRenderer);
-    }
-  }
-
-  async preview(event:Event) {
-    let field:HTMLInputElement;
-    if (event.type === 'blur') {
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-      field = (event as FocusEvent).relatedTarget as HTMLInputElement;
-    } else {
-      field = event.target as HTMLInputElement;
-    }
-
-    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 wpPath = this.ensureValidPathname(form.action);
-    const wpAction = wpPath.endsWith('/work_packages/new/progress') ? 'new' : 'edit';
-
-    const editUrl = `${wpPath}/${wpAction}?${new URLSearchParams(wpParams).toString()}`;
-    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
-
-    if (turboFrame) {
-      turboFrame.src = editUrl;
-    }
-  }
-
-  // Ensures that on create forms, there is an "id" for the un-persisted
-  // work package when sending requests to the edit action for previews.
-  private ensureValidPathname(formAction:string):string {
-    const wpPath = new URL(formAction);
-
-    if (wpPath.pathname.endsWith('/work_packages/progress')) {
-      // Replace /work_packages/progress with /work_packages/new/progress
-      wpPath.pathname = wpPath.pathname.replace('/work_packages/progress', '/work_packages/new/progress');
-    }
-
-    return wpPath.toString();
-  }
-}
diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
similarity index 56%
rename from frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts
rename to frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
index 7c96bcd31913..7337025c8f32 100644
--- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts
+++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
@@ -29,21 +29,73 @@
  */
 
 import { Controller } from '@hotwired/stimulus';
+import { debounce, DebouncedFunc } from 'lodash';
+import Idiomorph from 'idiomorph/dist/idiomorph.cjs';
 
-export default class TouchedFieldMarkerController extends Controller {
+interface TurboBeforeFrameRenderEventDetail {
+  render:(currentElement:HTMLElement, newElement:HTMLElement) => void;
+}
+
+export default class PreviewController extends Controller {
   static targets = [
+    'form',
+    'progressInput',
     'initialValueInput',
     'touchedFieldInput',
-    'progressInput',
   ];
 
+  declare readonly progressInputTargets:HTMLInputElement[];
+  declare readonly formTarget:HTMLFormElement;
   declare readonly initialValueInputTargets:HTMLInputElement[];
   declare readonly touchedFieldInputTargets:HTMLInputElement[];
-  declare readonly progressInputTargets:HTMLInputElement[];
 
+  private debouncedPreview:DebouncedFunc<(event:Event) => void>;
+  private frameMorphRenderer:(event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => void;
   private targetFieldName:string;
 
-  private markFieldAsTouched(event:{ target:HTMLInputElement }) {
+  connect() {
+    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.
+    // However, it has a bug, and it doesn't morphs when reloading the frame via javascript.
+    // See https://github.com/hotwired/turbo/issues/1161 . Once the issue is solved, we can remove
+    // this code and just use <turbo-frame refresh="morph"> instead.
+    this.frameMorphRenderer = (event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => {
+      event.detail.render = (currentElement:HTMLElement, newElement:HTMLElement) => {
+        Idiomorph.morph(currentElement, newElement, { ignoreActiveValue: true });
+      };
+    };
+
+    this.progressInputTargets.forEach((target) => {
+      if (target.tagName.toLowerCase() === 'select') {
+        target.addEventListener('change', this.debouncedPreview);
+      } else {
+        target.addEventListener('input', this.debouncedPreview);
+      }
+      target.addEventListener('blur', this.debouncedPreview);
+    });
+
+    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
+    turboFrame.addEventListener('turbo:before-frame-render', this.frameMorphRenderer);
+  }
+
+  disconnect() {
+    this.debouncedPreview.cancel();
+    this.progressInputTargets.forEach((target) => {
+      if (target.tagName.toLowerCase() === 'select') {
+        target.removeEventListener('change', this.debouncedPreview);
+      } else {
+        target.removeEventListener('input', this.debouncedPreview);
+      }
+      target.removeEventListener('blur', this.debouncedPreview);
+    });
+    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
+    if (turboFrame) {
+      turboFrame.removeEventListener('turbo:before-frame-render', this.frameMorphRenderer);
+    }
+  }
+
+  markFieldAsTouched(event:{ target:HTMLInputElement }) {
     this.targetFieldName = event.target.name.replace(/^work_package\[([^\]]+)\]$/, '$1');
     this.markTouched(this.targetFieldName);
 
@@ -52,6 +104,59 @@ export default class TouchedFieldMarkerController extends Controller {
     }
   }
 
+  async preview(event:Event) {
+    let field:HTMLInputElement;
+    if (event.type === 'blur') {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+      field = (event as FocusEvent).relatedTarget as HTMLInputElement;
+    } else {
+      field = event.target as HTMLInputElement;
+    }
+
+    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 wpPath = this.ensureValidPathname(form.action);
+    const wpAction = wpPath.endsWith('/work_packages/new/progress') ? 'new' : 'edit';
+
+    const editUrl = `${wpPath}/${wpAction}?${new URLSearchParams(wpParams).toString()}`;
+    const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement;
+
+    if (turboFrame) {
+      turboFrame.src = editUrl;
+    }
+  }
+
+  // Ensures that on create forms, there is an "id" for the un-persisted
+  // work package when sending requests to the edit action for previews.
+  private ensureValidPathname(formAction:string):string {
+    const wpPath = new URL(formAction);
+
+    if (wpPath.pathname.endsWith('/work_packages/progress')) {
+      // Replace /work_packages/progress with /work_packages/new/progress
+      wpPath.pathname = wpPath.pathname.replace('/work_packages/progress', '/work_packages/new/progress');
+    }
+
+    return wpPath.toString();
+  }
+
   private isWorkBasedMode() {
     return this.findValueInput('done_ratio') !== undefined;
   }

From c40bd5fdc248b8ceccf35b9f689f871de333ec23 Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Tue, 17 Sep 2024 09:06:11 +0200
Subject: [PATCH 3/5] Recompute progress values when changing status on new
 work package form

Because of the weird way we initialize the fake work package, it could
believe that the status did not change and no recomputation was needed.
This is an edge case, and I'm not satisfied with its design.
---
 .../derive_progress_values_status_based.rb    |  3 ++-
 .../work_packages/progress_modal_spec.rb      | 19 ++++++++++++++++
 ...erive_progress_values_status_based_spec.rb | 22 +++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb b/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb
index 7f140a9caf1a..00ea713c06af 100644
--- a/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb
+++ b/app/services/work_packages/set_attributes_service/derive_progress_values_status_based.rb
@@ -54,7 +54,8 @@ def derive_remaining_work?
     end
 
     def status_percent_complete_changed?
-      work_package.status_id_changed? && work_package.status.default_done_ratio != work_package.done_ratio_was
+      work_package.status_id.present? && work_package.status_id_came_from_user? \
+        && work_package.status.default_done_ratio != work_package.done_ratio_was
     end
 
     # Update +% complete+ from the status if the status changed.
diff --git a/spec/features/work_packages/progress_modal_spec.rb b/spec/features/work_packages/progress_modal_spec.rb
index 21d7ea323905..c4d76b53c481 100644
--- a/spec/features/work_packages/progress_modal_spec.rb
+++ b/spec/features/work_packages/progress_modal_spec.rb
@@ -265,6 +265,25 @@ def visit_progress_query_displaying_work_package
           work_package_create_page.save!
           work_package_table.expect_and_dismiss_toaster(message: "Successful creation.")
         end
+
+        it "updates remaining work when status is changed" do
+          work_package_create_page.visit!
+          work_package_create_page.set_attributes({ subject: "hello" })
+
+          progress_popover.open
+          progress_popover.expect_hints(work: nil, remaining_work: nil)
+          progress_popover.set_values(work: "14h")
+          progress_popover.expect_values(remaining_work: "14h")
+          progress_popover.expect_hints(remaining_work: :derived)
+          progress_popover.save
+
+          status_field = work_package_create_page.edit_field(:status)
+          status_field.update("in progress")
+
+          progress_popover.open
+          progress_popover.expect_values(work: "14h", remaining_work: "7h")
+          progress_popover.expect_hints(remaining_work: :derived)
+        end
       end
     end
   end
diff --git a/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb b/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb
index 5cc0bd8a0722..e2587b2816b1 100644
--- a/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb
+++ b/spec/services/work_packages/set_attributes_service/derive_progress_values_status_based_spec.rb
@@ -138,6 +138,28 @@
     end
   end
 
+  context "given a work package with status and % complete not being in sync" do
+    before do
+      work_package.status = status_50_pct_complete
+      work_package.done_ratio = 0
+      work_package.estimated_hours = 10.0
+      work_package.remaining_hours = 10.0
+      work_package.clear_changes_information
+    end
+
+    context "when status is set again to the same value" do
+      let(:set_attributes) { { status: status_50_pct_complete } }
+      let(:expected_derived_attributes) { { remaining_hours: 5.0, done_ratio: 50 } }
+      let(:expected_kept_attributes) { %w[estimated_hours] }
+
+      include_examples "update progress values", description: "updates % complete value to the status default % complete value " \
+                                                              "and derives remaining work",
+                                                 expected_hints: {
+                                                   remaining_work: :derived
+                                                 }
+    end
+  end
+
   context "given a work package with work and remaining work being empty, and a status with 0% complete" do
     before do
       work_package.status = status_0_pct_complete

From 0a5765f21a8e6f15cd6c38767b28d1c2ed2e2446 Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Tue, 17 Sep 2024 12:06:01 +0200
Subject: [PATCH 4/5] 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.
---
 .../work_based/modal_body_component.html.erb  |  2 +-
 .../work_packages/progress_controller.rb      | 30 +++++----
 .../work_packages/pre_14_4_progress_form.rb   |  8 +--
 app/forms/work_packages/progress_form.rb      |  2 +-
 .../progress_form/initial_values_form.rb      |  2 +-
 .../progress/update.turbo_stream.erb          |  3 -
 .../progress-popover-edit-field.component.ts  |  1 +
 .../progress/preview.controller.ts            | 64 ++++++++-----------
 .../work_packages/progress_modal_spec.rb      | 38 ++++++++++-
 .../work_packages/progress_popover.rb         | 12 ++++
 .../edit_fields/progress_edit_field.rb        | 25 ++++++--
 11 files changed, 125 insertions(+), 62 deletions(-)
 delete mode 100644 app/views/work_packages/progress/update.turbo_stream.erb

diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb
index 7f93df22f6ea..2726ce5db9ea 100644
--- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb
+++ b/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| %>
diff --git a/app/controllers/work_packages/progress_controller.rb b/app/controllers/work_packages/progress_controller.rb
index 3d1831c70f49..25e4c43fdce7 100644
--- a/app/controllers/work_packages/progress_controller.rb
+++ b/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,7 +102,9 @@ 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
@@ -112,6 +112,10 @@ def update
 
   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
diff --git a/app/forms/work_packages/pre_14_4_progress_form.rb b/app/forms/work_packages/pre_14_4_progress_form.rb
index 6dcb461a318b..f6b530f93dd7 100644
--- a/app/forms/work_packages/pre_14_4_progress_form.rb
+++ b/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:)
diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb
index 23b122c6c423..87542bb29ca2 100644
--- a/app/forms/work_packages/progress_form.rb
+++ b/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)
diff --git a/app/forms/work_packages/progress_form/initial_values_form.rb b/app/forms/work_packages/progress_form/initial_values_form.rb
index 8d8f3e10e329..91c10a2a5889 100644
--- a/app/forms/work_packages/progress_form/initial_values_form.rb
+++ b/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
diff --git a/app/views/work_packages/progress/update.turbo_stream.erb b/app/views/work_packages/progress/update.turbo_stream.erb
deleted file mode 100644
index eb5c77eb6d17..000000000000
--- a/app/views/work_packages/progress/update.turbo_stream.erb
+++ /dev/null
@@ -1,3 +0,0 @@
-<%= turbo_stream.update "work_package_progress_modal" do %>
-  <%= render modal_class.new(@work_package, focused_field: params[:field]) %>
-<% end %>
diff --git a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts b/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts
index d15042caf666..8abf179696ee 100644
--- a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts
+++ b/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);
diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
index 7337025c8f32..261bb72a3a74 100644
--- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
+++ b/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';
+      }
+    });
   }
 }
diff --git a/spec/features/work_packages/progress_modal_spec.rb b/spec/features/work_packages/progress_modal_spec.rb
index c4d76b53c481..8cfdd6499d67 100644
--- a/spec/features/work_packages/progress_modal_spec.rb
+++ b/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
diff --git a/spec/support/components/work_packages/progress_popover.rb b/spec/support/components/work_packages/progress_popover.rb
index 968018f26245..c1cbe42cfc77 100644
--- a/spec/support/components/work_packages/progress_popover.rb
+++ b/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)
diff --git a/spec/support/edit_fields/progress_edit_field.rb b/spec/support/edit_fields/progress_edit_field.rb
index 02b2862144fd..6b1693227a52 100644
--- a/spec/support/edit_fields/progress_edit_field.rb
+++ b/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

From b676e4b3c6e96b538a52811f796e9242550d2fe7 Mon Sep 17 00:00:00 2001
From: Christophe Bliard <c.bliard@openproject.com>
Date: Tue, 17 Sep 2024 14:21:30 +0200
Subject: [PATCH 5/5] Fix flakiness when checking progress field focus state

---
 spec/support/edit_fields/progress_edit_field.rb | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/spec/support/edit_fields/progress_edit_field.rb b/spec/support/edit_fields/progress_edit_field.rb
index 6b1693227a52..be72681ed16d 100644
--- a/spec/support/edit_fields/progress_edit_field.rb
+++ b/spec/support/edit_fields/progress_edit_field.rb
@@ -184,7 +184,12 @@ def display_selector
   # If they are the same, it means the modal field is in focus.
   # @return [Boolean] true if the modal field is in focus, false otherwise.
   def expect_modal_field_in_focus
-    expect(focused?).to be(true)
+    # Use capybara `synchronize` helper to wait until the modal field is in focus
+    page.document.synchronize do
+      unless focused?
+        raise Capybara::ExpectationNotMet, "Input #{input_element} does not have focus"
+      end
+    end
   end
 
   def focused?