feat(dashboards): frontend to restrict dashboard edit access to teams #80344
Conversation
github-actions
bot
added
the
Scope: Frontend
This comment was marked as outdated.
This comment was marked as outdated.
Bundle ReportChanges will increase total bundle size by 43.95kB (0.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
| ): Promise<DashboardDetails> { | ||
| const {projects, environment, permissions} = dashboard; | ||
| const data = { | ||
| projects, |
There was a problem hiding this comment.
these fields seem to be necessary to pass some backend validation (projects and environment)
There was a problem hiding this comment.
Let's just add a small note to explain why that's there. Just curious, what's the error you're seeing? Seems like it's something that we shouldn't be throwing
narsaynorath
left a comment
narsaynorath
left a comment
There was a problem hiding this comment.
Just some comments on what I noticed from the different states! This is mostly good to go, just let me know if you want to work on those feedback items in this PR or in other PRs. None of my comments are blockers and can be iterated on. I'd like to see this land and we turn the feature flag back on for the team while this stuff gets worked out
| ): Promise<DashboardDetails> { | ||
| const {projects, environment, permissions} = dashboard; | ||
| const data = { | ||
| projects, |
There was a problem hiding this comment.
Let's just add a small note to explain why that's there. Just curious, what's the error you're seeing? Seems like it's something that we shouldn't be throwing
| return dashboard.permissions.isEditableByEveryone | ||
| ? dashboard.permissions.isEditableByEveryone | ||
| : dashboard.createdBy?.id === currentUser.id; |
There was a problem hiding this comment.
Should this function also check if the user is also in the included teams?
| onClick={() => { | ||
| setMenuOpen(false); | ||
| }} |
There was a problem hiding this comment.
I expected clicking "Cancel" would reset my selected state, can we add reset functionality here? We can follow it up in another PR if you want, not a blocker.
There was a problem hiding this comment.
Just wanted to note that I noticed opening and closing the selector reset my state, but cancelling didn't. I think we should make these two behaviours consistent 🙏
| }} | ||
| disabled={!isCurrentUserDashboardOwner} | ||
| > | ||
| {t('Cancel')}{' '} |
There was a problem hiding this comment.
| {t('Cancel')}{' '} | |
| {t('Cancel')} |
| if (!isDefaultState && !isEqual(newDashboardPermissions, dashboard.permissions)) { | ||
| onChangeEditAccess?.(newDashboardPermissions); | ||
| } | ||
| setHasUnsavedChanges(true); |
There was a problem hiding this comment.
Instead of storing this state, is there a way we can derive it from comparing the dashboard prop and the state? e.g. if I select a team and unselect it to return to the original state, it still shows me in an unsaved state when saving wouldn't really change anything.
I think this would be similar to the logic you have for checking when to fire the request. You could possibly extract that logic into a helper
harshithadurai
deleted the
harshi/ref/dashboards-edit-access-frontend-modifications
branch
Addresses: #79828 and #79833
Corresponding backend change: #80136
EditAccessSelectorcomponentisCreatorOnlyEditableto useisEditableByEveryoneupdateDashbaordPermissions()function to make a separatePUTrequest to dashboard details endpoint that sends only the perms objectBefore:
After: