Bugfix: le RDV doit afficher l'adresse du responsable, pas du proche

[francois-ferrandis]

En faisant du fuzzing dans #4809, une spec est passée au rouge :

rdv-service-public/spec/models/rdv_spec.rb

Line 188 in fb68d72

context "when rdv is at home" do

L'intention de cette spec est de vérifier que lorsqu'on a un RDV avec un proche et un responsable, c'est l'adresse du responsable que l'on veut afficher. Or, dans le code, c'est l'adresse du proche qui est affichée !

Pourquoi cette spec était au vert ? Plusieurs raisons :

• le RDV testé n'était lié qu'au proche et pas au responsable
• l'adresse générée dans la factory :user est toujours la même, donc dans la spec les deux usagers ont la même adresse, donc it { is_expected.to eq responsible.address } passe mais ça ne nous apprend rien
• mais surtout, le code implémente l'inverse de la spec (code original dans [Agent][tunnel de prise de RDV] rendre le remplissage du "lieu" obligatoire #714) :

responsibles = users.where.not(responsible_id: [nil])

Ce code récupère les users qui ont un responsible_id, donc les proches !

Comportement attendu

À mon avis, le comportement attendu est :

• si seul le responsable a une adresse : on l'affiche
• si seul le proche a une adresse, on l'affiche
• si les deux ont une adresse, on affiche celle du responsable

Je ne suis vraiment pas sûr de mon coup niveau métier donc je vais demander l'avis du public.

Info importante : on est dans le troisième cas pour environ 200 usagers, et dans beaucoup de cas cas les adresses ont juste une virgule de différence.

Il est important de noter que jusqu'ici, c'est donc l'adresse du proche qui est affichée, et que ce fix consiste donc en un changement de comportement métier.

[victormours]

C'est entièrement possible que la description de la spec soit fausse, et que l'implémentation actuelle soit satisfaisante pour le métier.

Je pense que ça vaudrait le coup de clarifier où on indique l'adresse du proche et celle du responsable dans l'interface (et lecture et écriture), et qu'on soit capable de dire si ça a du sens.

[adipasquale]

🙇 merci de prendre le temps de te pencher là dessus, et bravo pour la détection de cette spec via le fuzzing 👏

Je rejoins Victor qu’une bonne piste d’amélioration serait d’expliciter de quel usager on affiche l’adresse dans les cas ambigus. Je vois que c’est explicité dans tous les cas dans l’étape 3 du admin rdv wizard : https://github.com/betagouv/rdv-service-public/blob/production/app/views/admin/rdv_wizard_steps/step3.html.slim#L35. On pourrait éventuellement rajouter la précision directement dans les méthodes address, address_complete, address_without_personal_information.

Néanmoins j’approuve cette PR car je pense que c’est déjà un beau pas en avant :

• on corrige le code qui fait l’inverse de ce qu’il prétend
• on corrige les specs
• le code est amélioré en rapprochant user_for_home_rdv du AddressConcern.

Je pense que le changement de comportement est tout à fait acceptable vu le peu de nombre de cas et que tu as l’air de dire que c’est souvent insignifiant.

[adipasquale]

élégant 👏 j’avais un doute que ça soit une légère régression de perfs de ne plus faire de distinction sur le loaded? mais après réfléxion j’imagine que rails ne va pas recharger les users si tu appelles partition dessus 🤔

[francois-ferrandis]

On aurait pu continuer de faire la distinction sur le loaded?, mais fait le pari que si l'association n'est pas chargée, ça ne coûte pas beaucoup plus cher de charger tous les users, puisque le nombre d'usager souvent 1, parfois 2-3 (c'est pour les RDVs individuels).

[francois-ferrandis]

Ah, et aussi :

• on peut faire le pari que tous les #users seront utilisés ailleurs aussi
• c'est facile de preloader l'association #users, d'ailleurs on le fait dans Admin::RdvsController
• le code est plus simple ici si on ne fait pas cette tentative d'optimisation (et on peut utiliser ce magnifique partition que j'aime)

[francois-ferrandis]

C'est entièrement possible que la description de la spec soit fausse, et que l'implémentation actuelle soit satisfaisante pour le métier.

Je pense surtout que c'est tellement marginal que je doute qu'un⋅e seul⋅e agent sache quelle adresse est censée s'afficher.

Je pense que ça vaudrait le coup de clarifier où on indique l'adresse du proche et celle du responsable dans l'interface (et lecture et écriture), et qu'on soit capable de dire si ça a du sens.

En effet ça vaudrait le coup, mais ça demande du temps supplémentaire à implémenter, et je pense qu'on peut merger cette PR et garder l'amélioration pour plus tard.

🙇 merci de prendre le temps de te pencher là dessus, et bravo pour la détection de cette spec via le fuzzing 👏

Je rejoins Victor qu’une bonne piste d’amélioration serait d’expliciter de quel usager on affiche l’adresse dans les cas ambigus. Je vois que c’est explicité dans tous les cas dans l’étape 3 du admin rdv wizard : https://github.com/betagouv/rdv-service-public/blob/production/app/views/admin/rdv_wizard_steps/step3.html.slim#L35. On pourrait éventuellement rajouter la précision directement dans les méthodes address, address_complete, address_without_personal_information.

Néanmoins j’approuve cette PR car je pense que c’est déjà un beau pas en avant :

* on corrige le code qui fait l’inverse de ce qu’il prétend

* on corrige les specs

* le code est amélioré en rapprochant `user_for_home_rdv` du `AddressConcern`.

Je pense que le changement de comportement est tout à fait acceptable vu le peu de nombre de cas et que tu as l’air de dire que c’est souvent insignifiant.

Merci beaucoup, je suis 100 % aligné. 🚀

[francois-ferrandis]

J'ai créé une issue pour aller plus loin et clarifier l'interface : #4850