Skip to content

Vie privée : Supprimer les emails des candidats de Brevo#5989

Merged
vincentporte merged 5 commits into
masterfrom
vp/GEN-2391_remove_jobseeker_emails_from_brevo
Jun 11, 2025
Merged

Vie privée : Supprimer les emails des candidats de Brevo#5989
vincentporte merged 5 commits into
masterfrom
vp/GEN-2391_remove_jobseeker_emails_from_brevo

Conversation

@vincentporte

@vincentporte vincentporte commented Apr 17, 2025

Copy link
Copy Markdown

🤔 Pourquoi ?

Supprimmer les emalis des candidats lors de leur archivage, en appelant l'API Brevo, sans générer de contention avec la commande notify_archive_jobseekers

🍰 Comment ?

  • mutualisation de BrevoClient et ajout de la méthode de suppression
  • appel de tâches unitaires pour chaque email à supprimer par la méthode de suppression delete_contacts
  • appel de la méthode de suppression dans notify_archive_jobseekers

@vincentporte vincentporte added the ajouté Ajouté dans le changelog. label Apr 17, 2025
@vincentporte vincentporte self-assigned this Apr 17, 2025
@notion-workspace

Copy link
Copy Markdown

@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 3 times, most recently from af3e451 to 16b6db1 Compare April 23, 2025 08:44
@vincentporte vincentporte changed the title Vie Privée: supprimer les emails des candidats de Brevo Vie Privée: Supprimer les emails des candidats de Brevo Apr 23, 2025
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 4 times, most recently from 08d1b45 to c43a72a Compare April 23, 2025 09:55
Comment thread tests/utils/test_brevo.py
Comment thread itou/archive/admin.py Outdated
Comment thread itou/archive/management/commands/delete_emails_from_brevo.py Outdated
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 4 times, most recently from fd89c2a to c99d1f1 Compare April 24, 2025 13:28

@rsebille rsebille left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

La PR étant en brouillon j'ai fait des commentaires généraux donc ils ne sont pas forcément cohérents entre-eux :).

Remarques générales

EmailWaitingForExternalDeletionAdmin()

J'ai un peu l'impression qu'on fait une queue persistante mais ultra-spécialisé, donc je me demande si ça serais pas "mieux" d'implémenter un storage personnalisé dans huey 🤔 en prenant comme exemple leur SqliteStorage.

Sinon je pense qu'on pourrais au moins rendre la classe plus générique afin de pouvoir gérer d'autre type de tâche et ne pas soit dupliquer ce genre de table ou avoir à faire une migration dans le futur 🤷.

notify_archive_jobseekers.py

Pas super convaincu de découpler par défaut la suppression dans le service distant de celui en local, je m'attendais un peu à qu'on essaye en synchrone et si erreur alors on met pour plus tard.

Ce qui d'ailleurs me fait me demander si on ne veux pas forcer le couplage entre la suppression dans les services externes avec celle en interne, un peu en mode "supprimé partout ou nul part".

delete_emails_from_brevo.py

Je pense qu'il manque un mécanisme pour les réessais, un last_try_at et tries_counter pourrais sans doute faire le boulot.

Comment thread tests/utils/test_brevo.py Outdated
Comment thread tests/utils/test_brevo.py Outdated
Comment thread itou/utils/brevo.py Outdated
Comment thread itou/utils/brevo.py Outdated
Comment thread itou/utils/brevo.py Outdated
Comment thread itou/archive/models.py Outdated
Comment thread itou/archive/models.py Outdated
Comment thread itou/archive/models.py Outdated
Comment thread clevercloud/cron.json Outdated
Comment thread itou/archive/management/commands/delete_emails_from_brevo.py Outdated
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from c99d1f1 to 0556120 Compare April 24, 2025 13:57
@vincentporte vincentporte changed the title Vie Privée: Supprimer les emails des candidats de Brevo Vie privée : Supprimer les emails des candidats de Brevo Apr 24, 2025
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 3 times, most recently from 2d48087 to 12d5cc9 Compare May 5, 2025 15:04
@vincentporte vincentporte requested review from rsebille and tonial May 5, 2025 15:04
@vincentporte vincentporte marked this pull request as ready for review May 5, 2025 15:04
Comment thread itou/utils/brevo.py Outdated
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from de84ac9 to 615d2c5 Compare May 6, 2025 06:25
@xavfernandez

xavfernandez commented May 6, 2025

Copy link
Copy Markdown
Contributor

