fix(site): ensure notification settings page follows RBAC correctly #19097
Conversation
Template admins have been unable to change their notification preferences via the website as we have historically hidden the settings for them. This PR ensures the frontend shows template events settings when a user has the `createTemplates` permission.
There was a problem hiding this comment.
Pull Request Overview
This PR enables template administrators to access and modify their notification preferences for template events through the frontend. Previously, these settings were hidden from users without full deployment configuration access.
- Updated notification filtering logic to show "Template Events" group to users with
createTemplatespermission - Added mock template event notification templates for testing
- Created a new story for template creators in the notifications page
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| site/src/testHelpers/entities.ts | Added mock template event notification templates for testing scenarios |
| site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx | Modified permission logic to display template events for users with createTemplates permission |
| site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.stories.tsx | Added TemplateCreator story to test the new permission scenario |
| "Workspace Events": groups["Workspace Events"], | ||
| }; | ||
|
|
||
| let displayedGroups: Record<string, NotificationTemplate[]> = { |
There was a problem hiding this comment.
[nitpick] The variable displayedGroups is initialized with only 'Workspace Events' and then potentially reassigned completely. Consider initializing it with the base case and then conditionally adding to it to make the logic clearer.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I would like to suggest a different approach:
Looking at the backend, it seems like user admins also have this exact same issue: by hiding the notifications based on the viewDeploymentConfig we're making it impossible for user admins to disable notifications they are opted into by default. That was always the wrong permission to check here, for both cases. For template notifications it's better to check createTemplates like you've done here, but we should fix users too by checking createUser.
I also don't think that a select is actually the right place to filter this, because the object wrangling we're doing is very awkward. I think it's easier to just check if a group should be rendered or not by returning early in the map later on. We can just have a little utility function that picks the right permission based on the group name, and if the permission is false then we return null.
I ended up just doing exactly all of that while looking at the code and pushed it to a branch of my own. If you want to bring that code into this branch you can just go click merge on that PR. I set this PR as the target.
Ensure template admin and user admins are able to see the correct notification groups on the notification settings page.