Skip to content

Commit 311bef6

Browse files
ChrisChVChrisChV
authored
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 #2438 (comment)
1 parent 195249e commit 311bef6

16 files changed

Lines changed: 196 additions & 28 deletions

File tree

‎src/container-comparison/CompareContainersWidget.test.tsx‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ describe('CompareContainersWidget', () => {
3131
expect((await screen.findAllByText('subsection block 2')).length).toEqual(1);
3232
expect((await screen.findAllByText('subsection block 11')).length).toEqual(1);
3333
expect((await screen.findAllByText('subsection block 22')).length).toEqual(1);
34+
expect(screen.queryByText(
35+
/the only change is to text block which has been edited in this course\. accepting will not remove local edits\./i,
36+
)).not.toBeInTheDocument();
3437
});
3538

3639
test('renders loading spinner when data is pending', async () => {
@@ -90,4 +93,36 @@ describe('CompareContainersWidget', () => {
9093
expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument();
9194
expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument();
9295
});
96+
97+
test('should show alert if the only change is a single text component with local overrides', async () => {
98+
const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId);
99+
axiosMock.onGet(url).reply(200, { publishedDisplayName: 'Test Title' });
100+
render(<CompareContainersWidget
101+
upstreamBlockId={mockGetContainerMetadata.sectionId}
102+
downstreamBlockId={mockGetCourseContainerChildren.sectionShowsAlertSingleText}
103+
/>);
104+
105+
expect((await screen.findAllByText('Test Title')).length).toEqual(2);
106+
107+
expect(screen.getByText(
108+
/the only change is to text block which has been edited in this course\. accepting will not remove local edits\./i,
109+
)).toBeInTheDocument();
110+
expect(screen.getByText(/Html block 11/i)).toBeInTheDocument();
111+
});
112+
113+
test('should show alert if the only changes is multiple text components with local overrides', async () => {
114+
const url = getLibraryContainerApiUrl(mockGetContainerMetadata.sectionId);
115+
axiosMock.onGet(url).reply(200, { publishedDisplayName: 'Test Title' });
116+
render(<CompareContainersWidget
117+
upstreamBlockId={mockGetContainerMetadata.sectionId}
118+
downstreamBlockId={mockGetCourseContainerChildren.sectionShowsAlertMultipleText}
119+
/>);
120+
121+
expect((await screen.findAllByText('Test Title')).length).toEqual(2);
122+
123+
expect(screen.getByText(
124+
/the only change is to which have been edited in this course\. accepting will not remove local edits\./i,
125+
)).toBeInTheDocument();
126+
expect(screen.getByText(/2 text blocks/i)).toBeInTheDocument();
127+
});
93128
});

‎src/container-comparison/CompareContainersWidget.tsx‎

Lines changed: 52 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
1+
import { useCallback, useMemo, useState } from 'react';
2+
13
import {
4+
Alert,
25
Breadcrumb, Button, Card, Icon, Stack,
36
} from '@openedx/paragon';
47
import { ArrowBack } from '@openedx/paragon/icons';
5-
import { useCallback, useMemo, useState } from 'react';
8+
import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n';
9+
610
import { ContainerType } from '@src/generic/key-utils';
711
import ErrorAlert from '@src/generic/alert-error';
812
import { LoadingSpinner } from '@src/generic/Loading';
913
import { useContainer, useContainerChildren } from '@src/library-authoring/data/apiHooks';
10-
import { useIntl } from '@edx/frontend-platform/i18n';
14+
import { BoldText } from '@src/utils';
15+
1116
import ChildrenPreview from './ChildrenPreview';
1217
import ContainerRow from './ContainerRow';
1318
import { useCourseContainerChildren } from './data/apiHooks';
@@ -18,12 +23,17 @@ import messages from './messages';
1823
interface ContainerInfoProps {
1924
upstreamBlockId: string;
2025
downstreamBlockId: string;
26+
isReadyToSyncIndividually?: boolean;
2127
}
2228

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

