[WIP] Add mailing support #10
Conversation
src/index.js
Outdated
| this._logger = new Logger(this.config.options.logging) | ||
| this._logger.init() | ||
|
|
||
| if (this.config.options.mailer.enabled) { |
There was a problem hiding this comment.
It is possible that someone has a config like this, in which case the if statement fails.
{ "options": { "mailer": null } }I suggest also checking the existence/truthiness of this.config.options.mailer.
| await this._logger.logComplete(id) | ||
| } | ||
|
|
||
| async _handleCompleteAlways (id) { |
There was a problem hiding this comment.
Not sure about the naming of this method. Could not find a better one that i really liked otoh though :)
| this._queue.on('job failed attempt', async (id, msg) => await this._logger.logFailedAttempt(id, msg)) | ||
| this._queue.on('job failed', async (id, msg) => await this._logger.logFailure(id, msg)) | ||
| this._queue.on('job complete', async (id) => await this._logger.logComplete(id)) | ||
| this._queue.on('job start', async (id) => await this._handleStart(id)) |
There was a problem hiding this comment.
Extracting or not extracting all these methods was largely a matter of choice before. Now that there are at least 2 event handlers that do more than simply logging
- the complete one that always runs to requeue when defined as a cron
- the failed one sending mail if configured to do so
So i think its a good choice to do this here!
| */ | ||
| const defaults = { | ||
| nodemailerConfig: { | ||
| transportOptions: null, |
There was a problem hiding this comment.
Looks good to me. I think there is a default transportOption we could pursue and add to this project so you don't have to have an smtp server at hand and can simply use this feature by enabling the email option and adding email addresses to the jobs configuration: the nodemailer-sendmail-transport package which uses sendmail under the hood.
Also, overriding the to property based on the email address(es) given in the jobs config file is an important one. When doing so and parsing this somewhere in parseJobs, tests for parseJobs should be updated to reflect this change as well.
@Draggha had to do something like this in his PR for incremental backoff - he added an additional utility method to do the configuration parsing for that which is called inside parseJobs and has its own tests. I like that idea/general direction :)
There was a problem hiding this comment.
I see. I could rebase my PR when #9 is merged.
|
@gonsfx Thank you so much. I have updated the PR. |
|
@kbariotis looking good. please resolve the conflicts and then i'm up for merging it in 👍 |
| } | ||
| } | ||
| ``` | ||
|
|
src/index.js
Outdated
| this._logger = new Logger(this.config.options.logging) | ||
| this._logger.init() | ||
|
|
||
| if ('enabled' in this.config.options.mailer && |
There was a problem hiding this comment.
This can still crash when someone defines { mailer: null }.
I suggest simply testing for truthiness:
if (this.config.options.mailer && this.config.options.mailer.enabled) {
// init mailer
}
|
Comments and merge conflicts resolved. Thank you @gonsfx |
|
@kbariotis Thank you for helping out! 👍 |
3 participants
This is my attempt to resolve #8 and its still a WIP.
I would love to hear your thoughts on the configuration part cause Im really not sure this is the way to go.