Should we present also the company name?

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

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.

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 👍