Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refonte de la validation des numéros de pré-demande ANTS #4770

Merged
merged 4 commits into from
Dec 9, 2024

Conversation

adipasquale
Copy link
Contributor

@adipasquale adipasquale commented Nov 3, 2024

Note

PR à lire commit par commit

Contexte

Je m’intéresse aux problèmes de codes ANTS invalides qu’on stocke en base : issue #4777 . J’envisage une solution pour corriger ce problème, cette PR est une (seconde) étape préliminaire avant le correctif.

Problèmes que cette PR attaque

  1. Il n’y a pas beaucoup de tests des validations du service
  2. Le service de validation suprenant et peu agréable à réutiliser ou modifier

Il y a d’autres problèmes (cf plus bas) mais ils seront attaqués dans des PRs successives

Solution

1er commit: specs

Je rajoute des specs plus ou moins exhaustives aux trois form models appelant le service de validation des numéros de pré-demande ANTS : UserRdvWizard::Step1, Admin::UserForm et BeneficiaireForm

2e commit : refacto

Je transforme le service de validation en un validateur custom ActiveModel.
Cela me semble un peu plus facilement lisible, découvrable et réutilisable.

Détails

Je rajoute des specs pour ces différentes validations actuellement effectuées sur les numéros ANTS par le service :

  • validation du format en 10 caractères
  • validation du status validated
  • validation de l’absence d’appointments
  • ou alors passage en force avec le BenignErrors

Problèmes à corriger dans des PRs ultérieures

  • incohérences et absence de tests pour les numéros ANTS des proches (commencé dans [WIP] introduction form object pour les proches + ajout de la validation #4824)
  • incohérence du hint qui « recommande fortement » alors que c’est obligatoire
  • incohérences sur la partie agents : on refait les tests et les validations à chaque sauvegarde du user, ce qui peut causer des problèmes (imaginons qu’on édite un usager 2 mois après son RDV ANTS).

@@ -5,17 +5,9 @@ class Admin::UserForm

validate :validate_duplicates

delegate :ignore_benign_errors, :ignore_benign_errors=, :add_benign_error, :benign_errors, :not_benign_errors, :errors_are_all_benign?, to: :user
delegate :ants_pre_demande_number, :ignore_benign_errors, :ignore_benign_errors=, :add_benign_error, :benign_errors, :not_benign_errors, :errors_are_all_benign?, to: :user
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nécessaire pour le validateur AntsPreDemandeNumberValidation

Comment on lines -102 to +93
@user_attributes = @attributes[:user]&.with_indifferent_access
@user&.assign_attributes(@attributes.fetch(:user, {}))
Copy link
Contributor Author

@adipasquale adipasquale Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on fait ce changement pour :

  • ne plus avoir besoin d’utiliser la variable d’instance @user_attributes dans le validateur de numéro de téléphone l.88
  • pouvoir utiliser AntsPreDemandeNumberValidation sur le user directement

on se rapproche peu à peu d’un form object plus classique.

Comme indiqué plus bas, pour continuer de nettoyer ce form object, il faudrait utiliser form_for(rdv_wizard) dans la vue plutôt que form_for(user).

# définies sur le user et on ne peut pas simplement appeler `validates_with AntsPreDemandeNumberValidation`
# dans ce form model
if rdv.requires_ants_predemande_number?
@user.singleton_class.validates_with(AntsPreDemandeNumberValidation)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cette ligne avec singleton_class est un peu impressionnante, mais ça me paraît en fait assez adapté dans l’état de ce form model actuel. Vu qu’on utilise form_for(user) ça semble « logique » de rajouter des validations sur le user. C’est en fait très similaire à ce qu’on fait déjà à savoir avoir le validateur dans ce form object mais ajouter les erreurs sur le user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

malheureusement GitHub n’affiche pas le diff unifié avec le service supprimé mais il y a très peu de changements à part qu’on appelle record plutôt que user

@adipasquale adipasquale force-pushed the refactor/validate-ants-numbers branch from cc02afd to 94aec7d Compare November 20, 2024 09:26
@adipasquale adipasquale changed the title [WIP] Améliorer la validation des numéros de pré-demande ANTS Refacto de la validation des numéros de pré-demande ANTS Nov 20, 2024
@adipasquale adipasquale changed the title Refacto de la validation des numéros de pré-demande ANTS Refactor: Validation des numéros de pré-demande ANTS Nov 20, 2024
@adipasquale adipasquale changed the title Refactor: Validation des numéros de pré-demande ANTS Refonte de la validation des numéros de pré-demande ANTS Nov 20, 2024
@adipasquale adipasquale marked this pull request as ready for review November 20, 2024 10:06
Copy link
Member

@AntoineGirard AntoineGirard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça me semble très bien merci 🙏🏻

J’apprécie beaucoup le fait qu’on ait beaucoup plus de tests sur ce type de validateurs.
Après, je me demande à quel point on devait répéter certaines choses au niveau des specs.

Et je préfère largement cette approche sous forme de validateur plutôt que sous forme de service.

@adipasquale adipasquale enabled auto-merge (squash) December 9, 2024 07:39
@adipasquale adipasquale merged commit 420d3b2 into production Dec 9, 2024
15 checks passed
@adipasquale adipasquale deleted the refactor/validate-ants-numbers branch December 9, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants