fix: several library unit page UX bugs (openedx#1868)

rpenido · pomegranited · commit aa908efba3b3 · 2025-05-02T12:48:06.000+09:30
* fix: rename "Organize" tab to "Manage" * fix: duplicate key warnings * fix: uniform messages while adding to collection * fix: do not allow units be added to a unit (cherry picked from commit 0fdc460)
diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx
@@ -433,7 +433,7 @@ describe('<LibraryAuthoringPage />', () => {
     const { getByRole, queryByText } = within(sidebar);
 
     await waitFor(() => expect(queryByText(displayName)).toBeInTheDocument());
-    expect(getByRole('tab', { selected: true })).toHaveTextContent('Organize');
+    expect(getByRole('tab', { selected: true })).toHaveTextContent('Manage');
     const closeButton = getByRole('button', { name: /close/i });
     fireEvent.click(closeButton);
 
diff --git a/src/library-authoring/add-content/AddContent.test.tsx b/src/library-authoring/add-content/AddContent.test.tsx
@@ -272,7 +272,7 @@ describe('<AddContent />', () => {
     await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(pasteUrl));
     await waitFor(() => expect(axiosMock.history.patch.length).toEqual(1));
     await waitFor(() => expect(axiosMock.history.patch[0].url).toEqual(collectionComponentUrl));
-    expect(mockShowToast).toHaveBeenCalledWith('There was an error linking the content to this collection.');
+    expect(mockShowToast).toHaveBeenCalledWith('Failed to add content to collection.');
   });
 
   it('should stop user from pasting unsupported blocks and show toast', async () => {
diff --git a/src/library-authoring/add-content/AddContent.tsx b/src/library-authoring/add-content/AddContent.tsx
@@ -29,6 +29,8 @@ import { useLibraryContext } from '../common/context/LibraryContext';
 import { PickLibraryContentModal } from './PickLibraryContentModal';
 import { blockTypes } from '../../editors/data/constants/app';
 
+import { ContentType as LibraryContentTypes } from '../routes';
+import genericMessages from '../generic/messages';
 import messages from './messages';
 import type { BlockTypeMetadata } from '../data/api';
 import { getContainerTypeFromId, ContainerType } from '../../generic/key-utils';
@@ -114,6 +116,9 @@ const AddContentView = ({
     blockType: 'libraryContent',
   };
 
+  const extraFilter = unitId ? ['NOT block_type = "unit"', 'NOT type = "collections"'] : undefined;
+  const visibleTabs = unitId ? [LibraryContentTypes.components] : undefined;
+
   return (
     <>
       {(collectionId || unitId) && componentPicker && (
@@ -123,6 +128,8 @@ const AddContentView = ({
           <PickLibraryContentModal
             isOpen={isAddLibraryContentModalOpen}
             onClose={closeAddLibraryContentModal}
+            extraFilter={extraFilter}
+            visibleTabs={visibleTabs}
           />
         </>
       )}
@@ -301,7 +308,7 @@ const AddContent = () => {
   const linkComponent = (opaqueKey: string) => {
     if (collectionId) {
       addComponentsToCollectionMutation.mutateAsync([opaqueKey]).catch(() => {
-        showToast(intl.formatMessage(messages.errorAssociateComponentToCollectionMessage));
+        showToast(intl.formatMessage(genericMessages.manageCollectionsFailed));
       });
     }
     if (unitId) {
diff --git a/src/library-authoring/add-content/PickLibraryContentModal.test.tsx b/src/library-authoring/add-content/PickLibraryContentModal.test.tsx
@@ -92,7 +92,10 @@ describe('<PickLibraryContentModal />', () => {
         }
       });
       expect(onClose).toHaveBeenCalled();
-      expect(mockShowToast).toHaveBeenCalledWith('Content linked successfully.');
+      const text = context === 'collection'
+        ? 'Content added to collection.'
+        : 'Content linked successfully.';
+      expect(mockShowToast).toHaveBeenCalledWith(text);
     });
 
     it(`show error when api call fails (${context})`, async () => {
@@ -130,8 +133,10 @@ describe('<PickLibraryContentModal />', () => {
         }
       });
       expect(onClose).toHaveBeenCalled();
-      const name = context === 'collection' ? 'collection' : 'container';
-      expect(mockShowToast).toHaveBeenCalledWith(`There was an error linking the content to this ${name}.`);
+      const text = context === 'collection'
+        ? 'Failed to add content to collection.'
+        : 'There was an error linking the content to this container.';
+      expect(mockShowToast).toHaveBeenCalledWith(text);
     });
   });
 });
diff --git a/src/library-authoring/add-content/PickLibraryContentModal.tsx b/src/library-authoring/add-content/PickLibraryContentModal.tsx
@@ -6,6 +6,8 @@ import { ToastContext } from '../../generic/toast-context';
 import { useLibraryContext } from '../common/context/LibraryContext';
 import type { SelectedComponent } from '../common/context/ComponentPickerContext';
 import { useAddItemsToCollection, useAddComponentsToContainer } from '../data/apiHooks';
+import genericMessages from '../generic/messages';
+import type { ContentType } from '../routes';
 import messages from './messages';
 
 interface PickLibraryContentModalFooterProps {
@@ -32,12 +34,14 @@ interface PickLibraryContentModalProps {
   isOpen: boolean;
   onClose: () => void;
   extraFilter?: string[];
+  visibleTabs?: ContentType[],
 }
 
 export const PickLibraryContentModal: React.FC<PickLibraryContentModalProps> = ({
   isOpen,
   onClose,
   extraFilter,
+  visibleTabs,
 }) => {
   const intl = useIntl();
 
@@ -69,16 +73,16 @@ export const PickLibraryContentModal: React.FC<PickLibraryContentModalProps> = (
     if (collectionId) {
       updateCollectionItemsMutation.mutateAsync(usageKeys)
         .then(() => {
-          showToast(intl.formatMessage(messages.successAssociateComponentMessage));
+          showToast(intl.formatMessage(genericMessages.manageCollectionsSuccess));
         })
         .catch(() => {
-          showToast(intl.formatMessage(messages.errorAssociateComponentToCollectionMessage));
+          showToast(intl.formatMessage(genericMessages.manageCollectionsFailed));
         });
     }
     if (unitId) {
       updateUnitComponentsMutation.mutateAsync(usageKeys)
         .then(() => {
-          showToast(intl.formatMessage(messages.successAssociateComponentMessage));
+          showToast(intl.formatMessage(messages.successAssociateComponentToContainerMessage));
         })
         .catch(() => {
           showToast(intl.formatMessage(messages.errorAssociateComponentToContainerMessage));
@@ -109,6 +113,7 @@ export const PickLibraryContentModal: React.FC<PickLibraryContentModalProps> = (
         componentPickerMode="multiple"
         onChangeComponentSelection={setSelectedComponents}
         extraFilter={extraFilter}
+        visibleTabs={visibleTabs}
       />
     </StandardModal>
   );
diff --git a/src/library-authoring/add-content/messages.ts b/src/library-authoring/add-content/messages.ts
@@ -84,15 +84,10 @@ const messages = defineMessages({
       + ' The {detail} text provides more information about the error.'
     ),
   },
-  successAssociateComponentMessage: {
-    id: 'course-authoring.library-authoring.associate-collection-content.success.text',
+  successAssociateComponentToContainerMessage: {
+    id: 'course-authoring.library-authoring.associate-container-content.success.text',
     defaultMessage: 'Content linked successfully.',
-    description: 'Message when linking of content to a collection in library is success',
-  },
-  errorAssociateComponentToCollectionMessage: {
-    id: 'course-authoring.library-authoring.associate-collection-content.error.text',
-    defaultMessage: 'There was an error linking the content to this collection.',
-    description: 'Message when linking of content to a collection in library fails',
+    description: 'Message when linking of content to a container in library is success',
   },
   errorAssociateComponentToContainerMessage: {
     id: 'course-authoring.library-authoring.associate-container-content.error.text',
diff --git a/src/library-authoring/common/context/SidebarContext.tsx b/src/library-authoring/common/context/SidebarContext.tsx
@@ -36,7 +36,7 @@ export const isComponentInfoTab = (tab: string): tab is ComponentInfoTab => (
 
 export const UNIT_INFO_TABS = {
   Preview: 'preview',
-  Organize: 'organize',
+  Manage: 'manage',
   Usage: 'usage',
   Settings: 'settings',
 } as const;
diff --git a/src/library-authoring/components/ContainerCard.tsx b/src/library-authoring/components/ContainerCard.tsx
@@ -147,7 +147,9 @@ const ContainerCardPreview = ({ containerId, showMaxChildren = 5 }: ContainerCar
           }
           return (
             <div
-              key={`container-card-preview-block-${id}`}
+              // A container can have multiple instances of the same block
+              // eslint-disable-next-line react/no-array-index-key
+              key={`${id}-${idx}`}
               className={classNames}
             >
               {blockPreview}
diff --git a/src/library-authoring/containers/ContainerOrganize.tsx b/src/library-authoring/containers/ContainerOrganize.tsx
@@ -85,7 +85,7 @@ const ContainerOrganize = () => {
             >
               <Stack gap={1} direction="horizontal">
                 <Icon src={Tag} />
-                {intl.formatMessage(messages.organizeTabTagsTitle, { count: tagsCount })}
+                {intl.formatMessage(messages.manageTabTagsTitle, { count: tagsCount })}
               </Stack>
               <Collapsible.Visible whenClosed>
                 <Icon src={ExpandMore} />
@@ -113,7 +113,7 @@ const ContainerOrganize = () => {
         >
           <Stack gap={1} direction="horizontal">
             <Icon src={BookOpen} />
-            {intl.formatMessage(messages.organizeTabCollectionsTitle, { count: collectionsCount })}
+            {intl.formatMessage(messages.manageTabCollectionsTitle, { count: collectionsCount })}
           </Stack>
           <Collapsible.Visible whenClosed>
             <Icon src={ExpandMore} />
diff --git a/src/library-authoring/containers/UnitInfo.tsx b/src/library-authoring/containers/UnitInfo.tsx
@@ -120,7 +120,7 @@ const UnitInfo = () => {
   useEffect(() => {
     // Show Organize tab if JumpToAddCollections action is set in sidebarComponentInfo
     if (jumpToCollections) {
-      setSidebarTab(UNIT_INFO_TABS.Organize);
+      setSidebarTab(UNIT_INFO_TABS.Manage);
     }
   }, [jumpToCollections, setSidebarTab]);
 
@@ -166,7 +166,7 @@ const UnitInfo = () => {
         onSelect={setSidebarTab}
       >
         {renderTab(UNIT_INFO_TABS.Preview, <LibraryUnitBlocks preview />, intl.formatMessage(messages.previewTabTitle))}
-        {renderTab(UNIT_INFO_TABS.Organize, <ContainerOrganize />, intl.formatMessage(messages.organizeTabTitle))}
+        {renderTab(UNIT_INFO_TABS.Manage, <ContainerOrganize />, intl.formatMessage(messages.manageTabTitle))}
         {renderTab(UNIT_INFO_TABS.Settings, 'Unit Settings', intl.formatMessage(messages.settingsTabTitle))}
       </Tabs>
     </Stack>
diff --git a/src/library-authoring/containers/messages.ts b/src/library-authoring/containers/messages.ts
@@ -11,20 +11,20 @@ const messages = defineMessages({
     defaultMessage: 'Preview',
     description: 'Title for preview tab',
   },
-  organizeTabTitle: {
-    id: 'course-authoring.library-authoring.container-sidebar.organize-tab.title',
-    defaultMessage: 'Organize',
-    description: 'Title for organize tab',
+  manageTabTitle: {
+    id: 'course-authoring.library-authoring.container-sidebar.manage-tab.title',
+    defaultMessage: 'Manage',
+    description: 'Title for manage tab',
   },
-  organizeTabTagsTitle: {
-    id: 'course-authoring.library-authoring.container-sidebar.organize-tab.tags.title',
+  manageTabTagsTitle: {
+    id: 'course-authoring.library-authoring.container-sidebar.manage-tab.tags.title',
     defaultMessage: 'Tags ({count})',
-    description: 'Title for tags section in organize tab',
+    description: 'Title for tags section in manage tab',
   },
-  organizeTabCollectionsTitle: {
-    id: 'course-authoring.library-authoring.container-sidebar.organize-tab.collections.title',
+  manageTabCollectionsTitle: {
+    id: 'course-authoring.library-authoring.container-sidebar.manage-tab.collections.title',
     defaultMessage: 'Collections ({count})',
-    description: 'Title for collections section in organize tab',
+    description: 'Title for collections section in manage tab',
   },
   publishContainerButton: {
     id: 'course-authoring.library-authoring.container-sidebar.publish-button',
diff --git a/src/library-authoring/generic/manage-collections/ManageCollections.test.tsx b/src/library-authoring/generic/manage-collections/ManageCollections.test.tsx
@@ -77,7 +77,7 @@ describe('<ManageCollections />', () => {
     await waitFor(() => {
       expect(axiosMock.history.patch.length).toEqual(1);
     });
-    expect(mockShowToast).toHaveBeenCalledWith('Item collections updated');
+    expect(mockShowToast).toHaveBeenCalledWith('Content added to collection.');
     expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({
       collection_keys: ['my-first-collection', 'my-second-collection'],
     });
@@ -103,7 +103,7 @@ describe('<ManageCollections />', () => {
     await waitFor(() => {
       expect(axiosMock.history.patch.length).toEqual(1);
     });
-    expect(mockShowToast).toHaveBeenCalledWith('Item collections updated');
+    expect(mockShowToast).toHaveBeenCalledWith('Content added to collection.');
     expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({
       collection_keys: ['my-first-collection', 'my-second-collection'],
     });
@@ -133,7 +133,7 @@ describe('<ManageCollections />', () => {
     expect(JSON.parse(axiosMock.history.patch[0].data)).toEqual({
       collection_keys: ['my-second-collection'],
     });
-    expect(mockShowToast).toHaveBeenCalledWith('Failed to update item collections');
+    expect(mockShowToast).toHaveBeenCalledWith('Failed to add content to collection.');
     expect(screen.queryByRole('search')).not.toBeInTheDocument();
   });
 
diff --git a/src/library-authoring/generic/manage-collections/ManageCollections.tsx b/src/library-authoring/generic/manage-collections/ManageCollections.tsx
@@ -16,6 +16,7 @@ import { ToastContext } from '../../../generic/toast-context';
 import { CollectionMetadata } from '../../data/api';
 import { useLibraryContext } from '../../common/context/LibraryContext';
 import { SidebarActions, useSidebarContext } from '../../common/context/SidebarContext';
+import genericMessages from '../messages';
 import messages from './messages';
 
 interface ManageCollectionsProps {
@@ -50,9 +51,9 @@ const CollectionsSelectableBox = ({
   const handleConfirmation = () => {
     setBtnState('pending');
     updateCollectionsMutation.mutateAsync(selectedCollections).then(() => {
-      showToast(intl.formatMessage(messages.manageCollectionsToComponentSuccess));
+      showToast(intl.formatMessage(genericMessages.manageCollectionsSuccess));
     }).catch(() => {
-      showToast(intl.formatMessage(messages.manageCollectionsToComponentFailed));
+      showToast(intl.formatMessage(genericMessages.manageCollectionsFailed));
     }).finally(() => {
       setBtnState('default');
       onClose();
diff --git a/src/library-authoring/generic/manage-collections/messages.ts b/src/library-authoring/generic/manage-collections/messages.ts
@@ -21,16 +21,6 @@ const messages = defineMessages({
     defaultMessage: 'Collection selection',
     description: 'Aria label text for collection selection box',
   },
-  manageCollectionsToComponentSuccess: {
-    id: 'course-authoring.library-authoring.manage-collections.add-success',
-    defaultMessage: 'Item collections updated',
-    description: 'Message to display on updating item collections',
-  },
-  manageCollectionsToComponentFailed: {
-    id: 'course-authoring.library-authoring.manage-collections.add-failed',
-    defaultMessage: 'Failed to update item collections',
-    description: 'Message to display on failure of updating item collections',
-  },
   manageCollectionsToComponentConfirmBtn: {
     id: 'course-authoring.library-authoring.manage-collections.add-confirm-btn',
     defaultMessage: 'Confirm',
diff --git a/src/library-authoring/generic/messages.ts b/src/library-authoring/generic/messages.ts
@@ -0,0 +1,16 @@
+import { defineMessages } from '@edx/frontend-platform/i18n';
+
+const messages = defineMessages({
+  manageCollectionsSuccess: {
+    id: 'course-authoring.library-authoring.manage-collections.success',
+    defaultMessage: 'Content added to collection.',
+    description: 'Message to display on updating item collections',
+  },
+  manageCollectionsFailed: {
+    id: 'course-authoring.library-authoring.manage-collections.failed',
+    defaultMessage: 'Failed to add content to collection.',
+    description: 'Message to display on failure of updating item collections',
+  },
+});
+
+export default messages;
diff --git a/src/library-authoring/units/LibraryUnitBlocks.tsx b/src/library-authoring/units/LibraryUnitBlocks.tsx
@@ -28,7 +28,7 @@ import {
   useUpdateXBlockFields,
 } from '../data/apiHooks';
 import { LibraryBlock } from '../LibraryBlock';
-import { useLibraryRoutes } from '../routes';
+import { useLibraryRoutes, ContentType } from '../routes';
 import messages from './messages';
 import { useSidebarContext } from '../common/context/SidebarContext';
 import { ToastContext } from '../../generic/toast-context';
@@ -200,8 +200,10 @@ export const LibraryUnitBlocks = ({ preview }: LibraryUnitBlocksProps) => {
     );
   };
 
-  const renderedBlocks = orderedBlocks?.map((block) => (
-    <IframeProvider key={block.id + block.modified}>
+  const renderedBlocks = orderedBlocks?.map((block, idx) => (
+    // A container can have multiple instances of the same block
+    // eslint-disable-next-line react/no-array-index-key
+    <IframeProvider key={`${block.id}-${idx}-${block.modified}`}>
       <SortableItem
         id={block.id}
         componentStyle={null}
@@ -218,16 +220,16 @@ export const LibraryUnitBlocks = ({ preview }: LibraryUnitBlocksProps) => {
         disabled={preview}
       >
         {hidePreviewFor !== block.id && (
-        <div className={classNames('p-3', {
-          'container-mw-md': block.blockType === blockTypes.video,
-        })}
-        >
-          <LibraryBlock
-            usageKey={block.id}
-            version={showOnlyPublished ? 'published' : undefined}
-            minHeight={calculateMinHeight(block)}
-          />
-        </div>
+          <div className={classNames('p-3', {
+            'container-mw-md': block.blockType === blockTypes.video,
+          })}
+          >
+            <LibraryBlock
+              usageKey={block.id}
+              version={showOnlyPublished ? 'published' : undefined}
+              minHeight={calculateMinHeight(block)}
+            />
+          </div>
         )}
       </SortableItem>
     </IframeProvider>
@@ -245,7 +247,7 @@ export const LibraryUnitBlocks = ({ preview }: LibraryUnitBlocksProps) => {
       >
         {renderedBlocks}
       </DraggableList>
-      { !preview && (
+      {!preview && (
         <div className="d-flex">
           <div className="w-100 mr-2">
             <Button
@@ -273,7 +275,8 @@ export const LibraryUnitBlocks = ({ preview }: LibraryUnitBlocksProps) => {
             <PickLibraryContentModal
               isOpen={isAddLibraryContentModalOpen}
               onClose={closeAddLibraryContentModal}
-              extraFilter={['NOT block_type = "unit"']}
+              extraFilter={['NOT block_type = "unit"', 'NOT type = "collection"']}
+              visibleTabs={[ContentType.components]}
             />
           </div>
         </div>
diff --git a/src/library-authoring/units/LibraryUnitPage.tsx b/src/library-authoring/units/LibraryUnitPage.tsx
@@ -137,7 +137,7 @@ export const LibraryUnitPage = () => {
     setDefaultTab({
       collection: COLLECTION_INFO_TABS.Details,
       component: COMPONENT_INFO_TABS.Manage,
-      unit: UNIT_INFO_TABS.Organize,
+      unit: UNIT_INFO_TABS.Manage,
     });
     setHiddenTabs([COMPONENT_INFO_TABS.Preview, UNIT_INFO_TABS.Preview]);
     return () => {

Original file line number	Diff line number	Diff line change
`@@ -147,7 +147,9 @@ const ContainerCardPreview = ({ containerId, showMaxChildren = 5 }: ContainerCar`
`147`	`147`	`}`
`148`	`148`	`return (`
`149`	`149`	`<div`
`150`		- key={`container-card-preview-block-${id}`}
	`150`	`+ // A container can have multiple instances of the same block`
	`151`	`+ // eslint-disable-next-line react/no-array-index-key`
	`152`	+ key={`${id}-${idx}`}
`151`	`153`	`className={classNames}`
`152`	`154`	`>`
`153`	`155`	`{blockPreview}`