-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor(upload): User list upload refactor #2559
Open
aminedhobb
wants to merge
22
commits into
staging
Choose a base branch
from
refactor/user-list-upload
base: staging
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #2447 , #2465 , #2468 , #2469 , #2470 , #2471 , #2472 , #2473
NOTE: pour une review technique vous pouvez passer directement à la partie Implémentation technique.
Contexte
La page permettant d'uploader une liste d'usager pour les créer et les inviter à prendre rdv est la première page qui a été mise en place sur l'application et il s'agit de la fonctionnalité principale de rdv-insertion.
En témoigne le premier commit sur ce repo où la seule page concernait cette fonctionnalité: 8894f93#diff-f39f53ad607869e40f515797c75db57d7c819396d221f274188db94bb780a061
Expérimentation Ardennes (Début 2021)
En effet l'application rdv-insertion a d'abord été créée suite à une expérimentation dans les Ardennes. On avait créé une page dans laquelle un agent des Ardennes pouvait uploader un fichier excel, pour pouvoir créer des comptes et générer des tokens d'invitations.
Il pouvait alors télécharger un fichier résultat où pour chaque ligne d'usager on avait inséré une url avec son token pour que l'usager crée son compte sur rdv-sp avec ses infos pré-remplies. L'usager devait alors choisir un mot de passe puis lui-même chercher un rdv en rentrant son adresse sur rdv-sp (bien loin du fonctionnement d'aujourd'hui ^^) !
Le fichier généré avec les liens d'invitations servait de base à un publipostage où on envoyait un courrier à l'usager pour qu'il crée son compte.
Le code de cette application est toujours disponible et faisait partie de l'outil d'analyse des flux insertion créé par Thomas Guillet sur ce repo: https://github.com/betagouv/analyse-flux-insertion (app Next.js)
Début rdv-insertion (juillet 2021)
La première page et feature de rdv-insertion était donc cette même page, avec plusieurs additions: possibilité d'afficher qu'un usager a déjà été créé et invité au ré-upload du fichier, et surtout possibilité d'envoyer un sms avec cette fois un lien de prise de rdv.
Cette page a bien évidemment beaucoup été étoffée au fil du temps mais reste la fonctionnalité principale de l'application et celle que l'on montre en premier en démo.
Besoin de changement
Du fait de l'ancienneté de cette feature, beaucoup de choses ont été rajoutée sur cette page au fil du temps: possibilité d'envoyer des mails, de générer des courriers, d'assigner un référent, de faire les actions en masse etc.
Problèmes d'UX
Ces features ont été ajoutées les unes après les autres sur ce qui a déjà été fait et a eu pour conséquence d'alourdir la lisibilité des actions possibles sur cette page.
Comme il s'agit de la fonctionnalité que l'on met le plus en avant sur l'app, il était nécessaire de faire une refonte complète pour une UX beaucoup plus agréable pour nos utilisateurs.
De plus cette fonctionnalité a été développée avant l'arrivée d'une UX designeuse dans notre équipe, il était donc important pour nous de la repenser avec notre experte design.
Problèmes techniques
Comme expliqué plus haut les fondements de cette page ont d'abord été développés en React au sein d'une appli Next.js.
On est donc parti de ce code pour lancer l'application rdv-insertion, en ajoutant une base de données pour sauvegarder le fait qu'une personne ait déjà été invitée.
Nous avons donc créé une appli Rails en gardant le code front de cette page en React et cela a plusieurs inconvénients:
Au vu de ce qu'il est possible de faire aujourd'hui en terme d'UX dynamique sans avoir à ajouter de librairie JS (via Hotwire pour rails), rien ne justifie de garder cette librairie.
Au contraire, le fait d'enlever la partie react et tout faire via Rails a plusieurs avantages:
Travail UX
Avant de passer à l'explication technique je tenais à saluer le gros travail de fond de notre designeuse Samantha qui a imaginé à quoi ressemblerait la version idéale de l'upload.
Cette refonte n'aurait jamais pu avoir le jour sans ce gros travail 🙏 !
Les maquettes sont disponibles ici et la feature a été développé dans l'idée d'être le plus fidèle possible à ces maquettes.
Vidéo démonstrative 🎥
Charger.un.fichier.usagers.-.CD.de.DIE.-.rdv-insertion.1.webm
Implémentation technique
Diagramme de séquence
Ci-dessous le diagramme de séquence qui résume tout le flow du début à la fin du nouveau parcours d'upload:
Tout le code au niveau des models, des jobs et des controllers sur cette partie a été namespacé dans des dossiers
user_list_upload
.Choix techniques
Architecture de données
J'introduis pour ce parcours trois nouvelles tables:
user_list_uploads
: c'est celle qui contient le nom du fichier uploadé, sa structure (= une organisation ou un département) ainsi que la catégorie sur laquelle on dépose le fichier (cette dernière est optionnelle)user_rows
: ce sont toutes les lignes du fichier. Chaque colonne correspond à une cellule du fichier. On y ajoute unmatching_user_id
, qui est l'id de l'usager matchant les données d'identification présentes sur cette ligne. Lorsqu'il n'est pas setté, lematching_user_id
est recherché et assigné dans unbefore_save
AR callback (voir concernUserListUpload::UserRow::MatchingUser
). Les booléensmarked_for_user_save
etmarked_for_invitation
servent à indiquer les lignes sélectionnées pour la création et l'invitation.user_save_attempts
: elle représente la tentative de sauvegarde d'un usager à partir d'uneuser_row
. Elle a un état desuccess
et est rattachée à l'usager sauvegardé lorsque ça a fonctionné. On sauvegarde aussi les messages d'erreur dans la colonneservice_errors
lorsque la tentative a échoué.invitation_attempts
: Analogues auxuser_save_attempts
, ces records contiennent les infos liées au succès ou non des envois d'invitations liés à uneuser_row
. On y a en plus une colonneformat
pour indiquer qu'il s'agit d'une invitation sms ou email.Précédente implémentation
Dans une première implémentation, je n'avais qu'une seule table
user_list_uploads
qui avait une colonneuser_list
. Cette colonne était un array de hash représentant exactement les mêmes données que celles présentes dans les tableuser_rows
,user_save_attempts
, etinvitation_attempts
.Des PORO remplaçaient les models AR qu'on a aujourd'hui et la logique était la même.
L'idée derrière ça était de limiter le nombres de requêtes SQL sur un même upload puisqu'on n'interagissait avec un seul record.
Seulement voilà, Le fait d'instancier tous ces PORO à chaque requête rendait le temps de réponse lent et l'expérience désagréable pour des gros fichiers (~+1s sur les pages chaque 100 lignes ajoutées). J'ai fait un peu d'optimisation qui a réduit le temps de réponse mais c'était toujours insuffisant. J'ai alors rapidement changé de cap et voir si stocker les infos dans des tables séparées améliorerait la performance.
En se faisant j'ai remarqué une nette amélioration des performances en local: le peu que je perdais en requêtes SQL supplémentaires je le gagnais en temps de rendu du html.
Mon intuition derrière ces améliorations est que Rails est conçu pour optimiser le rendu de grosses collections AR et que ces optimisations ne sont pas faites sur des simples classes ruby.
Cette intuition demanderait à être confortée par un benchmark précis pour être confirmée.
En testant en local l'expérience reste fluide pour des listes allant jusqu'à 500 usagers.
Classe
UserListUpload::Collection
J'ai créé une classe ruby spécifique qui s'instancie au niveau du model
user_List_upload
qui permet de gérer les différentesuser_rows
de l'upload.J'ai préféré isolé cette partie du reste du model pour plus de clarté.
Batch update
J'ai importé la gem
active-record-import
pour que les update de plusieurs lignes en même temps ne trigger qu'une seule requête SQL.Je ne l'utilise pas à la création des
user_rows
cependant, j'ai préféré passer par des nested attributes directement pour être sûr qu'on appelle tous les callbacks du model (non appelés via les imports).Interactions côté serveur
L'idée ici était d'avoir un parcours très fluide avec des pages très interactives pour l'utilisateur tout en gardant la logique métier côté serveur et refléter ces différents états avec du code ruby.
Ainsi pratiquement tous les endpoints renvoient du html et sont rendus de manière fluide par Turbo 8, qui remplace seulement les parties du DOM qui ont changé (via idiomorph).
Les différents états de changements et de statuts des lignes du fichier sont gérées au niveau du model
user_row
.Les helpers
user_list_upload_helper
,user_save_attempts_helper
etinvitation_attempts_helper
servent à régler l'affichage en fonction de ces états.Dans un souci d'optimisation, certaines réponses renvoient via des turbo streams du html qui va directement viser les éléments de la page où s'insérer. Ce sont les requêtes permettant d'afficher et de cacher le détails d'un usager (
/show_details
et/hide_details
) ainsi que les requêtes permettant d'afficher l'édition d'une cellule.Dans une première implémentation, ces petites interactions ne déclenchaient pas de requêtes et on avait du code js pour afficher ou non ces éléments. Seulement cela posait des problèmes de performances pour les grosses listes de centaines d'usagers car ça obligeait le serveur à répondre avec tout ce html supplémentaire à la première requête.
Certains endpoints renvoient tout de même du json: ceux permettant d'assigner une organisation lorsque celle-ci n'est pas retrouvée à l'upload du département.
Interactions côté client
La feature nécessite bien sûr aussi d'écrire pas mal de code côté pour le browser afin d'avoir une UX optimale.
On introduit donc plusieurs controller stimulus, les principaux étant:
user_list_upload_controller.js
: sert à parser le fichier des usagers que l'on upload, en extraire les informations qui nous intéressent, puis crée et soumet le formulaire destiné à appeler l'endpointPOST /user_list_uploads
enrich_with_cnaf_data_controller.js
: sert à parser le fichier le données de contact cnaf, voir s'il y a des usagers dans la l'upload qui match avec ces données de contact, puis crée et soumet le formulaire destiné à enrichir la liste avec ces donnéesPOST /user_list_uploads/:user_list_upload_id/enrich_with_cnaf_data
submit_selected_uids.js
: gère le fait de sélectionner les ids des lignes que l'on a coché avant d'appelerPOST user_list_uploads/:user_list_upload_id/user_save_attempts/create_many
ouPOST user_list_uploads/:user_list_upload_id/invitation_attempts/create_many
Création et invitations en masse
Lorsqu'un usager sélectionne puis soumet les usagers à créer ou inviter en masse (endpoints
POST user_list_uploads/:user_list_upload_id/user_save_attempts/create_many
ouPOST user_list_uploads/:user_list_upload_id/invitation_attempts/create_many
), voici ce qu'il se passe:On update les
user_rows
en question pour les marquer comme sélectionnées pour la création ou l'invitation (marked_for_user_save
oumarked_for_invitation
)On enqueue un job
UserListUpload::SaveUsersJob
ouUserListUpload::InviteUsersJob
. Ces jobs vont itérer sur lesuser_rows
sélectionnées et vont appeler les méthodes permettant de sauvegarder un usager ou lancer les invitations. On fait le choix pour l'instant de faire ça en série dans un seul job pour ne pas submerger nos workers et les API externes appelés dans ces process. On pourra si nécessaire revoir le mécanisme et paralléliser les jobs par batch d'usagers.Les pages permettant de suivre les statuts de création/d'invitation sont constamment rafraichies via le stimulus controller
refresh_page_periodically.js
qui trigger un refresh toutes les secondes. Ce code est appelé tant que toutes les sauvegardes ou invitations n'ont pas été toutes tentées. Le refresh via Turbo fait en sorte que seules les éléments ayant changés soient updatés dans le DOM. On choisit de le faire explicitement plutôt que lancer les refresh via des websocket (via la méthodebroadcast_refreshes
) pour 2 raisons:Turbo::BroadcastTurboStreamJob
est exécuté. Or beaucoup de jobs sont enqueued à la sauvegarde d'un usager, ce qui fait que les job de refresh seraient possiblement exécutés tardivement. Cela entrainerait une expérience désagréable avec une barre d'état n'avançant pas régulièrement.Ce qu'il reste à faire
J'ai déjà remarqué certaines choses que l'on devra ajouter à l'implémentation actuelle:
Pour la review
Comme souhaité par l'équipe, j'ai fait une grosse PR regroupant le code la feature de bout en bout.
Malheureusement je me suis un peu emmêlé les pinceaux dans les commits donc le premier commit regroupe déjà la fonctionnalité dans son entièreté, j'en suis désolé 🙏 .
Ce que je suggère pour review c'est de faire les étapes une par une comme montré dans le diagramme et regarder le code MVC associé.
Et bien sûr reste bien sûr disponible si vous avez besoin d'infos 🏃♂️ !