fix(duplication): make duplication UI aligned with BE user permissions#8385
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns the course duplication UI with the recently-tightened backend permission rules: only users who are an instance instructor/admin in the destination instance may duplicate a course to a new course there. Adds a non-blocking warning prompting the user to request the instructor role, disables the "New Course" option when no instance is a valid target, and partially reverts PR #7095 (also re-tightens within-instance duplication to require the instance-instructor permission). The PR also opportunistically converts most duplication JSX modules to TypeScript.
Changes:
- Tighten backend authorization: same-instance early-return removed in
Course::DuplicationsController; destination filtering inCourse::ObjectDuplicationsControlleris now uniform. - Drop
canDuplicateToAnotherInstancemetadata in favor of computing eligibility fromdestinationInstances; addcurrentInstanceHostand a warning Alert with a link to?request_instructor=true. - Large refactor: convert duplication bundle components/selectors/utils to TS, introduce
types.tswithDUPLICABLE_ITEM_TYPES/ITEM_SELECTOR_PANELS/DUPLICATION_MODES, consolidate selectors intoselectors.ts.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/course/duplications_controller.rb | Always authorize destination tenant; remove same-tenant short-circuit. |
| app/controllers/course/object_duplications_controller.rb | Unify non-admin branch — return only instances where the user is instance instructor/admin. |
| app/views/course/object_duplications/new.json.jbuilder | Replace canDuplicateToAnotherInstance with currentInstanceHost. |
| spec/controllers/course/object_duplication_controller_spec.rb | Add specs for new destinationInstances/metadata behavior. |
| spec/controllers/course/duplications_controller_spec.rb | Add specs for source/destination authorization with new rules. |
| client/app/bundles/course/duplication/types.ts | New TS types; central constants for item types/panels/modes. |
| client/app/bundles/course/duplication/utils.ts | Convert to TS; refactor nestFolders/getEmptySelectedItems (regression: Object.keys on tuple). |
| client/app/bundles/course/duplication/store.js | Replace duplicationModes/metadata.canDuplicateToAnotherInstance; add currentInstanceHost. |
| client/app/bundles/course/duplication/constants.js | Remove now-typed enums (duplicationModes, duplicableItemTypes, itemSelectorPanels, formNames). |
| client/app/bundles/course/duplication/selectors.ts | New consolidated selectors (replaces selectors/destinationInstance.ts & destinationCourse.js). |
| client/app/bundles/course/duplication/selectors/destinationInstance.ts / destinationCourse.js | Deleted. |
| client/app/bundles/course/duplication/types/course/duplication.ts | Removed (type moved into bundle). |
| client/app/bundles/course/duplication/components/TypeBadge/* | JSX → TSX rewrite using useTranslation. |
| client/app/bundles/course/duplication/components/IndentedCheckbox.* | JSX → TSX rewrite. |
| client/app/bundles/course/duplication/pages/Duplication/index.* | JSX → TSX rewrite; add destination-instance warning Alert; recompute modesAllowed from destinationInstances. |
| client/app/bundles/course/duplication/pages/Duplication/DuplicateAllButton.* | JSX → TSX rewrite. |
| client/app/bundles/course/duplication/pages/Duplication/DestinationCourseSelector/index.jsx | currentInstanceId becomes undefined when the current instance is not a valid destination. |
| client/app/bundles/course/duplication/pages/Duplication/DestinationCourseSelector/InstanceDropdown.tsx | Compute disabled state from instanceIds.length; consume new combined selector. |
| client/app/bundles/course/duplication/pages/Duplication/ItemsSelector/{Achievements,Assessments,Materials,Surveys,Videos}Selector.* | JSX → TSX rewrites (note: renamed message IDs and "Datas" naming). |
| client/app/bundles/course/duplication/pages/Duplication/ItemsSelector/index.* | JSX → TSX rewrite; uses selectDestinationCourse. |
| client/app/bundles/course/duplication/pages/Duplication/ItemsSelectorMenu/index.* | JSX → TSX rewrite. |
| client/app/bundles/course/duplication/pages/Duplication/DuplicateItemsConfirmation/{Achievements,Assessments,Materials,Survey,Videos}Listing.* | JSX → TSX rewrites. |
| client/app/bundles/course/duplication/pages/Duplication/test/ObjectDuplication.test.* | Expand coverage of the new alert/disabled-radio logic. |
| client/app/bundles/course/duplication/pages/Duplication/test/DestinationCourseSelector.test.tsx | New tests for the instance dropdown and MyLocation button. |
| client/app/bundles/course/duplication/pages/Duplication/test/DuplicateButton.test.tsx | Expanded selected-items / confirmation dialog coverage. |
| client/app/bundles/system/admin/instance/instance/components/forms/InstanceUserRoleRequestForm.tsx | Switch from text to (disabled) select with one option, hardcoded 'instructor'. |
| client/app/bundles/course/courses/pages/CoursesIndex/index.tsx | Auto-open the role-request dialog when ?request_instructor=true is set and the user can't create a course. |
Comments suppressed due to low confidence (2)
client/app/bundles/course/duplication/utils.ts:26
Object.keys(DUPLICABLE_ITEM_TYPES)is being called on a const tuple array, which returns the array indices as strings (["0","1","2",...]), not the type names. This meansgetEmptySelectedItems()returns{ "0": {}, "1": {}, ... }instead of{ ASSESSMENT: {}, TAB: {}, ... }. Since this is used as the initial value forselectedItemsin the store (and onSET_DESTINATION_COURSE_ID), and reducers/selectors index it viaselectedItems['ASSESSMENT'],selectedItems.VIDEO_TAB, etc., these will all beundefined. The very firstdispatch(actions.setItemSelectedBoolean('ASSESSMENT', id, value))will throw a TypeError (Cannot set property of undefined). IterateDUPLICABLE_ITEM_TYPESdirectly (e.g.DUPLICABLE_ITEM_TYPES.reduce(...)or.forEach) instead ofObject.keys(...).
client/app/bundles/course/duplication/pages/Duplication/ItemsSelector/SurveysSelector.tsx:35- Component and helper names like
DuplicationSurveyDatasSelector,setAllDuplicationSurveyDatasSelectionare awkward — the plural of "data" is "data" (not "datas"), and the prefixDuplicationduplicates the bundle namespace. The originalSurveysSelector/setAllSurveysSelectionnames were clearer. Consider reverting to the prior names.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7723d61 to
80c9b24
Compare
48543d9 to
777a9e6
Compare
- 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
- remove duplicableItemTypes/itemSelectorPanels dependencies
777a9e6 to
12d05ab
Compare
Partial revert of #7095, the duplicating user should be an instructor in the destination instance if they want to duplicate to a new course.
If there are no valid target instances, the "New Course" option will be disabled.