Là quand on supprime un candidat, on met l'email dans notre redis et on croise fort les doigts pour que l'API de Brevo soit rapidement dispo avant notre prochain redémarrage de redis.
Et quand on redémarrera notre redis (pour une mise à jour ou autre) il faudra penser à arrêter le cron d'archivage et attendre que les taches huey soient toutes purgées (ou les dumper puis restaurer) avant de l'arrêter.
Cela me semble très fragile 😬
Donc plutôt en faveur de l'ancien modèle EmailWaitingForExternalDeletion, idéalement généralisé soit via un modèle maison, soit via un PostgresStorage (comme initialement suggéré par Romain).

@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from 921496e to 25a951e Compare May 9, 2025 08:44
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from 25a951e to 0b99450 Compare June 2, 2025 15:58
@vincentporte

vincentporte commented Jun 2, 2025

Copy link
Copy Markdown
Author

02/06/2025, réunion de travail avec @francoisfreitag et @rsebille : décisions

  • ne plus contraindre cette PR à Stabilité: utiliser la DB Postgres en tant que Task Queue Backend #6125, car les données de Redis sont persistantes. Il y a donc peu de risque de perte de données,
  • allongement du délai de rejeu des tâches à 3 mois associé à un message dans sentry via le logger tous les 100 rejeux pour garder la visiblité sur les tâches en échec pour des causes non attrapées. (edited)

@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from 0b99450 to 44cc531 Compare June 2, 2025 16:03
@francoisfreitag

Copy link
Copy Markdown
Member

un message dans le logger

Warning/error dans Sentry*

@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 5 times, most recently from 01361f5 to 904e279 Compare June 5, 2025 09:27
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from 904e279 to 2e33ab9 Compare June 10, 2025 05:55

@tonial tonial left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ça me va.
Je pense qu'il faudra prévoir une carte pour gérer un nettoyage de brevo pour les utilisateurs qui changent d'email, mais ce n'est lié à ce changement, donc tu peux lire puis ignorer le commentaire qui en parle 😝

Comment thread itou/utils/enums.py Outdated
Comment thread itou/utils/brevo.py
)


@task(retries=24 * 6 * 90, retry_delay=10 * 60, context=True) # Retry every 10 minutes during 90 days.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Une bonne manière d'être sur de ne pas se louper :D

Je suis en train de me dCela dit, je me demande si une autre solution ne serait pas de gérer la suppression des emails depuis la commande de synchronisation.
On pourrait lister les candidats dans brevo et supprimer ceux qui ne sont plus dans notre DB 🤔
Après, vu le nombre de candidats, ça pourrait être un cron qui ne tourne qu'une fois par mois, et ça permettrait de réduire la durée de cette tâche ?
Ça ne remplacera sans doute pas cette tâche asynchrone, donc c'est plus une amélioration qu'on pourrait faire à côté (et qui permettra également de gérer les changements d'email de candidats).

Comment thread itou/utils/brevo.py
Comment thread itou/utils/brevo.py
Comment thread itou/utils/brevo.py Outdated
Comment thread tests/utils/test_brevo.py Outdated
Comment thread tests/utils/test_brevo.py Outdated
Comment on lines +117 to +126
def test_delete_contact_request_error(mocker, caplog):
mocker.patch("itou.utils.brevo.httpx.Client.delete", side_effect=httpx.RequestError("Connection timed out"))

brevo_client = BrevoClient()

with pytest.raises(httpx.RequestError):
brevo_client.delete_contact("somebody@mail.com")

error_record = next(record for record in caplog.records if record.levelname == "ERROR")
assert error_record.message == "Brevo API: Request failed: Connection timed out"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Peut-être combiner ce test avec le test de retries pour vérifier que la tâche est réessayée tant qu’elle est en erreur.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

J'ai conservé des tests appelant directement delete_contact pour tester les exceptions levées, et un appelant la méthode asynchrone async_delete_contact pour tester les retries.
Huey semble ne pas propager les exceptions levées dans les méthodes décorées, nous avons accès uniquement aux logs, donc sans certitude sur la présence d'un raise.
Peut-être une optimisation pourra être apportée plus tard.

@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch 3 times, most recently from d7714b1 to 4d91aaa Compare June 11, 2025 10:33
@vincentporte vincentporte force-pushed the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch from 4d91aaa to 5c1e917 Compare June 11, 2025 10:46
@vincentporte vincentporte added this pull request to the merge queue Jun 11, 2025
Merged via the queue into master with commit 4749c26 Jun 11, 2025
14 checks passed
@vincentporte vincentporte deleted the vp/GEN-2391_remove_jobseeker_emails_from_brevo branch June 11, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ajouté Ajouté dans le changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants