Migration de tous les mails à Swoosh

vdegove commented 2 months ago

Closes #3481

Cette PR :

Migre tous les emails envoyés depuis l’app à Swoosh, ainsi que les tests associés
Les regroupe dans deux modules, UserNotifier et AdminNotifier, avec quelques fonctions communes
Retire le code de l’ancien mailer et du client moulé main Mailjet associé. On utilise toujours Mailjet, mais via l’adaptateur intégré à Swoosh.

Les avantages :

Il est plus facile de changer de provider email avec les adaptateurs intégrés à Swoosh
On évite les gros krakens de code où tout est mélangé : les jobs sont allégés, s’occupent juste de la logique récurrente, et délèguent la fabrication ennuyante de l’email à un module dédié
Les emails sont tous regroupés à un endroit : on a une meilleure visibilité de ce qui sort et est déjà en place
On dispose de fonctions communes pour s’assurer d’une cohérence dans les envois d’email
On pourra faire des templates texte.

À savoir lors de la relecture :

Choix d’implémentation de Swoosh

Swoosh dispose d’une fonction deliver_many/2 que j’ai choisi de ne pas utiliser même si par endroit elle aurait été pertinente, car elle n’est pas implémentée dans tous les adaptateurs. On boucle donc sur du deliver/2
De même, Swoosh peut faire de l’asynchrone, je suis resté sur du synchrone, donc certaines pages attendent le retour de l’API Mailjet pour revenir vers l’utilisateur.

Concernant les tests

Deux-trois broutilles vues et assez importantes à comprendre :

Il y a une fonction assert_emails_sent/1 (au pluriel) mais elle n’est faite que pour fonctionner avec deliver_many/2 (vu plus haut). En cas de multiples mails envoyés avec deliver/2, il faut faire plusieurs assertions de suite assert_email_sent/1.
assert_email_sent marche sur le principe d’une file vidée au fur et à mesure. Si on veut s’assurer qu’il n’y a 3 emails envoyés, il faut faire suivre 3 appels à assert_email_sent/1 par un appel à assert_no_email_sent/0. De même, assert_email_sent/1 ne veut pas dire que un seul email a été envoyé, si on veut vraiment s’en assurer, il faut mettre assert_no_email_sent/0 après. Je ne l’ai pas fait pour ne pas alourdir tous les tests, mais ça pourrait être le cas. En réalité, on devrait presque vérifier à la fin de tous les tests (donc mettre dans le helper général de tests) que aucun email additionnel non prévu ne part.

En plus de la doc, c’est pas mal de lire le code des assertions de test, ainsi que les tests des assertions de test ;)

Choix de syntaxe des tests

On peut utiliser deux syntaxes possibles :

assert_email_sent(
  from: …
  to: "deploiement@transport.data.gouv.fr"
  body: ~r/truc/
)

C’est à préférer lorsque c’est possible. Il y a un peu de magie derrière ce fonctionnement en keywords :

Ça vérifie bien que c’est un email Swoosh qui est envoyé
On peut simplifier certaines choses : par exemple, on peut écrire to: "deploiement@transport.data.gouv.fr" alors que dans le Struct Swoosh.Email c’est en réalité des tuples à l’intérieur d’une liste : to: [{"Déploiement", "deploiement@transport.data.gouv.fr"}].
On peut utiliser aussi une regex pour le text_body ou le html_body.

Cependant, beaucoup de tests faisaient des assertions plus complexes sur le corps du mail, ce qui m’a forcé à utiliser la deuxième syntaxe :

assert_email_sent(fn Swoosh.Email{
  from: {"transport.datagouv.fr", "contact@transport.data.gouv.fr"},
  to: …
  html_body: html
} -> 
  assert html =~ ~r/regex1/
  assert html =~ ~r/regex2/
end)

Petite chose à savoir là dessus : on ne peut pas terminer par un refute dans le corps de la fonction, ça ne marche pas. Si il y en a, il faut les mettre en premier. Par contre toutes les assertions sont bien testées les unes après les autres (j’ai vérifié).

On avait au début utilisé une syntaxe encore plus complexe, mais j’ai estimé que le pattern matching dans la fonction suffisait, j’ai donc retiré tout ce qui ressemblait à :

assert_email_sent(fn Swoosh.Email{} = sent -> 
  assert sent = Swoosh.Email{
    from: {"transport.datagouv.fr", "contact@transport.data.gouv.fr"},
    to: …
    html_body: html
}
  assert html =~ …
end)

TODO list

Reste à faire :

[x] Merger master
[x] Relire les tests et voir si il y a des améliorations
[x] Nettoyer un peu les fonctions helpers dans UserNotifier et AdminNotifier
[x] Rajouter de quoi faire tourner les doctests !

À bouger dans une nouvelle issue :

Réintégrer les nouveaux mails shippés entre temps dans UserNotifier ou AdminNotifier
Bouger les mails texte dans des templates, et les fonctions helpers dans email_view.ex

=> Issue de follow up : https://github.com/etalab/transport-site/issues/3951

AntoineAugusti commented 1 month ago

on ne peut pas terminer par un refute dans le corps de la fonction, ça ne marche pas. Si il y en a, il faut les mettre en premier. Par contre toutes les assertions sont bien testées les unes après les autres (j’ai vérifié)

il y a une explication à ça, par curiosité ?

vdegove commented 1 month ago

Je pense que l'on fera prochainement des changements sur les notifiers pour avoir une chaine subscription > contact > email template > notifications pour bien tracer le fait qu'on a envoyé quelque chose à un contact par le biais d'un abonnement.

Ah oui, tiens, à savoir : il y a une abstraction, Swoosh.Email.Recipient qui permettrait de passer directement un DB.Contact au notifier et à la création d’email : https://hexdocs.pm/swoosh/Swoosh.html#module-recipient

Je ferai une issue followup en mettant ça dedans.

on ne peut pas terminer par un refute dans le corps de la fonction, ça ne marche pas. Si il y en a, il faut les mettre en premier. Par contre toutes les assertions sont bien testées les unes après les autres (j’ai vérifié)

il y a une explication à ça, par curiosité ?

Parce que assert_email_sent(fun) va faire un assert sur la fonction, donc vérifier que l’output de la dernière ligne est true. Donc chaque assertion de la fonction est bien jouée, mais en plus, il y a un assert englobant. Et bizarrement, de ce que je crois comprendre, l’output d’un refute qui marche n’est pas true mais false.

Voir https://github.com/swoosh/swoosh/blob/v1.16.7/lib/swoosh/test_assertions.ex#L114

Et

iex > ExUnit.Assertions.refute 1 == 2
false

ptitfred commented 1 month ago

Good job 👏

etalab / transport-site