fix: add fallback icons for notifications #17013
Conversation
coderd/inboxnotifications.go
Outdated
| var fallbackIcons = map[uuid.UUID]string{ | ||
| // workspace related notifications | ||
| notifications.TemplateWorkspaceCreated: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceCreated: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceManuallyUpdated: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceDeleted: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceAutobuildFailed: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceDormant: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceAutoUpdated: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceMarkedForDeletion: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceManualBuildFailed: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceOutOfMemory: FallbackIconWorkspace, | ||
| notifications.TemplateWorkspaceOutOfDisk: FallbackIconWorkspace, | ||
|
|
||
| // account related notifications | ||
| notifications.TemplateUserAccountCreated: FallbackIconAccount, | ||
| notifications.TemplateUserAccountDeleted: FallbackIconAccount, | ||
| notifications.TemplateUserAccountSuspended: FallbackIconAccount, | ||
| notifications.TemplateUserAccountActivated: FallbackIconAccount, | ||
| notifications.TemplateYourAccountSuspended: FallbackIconAccount, | ||
| notifications.TemplateYourAccountActivated: FallbackIconAccount, | ||
| notifications.TemplateUserRequestedOneTimePasscode: FallbackIconAccount, | ||
|
|
||
| // template related notifications | ||
| notifications.TemplateTemplateDeleted: FallbackIconTemplate, | ||
| notifications.TemplateTemplateDeprecated: FallbackIconTemplate, | ||
| notifications.TemplateWorkspaceBuildsFailedReport: FallbackIconTemplate, | ||
|
|
||
| // other related notifications | ||
| notifications.TemplateTestNotification: FallbackIconOther, | ||
| } |
There was a problem hiding this comment.
Can you add a test to ensure that each notification template type has a fallback icon associated with it?
There was a problem hiding this comment.
Added some tests 👀
coderd/inboxnotifications.go
Outdated
| payload.InboxNotification.Icon = fallbackIcons[payload.InboxNotification.TemplateID] | ||
| } |
There was a problem hiding this comment.
Same here, we should check that there is a fallback icon defined.
Thoought: would it make sense to encapsulate this logic in a function InboxNotification.Icon()?
There was a problem hiding this comment.
I created an util function to have only one place on the which we change it.
I was willing to do it as a method for the codersdk.InboxNotification receiver, but can't as it generates import cycle with the templates. :(
coderd/inboxnotifications.go
Outdated
| // other related notifications | ||
| notifications.TemplateTestNotification: FallbackIconOther, |
There was a problem hiding this comment.
WDYT about returning FallbackIconOther if fallbackIcons[notif.TemplateID] does not exist? so eventually the order would be:
- Custom notif icon.
fallbackIcons[notif.TemplateID]FallbackIconOther
In this case we won't need to add entries to fallbackIcons if FallbackIconOther is fine, including TemplateTestNotification.
There was a problem hiding this comment.
Thanks for the idea, like it and will make it more reliable. I added a fallback logic, please tell me if that's what you had in mind.
coderd/inboxnotifications.go
Outdated
| FallbackIconWorkspace = "DEFAULT_WORKSPACE_ICON" | ||
| FallbackIconAccount = "DEFAULT_ACCOUNT_ICON" | ||
| FallbackIconTemplate = "DEFAULT_TEMPLATE_ICON" | ||
| FallbackIconOther = "DEFAULT_OTHER_ICON" |
There was a problem hiding this comment.
If these are values processed by "site", you should place them in codersdk as Enum, so they will be generated to TS.
There was a problem hiding this comment.
This is a really good idea, thanks for that - I moved it.
cc @BrunoQuaresma you would be able to use it now.
| FallbackIconOther = "DEFAULT_OTHER_ICON" | ||
| ) | ||
|
|
||
| var fallbackIcons = map[uuid.UUID]string{ |
There was a problem hiding this comment.
alternative data structure:
fallbackIcons = map[NotificationIcon][]uuid.UUID
not a must, just a preference
coderd/inboxnotifications.go
Outdated
| // other related notifications | ||
| notifications.TemplateTestNotification: FallbackIconOther, |
|
/cherry-pick release/2.21 |
Related: coder/internal#522