Skip to content

Commit a256525

Browse files
fix: adding missing tooltip to delete button and fixing race condition
1 parent df73f14 commit a256525

8 files changed

Lines changed: 231 additions & 42 deletions

File tree

src/authz-module/audit-user/index.test.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ jest.mock('@edx/frontend-component-header', () => ({
3030
jest.mock('@src/data/hooks', () => ({
3131
...jest.requireActual('@src/data/hooks'),
3232
useUserAccount: jest.fn(),
33-
useValidateUserPermissions: jest.fn().mockReturnValue({
34-
data: [{ allowed: true }],
33+
useValidateUserPermissionsNonSuspense: jest.fn().mockReturnValue({
34+
data: [{ scope: 'lib:test', allowed: true }],
3535
isLoading: false,
3636
}),
3737
}));

src/authz-module/audit-user/index.tsx

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import {
55
import { useIntl } from '@edx/frontend-platform/i18n';
66
import { AppContext } from '@edx/frontend-platform/react';
77
import type { AppContextType } from '@edx/frontend-platform/react';
8-
import debounce from 'lodash.debounce';
98
import {
109
Container, DataTable,
1110
} from '@openedx/paragon';
@@ -15,7 +14,7 @@ import {
1514
} from '@src/authz-module/constants';
1615
import AuthZLayout from '@src/authz-module/components/AuthZLayout';
1716
import { useNavigate, useParams } from 'react-router-dom';
18-
import { useUserAccount, useValidateUserPermissions } from '@src/data/hooks';
17+
import { useUserAccount, useValidateUserPermissionsNonSuspense } from '@src/data/hooks';
1918
import baseMessages from '@src/authz-module/messages';
2019
import AddRoleButton from '@src/authz-module/components/AddRoleButton';
2120
import {
@@ -59,9 +58,25 @@ const AuditUserPage = () => {
5958
return uniqueScopes.map(scope => getScopeManageActionPermission(scope));
6059
}, [userAssignments]);
6160

62-
const { data: permissionsToManageScope } = useValidateUserPermissions(deletePermissions);
61+
const {
62+
data: permissionsToManageScope,
63+
} = useValidateUserPermissionsNonSuspense(deletePermissions);
64+
65+
const rowsWithPermissions = useMemo(() => {
66+
if (!permissionsToManageScope) { return userAssignments; }
67+
68+
return userAssignments.map(assignment => {
69+
const canManageScope = permissionsToManageScope.some(
70+
permission => permission.scope === assignment.scope && permission.allowed,
71+
);
72+
return {
73+
...assignment,
74+
canManageScope,
75+
};
76+
});
77+
}, [userAssignments, permissionsToManageScope]);
6378

64-
const fetchData = useMemo(() => debounce(handleTableFetch, 500), [handleTableFetch]);
79+
const fetchData = useMemo(() => handleTableFetch, [handleTableFetch]);
6580

6681
useEffect(() => {
6782
if (!user && !isLoadingUser) {
@@ -72,20 +87,13 @@ const AuditUserPage = () => {
7287
}
7388
}, [user, isLoadingUser, navigate, isErrorUser, errorUser]);
7489

75-
useEffect(() => () => fetchData.cancel(), [fetchData]);
76-
7790
const handleShowConfirmDeletionModal = useCallback((role: RoleToDelete) => {
7891
if (isRevokingUserRolePending) { return; }
7992

8093
setRoleToDelete(role);
8194
setShowConfirmDeletionModal(true);
8295
}, [isRevokingUserRolePending]);
8396

84-
const hasPermissionToDeleteScope = useCallback((scope: string) => {
85-
const permissionIndex = deletePermissions.findIndex(permission => permission.scope === scope);
86-
return permissionsToManageScope?.[permissionIndex]?.allowed;
87-
}, [deletePermissions, permissionsToManageScope]);
88-
8997
const navLinks = useMemo(() => [
9098
{
9199
label: formatMessage(baseMessages['authz.management.home.nav.link']),
@@ -105,10 +113,9 @@ const AuditUserPage = () => {
105113
Cell: createActionsCell({
106114
onClickDeleteButton: handleShowConfirmDeletionModal,
107115
isUserAuthenticatedPage: username === authenticatedUser.username,
108-
hasPermissionToDeleteScope,
109116
}),
110117
},
111-
], [authenticatedUser.username, formatMessage, handleShowConfirmDeletionModal, hasPermissionToDeleteScope, username]);
118+
], [authenticatedUser.username, formatMessage, handleShowConfirmDeletionModal, username]);
112119

113120
const columns = useMemo(() => [
114121
{
@@ -242,7 +249,7 @@ const AuditUserPage = () => {
242249
isFilterable
243250
isSortable
244251
manualPagination
245-
data={userAssignments}
252+
data={rowsWithPermissions}
246253
manualFilters
247254
manualSortBy
248255
fetchData={fetchData}

src/authz-module/components/TableCells.test.tsx

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,7 @@ describe('TableCells Components', () => {
486486
org: 'Test Org',
487487
scope: 'Test Scope',
488488
permissionCount: 1,
489+
canManageScope: true,
489490
},
490491
};
491492

@@ -497,7 +498,6 @@ describe('TableCells Components', () => {
497498
const user = userEvent.setup();
498499
const CustomActionsCell = createActionsCell({
499500
onClickDeleteButton: mockOnClickDeleteButton,
500-
hasPermissionToDeleteScope: () => true,
501501
isUserAuthenticatedPage: false,
502502
});
503503
renderWrapper(<CustomActionsCell row={baseRow} column={{ id: 'actions' }} />);
@@ -509,7 +509,7 @@ describe('TableCells Components', () => {
509509
expect(mockOnClickDeleteButton).toHaveBeenCalledWith({ name: 'Library Admin', role: 'library_admin', scope: 'Test Scope' });
510510
});
511511

512-
it('renders a disabled button for admin roles when isUserAuthenticatedPage is true', () => {
512+
it('renders a disabled delete icon for admin roles when isUserAuthenticatedPage is true', () => {
513513
const adminRow = {
514514
original: {
515515
role: 'course_admin',
@@ -521,12 +521,32 @@ describe('TableCells Components', () => {
521521
const CustomActionsCell = createActionsCell({
522522
onClickDeleteButton: mockOnClickDeleteButton,
523523
isUserAuthenticatedPage: true,
524-
hasPermissionToDeleteScope: () => true,
525524
});
526525
renderWrapper(<CustomActionsCell row={adminRow} column={{ id: 'actions' }} />);
527526

528-
const button = screen.getByRole('button', { name: /delete role action/i });
529-
expect(button).toBeDisabled();
527+
const infoIcon = screen.getByRole('img', { hidden: true });
528+
expect(infoIcon).toBeInTheDocument();
529+
});
530+
531+
it('renders a tooltip when hovering over delete icon for admin roles when isUserAuthenticatedPage is true', async () => {
532+
const adminRow = {
533+
original: {
534+
role: 'course_admin',
535+
org: 'Test Org',
536+
scope: 'Test Scope',
537+
permissionCount: 1,
538+
},
539+
};
540+
const user = userEvent.setup();
541+
const CustomActionsCell = createActionsCell({
542+
onClickDeleteButton: mockOnClickDeleteButton,
543+
isUserAuthenticatedPage: true,
544+
});
545+
renderWrapper(<CustomActionsCell row={adminRow} column={{ id: 'actions' }} />);
546+
547+
const infoIcon = screen.getByRole('img', { hidden: true });
548+
await user.hover(infoIcon);
549+
expect(screen.getByText(/You cant remove your own admin role/i)).toBeInTheDocument();
530550
});
531551

532552
it('renders info icon with tooltip for Django managed roles', async () => {
@@ -541,7 +561,6 @@ describe('TableCells Components', () => {
541561
const user = userEvent.setup();
542562
const CustomActionsCell = createActionsCell({
543563
onClickDeleteButton: mockOnClickDeleteButton,
544-
hasPermissionToDeleteScope: () => true,
545564
isUserAuthenticatedPage: true,
546565
});
547566
renderWrapper(<CustomActionsCell row={djangoRow} column={{ id: 'actions' }} />);
@@ -556,9 +575,14 @@ describe('TableCells Components', () => {
556575
const CustomActionsCell = createActionsCell({
557576
onClickDeleteButton: mockOnClickDeleteButton,
558577
isUserAuthenticatedPage: false,
559-
hasPermissionToDeleteScope: () => false,
560578
});
561-
renderWrapper(<CustomActionsCell row={baseRow} column={{ id: 'actions' }} />);
579+
const customRow = {
580+
original: {
581+
...baseRow,
582+
canManageScope: false,
583+
},
584+
};
585+
renderWrapper(<CustomActionsCell row={customRow} column={{ id: 'actions' }} />);
562586

563587
const deleteButton = screen.queryByRole('button', { name: /delete role action/i });
564588
expect(deleteButton).toBeDisabled();

src/authz-module/components/TableCells.tsx

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
Info,
77
} from '@openedx/paragon/icons';
88
import {
9-
TableCellValue, AppContextType, UserRole, RoleToDelete,
9+
TableCellValue, AppContextType, UserRoleWithPermissions, RoleToDelete,
1010
} from '@src/types';
1111
import { useNavigate } from 'react-router-dom';
1212
import { useContext, useMemo } from 'react';
@@ -27,7 +27,7 @@ interface DataTableInstance {
2727
toggleRowExpanded?: (rowId: string, expanded: boolean) => void;
2828
}
2929

30-
type CellProps = TableCellValue<UserRole>;
30+
type CellProps = TableCellValue<UserRoleWithPermissions>;
3131
type CellPropsWithValue = CellProps & {
3232
value: string;
3333
};
@@ -39,7 +39,6 @@ type ExtendedCellProps = CellPropsWithValue & {
3939

4040
type ActionsCellExtraProps = {
4141
onClickDeleteButton: (role: RoleToDelete) => void;
42-
hasPermissionToDeleteScope: (scope: string) => boolean;
4342
isUserAuthenticatedPage: boolean;
4443
};
4544

@@ -164,12 +163,10 @@ const ViewAllPermissionsCell = ({ row }: CellProps) => {
164163
};
165164

166165
const ActionsCell = ({
167-
row, onClickDeleteButton, isUserAuthenticatedPage, hasPermissionToDeleteScope,
166+
row, onClickDeleteButton, isUserAuthenticatedPage,
168167
}: ActionsCellProps) => {
169168
const { formatMessage } = useIntl();
170-
const { role, scope } = row.original;
171-
172-
const hasPermissionsToDelete = useMemo(() => hasPermissionToDeleteScope(scope), [hasPermissionToDeleteScope, scope]);
169+
const { role, canManageScope } = row.original;
173170

174171
const handleDelete = () => {
175172
const roleToDelete = {
@@ -200,21 +197,26 @@ const ActionsCell = ({
200197

201198
if (ADMIN_ROLES.includes(role) && isUserAuthenticatedPage) {
202199
return (
203-
<IconButton
204-
// @ts-ignore
205-
disabled
206-
isActive={false}
207-
variant="light"
208-
alt={formatMessage(messages['authz.user.table.delete.action.alt'])}
209-
src={Delete}
210-
/>
200+
<OverlayTrigger
201+
placement="left"
202+
overlay={(
203+
<Tooltip variant="light" id="tooltip-left">
204+
{formatMessage(messages['authz.user.table.delete.action.adminrole.tooltip'])}
205+
</Tooltip>
206+
)}
207+
>
208+
<Icon
209+
className="mx-2 pl-1 text-light-500"
210+
src={Delete}
211+
/>
212+
</OverlayTrigger>
211213
);
212214
}
213215

214216
return (
215217
<IconButton
216-
disabled={!hasPermissionsToDelete}
217-
variant={hasPermissionsToDelete ? 'danger' : 'light'}
218+
disabled={!canManageScope}
219+
variant={canManageScope ? 'danger' : 'light'}
218220
onClick={handleDelete}
219221
alt={formatMessage(messages['authz.user.table.delete.action.alt'])}
220222
src={Delete}

src/authz-module/components/messages.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,11 @@ const messages = defineMessages({
157157
defaultMessage: 'You can’t remove this role here. Please go to Django Admin to manage it.',
158158
description: 'Tooltip for delete button when hovering over Django roles',
159159
},
160+
'authz.user.table.delete.action.adminrole.tooltip': {
161+
id: 'authz.user.table.delete.action.adminrole.tooltip',
162+
defaultMessage: 'You can’t remove your own admin role. This prevents a resource from being left without an admin. Another user with the required permissions can revoke it.',
163+
description: 'Tooltip for delete button when hovering over Admin roles',
164+
},
160165
'authz.user.table.view_all_permissions.link.text.close': {
161166
id: 'authz.user.table.view_all_permissions.link.text.close',
162167
defaultMessage: 'Hide all permissions',

0 commit comments

Comments
 (0)