From 75ea7500e146d1df2e4453d19f2c3ce2d32bcc94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Thu, 19 Jun 2025 16:11:13 -0500 Subject: [PATCH] fix: Rename optimistic update in children containers (#2141) Fix: When you update the title of a unit/subsection in the subsection/section page, the text returns to the previous value for a while --- src/library-authoring/data/api.mocks.ts | 18 ++++++++- src/library-authoring/data/apiHooks.ts | 40 +++++++++++++++---- .../LibraryContainerChildren.tsx | 24 ++++++----- .../LibrarySectionSubsectionPage.test.tsx | 30 +++++++++----- .../units/LibraryUnitPage.test.tsx | 16 +++++--- 5 files changed, 95 insertions(+), 33 deletions(-) diff --git a/src/library-authoring/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index fa56547d1..ac98a7365 100644 --- a/src/library-authoring/data/api.mocks.ts +++ b/src/library-authoring/data/api.mocks.ts @@ -283,7 +283,7 @@ mockXBlockFields.dataHtml = { metadata: { displayName: 'Introduction to Testing' }, } satisfies api.XBlockFields; // Mock of another "regular" HTML (Text) block: -mockXBlockFields.usageKey0 = 'lb:org1:Demo_course:html:text-0'; +mockXBlockFields.usageKey0 = 'lb:org1:Demo_course_generated:html:text-0'; mockXBlockFields.dataHtml0 = { displayName: 'text block 0', data: '

This is a text component which uses HTML.

', @@ -493,6 +493,17 @@ export async function mockGetContainerMetadata(containerId: string): Promise ( { ...child, // Generate a unique ID for each child block to avoid "duplicate key" errors in tests - id: `lb:org1:Demo_course:${blockType}:${name}-${idx}`, + id: `${typeNamespace}:org1:Demo_course_generated:${blockType}:${name}-${idx}`, displayName: `${name} block ${idx}`, publishedDisplayName: `${name} block published ${idx}`, } diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index 6d583fa3c..72db994e6 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -611,9 +611,12 @@ export const useContainer = (containerId?: string) => ( ); /** - * Use this mutation to update the fields of a container in a library + * Use this mutation to update the fields of a container in a library. + * + * Use `affectedParentContainerId` to enable the optimistic update when the container + * is updated from a children list of a container */ -export const useUpdateContainer = (containerId: string) => { +export const useUpdateContainer = (containerId: string, affectedParentContainerId?: string) => { const libraryId = getLibraryId(containerId); const queryClient = useQueryClient(); const containerQueryKey = libraryAuthoringQueryKeys.container(containerId); @@ -621,15 +624,36 @@ export const useUpdateContainer = (containerId: string) => { mutationFn: (data: api.UpdateContainerDataRequest) => api.updateContainerMetadata(containerId, data), onMutate: (data) => { const previousData = queryClient.getQueryData(containerQueryKey) as api.Container; - queryClient.setQueryData(containerQueryKey, { - ...previousData, - ...data, - }); - return { previousData }; + if (previousData) { + queryClient.setQueryData(containerQueryKey, { + ...previousData, + ...data, + }); + } + + let childrenPreviousData; + if (affectedParentContainerId) { + const childrenQueryKey = libraryAuthoringQueryKeys.containerChildren(affectedParentContainerId); + childrenPreviousData = queryClient.getQueryData(childrenQueryKey) as api.Container[]; + if (childrenPreviousData) { + queryClient.setQueryData(childrenQueryKey, childrenPreviousData.map(item => ( + item.id === containerId ? { ...item, ...data } : item + ))); + } + } + + return { previousData, childrenPreviousData }; }, onError: (_err, _data, context) => { - queryClient.setQueryData(containerQueryKey, context?.previousData); + if (context?.previousData) { + queryClient.setQueryData(containerQueryKey, context?.previousData); + } + + if (affectedParentContainerId && context?.childrenPreviousData) { + const childrenQueryKey = libraryAuthoringQueryKeys.containerChildren(affectedParentContainerId); + queryClient.setQueryData(childrenQueryKey, context?.childrenPreviousData); + } }, onSettled: () => { // NOTE: We invalidate the library query here because we need to update the library's diff --git a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx index cd29078c1..cf5a62c88 100644 --- a/src/library-authoring/section-subsections/LibraryContainerChildren.tsx +++ b/src/library-authoring/section-subsections/LibraryContainerChildren.tsx @@ -6,6 +6,7 @@ import { ActionRow, Badge, Icon, Stack, } from '@openedx/paragon'; import { Description } from '@openedx/paragon/icons'; +import { InplaceTextEditor } from '@src/generic/inplace-text-editor'; import DraggableList, { SortableItem } from '../../generic/DraggableList'; import Loading from '../../generic/Loading'; import ErrorAlert from '../../generic/alert-error'; @@ -19,7 +20,6 @@ import { import { messages, subsectionMessages, sectionMessages } from './messages'; import containerMessages from '../containers/messages'; import { Container } from '../data/api'; -import { InplaceTextEditor } from '../../generic/inplace-text-editor'; import { ToastContext } from '../../generic/toast-context'; import TagCount from '../../generic/tag-count'; import { ContainerMenu } from '../components/ContainerCard'; @@ -40,10 +40,10 @@ interface ContainerRowProps extends LibraryContainerChildrenProps { container: LibraryContainerMetadataWithUniqueId; } -const ContainerRow = ({ container, readOnly }: ContainerRowProps) => { +const ContainerRow = ({ containerKey, container, readOnly }: ContainerRowProps) => { const intl = useIntl(); const { showToast } = useContext(ToastContext); - const updateMutation = useUpdateContainer(container.originalId); + const updateMutation = useUpdateContainer(container.originalId, containerKey); const { showOnlyPublished } = useLibraryContext(); const handleSaveDisplayName = async (newDisplayName: string) => { @@ -59,12 +59,18 @@ const ContainerRow = ({ container, readOnly }: ContainerRowProps) => { return ( <> - + {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} +
e.stopPropagation()} + > + +
void; mockClipboardEmpty.applyMock(); @@ -67,7 +69,7 @@ jest.mock('../../generic/DraggableList/verticalSortableList', () => ({ describe('', () => { beforeEach(() => { - ({ axiosMock, mockShowToast } = initializeMocks()); + ({ axiosMock, mockShowToast, queryClient } = initializeMocks()); }); afterEach(() => { @@ -104,6 +106,10 @@ describe('', () => { const childType = cType === ContainerType.Section ? ContainerType.Subsection : ContainerType.Unit; + let typeNamespace = 'lct'; + if (cType === ContainerType.Unit) { + typeNamespace = 'lb'; + } it(`shows the spinner before the query is complete in ${cType} page`, async () => { // This mock will never return data about the collection (it loads forever): const cId = cType === ContainerType.Section @@ -253,7 +259,8 @@ describe('', () => { }); it(`should rename child by clicking edit icon besides name in ${cType} page`, async () => { - const url = getLibraryContainerApiUrl(`lb:org1:Demo_course:${childType}:${childType}-0`); + const mockSetQueryData = jest.spyOn(queryClient, 'setQueryData'); + const url = getLibraryContainerApiUrl(`${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-0`); axiosMock.onPatch(url).reply(200); renderLibrarySectionPage(undefined, undefined, cType); @@ -282,10 +289,11 @@ describe('', () => { })); expect(textBox).not.toBeInTheDocument(); expect(mockShowToast).toHaveBeenCalledWith('Container updated successfully.'); + expect(mockSetQueryData).toHaveBeenCalledTimes(1); }); it(`should show error while updating child name in ${cType} page`, async () => { - const url = getLibraryContainerApiUrl(`lb:org1:Demo_course:${childType}:${childType}-0`); + const url = getLibraryContainerApiUrl(`${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-0`); axiosMock.onPatch(url).reply(400); renderLibrarySectionPage(undefined, undefined, cType); @@ -326,7 +334,7 @@ describe('', () => { .onPatch(getLibraryContainerChildrenApiUrl(cId)) .reply(200); verticalSortableListCollisionDetection.mockReturnValue([{ - id: `lb:org1:Demo_course:${childType}:${childType}-1----1`, + id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`, }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); @@ -344,7 +352,9 @@ describe('', () => { axiosMock .onPatch(getLibraryContainerChildrenApiUrl(cId)) .reply(200); - verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]); + verticalSortableListCollisionDetection.mockReturnValue([{ + id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`, + }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); }); @@ -361,7 +371,9 @@ describe('', () => { axiosMock .onPatch(getLibraryContainerChildrenApiUrl(cId)) .reply(500); - verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]); + verticalSortableListCollisionDetection.mockReturnValue([{ + id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`, + }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); }); @@ -372,9 +384,9 @@ describe('', () => { it(`should open ${childType} page on double click`, async () => { renderLibrarySectionPage(undefined, undefined, cType); const child = await screen.findByText(`${childType} block 0`); - // trigger double click - userEvent.click(child.parentElement!, undefined, { clickCount: 2 }); - expect((await screen.findAllByText(new RegExp(`Test ${childType}`, 'i')))[0]).toBeInTheDocument(); + // Trigger double click. Find the child card as the parent element + userEvent.click(child.parentElement!.parentElement!.parentElement!, undefined, { clickCount: 2 }); + expect((await screen.findAllByText(new RegExp(`${childType} block 0`, 'i')))[0]).toBeInTheDocument(); expect(await screen.findByRole('button', { name: new RegExp(`${childType} Info`, 'i') })).toBeInTheDocument(); }); }); diff --git a/src/library-authoring/units/LibraryUnitPage.test.tsx b/src/library-authoring/units/LibraryUnitPage.test.tsx index 01f186e42..1009147fb 100644 --- a/src/library-authoring/units/LibraryUnitPage.test.tsx +++ b/src/library-authoring/units/LibraryUnitPage.test.tsx @@ -217,7 +217,7 @@ describe('', () => { }); it('should rename component while clicking on name', async () => { - const url = getXBlockFieldsApiUrl('lb:org1:Demo_course:html:text-0'); + const url = getXBlockFieldsApiUrl('lb:org1:Demo_course_generated:html:text-0'); axiosMock.onPost(url).reply(200); renderLibraryUnitPage(); @@ -251,7 +251,7 @@ describe('', () => { }); it('should show error while updating component name', async () => { - const url = getXBlockFieldsApiUrl('lb:org1:Demo_course:html:text-0'); + const url = getXBlockFieldsApiUrl('lb:org1:Demo_course_generated:html:text-0'); axiosMock.onPost(url).reply(400); renderLibraryUnitPage(); @@ -290,7 +290,9 @@ describe('', () => { axiosMock .onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId)) .reply(200); - verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]); + verticalSortableListCollisionDetection.mockReturnValue([{ + id: 'lb:org1:Demo_course_generated:html:text-1----1', + }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); }); @@ -304,7 +306,9 @@ describe('', () => { axiosMock .onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId)) .reply(200); - verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]); + verticalSortableListCollisionDetection.mockReturnValue([{ + id: 'lb:org1:Demo_course_generated:html:text-1----1', + }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); }); @@ -318,7 +322,9 @@ describe('', () => { axiosMock .onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId)) .reply(500); - verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]); + verticalSortableListCollisionDetection.mockReturnValue([{ + id: 'lb:org1:Demo_course_generated:html:text-1----1', + }]); await act(async () => { fireEvent.keyDown(firstDragHandle, { code: 'Space' }); });