Skip to content

Commit db4f206

Browse files
fix(duplication): make duplication UI aligned with BE user permissions
- users need to be instructor to duplicate to a new course in the destination tenant - add alert for non-instructors in current tenant to request instructor role - only valid target instances will be selectable, FE will disable new course duplication if no valid targets - create types for duplication state reducer based on BE response
1 parent 500bad4 commit db4f206

24 files changed

Lines changed: 934 additions & 347 deletions

File tree

app/controllers/course/duplications_controller.rb

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,8 @@ def create
1717

1818
def authorize_duplication
1919
authorize!(:duplicate_from, current_course)
20-
return if instance_params == current_tenant.id
2120

2221
destination_tenant = Instance.find(instance_params)
23-
24-
authorize!(:duplicate_across_instances, current_tenant)
2522
authorize!(:duplicate_across_instances, destination_tenant)
2623
end
2724

app/controllers/course/object_duplications_controller.rb

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,15 +69,13 @@ def load_videos_component_data
6969
def load_destination_instances_data
7070
@destination_instances = if current_user.administrator?
7171
Instance.all
72-
elsif can?(:duplicate_across_instances, current_tenant)
72+
else
7373
instance_ids = InstanceUser.unscope(where: :instance_id).
7474
where(user_id: current_user.id,
7575
role: [InstanceUser.roles[:instructor],
7676
InstanceUser.roles[:administrator]]).
7777
pluck(:instance_id)
7878
Instance.where(id: instance_ids)
79-
else
80-
Instance.where(id: current_tenant.id)
8179
end
8280
end
8381

app/views/course/object_duplications/new.json.jbuilder

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ json.destinationInstances sorted_destination_instances do |instance|
2424
end
2525

2626
json.metadata do
27-
json.canDuplicateToAnotherInstance can?(:duplicate_across_instances, current_tenant)
2827
json.currentInstanceId current_tenant.id
28+
json.currentInstanceHost current_tenant.host
2929
end
3030

3131
json.partial! 'course_duplication_data'

