From e3a24d53ea080f54aa91986497a3615c2e680380 Mon Sep 17 00:00:00 2001 From: mfo Date: Wed, 5 Jun 2024 18:00:19 +0200 Subject: [PATCH] tech(refactor): procedure::error_summary and dossier::ErrorsFullMessagesComponent use same behaviour to compact/expand errors --- .../errors_full_messages_component.rb | 8 ++-- .../errors_full_messages_component.en.yml | 1 - .../errors_full_messages_component.fr.yml | 1 - .../errors_full_messages_component.html.haml | 17 ++----- app/components/expandable_error_list.rb | 9 ++++ .../expandable_error_list.html.en.yml | 3 ++ .../expandable_error_list.html.fr.yml | 3 ++ .../expandable_error_list.html.haml | 14 ++++++ app/components/procedure/errors_summary.rb | 18 +++++-- .../errors_summary/errors_summary.html.haml | 6 +-- app/views/shared/dossiers/_edit.html.haml | 2 +- config/locales/models/procedure/en.yml | 16 +++---- config/locales/models/procedure/fr.yml | 16 +++---- .../procedures/errors_summary_spec.rb | 47 ++++++++++++------- spec/models/procedure_spec.rb | 26 +++++----- .../administrateurs/procedure_publish_spec.rb | 6 +-- 16 files changed, 115 insertions(+), 78 deletions(-) create mode 100644 app/components/expandable_error_list.rb create mode 100644 app/components/expandable_error_list/expandable_error_list.html.en.yml create mode 100644 app/components/expandable_error_list/expandable_error_list.html.fr.yml create mode 100644 app/components/expandable_error_list/expandable_error_list.html.haml diff --git a/app/components/dossiers/errors_full_messages_component.rb b/app/components/dossiers/errors_full_messages_component.rb index fd8bafd94..207170e8c 100644 --- a/app/components/dossiers/errors_full_messages_component.rb +++ b/app/components/dossiers/errors_full_messages_component.rb @@ -3,17 +3,15 @@ class Dossiers::ErrorsFullMessagesComponent < ApplicationComponent ErrorDescriptor = Data.define(:anchor, :label, :error_message) - def initialize(dossier:, errors:) + def initialize(dossier:) @dossier = dossier - @errors = errors end def dedup_and_partitioned_errors - formated_errors = @errors.to_enum # ActiveModel::Errors.to_a is an alias to full_messages, we don't want that + @dossier.errors.to_enum # ActiveModel::Errors.to_a is an alias to full_messages, we don't want that .to_a # but enum.to_a gives back an array .uniq { |error| [error.inner_error.base] } # dedup cumulated errors from dossier.champs, dossier.champs_public, dossier.champs_private which run the validator one time per association .map { |error| to_error_descriptor(error) } - yield(Array(formated_errors[0..2]), Array(formated_errors[3..])) end def to_error_descriptor(error) @@ -27,6 +25,6 @@ class Dossiers::ErrorsFullMessagesComponent < ApplicationComponent end def render? - !@errors.empty? + !@dossier.errors.empty? end end diff --git a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.en.yml b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.en.yml index 3fab8164d..0a595e80a 100644 --- a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.en.yml +++ b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.en.yml @@ -5,4 +5,3 @@ en: Your file has 1 error. Fix-it to continue : other: | Your file has %{count} errors. Fix-them to continue : - see_more: Show all errors diff --git a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.fr.yml b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.fr.yml index 1fd0e7f8c..3d94f636f 100644 --- a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.fr.yml +++ b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.fr.yml @@ -5,4 +5,3 @@ fr: Votre dossier contient 1 champ en erreur. Corrigez-la pour poursuivre : other: | Votre dossier contient %{count} champs en erreurs. Corrigez-les pour poursuivre : - see_more: Afficher toutes les erreurs diff --git a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.html.haml b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.html.haml index 58d76cb56..ada4150b5 100644 --- a/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.html.haml +++ b/app/components/dossiers/errors_full_messages_component/errors_full_messages_component.html.haml @@ -1,15 +1,4 @@ .fr-alert.fr-alert--error.fr-mb-3w{ role: "alertdialog" } - - dedup_and_partitioned_errors do |head, tail| - %p#sumup-errors= t('.sumup_html', count: head.size + tail.size, url: head.first.anchor) - %ul.fr-mb-0#head-errors - - head.each do |error_descriptor| - %li - = link_to error_descriptor.label, error_descriptor.anchor, class: 'error-anchor' - = error_descriptor.error_message - - if tail.size > 0 - %button{ type: "button", "aria-controls": 'tail-errors', "aria-expanded": "false", class: "fr-btn fr-btn--sm fr-btn--tertiary-no-outline" }= t('.see_more') - %ul#tail-errors.fr-collapse.fr-mt-0 - - tail.each do |error_descriptor| - %li - = link_to error_descriptor.label, error_descriptor.anchor, class: 'error-anchor' - = "(#{error_descriptor.error_message})" + - if dedup_and_partitioned_errors.size > 0 + %p#sumup-errors= t('.sumup_html', count: dedup_and_partitioned_errors.size, url: dedup_and_partitioned_errors.first.anchor) + = render ExpandableErrorList.new(errors: dedup_and_partitioned_errors) diff --git a/app/components/expandable_error_list.rb b/app/components/expandable_error_list.rb new file mode 100644 index 000000000..43d5c9215 --- /dev/null +++ b/app/components/expandable_error_list.rb @@ -0,0 +1,9 @@ +class ExpandableErrorList < ApplicationComponent + def initialize(errors:) + @errors = errors + end + + def splitted_errors + yield(Array(@errors[0..2]), Array(@errors[3..])) + end +end diff --git a/app/components/expandable_error_list/expandable_error_list.html.en.yml b/app/components/expandable_error_list/expandable_error_list.html.en.yml new file mode 100644 index 000000000..b21ee7d8a --- /dev/null +++ b/app/components/expandable_error_list/expandable_error_list.html.en.yml @@ -0,0 +1,3 @@ +--- +en: + see_more: Show all errors diff --git a/app/components/expandable_error_list/expandable_error_list.html.fr.yml b/app/components/expandable_error_list/expandable_error_list.html.fr.yml new file mode 100644 index 000000000..755d13886 --- /dev/null +++ b/app/components/expandable_error_list/expandable_error_list.html.fr.yml @@ -0,0 +1,3 @@ +--- +fr: + see_more: Afficher toutes les erreurs diff --git a/app/components/expandable_error_list/expandable_error_list.html.haml b/app/components/expandable_error_list/expandable_error_list.html.haml new file mode 100644 index 000000000..1ab5221e5 --- /dev/null +++ b/app/components/expandable_error_list/expandable_error_list.html.haml @@ -0,0 +1,14 @@ +- splitted_errors do |head, tail| + %ul#head-errors.fr-mb-0 + - head.each do |error_descriptor| + %li + = link_to error_descriptor.label, error_descriptor.anchor, class: 'error-anchor' + = error_descriptor.error_message + + - if tail.size > 0 + %button.fr-mt-0.fr-btn.fr-btn--sm.fr-btn--tertiary-no-outline{ type: "button", "aria-controls": 'tail-errors', "aria-expanded": "false", class: "" }= t('see_more') + %ul#tail-errors.fr-collapse.fr-mt-0 + - tail.each do |error_descriptor| + %li + = link_to error_descriptor.label, error_descriptor.anchor, class: 'error-anchor' + = error_descriptor.error_message diff --git a/app/components/procedure/errors_summary.rb b/app/components/procedure/errors_summary.rb index adce8fd64..e185c81b3 100644 --- a/app/components/procedure/errors_summary.rb +++ b/app/components/procedure/errors_summary.rb @@ -1,4 +1,6 @@ class Procedure::ErrorsSummary < ApplicationComponent + ErrorDescriptor = Data.define(:anchor, :label, :error_message) + def initialize(procedure:, validation_context:) @procedure = procedure @validation_context = validation_context @@ -24,10 +26,8 @@ class Procedure::ErrorsSummary < ApplicationComponent @procedure.errors.present? end - def error_messages - @procedure.errors.map do |error| - [error, error_correction_page(error)] - end + def errors + @procedure.errors.map { to_error_descriptor(_1) } end def error_correction_page(error) @@ -45,4 +45,14 @@ class Procedure::ErrorsSummary < ApplicationComponent edit_admin_procedure_mail_template_path(@procedure, klass.const_get(:SLUG)) end end + + def to_error_descriptor(error) + libelle = case error.attribute + when :draft_types_de_champ_public, :draft_types_de_champ_private + error.options[:type_de_champ].libelle.truncate(200) + else + error.base.class.human_attribute_name(error.attribute) + end + ErrorDescriptor.new(error_correction_page(error), libelle, error.message) + end end diff --git a/app/components/procedure/errors_summary/errors_summary.html.haml b/app/components/procedure/errors_summary/errors_summary.html.haml index 72780bcd1..e5042916e 100644 --- a/app/components/procedure/errors_summary/errors_summary.html.haml +++ b/app/components/procedure/errors_summary/errors_summary.html.haml @@ -2,8 +2,4 @@ - if invalid? = render Dsfr::AlertComponent.new(state: :error, title: , extra_class_names: 'fr-mb-2w') do |c| - c.with_body do - - error_messages.each do |(error, path)| - %p.mt-2 - = error.full_message - - if path.present? - = "(#{link_to 'corriger', path, class: 'fr-link'})" + = render ExpandableErrorList.new(errors:) diff --git a/app/views/shared/dossiers/_edit.html.haml b/app/views/shared/dossiers/_edit.html.haml index 1962951e8..c797d35f2 100644 --- a/app/views/shared/dossiers/_edit.html.haml +++ b/app/views/shared/dossiers/_edit.html.haml @@ -10,7 +10,7 @@ = render NestedForms::FormOwnerComponent.new = form_for dossier_for_editing, url: brouillon_dossier_url(dossier), method: :patch, html: { id: 'dossier-edit-form', class: 'form', multipart: true, novalidate: 'novalidate' } do |f| - = render Dossiers::ErrorsFullMessagesComponent.new(dossier: @dossier, errors: @errors || []) + = render Dossiers::ErrorsFullMessagesComponent.new(dossier: dossier) %header.mb-6 .fr-highlight %p.fr-text--sm diff --git a/config/locales/models/procedure/en.yml b/config/locales/models/procedure/en.yml index 55a9c1ddd..34fc89e35 100644 --- a/config/locales/models/procedure/en.yml +++ b/config/locales/models/procedure/en.yml @@ -72,16 +72,16 @@ en: invalid: 'invalid format' draft_types_de_champ_public: format: 'Public field %{message}' - invalid_condition: "« %{value} » have an invalid logic" - empty_repetition: '« %{value} » requires at least one field' - empty_drop_down: '« %{value} » requires at least one option' - inconsistent_header_section: "« %{value} » %{custom_message}" + invalid_condition: "have an invalid logic" + empty_repetition: 'requires at least one field' + empty_drop_down: 'requires at least one option' + inconsistent_header_section: "%{custom_message}" draft_types_de_champ_private: format: 'Private field %{message}' - invalid_condition: "« %{value} » have an invalid logic" - empty_repetition: '« %{value} » requires at least one field' - empty_drop_down: '« %{value} » requires at least one option' - inconsistent_header_section: "« %{value} » %{custom_message}" + invalid_condition: "have an invalid logic" + empty_repetition: 'requires at least one field' + empty_drop_down: 'requires at least one option' + inconsistent_header_section: "%{custom_message}" attestation_template: format: "%{attribute} %{message}" initiated_mail: diff --git a/config/locales/models/procedure/fr.yml b/config/locales/models/procedure/fr.yml index 4a0fdeca5..85df92d73 100644 --- a/config/locales/models/procedure/fr.yml +++ b/config/locales/models/procedure/fr.yml @@ -78,16 +78,16 @@ fr: invalid: 'n’a pas le bon format' draft_types_de_champ_public: format: 'Le champ %{message}' - invalid_condition: "« %{value} » a une logique conditionnelle invalide" - empty_repetition: '« %{value} » doit comporter au moins un champ répétable' - empty_drop_down: '« %{value} » doit comporter au moins un choix sélectionnable' - inconsistent_header_section: "« %{value} » %{custom_message}" + invalid_condition: "a une logique conditionnelle invalide" + empty_repetition: 'doit comporter au moins un champ répétable' + empty_drop_down: 'doit comporter au moins un choix sélectionnable' + inconsistent_header_section: "%{custom_message}" draft_types_de_champ_private: format: 'L’annotation privée %{message}' - invalid_condition: "« %{value} » a une logique conditionnelle invalide" - empty_repetition: '« %{value} » doit comporter au moins un champ répétable' - empty_drop_down: '« %{value} » doit comporter au moins un choix sélectionnable' - inconsistent_header_section: "« %{value} » %{custom_message}" + invalid_condition: "a une logique conditionnelle invalide" + empty_repetition: 'doit comporter au moins un champ répétable' + empty_drop_down: 'doit comporter au moins un choix sélectionnable' + inconsistent_header_section: "%{custom_message}" attestation_template: format: "%{attribute} %{message}" initiated_mail: diff --git a/spec/components/procedures/errors_summary_spec.rb b/spec/components/procedures/errors_summary_spec.rb index 4c3ef6337..3e64cc790 100644 --- a/spec/components/procedures/errors_summary_spec.rb +++ b/spec/components/procedures/errors_summary_spec.rb @@ -11,27 +11,33 @@ describe Procedure::ErrorsSummary, type: :component do context 'when :publication' do let(:validation_context) { :publication } - it 'shows errors for public and private tdc' do - expect(page).to have_text("Le champ « public » doit comporter au moins un choix sélectionnable") - expect(page).to have_text("L’annotation privée « private » doit comporter au moins un choix sélectionnable") + it 'shows errors and links for public and private tdc' do + expect(page).to have_content("Erreur : Des problèmes empêchent la publication de la démarche") + expect(page).to have_selector("a", text: "public") + expect(page).to have_selector("a", text: "private") + expect(page).to have_text("doit comporter au moins un choix sélectionnable", count: 2) end end context 'when :types_de_champ_public_editor' do let(:validation_context) { :types_de_champ_public_editor } - it 'shows errors for public only tdc' do - expect(page).to have_text("Le champ « public » doit comporter au moins un choix sélectionnable") - expect(page).not_to have_text("L’annotation privée « private » doit comporter au moins un choix sélectionnable") + it 'shows errors and links for public only tdc' do + expect(page).to have_text("Erreur : Les champs formulaire contiennent des erreurs") + expect(page).to have_selector("a", text: "public") + expect(page).to have_text("doit comporter au moins un choix sélectionnable", count: 1) + expect(page).not_to have_selector("a", text: "private") end end context 'when :types_de_champ_private_editor' do let(:validation_context) { :types_de_champ_private_editor } - it 'shows errors for private only tdc' do - expect(page).not_to have_text("Le champ « public » doit comporter au moins un choix sélectionnable") - expect(page).to have_text("L’annotation privée « private » doit comporter au moins un choix sélectionnable") + it 'shows errors and links for private only tdc' do + expect(page).to have_text("Erreur : Les annotations privées contiennent des erreurs") + expect(page).to have_selector("a", text: "private") + expect(page).to have_text("doit comporter au moins un choix sélectionnable") + expect(page).not_to have_selector("a", text: "public") end end end @@ -52,12 +58,18 @@ describe Procedure::ErrorsSummary, type: :component do before { subject } - it 'renders all errors on champ' do - expect(page).to have_text("Le champ « drop down list requires options » doit comporter au moins un choix sélectionnable") - expect(page).to have_text("Le champ « repetition requires children » doit comporter au moins un champ répétable") - expect(page).to have_text("Le champ « invalid condition » a une logique conditionnelle invalide") - expect(page).to have_text("Le champ « header sections must have consistent order » devrait être précédé d'un titre de niveau 1") - # TODO, test attestation_template, initiated_mail, :received_mail, :closed_mail, :refused_mail, :without_continuation_mail, :re_instructed_mail + it 'renders all errors and links on champ' do + expect(page).to have_selector("a", text: "drop down list requires options") + expect(page).to have_content("doit comporter au moins un choix sélectionnable") + + expect(page).to have_selector("a", text: "repetition requires children") + expect(page).to have_content("doit comporter au moins un champ répétable") + + expect(page).to have_selector("a", text: "invalid condition") + expect(page).to have_content("a une logique conditionnelle invalide") + + expect(page).to have_selector("a", text: "header sections must have consistent order") + expect(page).to have_content("devrait être précédé d'un titre de niveau 1") end end @@ -73,8 +85,9 @@ describe Procedure::ErrorsSummary, type: :component do end it 'render error nicely' do - expect(page).to have_text("Le modèle d’attestation n'est pas valide") - expect(page).to have_text("L’email de notification de passage de dossier en instruction n'est pas valide") + expect(page).to have_selector("a", text: "Le modèle d’attestation") + expect(page).to have_selector("a", text: "L’email de notification de passage de dossier en instruction") + expect(page).to have_text("n'est pas valide", count: 2) end end end diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 2a20829ab..0fa94a425 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -372,12 +372,12 @@ describe Procedure do ] end let(:types_de_champ_private) { [] } - let(:invalid_repetition_error_message) { 'Le champ « Enfants » doit comporter au moins un champ répétable' } - let(:invalid_drop_down_error_message) { 'Le champ « Civilité » doit comporter au moins un choix sélectionnable' } + let(:invalid_repetition_error_message) { "doit comporter au moins un champ répétable" } + let(:invalid_drop_down_error_message) { "doit comporter au moins un choix sélectionnable" } it 'validates that no repetition type de champ is empty' do procedure.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_public)).to include(invalid_repetition_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_public)).to include(invalid_repetition_error_message) new_draft = procedure.draft_revision repetition = procedure.draft_revision.types_de_champ_public.find(&:repetition?) @@ -385,17 +385,17 @@ describe Procedure do new_draft.revision_types_de_champ.create(type_de_champ: create(:type_de_champ), position: 0, parent: parent_coordinate) procedure.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_public)).not_to include(invalid_repetition_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_public)).not_to include(invalid_repetition_error_message) end it 'validates that no drop-down type de champ is empty' do procedure.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_public)).to include(invalid_drop_down_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_public)).to include(invalid_drop_down_error_message) drop_down = procedure.draft_revision.types_de_champ_public.find(&:drop_down_list?) drop_down.update!(drop_down_list_value: "--title--\r\nsome value") procedure.reload.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_public)).not_to include(invalid_drop_down_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_public)).not_to include(invalid_drop_down_error_message) end end @@ -408,17 +408,21 @@ describe Procedure do end let(:types_de_champ_public) { [] } - let(:invalid_repetition_error_message) { 'L’annotation privée « Enfants » doit comporter au moins un champ répétable' } - let(:invalid_drop_down_error_message) { 'L’annotation privée « Civilité » doit comporter au moins un choix sélectionnable' } + let(:invalid_repetition_error_message) { "doit comporter au moins un champ répétable" } + let(:invalid_drop_down_error_message) { "doit comporter au moins un choix sélectionnable" } it 'validates that no repetition type de champ is empty' do procedure.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_private)).to include(invalid_repetition_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_private)).to include(invalid_repetition_error_message) + repetition = procedure.draft_revision.types_de_champ_private.find(&:repetition?) + expect(procedure.errors.to_enum.to_a.map { _1.options[:type_de_champ] }).to include(repetition) end it 'validates that no drop-down type de champ is empty' do procedure.validate(:publication) - expect(procedure.errors.full_messages_for(:draft_types_de_champ_private)).to include(invalid_drop_down_error_message) + expect(procedure.errors.messages_for(:draft_types_de_champ_private)).to include(invalid_drop_down_error_message) + drop_down = procedure.draft_revision.types_de_champ_private.find(&:drop_down_list?) + expect(procedure.errors.to_enum.to_a.map { _1.options[:type_de_champ] }).to include(drop_down) end end @@ -441,7 +445,7 @@ describe Procedure do include Logic let(:types_de_champ_public) { [{ type: :text, libelle: 'condition', condition: ds_eq(champ_value(1), constant(2)), stable_id: 2 }] } let(:types_de_champ_private) { [{ type: :decimal_number, stable_id: 1 }] } - let(:error_on_condition) { "Le champ « condition » a une logique conditionnelle invalide" } + let(:error_on_condition) { "Le champ a une logique conditionnelle invalide" } it 'validate without context' do procedure.validate diff --git a/spec/system/administrateurs/procedure_publish_spec.rb b/spec/system/administrateurs/procedure_publish_spec.rb index 10818971c..dc1858358 100644 --- a/spec/system/administrateurs/procedure_publish_spec.rb +++ b/spec/system/administrateurs/procedure_publish_spec.rb @@ -72,8 +72,8 @@ describe 'Publishing a procedure', js: true do visit admin_procedure_path(procedure) expect(page).to have_content('Des problèmes empêchent la publication de la démarche') - expect(page).to have_content("« Enfants » doit comporter au moins un champ répétable") - expect(page).to have_content("« Civilité » doit comporter au moins un choix sélectionnable") + expect(page).to have_content("Enfants doit comporter au moins un champ répétable") + expect(page).to have_content("Civilité doit comporter au moins un choix sélectionnable") visit admin_procedure_publication_path(procedure) expect(find_field('procedure_path').value).to eq procedure.path @@ -195,7 +195,7 @@ describe 'Publishing a procedure', js: true do scenario 'an error message prevents the publication' do visit admin_procedure_path(procedure) expect(page).to have_content('Des problèmes empêchent la publication des modifications') - expect(page).to have_link('corriger', href: edit_admin_procedure_mail_template_path(procedure, Mails::InitiatedMail::SLUG)) + expect(page).to have_link(href: edit_admin_procedure_mail_template_path(procedure, Mails::InitiatedMail::SLUG)) expect(page).to have_button('Publier les modifications', disabled: true) end end