Thinking loud:

Logo URL does not seem to be a Label or Data. Maybe we need to introduce another property like Meta? Or just put it on the same level as title and body.

I agree with @mtojek here.

I don't think this should go into the payload. It's going to be the same for every notification, and if it's in the payload then we'll be storing the same URL for every notification.

Have a look at coderd/notifications/dispatch/smtp/html.gotmpl: you'll see {{ base_url }}. This is provided by a helper function, defined in cli/server.go`:

// templateHelpers builds a set of functions which can be called in templates.
// We build them here to avoid an import cycle by using coderd.Options in notifications.Manager.
// We can later use this to inject whitelabel fields when app name / logo URL are overridden. <----
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()) },
	}
}

We should create a new helper which retrieves the logo instead, I think.

Thanks, indeed it makes complete sense 🙏

I modified the approach to have both logo_url and app_name defined in the templateHelpers managinf both the normal flow, default value, and error cases. Keen to have your opinion on it.

Super 👍

This is an interesting use case. Some customers prefer to use their company logos instead of Coder default one. I think we should rather fail fast the notifier flow than default to Coder logo.

cc @mtojek @mafredri as you both raised this one. In case of errors I can :

• add default values (seems we do not want to avoid displaying Coder information to customers that prefer not.)
• skip the logo or name
• retry the notification

As this situation means that we had a failure fetching data from the db, I don't know what is the preferred solution.

This could be an option to make it less awkward: #14935 (comment)

... and just retry.

Errors in retrieving the data from the db will cause the notification to fail, and it'll be automatically retried later.
If we have a persistent issue which causes the notifications to exceed their retry limit (5 by default) then that's actually a good signal that something is wrong.

Should we present also the company name?

🤓 Will add it and present some screenshots so we can iterate with the name.

If the reason for exposing NotificationsDefaultLogoURL is just this test, I think you can safely switch to a generic http://foobar.tld/logo.png.

Was indeed not making sense at the time of your comment - good catch. Now that I changed to have these values defined in cli/server.go I need these. Anyway, I feel like it should maybe not be only related to notifications. Any idea about where I can put this kind of default values ?

Super 👍

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.

👍

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.

Good catch 👍

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 ?

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?

Nit: when passing arguments, I try think of them in order of significance.
The helpers are not the most significant argument here; they're likely the least. I'd move helpers to the end.

Nice 👍

note: my immediate thought was to use a Data URL but apparently support for this is extremely limited.

Have you validated that failures here cause a "temporary failure" so that subsequent requests are retried?