Skip to content

Commit 8e44be1

Browse files
refactor: replace useUserPermissionsWithAuthzCourse with useCourseUserPermissions
Remove the deprecated useUserPermissionsWithAuthzCourse hook and migrate all consumers to use useCourseUserPermissions, which provides a flatter API by spreading permission booleans directly into the return object.
1 parent 5dedece commit 8e44be1

7 files changed

Lines changed: 88 additions & 105 deletions

File tree

src/authz/hooks.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -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: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
55
import * as authzApi from '@src/authz/data/api';
66
import { PermissionValidationQuery } from '@src/authz/types';
77
import { mockWaffleFlags } from '@src/data/apiHooks.mock';
8-
import { useUserPermissionsWithAuthzCourse } from './hooks';
8+
import { useCourseUserPermissions } from './hooks';
99

1010
jest.mock('@src/data/api');
1111
jest.mock('@src/authz/data/api');
1212

1313
const mockedAuthzApi = jest.mocked(authzApi);
1414

15-
describe('useUserPermissionsWithAuthzCourse', () => {
15+
describe('useCourseUserPermissions', () => {
1616
let queryClient: QueryClient;
1717

1818
const createWrapper = () =>
@@ -50,7 +50,7 @@ describe('useUserPermissionsWithAuthzCourse', () => {
5050
});
5151

5252
const { result } = renderHook(
53-
() => useUserPermissionsWithAuthzCourse('course-v1:Test+101+2023', mockPermissions),
53+
() => useCourseUserPermissions('course-v1:Test+101+2023', mockPermissions),
5454
{ wrapper: createWrapper() },
5555
);
5656

@@ -59,8 +59,8 @@ describe('useUserPermissionsWithAuthzCourse', () => {
5959
});
6060

6161
expect(result.current.isAuthzEnabled).toBe(false);
62-
expect(result.current.permissions.canViewFiles).toBe(true);
63-
expect(result.current.permissions.canManageFiles).toBe(true);
62+
expect(result.current.canViewFiles).toBe(true);
63+
expect(result.current.canManageFiles).toBe(true);
6464
});
6565

6666
it('returns loading state when authz is enabled and permissions are loading', async () => {
@@ -73,7 +73,7 @@ describe('useUserPermissionsWithAuthzCourse', () => {
7373
);
7474

7575
const { result } = renderHook(
76-
() => useUserPermissionsWithAuthzCourse('course-v1:Test+101+2023', mockPermissions),
76+
() => useCourseUserPermissions('course-v1:Test+101+2023', mockPermissions),
7777
{ wrapper: createWrapper() },
7878
);
7979

@@ -82,6 +82,8 @@ describe('useUserPermissionsWithAuthzCourse', () => {
8282
});
8383

8484
expect(result.current.isLoading).toBe(true);
85+
expect(result.current.canViewFiles).toBe(false);
86+
expect(result.current.canManageFiles).toBe(false);
8587
});
8688

8789
it('returns actual permission values when authz is enabled and permissions loaded', async () => {
@@ -95,7 +97,7 @@ describe('useUserPermissionsWithAuthzCourse', () => {
9597
});
9698

9799
const { result } = renderHook(
98-
() => useUserPermissionsWithAuthzCourse('course-v1:Test+101+2023', mockPermissions),
100+
() => useCourseUserPermissions('course-v1:Test+101+2023', mockPermissions),
99101
{ wrapper: createWrapper() },
100102
);
101103

@@ -104,8 +106,8 @@ describe('useUserPermissionsWithAuthzCourse', () => {
104106
});
105107

106108
expect(result.current.isAuthzEnabled).toBe(true);
107-
expect(result.current.permissions.canViewFiles).toBe(true);
108-
expect(result.current.permissions.canManageFiles).toBe(false);
109+
expect(result.current.canViewFiles).toBe(true);
110+
expect(result.current.canManageFiles).toBe(false);
109111
});
110112

111113
it('falls back to false for undefined permissions when authz is enabled', async () => {
@@ -118,7 +120,7 @@ describe('useUserPermissionsWithAuthzCourse', () => {
118120
});
119121

120122
const { result } = renderHook(
121-
() => useUserPermissionsWithAuthzCourse('course-v1:Test+101+2023', mockPermissions),
123+
() => useCourseUserPermissions('course-v1:Test+101+2023', mockPermissions),
122124
{ wrapper: createWrapper() },
123125
);
124126

@@ -127,7 +129,7 @@ describe('useUserPermissionsWithAuthzCourse', () => {
127129
});
128130

129131
expect(result.current.isAuthzEnabled).toBe(true);
130-
expect(result.current.permissions.canViewFiles).toBe(true);
131-
expect(result.current.permissions.canManageFiles).toBe(false);
132+
expect(result.current.canViewFiles).toBe(true);
133+
expect(result.current.canManageFiles).toBe(false);
132134
});
133135
});

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) {
133-
// Authz is disabled, default all permissions to true immediately
134-
Object.keys(permissions).forEach((permissionKey: string) => {
135-
permissionResults[permissionKey] = true;
136-
});
137-
} else if (!isLoadingUserPermissions) {
138-
// Authz is enabled and permissions loaded, use actual permission values with fallback to false
139-
Object.keys(permissions).forEach((permissionKey: string) => {
140-
permissionResults[permissionKey] = userPermissions?.[permissionKey] ?? false;
141-
});
142-
}
143-
144-
return {
145-
isLoading: isAuthzEnabled ? isLoadingUserPermissions : false,
146-
permissions: permissionResults,
147-
isAuthzEnabled,
148-
};
149-
};

