-
Notifications
You must be signed in to change notification settings - Fork 220
feat(auth): Use libs email in auth server #19952
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
base: main
Are you sure you want to change the base?
Conversation
| ...FxaMailerFormat.localTime(request), | ||
| ...FxaMailerFormat.location(request), | ||
| ...FxaMailerFormat.device(request), | ||
| code: 'todo', |
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.
Don't forget to provide these!
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.
Are redirect to and service needed?
| ...FxaMailerFormat.location(request), | ||
| ...FxaMailerFormat.device(request), | ||
| ...FxaMailerFormat.sync(false), | ||
| code: 'SEC456', |
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.
Don't forget to provide actual code!
| ...FxaMailerFormat.location(request), | ||
| ...FxaMailerFormat.device(request), | ||
| code: emailCode, | ||
| resume, |
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.
Are redirectTo & service needed? I see they are provided in the original implementation?
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.
Very likely! I didn't see them on the underlying type but I'll update that if needed
| clientName: 'Firefox', | ||
| redirectTo: 'https://example.com', | ||
| resume: 'resumeToken', | ||
| }); |
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.
Is service needed? I see it's provided in the original data.
| mockFxaMailer.sendVerifyEmail.callCount, | ||
| 0, | ||
| 'mailer.sendVerifyEmail was not called' | ||
| 'mockFxaMailer.sendVerifyEmail was called' |
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.
was not?
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.
Good call - I was fixing some of them since they weren't right and just mixed this one up
b6e02d5 to
4db63fd
Compare
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.