feat(auth): Use libs email in auth server #19952
Conversation
nshirley
force-pushed
the
FXA-12883
branch
4 times, most recently
from
b5529cc to
9ecdac7
Compare
| if (fxaMailer.canSend('passwordChangeRequired')) { | ||
| // the new fxa-mailer is more type script, so we create a 'fake' | ||
| // request to satisfy the type checking | ||
| const request = { |
There was a problem hiding this comment.
@dschom - curious if you have a better idea for how to handle this. The mailer functions being type safe means it's expecting all of this, but also I don't want to just cast request as unknown as any
There was a problem hiding this comment.
I actually think this is great. All this stuff can and is used in templates. So IMO, it makes sense that we explicitly declare it and pass it. It might make sense to move this 'mock' to a central location where it can be reused though. There might even already be a mock somewhere that's pretty close.
| OmitCommonLinks<TemplateData> & | ||
| OmitCommonLinks<verifyPrimary.TemplateData> & { | ||
| code: string; | ||
| service?: string; |
There was a problem hiding this comment.
I wonder if this should be ({ code:string } || { code: string, service:string, redirectTo:string, resule:string }). I think you had a similar comment during your review on my code. I am pretty sure if we have a service we want resume and redirectTo also.
There was a problem hiding this comment.
So, I tried to do this but it doesn't enforce the additional parameters when one is defined (i.e., adding in service doesn't then make redirectTo and resume required als), if that's what the intent was. And from what I can tell, we can't always guarantee that places that call this will have those available to them... maybe I can dig in more to check the routes, handlers, joi validation etc
| metricsEnabled, | ||
| { email: opts.to } | ||
| ), | ||
| link: this.linkBuilder.buildVerifyEmailLink(template, metricsEnabled, { |
There was a problem hiding this comment.
If I'm following the templates correctly then it does look like it's used in libs/accounts/email-renderer/src/templates/verificationReminderFirst/index.txt
Though, now that I'm looking at this - your comment may have been for a different line and I've since pushed so I don't know for sure...
| postalCode: '', | ||
| }, | ||
| }, | ||
| acceptLanguage: 'en/US', |
There was a problem hiding this comment.
The browser posts something like this: en-US,en;q=0.9. I think en is the safest fallback. Basically I'm a little unsure if the /US is valid?
We also have a locale column on the accounts table in the database. I think in this case we should use that to target the language that's most likely what the user is used to.
There was a problem hiding this comment.
I ended up fixing this to a better 'default' value but also trying to use the account.locale then falling back if it's not set for some reason
nshirley
force-pushed
the
FXA-12883
branch
8 times, most recently
from
f87de31 to
8114620
Compare
|
|
||
| // since these are sent via a scheduled job, we need to fabricate a request object | ||
| // for the fxa-mailer formatting functions | ||
| const request = { |
There was a problem hiding this comment.
As mentioned above, can we move this to a central location?
| return { | ||
| privacy: this.config.privacyUrl, | ||
| support: this.config.supportUrl, | ||
| unsubscribe: this.config.unsubscribeUrl, |
There was a problem hiding this comment.
Do we use these anywhere? I can't seem to find reference. I think these can just go away, all refs to these values seem to be internal to this class and can just reference the config directly.
| metricsEnabled: boolean, | ||
| content?: string | ||
| content?: string, | ||
| oneClickLink = false |
| await this.fxaMailer.sendVerifyAccountChangeEmail({ | ||
| ...FxaMailerFormat.account( | ||
| // probably just update the type interface above | ||
| account as Account & |
There was a problem hiding this comment.
Weird, it definitely was yelling before... 🤔 but now it isn't!
There was a problem hiding this comment.
Oh, right, I updated the formatter to accept metricsOptOutAt as metricsOptOutAt?: number | null instead of just metricsOptOutAt: number | null - the underlying property on the auth model is optional (number | undefined) which isn't compatible with number | null. Marking it optional gave sufficient overlap
Because: - We've moved email sending and rendering out of auth-server This Commit: - Updates several emails to send using the new fxa-mailer, renderer and sender Closes: FXA-12883
Because:
This Commit:
Closes: FXA-12883
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.