Merge pull request #16735 from opf/bug/57423-prevent-popover-save-on-…

…invalid-progress-values [57423] Progress modal should always prevent save when there are invalid values
opf · Sep 18, 2024 · 2517cfe · 2517cfe
2 parents e8e6367 + b676e4b
commit 2517cfe
Show file tree

Hide file tree

Showing 15 changed files with 280 additions and 206 deletions.
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
@@ -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
@@ -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/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,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

diff --git a/app/forms/work_packages/pre_14_4_progress_form.rb b/app/forms/work_packages/pre_14_4_progress_form.rb
@@ -91,12 +91,12 @@ 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",
-                             "referrer-field": "work_package[status_id]" })
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
+                             "referrer-field": "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",
-                             "referrer-field": "work_package[estimated_hours]" })
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
+                             "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),
@@ -105,12 +105,12 @@ 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",
-                             "referrer-field": "work_package[estimated_hours]" })
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
+                             "referrer-field": "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",
-                             "referrer-field": "work_package[remaining_hours]" })
+                     data: { "work-packages--progress--preview-target": "touchedFieldInput",
+                             "referrer-field": "remaining_hours" })
       end
       group.fields_for(:initial) do |builder|
         ::WorkPackages::ProgressForm::InitialValuesForm.new(builder, work_package:, mode:)
@@ -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
@@ -163,8 +163,8 @@ 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",
-                         "referrer-field": "work_package[#{name}]" })
+                 data: { "work-packages--progress--preview-target": "touchedFieldInput",
+                         "referrer-field": name })
   end
 
   def touched(name)
@@ -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
@@ -57,8 +57,8 @@ 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",
-                          "referrer-field": "work_package[#{name}]" })
+                  data: { "work-packages--progress--preview-target": "initialValueInput",
+                          "referrer-field": name })
     end
   end
 end
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
@@ -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/app/views/work_packages/progress/update.turbo_stream.erb b/app/views/work_packages/progress/update.turbo_stream.erb
diff --git a/...rc/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts b/...rc/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/...nd/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/...nd/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts