-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: Notification Email Templates #13940
base: main
Are you sure you want to change the base?
feat: Notification Email Templates #13940
Conversation
Without the HTML viewer for styling and such, it is very hard to check for the modification, right? |
It may indeed be handy to have a preview to check the modifications. The implementation right now is bare bones. A basic optimization would be to either add a wysiwyg editor, or add the ability to send a test mail? |
The PR is updated using the Svelte 5 syntax. |
web/src/lib/components/shared-components/settings/setting-textarea-wysiwyg.svelte
Show resolved
Hide resolved
@alextran1502 , i updated the last file to Svelte 5. Let me know if i need to do some more work on this pr. |
📖 Documentation deployed to pr-13940.preview.immich.app |
There was a strange lint error on the translations, but after readding the keys a couple of times, the linter tests successfully. |
I will temporary set this PR back to draft until the svelte 5 migration is done. |
@alextran1502 , the PR is ready for review after your svelte 5 migration. |
|
||
## Notification templates | ||
|
||
You can override the default notification text with custom templates. You can uses tags to show dynamic tags in your templates. |
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.
You can override the default notification text with custom templates. You can uses tags to show dynamic tags in your templates. | |
You can override the default notification text with custom templates. You can use tags to show dynamic tags in your templates. |
This is kind of a weird sentence. "You can use tags to show dynamic tags[...]"?
## Notification templates | ||
|
||
You can override the default notification text with custom templates. You can uses tags to show dynamic tags in your templates. | ||
Templates support HTML tags like P, BR, H1, H2, ... |
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 don't think we need to enumerate those here
@@ -1311,4 +1321,4 @@ | |||
"yes": "Yes", | |||
"you_dont_have_any_shared_links": "You don't have any shared links", | |||
"zoom_image": "Zoom Image" | |||
} | |||
} |
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.
Please add the newline back to this file
"template_email_update_album": "Update Album Template", | ||
"template_settings": "Notification Templates", | ||
"template_settings_description": "Manage custom templates for notifications.", | ||
"template_email_if_empty": "If template is empty, the default email will be used.", |
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.
"template_email_if_empty": "If template is empty, the default email will be used.", | |
"template_email_if_empty": "If the template is empty, the default email will be used.", |
"template_settings": "Notification Templates", | ||
"template_settings_description": "Manage custom templates for notifications.", | ||
"template_email_if_empty": "If template is empty, the default email will be used.", | ||
"template_email_available_tags": "You can use following variables in your template: {tags}", |
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.
"template_email_available_tags": "You can use following variables in your template: {tags}", | |
"template_email_available_tags": "You can use the following variables in your template: {tags}", |
|
||
let templateResponse = ''; | ||
|
||
switch (name) { |
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.
That name should probably be an enum
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 feel like we shouldn't write this ourselves but use an existing, proper library that also takes care of properly escaping malicious code (although you may argue that an admin is trusted, not sure)
@@ -112,6 +112,7 @@ export class BaseService { | |||
configRepo: this.configRepository, | |||
metadataRepo: this.systemMetadataRepository, | |||
logger: this.logger, | |||
notifications: this.notificationRepository, |
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.
Why did you add this?
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.
What are these used for?
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.
What about this is a WYSIWYG text area and not just any normal text area?
This PR introduces a feature to allow system users to customize the text of email notifications by setting up custom templates in the system settings. Users can tailor their email content with dynamic tags, and HTML is supported for formatting. This update aims to improve flexibility and personalization in user communications.
Key Changes
This enhancement enables administrators to align notifications with specific branding or informational needs more effectively.