fix(notifications): improve tests and some nit fixes

coder · defelmnq · Oct 22, 2024 · Oct 2, 2024 · Oct 3, 2024 · Oct 3, 2024
commit 4c5cb3d94d6e68eb42d7515e4e1c1c38fad4653f
diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go
@@ -61,7 +61,7 @@ func NewSMTPHandler(cfg codersdk.NotificationsEmailConfig, log slog.Logger) *SMT
 	return &SMTPHandler{cfg: cfg, log: log}
 }
 
-func (s *SMTPHandler) Dispatcher(helpers template.FuncMap, payload types.MessagePayload, titleTmpl, bodyTmpl string) (DeliveryFunc, error) {
+func (s *SMTPHandler) Dispatcher(payload types.MessagePayload, titleTmpl, bodyTmpl string, helpers template.FuncMap) (DeliveryFunc, error) {
 	// First render the subject & body into their own discrete strings.
 	subject, err := markdown.PlaintextFromMarkdown(titleTmpl)
 	if err != nil {

diff --git a/coderd/notifications/dispatch/smtp_test.go b/coderd/notifications/dispatch/smtp_test.go
@@ -480,7 +480,7 @@ func TestSMTP(t *testing.T) {
 				Labels:    make(map[string]string),
 			}
 
-			dispatchFn, err := handler.Dispatcher(helpers, payload, subject, body)
+			dispatchFn, err := handler.Dispatcher(payload, subject, body, helpers())
 			require.NoError(t, err)
 
 			msgID := uuid.New()

diff --git a/coderd/notifications/dispatch/utils_test.go b/coderd/notifications/dispatch/utils_test.go
@@ -1,8 +1,10 @@
 package dispatch_test
 
-var helpers = map[string]any{
-	"base_url":     func() string { return "http://test.com" },
-	"current_year": func() string { return "2024" },
-	"logo_url":     func() string { return "https://coder.com/coder-logo-horizontal.png" },
-	"app_name":     func() string { return "Coder" },
+func helpers() 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://coder.com/coder-logo-horizontal.png" },
+		"app_name":     func() string { return "Coder" },
+	}
 }
diff --git a/coderd/notifications/dispatch/webhook.go b/coderd/notifications/dispatch/webhook.go
@@ -42,7 +42,7 @@ func NewWebhookHandler(cfg codersdk.NotificationsWebhookConfig, log slog.Logger)
 	return &WebhookHandler{cfg: cfg, log: log, cl: &http.Client{}}
 }
 
-func (w *WebhookHandler) Dispatcher(_ template.FuncMap, payload types.MessagePayload, titleMarkdown, bodyMarkdown string) (DeliveryFunc, error) {
+func (w *WebhookHandler) Dispatcher(payload types.MessagePayload, titleMarkdown, bodyMarkdown string, _ template.FuncMap) (DeliveryFunc, error) {
 	if w.cfg.Endpoint.String() == "" {
 		return nil, xerrors.New("webhook endpoint not defined")
 	}

diff --git a/coderd/notifications/dispatch/webhook_test.go b/coderd/notifications/dispatch/webhook_test.go
@@ -141,7 +141,7 @@ func TestWebhook(t *testing.T) {
 				Endpoint: *serpent.URLOf(endpoint),
 			}
 			handler := dispatch.NewWebhookHandler(cfg, logger.With(slog.F("test", tc.name)))
-			deliveryFn, err := handler.Dispatcher(helpers, msgPayload, titleMarkdown, bodyMarkdown)
+			deliveryFn, err := handler.Dispatcher(msgPayload, titleMarkdown, bodyMarkdown, helpers())
 			require.NoError(t, err)
 
 			retryable, err := deliveryFn(ctx, msgID)

diff --git a/coderd/notifications/fetcher.go b/coderd/notifications/fetcher.go
@@ -4,17 +4,39 @@ import (
 	"context"
 	"database/sql"
 	"errors"
+	"text/template"
 
 	"golang.org/x/xerrors"
 )
 
+func (n *notifier) fetchHelpers(ctx context.Context) (map[string]any, error) {
+	appName, err := n.fetchAppName(ctx)
+	if err != nil {
+		return nil, xerrors.Errorf("fetch app name: %w", err)
+	}
+	logoURL, err := n.fetchLogoURL(ctx)
+	if err != nil {
+		return nil, xerrors.Errorf("fetch logo URL: %w", err)
+	}
+
+	helpers := make(template.FuncMap)
+	for k, v := range n.helpers {
+		helpers[k] = v
+	}
+
+	helpers["app_name"] = func() string { return appName }
+	helpers["logo_url"] = func() string { return logoURL }
+
+	return helpers, nil
+}
+
 func (n *notifier) fetchAppName(ctx context.Context) (string, error) {
 	appName, err := n.store.GetApplicationName(ctx)
 	if err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
 			return notificationsDefaultAppName, nil
 		}
-		return "", xerrors.Errorf("get organization: %w", err)
+		return "", xerrors.Errorf("get application name: %w", err)
 	}
 	return appName, nil
 }

diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go
@@ -206,7 +206,7 @@ type santaHandler struct {
 	nice    atomic.Int32
 }
 
-func (s *santaHandler) Dispatcher(_ template.FuncMap, payload types.MessagePayload, _, _ string) (dispatch.DeliveryFunc, error) {
+func (s *santaHandler) Dispatcher(payload types.MessagePayload, _, _ string, _ template.FuncMap) (dispatch.DeliveryFunc, error) {
 	return func(_ context.Context, _ uuid.UUID) (retryable bool, err error) {
 		if payload.Labels["nice"] != "true" {
 			s.naughty.Add(1)

diff --git a/coderd/notifications/metrics_test.go b/coderd/notifications/metrics_test.go
@@ -516,8 +516,8 @@ func newBarrierHandler(total int, handler notifications.Handler) *barrierHandler
 	}
 }
 
-func (bh *barrierHandler) Dispatcher(helpers template.FuncMap, payload types.MessagePayload, title, body string) (dispatch.DeliveryFunc, error) {
-	deliverFn, err := bh.h.Dispatcher(helpers, payload, title, body)
+func (bh *barrierHandler) Dispatcher(payload types.MessagePayload, title, body string, helpers template.FuncMap) (dispatch.DeliveryFunc, error) {
+	deliverFn, err := bh.h.Dispatcher(payload, title, body, helpers)
 	if err != nil {
 		return nil, err
 	}

diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go
@@ -1594,7 +1594,7 @@ type fakeHandler struct {
 	succeeded, failed []string
 }
 
-func (f *fakeHandler) Dispatcher(_ template.FuncMap, payload types.MessagePayload, _, _ string) (dispatch.DeliveryFunc, error) {
+func (f *fakeHandler) Dispatcher(payload types.MessagePayload, _, _ string, _ template.FuncMap) (dispatch.DeliveryFunc, error) {
 	return func(_ context.Context, msgID uuid.UUID) (retryable bool, err error) {
 		f.mu.Lock()
 		defer f.mu.Unlock()

diff --git a/coderd/notifications/notifier.go b/coderd/notifications/notifier.go
@@ -228,22 +228,10 @@ func (n *notifier) prepare(ctx context.Context, msg database.AcquireNotification
 		return nil, xerrors.Errorf("failed to resolve handler %q", msg.Method)
 	}
 
-	helpers := make(template.FuncMap)
-	for k, v := range n.helpers {
-		helpers[k] = v
-	}
-
-	appName, err := n.fetchAppName(ctx)
+	helpers, err := n.fetchHelpers(ctx)
 	if err != nil {
-		return nil, xerrors.Errorf("fetch app name: %w", err)
+		return nil, xerrors.Errorf("fetch helpers: %w", err)
 	}
-	logoURL, err := n.fetchLogoURL(ctx)
-	if err != nil {
-		return nil, xerrors.Errorf("fetch logo URL: %w", err)
-	}
-
-	helpers["app_name"] = func() string { return appName }
-	helpers["logo_url"] = func() string { return logoURL }
 
 	var title, body string
 	if title, err = render.GoTemplate(msg.TitleTemplate, payload, helpers); err != nil {
@@ -253,7 +241,7 @@ func (n *notifier) prepare(ctx context.Context, msg database.AcquireNotification
 		return nil, xerrors.Errorf("render body: %w", err)
 	}
 
-	return handler.Dispatcher(helpers, payload, title, body)
+	return handler.Dispatcher(payload, title, body, helpers)
 }
 
 // deliver sends a given notification message via its defined method.

diff --git a/coderd/notifications/spec.go b/coderd/notifications/spec.go
@@ -30,7 +30,7 @@ type Store interface {
 // Handler is responsible for preparing and delivering a notification by a given method.
 type Handler interface {
 	// Dispatcher constructs a DeliveryFunc to be used for delivering a notification via the chosen method.
-	Dispatcher(helpers template.FuncMap, payload types.MessagePayload, title, body string) (dispatch.DeliveryFunc, error)
+	Dispatcher(payload types.MessagePayload, title, body string, helpers template.FuncMap) (dispatch.DeliveryFunc, error)
 }
 
 // Enqueuer enqueues a new notification message in the store and returns its ID, should it enqueue without failure.

diff --git a/coderd/notifications/utils_test.go b/coderd/notifications/utils_test.go
@@ -70,9 +70,9 @@ func newDispatchInterceptor(h notifications.Handler) *dispatchInterceptor {
 	return &dispatchInterceptor{handler: h}
 }
 
-func (i *dispatchInterceptor) Dispatcher(_ template.FuncMap, payload types.MessagePayload, title, body string) (dispatch.DeliveryFunc, error) {
+func (i *dispatchInterceptor) Dispatcher(payload types.MessagePayload, title, body string, _ template.FuncMap) (dispatch.DeliveryFunc, error) {
 	return func(ctx context.Context, msgID uuid.UUID) (retryable bool, err error) {
-		deliveryFn, err := i.handler.Dispatcher(defaultHelpers(), payload, title, body)
+		deliveryFn, err := i.handler.Dispatcher(payload, title, body, defaultHelpers())
 		if err != nil {
 			return false, err
 		}