fix: show before and after title in diff preview (#2509)

Fix display of Before and After display name in section/subsection sync preview modal.

codecov.yml

@@ -11,4 +11,5 @@ coverage:
 ignore:
   - "src/grading-settings/grading-scale/react-ranger.js"
   - "src/generic/DraggableList/verticalSortableList.ts"
+  - "src/container-comparison/data/api.mock.ts"
   - "src/index.js"

src/container-comparison/CompareContainersWidget.test.tsx

@@ -1,64 +1,79 @@
 import userEvent from '@testing-library/user-event';
-import { mockGetContainerChildren, mockGetContainerMetadata } from '../library-authoring/data/api.mocks';
+import MockAdapter from 'axios-mock-adapter/types';
-import { initializeMocks, render, screen } from '../testUtils';
+import { getLibraryContainerApiUrl } from '@src/library-authoring/data/api';
+import { mockGetContainerChildren, mockGetContainerMetadata } from '@src/library-authoring/data/api.mocks';
+import { initializeMocks, render, screen } from '@src/testUtils';
 import { CompareContainersWidget } from './CompareContainersWidget';
 import { mockGetCourseContainerChildren } from './data/api.mock';
 
 mockGetCourseContainerChildren.applyMock();
 mockGetContainerChildren.applyMock();
+let axiosMock: MockAdapter;
 
 describe('CompareContainersWidget', () => {
   beforeEach(() => {
-    initializeMocks();
+    ({ axiosMock } = initializeMocks());
   });
 
   test('renders the component with a title', async () => {
+    const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId);
+    axiosMock.onGet(url).reply(200, { publishedDisplayName: 'Test Title' });
     render(<CompareContainersWidget
-      title="Test Title"
       upstreamBlockId={mockGetContainerMetadata.sectionId}
       downstreamBlockId={mockGetCourseContainerChildren.sectionId}
     />);
     expect((await screen.findAllByText('Test Title')).length).toEqual(2);
-    expect((await screen.findAllByText('subsection block 0')).length).toEqual(2);
+    expect((await screen.findAllByText('subsection block 0')).length).toEqual(1);
+    expect((await screen.findAllByText('subsection block 00')).length).toEqual(1);
     expect((await screen.findAllByText('This subsection will be modified')).length).toEqual(3);
     expect((await screen.findAllByText('This subsection was modified')).length).toEqual(3);
-    expect((await screen.findAllByText('subsection block 1')).length).toEqual(2);
+    expect((await screen.findAllByText('subsection block 1')).length).toEqual(1);
-    expect((await screen.findAllByText('subsection block 2')).length).toEqual(2);
+    expect((await screen.findAllByText('subsection block 2')).length).toEqual(1);
+    expect((await screen.findAllByText('subsection block 11')).length).toEqual(1);
+    expect((await screen.findAllByText('subsection block 22')).length).toEqual(1);
   });
 
   test('renders loading spinner when data is pending', async () => {
+    const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionIdLoading);
+    axiosMock.onGet(url).reply(() => new Promise(() => {}));
     render(<CompareContainersWidget
-      title="Test Title"
       upstreamBlockId={mockGetContainerMetadata.sectionIdLoading}
       downstreamBlockId={mockGetCourseContainerChildren.sectionIdLoading}
     />);
-    expect((await screen.findAllByText('Test Title')).length).toEqual(2);
     const spinner = await screen.findAllByRole('status');
-    expect(spinner.length).toEqual(2);
+    expect(spinner.length).toEqual(4);
     expect(spinner[0].textContent).toEqual('Loading...');
     expect(spinner[1].textContent).toEqual('Loading...');
+    expect(spinner[2].textContent).toEqual('Loading...');
+    expect(spinner[3].textContent).toEqual('Loading...');
   });
 
   test('calls onRowClick when a row is clicked and updates diff view', async () => {
+    // mocks title
+    axiosMock.onGet(getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId)).reply(200, { publishedDisplayName: 'Test Title' });
+    axiosMock.onGet(
+      getLibraryContainerApiUrl('lct:org1:Demo_course_generated:subsection:subsection-0'),
+    ).reply(200, { publishedDisplayName: 'subsection block 0' });
+
     const user = userEvent.setup();
     render(<CompareContainersWidget
-      title="Test Title"
       upstreamBlockId={mockGetContainerMetadata.sectionId}
       downstreamBlockId={mockGetCourseContainerChildren.sectionId}
     />);
     expect((await screen.findAllByText('Test Title')).length).toEqual(2);
-    let blocks = await screen.findAllByText('subsection block 0');
+    // left i.e. before side block
-    expect(blocks.length).toEqual(2);
+    let block = await screen.findByText('subsection block 00');
-    await user.click(blocks[0]);
+    await user.click(block);
-    // Breadcrumbs
+    // Breadcrumbs - shows old and new name
-    const breadcrumbs = await screen.findAllByRole('button', { name: 'subsection block 0' });
+    expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument();
-    expect(breadcrumbs.length).toEqual(2);
+    expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument();
 
     const removedRows = await screen.findAllByText('This unit was removed');
     // clicking on removed or added rows does not updated the page.
     await user.click(removedRows[0]);
     // Still in same page
-    expect((await screen.findAllByRole('button', { name: 'subsection block 0' })).length).toEqual(2);
+    expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument();
+    expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument();
 
     // Back breadcrumb
     const backbtns = await screen.findAllByRole('button', { name: 'Back' });
@@ -67,11 +82,12 @@ describe('CompareContainersWidget', () => {
     // Go back
     await user.click(backbtns[0]);
     expect((await screen.findAllByText('Test Title')).length).toEqual(2);
-    blocks = await screen.findAllByText('subsection block 0');
+    // right i.e. after side block
-    expect(blocks.length).toEqual(2);
+    block = await screen.findByText('subsection block 0');
 
     // After side click also works
-    await user.click(blocks[1]);
+    await user.click(block);
-    expect((await screen.findAllByRole('button', { name: 'subsection block 0' })).length).toEqual(2);
+    expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument();
+    expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument();
   });
 });

