-
Notifications
You must be signed in to change notification settings - Fork 33
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 services email et sms #4066
Conversation
@Shamzic Tu veux qu'on regarde pour inverser ces PR ? Le refactoring a plusieurs buts dont un qui est de faciliter la review. Là le soucis c'est qu'on ajoute la feature avant de refacto donc dans un code pas vraiment prévu pour, (en plus de ça ça nécessite soit de merger les deux en même temps soit de merger la partie non refacto tout seul, ce qui revient a faire un refacto sans but après). Je ne connais pas l'urgence de ce sujet, mais si tu pense que ça passe de les inverser dis moi ! Dans le doute je commence a review la première. |
Bonne réflexion, ce serait plus lisible pour vous de faire dans l'autre sens, effectivement. Pas de problème pour inverser les deux PR |
d58c3be
to
e56262e
Compare
Mise à jour de la description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dis moi si tu veux qu'on re-reflechisse la refacto, car je vois des choses qui me gène mais c'est p.e. aussi déjà un avancement.
5b13d8c
to
1d530eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selon moi on est pas loin, après ces modifs on peut sortir le renaming je pense, dispo pour en parler si besoin
f402fb2
to
08007fc
Compare
…pr sendEmail d'email-service
08007fc
to
e53f734
Compare
backend/controllers/emails.ts
Outdated
const result = await renderFollowupEmailByType(followup, emailType) | ||
res.send(result["html"]) | ||
const result = await renderEmailByType(followup, emailType) | ||
if (result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si on rajoute ce if
sans else
on pourrait ne jamais répondre à la requête, on peut soit l'enlever, en se disant qu'au pire on repond rien, soit throw une erreur, mais normalement la fonction au dessus le fait deja non ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shamzic je sais pas si tu avais vu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai supprimé la condition car, comme tu l'indiques, renderEmailByType
relève déjà les cas d'erreur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je n'ai pas encore testé en local mais 3 retours
Y'a un truc que je veux tester encore, mais selon moi on est good, merge probable lundi :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Petits changements, deux emails testé en local, c'est ok pour moi
8bb2ce3
to
258b8aa
Compare
La gestion d'erreur est traitée dans la fonction renderEmailByType
|
||
const renderFollowupEmailByType = async (followup, emailType: EmailType) => { | ||
const renderEmailByType = async (followup, emailType: EmailType) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pour avancé on va pas changer cette PR, mais ici j'ai l'impression finalement que c'est exactement la fonction emailRender
qu'on veut recoder (en inversant le sens des argument ? Je créé un ticket pour plus tard.
Merci pour tes retours @baptou12 |
PR intermédiaire à la PR d'envoi des sondages par SMS
Refactoring sur l'OPS associé : betagouv/aides-jeunes-ops#177