feat: add configurable email templates #1173
Conversation
gnyiri
changed the title
…/UserOfficeProject/user-office-core into add-configurable-email-templates
gnyiri
requested review from
William-Edwards-STFC,
jekabs-karklins and
yoganandaness
and removed request for
a team
|
@gnyiri Could you resolve the conflicts? |
There was a problem hiding this comment.
Apologies for the late review.
Good work this. However, there are few areas that i felt could be improved and have commented here. Feel free to mail me or msg me, in case you need a call and we can talk.
And there are few conflicts in the PR
Thanks
apps/backend/src/datasources/postgres/EmailTemplateDataSource.ts
Outdated
Show resolved
Hide resolved
apps/backend/src/datasources/postgres/EmailTemplateDataSource.ts
Outdated
Show resolved
Hide resolved
apps/backend/src/datasources/postgres/EmailTemplateDataSource.ts
Outdated
Show resolved
Hide resolved
|
|
||
| IF email_template_id_var IS NOT NULL THEN | ||
| UPDATE workflow_connection_has_actions | ||
| SET config = jsonb_set(config::jsonb, ('{recipientsWithEmailTemplate, ' || array_loop_var || ', emailTemplate, id}')::text[], to_jsonb(email_template_id_var), false) |
There was a problem hiding this comment.
This is good, as i can see instead of saving the hardcoded template id(ISIS Xpress PI Co-I Finish Email), we are going to save the primary key from the new table(email_templates)
Given that case, is it a good idea to go for a new table with foreign constraint for a safer transaction. This can exist only for smtp related operations and not for sparkpost. sparkpost can stay as it is.
with fk, it will be easy to handle deletion of email templates. for ex., if we delete an email template, we can add a cascading effect, so that the corresponding status actions will also be cleared with the email template.
There was a problem hiding this comment.
If we have more time, this can be done as a part of this PR. Or else, this can be taken up on the later phase.
There was a problem hiding this comment.
I am still struggling with the spark post emails. What should be the email template id in these cases? Should it be zero? If so, the validation will fail. In the recent update I use the unique template name for identifying templates instead of the template id because for Spark Post emails there is no associated db id.
| proposals: [proposal], | ||
| template: recipientsWithEmailTemplate.emailTemplate.id, | ||
| template: recipientsWithEmailTemplate.emailTemplate.name, | ||
| templateId: recipientsWithEmailTemplate.emailTemplate.id, |
There was a problem hiding this comment.
I think, flipping this will cause variability in the app behaviour especially in terms of logging and saving event logs
| const getEmailTemplate = (foundRecipientWithEmailTemplateIndex: number) => { | ||
| if (foundRecipientWithEmailTemplateIndex !== -1) { | ||
| return ( | ||
| emailTemplates.find( |
There was a problem hiding this comment.
I think we are doing this to ensure that the setted up Email template is present in the Overall list, and if it does not, return null. If we could implement the foreign key relationship, then we will not have this issue.
apps/frontend/src/graphql/emailTemplate/createEmailTemplate.graphql
Outdated
Show resolved
Hide resolved
apps/frontend/src/graphql/emailTemplate/updateEmailTemplate.graphql
Outdated
Show resolved
Hide resolved
|
|
5 similar comments
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Description
Configure email templates from UI. The PR introduces a new table for storing email templates (name, description, subject, body). The logic of assigning email templates to status actions will be the same as before: the user selects the template name from the list of available templates (populated from DB).
Important notes:
Motivation and Context
At ELI we frequently change the email templates and found it uncomfortable to write physical files on the host of the backend.
How Has This Been Tested
Manual tests + backend test.
Fixes
N/A
Changes
See above
Depends on
N/A
Tests included/Docs Updated?
Yes