- Notifications
You must be signed in to change notification settings - Fork906
fix: add fallback icons for notifications#17013
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added some tests 👀
Uh oh!
There was an error while loading.Please reload this page.
coderd/inboxnotifications.go Outdated
| payload.InboxNotification.Icon = fallbackIcons[payload.InboxNotification.TemplateID] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here, we should check that there is a fallback icon defined.
Thoought: would it make sense to encapsulate this logic in a functionInboxNotification.Icon()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thecodersdk.InboxNotification receiver, but can't as it generates import cycle with the templates. :(
Uh oh!
There was an error while loading.Please reload this page.
coderd/inboxnotifications.go Outdated
| // other related notifications | ||
| notifications.TemplateTestNotification: FallbackIconOther, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
WDYT about returningFallbackIconOther iffallbackIcons[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, includingTemplateTestNotification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@defelmnq is it still a draft PR?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a really good idea, thanks for that - I moved it.
cc@BrunoQuaresma you would be able to use it now.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| FallbackIconOther = "DEFAULT_OTHER_ICON" | ||
| ) | ||
| var fallbackIcons = map[uuid.UUID]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
alternative data structure:
fallbackIcons = map[NotificationIcon][]uuid.UUID
not a must, just a preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you!!
Uh oh!
There was an error while loading.Please reload this page.
coderd/inboxnotifications.go Outdated
| // other related notifications | ||
| notifications.TemplateTestNotification: FallbackIconOther, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, thank you!
Uh oh!
There was an error while loading.Please reload this page.
148dae1 intomainUh oh!
There was an error while loading.Please reload this page.
/cherry-pick release/2.21 |
Uh oh!
There was an error while loading.Please reload this page.
Related:coder/internal#522