Refacto de la délégation des erreurs dans le UserRdvWizard

[adipasquale]

Note

Accrochez-vous avant de lire cette PR de deux lignes mais compliquée :)
Ou alors lisez la en diagonale et faites moi confiance 😇

Contexte

Je m’intéresse aux validations des numéros de pré-demande ANTS cf #4777

Avant de modifier la manière dont sont faites les validations des pré–demandes ANTS je me suis rendu compte que le UserRdvWizard est un objet assez étrange et un peu cassé.

Problèmes & Solutions

Step2 et Step3

UserRdvWizard::Base délègue les erreurs à un des deux objets proxied : le Rdv.
Pourtant UserRdvWizard::Step2 et UserRdvWizard::Step3 ne font aucune validation et leur #save renvoie toujours true.
Ce ne sont pas de « vrais » form objects.

J’ai choisi de ne rien changer dans ce refacto mais peut-être qu’on pourrait envisager de sortir Step2 et Step3 et de bien séparer de Base ce qui relève du Form Model et ce qui relève d’un objet utilitaire pour les vues de ce wizard.

Step1

UserRdvWizard::Step1 lui est bien un form object mais un peu étonnant.
Il ne définit pas d’attr_accessor pour chaque attribut du formulaire.
Il définit un attr_accessor pour l’objet proxied (ici un User).

La vue affiche un form_for(user) et pas un form_for(user_rdv_wizard).
Il faut donc que les erreurs soient définies sur le user pour être affichées correctement.

Pourtant les erreurs sont délèguées au rdv par Base !
Cette étape n’affiche pourtant aucun attribut lié au RDV.
C’est probablement un héritage du passé où cette étape contenait des champs du RDV et du user.

Validation numéro de tel côté usagers

La validation de présence du numéro de téléphone est un peu cassée : elle ajoute ses erreurs au rdv via la méthode déléguée Base#errors.
Il y a une validation frontend faite par le navigateur via l’attribut required donc en réalité il faut « hacker » pour envoyer un numéro de téléphone vide.
Si on contourne ce required, la validation est bloquée et l’usager ne peut pas passer à l’étape suivante.
Mais aucune erreur n’est affichée car il n’y a aucune erreur sur user.

J’ai d’abord mis en place une nouvelle feature spec qui échoue pour vérifier que mes modifications corrigent ce souci et empêcher une régression future.
Pour corriger, j’ai supprimé Base#errors et implémenté Step1#errors qui délègue maintenant à user.
J’ai du déplacer au passage la clé de traduction du rdv au user.
Cette clé de traduction concerne « autant » le rdv que le user puisqu’elle apparait quand un numéro de tel manque au user pour un rdv téléphonique.
La « bonne » place pour cette clé de traduction serait probablement dans un form object si on affichait le formulaire du form object.

Validation numéro ANTS

La validation du numéro de demande ANTS quant à elle ajoute les erreurs directement sur le user passé en param.
Elles sont donc bien affichées.

Mais il y a aussi besoin d’une ligne errors.merge!(@user) qui rappatrie ces erreurs sur le rdv pour que la validation échoue bien 🤯
Si on retire cette ligne :

• la validation du rdv passe, donc la ligne suivante passe valid? && @user.update(@user_attributes).
• Le user est bien updaté car il n’y a pas de validation au niveau du modèle AR lui même.
• L’usager passe à l’étape suivante

Ma correction ici aussi c’est le fait d’avoir supprimé Base#errors et implémenté Step1#errors qui délègue maintenant à user.
On peut alors supprimer sereinement la ligne errors.merge!(@user) 😅

Validation numéro de téléphone côté agents

Au moment de finir cette PR, je me suis rendu compte que mon déplacement de la clé de traduction i18n de l’erreur missing_for_phone_motif du modèle rdv vers user avait cassé l’affichage d’une erreur similaire côté agent.
J’ai commencé par rajouter une spec pour m’assurer que c’était testé car ce n’était pas le cas jusqu’ici, je n’avais reçu aucune alerte que j’avais cassé cette vérification.

Côté agents on fait une vérification légèrement différente : il faut qu’au moins un usager ait un numéro de téléphone.
Le message affiché est pertinent « Aucun usager n’a de numéro de téléphone renseigné alors que le rendez-vous est téléphonique ».

Le problème est qu’on utilisait aussi ce message côté usagers or c’est vraiment peu clair.
Je l’ai remplacé par « Le numéro de téléphone est obligatoire car le RDV aura lieu par téléphone ».

J’ai fait un choix discutable dans ce changement qui est d’inliner le message côté agents plutôt que de passer par les clés de traduction i18n.
Je pense que cela enlève une indirection inutile ici.

J’aurais bien aimé faire de même côté usagers et ne pas passer par i18n mais je ne sais pas comment appeler errors.add en lui passant un message inliné plutôt qu’une clé ET en spécifiant que le message d’erreur ne doit pas afficher le nom de l’attribut.

J’aimerais faire errors.add(:phone_number, "Le numéro est obligatoire...", format: "%{message}") mais ça ne fonctionne pas. J’ai regardé le code de ActiveModel mais je n’ai pas trouvé de façon simple de faire ça 🤷

[francois-ferrandis]

Merci beaucoup d'avoir démêlé tout ça en écrit ce rapport, ça pourra nous faire un checkpoint lors de futurs refactos de cette partie du code qui n'est pas la plus belle ! 👌

[francois-ferrandis]

Peut-être pas la peine de vérifier ce status, qui d'ailleurs pourrait être une 422.

[francois-ferrandis]

En effet, dans la grande majorité des cas, ce message plus simple est pertinent. Si vraiment on veut être dans le détail on peut compter le nombre de participants, mais c'est du bonus.

[adipasquale]

en fait il y a une différence entre ce qu’on exige côté usagers et côté admin :

• côté usagers : le user « principal », celui qui prend RDV doit avoir un numéro
• côté admin : au moins un usager parmi le principal et les proches doit avoir un numéro