betagouv / rdv-service-public

Prise de RDV pour les services publics
https://rdv.anct.gouv.fr
GNU Affero General Public License v3.0
14 stars 2 forks source link

RDV Collectif annulé par un usager #3227

Closed victormours closed 1 year ago

victormours commented 1 year ago

Bug remonté par une cnfs : Capture d’écran 2023-01-02 à 18 04 37

Pour reproduire le bug :

Historique

La mise à jour des rdv à partir des participations a été introduit dans #2898, puis corrigé dans #3106, puis étendu au statut annulé par l'usager dans #3116, c'est sans doute à ce moment que le bug a été introduit.

Je n'ai pas retrouvé toutes les infos sur l'ajout de cette fonctionnalité, mais j'ai l'impression que c'était pensé comme un nice-to-have en plus de la fonctionnalité "le changement de statut du rdv change le statut des participations".

Est-ce que c'est vraiment une bonne idée de garder cette fonctionnalité ? On peut aussi imaginer que ce bug ai lieu quand un agent annule la participation d'un usager, et ne se rend pas compte qu'il annule tout le rdv.

Correctif possible

Technique

On remarque aussi que le bug est dû à l'usage d'un callback after_touch. Quand on a besoin de logique métier, le choix entre "appeler explicitement la méthode" et "ajouter un callback implicite" est aussi le choix entre deux catégories de bugs "des actions métiers qui n'auront pas lieu alors qu'elles devraient" ou "des actions métiers qui auront lieu alors qu'elles ne devraient pas". Là on a une action destructive (l'annulation d'un rdv), donc ça pourrait être moins risqué de préférer un appel explicite de méthode. Cependant, si on enlève ce comportement pour les participations excusées, alors cette approche en callback est moins risquée.

victormours commented 1 year ago

vu avec Rémi : on ne modifie le statut du rdv que si tous les participants sont notés comme présents. Les autres modifications n'ont pas vraiment de sens.

Holist commented 1 year ago

Hello Victor, merci beaucoup pour ton analyse. Je confirme que le bug a bien été introduit ici dans #3116 lors d'une refacto de la méthode update_status_priority_order_participations et ce n'est pas le comportement attendu évidemment. En attendant que je rentre de congés mardi prochain pour me poser sur ces questions (utiliser un appel explicite à la méthode plutôt qu'un callback.... J'avais réfléchi à cela et le callback m'avait paru plus logique mais on peut se poser pour regarder cela ensemble si tu veux) on peut appliquer le fix que tu as proposé dans la PR.