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
2025-06-19 16:11:13 -05:00
parent 08c3d123d8
commit 75ea7500e1
5 changed files with 95 additions and 33 deletions
--- 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: '<p>This is a text component which uses <strong>HTML</strong>.</p>',
@@ -493,6 +493,17 @@ export async function mockGetContainerMetadata(containerId: string): Promise<api
    case mockGetContainerMetadata.subsectionIdEmpty:
      return Promise.resolve(mockGetContainerMetadata.subsectionData);
    default:
+      if (containerId.startsWith('lct:org1:Demo_course_generated:')) {
+        const lastPart = containerId.split(':').pop();
+        if (lastPart) {
+          const [name, idx] = lastPart.split('-');
+          return Promise.resolve({
+            ...mockGetContainerMetadata.containerData,
+            displayName: `${name} block ${idx}`,
+            publishedDisplayName: `${name} block published ${idx}`,
+          });
+        }
+      }
      return Promise.resolve(mockGetContainerMetadata.containerData);
  }
 }
@@ -575,19 +586,22 @@ export async function mockGetContainerChildren(containerId: string): Promise<api
  }
  let blockType = 'html';
  let name = 'text';
+  let typeNamespace = 'lb';
  if (containerId.includes('subsection')) {
    blockType = 'unit';
    name = blockType;
+    typeNamespace = 'lct';
  } else if (containerId.includes('section')) {
    blockType = 'subsection';
    name = blockType;
+    typeNamespace = 'lct';
  }
  return Promise.resolve(
    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: `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}`,
      }
--- 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
--- 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 (
    <>
-      <InplaceTextEditor
-        onSave={handleSaveDisplayName}
-        text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
-        textClassName="font-weight-bold small"
-        readOnly={readOnly || showOnlyPublished}
-      />
+      {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
+      <div
+        // Prevent parent card from being clicked.
+        onClick={(e) => e.stopPropagation()}
+      >
+        <InplaceTextEditor
+          onSave={handleSaveDisplayName}
+          text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
+          textClassName="font-weight-bold small"
+          readOnly={readOnly || showOnlyPublished}
+        />
+      </div>
      <ActionRow.Spacer />
      <Stack
        direction="horizontal"
--- a/src/library-authoring/section-subsections/LibrarySectionSubsectionPage.test.tsx
+++ b/src/library-authoring/section-subsections/LibrarySectionSubsectionPage.test.tsx
@@ -1,5 +1,6 @@
 import userEvent from '@testing-library/user-event';
 import type MockAdapter from 'axios-mock-adapter';
+import { QueryClient } from '@tanstack/react-query';

 import { act } from 'react';
 import {
@@ -31,6 +32,7 @@ const path = '/library/:libraryId/*';
 const libraryTitle = mockContentLibrary.libraryData.title;

 let axiosMock: MockAdapter;
+let queryClient: QueryClient;
 let mockShowToast: (message: string, action?: ToastActionData | undefined) => void;

 mockClipboardEmpty.applyMock();
@@ -67,7 +69,7 @@ jest.mock('../../generic/DraggableList/verticalSortableList', () => ({

 describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
  beforeEach(() => {
-    ({ axiosMock, mockShowToast } = initializeMocks());
+    ({ axiosMock, mockShowToast, queryClient } = initializeMocks());
  });

  afterEach(() => {
@@ -104,6 +106,10 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
    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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
    });

    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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
      }));
      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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
        .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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
      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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
      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('<LibrarySectionPage / LibrarySubsectionPage />', () => {
    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();
    });
  });
--- a/src/library-authoring/units/LibraryUnitPage.test.tsx
+++ b/src/library-authoring/units/LibraryUnitPage.test.tsx
@@ -217,7 +217,7 @@ describe('<LibraryUnitPage />', () => {
  });

  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('<LibraryUnitPage />', () => {
  });

  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('<LibraryUnitPage />', () => {
    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('<LibraryUnitPage />', () => {
    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('<LibraryUnitPage />', () => {
    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' });
    });