Skip to content
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

ref(dashboards): Fixes and refactoring for edit access selector button #80633

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

harshithadurai
Copy link
Contributor

@harshithadurai harshithadurai commented Nov 12, 2024

#79833

Some fixes for the Edit Access dropdown button in Dashboards.

  1. modified behaviour of Save Changes button in EditAccessSelector.tsx so that it's enabled only when the changed selections are not the same as the existing perms
  2. pass down props so that Widget edit buttons are disabled when user does not have edit perms
  3. pass down props to top level dashboard filters so that users without edit perms cannot save changes
  4. pass down props to widget context menu to disable the Edit Widget button there
Screenshot 2024-11-13 at 11 04 49 AM Screenshot 2024-11-14 at 4 12 23 PM

@harshithadurai harshithadurai requested review from a team as code owners November 12, 2024 22:45
@harshithadurai harshithadurai marked this pull request as draft November 12, 2024 22:45
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 12, 2024
Copy link

codecov bot commented Nov 12, 2024

Bundle Report

Changes will decrease total bundle size by 35.84kB (-0.11%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 31.89MB 35.84kB (-0.11%) ⬇️

Copy link

codecov bot commented Nov 12, 2024

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
8215 4 8211 0
View the top 3 failed tests by shortest run time
Dashboards > WidgetCard calls onEdit when Edit Widget is clicked
Stack Traces | 0.148s run time
Error: expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 1
Received number of calls: 0
    at Object.<anonymous> (.../dashboards/widgetCard/index.spec.tsx:351:18)
Dashboards > WidgetCard calls onDuplicate when Duplicate Widget is clicked
Stack Traces | 0.17s run time
Error: expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 1
Received number of calls: 0
    at Object.<anonymous> (.../dashboards/widgetCard/index.spec.tsx:295:18)
Dashboards > IssueWidgetCard calls onDuplicate when Duplicate Widget is clicked
Stack Traces | 0.283s run time
Error: expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 1
Received number of calls: 0
    at Object.<anonymous> (.../dashboards/widgetCard/issueWidgetCard.spec.tsx:183:18)

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

});

menuOptions.push({
key: 'edit-widget',
label: t('Edit Widget'),
onAction: () => onEdit?.(),
disabled: !hasEditAccess,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a button in the widget viewer (which opens up when you click the button next to the ... context menu) that has an edit widget button. Could we also disable that? Doesn't need to be this PR.

Comment on lines 53 to 58
if (isEqual(newSelectedValues, getSelectedOptions())) {
setHasUnsavedChanges(false);
} else {
setHasUnsavedChanges(true);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just pass isEqual(newSelectedValues, getSelectedOptions()) into disabled on line 207? so disabed={!isCurrentUserDashboardOwner || isEqual(newSelectedValues, getSelectedOptions())}, or you can just assign it to a variable instead of storing it in state like this

setHasUnsavedChanges(false);
setSelectedOptions(getSelectedOptions());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set the selected options when the menu is closed? The state should be handled by the onSelectOptions handler so opening and closing shouldn't have an effect on that

Comment on lines +56 to +67
const currentUser = useUser();
const {teams: userTeams} = useUserTeams();
let hasEditAccess = true;
if (organization.features.includes('dashboards-edit-access')) {
hasEditAccess = checkUserHasEditAccess(
currentUser,
userTeams,
organization,
dashboardPermissions,
dashboardCreator
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just pass around hasEditAccess from above? It seems like a simpler change than checking this at multiple locations.

Also, an idea for a future clean up is that I think we could put this into a context to store edit access information and create a hook called useHasEditAccess or something to avoid passing around the prop as much

@harshithadurai harshithadurai merged commit df0147f into master Nov 15, 2024
45 checks passed
@harshithadurai harshithadurai deleted the harshi/feat/dashbaords-edit-access-fixes branch November 15, 2024 16:34
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants