Send email on doi registration desy changes#2430
Send email on doi registration desy changes#2430rkweehinzmann wants to merge 8 commits intomasterfrom
Conversation
rkweehinzmann
force-pushed
the
send-email-on-doi-registration-DESY-changes
branch
from
06c6942 to
4abd11e
Compare
rkweehinzmann
force-pushed
the
send-email-on-doi-registration-DESY-changes
branch
from
11ccc92 to
f51b828
Compare
sbliven
left a comment
sbliven
left a comment
There was a problem hiding this comment.
Generalizing EmailAction.perform so that it can be used in other places is a good idea. I have a few suggestions how to implement that.
The JSON file keys are fundamentally a way to configure an event that is run at the end of the publishedDataset/register call. I think a more idiomatic way to implement this would be to use the NestJS Event Emitter package. That way we can easily emit events for other endpoints if needed. By using the event emitter there is a clear place to parse the file (the module implementing the handler functions), something that is currently lacking.
|
|
||
| // Fetch the email content for a template from the configuration | ||
| getEmailContent(templateName: string, doi: string, userEmail: string): { subject: string; body: string } | null { | ||
| const emailTemplates = this.configService.get("emailTemplate") |
There was a problem hiding this comment.
emailTemplates is the filename. You need to parse it at some point; either here (needs to be parsed for each email) or once on startup (see how JOB_CONFIG_FILE gets parsed). I suggest using the yaml parser (which also supports json).
There was a problem hiding this comment.
Ah, I guess you meant to inject EmailConfigService
| getEmailContent(templateName: string, doi: string, userEmail: string): { subject: string; body: string } | null { | ||
| const emailTemplates = this.configService.get("emailTemplate") | ||
| if (emailTemplates) { | ||
| const subject = emailTemplates.subject.replace("{doi}", doi); |
There was a problem hiding this comment.
This should use handlebars for templating. Rather than passing the doi, I would pass a context object with a well-defined interface. I recommend keeping it similar to the JobTemplateContext for consistency, eg
export interface PublicDatasetContext<DtoType extends JobDto> {
publishedData: PublishedData; // res
env: Record<string, string | undefined>; //process.env
}
The method should be similar to EmailAction.perform(contxt)
| @@ -0,0 +1,39 @@ | |||
| stages: | |||
There was a problem hiding this comment.
I guess .gitlab-ci is desy-specific and should not be included in the final PR
| @@ -1,5 +1,5 @@ | |||
| import { Module } from "@nestjs/common"; | |||
| import { MailService } from "./mail.service"; | |||
There was a problem hiding this comment.
We don't have 'services' directories elsewhere in the project. Is this file rename necessary?
|
|
||
| if (fs.existsSync(configFilePath)) { | ||
| const configFile = fs.readFileSync(configFilePath, "utf-8"); | ||
| this.config = JSON.parse(configFile); |
There was a problem hiding this comment.
This can be very easily swapped out with js-yaml.load while keeping full support for json. See jobconfig.service.ts:60.
|
|
||
| // Loads the configuration file if it exists | ||
| private loadConfig(): void { | ||
| const configFilePath = path.join(__dirname, "email-templates.json"); |
There was a problem hiding this comment.
| const configFilePath = path.join(__dirname, "email-templates.json"); | |
| const configFilePath = this.configService.get("emailTemplate") |
2 participants
DRAFT PR for
The idea was that we can have emails as a separate action, not only happening after a job, but refactor the email action logic so that it can be used as a standalone module whenever needed.