Skip to content
35 changes: 35 additions & 0 deletions src/container-comparison/CompareContainersWidget.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ describe('CompareContainersWidget', () => {
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);
expect(screen.queryByText(
/the only change is to text block which has been edited in this course\. accepting will not remove local edits\./i,
)).not.toBeInTheDocument();
});

test('renders loading spinner when data is pending', async () => {
Expand Down Expand Up @@ -90,4 +93,36 @@ describe('CompareContainersWidget', () => {
expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument();
expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument();
});

test('should show alert if the only change is a single text component with local overrides', async () => {
const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId);
axiosMock.onGet(url).reply(200, { publishedDisplayName: 'Test Title' });
render(<CompareContainersWidget
upstreamBlockId={mockGetContainerMetadata.sectionId}
downstreamBlockId={mockGetCourseContainerChildren.sectionShowsAlertSingleText}
/>);

expect((await screen.findAllByText('Test Title')).length).toEqual(2);

expect(screen.getByText(
/the only change is to text block which has been edited in this course\. accepting will not remove local edits\./i,
)).toBeInTheDocument();
expect(screen.getByText(/Html block 11/i)).toBeInTheDocument();
});

test('should show alert if the only changes is multiple text components with local overrides', async () => {
const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId);
axiosMock.onGet(url).reply(200, { publishedDisplayName: 'Test Title' });
render(<CompareContainersWidget
upstreamBlockId={mockGetContainerMetadata.sectionId}
downstreamBlockId={mockGetCourseContainerChildren.sectionShowsAlertMultipleText}
/>);

expect((await screen.findAllByText('Test Title')).length).toEqual(2);

expect(screen.getByText(
/the only change is to which have been edited in this course\. accepting will not remove local edits\./i,
)).toBeInTheDocument();
expect(screen.getByText(/2 text blocks/i)).toBeInTheDocument();
});
});
57 changes: 52 additions & 5 deletions src/container-comparison/CompareContainersWidget.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import { useCallback, useMemo, useState } from 'react';

import {
Alert,
Breadcrumb, Button, Card, Icon, Stack,
} from '@openedx/paragon';
import { ArrowBack } from '@openedx/paragon/icons';
import { useCallback, useMemo, useState } from 'react';
import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';

import { ContainerType } from '@src/generic/key-utils';
import ErrorAlert from '@src/generic/alert-error';
import { LoadingSpinner } from '@src/generic/Loading';
import { useContainer, useContainerChildren } from '@src/library-authoring/data/apiHooks';
import { useIntl } from '@edx/frontend-platform/i18n';
import { BoldText } from '@src/utils';

import ChildrenPreview from './ChildrenPreview';
import ContainerRow from './ContainerRow';
import { useCourseContainerChildren } from './data/apiHooks';
Expand All @@ -18,12 +23,17 @@ import messages from './messages';
interface ContainerInfoProps {
upstreamBlockId: string;
downstreamBlockId: string;
isReadyToSyncIndividually?: boolean;
}

interface Props extends ContainerInfoProps {
parent: ContainerInfoProps[];
onRowClick: (row: WithState<ContainerChild>) => void;
onBackBtnClick: () => void;
// This two props are used to show an alert for the changes to text components with local overrides.
// They may be removed in the future.
localUpdateAlertCount: number;
localUpdateAlertBlockName: string;
}

