Skip to content

Commit 755ecb2

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 9336988 commit 755ecb2

13 files changed

Lines changed: 241 additions & 312 deletions

File tree

src/authz/hooks.test.ts

Lines changed: 1 addition & 1 deletion
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,

src/authz/hooks.test.tsx

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

src/authz/hooks.ts

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

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();

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,13 @@ import EditFileAlertsSlot from '@src/plugin-slots/EditFileAlertsSlot';
1313
import { AlertAgreementGatedFeature } from '@src/generic/agreement-gated-feature';
1414
import { AgreementGated } from '@src/constants';
1515

16-
import { useUserPermissionsWithAuthzCourse } from '@src/authz/hooks';
16+
import { useCourseUserPermissions } from '@src/authz/hooks';
1717
import { getFilesPermissions } from '@src/authz/permissionHelpers';
1818
import PermissionDeniedAlert from '@src/generic/PermissionDeniedAlert';
1919
import { EditFileErrors } from '../generic';
2020
import { fetchAssets, resetErrors } from './data/thunks';
2121
import FilesPageProvider from './FilesPageProvider';
22+
import Loading from '@src/generic/Loading';
2223
import messages from './messages';
2324
import './FilesPage.scss';
2425

@@ -37,17 +38,17 @@ const FilesPage = () => {
3738

3839
const {
3940
isLoading: isLoadingPermissions,
40-
permissions,
41-
} = useUserPermissionsWithAuthzCourse(courseId, getFilesPermissions(courseId));
42-
43-
const {
4441
canViewFiles,
45-
} = permissions;
42+
} = useCourseUserPermissions(courseId, getFilesPermissions(courseId));
4643

4744
useEffect(() => {
4845
dispatch(fetchAssets(courseId));
4946
}, [courseId]);
5047

48+
if (isLoadingPermissions) {
49+
return <Loading />;
50+
}
51+
5152
if (!isLoadingPermissions && !canViewFiles) {
5253
return <PermissionDeniedAlert />;
5354
}

0 commit comments

Comments
 (0)