From afea96e522612a3006783c650d4ce791577e71be Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 15:27:14 +0530 Subject: [PATCH 1/9] feat: handle duplicate children in container pages Adds index number to url and allow selecting one instance and removing it without affecting other instances in the same page --- .../CompareContainersWidget.tsx | 3 +- .../common/context/SidebarContext.tsx | 29 ++++++---- .../component-info/ComponentInfo.tsx | 4 +- .../components/ComponentMenu.tsx | 8 ++- .../components/ComponentRemover.tsx | 53 ++++++++++++++++--- .../containers/ContainerCard.tsx | 4 +- .../containers/ContainerRemover.tsx | 26 +++++++-- src/library-authoring/data/api.mocks.ts | 32 ++++++----- src/library-authoring/data/api.ts | 4 +- src/library-authoring/data/apiHooks.ts | 9 ++-- src/library-authoring/routes.ts | 13 +++-- .../LibraryContainerChildren.tsx | 19 ++++--- .../units/LibraryUnitBlocks.tsx | 23 ++++---- .../units/LibraryUnitPage.test.tsx | 37 +++++++++++-- 14 files changed, 194 insertions(+), 70 deletions(-) diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index cb1b318bf4..6cb93f4c69 100644 --- a/src/container-comparison/CompareContainersWidget.tsx +++ b/src/container-comparison/CompareContainersWidget.tsx @@ -21,6 +21,7 @@ import { } from './types'; import { diffPreviewContainerChildren, isRowClickable } from './utils'; import messages from './messages'; +import { Container, LibraryBlockMetadata } from '@src/library-authoring/data/api'; interface ContainerInfoProps { upstreamBlockId: string; @@ -60,7 +61,7 @@ const CompareContainersWidgetInner = ({ data: libData, isError: isLibError, error: libError, - } = useContainerChildren(state === 'removed' ? undefined : upstreamBlockId, true); + } = useContainerChildren(state === 'removed' ? undefined : upstreamBlockId, true); const { data: containerData, isError: isContainerTitleError, diff --git a/src/library-authoring/common/context/SidebarContext.tsx b/src/library-authoring/common/context/SidebarContext.tsx index fea6d9c353..435089bcd3 100644 --- a/src/library-authoring/common/context/SidebarContext.tsx +++ b/src/library-authoring/common/context/SidebarContext.tsx @@ -72,6 +72,7 @@ export interface DefaultTabs { export interface SidebarItemInfo { type: SidebarBodyItemId; id: string; + index?: number; } export enum SidebarActions { @@ -88,7 +89,7 @@ export type SidebarContextData = { openCollectionInfoSidebar: (collectionId: string) => void; openComponentInfoSidebar: (usageKey: string) => void; openContainerInfoSidebar: (usageKey: string) => void; - openItemSidebar: (selectedItemId: string, type: SidebarBodyItemId) => void; + openItemSidebar: (selectedItemId: string, type: SidebarBodyItemId, index?: number) => void; sidebarItemInfo?: SidebarItemInfo; sidebarAction: SidebarActions; setSidebarAction: (action: SidebarActions) => void; @@ -154,35 +155,38 @@ export const SidebarProvider = ({ setSidebarItemInfo({ id: '', type: SidebarBodyItemId.Info }); }, []); - const openComponentInfoSidebar = useCallback((usageKey: string) => { + const openComponentInfoSidebar = useCallback((usageKey: string, index?: number) => { setSidebarItemInfo({ id: usageKey, type: SidebarBodyItemId.ComponentInfo, + index, }); }, []); - const openCollectionInfoSidebar = useCallback((newCollectionId: string) => { + const openCollectionInfoSidebar = useCallback((newCollectionId: string, index?: number) => { setSidebarItemInfo({ id: newCollectionId, type: SidebarBodyItemId.CollectionInfo, + index, }); }, []); - const openContainerInfoSidebar = useCallback((usageKey: string) => { + const openContainerInfoSidebar = useCallback((usageKey: string, index?: number) => { setSidebarItemInfo({ id: usageKey, type: SidebarBodyItemId.ContainerInfo, + index, }); }, []); const { navigateTo } = useLibraryRoutes(); - const openItemSidebar = useCallback((selectedItemId: string, type: SidebarBodyItemId) => { - navigateTo({ selectedItemId }); - setSidebarItemInfo({ id: selectedItemId, type }); + const openItemSidebar = useCallback((selectedItemId: string, type: SidebarBodyItemId, index?: number) => { + navigateTo({ selectedItemId, index }); + setSidebarItemInfo({ id: selectedItemId, type, index }); }, [navigateTo, setSidebarItemInfo]); // Set the initial sidebar state based on the URL parameters and context. - const { selectedItemId } = useParams(); + const { selectedItemId, index: indexParam } = useParams(); const { collectionId, containerId } = useLibraryContext(); const { componentPickerMode } = useComponentPickerContext(); @@ -198,12 +202,15 @@ export const SidebarProvider = ({ // Handle selected item id changes if (selectedItemId) { + // if a item is selected that means we have list of items displayed + // which means we can get the index from url and set it. + const indexNumber = indexParam ? Number(indexParam): undefined; if (selectedItemId.startsWith('lct:')) { - openContainerInfoSidebar(selectedItemId); + openContainerInfoSidebar(selectedItemId, indexNumber); } else if (selectedItemId.startsWith('lb:')) { - openComponentInfoSidebar(selectedItemId); + openComponentInfoSidebar(selectedItemId, indexNumber); } else { - openCollectionInfoSidebar(selectedItemId); + openCollectionInfoSidebar(selectedItemId, indexNumber); } } else if (collectionId) { openCollectionInfoSidebar(collectionId); diff --git a/src/library-authoring/component-info/ComponentInfo.tsx b/src/library-authoring/component-info/ComponentInfo.tsx index 967a4050a6..8e8d8adaa2 100644 --- a/src/library-authoring/component-info/ComponentInfo.tsx +++ b/src/library-authoring/component-info/ComponentInfo.tsx @@ -111,6 +111,8 @@ const ComponentActions = ({ const [isPublisherOpen, openPublisher, closePublisher] = useToggle(false); const canEdit = canEditComponent(componentId); + const { sidebarItemInfo } = useSidebarContext(); + if (isPublisherOpen) { return (
- +
); diff --git a/src/library-authoring/components/ComponentMenu.tsx b/src/library-authoring/components/ComponentMenu.tsx index 1a46ce50f2..74381870ad 100644 --- a/src/library-authoring/components/ComponentMenu.tsx +++ b/src/library-authoring/components/ComponentMenu.tsx @@ -23,7 +23,12 @@ import containerMessages from '../containers/messages'; import { useLibraryRoutes } from '../routes'; import { useRunOnNextRender } from '../../utils'; -export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { +interface Props { + usageKey: string; + index?: number; +} + +export const ComponentMenu = ({ usageKey, index }: Props) => { const intl = useIntl(); const { libraryId, @@ -135,6 +140,7 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { {isRemoveModalOpen && ( )} diff --git a/src/library-authoring/components/ComponentRemover.tsx b/src/library-authoring/components/ComponentRemover.tsx index 1f3a1ca6df..070345a379 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -9,26 +9,33 @@ import { useSidebarContext } from '../common/context/SidebarContext'; import { useContainer, useRemoveContainerChildren, - useAddItemsToContainer, useLibraryBlockMetadata, + useContainerChildren, + useUpdateContainerChildren, } from '../data/apiHooks'; import messages from './messages'; +import { LibraryBlockMetadata } from '../data/api'; interface Props { usageKey: string; + index?: number; close: () => void; } -const ComponentRemover = ({ usageKey, close }: Props) => { +const ComponentRemover = ({ usageKey, index, close }: Props) => { const intl = useIntl(); const { sidebarItemInfo, closeLibrarySidebar } = useSidebarContext(); - const { containerId } = useLibraryContext(); + const { containerId, showOnlyPublished } = useLibraryContext(); const { showToast } = useContext(ToastContext); const removeContainerItemMutation = useRemoveContainerChildren(containerId); - const addItemToContainerMutation = useAddItemsToContainer(containerId); + const updateContainerChildrenMutation = useUpdateContainerChildren(containerId); const { data: container, isPending: isPendingParentContainer } = useContainer(containerId); const { data: component, isPending } = useLibraryBlockMetadata(usageKey); + // Use update api for children if duplicates are present to avoid removing all instances of the child + const { data: children } = useContainerChildren(containerId, showOnlyPublished); + const childrenUsageIds = children?.map((child) => child.id); + const hasDuplicates = (childrenUsageIds?.filter((child) => child === usageKey).length || 0) > 1; // istanbul ignore if: loading state if (isPending || isPendingParentContainer) { @@ -37,8 +44,11 @@ const ComponentRemover = ({ usageKey, close }: Props) => { } const removeFromContainer = () => { + if (!childrenUsageIds) { + return; + } const restoreComponent = () => { - addItemToContainerMutation.mutateAsync([usageKey]).then(() => { + updateContainerChildrenMutation.mutateAsync(childrenUsageIds).then(() => { showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); }).catch(() => { showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); @@ -63,6 +73,37 @@ const ComponentRemover = ({ usageKey, close }: Props) => { close(); }; + const excludeOneInstance = () => { + if (!childrenUsageIds || typeof index === 'undefined') { + return; + } + const updatedKeys = childrenUsageIds.filter((childId, idx) => childId !== usageKey || idx !== index); + const restoreComponent = () => { + updateContainerChildrenMutation.mutateAsync(childrenUsageIds).then(() => { + showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); + }).catch(() => { + showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); + }); + }; + updateContainerChildrenMutation.mutateAsync(updatedKeys).then(() => { + if (sidebarItemInfo?.id === usageKey && sidebarItemInfo?.index === index) { + // Close sidebar if current component is open + closeLibrarySidebar(); + } + showToast( + intl.formatMessage(messages.removeComponentFromContainerSuccess), + { + label: intl.formatMessage(messages.undoRemoveComponentFromContainerToastAction), + onClick: restoreComponent, + }, + ); + }).catch(() => { + showToast(intl.formatMessage(messages.removeComponentFromContainerFailure)); + }); + + close(); + } + const removeText = intl.formatMessage(messages.removeComponentConfirm, { componentName: {component?.displayName}, parentContainerName: {container?.displayName}, @@ -76,7 +117,7 @@ const ComponentRemover = ({ usageKey, close }: Props) => { title={intl.formatMessage(messages.removeComponentWarningTitle)} icon={Warning} description={removeText} - onDeleteSubmit={removeFromContainer} + onDeleteSubmit={hasDuplicates ? excludeOneInstance: removeFromContainer} btnLabel={intl.formatMessage(messages.componentRemoveButtonLabel)} buttonVariant="primary" /> diff --git a/src/library-authoring/containers/ContainerCard.tsx b/src/library-authoring/containers/ContainerCard.tsx index fb1132af7e..8defc81d9e 100644 --- a/src/library-authoring/containers/ContainerCard.tsx +++ b/src/library-authoring/containers/ContainerCard.tsx @@ -31,9 +31,10 @@ import AddComponentWidget from '../components/AddComponentWidget'; type ContainerMenuProps = { containerKey: string; displayName: string; + index?: number; }; -export const ContainerMenu = ({ containerKey, displayName } : ContainerMenuProps) => { +export const ContainerMenu = ({ containerKey, displayName, index } : ContainerMenuProps) => { const intl = useIntl(); const { libraryId, collectionId, containerId } = useLibraryContext(); const { @@ -144,6 +145,7 @@ export const ContainerMenu = ({ containerKey, displayName } : ContainerMenuProps close={cancelRemove} containerKey={containerKey} displayName={displayName} + index={index} /> )} diff --git a/src/library-authoring/containers/ContainerRemover.tsx b/src/library-authoring/containers/ContainerRemover.tsx index 2261edb593..bdd105d893 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -9,30 +9,38 @@ import { getBlockType } from '@src/generic/key-utils'; import { useSidebarContext } from '../common/context/SidebarContext'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { useContainer, useRemoveContainerChildren } from '../data/apiHooks'; +import { useContainer, useContainerChildren, useRemoveContainerChildren, useUpdateContainerChildren } from '../data/apiHooks'; import messages from '../components/messages'; +import { Container } from '../data/api'; type ContainerRemoverProps = { close: () => void, containerKey: string, displayName: string, + index?: number, }; const ContainerRemover = ({ close, containerKey, displayName, + index, }: ContainerRemoverProps) => { const intl = useIntl(); const { sidebarItemInfo, closeLibrarySidebar, } = useSidebarContext(); - const { containerId } = useLibraryContext(); + const { containerId, showOnlyPublished } = useLibraryContext(); const { showToast } = useContext(ToastContext); const removeContainerMutation = useRemoveContainerChildren(containerId); + const updateContainerChildrenMutation = useUpdateContainerChildren(containerId); const { data: container, isPending } = useContainer(containerId); + // Use update api for children if duplicates are present to avoid removing all instances of the child + const { data: children } = useContainerChildren(containerId, showOnlyPublished); + const childrenUsageIds = children?.map((child) => child.id); + const hasDuplicates = (childrenUsageIds?.filter((child) => child === containerKey).length || 0) > 1; const itemType = getBlockType(containerKey); const removeWarningTitle = intl.formatMessage(messages.removeContainerWarningTitle, { @@ -50,9 +58,17 @@ const ContainerRemover = ({ const onRemove = useCallback(async () => { try { - await removeContainerMutation.mutateAsync([containerKey]); - if (sidebarItemInfo?.id === containerKey) { - closeLibrarySidebar(); + if (hasDuplicates && childrenUsageIds && typeof index !== 'undefined') { + const updatedKeys = childrenUsageIds.filter((childId, idx) => childId !== containerKey || idx !== index); + await updateContainerChildrenMutation.mutateAsync(updatedKeys); + if (sidebarItemInfo?.id === containerKey && sidebarItemInfo?.index === index) { + closeLibrarySidebar(); + } + } else { + await removeContainerMutation.mutateAsync([containerKey]); + if (sidebarItemInfo?.id === containerKey) { + closeLibrarySidebar(); + } } showToast(removeSuccess); } catch (e) { diff --git a/src/library-authoring/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index 44151f34ba..5d4ca3041f 100644 --- a/src/library-authoring/data/api.mocks.ts +++ b/src/library-authoring/data/api.mocks.ts @@ -603,6 +603,7 @@ mockGetContainerMetadata.applyMock = () => { export async function mockGetContainerChildren(containerId: string): Promise { let numChildren: number; let blockType = 'html'; + let addDuplicate = false; switch (containerId) { case mockGetContainerMetadata.unitId: case mockGetContainerMetadata.sectionId: @@ -615,6 +616,10 @@ export async function mockGetContainerChildren(containerId: string): Promise ( - { - ...child, - // Generate a unique ID for each child block to avoid "duplicate key" errors in tests - id: `${typeNamespace}:org1:Demo_course_generated:${blockType}:${name}-${idx}`, - displayName: `${name} block ${idx}`, - publishedDisplayName: `${name} block published ${idx}`, - blockType, - } - )), - ); + let result =Array(numChildren).fill(mockGetContainerChildren.childTemplate).map((child, idx) => ( + { + ...child, + // Generate a unique ID for each child block to avoid "duplicate key" errors in tests + id: `${typeNamespace}:org1:Demo_course_generated:${blockType}:${name}-${idx}`, + displayName: `${name} block ${idx}`, + publishedDisplayName: `${name} block published ${idx}`, + blockType, + } + )); + if (addDuplicate) { + result = [...result, result[0]]; + } + return Promise.resolve(result); } +mockGetContainerChildren.unitIdWithDuplicate = 'lct:org1:Demo_Course:unit:unit-duplicate'; mockGetContainerChildren.fiveChildren = 'lct:org1:Demo_Course:unit:unit-5'; mockGetContainerChildren.sixChildren = 'lct:org1:Demo_Course:unit:unit-6'; mockGetContainerChildren.childTemplate = { diff --git a/src/library-authoring/data/api.ts b/src/library-authoring/data/api.ts index f1d29d4c21..d54b3934d6 100644 --- a/src/library-authoring/data/api.ts +++ b/src/library-authoring/data/api.ts @@ -702,10 +702,10 @@ export async function restoreContainer(containerId: string) { /** * Fetch a library container's children's metadata. */ -export async function getLibraryContainerChildren( +export async function getLibraryContainerChildren( containerId: string, published: boolean = false, -): Promise { +): Promise { const { data } = await getAuthenticatedHttpClient().get( getLibraryContainerChildrenApiUrl(containerId, published), ); diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index 64e571d94e..dfe9b828cc 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -736,15 +736,12 @@ export const useRestoreContainer = (containerId: string) => { /** * Get the metadata and children for a container in a library */ -export const useContainerChildren = (containerId?: string, published: boolean = false) => ( +export const useContainerChildren = (containerId?: string, published: boolean = false) => ( useQuery({ enabled: !!containerId, queryKey: libraryAuthoringQueryKeys.containerChildren(containerId!), - queryFn: () => api.getLibraryContainerChildren(containerId!, published), - structuralSharing: ( - oldData: api.LibraryBlockMetadata[] | api.Container[], - newData: api.LibraryBlockMetadata[] | api.Container[], - ) => { + queryFn: () => api.getLibraryContainerChildren(containerId!, published), + structuralSharing: (oldData: T[], newData: T[]) => { // This just sets `isNew` flag to new children components if (oldData) { const oldDataIds = oldData.map((obj) => obj.id); diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts index 82c1813298..58dba0da05 100644 --- a/src/library-authoring/routes.ts +++ b/src/library-authoring/routes.ts @@ -33,13 +33,13 @@ export const ROUTES = { COLLECTION: '/collection/:collectionId/:selectedItemId?', // LibrarySectionPage route: // * with a selected containerId and an optionally selected subsection. - SECTION: '/section/:containerId/:selectedItemId?', + SECTION: '/section/:containerId/:selectedItemId?/:index?', // LibrarySubsectionPage route: // * with a selected containerId and an optionally selected unit. - SUBSECTION: '/subsection/:containerId/:selectedItemId?', + SUBSECTION: '/subsection/:containerId/:selectedItemId?/:index?', // LibraryUnitPage route: // * with a selected containerId and/or an optionally selected componentId. - UNIT: '/unit/:containerId/:selectedItemId?', + UNIT: '/unit/:containerId/:selectedItemId?/:index?', // LibraryBackupPage route: BACKUP: '/backup', }; @@ -60,6 +60,7 @@ export type NavigateToData = { collectionId?: string, containerId?: string, contentType?: ContentType, + index?: number, }; export type LibraryRoutesData = { @@ -122,6 +123,7 @@ export const useLibraryRoutes = (): LibraryRoutesData => { collectionId, containerId, contentType, + index, }: NavigateToData = {}) => { const routeParams = { ...params, @@ -129,6 +131,7 @@ export const useLibraryRoutes = (): LibraryRoutesData => { ...((selectedItemId !== undefined) && { selectedItemId }), ...((containerId !== undefined) && { containerId }), ...((collectionId !== undefined) && { collectionId }), + ...((index !== undefined) && { index }), }; let route: string; @@ -230,6 +233,10 @@ export const useLibraryRoutes = (): LibraryRoutesData => { route = ROUTES.HOME; } + if (routeParams.selectedItemId === undefined) { + routeParams.index = undefined; + } + // Also remove the `sa` (sidebar action) search param if it exists. searchParams.delete('sa'); diff --git a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx index 9a315a4aa0..797062a632 100644 --- a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx +++ b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx @@ -39,9 +39,10 @@ interface LibraryContainerMetadataWithUniqueId extends Container { interface ContainerRowProps extends LibraryContainerChildrenProps { container: LibraryContainerMetadataWithUniqueId; + index?: number; } -const ContainerRow = ({ containerKey, container, readOnly }: ContainerRowProps) => { +const ContainerRow = ({ containerKey, container, readOnly, index }: ContainerRowProps) => { const intl = useIntl(); const { showToast } = useContext(ToastContext); const updateMutation = useUpdateContainer(container.originalId, containerKey); @@ -112,6 +113,7 @@ const ContainerRow = ({ containerKey, container, readOnly }: ContainerRowProps) )} @@ -148,7 +150,7 @@ export const LibraryContainerChildren = ({ containerKey, readOnly }: LibraryCont isLoading, isError, error, - } = useContainerChildren(containerKey, showOnlyPublished); + } = useContainerChildren(containerKey, showOnlyPublished); useEffect(() => { // Create new ids which are unique using index. @@ -164,14 +166,14 @@ export const LibraryContainerChildren = ({ containerKey, readOnly }: LibraryCont return setOrderedChildren(newChildren || []); }, [children, setOrderedChildren]); - const handleChildClick = useCallback((child: LibraryContainerMetadataWithUniqueId, numberOfClicks: number) => { + const handleChildClick = useCallback((child: LibraryContainerMetadataWithUniqueId, numberOfClicks: number, index: number) => { if (readOnly) { // don't allow interaction if rendered as preview return; } const doubleClicked = numberOfClicks > 1; if (!doubleClicked) { - openItemSidebar(child.originalId, SidebarBodyItemId.ContainerInfo); + openItemSidebar(child.originalId, SidebarBodyItemId.ContainerInfo, index); } else { navigateTo({ containerId: child.originalId }); } @@ -215,7 +217,7 @@ export const LibraryContainerChildren = ({ containerKey, readOnly }: LibraryCont activeId={activeDraggingId} setActiveId={setActiveDraggingId} > - {orderedChildren?.map((child) => ( + {orderedChildren?.map((child, index) => ( // A container can have multiple instances of the same block // eslint-disable-next-line react/no-array-index-key skipIfUnwantedTarget(e, (event) => handleChildClick(child, event.detail))} + onClick={(e) => skipIfUnwantedTarget(e, (event) => handleChildClick(child, event.detail, index))} onKeyDown={(e) => { if (e.key === 'Enter') { - handleChildClick(child, 1); + handleChildClick(child, 1, index); } }} disabled={readOnly || libReadOnly} - cardClassName={sidebarItemInfo?.id === child.originalId ? 'selected' : undefined} + cardClassName={sidebarItemInfo?.id === child.originalId && sidebarItemInfo?.index === index ? 'selected' : undefined} actions={( )} /> diff --git a/src/library-authoring/units/LibraryUnitBlocks.tsx b/src/library-authoring/units/LibraryUnitBlocks.tsx index 7bd5985fba..508fbd646a 100644 --- a/src/library-authoring/units/LibraryUnitBlocks.tsx +++ b/src/library-authoring/units/LibraryUnitBlocks.tsx @@ -46,13 +46,14 @@ interface LibraryBlockMetadataWithUniqueId extends LibraryBlockMetadata { } interface ComponentBlockProps { + index: number; block: LibraryBlockMetadataWithUniqueId; readOnly?: boolean; isDragging?: boolean; } /** Component header */ -const BlockHeader = ({ block, readOnly }: ComponentBlockProps) => { +const BlockHeader = ({ block, index, readOnly }: ComponentBlockProps) => { const intl = useIntl(); const { showOnlyPublished } = useLibraryContext(); const { showToast } = useContext(ToastContext); @@ -118,17 +119,16 @@ const BlockHeader = ({ block, readOnly }: ComponentBlockProps) => { )} - {!readOnly && } + {!readOnly && } ); }; /** ComponentBlock to render preview of given component under Unit */ -const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => { - const { showOnlyPublished } = useLibraryContext(); +const ComponentBlock = ({ block, readOnly, isDragging, index }: ComponentBlockProps) => { + const { showOnlyPublished, openComponentEditor } = useLibraryContext(); - const { openComponentEditor } = useLibraryContext(); const { sidebarItemInfo, openItemSidebar } = useSidebarContext(); const handleComponentSelection = useCallback((numberOfClicks: number) => { @@ -136,7 +136,11 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => // don't allow interaction if rendered as preview return; } - openItemSidebar(block.originalId, SidebarBodyItemId.ComponentInfo); + openItemSidebar( + block.originalId, + SidebarBodyItemId.ComponentInfo, + index, + ); const canEdit = canEditComponent(block.originalId); if (numberOfClicks > 1 && canEdit) { // Open editor on double click. @@ -174,7 +178,7 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => } + actions={} actionStyle={{ borderRadius: '8px 8px 0px 0px', padding: '0.5rem 1rem', @@ -189,7 +193,7 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => } }} disabled={readOnly} - cardClassName={sidebarItemInfo?.id === block.originalId ? 'selected' : undefined} + cardClassName={sidebarItemInfo?.id === block.originalId && sidebarItemInfo?.index === index ? 'selected' : undefined} > {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
(unitId, showOnlyPublished); const handleReorder = useCallback(() => async (newOrder?: LibraryBlockMetadataWithUniqueId[]) => { if (!newOrder) { @@ -294,6 +298,7 @@ export const LibraryUnitBlocks = ({ unitId, readOnly: componentReadOnly }: Libra // eslint-disable-next-line react/no-array-index-key key={`${block.originalId}-${idx}-${block.modified}`} block={block} + index={idx} isDragging={hidePreviewFor === block.id} readOnly={readOnly} /> diff --git a/src/library-authoring/units/LibraryUnitPage.test.tsx b/src/library-authoring/units/LibraryUnitPage.test.tsx index 1f9fe10eac..2243181378 100644 --- a/src/library-authoring/units/LibraryUnitPage.test.tsx +++ b/src/library-authoring/units/LibraryUnitPage.test.tsx @@ -381,15 +381,44 @@ describe('', () => { const restoreFn = mockShowToast.mock.calls[0][1].onClick; const restoreUrl = getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId); - axiosMock.onPost(restoreUrl).reply(200); + axiosMock.onPatch(restoreUrl).reply(200); // restore collection restoreFn(); await waitFor(() => { - expect(axiosMock.history.post.length).toEqual(1); + expect(axiosMock.history.patch.length).toEqual(1); }); expect(mockShowToast).toHaveBeenCalledWith('Undo successful'); }); + it('should remove only one instance of component even if it is present multiple times in library', async () => { + const user = userEvent.setup(); + const url = getLibraryContainerChildrenApiUrl(mockGetContainerChildren.unitIdWithDuplicate); + axiosMock.onPatch(url).reply(200); + renderLibraryUnitPage(mockGetContainerChildren.unitIdWithDuplicate); + + expect((await screen.findAllByText('text block 0')).length).toEqual(2); + const menu = (await screen.findAllByRole('button', { name: /component actions menu/i }))[0]; + await user.click(menu); + + const removeButton = await screen.findByText('Remove from unit'); + await user.click(removeButton); + + const modal = await screen.findByRole('dialog', { name: 'Remove Component' }); + expect(modal).toBeVisible(); + + const confirmButton = await within(modal).findByRole('button', { name: 'Remove' }); + await user.click(confirmButton); + const result = await mockGetContainerChildren(mockGetContainerChildren.unitIdWithDuplicate); + const resultIds = result.map((obj) => obj.id); + + await waitFor(() => { + expect(axiosMock.history.patch[0].url).toEqual(url); + // Only the first element is removed even though the last element has the same id. + expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); + }); + await waitFor(() => expect(mockShowToast).toHaveBeenCalled()); + }); + it('should show error on remove a component', async () => { const user = userEvent.setup(); const url = getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId); @@ -444,11 +473,11 @@ describe('', () => { const restoreFn = mockShowToast.mock.calls[0][1].onClick; const restoreUrl = getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId); - axiosMock.onPost(restoreUrl).reply(404); + axiosMock.onPatch(restoreUrl).reply(404); // restore collection restoreFn(); await waitFor(() => { - expect(axiosMock.history.post.length).toEqual(1); + expect(axiosMock.history.patch.length).toEqual(1); }); expect(mockShowToast).toHaveBeenCalledWith('Failed to undo remove component operation'); }); From 836e3a4c9c4fe3f729b29190c6edcb286fe7353b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 15:29:50 +0530 Subject: [PATCH 2/9] fix: lint issues --- .../CompareContainersWidget.tsx | 2 +- .../common/context/SidebarContext.tsx | 2 +- .../components/ComponentRemover.tsx | 6 +-- .../containers/ContainerRemover.tsx | 4 +- src/library-authoring/data/api.mocks.ts | 2 +- src/library-authoring/data/apiHooks.ts | 43 ++++++++++--------- .../LibraryContainerChildren.tsx | 10 ++++- .../units/LibraryUnitBlocks.tsx | 4 +- 8 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index 6cb93f4c69..06968a503c 100644 --- a/src/container-comparison/CompareContainersWidget.tsx +++ b/src/container-comparison/CompareContainersWidget.tsx @@ -13,6 +13,7 @@ import { LoadingSpinner } from '@src/generic/Loading'; import { useContainer, useContainerChildren } from '@src/library-authoring/data/apiHooks'; import { BoldText } from '@src/utils'; +import { Container, LibraryBlockMetadata } from '@src/library-authoring/data/api'; import ChildrenPreview from './ChildrenPreview'; import ContainerRow from './ContainerRow'; import { useCourseContainerChildren } from './data/apiHooks'; @@ -21,7 +22,6 @@ import { } from './types'; import { diffPreviewContainerChildren, isRowClickable } from './utils'; import messages from './messages'; -import { Container, LibraryBlockMetadata } from '@src/library-authoring/data/api'; interface ContainerInfoProps { upstreamBlockId: string; diff --git a/src/library-authoring/common/context/SidebarContext.tsx b/src/library-authoring/common/context/SidebarContext.tsx index 435089bcd3..d1323adf6f 100644 --- a/src/library-authoring/common/context/SidebarContext.tsx +++ b/src/library-authoring/common/context/SidebarContext.tsx @@ -204,7 +204,7 @@ export const SidebarProvider = ({ if (selectedItemId) { // if a item is selected that means we have list of items displayed // which means we can get the index from url and set it. - const indexNumber = indexParam ? Number(indexParam): undefined; + const indexNumber = indexParam ? Number(indexParam) : undefined; if (selectedItemId.startsWith('lct:')) { openContainerInfoSidebar(selectedItemId, indexNumber); } else if (selectedItemId.startsWith('lb:')) { diff --git a/src/library-authoring/components/ComponentRemover.tsx b/src/library-authoring/components/ComponentRemover.tsx index 070345a379..737b02820d 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -86,7 +86,7 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { }); }; updateContainerChildrenMutation.mutateAsync(updatedKeys).then(() => { - if (sidebarItemInfo?.id === usageKey && sidebarItemInfo?.index === index) { + if (sidebarItemInfo?.id === usageKey && sidebarItemInfo?.index === index) { // Close sidebar if current component is open closeLibrarySidebar(); } @@ -102,7 +102,7 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { }); close(); - } + }; const removeText = intl.formatMessage(messages.removeComponentConfirm, { componentName: {component?.displayName}, @@ -117,7 +117,7 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { title={intl.formatMessage(messages.removeComponentWarningTitle)} icon={Warning} description={removeText} - onDeleteSubmit={hasDuplicates ? excludeOneInstance: removeFromContainer} + onDeleteSubmit={hasDuplicates ? excludeOneInstance : removeFromContainer} btnLabel={intl.formatMessage(messages.componentRemoveButtonLabel)} buttonVariant="primary" /> diff --git a/src/library-authoring/containers/ContainerRemover.tsx b/src/library-authoring/containers/ContainerRemover.tsx index bdd105d893..dd23b7cbde 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -9,7 +9,9 @@ import { getBlockType } from '@src/generic/key-utils'; import { useSidebarContext } from '../common/context/SidebarContext'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { useContainer, useContainerChildren, useRemoveContainerChildren, useUpdateContainerChildren } from '../data/apiHooks'; +import { + useContainer, useContainerChildren, useRemoveContainerChildren, useUpdateContainerChildren, +} from '../data/apiHooks'; import messages from '../components/messages'; import { Container } from '../data/api'; diff --git a/src/library-authoring/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index 5d4ca3041f..760642bb55 100644 --- a/src/library-authoring/data/api.mocks.ts +++ b/src/library-authoring/data/api.mocks.ts @@ -635,7 +635,7 @@ export async function mockGetContainerChildren(containerId: string): Promise ( + let result = Array(numChildren).fill(mockGetContainerChildren.childTemplate).map((child, idx) => ( { ...child, // Generate a unique ID for each child block to avoid "duplicate key" errors in tests diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index dfe9b828cc..ca1a30c14f 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -736,29 +736,32 @@ export const useRestoreContainer = (containerId: string) => { /** * Get the metadata and children for a container in a library */ -export const useContainerChildren = (containerId?: string, published: boolean = false) => ( - useQuery({ - enabled: !!containerId, - queryKey: libraryAuthoringQueryKeys.containerChildren(containerId!), - queryFn: () => api.getLibraryContainerChildren(containerId!, published), - structuralSharing: (oldData: T[], newData: T[]) => { +export const useContainerChildren = ( + containerId?: string, + published: boolean = false, +) => ( + useQuery({ + enabled: !!containerId, + queryKey: libraryAuthoringQueryKeys.containerChildren(containerId!), + queryFn: () => api.getLibraryContainerChildren(containerId!, published), + structuralSharing: (oldData: T[], newData: T[]) => { // This just sets `isNew` flag to new children components - if (oldData) { - const oldDataIds = oldData.map((obj) => obj.id); - // eslint-disable-next-line no-param-reassign - newData = newData.map((newObj) => { - if (!oldDataIds.includes(newObj.id)) { + if (oldData) { + const oldDataIds = oldData.map((obj) => obj.id); + // eslint-disable-next-line no-param-reassign + newData = newData.map((newObj) => { + if (!oldDataIds.includes(newObj.id)) { // Set isNew = true if we have new child on refetch // eslint-disable-next-line no-param-reassign - newObj.isNew = true; - } - return newObj; - }); - } - return replaceEqualDeep(oldData, newData); - }, - }) -); + newObj.isNew = true; + } + return newObj; + }); + } + return replaceEqualDeep(oldData, newData); + }, + }) + ); /** * If you work with `useContentFromSearchIndex`, you can use this diff --git a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx index 797062a632..cf1d731b3e 100644 --- a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx +++ b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx @@ -42,7 +42,9 @@ interface ContainerRowProps extends LibraryContainerChildrenProps { index?: number; } -const ContainerRow = ({ containerKey, container, readOnly, index }: ContainerRowProps) => { +const ContainerRow = ({ + containerKey, container, readOnly, index, +}: ContainerRowProps) => { const intl = useIntl(); const { showToast } = useContext(ToastContext); const updateMutation = useUpdateContainer(container.originalId, containerKey); @@ -166,7 +168,11 @@ export const LibraryContainerChildren = ({ containerKey, readOnly }: LibraryCont return setOrderedChildren(newChildren || []); }, [children, setOrderedChildren]); - const handleChildClick = useCallback((child: LibraryContainerMetadataWithUniqueId, numberOfClicks: number, index: number) => { + const handleChildClick = useCallback(( + child: LibraryContainerMetadataWithUniqueId, + numberOfClicks: number, + index: number, + ) => { if (readOnly) { // don't allow interaction if rendered as preview return; diff --git a/src/library-authoring/units/LibraryUnitBlocks.tsx b/src/library-authoring/units/LibraryUnitBlocks.tsx index 508fbd646a..a96636def9 100644 --- a/src/library-authoring/units/LibraryUnitBlocks.tsx +++ b/src/library-authoring/units/LibraryUnitBlocks.tsx @@ -126,7 +126,9 @@ const BlockHeader = ({ block, index, readOnly }: ComponentBlockProps) => { }; /** ComponentBlock to render preview of given component under Unit */ -const ComponentBlock = ({ block, readOnly, isDragging, index }: ComponentBlockProps) => { +const ComponentBlock = ({ + block, readOnly, isDragging, index, +}: ComponentBlockProps) => { const { showOnlyPublished, openComponentEditor } = useLibraryContext(); const { sidebarItemInfo, openItemSidebar } = useSidebarContext(); From 4d6d9f7fad4fe53be47feb5bb8df2156b07420d6 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 15:40:15 +0530 Subject: [PATCH 3/9] chore: update imports --- src/library-authoring/components/ComponentMenu.tsx | 12 ++++++------ .../components/ComponentRemover.tsx | 8 ++++---- src/library-authoring/containers/ContainerCard.tsx | 14 +++++++------- .../containers/ContainerRemover.tsx | 10 +++++----- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/library-authoring/components/ComponentMenu.tsx b/src/library-authoring/components/ComponentMenu.tsx index 74381870ad..33b67f1e65 100644 --- a/src/library-authoring/components/ComponentMenu.tsx +++ b/src/library-authoring/components/ComponentMenu.tsx @@ -12,16 +12,16 @@ import { useClipboard } from '@src/generic/clipboard'; import { getBlockType } from '@src/generic/key-utils'; import { ToastContext } from '@src/generic/toast-context'; -import { useLibraryContext } from '../common/context/LibraryContext'; -import { SidebarActions, SidebarBodyItemId, useSidebarContext } from '../common/context/SidebarContext'; -import { useRemoveItemsFromCollection } from '../data/apiHooks'; +import { useLibraryContext } from '@src/library-authoring/common/context/LibraryContext'; +import { SidebarActions, SidebarBodyItemId, useSidebarContext } from '@src/library-authoring/common/context/SidebarContext'; +import { useRemoveItemsFromCollection } from '@src/library-authoring/data/apiHooks'; +import containerMessages from '@src/library-authoring/containers/messages'; +import { useLibraryRoutes } from '@src/library-authoring/routes'; +import { useRunOnNextRender } from '@src/utils'; import { canEditComponent } from './ComponentEditorModal'; import ComponentDeleter from './ComponentDeleter'; import ComponentRemover from './ComponentRemover'; import messages from './messages'; -import containerMessages from '../containers/messages'; -import { useLibraryRoutes } from '../routes'; -import { useRunOnNextRender } from '../../utils'; interface Props { usageKey: string; diff --git a/src/library-authoring/components/ComponentRemover.tsx b/src/library-authoring/components/ComponentRemover.tsx index 737b02820d..6983beadcf 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -4,17 +4,17 @@ import { Warning } from '@openedx/paragon/icons'; import DeleteModal from '@src/generic/delete-modal/DeleteModal'; import { ToastContext } from '@src/generic/toast-context'; -import { useLibraryContext } from '../common/context/LibraryContext'; -import { useSidebarContext } from '../common/context/SidebarContext'; +import { useLibraryContext } from '@src/library-authoring/common/context/LibraryContext'; +import { useSidebarContext } from '@src/library-authoring/common/context/SidebarContext'; import { useContainer, useRemoveContainerChildren, useLibraryBlockMetadata, useContainerChildren, useUpdateContainerChildren, -} from '../data/apiHooks'; +} from '@src/library-authoring/data/apiHooks'; +import { LibraryBlockMetadata } from '@src/library-authoring/data/api'; import messages from './messages'; -import { LibraryBlockMetadata } from '../data/api'; interface Props { usageKey: string; diff --git a/src/library-authoring/containers/ContainerCard.tsx b/src/library-authoring/containers/ContainerCard.tsx index 8defc81d9e..229c22b7f5 100644 --- a/src/library-authoring/containers/ContainerCard.tsx +++ b/src/library-authoring/containers/ContainerCard.tsx @@ -17,16 +17,16 @@ import { type ContainerHit, Highlight, PublishStatus } from '@src/search-manager import { ToastContext } from '@src/generic/toast-context'; import { useRunOnNextRender } from '@src/utils'; -import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; -import { useLibraryContext } from '../common/context/LibraryContext'; -import { SidebarActions, SidebarBodyItemId, useSidebarContext } from '../common/context/SidebarContext'; -import { useRemoveItemsFromCollection } from '../data/apiHooks'; -import { useLibraryRoutes } from '../routes'; +import { useComponentPickerContext } from '@src/library-authoring/common/context/ComponentPickerContext'; +import { useLibraryContext } from '@src/library-authoring/common/context/LibraryContext'; +import { SidebarActions, SidebarBodyItemId, useSidebarContext } from '@src/library-authoring/common/context/SidebarContext'; +import { useRemoveItemsFromCollection } from '@src/library-authoring/data/apiHooks'; +import { useLibraryRoutes } from '@src/library-authoring/routes'; +import BaseCard from '@src/library-authoring/components/BaseCard'; +import AddComponentWidget from '@src/library-authoring/components/AddComponentWidget'; import messages from './messages'; import ContainerDeleter from './ContainerDeleter'; import ContainerRemover from './ContainerRemover'; -import BaseCard from '../components/BaseCard'; -import AddComponentWidget from '../components/AddComponentWidget'; type ContainerMenuProps = { containerKey: string; diff --git a/src/library-authoring/containers/ContainerRemover.tsx b/src/library-authoring/containers/ContainerRemover.tsx index dd23b7cbde..0e39b8cddd 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -7,13 +7,13 @@ import DeleteModal from '@src/generic/delete-modal/DeleteModal'; import { ToastContext } from '@src/generic/toast-context'; import { getBlockType } from '@src/generic/key-utils'; -import { useSidebarContext } from '../common/context/SidebarContext'; -import { useLibraryContext } from '../common/context/LibraryContext'; +import { useSidebarContext } from '@src/library-authoring/common/context/SidebarContext'; +import { useLibraryContext } from '@src/library-authoring/common/context/LibraryContext'; import { useContainer, useContainerChildren, useRemoveContainerChildren, useUpdateContainerChildren, -} from '../data/apiHooks'; -import messages from '../components/messages'; -import { Container } from '../data/api'; +} from '@src/library-authoring/data/apiHooks'; +import messages from '@src/library-authoring/components/messages'; +import { Container } from '@src/library-authoring/data/api'; type ContainerRemoverProps = { close: () => void, From 2768d495b01ed11bca315fa5d944164390c630c3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 16:48:23 +0530 Subject: [PATCH 4/9] test: add tests --- .../components/ComponentRemover.tsx | 60 +++++------ .../containers/ContainerRemover.test.tsx | 102 ++++++++++++++++++ .../containers/ContainerRemover.tsx | 4 + .../units/LibraryUnitPage.test.tsx | 2 +- 4 files changed, 136 insertions(+), 32 deletions(-) create mode 100644 src/library-authoring/containers/ContainerRemover.test.tsx diff --git a/src/library-authoring/components/ComponentRemover.tsx b/src/library-authoring/components/ComponentRemover.tsx index 6983beadcf..a8c5bfbf07 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -43,31 +43,38 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { return null; } - const removeFromContainer = () => { + const restoreComponent = () => { if (!childrenUsageIds) { return; } - const restoreComponent = () => { - updateContainerChildrenMutation.mutateAsync(childrenUsageIds).then(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); - }).catch(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); - }); - }; + updateContainerChildrenMutation.mutateAsync(childrenUsageIds).then(() => { + showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); + }).catch(() => { + showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); + }); + }; + + const showSuccessToast = () => { + showToast( + intl.formatMessage(messages.removeComponentFromContainerSuccess), + { + label: intl.formatMessage(messages.undoRemoveComponentFromContainerToastAction), + onClick: restoreComponent, + }, + ); + }; + + const showFailureToast = () => showToast(intl.formatMessage(messages.removeComponentFromContainerFailure)); + + const removeFromContainer = () => { removeContainerItemMutation.mutateAsync([usageKey]).then(() => { if (sidebarItemInfo?.id === usageKey) { // Close sidebar if current component is open closeLibrarySidebar(); } - showToast( - intl.formatMessage(messages.removeComponentFromContainerSuccess), - { - label: intl.formatMessage(messages.undoRemoveComponentFromContainerToastAction), - onClick: restoreComponent, - }, - ); + showSuccessToast(); }).catch(() => { - showToast(intl.formatMessage(messages.removeComponentFromContainerFailure)); + showFailureToast(); }); close(); @@ -78,27 +85,18 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { return; } const updatedKeys = childrenUsageIds.filter((childId, idx) => childId !== usageKey || idx !== index); - const restoreComponent = () => { - updateContainerChildrenMutation.mutateAsync(childrenUsageIds).then(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); - }).catch(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); - }); - }; updateContainerChildrenMutation.mutateAsync(updatedKeys).then(() => { if (sidebarItemInfo?.id === usageKey && sidebarItemInfo?.index === index) { // Close sidebar if current component is open closeLibrarySidebar(); } - showToast( - intl.formatMessage(messages.removeComponentFromContainerSuccess), - { - label: intl.formatMessage(messages.undoRemoveComponentFromContainerToastAction), - onClick: restoreComponent, - }, - ); + // Already tested as part of removeFromContainer + // istanbul ignore next + showSuccessToast(); }).catch(() => { - showToast(intl.formatMessage(messages.removeComponentFromContainerFailure)); + // Already tested as part of removeFromContainer + // istanbul ignore next + showFailureToast(); }); close(); diff --git a/src/library-authoring/containers/ContainerRemover.test.tsx b/src/library-authoring/containers/ContainerRemover.test.tsx new file mode 100644 index 0000000000..4b5d9cc1d6 --- /dev/null +++ b/src/library-authoring/containers/ContainerRemover.test.tsx @@ -0,0 +1,102 @@ +import userEvent from '@testing-library/user-event'; +import type MockAdapter from 'axios-mock-adapter'; +import { QueryClient } from '@tanstack/react-query'; + +import { act } from 'react'; +import { + initializeMocks, + fireEvent, + render, + screen, + waitFor, +} from '../../testUtils'; +import { + getLibraryContainerApiUrl, + getLibraryContainerChildrenApiUrl, + getLibraryContainersApiUrl, +} from '../data/api'; +import { + mockContentLibrary, + mockXBlockFields, + mockGetContainerMetadata, + mockGetContainerChildren, + mockLibraryBlockMetadata, +} from '../data/api.mocks'; +import { mockContentSearchConfig, mockGetBlockTypes, mockSearchResult } from '../../search-manager/data/api.mock'; +import { mockClipboardEmpty } from '../../generic/data/api.mock'; +import LibraryLayout from '../LibraryLayout'; +import { ToastActionData, ToastProvider } from '../../generic/toast-context'; +import mockResult from '../__mocks__/subsection-single.json'; +import { ContainerType } from '../../generic/key-utils'; +import ContainerRemover from './ContainerRemover'; +import { LibraryProvider } from '../common/context/LibraryContext'; + +const path = '/library/:libraryId/*'; +const libraryTitle = mockContentLibrary.libraryData.title; + +let axiosMock: MockAdapter; +let queryClient: QueryClient; +let mockShowToast: (message: string, action?: ToastActionData | undefined) => void; + +mockClipboardEmpty.applyMock(); +mockGetContainerMetadata.applyMock(); +mockGetContainerChildren.applyMock(); +mockContentSearchConfig.applyMock(); +mockGetBlockTypes.applyMock(); +mockContentLibrary.applyMock(); +mockXBlockFields.applyMock(); +mockLibraryBlockMetadata.applyMock(); + +const mockClose = jest.fn(); + +const { libraryId } = mockContentLibrary; +const renderModal = (element: React.ReactNode) => { + render( + + + {element} + + , + ) +} + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useParams: jest.fn().mockImplementation(() => ({ + containerId: mockGetContainerChildren.unitIdWithDuplicate, + })), +})); + +describe('', () => { + beforeEach(() => { + ({ axiosMock, mockShowToast, queryClient } = initializeMocks()); + }); + + afterEach(() => { + jest.clearAllMocks(); + axiosMock.restore(); + }); + + it('triggers update container children api call when duplicates are present', async () => { + const user = userEvent.setup(); + const url = getLibraryContainerChildrenApiUrl(mockGetContainerChildren.unitIdWithDuplicate); + axiosMock.onPatch(url).reply(200); + const result = await mockGetContainerChildren(mockGetContainerChildren.unitIdWithDuplicate); + const resultIds = result.map((obj) => obj.id); + renderModal() + const btn = await screen.findByRole('button', { name: 'Remove' }); + await user.click(btn); + + await waitFor(() => { + expect(axiosMock.history.patch[0].url).toEqual(url); + // Only the first element is removed even though the last element has the same id. + expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); + }); + expect(mockClose).toHaveBeenCalled(); + }); +}); diff --git a/src/library-authoring/containers/ContainerRemover.tsx b/src/library-authoring/containers/ContainerRemover.tsx index 0e39b8cddd..854e74486f 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -81,12 +81,16 @@ const ContainerRemover = ({ }, [ containerKey, removeContainerMutation, + updateContainerChildrenMutation, sidebarItemInfo, closeLibrarySidebar, showToast, removeSuccess, removeError, close, + hasDuplicates, + childrenUsageIds, + index, ]); // istanbul ignore if: loading state diff --git a/src/library-authoring/units/LibraryUnitPage.test.tsx b/src/library-authoring/units/LibraryUnitPage.test.tsx index 2243181378..488d217c18 100644 --- a/src/library-authoring/units/LibraryUnitPage.test.tsx +++ b/src/library-authoring/units/LibraryUnitPage.test.tsx @@ -390,7 +390,7 @@ describe('', () => { expect(mockShowToast).toHaveBeenCalledWith('Undo successful'); }); - it('should remove only one instance of component even if it is present multiple times in library', async () => { + it('should remove only one instance of component even if it is present multiple times in this page', async () => { const user = userEvent.setup(); const url = getLibraryContainerChildrenApiUrl(mockGetContainerChildren.unitIdWithDuplicate); axiosMock.onPatch(url).reply(200); From be37af4cc95e97f4461c47f8967980d0dab38b22 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 16:51:03 +0530 Subject: [PATCH 5/9] fix: lint issues --- .../containers/ContainerRemover.test.tsx | 25 +++++-------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/library-authoring/containers/ContainerRemover.test.tsx b/src/library-authoring/containers/ContainerRemover.test.tsx index 4b5d9cc1d6..4abba3e323 100644 --- a/src/library-authoring/containers/ContainerRemover.test.tsx +++ b/src/library-authoring/containers/ContainerRemover.test.tsx @@ -1,19 +1,14 @@ import userEvent from '@testing-library/user-event'; import type MockAdapter from 'axios-mock-adapter'; -import { QueryClient } from '@tanstack/react-query'; -import { act } from 'react'; import { initializeMocks, - fireEvent, render, screen, waitFor, } from '../../testUtils'; import { - getLibraryContainerApiUrl, getLibraryContainerChildrenApiUrl, - getLibraryContainersApiUrl, } from '../data/api'; import { mockContentLibrary, @@ -22,21 +17,13 @@ import { mockGetContainerChildren, mockLibraryBlockMetadata, } from '../data/api.mocks'; -import { mockContentSearchConfig, mockGetBlockTypes, mockSearchResult } from '../../search-manager/data/api.mock'; +import { mockContentSearchConfig, mockGetBlockTypes } from '../../search-manager/data/api.mock'; import { mockClipboardEmpty } from '../../generic/data/api.mock'; -import LibraryLayout from '../LibraryLayout'; -import { ToastActionData, ToastProvider } from '../../generic/toast-context'; -import mockResult from '../__mocks__/subsection-single.json'; -import { ContainerType } from '../../generic/key-utils'; +import { ToastProvider } from '../../generic/toast-context'; import ContainerRemover from './ContainerRemover'; import { LibraryProvider } from '../common/context/LibraryContext'; -const path = '/library/:libraryId/*'; -const libraryTitle = mockContentLibrary.libraryData.title; - let axiosMock: MockAdapter; -let queryClient: QueryClient; -let mockShowToast: (message: string, action?: ToastActionData | undefined) => void; mockClipboardEmpty.applyMock(); mockGetContainerMetadata.applyMock(); @@ -57,8 +44,8 @@ const renderModal = (element: React.ReactNode) => { {element} , - ) -} + ); +}; jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), @@ -69,7 +56,7 @@ jest.mock('react-router-dom', () => ({ describe('', () => { beforeEach(() => { - ({ axiosMock, mockShowToast, queryClient } = initializeMocks()); + ({ axiosMock } = initializeMocks()); }); afterEach(() => { @@ -88,7 +75,7 @@ describe('', () => { containerKey={result[0].id} displayName="Title" index={0} - />) + />); const btn = await screen.findByRole('button', { name: 'Remove' }); await user.click(btn); From 187440f7c0b17e33baf330a69356f6396ab57bdc Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 19:16:30 +0530 Subject: [PATCH 6/9] chore: ignore trivial lines of code --- src/library-authoring/components/ComponentRemover.tsx | 2 ++ src/library-authoring/containers/ContainerRemover.tsx | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/library-authoring/components/ComponentRemover.tsx b/src/library-authoring/components/ComponentRemover.tsx index a8c5bfbf07..b776978b53 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -44,6 +44,7 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { } const restoreComponent = () => { + // istanbul ignore if: this should never happen if (!childrenUsageIds) { return; } @@ -86,6 +87,7 @@ const ComponentRemover = ({ usageKey, index, close }: Props) => { } const updatedKeys = childrenUsageIds.filter((childId, idx) => childId !== usageKey || idx !== index); updateContainerChildrenMutation.mutateAsync(updatedKeys).then(() => { + // istanbul ignore if if (sidebarItemInfo?.id === usageKey && sidebarItemInfo?.index === index) { // Close sidebar if current component is open closeLibrarySidebar(); diff --git a/src/library-authoring/containers/ContainerRemover.tsx b/src/library-authoring/containers/ContainerRemover.tsx index 854e74486f..b60ca6240e 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -63,11 +63,13 @@ const ContainerRemover = ({ if (hasDuplicates && childrenUsageIds && typeof index !== 'undefined') { const updatedKeys = childrenUsageIds.filter((childId, idx) => childId !== containerKey || idx !== index); await updateContainerChildrenMutation.mutateAsync(updatedKeys); + // istanbul ignore if if (sidebarItemInfo?.id === containerKey && sidebarItemInfo?.index === index) { closeLibrarySidebar(); } } else { await removeContainerMutation.mutateAsync([containerKey]); + // istanbul ignore if if (sidebarItemInfo?.id === containerKey) { closeLibrarySidebar(); } From e4e407c961246b74df6e94edc4dc399b112d5aaa Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 30 Oct 2025 19:37:58 +0530 Subject: [PATCH 7/9] chore: update docs --- src/library-authoring/routes.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts index 58dba0da05..c0b4d35da4 100644 --- a/src/library-authoring/routes.ts +++ b/src/library-authoring/routes.ts @@ -233,6 +233,8 @@ export const useLibraryRoutes = (): LibraryRoutesData => { route = ROUTES.HOME; } + // Since index is just the order number of the selectedItemId + // clear index if selectedItemId is undefined if (routeParams.selectedItemId === undefined) { routeParams.index = undefined; } From dbf92474cff6d63b7e001bf18a77ab35b999afe4 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 31 Oct 2025 11:25:17 +0530 Subject: [PATCH 8/9] refactor: container children types --- .../containers/ContainerRemover.test.tsx | 5 ----- src/library-authoring/data/api.ts | 4 ++-- src/library-authoring/data/apiHooks.ts | 15 +++++++++------ 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/library-authoring/containers/ContainerRemover.test.tsx b/src/library-authoring/containers/ContainerRemover.test.tsx index 4abba3e323..f2e72a0add 100644 --- a/src/library-authoring/containers/ContainerRemover.test.tsx +++ b/src/library-authoring/containers/ContainerRemover.test.tsx @@ -59,11 +59,6 @@ describe('', () => { ({ axiosMock } = initializeMocks()); }); - afterEach(() => { - jest.clearAllMocks(); - axiosMock.restore(); - }); - it('triggers update container children api call when duplicates are present', async () => { const user = userEvent.setup(); const url = getLibraryContainerChildrenApiUrl(mockGetContainerChildren.unitIdWithDuplicate); diff --git a/src/library-authoring/data/api.ts b/src/library-authoring/data/api.ts index d54b3934d6..211b1614b2 100644 --- a/src/library-authoring/data/api.ts +++ b/src/library-authoring/data/api.ts @@ -702,10 +702,10 @@ export async function restoreContainer(containerId: string) { /** * Fetch a library container's children's metadata. */ -export async function getLibraryContainerChildren( +export async function getLibraryContainerChildren( containerId: string, published: boolean = false, -): Promise { +): Promise { const { data } = await getAuthenticatedHttpClient().get( getLibraryContainerChildrenApiUrl(containerId, published), ); diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index ca1a30c14f..dca9de9d67 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -736,15 +736,18 @@ export const useRestoreContainer = (containerId: string) => { /** * Get the metadata and children for a container in a library */ -export const useContainerChildren = ( - containerId?: string, - published: boolean = false, -) => ( +export const useContainerChildren = ( + containerId?: string, + published: boolean = false, + ) => ( useQuery({ enabled: !!containerId, queryKey: libraryAuthoringQueryKeys.containerChildren(containerId!), - queryFn: () => api.getLibraryContainerChildren(containerId!, published), - structuralSharing: (oldData: T[], newData: T[]) => { + queryFn: () => api.getLibraryContainerChildren(containerId!, published), + structuralSharing: (oldData: ChildType[], newData: ChildType[]) => { // This just sets `isNew` flag to new children components if (oldData) { const oldDataIds = oldData.map((obj) => obj.id); From 5d929aec32e056499d7de16ca0ea0d9c320e3a6c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 3 Nov 2025 12:41:40 +0530 Subject: [PATCH 9/9] chore: address review comments --- .../containers/ContainerRemover.test.tsx | 19 ++++--------------- .../units/LibraryUnitPage.test.tsx | 4 ++-- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/library-authoring/containers/ContainerRemover.test.tsx b/src/library-authoring/containers/ContainerRemover.test.tsx index f2e72a0add..7a4945238d 100644 --- a/src/library-authoring/containers/ContainerRemover.test.tsx +++ b/src/library-authoring/containers/ContainerRemover.test.tsx @@ -6,33 +6,22 @@ import { render, screen, waitFor, -} from '../../testUtils'; +} from '@src/testUtils'; +import { ToastProvider } from '@src/generic/toast-context'; import { getLibraryContainerChildrenApiUrl, } from '../data/api'; import { mockContentLibrary, - mockXBlockFields, - mockGetContainerMetadata, mockGetContainerChildren, - mockLibraryBlockMetadata, } from '../data/api.mocks'; -import { mockContentSearchConfig, mockGetBlockTypes } from '../../search-manager/data/api.mock'; -import { mockClipboardEmpty } from '../../generic/data/api.mock'; -import { ToastProvider } from '../../generic/toast-context'; import ContainerRemover from './ContainerRemover'; import { LibraryProvider } from '../common/context/LibraryContext'; let axiosMock: MockAdapter; -mockClipboardEmpty.applyMock(); -mockGetContainerMetadata.applyMock(); mockGetContainerChildren.applyMock(); -mockContentSearchConfig.applyMock(); -mockGetBlockTypes.applyMock(); mockContentLibrary.applyMock(); -mockXBlockFields.applyMock(); -mockLibraryBlockMetadata.applyMock(); const mockClose = jest.fn(); @@ -76,9 +65,9 @@ describe('', () => { await waitFor(() => { expect(axiosMock.history.patch[0].url).toEqual(url); - // Only the first element is removed even though the last element has the same id. - expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); }); + // Only the first element is removed even though the last element has the same id. + expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); expect(mockClose).toHaveBeenCalled(); }); }); diff --git a/src/library-authoring/units/LibraryUnitPage.test.tsx b/src/library-authoring/units/LibraryUnitPage.test.tsx index 488d217c18..991436d2fb 100644 --- a/src/library-authoring/units/LibraryUnitPage.test.tsx +++ b/src/library-authoring/units/LibraryUnitPage.test.tsx @@ -413,9 +413,9 @@ describe('', () => { await waitFor(() => { expect(axiosMock.history.patch[0].url).toEqual(url); - // Only the first element is removed even though the last element has the same id. - expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); }); + // Only the first element is removed even though the last element has the same id. + expect(JSON.parse(axiosMock.history.patch[0].data).usage_keys).toEqual(resultIds.slice(1)); await waitFor(() => expect(mockShowToast).toHaveBeenCalled()); });