Send email when new passlist entry is added #2087
Conversation
rfontanarosa
changed the title
gino-m
changed the title
|
@rfontanarosa Any updates on this PR? |
| host: string; | ||
| port: number; | ||
| username: string; | ||
| password: string; |
There was a problem hiding this comment.
Will we be storing the password in plain-text? This sounds like a big risk. Does the mail server offer some sort of auth token or other mechanism for authenticating?
| export interface MailServiceEmail { | ||
| to: string; | ||
| subject: string; | ||
| html: string; |
There was a problem hiding this comment.
s/html/template/? That way we can also introduce Markdown in the future without breaking the name, and it will also make it clear there can be placeholders?
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| this.transporter_.sendMail( | ||
| {from: this.sender_, ...email, html: safeHtnl}, |
| if (error) { | ||
| // 501 and 550 are errors from the mail server: email address not found | ||
| if (error.responseCode === 501 || error.responseCode === 550) { | ||
| reject(new Error(error.response)); |
There was a problem hiding this comment.
Why not just reject(error) in both cases?
| db: Datastore | ||
| ): Promise<MailServerConfig | undefined> { | ||
| const mailConfig = (await db.fetchMailConfig()) as MailConfig; | ||
| if (!mailConfig) console.error('Unable to find mail configuration'); |
There was a problem hiding this comment.
Is it really an error if an instance of Ground doesn't have a mail serve? Assuming it's optional, perhaps this should be a debug log message instead.
| if (!mailConfig) console.error('Unable to find mail configuration'); | ||
| const {server: mailServerConfig} = mailConfig; | ||
| if (!mailServerConfig) | ||
| console.error('Mail server config not found in /config/mail/server'); |
There was a problem hiding this comment.
Since the if branch doesn't return this code will log an error twice. Let's instead use !mailConfig?.mailServerConfig to check and log once?
| import {MailServiceEmail} from './common/mail-service'; | ||
| import {stringFormat} from './common/utils'; | ||
|
|
||
| export async function onCreatePasslistEntryHandler( |
There was a problem hiding this comment.
Please add TSDoc at least to all public methods and classes.
| const template = await db.fetchMailTemplate('passlisted'); | ||
|
|
||
| if (!template) { | ||
| console.error('Template not found in /config/mail/templates/passlisted'); |
There was a problem hiding this comment.
s/Template/Passlist notification email template/
Also, if this is an optional feature, reduce the to console.debug
closes #2077