etalab / transport-site

Rendre disponible, valoriser et améliorer les données transports
https://transport.data.gouv.fr
184 stars 28 forks source link

Table notifications : ajout de colonnes, sauvegarde de métadonnées #4000

Closed AntoineAugusti closed 19 hours ago

AntoineAugusti commented 2 weeks ago

Fixes https://github.com/etalab/transport-site/issues/3196

Cette PR contient les modifications suivantes :

Cette PR débloque la possibilité de sortir l'espace réutilisateur, le point le plus bloquant étant le bloc "Notifications envoyées aux producteurs" sur dataset#details qui aurait mixé producteurs et réutilisateurs sans informations supplémentaires.

À traiter ultérieurement

vdegove commented 1 week ago

Réflexion comme ça. Il y a des cas où un seul email envoyé correspond à plusieurs objets DB.Notification, si je ne me trompe pas, parce que plusieurs dataset_id ? Et le traitement de ce problème a été reporté à plus tard (ce qui est très bien ?) Est-ce que dans ce cas, on ne pourrait pas normaliser et identifier les cas où ça survient avec un truc similaire dès maintenant dans la payload ?

AntoineAugusti commented 1 week ago

@vdegove Je ne suis pas certain d'avoir compris. Peux-tu reformuler ? Tu voulais dire d'ajouter dès maintenant payload: %{dataset_ids: ids} pour les notifications où on enregistre plusieurs lignes avec dataset_id renseigné dès maintenant, avant le refactor suivant ?

vdegove commented 1 week ago

Je reformule (même si c’est un peu flou pour moi aussi) !

  1. Tu confirmes qu’il peut toujours y avoir plusieurs lignes dans la table notification pour un seul email envoyé ? Quels sont les cas, déjà ? En revanche, ces lignes de notifications correspondent en 1 à 1 chacune à une ligne dans notifications_ubscription ?
  2. Je pense que ça pourrait être pas mal de réussir à identifier ces notifications dans la table, et pouvoir relativement facilement les grouper, faire du débug… Sur le comment, je sais pas trop, peut-être un truc dans la payload du genre : %{grouped_notification: %{uuid: (générer un uuid faute de mieux), dataset_ids: []}.
AntoineAugusti commented 1 week ago

Tu confirmes qu’il peut toujours y avoir plusieurs lignes dans la table notification pour un seul email envoyé ? Quels sont les cas, déjà ?

Oui, il y a 7 de mémoire. On peut les trouver dans la codebase (sur cette branche) en cherchant DB.Notification.insert(dataset (quand on passe un dataset on remplit dataset_id).

En revanche, ces lignes de notifications correspondent en 1 à 1 chacune à une ligne dans notification_subscription ?

Oui

Je pense que ça pourrait être pas mal de réussir à identifier ces notifications dans la table, et pouvoir relativement facilement les grouper

C'était déjà fait en partie dans les commits précédents, en ajoutant job_id dans notifications.payload où la valeur est l'identifiant du job Oban qui a envoyé un batch. https://github.com/etalab/transport-site/pull/4000/commits/d31e995c48a2c58743816305271e41bca7c1a5a4 fait le tour de ce qui manquait et il y a désormais un pattern-match dans DB.Notification.insert! qui force sa présence.

AntoineAugusti commented 1 week ago

@vdegove Si tu as du temps pour une dernière phase de retours, je ferai de nouveau une passe avant de merger. Il y aura d'autres refactors à suivre. Tes précédents commentaires ont permis de belles améliorations/refactor et je t'en remercie. 🙏

Concernant le backfill ce qui m'embête c'est que les requêtes à exécuter vont faire apparaître des données personnelles (adresses e-mails de certains membres du PAN ou de calculateurs d'itinéraires/ART) et je préfère ne pas mettre ça dans GitHub.

vdegove commented 19 hours ago

4000 !

AntoineAugusti commented 16 hours ago

Pour migrer quelques valeurs de notifications.role vers reuser.

update notifications set role = 'reuser' where reason in ('resources_changed', 'new_dataset', 'datasets_switching_climate_resilience_bill', 'promote_reuser_space', 'dataset_now_licence_ouverte', 'datasets_switching_licences');
-- UPDATE 1697

Je mettrai à jour ce message