From 0a5765f21a8e6f15cd6c38767b28d1c2ed2e2446 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 | 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) => 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..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