Skip to content
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

The email HTML templates added #6146 #6147

Merged
merged 5 commits into from
May 3, 2023
Merged

Conversation

2403905
Copy link
Contributor

@2403905 2403905 commented Apr 26, 2023

Description

The email HTML templates has been added to the notifications service.
The built-in templates can be replaced with custom.

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment: local
  • tested manually via inbucket

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@update-docs
Copy link

update-docs bot commented Apr 26, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@mmattel
Copy link
Contributor

mmattel commented Apr 26, 2023

@2403905 this needs an update in README.md in section Email Notification Templates. See how it is rendered atm.

@2403905 2403905 force-pushed the issue-6146 branch 2 times, most recently from 7ceab9b to 53c2455 Compare April 27, 2023 13:49
@ownclouders
Copy link
Contributor

💥 Acceptance test Core-API-Tests-ocis-storage-10 failed. Further test are cancelled...

@2403905
Copy link
Contributor Author

2403905 commented Apr 27, 2023

@micbar @kobergj Do we need to have the multiple html templates for each kind of message or it can be a single one?
After we move the content to the transifex the templates became very similar.

@2403905 2403905 force-pushed the issue-6146 branch 3 times, most recently from e6bf6aa to 8f94a45 Compare April 27, 2023 22:17
@2403905 2403905 marked this pull request as ready for review April 28, 2023 08:37
Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions

services/notifications/README.md Outdated Show resolved Hide resolved
services/notifications/README.md Outdated Show resolved Hide resolved
Comment on lines 50 to 62
var data map[string][]byte
data, err = readImages(emailTemplatePath)
if err != nil {
return "", "", err
data, err = readFs()
if err != nil {
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the try/catch logic. Two possible alternatives:

(1) Simple Solution: check emailTemplatePath == "" and use readFS if true, otherwise readImages

(2) Master Solution: Make RenderEmailTemplate use an fs.FS instead the emailTemplatePath. On service start create a fs.FS either using templatesFS or os.DirFS(emailTemplatePath)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not nice either 😄 Let's do it correctly here.

Copy link
Contributor Author

@2403905 2403905 May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kobergj done. These changes impacted the email Template Path and now it shouldn't contain the templates subdirectory. The reason why it happened was the templatesFS contains templates and no way to get the subdirectory for the embed FS. Is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NOTIFICATIONS_EMAIL_TEMPLATE_PATH will be like this

{NOTIFICATIONS_EMAIL_TEMPLATE_PATH}/templates/text/email.text.tmpl
{NOTIFICATIONS_EMAIL_TEMPLATE_PATH}/templates/html/email.html.tmpl

@micbar @kobergj @dragonchaser are you ok with it?
I updated the readme

services/notifications/pkg/email/email.go Show resolved Hide resolved
services/notifications/pkg/email/templates.go Outdated Show resolved Hide resolved
services/notifications/pkg/service/service.go Outdated Show resolved Hide resolved
@@ -35,6 +43,7 @@ templates
```

Custom email templates referenced via `NOTIFICATIONS_EMAIL_TEMPLATE_PATH` must also be located in subfolders `shares` and `spaces` and must have the same names as the embedded templates. It is important that the names of these files and folders match the embedded ones.
In the subfolder `html` contains a default HTML template provided by ocis. The images can be embedded in a custom HTML template as a CID source ```<img src="cid:logo-mail.gif" alt="logo-mail"/>``` The image files should be located in `html/img` subfolder. Supported image types are png, jpeg, and gif.
Copy link
Contributor

@mmattel mmattel Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In the subfolder `html` contains a default HTML template provided by ocis. The images can be embedded in a custom HTML template as a CID source ```<img src="cid:logo-mail.gif" alt="logo-mail"/>``` The image files should be located in `html/img` subfolder. Supported image types are png, jpeg, and gif.
The `html` subfolder contains a default HTML template provided by ocis. When using a custom HTML template, images can either be linked with standard HTML code or embedded as a CID source ```<img src="cid:logo-mail.gif" alt="logo-mail"/>``` In the latter case, image files must be located in the `html/img` subfolder. Supported image types are png, jpeg, and gif.

What about svg images, are we open for this when embedding?
Note that I have read (and I may be wrong) that embedding images via CID can cause issues_1 or issues_2 in web clients. Should we drop a note here? like:
Consider that embedding images via a CID resource may not be fully supported in all email web clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmattel Good points

  1. We do not support svg when embedding.
  2. Let's make a notice about embedding and add that hosted images source are also available like <img src="https://raw.githubusercontent.com/owncloud/core/master/core/img/logo-mail.gif" alt="logo-mail"/>
    Or better make the hosted images as a default way?
    BTW How it works in ownCloud 10?
    FYI @kobergj @micbar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better make the hosted images as a default way?

CID linking is ok then admins can see how that has to look like.
Embedding via standard html is imho commonly known.

@mmattel
Copy link
Contributor

mmattel commented Apr 28, 2023

Note that the blank line 23 in readme.md (between the list items) could be removed. I cant make a suggestable commit...

services/notifications/README.md Outdated Show resolved Hide resolved
@2403905 2403905 force-pushed the issue-6146 branch 3 times, most recently from b82cf3d to 7724950 Compare May 2, 2023 16:15
@micbar micbar dismissed kobergj’s stale review May 2, 2023 18:44

Julian is on vacation 😄

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the suggestions.

All in all it looks really awesome to me. Thank you @2403905

@micbar micbar requested a review from dragonchaser May 2, 2023 18:57
Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK from the docs pov

2403905 and others added 2 commits May 3, 2023 10:02
Co-authored-by: Martin <github@diemattels.at>
@sonarcloud
Copy link

sonarcloud bot commented May 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability C 3 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

18.9% 18.9% Coverage
0.0% 0.0% Duplication

@2403905 2403905 merged commit 27322c5 into owncloud:master May 3, 2023
ownclouders pushed a commit that referenced this pull request May 3, 2023
* The email HTML templates added #6146

* use a single palne text email template. use fs.FS

* Update services/notifications/README.md

Co-authored-by: Martin <github@diemattels.at>

* Update services/notifications/README.md

Co-authored-by: Martin <github@diemattels.at>

* fix md

---------

Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>
Co-authored-by: Martin <github@diemattels.at>
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
86 tasks
fschade pushed a commit that referenced this pull request Jul 10, 2023
* The email HTML templates added #6146

* use a single palne text email template. use fs.FS

* Update services/notifications/README.md

Co-authored-by: Martin <github@diemattels.at>

* Update services/notifications/README.md

Co-authored-by: Martin <github@diemattels.at>

* fix md

---------

Co-authored-by: Roman Perekhod <rperekhod@owncloud.com>
Co-authored-by: Martin <github@diemattels.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the email HTML templates to the notifications service
6 participants