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