Skip to content

Commit a4e5a52

Browse files
refactor(authz): remove duplicate hook and gate lock/unlock behind canEditFiles
- Remove useUserPermissionsWithAuthzCourse in favor of useCourseUserPermissions which provides the same functionality with better generic typing - Migrate all consumers (CourseFilesTable, FilesPage, header hooks) to use useCourseUserPermissions with flat destructuring - Hide Lock/Unlock option in FileMenu, MoreInfoColumn, and FileInfoModalSidebar when canEditFiles is false (course_auditor should not see lock/unlock) - Add unit tests for lock/unlock visibility based on permissions - Fix clipboard mock in tests using Object.defineProperty - Update FilesPage.test.jsx mocks to match new flat return shape
1 parent 04a8ff9 commit a4e5a52

15 files changed

Lines changed: 255 additions & 318 deletions

File tree

src/authz/hooks.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ describe('useCourseUserPermissions', () => {
4949
expect(result.current.canEdit).toBe(false);
5050
});
5151

52-
it('returns isLoading=true and no permission keys while authz permissions are loading', () => {
52+
it('returns isLoading=true and permissions as false while authz permissions are loading', () => {
5353
mockWaffleFlags({ enableAuthzCourseAuthoring: true });
5454
jest.mocked(useUserPermissions).mockReturnValue({
5555
isLoading: true,
@@ -60,8 +60,8 @@ describe('useCourseUserPermissions', () => {
6060

6161
expect(result.current.isLoading).toBe(true);
6262
expect(result.current.isAuthzEnabled).toBe(true);
63-
expect(result.current.canView).toBeUndefined();
64-
expect(result.current.canEdit).toBeUndefined();
63+
expect(result.current.canView).toBe(false);
64+
expect(result.current.canEdit).toBe(false);
6565
});
6666

6767
it('falls back to false for permissions absent from server response when authz is enabled', () => {

src/authz/hooks.test.tsx

Lines changed: 0 additions & 133 deletions
This file was deleted.

src/authz/hooks.ts

Lines changed: 9 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,6 @@ type UseCourseUserPermissionsReturn<Query extends PermissionValidationQuery> = {
77
isAuthzEnabled: boolean;
88
} & PermissionValidationAnswer<Query>;
99

10-
/**
11-
* Return type for the useUserCoursePermissionsWithAuthz hook
12-
*/
13-
interface UseUserPermissionsWithAuthzCourseReturn {
14-
/** Whether permissions are currently loading */
15-
isLoading: boolean;
16-
/** Object containing permission results with boolean values */
17-
permissions: PermissionValidationAnswer;
18-
/** Whether authorization is enabled for the course */
19-
isAuthzEnabled: boolean;
20-
}
21-
2210
/**
2311
* Custom hook to retrieve and evaluate user permissions for the current course using the openedx-authz service.
2412
*
@@ -54,96 +42,36 @@ export const useCourseUserPermissions = <Query extends PermissionValidationQuery
5442
permissions: Query,
5543
): UseCourseUserPermissionsReturn<Query> => {
5644
const waffleFlags = useWaffleFlags(courseId);
45+
const isWaffleFlagsLoading: boolean = waffleFlags?.isLoading ?? true;
5746
const isAuthzEnabled: boolean = waffleFlags?.enableAuthzCourseAuthoring ?? false;
5847

5948
const {
6049
isLoading: isLoadingUserPermissions,
6150
data: userPermissions,
6251
} = useUserPermissions(permissions, isAuthzEnabled);
6352

53+
const isLoading = isWaffleFlagsLoading || (isAuthzEnabled && isLoadingUserPermissions);
54+
6455
const resolvePermission = (key: string): boolean => {
6556
if (!isAuthzEnabled) {
6657
return true;
6758
}
6859
return userPermissions?.[key] ?? false;
6960
};
7061

71-
const permissionResults: Record<string, boolean> = isLoadingUserPermissions
72-
? {}
62+
const permissionResults: Record<string, boolean> = isLoading
63+
? Object.keys(permissions).reduce<Record<string, boolean>>((acc, key) => {
64+
acc[key] = false;
65+
return acc;
66+
}, {})
7367
: Object.keys(permissions).reduce<Record<string, boolean>>((acc, key) => {
7468
acc[key] = resolvePermission(key);
7569
return acc;
7670
}, {});
7771

7872
return {
79-
isLoading: isAuthzEnabled ? isLoadingUserPermissions : false,
73+
isLoading,
8074
isAuthzEnabled,
8175
...permissionResults as PermissionValidationAnswer<Query>,
8276
};
8377
};
84-
85-
/**
86-
* Custom hook to handle user permissions with course authorization waffle flag
87-
*
88-
* This hook abstracts the common pattern of:
89-
* 1. Checking if authz is enabled via waffle flag
90-
* 2. Fetching user permissions when authz is enabled
91-
* 3. Defaulting all permissions to true when authz is disabled
92-
* 4. Providing fallback values for undefined permissions
93-
*
94-
* @param courseId - The course ID to check permissions for
95-
* @param permissions - Object mapping permission names to their action/scope definitions
96-
* @returns Object containing loading state, permissions results, and authz status
97-
*
98-
* @example
99-
* ```tsx
100-
* const { isLoading, permissions, isAuthzEnabled } = useUserPermissionsWithAuthzCourse(
101-
* courseId,
102-
* {
103-
* canViewFiles: {
104-
* action: COURSE_PERMISSIONS.VIEW_FILES,
105-
* scope: courseId,
106-
* },
107-
* canManageFiles: {
108-
* action: COURSE_PERMISSIONS.MANAGE_FILES,
109-
* scope: courseId,
110-
* },
111-
* }
112-
* );
113-
*
114-
* const { canViewFiles, canManageFiles } = permissions;
115-
* ```
116-
*/
117-
export const useUserPermissionsWithAuthzCourse = (
118-
courseId: string,
119-
permissions: PermissionValidationQuery,
120-
): UseUserPermissionsWithAuthzCourseReturn => {
121-
const waffleFlags = useWaffleFlags(courseId);
122-
const isAuthzEnabled: boolean = waffleFlags?.enableAuthzCourseAuthoring ?? false;
123-
124-
const {
125-
isLoading: isLoadingUserPermissions,
126-
data: userPermissions,
127-
} = useUserPermissions(permissions, isAuthzEnabled);
128-
129-
// Build permission results object
130-
const permissionResults: PermissionValidationAnswer = {};
131-
132-
if (isAuthzEnabled && !isLoadingUserPermissions) {
133-
// Authz is enabled and permissions loaded, use actual permission values with fallback to false
134-
Object.keys(permissions).forEach((permissionKey: string) => {
135-
permissionResults[permissionKey] = userPermissions?.[permissionKey] ?? false;
136-
});
137-
} else if (!isLoadingUserPermissions) {
138-
// Authz is disabled or permissions still loading, default all to true
139-
Object.keys(permissions).forEach((permissionKey: string) => {
140-
permissionResults[permissionKey] = true;
141-
});
142-
}
143-
144-
return {
145-
isLoading: isAuthzEnabled ? isLoadingUserPermissions : false,
146-
permissions: permissionResults,
147-
isAuthzEnabled,
148-
};
149-
};

src/files-and-videos/files-page/CourseFilesTable.tsx

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { getFileSizeToClosestByte } from '@src/utils';
2828
import React from 'react';
2929
import { useDispatch, useSelector } from 'react-redux';
3030
import { useParams } from 'react-router-dom';
31-
import { useUserPermissionsWithAuthzCourse } from '@src/authz/hooks';
31+
import { useCourseUserPermissions } from '@src/authz/hooks';
3232
import { getFilesPermissions } from '@src/authz/permissionHelpers';
3333

3434
export const CourseFilesTable = () => {
@@ -43,8 +43,10 @@ export const CourseFilesTable = () => {
4343
} = useSelector((state: DeprecatedReduxState) => state.assets);
4444

4545
const {
46-
permissions,
47-
} = useUserPermissionsWithAuthzCourse(courseId, getFilesPermissions(courseId));
46+
canCreateFiles,
47+
canDeleteFiles,
48+
canEditFiles,
49+
} = useCourseUserPermissions(courseId, getFilesPermissions(courseId));
4850

4951
const handleErrorReset = (error) => dispatch(resetErrors(error));
5052
const handleDeleteFile = (id) => dispatch(deleteAssetFile(courseId, id));
@@ -75,7 +77,7 @@ export const CourseFilesTable = () => {
7577
FileInfoModalSidebar({
7678
asset,
7779
handleLockedAsset: handleLockFile,
78-
canLockFile: permissions.canEditFiles,
80+
canLockFile: canEditFiles,
7981
});
8082

8183
const assets = useModels('assets', assetIds);
@@ -188,9 +190,9 @@ export const CourseFilesTable = () => {
188190
infoModalSidebar,
189191
files: assets,
190192
permissions: {
191-
canCreateFiles: permissions.canCreateFiles,
192-
canDeleteFiles: permissions.canDeleteFiles,
193-
canEditFiles: permissions.canEditFiles,
193+
canCreateFiles,
194+
canDeleteFiles,
195+
canEditFiles,
194196
},
195197
}}
196198
/>

src/files-and-videos/files-page/FileInfoModalSidebar.jsx

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -79,27 +79,28 @@ const FileInfoModalSidebar = ({
7979
onClick={() => navigator.clipboard.writeText(asset?.externalUrl)}
8080
/>
8181
</ActionRow>
82-
<ActionRow className=" border-top mt-3 pt-3">
83-
<div className="font-weight-bold">
84-
<FormattedMessage {...messages.lockFileTitle} />
85-
</div>
86-
<IconButtonWithTooltip
87-
key="lock-file-info"
88-
tooltipPlacement="top"
89-
tooltipContent={intl.formatMessage(messages.lockFileTooltipContent)}
90-
src={InfoOutline}
91-
iconAs={Icon}
92-
alt="Info"
93-
size="inline"
94-
/>
95-
<ActionRow.Spacer />
96-
<CheckboxControl
97-
disabled={!canLockFile}
98-
checked={lockedState}
99-
onChange={handleLock}
100-
aria-label="Checkbox"
101-
/>
102-
</ActionRow>
82+
{canLockFile && (
83+
<ActionRow className=" border-top mt-3 pt-3">
84+
<div className="font-weight-bold">
85+
<FormattedMessage {...messages.lockFileTitle} />
86+
</div>
87+
<IconButtonWithTooltip
88+
key="lock-file-info"
89+
tooltipPlacement="top"
90+
tooltipContent={intl.formatMessage(messages.lockFileTooltipContent)}
91+
src={InfoOutline}
92+
iconAs={Icon}
93+
alt="Info"
94+
size="inline"
95+
/>
96+
<ActionRow.Spacer />
97+
<CheckboxControl
98+
checked={lockedState}
99+
onChange={handleLock}
100+
aria-label="Checkbox"
101+
/>
102+
</ActionRow>
103+
)}
103104
</Stack>
104105
);
105106
};

src/files-and-videos/files-page/FileInfoModalSidebar.test.jsx

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,20 @@ describe('FileInfoModalSidebar', () => {
4848
expect(screen.getByText('Lock file')).toBeInTheDocument();
4949
});
5050

51+
it('hides Lock file section when canLockFile is false', () => {
52+
renderComponent({ canLockFile: false });
53+
54+
expect(screen.getByText('Date added')).toBeInTheDocument();
55+
expect(screen.getByText('Studio URL')).toBeInTheDocument();
56+
expect(screen.queryByText('Lock file')).not.toBeInTheDocument();
57+
});
58+
59+
it('shows Lock file section when canLockFile is true', () => {
60+
renderComponent({ canLockFile: true });
61+
62+
expect(screen.getByText('Lock file')).toBeInTheDocument();
63+
});
64+
5165
it('displays the portable URL', () => {
5266
renderComponent();
5367
expect(screen.getByText('/static/test-file.png')).toBeInTheDocument();

0 commit comments

Comments
 (0)