Skip to content
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

feat(pipeline) : Simplify notify_rgpd_contacts #326

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

vperron
Copy link
Contributor

@vperron vperron commented Oct 24, 2024

After an entire year of operation, we noticed that our sources never change their contact emails. Ever.

Even if someday this happens, we can probably suppose that it's till going to be quite rare.

In those exceptional cases, it means that the structure would get a new RGPD notification.

But it does make the code way easier to maintain and more robust:

  • now we will never miss an email that would have been "forgotten" by Brevo (due to rate limits, payment issues, ...)
  • it can also be seen as a feature to send a new notification if the email changes, for instance when the previous one was incorrect.

Moreover, we don't need to maintain the contact_uid list that was sent to brevo.

@vperron vperron self-assigned this Oct 24, 2024
@vperron vperron requested a review from vmttn as a code owner October 24, 2024 13:36
Copy link
Contributor

@vmttn vmttn left a comment

Choose a reason for hiding this comment

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

est-ce qu'il y a une migration de données à prévoir sur brevo ?

),

final AS (
SELECT
contacts.source || '-' || contacts.id AS "_di_surrogate_id",
contacts.id AS "id",
contacts.source AS "source",
contacts.courriel AS "_courriel_original",
Copy link
Contributor

Choose a reason for hiding this comment

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

j'ai l'impression que ce champ a 2 fonctions ici :

  1. permettre d'avoir la vue avant/après
  2. permettre de récupérer la liste des mails à charger dans brevo

Pour 1. , ça n'est pas nécessaire. Il suffira de faire une jointure avec int__union_contacts.

Pour 2., cf commentaire plus haut

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pas totalement certain de comprendre ta remarque, cf. moi aussi plus haut :)

pipeline/dags/notify_rgpd_contacts.py Show resolved Hide resolved
Comment on lines 22 to 31
"""
SELECT
json_build_object('email', _courriel_original),
rgpd_notice_has_hardbounced
FROM public_intermediate.int__union_contacts__enhanced
GROUP BY _courriel_original, rgpd_notice_has_hardbounced
ORDER BY _courriel_original
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

pourquoi utiliser rgpd_notice_has_hardbounced ?

utiliser int__union_contacts serait moins compliqué (pas de rétroaction)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On utilise (détourne ?) le flag hardbounced pour savoir si on a déjà envoyé un email ou non.

  • True/False : oui, un email a déjà été envoyé
  • None : on n'a jamais contacté cet email et donc on le met dans la liste des "nouveaux"

On n'a pas cette info au niveau int__union_contacts qui provient exclusivement des sources mes-aides, mediation numérique et dora; il faut donc faire un JOIN mais je voulais éviter de refaire dans un DAG un JOIN complexe qui par ailleurs est déjà fait dans le contacts__enhanced.

Mais bon, j'ai fait une proposition de fix, tu me diras si ça te convient mieux.

pipeline/RGPD.md Show resolved Hide resolved
@vperron vperron force-pushed the vperron/retry-hardbounced branch from 5902e9f to ca6277e Compare October 29, 2024 10:16
@vperron
Copy link
Contributor Author

vperron commented Oct 29, 2024

Pas de migration de données à prévoir sur Brevo.
Les listes seront mises à jour automatiquement, la seule chose différente est que certains champs que l'on uploadait auparavant (les contact_uids) ne seront plus mis à jour ni utilisés, mais inutile de les "nettoyer".

@vperron vperron requested a review from vmttn October 29, 2024 14:33
@vmttn vmttn force-pushed the vperron/retry-hardbounced branch from ca6277e to 7b39693 Compare November 4, 2024 15:48
@vmttn
Copy link
Contributor

vmttn commented Nov 4, 2024

lgtm

j'ai simplement rebased

After an entire year of operation, we noticed that our sources never
change their contact emails. Ever.

Even if someday this happens, we can probably suppose that it's till
going to be quite rare.

In those exceptional cases, it means that the structure would get a new
RGPD notification.

But it does make the code way easier to maintain and more robust:
- now we will never miss an email that would have been "forgotten" by
  Brevo (due to rate limits, payment issues, ...)
- it can also be seen as a feature to send a new notification if the
  email changes, for instance when the previous one was incorrect.

Moreover, we don't need to maintain the `contact_uid` list that was sent
to brevo.
@vperron vperron force-pushed the vperron/retry-hardbounced branch from 7b39693 to fd8213c Compare November 13, 2024 10:22
@vperron vperron merged commit c083909 into main Nov 13, 2024
6 checks passed
@vperron vperron deleted the vperron/retry-hardbounced branch November 13, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants