From 75ae9d549cf6cee6771c0b143dba12357bbb2a05 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 3 Nov 2025 20:29:37 +0530 Subject: [PATCH] feat: handle duplicate children in container pages [FC-0112] (#2584) If we have duplicate container or component in parent page in library, clicking on one of them selects both and removing any one from the parent blocks removes all instances. This PR handles duplicates by including index/order_number of each child component in the url and using it to exclude a single instance and update parent structure. --- .../CompareContainersWidget.tsx | 3 +- .../common/context/SidebarContext.tsx | 29 ++++--- .../component-info/ComponentInfo.tsx | 4 +- .../components/ComponentMenu.tsx | 20 +++-- .../components/ComponentRemover.tsx | 87 ++++++++++++++----- .../containers/ContainerCard.tsx | 18 ++-- .../containers/ContainerRemover.test.tsx | 73 ++++++++++++++++ .../containers/ContainerRemover.tsx | 40 +++++++-- src/library-authoring/data/api.mocks.ts | 32 ++++--- src/library-authoring/data/api.ts | 4 +- src/library-authoring/data/apiHooks.ts | 49 ++++++----- src/library-authoring/routes.ts | 15 +++- .../LibraryContainerChildren.tsx | 25 ++++-- .../units/LibraryUnitBlocks.tsx | 25 ++++-- .../units/LibraryUnitPage.test.tsx | 37 +++++++- 15 files changed, 341 insertions(+), 120 deletions(-) create mode 100644 src/library-authoring/containers/ContainerRemover.test.tsx diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index cb1b318bf..06968a503 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'; @@ -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 fea6d9c35..d1323adf6 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 967a4050a..8e8d8adaa 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 1a46ce50f..33b67f1e6 100644 --- a/src/library-authoring/components/ComponentMenu.tsx +++ b/src/library-authoring/components/ComponentMenu.tsx @@ -12,18 +12,23 @@ 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'; -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 1f3a1ca6d..b776978b5 100644 --- a/src/library-authoring/components/ComponentRemover.tsx +++ b/src/library-authoring/components/ComponentRemover.tsx @@ -4,31 +4,38 @@ 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, - useAddItemsToContainer, useLibraryBlockMetadata, -} from '../data/apiHooks'; + useContainerChildren, + useUpdateContainerChildren, +} from '@src/library-authoring/data/apiHooks'; +import { LibraryBlockMetadata } from '@src/library-authoring/data/api'; import messages from './messages'; 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) { @@ -36,28 +43,62 @@ const ComponentRemover = ({ usageKey, close }: Props) => { return null; } + const restoreComponent = () => { + // istanbul ignore if: this should never happen + if (!childrenUsageIds) { + return; + } + 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 = () => { - const restoreComponent = () => { - addItemToContainerMutation.mutateAsync([usageKey]).then(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastSuccess)); - }).catch(() => { - showToast(intl.formatMessage(messages.undoRemoveComponentFromContainerToastFailed)); - }); - }; 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(); + }; + + const excludeOneInstance = () => { + if (!childrenUsageIds || typeof index === 'undefined') { + return; + } + 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(); + } + // Already tested as part of removeFromContainer + // istanbul ignore next + showSuccessToast(); + }).catch(() => { + // Already tested as part of removeFromContainer + // istanbul ignore next + showFailureToast(); }); close(); @@ -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 fb1132af7..229c22b7f 100644 --- a/src/library-authoring/containers/ContainerCard.tsx +++ b/src/library-authoring/containers/ContainerCard.tsx @@ -17,23 +17,24 @@ 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; 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.test.tsx b/src/library-authoring/containers/ContainerRemover.test.tsx new file mode 100644 index 000000000..7a4945238 --- /dev/null +++ b/src/library-authoring/containers/ContainerRemover.test.tsx @@ -0,0 +1,73 @@ +import userEvent from '@testing-library/user-event'; +import type MockAdapter from 'axios-mock-adapter'; + +import { + initializeMocks, + render, + screen, + waitFor, +} from '@src/testUtils'; +import { ToastProvider } from '@src/generic/toast-context'; +import { + getLibraryContainerChildrenApiUrl, +} from '../data/api'; +import { + mockContentLibrary, + mockGetContainerChildren, +} from '../data/api.mocks'; +import ContainerRemover from './ContainerRemover'; +import { LibraryProvider } from '../common/context/LibraryContext'; + +let axiosMock: MockAdapter; + +mockGetContainerChildren.applyMock(); +mockContentLibrary.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 } = initializeMocks()); + }); + + 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 2261edb59..b60ca6240 100644 --- a/src/library-authoring/containers/ContainerRemover.tsx +++ b/src/library-authoring/containers/ContainerRemover.tsx @@ -7,32 +7,42 @@ 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 { useContainer, useRemoveContainerChildren } from '../data/apiHooks'; -import messages from '../components/messages'; +import { useSidebarContext } from '@src/library-authoring/common/context/SidebarContext'; +import { useLibraryContext } from '@src/library-authoring/common/context/LibraryContext'; +import { + useContainer, useContainerChildren, useRemoveContainerChildren, useUpdateContainerChildren, +} 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, 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 +60,19 @@ 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); + // istanbul ignore if + if (sidebarItemInfo?.id === containerKey && sidebarItemInfo?.index === index) { + closeLibrarySidebar(); + } + } else { + await removeContainerMutation.mutateAsync([containerKey]); + // istanbul ignore if + if (sidebarItemInfo?.id === containerKey) { + closeLibrarySidebar(); + } } showToast(removeSuccess); } catch (e) { @@ -63,12 +83,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/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index 44151f34b..760642bb5 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 f1d29d4c2..211b1614b 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 64e571d94..dca9de9d6 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -736,32 +736,35 @@ 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: api.LibraryBlockMetadata[] | api.Container[], - newData: api.LibraryBlockMetadata[] | api.Container[], - ) => { +export const useContainerChildren = ( + containerId?: string, + published: boolean = false, + ) => ( + useQuery({ + enabled: !!containerId, + queryKey: libraryAuthoringQueryKeys.containerChildren(containerId!), + 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); - // 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/routes.ts b/src/library-authoring/routes.ts index 82c181329..c0b4d35da 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,12 @@ 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; + } + // 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 9a315a4aa..cf1d731b3 100644 --- a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx +++ b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx @@ -39,9 +39,12 @@ 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 +115,7 @@ const ContainerRow = ({ containerKey, container, readOnly }: ContainerRowProps) )} @@ -148,7 +152,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 +168,18 @@ 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 +223,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 7bd5985fb..a96636def 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,18 @@ 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 +138,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 +180,7 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => } + actions={} actionStyle={{ borderRadius: '8px 8px 0px 0px', padding: '0.5rem 1rem', @@ -189,7 +195,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 +300,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 1f9fe10ea..991436d2f 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 this page', 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'); });