src/course-updates/CourseUpdates.test.tsx

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
fireEvent,
99
screen,
1010
} from '@src/testUtils';
11+
import userEvent from '@testing-library/user-event';
1112

1213
import { useUserPermissions } from '@src/authz/data/apiHooks';
1314

@@ -42,7 +43,7 @@ jest.mock('@src/authz/data/apiHooks', () => ({
4243

4344
jest.mock('@src/data/apiHooks', () => ({
4445
...jest.requireActual('@src/data/apiHooks'),
45-
useWaffleFlags: jest.fn(() => ({ enableAuthzCourseAuthoring: false })),
46+
useWaffleFlags: jest.fn(() => ({ enableAuthzCourseAuthoring: false, isLoading: false })),
4647
}));
4748

4849
jest.mock('react-router-dom', () => ({
@@ -403,6 +404,15 @@ describe('<CourseUpdates />', () => {
403404
expect(await screen.findAllByRole('button', { name: /edit/i })).toHaveLength(4); // 3 for course updates and 1 for handouts
404405
expect(await screen.findAllByRole('button', { name: /delete/i })).toHaveLength(3);
405406
});
407+
408+
it('should open delete modal when clicking delete button on a course update', async () => {
409+
render(<RootWrapper />);
410+
411+
const deleteButtons = await screen.findAllByTestId('course-update-delete-button');
412+
await userEvent.click(deleteButtons[0]);
413+
414+
expect(screen.getByText('Are you sure you want to delete this update?')).toBeInTheDocument();
415+
});
406416
});
407417

408418
describe('when user does NOT have permission to manage course updates and enableAuthzCourseAuthoring is enabled', () => {
@@ -479,6 +489,35 @@ describe('<CourseUpdates />', () => {
479489
});
480490
});
481491

492+
describe('when permissions are still loading', () => {
493+
beforeEach(() => {
494+
const mocks = initializeMocks();
495+
store = mocks.reduxStore;
496+
axiosMock = mocks.axiosMock;
497+
498+
(apiHooks.useWaffleFlags as jest.Mock).mockReturnValue({ enableAuthzCourseAuthoring: true });
499+
(useUserPermissions as jest.Mock).mockReturnValue({
500+
isLoading: true,
501+
data: undefined,
502+
});
503+
504+
axiosMock
505+
.onGet(getCourseUpdatesApiUrl(courseId))
506+
.reply(200, courseUpdatesMock);
507+
axiosMock
508+
.onGet(getCourseHandoutApiUrl(courseId))
509+
.reply(200, courseHandoutsMock);
510+
});
511+
512+
it('should render loading spinner while permissions are loading', () => {
513+
render(<RootWrapper />);
514+
515+
expect(screen.getByRole('status')).toBeInTheDocument();
516+
expect(screen.queryByText(messages.headingTitle.defaultMessage)).not.toBeInTheDocument();
517+
expect(screen.queryByRole('button', { name: messages.newUpdateButton.defaultMessage })).not.toBeInTheDocument();
518+
});
519+
});
520+
482521
describe('when user does NOT have permission to view course updates', () => {
483522
beforeEach(() => {
484523
const mocks = initializeMocks();

src/course-updates/CourseUpdates.tsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ import { matchesAnyStatus } from './utils';
3131
import getPageHeadTitle from '../generic/utils';
3232
import AlertMessage from '../generic/alert-message';
3333
import { getCourseUpdatesPermissions } from '@src/authz/permissionHelpers';
34-
import { useUserPermissionsWithAuthzCourse } from '@src/authz/hooks';
34+
import { useCourseUserPermissions } from '@src/authz/hooks';
35+
import Loading from '@src/generic/Loading';
3536

3637
const CourseUpdates = () => {
3738
const intl = useIntl();
@@ -56,10 +57,9 @@ const CourseUpdates = () => {
5657

5758
const {
5859
isLoading: isLoadingPermissions,
59-
permissions,
60-
} = useUserPermissionsWithAuthzCourse(courseId, getCourseUpdatesPermissions(courseId));
61-
62-
const { canViewCourseUpdates, canManageCourseUpdates } = permissions;
60+
canViewCourseUpdates,
61+
canManageCourseUpdates,
62+
} = useCourseUserPermissions(courseId, getCourseUpdatesPermissions(courseId));
6363

6464
const loadingStatuses = useSelector(getLoadingStatuses);
6565
const savingStatuses = useSelector(getSavingStatuses);
@@ -81,7 +81,7 @@ const CourseUpdates = () => {
8181
);
8282
}
8383
if (isLoadingPermissions) {
84-
return null;
84+
return <Loading />;
8585
}
8686

8787
return (

src/header/hooks.test.tsx

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,20 @@ describe('header utils', () => {
135135
const actualItemsTitle = actualItems.map((item) => item.title);
136136
expect(actualItemsTitle).toContain(messages['header.links.updates'].defaultMessage);
137137
});
138+
it('when useNewUpdatesPage is false should use legacy studio URL for updates', () => {
139+
mockWaffleFlags({ enableAuthzCourseAuthoring: false, useNewUpdatesPage: false });
140+
jest.mocked(useUserPermissions).mockReturnValue({
141+
isLoading: false,
142+
data: { canViewCourseUpdates: true },
143+
} as any);
144+
jest.mocked(useSelector).mockReturnValue({
145+
librariesV2Enabled: false,
146+
});
147+
const actualItems =
148+
renderHook(() => useContentMenuItems('course-123'), { wrapper: createWrapper() }).result.current;
149+
const updatesItem = actualItems.find((item) => item.title === messages['header.links.updates'].defaultMessage);
150+
expect(updatesItem?.href).toContain('/course_info/course-123');
151+
});
138152
});
139153

140154
describe('getSettingsMenuitems', () => {

src/header/hooks.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { useUserPermissions } from '@src/authz/data/apiHooks';
1414
import { COURSE_PERMISSIONS } from '@src/authz/constants';
1515
import messages from './messages';
1616
import { getCourseUpdatesPermissions } from '@src/authz/permissionHelpers';
17-
import { useUserPermissionsWithAuthzCourse } from '@src/authz/hooks';
17+
import { useCourseUserPermissions } from '@src/authz/hooks';
1818

1919
export const useContentMenuItems = (courseId: string) => {
2020
const intl = useIntl();
@@ -23,8 +23,8 @@ export const useContentMenuItems = (courseId: string) => {
2323
const { librariesV2Enabled } = useSelector(getStudioHomeData);
2424

2525
const {
26-
permissions: { canViewCourseUpdates },
27-
} = useUserPermissionsWithAuthzCourse(courseId, getCourseUpdatesPermissions(courseId));
26+
canViewCourseUpdates,
27+
} = useCourseUserPermissions(courseId, getCourseUpdatesPermissions(courseId));
2828

2929
const items = [
3030
{

0 commit comments

Comments
 (0)