Skip to content

feat(coderd): add company logo when available for email notifications #14935

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

Merged
merged 27 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a42f108
feat(notifications): add company logo url when available for email no…
defelmnq Oct 2, 2024
1fa1271
feat(notifications): adapt tests with new labels
defelmnq Oct 3, 2024
2def52e
fix: revert changes on generated code version
defelmnq Oct 3, 2024
1b1a4c4
feat(notification): move logo_url and app_name logic to helpers funct…
defelmnq Oct 3, 2024
779260e
chore: remove unused GetLogoURL method from notifications store inter…
defelmnq Oct 3, 2024
1e6899f
fix: add better context and timeout for db related queries
defelmnq Oct 4, 2024
b552267
chore: lint
defelmnq Oct 4, 2024
73e07e9
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq Oct 16, 2024
70e23ae
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq Oct 16, 2024
2f620c9
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq Oct 16, 2024
fdcdf7b
feat(notifications): improve logo_url and app_name fetching moving it…
defelmnq Oct 16, 2024
9c6c105
feat(coderd): regenerate golden files
Oct 16, 2024
0c131a5
feat(notifications): working on tests for logo and app_name
defelmnq Oct 17, 2024
34d6611
feat(notifications): change helpers logic to be defined in the Dispat…
Oct 17, 2024
0a9a66a
feat(notifications): add golden file for custom appearance
Oct 17, 2024
bf558cb
feat(notifications): add golden file for custom appearance
Oct 17, 2024
a420f37
feat(notifications): work on tests
defelmnq Oct 18, 2024
51d8d33
feat(notifications): add golden file for custom appearance
defelmnq Oct 18, 2024
d492d09
feat(notifications): add golden file for custom appearance
defelmnq Oct 18, 2024
0c9c485
feat(notifications): add comment ent in tests for enterprise feature
defelmnq Oct 18, 2024
a6d4a0c
feat(coderd): remove unused call
defelmnq Oct 18, 2024
4c5cb3d
fix(notifications): improve tests and some nit fixes
defelmnq Oct 18, 2024
e419431
chore(retry): improve retry policy on fetcher
defelmnq Oct 18, 2024
a7fec66
Merge remote-tracking branch 'origin/main' into feat/notif-custom-logo
defelmnq Oct 21, 2024
8b766f6
improve errors handling
defelmnq Oct 22, 2024
157e086
improve errors handling
defelmnq Oct 22, 2024
790ff33
improve errors handling
defelmnq Oct 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat(notification): move logo_url and app_name logic to helpers funct…
…ions
  • Loading branch information
defelmnq committed Oct 16, 2024
commit 1b1a4c413d4b33918e06e1f7163d49b86dd06cd2
24 changes: 24 additions & 0 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1309,6 +1309,30 @@ func templateHelpers(options *coderd.Options) map[string]any {
return map[string]any{
"base_url": func() string { return options.AccessURL.String() },
"current_year": func() string { return strconv.Itoa(time.Now().Year()) },
"logo_url": func() string {
logoURL, err := options.Database.GetLogoURL(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps passing a context to templateHelpers (server lifetime) would be a good idea, and perhaps we should define a maximum execution time here (context.WithTimeout) so that we don't end up in a situation where the DB query simply blocks for an indefinite amount of time.

Copy link
Member

Choose a reason for hiding this comment

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

👍

I have similar feelings. We might be risking by placing DB call inside a template helper. I'm wondering if we shouldn't implement something similar to AppearanceFetcher for the site handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed thanks, good catch ! 🙏

As these methods seem directly connected to templating engine from go , I've not been able to find a way to pass a context to it. Anyway, I created a context with a hardcoded 1 second timeout for it , to at least make it safer.

Not sure if there's a better way to do ?

Copy link
Member

@mtojek mtojek Oct 4, 2024

Choose a reason for hiding this comment

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

What would you say if we slightly modify this concept. My main concern is around swallowing the error in favor of returning an empty string "", so:

Wherever we render the htmlBody:

	htmlBody, err = render.GoTemplate(htmlTemplate, payload, s.helpers)
	if err != nil {
		return nil, xerrors.Errorf("render full html template: %w", err)
	}

we create a dynamic template.FuncMap with app_name and logo_url. We could add a reference to the store to SMTPHandler, and retrieve values before rendering.

Thoughts?

if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return notifications.NotificationsDefaultLogoURL
}

return ""
}

return logoURL
},
"app_name": func() string {
appName, err := options.Database.GetApplicationName(context.Background())
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return notifications.NotificationsDefaultAppName
}

return ""
}

