Fxa 12583 #19844
Fxa 12583 #19844
Conversation
| constructor( | ||
| private emailSender: sender.EmailSender, | ||
| private linkBuilder: renderer.EmailLinkBuilder, | ||
| private mailerConfig: ConfigType['smtp'], |
There was a problem hiding this comment.
probably want mozlog and/or statsd here too, right?
libs/accounts/email-renderer/src/templates/passwordForgotOtp/en.ftl
Outdated
Show resolved
Hide resolved
| >; | ||
| }; | ||
|
|
||
| type PasswordForgotOtpEmailInput = { |
There was a problem hiding this comment.
I really don't love these. BUT it does make sure we maintain strong typing. Since each email just has unique data they need we kind of have to do this. Any other ideas?
| * @param date - Date to format (defaults to now) | ||
| * @param formatString - Moment.js format string (defaults to 'L' for localized date) | ||
| */ | ||
| constructLocalDateString( |
There was a problem hiding this comment.
Does this get used somewhere other than a test?
There was a problem hiding this comment.
Not yet. It's based on the existing _constructLocalDateString in email.js which is used heavily for subplat emails. I can remove it if we don't need it yet though!
libs/accounts/email-renderer/src/renderer/email-link-builder.ts
Outdated
Show resolved
Hide resolved
| primaryEmail?: string; | ||
| date?: string; | ||
| time?: string; | ||
| acceptLanguage?: string; |
There was a problem hiding this comment.
acceptLanguage is a parameter on the renderEmail but I couldn't find any other of the union-ed types that had this. But with this, calling functions could have it strongly typed. This felt like a good place for it
| */ | ||
| async buildHeaders({ | ||
| template, | ||
| buildHeaders({ |
There was a problem hiding this comment.
There were no awaited calls so it doesn't need to be async
| deviceId?: string; | ||
| flowId?: string; | ||
| flowBeginTime?: number; | ||
| layoutTemplateValues?: Partial<Omit<renderer.TemplateData, 'privacyUrl'>>; |
There was a problem hiding this comment.
If it's a partial, why have an ommit?
| flowId?: string; | ||
| flowBeginTime?: number; | ||
| layoutTemplateValues?: Partial<Omit<renderer.TemplateData, 'privacyUrl'>>; | ||
| templateValues: Omit< |
There was a problem hiding this comment.
Why do we have these omits?
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
Just leaving some intermediate feedback. Looks great. I think there is probably a little clean up to do so we get a nicer type definition for the PasswordForgotOtpEmailInput.
| opts.templateValues.timeZone, | ||
| opts.templateValues.acceptLanguage | ||
| ); | ||
|
|
There was a problem hiding this comment.
Just kind of thinking out loud, but maybe the link build should part of the renderPasswordForgotOtp method. It seems like generating the link is part of the rendering the template to me. This also means that we wouldn't need those 'omits' on the type above you were questioning, since that data could be determined interally.
Let me know what you think.
Because
This pull request
Issue that this pull request solves
Closes: (issue number)
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.