/**
Expand All @@ -35,9 +45,11 @@ const CompareContainersWidgetInner = ({
parent,
onRowClick,
onBackBtnClick,
localUpdateAlertCount,
localUpdateAlertBlockName,
}: Props) => {
const intl = useIntl();
const { data, isError, error } = useCourseContainerChildren(downstreamBlockId);
const { data, isError, error } = useCourseContainerChildren(downstreamBlockId, parent.length === 0);
const {
data: libData,
isError: isLibError,
Expand Down Expand Up @@ -127,7 +139,19 @@ const CompareContainersWidgetInner = ({
}

return (
<div className="row">
<div className="row justify-content-center">
{localUpdateAlertCount > 0 && (
<Alert variant="info">
<FormattedMessage
{...messages.localChangeInTextAlert}
values={{
blockName: localUpdateAlertBlockName,
count: localUpdateAlertCount,
b: BoldText,
}}
/>
</Alert>
)}
<div className="col col-6 p-1">
<Card className="p-4">
<ChildrenPreview title={getTitleComponent(data?.displayName)} side="Before">
Expand All @@ -151,7 +175,11 @@ 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 = ({ upstreamBlockId, downstreamBlockId }: ContainerInfoProps) => {
export const CompareContainersWidget = ({
upstreamBlockId,
downstreamBlockId,
isReadyToSyncIndividually = false,
}: ContainerInfoProps) => {
const [currentContainerState, setCurrentContainerState] = useState<ContainerInfoProps & {
parent: ContainerInfoProps[];
}>({
Expand All @@ -160,6 +188,23 @@ export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }:
parent: [],
});

const { data } = useCourseContainerChildren(downstreamBlockId, true);
let localUpdateAlertBlockName = '';
let localUpdateAlertCount = 0;

// Show this alert if the only change is text components with local overrides.
// We decided not to put this in `CompareContainersWidgetInner` because if you enter a child,
// the alert would disappear. By keeping this call in CompareContainersWidget,
// the alert remains in the modal regardless of whether you navigate within the children.
if (!isReadyToSyncIndividually && data?.upstreamReadyToSyncChildrenInfo
&& data.upstreamReadyToSyncChildrenInfo.every(value => value.isModified && value.blockType === 'html')
) {
localUpdateAlertCount = data.upstreamReadyToSyncChildrenInfo.length;
if (localUpdateAlertCount === 1) {
localUpdateAlertBlockName = data.upstreamReadyToSyncChildrenInfo[0].name;
}
}

const onRowClick = (row: WithState<ContainerChild>) => {
if (!isRowClickable(row.state, row.blockType as ContainerType)) {
return;
Expand Down Expand Up @@ -197,6 +242,8 @@ export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }:
parent={currentContainerState.parent}
onRowClick={onRowClick}
onBackBtnClick={onBackBtnClick}
localUpdateAlertCount={localUpdateAlertCount}
localUpdateAlertBlockName={localUpdateAlertBlockName}
/>
);
};
5 changes: 5 additions & 0 deletions src/container-comparison/ContainerRow.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ describe('<ContainerRow />', () => {
expect(await screen.findByText(messages.renamedDiffBeforeMessage.defaultMessage.replace('{name}', 'Modified name'))).toBeInTheDocument();
});

test('renders with local content update', async () => {
render(<ContainerRow title="Test title" containerType="html" side="Before" state="locallyContentUpdated" />);
expect(await screen.findByText(messages.locallyContentUpdatedBeforeMessage.defaultMessage.replace('{blockType}', 'html'))).toBeInTheDocument();
});

test('renders with moved state', async () => {
render(<ContainerRow title="Test title" containerType="subsection" side="After" state="moved" />);
expect(await screen.findByText(
Expand Down
5 changes: 4 additions & 1 deletion src/container-comparison/ContainerRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ const ContainerRow = ({
message = side === 'Before' ? messages.removedDiffBeforeMessage : messages.removedDiffAfterMessage;
return ['text-white bg-danger-600', Delete, message];
case 'locallyRenamed':
message = side === 'Before' ? messages.renamedDiffBeforeMessage : messages.renamedDiffAfterMessage;
message = side === 'Before' ? messages.renamedDiffBeforeMessage : messages.renamedUpdatedDiffAfterMessage;
return ['bg-light-300 text-light-300 ', Done, message];
case 'locallyContentUpdated':
message = side === 'Before' ? messages.locallyContentUpdatedBeforeMessage : messages.locallyContentUpdatedAfterMessage;
return ['bg-light-300 text-light-300 ', Done, message];
case 'moved':
message = side === 'Before' ? messages.movedDiffBeforeMessage : messages.movedDiffAfterMessage;
Expand Down
37 changes: 36 additions & 1 deletion src/container-comparison/data/api.mock.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* istanbul ignore file */
import { CourseContainerChildrenData } from '@src/course-unit/data/types';
import { CourseContainerChildrenData, type UpstreamReadyToSyncChildrenInfo } from '@src/course-unit/data/types';
import * as unitApi from '@src/course-unit/data/api';