return appName
},
}
}

Expand Down
5 changes: 4 additions & 1 deletion coderd/notifications/dispatch/smtp/html.gotmpl
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
<body style="margin: 0; padding: 0; font-family: -apple-system, system-ui, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', 'Ubuntu', 'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif; color: #020617; background: #f8fafc;">
<div style="max-width: 600px; margin: 20px auto; padding: 60px; border: 1px solid #e2e8f0; border-radius: 8px; background-color: #fff; text-align: left; font-size: 14px; line-height: 1.5;">
<div style="text-align: center;">
<img src="{{ .Labels._logo_url }}" alt="Company Logo" style="height: 40px;" />
<img src="{{ logo_url }}" alt="Company Logo" style="height: 40px;" />
<h1 style="text-align: center; font-size: 36px; font-weight: 400; margin: 8px 0 32px; line-height: 1.5;">
{{ app_name }}
</h1>
</div>
<h1 style="text-align: center; font-size: 24px; font-weight: 400; margin: 8px 0 32px; line-height: 1.5;">
{{ .Labels._subject }}
Expand Down
2 changes: 2 additions & 0 deletions coderd/notifications/dispatch/smtp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,8 @@ func TestSMTP(t *testing.T) {
helpers := map[string]any{
"base_url": func() string { return "http://test.com" },
"current_year": func() string { return "2024" },
"logo_url": func() string { return "https://logo.company" },
"app_name": func() string { return "TestCompany" },
}
handler := dispatch.NewSMTPHandler(tc.cfg, helpers, logger.Named("smtp"))

Expand Down
5 changes: 2 additions & 3 deletions coderd/notifications/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,8 @@ func TestWebhookDispatch(t *testing.T) {

// WHEN: a notification is enqueued (including arbitrary labels)
input := map[string]string{
"a": "b",
"c": "d",
"_logo_url": notifications.NotificationsDefaultLogoURL,
"a": "b",
"c": "d",
}
msgID, err := enq.Enqueue(ctx, user.ID, notifications.TemplateWorkspaceDeleted, input, "test")
require.NoError(t, err)
Expand Down
15 changes: 1 addition & 14 deletions coderd/notifications/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ package notifications

import (
"context"
"database/sql"
"encoding/json"
"errors"
"sync"
"text/template"

Expand All @@ -26,6 +24,7 @@ import (

const (
NotificationsDefaultLogoURL = "https://coder.com/coder-logo-horizontal.png"
NotificationsDefaultAppName = "Coder"
)

// notifier is a consumer of the notifications_messages queue. It dequeues messages from that table and processes them
Expand Down Expand Up @@ -229,18 +228,6 @@ func (n *notifier) prepare(ctx context.Context, msg database.AcquireNotification
return nil, xerrors.Errorf("failed to resolve handler %q", msg.Method)
}

logoURL, err := n.store.GetLogoURL(ctx)
if err != nil && !errors.Is(err, sql.ErrNoRows) {
n.log.Error(ctx, "failed fetching logo url", slog.Error(err))
}

if logoURL == "" {
//nolint:ineffassign // define to default value if unable to fetch one from db
logoURL = NotificationsDefaultLogoURL
}

payload.Labels["_logo_url"] = logoURL

var title, body string
if title, err = render.GoTemplate(msg.TitleTemplate, payload, n.helpers); err != nil {
return nil, xerrors.Errorf("render title: %w", err)
Expand Down
2 changes: 2 additions & 0 deletions coderd/notifications/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ func defaultHelpers() map[string]any {
return map[string]any{
"base_url": func() string { return "http://test.com" },
"current_year": func() string { return "2024" },
"logo_url": func() string { return "https://logo.company" },
"app_name": func() string { return "TestCompany" },
}
}

Expand Down