client/app/bundles/course/courses/pages/CoursesIndex/index.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ const CoursesIndex: FC = () => {
4949
const [params] = useSearchParams();
5050

5151
const [isLoading, setIsLoading] = useState(true);
52-
const [isRoleRequestDialogOpen, setRoleRequestDialogOpen] = useState(false);
5352
const courses = useAppSelector(getAllCourseMiniEntities);
5453

5554
const coursesPermissions = useAppSelector(getCoursePermissions);
@@ -67,6 +66,13 @@ const CoursesIndex: FC = () => {
6766
shouldOpenNewCourseDialog,
6867
);
6968

69+
const shouldOpenRoleRequestDialog =
70+
Boolean(params.get('request_instructor')) && !coursesPermissions?.canCreate;
71+
72+
const [isRoleRequestDialogOpen, setRoleRequestDialogOpen] = useState(
73+
shouldOpenRoleRequestDialog,
74+
);
75+
7076
useEffect(() => {
7177
dispatch(fetchCourses())
7278
.finally(() => setIsLoading(false))
@@ -77,6 +83,10 @@ const CoursesIndex: FC = () => {
7783
setIsNewCourseDialogOpen(shouldOpenNewCourseDialog);
7884
}, [shouldOpenNewCourseDialog]);
7985

86+
useEffect(() => {
87+
setRoleRequestDialogOpen(shouldOpenRoleRequestDialog);
88+
}, [shouldOpenRoleRequestDialog]);
89+
8090
// Adding appropriate button to the header
8191
const headerToolbars: ReactElement[] = [];
8292
if (coursesPermissions?.canCreate) {

client/app/bundles/course/duplication/constants.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
import mirrorCreator from 'mirror-creator';
22

3-
export const duplicationModes = mirrorCreator(['OBJECT', 'COURSE']);
4-
53
// These are mirrored in app/helpers/course/object_duplications_helper.rb
64
export const duplicableItemTypes = mirrorCreator([
75
'ASSESSMENT',

client/app/bundles/course/duplication/pages/Duplication/DestinationCourseSelector/InstanceDropdown.tsx

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,7 @@ import { defineMessages, FormattedMessage } from 'react-intl';
99
import MyLocation from '@mui/icons-material/MyLocation';
1010
import { Autocomplete, Box, IconButton, Tooltip } from '@mui/material';
1111

12-
import {
13-
selectDestinationInstances,
14-
selectMetadata,
15-
} from 'course/duplication/selectors/destinationInstance';
12+
import { selectDestinationInstances } from 'course/duplication/selectors';
1613
import TextField from 'lib/components/core/fields/TextField';
1714
import { formatErrorMessage } from 'lib/components/form/fields/utils/mapError';
1815
import { useAppSelector } from 'lib/hooks/store';
@@ -40,7 +37,6 @@ const translations = defineMessages({
4037
const InstanceDropdown: FC<InstanceDropdownProps> = (props) => {
4138
const { currentInstanceId, disabled, field, fieldState, setValue } = props;
4239
const instances = useAppSelector(selectDestinationInstances);
43-
const metadata = useAppSelector(selectMetadata);
4440
const instanceIds = useMemo(
4541
() =>
4642
Object.keys(instances).toSorted(
@@ -54,7 +50,7 @@ const InstanceDropdown: FC<InstanceDropdownProps> = (props) => {
5450
<div className="flex">
5551
<Autocomplete
5652
{...field}
57-
disabled={disabled || !metadata.canDuplicateToAnotherInstance}
53+
disabled={disabled || !instanceIds.length}
5854
fullWidth
5955
getOptionLabel={(instanceId): string =>
6056
instances[instanceId]?.name ?? ''
@@ -85,7 +81,7 @@ const InstanceDropdown: FC<InstanceDropdownProps> = (props) => {
8581
<div className="flex items-end">
8682
<Tooltip title={t(translations.currentInstance)}>
8783
<IconButton
88-
disabled={disabled}
84+
disabled={disabled || !(currentInstanceId in instances)}
8985
onClick={() =>
9086
setValue('destination_instance_id', currentInstanceId)
9187
}

client/app/bundles/course/duplication/pages/Duplication/DestinationCourseSelector/index.jsx

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { connect } from 'react-redux';
44
import PropTypes from 'prop-types';
55

66
import CourseDropdownMenu from 'course/duplication/components/CourseDropdownMenu';
7-
import { duplicationModes } from 'course/duplication/constants';
87
import { duplicateCourse } from 'course/duplication/operations';
98
import { courseShape, sourceCourseShape } from 'course/duplication/propTypes';
109
import { actions } from 'course/duplication/store';
@@ -116,7 +115,7 @@ class DestinationCourseSelector extends Component {
116115
render() {
117116
const { duplicationMode } = this.props;
118117

119-
return duplicationMode === duplicationModes.COURSE
118+
return duplicationMode === 'COURSE'
120119
? this.renderNewCourseForm()
121120
: this.renderExistingCourseForm();
122121
}
@@ -144,7 +143,9 @@ export default connect(({ duplication }) => ({
144143
destinationCourseId: duplication.destinationCourseId,
145144
duplicationMode: duplication.duplicationMode,
146145
sourceCourse: duplication.sourceCourse,
147-
currentInstanceId: duplication.metadata.currentInstanceId,
146+
currentInstanceId:
147+
duplication.destinationInstances[duplication.metadata.currentInstanceId]
148+
?.id,
148149
destinationInstances: duplication.destinationInstances,
149150
isDuplicating: duplication.isDuplicating,
150151
}))(injectIntl(DestinationCourseSelector));
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
import { createMockAdapter } from 'mocks/axiosMock';
2+
import { store } from 'store';
3+
import { fireEvent, render, screen, waitFor } from 'test-utils';
4+
5+
import CourseAPI from 'api/course';
6+
import { loadObjectsList } from 'course/duplication/store';
7+
8+
import ObjectDuplication from '../index';
9+
10+
const client = CourseAPI.duplication.client;
11+
const mock = createMockAdapter(client);
12+
const url = `/courses/${global.courseId}/object_duplication/new`;
13+
const defaultInstance = {
14+
id: 1,
15+
name: 'Default',
16+
host: 'example.org',
17+
};
18+
19+
const baseData = {
20+
sourceCourse: {
21+
id: 5,
22+
title: 'Source Course',
23+
start_at: '2024-01-01',
24+
duplicationModesAllowed: ['COURSE', 'OBJECT'],
25+
enabledComponents: ['ASSESSMENTS'],
26+
unduplicableObjectTypes: [],
27+
},
28+
metadata: { currentInstanceId: 1, currentInstanceHost: 'coursemology.org' },
29+
destinationInstances: [defaultInstance],
30+
destinationCourses: [],
31+
materialsComponent: [],
32+
assessmentsComponent: [],
33+
surveyComponent: [],
34+
achievementsComponent: [],
35+
videosComponent: [],
36+
};
37+
38+
const waitForCombobox = (): Promise<HTMLElement> =>
39+
screen.findByRole('combobox', { name: /destination instance/i });
40+
41+
beforeEach(() => {
42+
mock.reset();
43+
store.dispatch(loadObjectsList(baseData));
44+
});
45+
46+
describe('<DestinationCourseSelector /> instance dropdown', () => {
47+
it('pre-fills instance dropdown with current instance when it is a valid destination', async () => {
48+
const testData = {
49+
...baseData,
50+
metadata: {
51+
currentInstanceId: 1,
52+
currentInstanceHost: 'coursemology.org',
53+
},
54+
destinationInstances: [defaultInstance],
55+
};
56+
store.dispatch(loadObjectsList(testData));
57+
mock.onGet(url).reply(200, testData);
58+
59+
render(<ObjectDuplication />);
60+
const combobox = await waitForCombobox();
61+
62+
expect(combobox).toHaveValue('Default');
63+
});
64+
65+
it('does not pre-fill instance dropdown when current instance is not a valid destination', async () => {
66+
const testData = {
67+
...baseData,
68+
metadata: {
69+
currentInstanceId: 99,
70+
currentInstanceHost: 'other.coursemology.org',
71+
},
72+
destinationInstances: [defaultInstance],
73+
};
74+
store.dispatch(loadObjectsList(testData));
75+
mock.onGet(url).reply(200, testData);
76+
77+
render(<ObjectDuplication />);
78+
const combobox = await waitForCombobox();
79+
80+
expect(combobox).toHaveValue('');
81+
});
82+
83+
it('MyLocation button sets the Autocomplete to the current instance', async () => {
84+
const testData = {
85+
...baseData,
86+
metadata: {
87+
currentInstanceId: 1,
88+
currentInstanceHost: 'coursemology.org',
89+
},
90+
destinationInstances: [
91+
defaultInstance,
92+
{ id: 2, name: 'Other', host: 'other.org' },
93+
],
94+
};
95+
store.dispatch(loadObjectsList(testData));
96+
mock.onGet(url).reply(200, testData);
97+
98+
render(<ObjectDuplication />);
99+
const combobox = await waitForCombobox();
100+
await waitFor(() => expect(combobox).toHaveValue('Default'));
101+
102+
// Click MyLocation to confirm it sets the current instance
103+
fireEvent.click(
104+
screen.getByRole('button', { name: /select current instance/i }),
105+
);
106+
107+
// Combobox should show the current instance name
108+
await waitFor(() => expect(combobox).toHaveValue('Default'));
109+
});
110+
111+
/**
112+
* The MyLocation IconButton is currently only disabled when `isDuplicating` is true,
113+
* but it should also be disabled when the current instance is not a valid destination.
114+
*/
115+
it('disables MyLocation button when current instance is not a valid destination', async () => {
116+
const testData = {
117+
...baseData,
118+
metadata: {
119+
currentInstanceId: 99,
120+
currentInstanceHost: 'other.coursemology.org',
121+
},
122+
destinationInstances: [defaultInstance],
123+
};
124+
store.dispatch(loadObjectsList(testData));
125+
mock.onGet(url).reply(200, testData);
126+
127+
render(<ObjectDuplication />);
128+
await waitForCombobox();
129+
130+
const locationButton = screen.getByRole('button', {
131+
name: /select current instance/i,
132+
});
133+
expect(locationButton).toBeDisabled();
134+
});
135+
});

client/app/bundles/course/duplication/pages/Duplication/__test__/DuplicateButton.test.tsx

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { store } from 'store';
2-
import { fireEvent, render } from 'test-utils';
2+
import { fireEvent, render, screen, within } from 'test-utils';
33

44
import CourseAPI from 'api/course';
55
import { duplicableItemTypes } from 'course/duplication/constants';
@@ -9,7 +9,7 @@ import DuplicateButton from '../DuplicateButton';
99

1010
const data = {
1111
sourceCourse: { id: 37 },
12-
metadata: { canDuplicateToAnotherInstance: false, currentInstanceId: 0 },
12+
metadata: { currentInstanceId: 0 },
1313
destinationCourseId: 9,
1414
destinationCourses: [
1515
{
@@ -21,9 +21,15 @@ const data = {
2121
],
2222
destinationInstances: [{ id: 0, name: 'default', host: 'example.org' }],
2323
selectedItems: {
24-
[duplicableItemTypes.TAB]: { 3: true, 4: true, 5: false },
25-
[duplicableItemTypes.CATEGORY]: { 6: false },
26-
[duplicableItemTypes.ASSESSMENT]: { 7: true },
24+
ASSESSMENT: { 7: true },
25+
TAB: { 3: true, 4: true, 5: false },
26+
CATEGORY: { 6: false },
27+
SURVEY: { 10: true },
28+
ACHIEVEMENT: { 11: true, 12: false },
29+
FOLDER: { 13: true },
30+
MATERIAL: { 14: true },
31+
VIDEO: { 15: true },
32+
VIDEO_TAB: { 16: true },
2733
},
2834
materialsComponent: [],
2935
assessmentsComponent: [
@@ -54,14 +60,32 @@ const data = {
5460
],
5561
},
5662
],
57-
videosComponent: [],
63+
surveyComponent: [{ id: 10, title: 'Survey 10', published: true }],
64+
achievementsComponent: [
65+
{ id: 11, title: 'Achievement 11', published: true, url: '/badges/11' },
66+
{ id: 12, title: 'Achievement 12', published: true, url: '/badges/12' },
67+
],
68+
videosComponent: [
69+
{
70+
id: 16,
71+
title: 'Video Tab 16',
72+
parent_id: null,
73+
videos: [{ id: 15, title: 'Video 15', published: true }],
74+
},
75+
],
5876
};
5977

6078
const expectedPayload = {
6179
object_duplication: {
6280
items: {
6381
ASSESSMENT: ['7'],
6482
TAB: ['3', '4'],
83+
SURVEY: ['10'],
84+
ACHIEVEMENT: ['11'],
85+
FOLDER: ['13'],
86+
MATERIAL: ['14'],
87+
VIDEO: ['15'],
88+
VIDEO_TAB: ['16'],
6589
},
6690
destination_course_id: 9,
6791
},
@@ -79,4 +103,43 @@ describe('<DuplicateButton />', () => {
79103

80104
expect(spy).toHaveBeenCalledWith(data.sourceCourse.id, expectedPayload);
81105
});
106+
107+
it('shows only selected items in the confirmation dialog and sends the correct request', async () => {
108+
const spy = jest.spyOn(CourseAPI.duplication, 'duplicateItems');
109+
110+
store.dispatch(loadObjectsList(data));
111+
const page = render(<DuplicateButton />);
112+
113+
fireEvent.click(
114+
await page.findByRole('button', { name: /duplicate items/i }),
115+
);
116+
117+
const dialog = await screen.findByRole('dialog');
118+
const d = within(dialog);
119+
120+
// Destination course card
121+
expect(d.getByText('destination')).toBeInTheDocument();
122+
123+
// Selected assessments and tabs appear; unselected ones do not
124+
expect(d.getByText('Assessment 7')).toBeInTheDocument();
125+
expect(d.getByText('Tab 3')).toBeInTheDocument();
126+
expect(d.getByText('Tab 4')).toBeInTheDocument();
127+
expect(d.queryByText('Tab 5')).not.toBeInTheDocument();
128+
expect(d.queryByText('Category 6')).not.toBeInTheDocument();
129+
130+
// Selected survey appears
131+
expect(d.getByText('Survey 10')).toBeInTheDocument();
132+
133+
// Selected achievement appears; unselected one does not
134+
expect(d.getByText('Achievement 11')).toBeInTheDocument();
135+
expect(d.queryByText('Achievement 12')).not.toBeInTheDocument();
136+
137+
// Selected video and video tab appear
138+
expect(d.getByText('Video Tab 16')).toBeInTheDocument();
139+
expect(d.getByText('Video 15')).toBeInTheDocument();
140+
141+
// Clicking Duplicate sends the correct API request
142+
fireEvent.click(d.getByRole('button', { name: 'Duplicate' }));
143+
expect(spy).toHaveBeenCalledWith(data.sourceCourse.id, expectedPayload);
144+
});
82145
});

0 commit comments

Comments
 (0)