-
Notifications
You must be signed in to change notification settings - Fork 32
feat: email actions improvements #2281
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
Conversation
| auth: "#jobOwnerUser" | ||
| actions: [] | ||
| actions: | ||
| - # Only send emails for updates to status 'finished*' |
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.
Should this be added to jobConfig.recommended.yaml as well, since sending email is such a common task?
| jobParams: true, | ||
| contactEmail: true, | ||
| }, | ||
| allowProtoPropertiesByDefault: false, // limit accessible fields for security |
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.
There were two possible solutions to the problem of template variable coming from mongo (JobClass, DatasetClass, etc). The previous solution was whitelisting properties which we wanted to support. This wasn't maintained (now they are nested under job) so this broke. Instead we call toObject on mongo classes before adding them to the template context.
Another solution would have been to just allow all ProtoProperties here, and then we wouldn't need the overhead of toObject (at least for handlebars). I didn't feel confident about the security ramifications of that, and we might still need toObject for JSONPath calls (which also fail on ProtoProperties).
|
This is ready to review. I am still working on a new job-template.html file, but it will come in a fresh PR. |
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.
LGTM, thanks! some minor comments for eventual later PRs
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.
would it make sense to test some of the handlers here?
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.
Which handlers do you mean?
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.
I meant to have a template that has some handlers inside, e.g. "eq" ro "regex" or similar and test that the email is rendered correctly. But this could indeed be part of a later PR, if useful
| urlencode: urlencode, | ||
| base64enc: base64enc, | ||
| }), | ||
| adapter: new HandlebarsAdapter(handlebarsHelpers), |
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.
for a later PR maybe, should we move the content of useFactory to a dedicated mailer factory class/file?
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.
Agreed. It is getting rather long. It might make sense to wrap handlebars too. Currently you need to remember to register helpers if you call hb directly from outside of a module (eg during testing).
e995ad6 to
522245d
Compare
Also some test cleanup: - Rename testing job template to make it obviously test-only - Remove some optional fields with default values from mock datasets and jobs
Fixes: - improve error message in the switch regex case - flatten Job objects in update notifications Changes: - Add an example job config for sending emails only for finished jobs Features: - Add 'matches' and 'default' handlebars helpers
Previously HTTP error codes were ignored but network errors or templating errors still caused exceptions. Now these should all be caught.
522245d to
70a0bb3
Compare
Description
Following #2271, this adds a
ignoreErrorsconfiguration option to the email action for cases where you don't want an error sending the email to block the job POST or PATCH request.It also makes other small improvements and bug fixes regarding emails.
Motivation
Generating the email can cause errors, either from templating or from sending the URL itself. This allows jobs to continue despite the error. Errors are logged in all cases.
Fixes
Changes:
Tests included
Documentation
official documentation info