/**
Expand All @@ -11,6 +11,7 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
const numChildren: number = 3;
let blockType: string;
let displayName: string;
let upstreamReadyToSyncChildrenInfo: UpstreamReadyToSyncChildrenInfo[] = [];
switch (containerId) {
case mockGetCourseContainerChildren.unitId:
blockType = 'text';
Expand All @@ -24,6 +25,37 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
blockType = 'unit';
displayName = 'subsection block 00';
break;
case mockGetCourseContainerChildren.sectionShowsAlertSingleText:
blockType = 'subsection';
displayName = 'Test Title';
upstreamReadyToSyncChildrenInfo = [{
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@1',
name: 'Html block 11',
blockType: 'html',
isModified: true,
upstream: 'upstream-id',
}];
break;
case mockGetCourseContainerChildren.sectionShowsAlertMultipleText:
blockType = 'subsection';
displayName = 'Test Title';
upstreamReadyToSyncChildrenInfo = [
{
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@1',
name: 'Html block 11',
blockType: 'html',
isModified: true,
upstream: 'upstream-id',
},
{
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@2',
name: 'Html block 22',
blockType: 'html',
isModified: true,
upstream: 'upstream-id',
},
];
break;
case mockGetCourseContainerChildren.unitIdLoading:
case mockGetCourseContainerChildren.sectionIdLoading:
case mockGetCourseContainerChildren.subsectionIdLoading:
Expand Down Expand Up @@ -54,11 +86,14 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
isPublished: false,
children,
displayName,
upstreamReadyToSyncChildrenInfo,
});
}
mockGetCourseContainerChildren.unitId = 'block-v1:UNIX+UX1+2025_T3+type@unit+block@0';
mockGetCourseContainerChildren.subsectionId = 'block-v1:UNIX+UX1+2025_T3+type@subsection+block@0';
mockGetCourseContainerChildren.sectionId = 'block-v1:UNIX+UX1+2025_T3+type@section+block@0';
mockGetCourseContainerChildren.sectionShowsAlertSingleText = 'block-v1:UNIX+UX1+2025_T3+type@section2+block@0';
mockGetCourseContainerChildren.sectionShowsAlertMultipleText = 'block-v1:UNIX+UX1+2025_T3+type@section3+block@0';
mockGetCourseContainerChildren.unitIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@unit+block@loading';
mockGetCourseContainerChildren.subsectionIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@subsection+block@loading';
mockGetCourseContainerChildren.sectionIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@section+block@loading';
Expand Down
10 changes: 5 additions & 5 deletions src/container-comparison/data/apiHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@ export const containerComparisonQueryKeys = {
/**
* Key for a single container
*/
container: (usageKey: string) => {
container: (usageKey: string, getUpstreamInfo: boolean) => {
const courseKey = getCourseKey(usageKey);
return [...containerComparisonQueryKeys.course(courseKey), usageKey];
return [...containerComparisonQueryKeys.course(courseKey), usageKey, getUpstreamInfo.toString()];
},
};