2939
/**
@@ -35,9 +45,11 @@ const CompareContainersWidgetInner = ({
3545
parent,
3646
onRowClick,
3747
onBackBtnClick,
48+
localUpdateAlertCount,
49+
localUpdateAlertBlockName,
3850
}: Props) => {
3951
const intl = useIntl();
40-
const { data, isError, error } = useCourseContainerChildren(downstreamBlockId);
52+
const { data, isError, error } = useCourseContainerChildren(downstreamBlockId, parent.length === 0);
4153
const {
4254
data: libData,
4355
isError: isLibError,
@@ -127,7 +139,19 @@ const CompareContainersWidgetInner = ({
127139
}
128140

129141
return (
130-
<div className="row">
142+
<div className="row justify-content-center">
143+
{localUpdateAlertCount > 0 && (
144+
<Alert variant="info">
145+
<FormattedMessage
146+
{...messages.localChangeInTextAlert}
147+
values={{
148+
blockName: localUpdateAlertBlockName,
149+
count: localUpdateAlertCount,
150+
b: BoldText,
151+
}}
152+
/>
153+
</Alert>
154+
)}
131155
<div className="col col-6 p-1">
132156
<Card className="p-4">
133157
<ChildrenPreview title={getTitleComponent(data?.displayName)} side="Before">
@@ -151,7 +175,11 @@ const CompareContainersWidgetInner = ({
151175
* and allows the user to select the container to view. This is a wrapper component that maintains current
152176
* source state. Actual implementation of the diff view is done by CompareContainersWidgetInner.
153177
*/
154-
export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }: ContainerInfoProps) => {
178+
export const CompareContainersWidget = ({
179+
upstreamBlockId,
180+
downstreamBlockId,
181+
isReadyToSyncIndividually = false,
182+
}: ContainerInfoProps) => {
155183
const [currentContainerState, setCurrentContainerState] = useState<ContainerInfoProps & {
156184
parent: ContainerInfoProps[];
157185
}>({
@@ -160,6 +188,23 @@ export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }:
160188
parent: [],
161189
});
162190

191+
const { data } = useCourseContainerChildren(downstreamBlockId, true);
192+
let localUpdateAlertBlockName = '';
193+
let localUpdateAlertCount = 0;
194+
195+
// Show this alert if the only change is text components with local overrides.
196+
// We decided not to put this in `CompareContainersWidgetInner` because if you enter a child,
197+
// the alert would disappear. By keeping this call in CompareContainersWidget,
198+
// the alert remains in the modal regardless of whether you navigate within the children.
199+
if (!isReadyToSyncIndividually && data?.upstreamReadyToSyncChildrenInfo
200+
&& data.upstreamReadyToSyncChildrenInfo.every(value => value.isModified && value.blockType === 'html')
201+
) {
202+
localUpdateAlertCount = data.upstreamReadyToSyncChildrenInfo.length;
203+
if (localUpdateAlertCount === 1) {
204+
localUpdateAlertBlockName = data.upstreamReadyToSyncChildrenInfo[0].name;
205+
}
206+
}
207+
163208
const onRowClick = (row: WithState<ContainerChild>) => {
164209
if (!isRowClickable(row.state, row.blockType as ContainerType)) {
165210
return;
@@ -197,6 +242,8 @@ export const CompareContainersWidget = ({ upstreamBlockId, downstreamBlockId }:
197242
parent={currentContainerState.parent}
198243
onRowClick={onRowClick}
199244
onBackBtnClick={onBackBtnClick}
245+
localUpdateAlertCount={localUpdateAlertCount}
246+
localUpdateAlertBlockName={localUpdateAlertBlockName}
200247
/>
201248
);
202249
};

‎src/container-comparison/ContainerRow.test.tsx‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ describe('<ContainerRow />', () => {
8383
expect(await screen.findByText(messages.renamedDiffBeforeMessage.defaultMessage.replace('{name}', 'Modified name'))).toBeInTheDocument();
8484
});
8585

86+
test('renders with local content update', async () => {
87+
render(<ContainerRow title="Test title" containerType="html" side="Before" state="locallyContentUpdated" />);
88+
expect(await screen.findByText(messages.locallyContentUpdatedBeforeMessage.defaultMessage.replace('{blockType}', 'html'))).toBeInTheDocument();
89+
});
90+
8691
test('renders with moved state', async () => {
8792
render(<ContainerRow title="Test title" containerType="subsection" side="After" state="moved" />);
8893
expect(await screen.findByText(

‎src/container-comparison/ContainerRow.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@ const ContainerRow = ({
4040
message = side === 'Before' ? messages.removedDiffBeforeMessage : messages.removedDiffAfterMessage;
4141
return ['text-white bg-danger-600', Delete, message];
4242
case 'locallyRenamed':
43-
message = side === 'Before' ? messages.renamedDiffBeforeMessage : messages.renamedDiffAfterMessage;
43+
message = side === 'Before' ? messages.renamedDiffBeforeMessage : messages.renamedUpdatedDiffAfterMessage;
44+
return ['bg-light-300 text-light-300 ', Done, message];
45+
case 'locallyContentUpdated':
46+
message = side === 'Before' ? messages.locallyContentUpdatedBeforeMessage : messages.locallyContentUpdatedAfterMessage;
4447
return ['bg-light-300 text-light-300 ', Done, message];
4548
case 'moved':
4649
message = side === 'Before' ? messages.movedDiffBeforeMessage : messages.movedDiffAfterMessage;

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

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/* istanbul ignore file */
2-
import { CourseContainerChildrenData } from '@src/course-unit/data/types';
2+
import { CourseContainerChildrenData, type UpstreamReadyToSyncChildrenInfo } from '@src/course-unit/data/types';
33
import * as unitApi from '@src/course-unit/data/api';
44

55
/**
@@ -11,6 +11,7 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
1111
const numChildren: number = 3;
1212
let blockType: string;
1313
let displayName: string;
14+
let upstreamReadyToSyncChildrenInfo: UpstreamReadyToSyncChildrenInfo[] = [];
1415
switch (containerId) {
1516
case mockGetCourseContainerChildren.unitId:
1617
blockType = 'text';
@@ -24,6 +25,37 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
2425
blockType = 'unit';
2526
displayName = 'subsection block 00';
2627
break;
28+
case mockGetCourseContainerChildren.sectionShowsAlertSingleText:
29+
blockType = 'subsection';
30+
displayName = 'Test Title';
31+
upstreamReadyToSyncChildrenInfo = [{
32+
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@1',
33+
name: 'Html block 11',
34+
blockType: 'html',
35+
isModified: true,
36+
upstream: 'upstream-id',
37+
}];
38+
break;
39+
case mockGetCourseContainerChildren.sectionShowsAlertMultipleText:
40+
blockType = 'subsection';
41+
displayName = 'Test Title';
42+
upstreamReadyToSyncChildrenInfo = [
43+
{
44+
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@1',
45+
name: 'Html block 11',
46+
blockType: 'html',
47+
isModified: true,
48+
upstream: 'upstream-id',
49+
},
50+
{
51+
id: 'block-v1:UNIX+UX1+2025_T3+type@html+block@2',
52+
name: 'Html block 22',
53+
blockType: 'html',
54+
isModified: true,
55+
upstream: 'upstream-id',
56+
},
57+
];
58+
break;
2759
case mockGetCourseContainerChildren.unitIdLoading:
2860
case mockGetCourseContainerChildren.sectionIdLoading:
2961
case mockGetCourseContainerChildren.subsectionIdLoading:
@@ -54,11 +86,14 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi
5486
isPublished: false,
5587
children,
5688
displayName,
89+
upstreamReadyToSyncChildrenInfo,
5790
});
5891
}
5992
mockGetCourseContainerChildren.unitId = 'block-v1:UNIX+UX1+2025_T3+type@unit+block@0';
6093
mockGetCourseContainerChildren.subsectionId = 'block-v1:UNIX+UX1+2025_T3+type@subsection+block@0';
6194
mockGetCourseContainerChildren.sectionId = 'block-v1:UNIX+UX1+2025_T3+type@section+block@0';
95+
mockGetCourseContainerChildren.sectionShowsAlertSingleText = 'block-v1:UNIX+UX1+2025_T3+type@section2+block@0';
96+
mockGetCourseContainerChildren.sectionShowsAlertMultipleText = 'block-v1:UNIX+UX1+2025_T3+type@section3+block@0';
6297
mockGetCourseContainerChildren.unitIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@unit+block@loading';
6398
mockGetCourseContainerChildren.subsectionIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@subsection+block@loading';
6499
mockGetCourseContainerChildren.sectionIdLoading = 'block-v1:UNIX+UX1+2025_T3+type@section+block@loading';

‎src/container-comparison/data/apiHooks.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,16 @@ export const containerComparisonQueryKeys = {
1111
/**
1212
* Key for a single container
1313
*/
14-
container: (usageKey: string) => {
14+
container: (usageKey: string, getUpstreamInfo: boolean) => {
1515
const courseKey = getCourseKey(usageKey);
16-
return [...containerComparisonQueryKeys.course(courseKey), usageKey];
16+
return [...containerComparisonQueryKeys.course(courseKey), usageKey, getUpstreamInfo.toString()];
1717
},
1818
};
1919

20-
export const useCourseContainerChildren = (usageKey?: string) => (
20+
export const useCourseContainerChildren = (usageKey?: string, getUpstreamInfo?: boolean) => (
2121
useQuery({
2222
enabled: !!usageKey,
23-
queryFn: () => getCourseContainerChildren(usageKey!),
24-
queryKey: containerComparisonQueryKeys.container(usageKey!),
23+
queryFn: () => getCourseContainerChildren(usageKey!, getUpstreamInfo),
24+
queryKey: containerComparisonQueryKeys.container(usageKey!, getUpstreamInfo || false),
2525
})
2626
);

‎src/container-comparison/messages.ts‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,24 @@ const messages = defineMessages({
3737
description: 'Description for added component in after section of diff preview',
3838
},
3939
renamedDiffBeforeMessage: {
40-
id: 'course-authoring.container-comparison.diff.before.renamed-message',
41-
defaultMessage: 'Original Library Name: {name}',
42-
description: 'Description for renamed component in before section of diff preview',
40+
id: 'course-authoring.container-comparison.diff.before.locally-updated-message',
41+
defaultMessage: 'Library Name: {name}',
42+
description: 'Description for locally updated component in before section of diff preview',
4343
},
44-
renamedDiffAfterMessage: {
45-
id: 'course-authoring.container-comparison.diff.after.renamed-message',
46-
defaultMessage: 'This {blockType} will remain renamed',
47-
description: 'Description for renamed component in after section of diff preview',
44+
renamedUpdatedDiffAfterMessage: {
45+
id: 'course-authoring.container-comparison.diff.after.locally-updated-message',
46+
defaultMessage: 'Library name remains overwritten',
47+
description: 'Description for locally updated component in after section of diff preview',
48+
},
49+
locallyContentUpdatedBeforeMessage: {
50+
id: 'course-authoring.container-comparison.diff.before.locally-content-updated-message',
51+
defaultMessage: 'This {blockType} was edited locally',
52+
description: 'Description for locally content updated component in before section of diff preview',
53+
},
54+
locallyContentUpdatedAfterMessage: {
55+
id: 'course-authoring.container-comparison.diff.after.locally-content-updated-message',
56+
defaultMessage: 'Local edit will remain',
57+
description: 'Description for locally content updated component in after section of diff preview',
4858
},
4959
movedDiffBeforeMessage: {
5060
id: 'course-authoring.container-comparison.diff.before.moved-message',
@@ -71,6 +81,11 @@ const messages = defineMessages({
7181
defaultMessage: 'After',
7282
description: 'After section title text',
7383
},
84+
localChangeInTextAlert: {
85+
id: 'course-authoring.container-comparison.text-with-local-change.alert',
86+
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.',
87+
description: 'Alert to show if the only change is on text components with local overrides.',
88+
},
7489
});
7590

7691
export default messages;

‎src/container-comparison/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { UpstreamInfo } from '@src/data/types';
22

3-
export type ContainerState = 'removed' | 'added' | 'modified' | 'childrenModified' | 'locallyRenamed' | 'moved';
3+
export type ContainerState = 'removed' | 'added' | 'modified' | 'childrenModified' | 'locallyContentUpdated' | 'locallyRenamed' | 'moved';
44

55
export type WithState<T> = T & { state?: ContainerState, originalName?: string };
66
export type WithIndex<T> = T & { index: number };

‎src/container-comparison/utils.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ export function diffPreviewContainerChildren<A extends CourseContainerChildBase,
3636
for (let index = 0; index < b.length; index++) {
3737
const newVersion = b[index];
3838
const oldVersion = mapA.get(newVersion.id);
39+
3940
if (!oldVersion) {
4041
// This is a newly added component
4142
addedA.push({
@@ -55,16 +56,25 @@ export function diffPreviewContainerChildren<A extends CourseContainerChildBase,
5556
let state: ContainerState | undefined;
5657
const displayName = oldVersion.upstreamLink.isModified ? oldVersion.name : newVersion.displayName;
5758
let originalName: string | undefined;
59+
// FIXME: This logic doesn't work when the content is updated locally and the upstream display name is updated.
60+
// `isRenamed` becomes true.
61+
// We probably need to differentiate between `contentModified` and `rename` in the backend or
62+
// send `downstream_customized` field to the frontend and use it here.
63+
const isRenamed = displayName !== newVersion.displayName && displayName === oldVersion.name;
5864
if (index !== oldVersion.index) {
5965
// has moved from its position
6066
state = 'moved';
6167
}
62-
if (displayName !== newVersion.displayName && displayName === oldVersion.name) {
63-
// Has been renamed
68+
if (oldVersion.upstreamLink.isModified && !isRenamed) {
69+
// The content is updated, not the name.
70+
state = 'locallyContentUpdated';
71+
} else if (isRenamed) {
72+
// Has been renamed.
73+
// TODO: At this point we can't know if the content is updated or not
74+
// because `upstreamLink.isModified` is also true when renaming.
6475
state = 'locallyRenamed';
6576
originalName = newVersion.displayName;
66-
}
67-
if (checkIsReadyToSync(oldVersion.upstreamLink)) {
77+
} else if (checkIsReadyToSync(oldVersion.upstreamLink)) {
6878
// has a new version ready to sync
6979
state = 'modified';
7080
}

‎src/course-outline/section-card/SectionCard.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ const SectionCard = ({
144144
downstreamBlockId: id,
145145
upstreamBlockId: upstreamInfo.upstreamRef,
146146
upstreamBlockVersionSynced: upstreamInfo.versionSynced,
147+
isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually,
147148
isContainer: true,
148149
};
149150
}, [upstreamInfo]);

0 commit comments

Comments
 (0)