src/container-comparison/CompareContainersWidget.tsx

@@ -4,8 +4,9 @@ import {
 import { ArrowBack } from '@openedx/paragon/icons';
 import { useCallback, useMemo, useState } from 'react';
 import { ContainerType } from '@src/generic/key-utils';
+import ErrorAlert from '@src/generic/alert-error';
 import { LoadingSpinner } from '@src/generic/Loading';
-import { useContainerChildren } from '@src/library-authoring/data/apiHooks';
+import { useContainer, useContainerChildren } from '@src/library-authoring/data/apiHooks';
 import { useIntl } from '@edx/frontend-platform/i18n';
 import ChildrenPreview from './ChildrenPreview';
 import ContainerRow from './ContainerRow';
@@ -15,7 +16,6 @@ import { diffPreviewContainerChildren, isRowClickable } from './utils';
 import messages from './messages';
 
 interface ContainerInfoProps {
-  title: string;
   upstreamBlockId: string;
   downstreamBlockId: string;
 }
@@ -30,7 +30,6 @@ interface Props extends ContainerInfoProps {
  * Actual implementation of the displaying diff between children of containers.
  */
 const CompareContainersWidgetInner = ({
-  title,
   upstreamBlockId,
   downstreamBlockId,
   parent,
@@ -38,11 +37,17 @@ const CompareContainersWidgetInner = ({
   onBackBtnClick,
 }: Props) => {
   const intl = useIntl();
-  const { data, isPending } = useCourseContainerChildren(downstreamBlockId);
+  const { data, isError, error } = useCourseContainerChildren(downstreamBlockId);
   const {
     data: libData,
-    isPending: libPending,
+    isError: isLibError,
+    error: libError,
   } = useContainerChildren(upstreamBlockId, true);
+  const {
+    data: containerData,
+    isError: isContainerTitleError,
+    error: containerTitleError,
+  } = useContainer(upstreamBlockId);
 
   const result = useMemo(() => {
     if (!data || !libData) {
@@ -52,7 +57,7 @@ const CompareContainersWidgetInner = ({
   }, [data, libData]);
 
   const renderBeforeChildren = useCallback(() => {
-    if (isPending) {
+    if (!result[0]) {
       return <div className="m-auto"><LoadingSpinner /></div>;
     }
 
@@ -67,10 +72,10 @@ const CompareContainersWidgetInner = ({
         onClick={() => onRowClick(child)}
       />
     ));
-  }, [isPending, result]);
+  }, [result]);
 
   const renderAfterChildren = useCallback(() => {
-    if (libPending || isPending) {
+    if (!result[1]) {
       return <div className="m-auto"><LoadingSpinner /></div>;
     }
 
@@ -84,9 +89,13 @@ const CompareContainersWidgetInner = ({
         onClick={() => onRowClick(child)}
       />
     ));
-  }, [libPending, isPending, result]);
+  }, [result]);
+
+  const getTitleComponent = useCallback((title?: string | null) => {
+    if (!title) {
+      return <div className="m-auto"><LoadingSpinner /></div>;
+    }
 
-  const getTitleComponent = () => {
     if (parent.length === 0) {
       return title;
     }
@@ -95,6 +104,7 @@ const CompareContainersWidgetInner = ({
         ariaLabel={intl.formatMessage(messages.breadcrumbAriaLabel)}
         links={[
           {
+            // This raises failed prop-type error as label expects a string but it works without any issues
             label: <Stack direction="horizontal" gap={1}><Icon size="xs" src={ArrowBack} />Back</Stack>,
             onClick: onBackBtnClick,
             variant: 'link',
@@ -110,20 +120,24 @@ const CompareContainersWidgetInner = ({
         linkAs={Button}
       />
     );
-  };
+  }, [parent]);
+
+  if (isError || isLibError || isContainerTitleError) {
+    return <ErrorAlert error={error || libError || containerTitleError} />;
+  }
 
   return (
     <div className="row">
       <div className="col col-6 p-1">
         <Card className="p-4">
-          <ChildrenPreview title={getTitleComponent()} side="Before">
+          <ChildrenPreview title={getTitleComponent(data?.displayName)} side="Before">
             {renderBeforeChildren()}
           </ChildrenPreview>
         </Card>
       </div>
       <div className="col col-6 p-1">
         <Card className="p-4">
-          <ChildrenPreview title={getTitleComponent()} side="After">
+          <ChildrenPreview title={getTitleComponent(containerData?.publishedDisplayName)} side="After">
             {renderAfterChildren()}
           </ChildrenPreview>
         </Card>
@@ -137,11 +151,10 @@ const CompareContainersWidgetInner = ({
  * and allows the user to select the container to view. This is a wrapper component that maintains current
  * source state. Actual implementation of the diff view is done by CompareContainersWidgetInner.
  */
-export const CompareContainersWidget = ({ title, upstreamBlockId, downstreamBlockId }: ContainerInfoProps) => {
+export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }: ContainerInfoProps) => {
   const [currentContainerState, setCurrentContainerState] = useState<ContainerInfoProps & {
     parent: ContainerInfoProps[];
   }>({
-    title,
     upstreamBlockId,
     downstreamBlockId,
     parent: [],
@@ -153,11 +166,9 @@ export const CompareContainersWidget = ({ title, upstreamBlockId, downstreamBloc
     }
 
     setCurrentContainerState((prev) => ({
-      title: row.name,
       upstreamBlockId: row.id!,
       downstreamBlockId: row.downstreamId!,
       parent: [...prev.parent, {
-        title: prev.title,
         upstreamBlockId: prev.upstreamBlockId,
         downstreamBlockId: prev.downstreamBlockId,
       }],
@@ -172,7 +183,6 @@ export const CompareContainersWidget = ({ title, upstreamBlockId, downstreamBloc
       }
       const prevParent = prev.parent[prev.parent.length - 1];
       return {
-        title: prevParent!.title,
         upstreamBlockId: prevParent!.upstreamBlockId,
         downstreamBlockId: prevParent!.downstreamBlockId,
         parent: prev.parent.slice(0, -1),
@@ -182,7 +192,6 @@ export const CompareContainersWidget = ({ title, upstreamBlockId, downstreamBloc
 
   return (
     <CompareContainersWidgetInner
-      title={currentContainerState.title}
       upstreamBlockId={currentContainerState.upstreamBlockId}
       downstreamBlockId={currentContainerState.downstreamBlockId}
       parent={currentContainerState.parent}

src/container-comparison/data/api.mock.ts

@@ -1,3 +1,4 @@
+/* istanbul ignore file */
 import { CourseContainerChildrenData } from '@src/course-unit/data/types';
 import * as unitApi from '@src/course-unit/data/api';
 
@@ -9,15 +10,19 @@ import * as unitApi from '@src/course-unit/data/api';
 export async function mockGetCourseContainerChildren(containerId: string): Promise<CourseContainerChildrenData> {
   const numChildren: number = 3;
   let blockType: string;
+  let displayName: string;
   switch (containerId) {
     case mockGetCourseContainerChildren.unitId:
       blockType = 'text';
+      displayName = 'unit block 00';
       break;
     case mockGetCourseContainerChildren.sectionId:
       blockType = 'subsection';
+      displayName = 'Test Title';
       break;
     case mockGetCourseContainerChildren.subsectionId:
       blockType = 'unit';
+      displayName = 'subsection block 00';
       break;
     case mockGetCourseContainerChildren.unitIdLoading:
     case mockGetCourseContainerChildren.sectionIdLoading:
@@ -25,6 +30,7 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
       return new Promise(() => { });
     default:
       blockType = 'unit';
+      displayName = 'subsection block 00';
       break;
   }
   const children = Array(numChildren).fill(mockGetCourseContainerChildren.childTemplate).map((child, idx) => (
@@ -32,7 +38,7 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
       ...child,
       // Generate a unique ID for each child block to avoid "duplicate key" errors in tests
       id: `block-v1:UNIX+UX1+2025_T3+type@${blockType}+block@${idx}`,
-      name: `${blockType} block ${idx}`,
+      name: `${blockType} block ${idx}${idx}`,
       blockType,
       upstreamLink: {
         upstreamRef: `lct:org1:Demo_course_generated:${blockType}:${blockType}-${idx}`,
@@ -47,6 +53,7 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
     canPasteComponent: true,
     isPublished: false,
     children,
+    displayName,
   });
 }
 mockGetCourseContainerChildren.unitId = 'block-v1:UNIX+UX1+2025_T3+type@unit+block@0';

src/container-comparison/messages.ts

@@ -1,6 +1,11 @@
 import { defineMessages } from '@edx/frontend-platform/i18n';
 
 const messages = defineMessages({
+  error: {
+    id: 'course-authoring.container-comparison.diff.error.message',
+    defaultMessage: 'Unexpected error: Failed to fetch container data',
+    description: 'Generic error message',
+  },
   removedDiffBeforeMessage: {
     id: 'course-authoring.container-comparison.diff.before.removed-message',
     defaultMessage: 'This {blockType} will be removed in the new version',

src/course-unit/data/types.ts

@@ -42,4 +42,5 @@ export interface CourseContainerChildrenData {
   canPasteComponent: boolean;
   children: ContainerChildData[],
   isPublished: boolean;
+  displayName: string;
 }

src/course-unit/preview-changes/index.tsx

@@ -141,7 +141,6 @@ export const PreviewLibraryXBlockChanges = ({
     if (blockData.isContainer) {
       return (
         <CompareContainersWidget
-          title={blockData.displayName}
           upstreamBlockId={blockData.upstreamBlockId}
           downstreamBlockId={blockData.downstreamBlockId}
         />