export const useCourseContainerChildren = (usageKey?: string) => (
export const useCourseContainerChildren = (usageKey?: string, getUpstreamInfo?: boolean) => (
useQuery({
enabled: !!usageKey,
queryFn: () => getCourseContainerChildren(usageKey!),
queryKey: containerComparisonQueryKeys.container(usageKey!),
queryFn: () => getCourseContainerChildren(usageKey!, getUpstreamInfo),
queryKey: containerComparisonQueryKeys.container(usageKey!, getUpstreamInfo || false),
})
);
29 changes: 22 additions & 7 deletions src/container-comparison/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,24 @@ const messages = defineMessages({
description: 'Description for added component in after section of diff preview',
},
renamedDiffBeforeMessage: {
id: 'course-authoring.container-comparison.diff.before.renamed-message',
defaultMessage: 'Original Library Name: {name}',
description: 'Description for renamed component in before section of diff preview',
id: 'course-authoring.container-comparison.diff.before.locally-updated-message',
defaultMessage: 'Library Name: {name}',
description: 'Description for locally updated component in before section of diff preview',
},
renamedDiffAfterMessage: {
id: 'course-authoring.container-comparison.diff.after.renamed-message',
defaultMessage: 'This {blockType} will remain renamed',
description: 'Description for renamed component in after section of diff preview',
renamedUpdatedDiffAfterMessage: {
id: 'course-authoring.container-comparison.diff.after.locally-updated-message',
defaultMessage: 'Library name remains overwritten',
description: 'Description for locally updated component in after section of diff preview',
},
locallyContentUpdatedBeforeMessage: {
id: 'course-authoring.container-comparison.diff.before.locally-content-updated-message',
defaultMessage: 'This {blockType} was edited locally',
description: 'Description for locally content updated component in before section of diff preview',
},
locallyContentUpdatedAfterMessage: {
id: 'course-authoring.container-comparison.diff.after.locally-content-updated-message',
defaultMessage: 'Local edit will remain',
description: 'Description for locally content updated component in after section of diff preview',
},
movedDiffBeforeMessage: {
id: 'course-authoring.container-comparison.diff.before.moved-message',
Expand All @@ -71,6 +81,11 @@ const messages = defineMessages({
defaultMessage: 'After',
description: 'After section title text',
},
localChangeInTextAlert: {
id: 'course-authoring.container-comparison.text-with-local-change.alert',
defaultMessage: 'The only change is to {count, plural, one {text block <b>{blockName}</b> which has been edited} other {<b>{count} text blocks</b> which have been edited}} in this course. Accepting will not remove local edits.',
description: 'Alert to show if the only change is on text components with local overrides.',
},
});

export default messages;
2 changes: 1 addition & 1 deletion src/container-comparison/types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { UpstreamInfo } from '@src/data/types';

export type ContainerState = 'removed' | 'added' | 'modified' | 'childrenModified' | 'locallyRenamed' | 'moved';
export type ContainerState = 'removed' | 'added' | 'modified' | 'childrenModified' | 'locallyContentUpdated' | 'locallyRenamed' | 'moved';

export type WithState<T> = T & { state?: ContainerState, originalName?: string };
export type WithIndex<T> = T & { index: number };
Expand Down
18 changes: 14 additions & 4 deletions src/container-comparison/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export function diffPreviewContainerChildren<A extends CourseContainerChildBase,
for (let index = 0; index < b.length; index++) {
const newVersion = b[index];
const oldVersion = mapA.get(newVersion.id);

if (!oldVersion) {
// This is a newly added component
addedA.push({
Expand All @@ -55,16 +56,25 @@ export function diffPreviewContainerChildren<A extends CourseContainerChildBase,
let state: ContainerState | undefined;
const displayName = oldVersion.upstreamLink.isModified ? oldVersion.name : newVersion.displayName;
let originalName: string | undefined;
// FIXME: This logic doesn't work when the content is updated locally and the upstream display name is updated.
// `isRenamed` becomes true.
// We probably need to differentiate between `contentModified` and `rename` in the backend or
// send `downstream_customized` field to the frontend and use it here.
const isRenamed = displayName !== newVersion.displayName && displayName === oldVersion.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic doesn't work when the content is updated locally and the upstream display name is updated. isRenamed becomes true.

We probably need to differentiate between contentModified and rename in the backend or send downstream_customized field to the frontend and use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can fix it in a separate ticket; we are out of budget on this ticket. I will add a coment about that

if (index !== oldVersion.index) {
// has moved from its position
state = 'moved';
}
if (displayName !== newVersion.displayName && displayName === oldVersion.name) {
// Has been renamed
if (oldVersion.upstreamLink.isModified && !isRenamed) {
// The content is updated, not the name.
state = 'locallyContentUpdated';
} else if (isRenamed) {
// Has been renamed.
// TODO: At this point we can't know if the content is updated or not
// because `upstreamLink.isModified` is also true when renaming.
state = 'locallyRenamed';
originalName = newVersion.displayName;
}
if (checkIsReadyToSync(oldVersion.upstreamLink)) {
} else if (checkIsReadyToSync(oldVersion.upstreamLink)) {
// has a new version ready to sync
state = 'modified';
}
Expand Down
1 change: 1 addition & 0 deletions src/course-outline/section-card/SectionCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ const SectionCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);
Expand Down
1 change: 1 addition & 0 deletions src/course-outline/subsection-card/SubsectionCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ const SubsectionCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);
Expand Down
1 change: 1 addition & 0 deletions src/course-outline/unit-card/UnitCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ const UnitCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);
Expand Down
Loading