From 284f559f2d4abf3c004dddeae3b6e7308a2bc549 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 17 Sep 2024 12:06:01 +0200 Subject: [PATCH] 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 | 26 +++++--- .../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 | 22 +++++++ .../work_packages/progress_popover.rb | 12 ++++ .../edit_fields/progress_edit_field.rb | 25 ++++++-- 11 files changed, 107 insertions(+), 60 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..638c7cc5f775 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 @@ -95,7 +89,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 +100,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 +110,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 +122,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) => void; private targetFieldName:string; + private touchedFields:Set; 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 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..56443c43b7e2 100644 --- a/spec/features/work_packages/progress_modal_spec.rb +++ b/spec/features/work_packages/progress_modal_spec.rb @@ -405,6 +405,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.5 + 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