From 15e20ae585af332ca25f195f7bcd5afa1a5d4796 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 3 Nov 2025 20:29:37 +0530 Subject: [PATCH 1/2] 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. (cherry picked from commit 75ae9d549cf6cee6771c0b143dba12357bbb2a05) --- .../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 cb1b318bf4..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'; @@ -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..d1323adf6f 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..33b67f1e65 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 1f3a1ca6df..b776978b53 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(() => { + 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(() => { - showToast(intl.formatMessage(messages.removeComponentFromContainerFailure)); + // 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 fb1132af7e..229c22b7f5 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 0000000000..7a4945238d --- /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 2261edb593..b60ca6240e 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 44151f34ba..760642bb55 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..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 64e571d94e..dca9de9d67 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 82c1813298..c0b4d35da4 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 9a315a4aa0..cf1d731b3e 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 7bd5985fba..a96636def9 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 1f9fe10eac..991436d2fb 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'); }); From 2d7bd173ad0f204181133058798f9922b4236157 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 3 Nov 2025 20:31:30 +0530 Subject: [PATCH 2/2] fix: publish status of container on adding new children (#2587) Updates publish status of container when adding new child components to a unit or other containers. (cherry picked from commit bd82c1d33dff8073a58f8e8f0fc6d28cf7ad5901) --- .../ConfirmationView.tsx | 1 + .../LegacyLibMigrationPage.test.tsx | 62 +++++++++++++------ src/legacy-libraries-migration/messages.ts | 14 +++-- src/library-authoring/data/apiHooks.test.tsx | 9 +-- src/library-authoring/data/apiHooks.ts | 2 + 5 files changed, 58 insertions(+), 30 deletions(-) diff --git a/src/legacy-libraries-migration/ConfirmationView.tsx b/src/legacy-libraries-migration/ConfirmationView.tsx index 6cb8e025ec..99acf23c33 100644 --- a/src/legacy-libraries-migration/ConfirmationView.tsx +++ b/src/legacy-libraries-migration/ConfirmationView.tsx @@ -81,6 +81,7 @@ export const ConfirmationView = ({ {legacyLibraries.map((legacyLib) => ( diff --git a/src/legacy-libraries-migration/LegacyLibMigrationPage.test.tsx b/src/legacy-libraries-migration/LegacyLibMigrationPage.test.tsx index 79110706a7..dea7ab7ef1 100644 --- a/src/legacy-libraries-migration/LegacyLibMigrationPage.test.tsx +++ b/src/legacy-libraries-migration/LegacyLibMigrationPage.test.tsx @@ -6,6 +6,7 @@ import { render, screen, waitFor, + within, } from '@src/testUtils'; import studioHomeMock from '@src/studio-home/__mocks__/studioHomeMock'; import { mockGetContentLibraryV2List } from '@src/library-authoring/data/api.mocks'; @@ -184,7 +185,7 @@ describe('', () => { nextButton.click(); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); const backButton = screen.getByRole('button', { name: /back/i }); backButton.click(); @@ -210,7 +211,7 @@ describe('', () => { nextButton.click(); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); // The next button is disabled expect(nextButton).toBeDisabled(); @@ -224,24 +225,31 @@ describe('', () => { }); it('should back to select library destination', async () => { + const user = userEvent.setup(); renderPage(); expect(await screen.findByText('Migrate Legacy Libraries')).toBeInTheDocument(); expect(await screen.findByText('MBA')).toBeInTheDocument(); const legacyLibrary = screen.getByRole('checkbox', { name: 'MBA' }); - legacyLibrary.click(); + await user.click(legacyLibrary); - const nextButton = screen.getByRole('button', { name: /next/i }); - nextButton.click(); + const nextButton = await screen.findByRole('button', { name: /next/i }); + await user.click(nextButton); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); const radioButton = screen.getByRole('radio', { name: /test library 1/i }); - radioButton.click(); + await user.click(radioButton); - nextButton.click(); - expect(await screen.findByText(/these 1 legacy library will be migrated to/i)).toBeInTheDocument(); + await user.click(nextButton); + const alert = await screen.findByRole('alert'); + expect(await within(alert).findByText( + /All content from the 1 legacy library you selected will be migrated to/i, + )).toBeInTheDocument(); + expect(await within(alert).findByText( + /test library 1/i, + )).toBeInTheDocument(); const backButton = screen.getByRole('button', { name: /back/i }); backButton.click(); @@ -269,7 +277,7 @@ describe('', () => { nextButton.click(); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); const createButton = await screen.findByRole('button', { name: /create new library/i }); expect(createButton).toBeInTheDocument(); @@ -336,18 +344,24 @@ describe('', () => { legacyLibrary3.click(); const nextButton = screen.getByRole('button', { name: /next/i }); - nextButton.click(); + await user.click(nextButton); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); const radioButton = screen.getByRole('radio', { name: /test library 1/i }); - radioButton.click(); + await user.click(radioButton); - nextButton.click(); + await user.click(nextButton); // Should show alert of ConfirmationView - expect(await screen.findByText(/these 3 legacy libraries will be migrated to/i)).toBeInTheDocument(); + const alert = await screen.findByRole('alert'); + expect(await within(alert).findByText( + /All content from the 3 legacy libraries you selected will be migrated to/i, + )).toBeInTheDocument(); + expect(await within(alert).findByText( + /test library 1/i, + )).toBeInTheDocument(); expect(screen.getByText('MBA')).toBeInTheDocument(); expect(screen.getByText('Legacy library 1')).toBeInTheDocument(); expect(screen.getByText('MBA 1')).toBeInTheDocument(); @@ -390,18 +404,26 @@ describe('', () => { legacyLibrary3.click(); const nextButton = screen.getByRole('button', { name: /next/i }); - nextButton.click(); + await user.click(nextButton); // Should show alert of SelectDestinationView - expect(await screen.findByText(/any legacy libraries that are used/i)).toBeInTheDocument(); + expect(await screen.findByText(/you selected will be migrated to this new library/i)).toBeInTheDocument(); expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); const radioButton = screen.getByRole('radio', { name: /test library 1/i }); - radioButton.click(); + await user.click(radioButton); - nextButton.click(); + await user.click(nextButton); // Should show alert of ConfirmationView - expect(await screen.findByText(/these 3 legacy libraries will be migrated to/i)).toBeInTheDocument(); + const alert = await screen.findByRole('alert'); + expect(await within(alert).findByText( + /All content from the 3 legacy libraries you selected will be migrated to /i, + { exact: false }, + )).toBeInTheDocument(); + expect(await within(alert).findByText( + /test library 1/i, + { exact: false }, + )).toBeInTheDocument(); expect(screen.getByText('MBA')).toBeInTheDocument(); expect(screen.getByText('Legacy library 1')).toBeInTheDocument(); expect(screen.getByText('MBA 1')).toBeInTheDocument(); diff --git a/src/legacy-libraries-migration/messages.ts b/src/legacy-libraries-migration/messages.ts index e7322d9a70..eaf1a2168b 100644 --- a/src/legacy-libraries-migration/messages.ts +++ b/src/legacy-libraries-migration/messages.ts @@ -65,16 +65,18 @@ const messages = defineMessages({ id: 'legacy-libraries-migration.select-destination.alert.text', defaultMessage: 'All content from the' + ' {count, plural, one {{count} legacy library} other {{count} legacy libraries}} you selected will' - + ' be migrated to this new library, organized into collections. Any legacy libraries that are used in' - + ' problem banks will maintain their link with migrated content the first time they are migrated.', + + ' be migrated to this new library, organized into collections. Legacy library content used in courses will' + + ' continue to work as-is. To receive any future changes to migrated content, you must update these' + + ' references within your course.', description: 'Alert text in the select destination step of the legacy libraries migration page.', }, confirmationViewAlert: { id: 'legacy-libraries-migration.select-destination.alert.text', - defaultMessage: 'These {count, plural, one {{count} legacy library} other {{count} legacy libraries}}' - + ' will be migrated to {libraryName} and organized as collections. Legacy library content used' - + ' in courses will continue to work as-is. To receive any future changes to migrated content,' - + ' you must update these references within your course.', + defaultMessage: 'All content from the' + + ' {count, plural, one {{count} legacy library} other {{count} legacy libraries}} you selected will' + + ' be migrated to {libraryName}, organized into collections. Legacy library content used in courses will' + + ' continue to work as-is. To receive any future changes to migrated content, you must update these' + + ' references within your course.', description: 'Alert text in the confirmation step of the legacy libraries migration page.', }, previouslyMigratedAlert: { diff --git a/src/library-authoring/data/apiHooks.test.tsx b/src/library-authoring/data/apiHooks.test.tsx index df5d5166a4..ba6a3ccf4b 100644 --- a/src/library-authoring/data/apiHooks.test.tsx +++ b/src/library-authoring/data/apiHooks.test.tsx @@ -329,10 +329,11 @@ describe('library api hooks', () => { // Keys should be invalidated: // 1. library // 2. containerChildren - // 3. containerHierarchy - // 4 & 5. subsections - // 6 all hierarchies - expect(spy).toHaveBeenCalledTimes(6); + // 3. container + // 4. containerHierarchy + // 5 & 6. subsections + // 7 all hierarchies + expect(spy).toHaveBeenCalledTimes(7); }); describe('publishContainer', () => { diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index dca9de9d67..5646d2f6b7 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -817,6 +817,8 @@ export const useAddItemsToContainer = (containerId?: string) => { // It would be complex to bring the entire hierarchy and only update the items within that hierarchy. queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.containerHierarchy(undefined) }); queryClient.invalidateQueries({ queryKey: xblockQueryKeys.componentHierarchy(undefined) }); + // Invalidate the container to update its publish status + queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.container(containerId) }); const containerType = getBlockType(containerId); if (containerType === 'section') {