feat: Shows an alert in container sync if the only change is a local override to a text component [FC-0097] (#2516)

- Implements the alert described in https://github.com/openedx/frontend-app-authoring/issues/2438#issuecomment-3358670967
This commit is contained in:
Chris Chávez
2025-10-15 19:17:56 -05:00
committed by GitHub
parent 195249ef26
commit 311bef67ed
16 changed files with 196 additions and 28 deletions

View File

@@ -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 () => {
@@ -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();
});
});

View File

@@ -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';
@@ -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;
}
/**
@@ -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,
@@ -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">
@@ -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[];
}>({
@@ -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;
@@ -197,6 +242,8 @@ export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }:
parent={currentContainerState.parent}
onRowClick={onRowClick}
onBackBtnClick={onBackBtnClick}
localUpdateAlertCount={localUpdateAlertCount}
localUpdateAlertBlockName={localUpdateAlertBlockName}
/>
);
};

View File

@@ -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(

View File

@@ -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;

View File

@@ -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';
/**
@@ -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';
@@ -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:
@@ -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';

View File

@@ -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),
})
);

View File

@@ -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',
@@ -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;

View File

@@ -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 };

View File

@@ -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({
@@ -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;
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';
}

View File

@@ -144,6 +144,7 @@ const SectionCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);

View File

@@ -126,6 +126,7 @@ const SubsectionCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);

View File

@@ -103,6 +103,7 @@ const UnitCard = ({
downstreamBlockId: id,
upstreamBlockId: upstreamInfo.upstreamRef,
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
isContainer: true,
};
}, [upstreamInfo]);

View File

@@ -9,7 +9,7 @@ const getStudioBaseUrl = () => getConfig().STUDIO_BASE_URL;
export const getXBlockBaseApiUrl = (itemId: string) => `${getStudioBaseUrl()}/xblock/${itemId}`;
export const getCourseSectionVerticalApiUrl = (itemId: string) => `${getStudioBaseUrl()}/api/contentstore/v1/container_handler/${itemId}`;
export const getCourseVerticalChildrenApiUrl = (itemId: string) => `${getStudioBaseUrl()}/api/contentstore/v1/container/${itemId}/children`;
export const getCourseVerticalChildrenApiUrl = (itemId: string, getUpstreamInfo: boolean = false) => `${getStudioBaseUrl()}/api/contentstore/v1/container/${itemId}/children?get_upstream_info=${getUpstreamInfo}`;
export const getCourseOutlineInfoUrl = (courseId: string) => `${getStudioBaseUrl()}/course/${courseId}?format=concise`;
export const postXBlockBaseApiUrl = () => `${getStudioBaseUrl()}/xblock/`;
export const libraryBlockChangesUrl = (blockId: string) => `${getStudioBaseUrl()}/api/contentstore/v2/downstreams/${blockId}/sync`;
@@ -108,9 +108,12 @@ export async function handleCourseUnitVisibilityAndData(
/**
* Get an object containing course vertical children data.
*/
export async function getCourseContainerChildren(itemId: string): Promise<CourseContainerChildrenData> {
export async function getCourseContainerChildren(
itemId: string,
getUpstreamInfo: boolean = false,
): Promise<CourseContainerChildrenData> {
const { data } = await getAuthenticatedHttpClient()
.get(getCourseVerticalChildrenApiUrl(itemId));
.get(getCourseVerticalChildrenApiUrl(itemId, getUpstreamInfo));
const camelCaseData = camelCaseObject(data);
return updateXBlockBlockIdToId(camelCaseData) as CourseContainerChildrenData;

View File

@@ -38,9 +38,18 @@ export interface ContainerChildData {
upstreamLink: UpstreamInfo;
}
export interface UpstreamReadyToSyncChildrenInfo {
id: string;
name: string;
upstream: string;
blockType: string;
isModified: boolean;
}
export interface CourseContainerChildrenData {
canPasteComponent: boolean;
children: ContainerChildData[],
children: ContainerChildData[];
isPublished: boolean;
displayName: string;
upstreamReadyToSyncChildrenInfo: UpstreamReadyToSyncChildrenInfo[];
}

View File

@@ -105,6 +105,7 @@ export interface LibraryChangesMessageData {
isLocallyModified?: boolean,
isContainer: boolean,
blockType?: string | null,
isReadyToSyncIndividually?: boolean,
}
export interface PreviewLibraryXBlockChangesProps {
@@ -143,6 +144,7 @@ export const PreviewLibraryXBlockChanges = ({
<CompareContainersWidget
upstreamBlockId={blockData.upstreamBlockId}
downstreamBlockId={blockData.downstreamBlockId}
isReadyToSyncIndividually={blockData.isReadyToSyncIndividually}
/>
);
}

View File

@@ -64,6 +64,7 @@ export interface UpstreamInfo {
isModified?: boolean,
hasTopLevelParent?: boolean,
readyToSyncChildren?: UpstreamChildrenInfo[],
isReadyToSyncIndividually?: boolean,
}
export interface XBlock {