-
Notifications
You must be signed in to change notification settings - Fork 0
fix(dashboards): Removes edit permission on widget context menu for prebuilt dashboards #160
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(dashboards): Removes edit permission on widget context menu for prebuilt dashboards #160
Conversation
| const hasEditAccess = | ||
| checkUserHasEditAccess( | ||
| currentUser, | ||
| userTeams, | ||
| organization, | ||
| dashboardPermissions, | ||
| dashboardCreator | ||
| ) && !isPrebuiltDashboard; |
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.
[BestPractice]
The hasEditAccess logic now prevents editing on prebuilt dashboards by adding && !isPrebuiltDashboard to the permission check. This implementation looks correct and follows the existing permission pattern.
However, consider if this is the right place for this check. Currently, prebuilt dashboards will show as having edit access in the checkUserHasEditAccess function but then have it revoked at the component level. If prebuilt dashboards should never be editable regardless of user permissions, it might be cleaner to include this check inside the checkUserHasEditAccess utility function itself.
Current approach:
const hasEditAccess =
checkUserHasEditAccess(...) && !isPrebuiltDashboard;Alternative approach (in checkUserHasEditAccess function):
export function checkUserHasEditAccess(
currentUser: User,
userTeams: Team[],
organization: Organization,
dashboardPermissions?: DashboardPermissions,
dashboardCreator?: User,
isPrebuiltDashboard?: boolean
): boolean {
if (isPrebuiltDashboard) {
return false;
}
// ... rest of existing logic
}This would centralize the permission logic and make it clear that prebuilt dashboards are never editable.
Context for Agents
The `hasEditAccess` logic now prevents editing on prebuilt dashboards by adding `&& !isPrebuiltDashboard` to the permission check. This implementation looks correct and follows the existing permission pattern.
However, consider if this is the right place for this check. Currently, prebuilt dashboards will show as having edit access in the `checkUserHasEditAccess` function but then have it revoked at the component level. If prebuilt dashboards should never be editable regardless of user permissions, it might be cleaner to include this check inside the `checkUserHasEditAccess` utility function itself.
Current approach:
```typescript
const hasEditAccess =
checkUserHasEditAccess(...) && !isPrebuiltDashboard;
```
Alternative approach (in `checkUserHasEditAccess` function):
```typescript
export function checkUserHasEditAccess(
currentUser: User,
userTeams: Team[],
organization: Organization,
dashboardPermissions?: DashboardPermissions,
dashboardCreator?: User,
isPrebuiltDashboard?: boolean
): boolean {
if (isPrebuiltDashboard) {
return false;
}
// ... rest of existing logic
}
```
This would centralize the permission logic and make it clear that prebuilt dashboards are never editable.
File: static/app/views/dashboards/sortableWidget.tsx
Line: 90|
/propel review |
4 similar comments
|
/propel review |
|
/propel review |
|
/propel review |
|
/propel review |
Copied PR
Copied from getsentry#104133
Original PR: getsentry#104133