sendVerificationEmail and setup email #59
Conversation
| if (!address || !includes(user.emails.map((email: string) => email.address), address)) { | ||
| throw new AccountsError('No such email address for user'); | ||
| } | ||
| const token = await this.db.addEmailVerificationToken(userId, address); |
There was a problem hiding this comment.
The db should layer should probably not be responsible of creating the token as this will be the same logic for every store. The only caveat with moving that logic to here is that the token should be unique and so we need the db to allow search by email verification token to make sure the "new token" is valid.
There was a problem hiding this comment.
Right now there is this function that generate the token https://github.com/js-accounts/accounts/blob/master/packages/server/src/tokens.js#L5 and in verifyEmail function there is this call to the bdd (who must take the token as parameter btw) to find the user with the token
| throw new AccountsError('No such email address for user'); | ||
| } | ||
| const token = await this.db.addEmailVerificationToken(userId, address); | ||
| const resetPasswordUrl = `${this._options.siteUrl}/reset-password/${token}`; |
There was a problem hiding this comment.
Why this has reset-password route?
|
@davidyaha Now the token is generated in the function, is it better? |
| @@ -302,6 +321,57 @@ export class AccountsServer { | |||
| const res = await this.db.setProfile(userId, { ...user.profile, ...profile }); | |||
| return res; | |||
| } | |||
|
|
|||
| async sendVerificationEmail(userId: string, address: string): Promise<void> { | |||
| const user = await this.db.findUserById(userId); | |||
There was a problem hiding this comment.
You should probably extract the code from this line to L338 to another function because it is repeating on the sendResetPasswordEmail method
There was a problem hiding this comment.
It is not exactly the same things for sendVerificationEmail this will return the first unverified email.
For sendResetPasswordEmail it will return the first email.
| const emailRecord = find(user.emails, (e: Object) => e.address === tokenRecord.address); | ||
| if (!emailRecord) { | ||
| throw new AccountsError('Verify email link is for unknown address'); | ||
| } | ||
| await this.db.verifyEmail(user.id, emailRecord); | ||
| } | ||
|
|
||
| async resetPassword(token: string, newPassword: string): Promise<void> { | ||
| const user = await this.db.findUserByResetPasswordToken(token); |
There was a problem hiding this comment.
Could be named findByResetPasswordToken. For example theres no "User" in the findByEmail name.
There was a problem hiding this comment.
All our find are with Usereven findUserByEmail
packages/server/src/email.js
Outdated
|
|
||
| sendMail(mail: Object): Promise<Object> { | ||
| return new Promise((resolve, reject) => { // eslint-disable-line flowtype/require-parameter-type | ||
| // If no configuration for email just warm the user |
| resolve(); | ||
| return; | ||
| } | ||
| this.server.send(mail, (err: Object, message: Object) => { |
There was a problem hiding this comment.
Would be cool if user could set a function in the config to handle sending emails. At the moment I think this only supports smtp, but what about the use case when you want to do an api call to mailgun to send the email?
|
@TimMikeladze If you can review the README |
|
@pradel Looks good |
|
you must modify the resolver file of mutaion function |
#40
Setup email part.
Add siteUrl in config to construct absolute urls for email.
Add sendVerificationEmail.
Add sendResetPasswordEmail.
To overwrite the email templates: