From eb855cb3f84f75a2cd6e5332c3c3e355ea688446 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 20 Oct 2025 13:13:02 -0500 Subject: [PATCH 1/5] feat: Deleted/Added level diff in sync modal --- .../CompareContainersWidget.test.tsx | 7 -- .../CompareContainersWidget.tsx | 83 +++++++++++++++---- .../ContainerRow.test.tsx | 14 ---- src/container-comparison/data/apiHooks.ts | 8 +- src/container-comparison/index.scss | 10 +++ src/container-comparison/messages.ts | 10 +++ src/container-comparison/utils.ts | 2 +- src/index.scss | 1 + 8 files changed, 93 insertions(+), 42 deletions(-) create mode 100644 src/container-comparison/index.scss diff --git a/src/container-comparison/CompareContainersWidget.test.tsx b/src/container-comparison/CompareContainersWidget.test.tsx index 348aff0546..3134fc1d17 100644 --- a/src/container-comparison/CompareContainersWidget.test.tsx +++ b/src/container-comparison/CompareContainersWidget.test.tsx @@ -71,13 +71,6 @@ describe('CompareContainersWidget', () => { expect(await screen.findByRole('button', { name: 'subsection block 00' })).toBeInTheDocument(); 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.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' }); expect(backbtns.length).toEqual(2); diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index 96fec4a51a..b388a9e3a8 100644 --- a/src/container-comparison/CompareContainersWidget.tsx +++ b/src/container-comparison/CompareContainersWidget.tsx @@ -4,10 +4,10 @@ import { Alert, Breadcrumb, Button, Card, Icon, Stack, } from '@openedx/paragon'; -import { ArrowBack } from '@openedx/paragon/icons'; +import { ArrowBack, Add, Delete } from '@openedx/paragon/icons'; import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n'; -import { ContainerType } from '@src/generic/key-utils'; +import { ContainerType, getBlockType } 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'; @@ -16,7 +16,9 @@ import { BoldText } from '@src/utils'; import ChildrenPreview from './ChildrenPreview'; import ContainerRow from './ContainerRow'; import { useCourseContainerChildren } from './data/apiHooks'; -import { ContainerChild, ContainerChildBase, WithState } from './types'; +import { + ContainerChild, ContainerChildBase, ContainerState, WithState, +} from './types'; import { diffPreviewContainerChildren, isRowClickable } from './utils'; import messages from './messages'; @@ -30,6 +32,7 @@ interface Props extends ContainerInfoProps { parent: ContainerInfoProps[]; onRowClick: (row: WithState) => void; onBackBtnClick: () => void; + state?: ContainerState; // 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; @@ -43,6 +46,7 @@ const CompareContainersWidgetInner = ({ upstreamBlockId, downstreamBlockId, parent, + state, onRowClick, onBackBtnClick, localUpdateAlertCount, @@ -62,17 +66,31 @@ const CompareContainersWidgetInner = ({ } = useContainer(upstreamBlockId); const result = useMemo(() => { - if (!data || !libData) { + if ((!data || !libData) && !['added', 'removed'].includes(state || '')) { return [undefined, undefined]; } - return diffPreviewContainerChildren(data.children, libData as ContainerChildBase[]); + return diffPreviewContainerChildren(data?.children || [], libData as ContainerChildBase[] || []); }, [data, libData]); const renderBeforeChildren = useCallback(() => { - if (!result[0]) { + if (!result[0] && state !== 'added') { return
; } + if (state === 'added') { + return ( + + + + + ); + } + return result[0]?.map((child) => ( { - if (!result[1]) { + if (!result[1] && state !== 'removed') { return
; } + if (state === 'removed') { + return ( + + + + + ); + } + return result[1]?.map((child) => ( ; } return ( -
+
{localUpdateAlertCount > 0 && ( )}
- - + + {renderBeforeChildren()}
- - + + {renderAfterChildren()} @@ -181,12 +222,14 @@ export const CompareContainersWidget = ({ isReadyToSyncIndividually = false, }: ContainerInfoProps) => { const [currentContainerState, setCurrentContainerState] = useState({ - upstreamBlockId, - downstreamBlockId, - parent: [], - }); + upstreamBlockId, + downstreamBlockId, + parent: [], + state: 'modified', + }); const { data } = useCourseContainerChildren(downstreamBlockId, true); let localUpdateAlertBlockName = ''; @@ -213,9 +256,11 @@ export const CompareContainersWidget = ({ setCurrentContainerState((prev) => ({ upstreamBlockId: row.id!, downstreamBlockId: row.downstreamId!, + state: row.state, parent: [...prev.parent, { upstreamBlockId: prev.upstreamBlockId, downstreamBlockId: prev.downstreamBlockId, + state: prev.state, }], })); }; @@ -230,6 +275,7 @@ export const CompareContainersWidget = ({ return { upstreamBlockId: prevParent!.upstreamBlockId, downstreamBlockId: prevParent!.downstreamBlockId, + state: prevParent!.state, parent: prev.parent.slice(0, -1), }; }); @@ -240,6 +286,7 @@ export const CompareContainersWidget = ({ upstreamBlockId={currentContainerState.upstreamBlockId} downstreamBlockId={currentContainerState.downstreamBlockId} parent={currentContainerState.parent} + state={currentContainerState.state} onRowClick={onRowClick} onBackBtnClick={onBackBtnClick} localUpdateAlertCount={localUpdateAlertCount} diff --git a/src/container-comparison/ContainerRow.test.tsx b/src/container-comparison/ContainerRow.test.tsx index dfd811a6f2..669c9a7c78 100644 --- a/src/container-comparison/ContainerRow.test.tsx +++ b/src/container-comparison/ContainerRow.test.tsx @@ -29,20 +29,6 @@ describe('', () => { )).toBeInTheDocument(); }); - test('is not clickable when state !== modified', async () => { - const onClick = jest.fn(); - render(); - const titleDiv = await screen.findByText('Test title'); - const card = titleDiv.closest('.clickable'); - expect(card).toBe(null); - }); - test('calls onClick when clicked', async () => { const onClick = jest.fn(); const user = userEvent.setup(); diff --git a/src/container-comparison/data/apiHooks.ts b/src/container-comparison/data/apiHooks.ts index 053ee47fab..898b2a695a 100644 --- a/src/container-comparison/data/apiHooks.ts +++ b/src/container-comparison/data/apiHooks.ts @@ -11,7 +11,10 @@ export const containerComparisonQueryKeys = { /** * Key for a single container */ - container: (usageKey: string, getUpstreamInfo: boolean) => { + container: (getUpstreamInfo: boolean, usageKey?: string) => { + if (usageKey === undefined) { + return [undefined, undefined, getUpstreamInfo.toString()]; + } const courseKey = getCourseKey(usageKey); return [...containerComparisonQueryKeys.course(courseKey), usageKey, getUpstreamInfo.toString()]; }, @@ -21,6 +24,7 @@ export const useCourseContainerChildren = (usageKey?: string, getUpstreamInfo?: useQuery({ enabled: !!usageKey, queryFn: () => getCourseContainerChildren(usageKey!, getUpstreamInfo), - queryKey: containerComparisonQueryKeys.container(usageKey!, getUpstreamInfo || false), + // If we first get data with a valid `usageKey` and then the `usageKey` changes to undefinded, an error occurs. + queryKey: containerComparisonQueryKeys.container(getUpstreamInfo || false, usageKey), }) ); diff --git a/src/container-comparison/index.scss b/src/container-comparison/index.scss new file mode 100644 index 0000000000..cc47033f0a --- /dev/null +++ b/src/container-comparison/index.scss @@ -0,0 +1,10 @@ +.compare-changes-widget { + .compare-card { + min-height: 350px; + } + + .big-icon { + height: 68px; + width: 68px; + } +} diff --git a/src/container-comparison/messages.ts b/src/container-comparison/messages.ts index 1a8e61d2cb..549824c90d 100644 --- a/src/container-comparison/messages.ts +++ b/src/container-comparison/messages.ts @@ -86,6 +86,16 @@ const messages = defineMessages({ defaultMessage: 'The only change is to {count, plural, one {text block {blockName} which has been edited} other {{count} text blocks 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.', }, + newContainer: { + id: 'course-authoring.container-comparison.new-container.text', + defaultMessage: 'This {containerType} is new', + description: 'Text to show in the comparison when a container is new.', + }, + deletedContainer: { + id: 'course-authoring.container-comparison.deleted-container.text', + defaultMessage: 'This {containerType} has been removed', + description: 'Text to show in the comparison when a container is removed.', + }, }); export default messages; diff --git a/src/container-comparison/utils.ts b/src/container-comparison/utils.ts index ab0fa14439..2d4ddb4d21 100644 --- a/src/container-comparison/utils.ts +++ b/src/container-comparison/utils.ts @@ -132,7 +132,7 @@ export function diffPreviewContainerChildren div > div.highlight) { From 65789d44627ef29f323f3b2adc075be541244c8e Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 20 Oct 2025 14:55:30 -0500 Subject: [PATCH 2/5] test: Tests added for new added/deleted states on sync modal --- .../CompareContainersWidget.test.tsx | 42 +++++++++++++++++++ src/container-comparison/data/api.mock.ts | 7 ++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/container-comparison/CompareContainersWidget.test.tsx b/src/container-comparison/CompareContainersWidget.test.tsx index 3134fc1d17..caa2077866 100644 --- a/src/container-comparison/CompareContainersWidget.test.tsx +++ b/src/container-comparison/CompareContainersWidget.test.tsx @@ -87,6 +87,48 @@ describe('CompareContainersWidget', () => { expect(await screen.findByRole('button', { name: 'subsection block 0' })).toBeInTheDocument(); }); + test('should show removed container diff state', 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(); + expect((await screen.findAllByText('Test Title')).length).toEqual(2); + // left i.e. before side block + const block = await screen.findByText('subsection block 00'); + await user.click(block); + + const removedRows = await screen.findAllByText('This unit was removed'); + await user.click(removedRows[0]); + + expect(await screen.findByText('This unit has been removed')).toBeInTheDocument(); + }); + + test('should show new added container diff state', 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(); + const blocks = await screen.findAllByText('This subsection will be added in the new version'); + await user.click(blocks[0]); + + screen.logTestingPlaygroundURL(); + expect(await screen.findByText(/this subsection is new/i)).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' }); diff --git a/src/container-comparison/data/api.mock.ts b/src/container-comparison/data/api.mock.ts index e27e23681f..49dccb95fc 100644 --- a/src/container-comparison/data/api.mock.ts +++ b/src/container-comparison/data/api.mock.ts @@ -8,7 +8,7 @@ import * as unitApi from '@src/course-unit/data/api'; * This mock returns a fixed response for the given container ID. */ export async function mockGetCourseContainerChildren(containerId: string): Promise { - const numChildren: number = 3; + let numChildren: number = 3; let blockType: string; let displayName: string; let upstreamReadyToSyncChildrenInfo: UpstreamReadyToSyncChildrenInfo[] = []; @@ -61,8 +61,9 @@ export async function mockGetCourseContainerChildren(containerId: string): Promi case mockGetCourseContainerChildren.subsectionIdLoading: return new Promise(() => { }); default: - blockType = 'unit'; - displayName = 'subsection block 00'; + blockType = 'section'; + displayName = 'section block 00'; + numChildren = 0; break; } const children = Array(numChildren).fill(mockGetCourseContainerChildren.childTemplate).map((child, idx) => ( From 5c073aa32790e8998811776782a2375336557b51 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 20 Oct 2025 16:49:14 -0500 Subject: [PATCH 3/5] fix: Issue with icons in sync modal --- .../CompareContainersWidget.test.tsx | 1 - src/container-comparison/CompareContainersWidget.tsx | 12 +++++++++++- src/course-outline/section-card/SectionCard.tsx | 1 + .../subsection-card/SubsectionCard.tsx | 1 + src/course-outline/unit-card/UnitCard.tsx | 1 + 5 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/container-comparison/CompareContainersWidget.test.tsx b/src/container-comparison/CompareContainersWidget.test.tsx index caa2077866..dcfe6cb8ed 100644 --- a/src/container-comparison/CompareContainersWidget.test.tsx +++ b/src/container-comparison/CompareContainersWidget.test.tsx @@ -125,7 +125,6 @@ describe('CompareContainersWidget', () => { const blocks = await screen.findAllByText('This subsection will be added in the new version'); await user.click(blocks[0]); - screen.logTestingPlaygroundURL(); expect(await screen.findByText(/this subsection is new/i)).toBeInTheDocument(); }); diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index b388a9e3a8..370373bc1c 100644 --- a/src/container-comparison/CompareContainersWidget.tsx +++ b/src/container-comparison/CompareContainersWidget.tsx @@ -69,7 +69,17 @@ const CompareContainersWidgetInner = ({ if ((!data || !libData) && !['added', 'removed'].includes(state || '')) { return [undefined, undefined]; } - return diffPreviewContainerChildren(data?.children || [], libData as ContainerChildBase[] || []); + + let libChildren = libData as ContainerChildBase[] || []; + if (state === 'removed') { + // There is the case in which the item is removed, but it still exists + // in the library, in that case the query will bring its children, + // but we must put it empty so that the status of all the children of + // the downstream are 'removed' + libChildren = []; + } + + return diffPreviewContainerChildren(data?.children || [], libChildren); }, [data, libData]); const renderBeforeChildren = useCallback(() => { diff --git a/src/course-outline/section-card/SectionCard.tsx b/src/course-outline/section-card/SectionCard.tsx index b7af424a60..02c1d5c644 100644 --- a/src/course-outline/section-card/SectionCard.tsx +++ b/src/course-outline/section-card/SectionCard.tsx @@ -146,6 +146,7 @@ const SectionCard = ({ upstreamBlockVersionSynced: upstreamInfo.versionSynced, isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually, isContainer: true, + blockType: 'section', }; }, [upstreamInfo]); diff --git a/src/course-outline/subsection-card/SubsectionCard.tsx b/src/course-outline/subsection-card/SubsectionCard.tsx index 20d8451842..389338519c 100644 --- a/src/course-outline/subsection-card/SubsectionCard.tsx +++ b/src/course-outline/subsection-card/SubsectionCard.tsx @@ -128,6 +128,7 @@ const SubsectionCard = ({ upstreamBlockVersionSynced: upstreamInfo.versionSynced, isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually, isContainer: true, + blockType: 'subsection', }; }, [upstreamInfo]); diff --git a/src/course-outline/unit-card/UnitCard.tsx b/src/course-outline/unit-card/UnitCard.tsx index e59dac82bb..8029f603cc 100644 --- a/src/course-outline/unit-card/UnitCard.tsx +++ b/src/course-outline/unit-card/UnitCard.tsx @@ -105,6 +105,7 @@ const UnitCard = ({ upstreamBlockVersionSynced: upstreamInfo.versionSynced, isReadyToSyncIndividually: upstreamInfo.isReadyToSyncIndividually, isContainer: true, + blockType: 'unit', }; }, [upstreamInfo]); From f82f020c7a60366164d11343c631e1a116058347 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 22 Oct 2025 10:12:28 -0500 Subject: [PATCH 4/5] refactor: Pass undefined on removed state in useContainerChildren --- .../CompareContainersWidget.tsx | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/container-comparison/CompareContainersWidget.tsx b/src/container-comparison/CompareContainersWidget.tsx index 370373bc1c..90f58e5f26 100644 --- a/src/container-comparison/CompareContainersWidget.tsx +++ b/src/container-comparison/CompareContainersWidget.tsx @@ -54,11 +54,13 @@ const CompareContainersWidgetInner = ({ }: Props) => { const intl = useIntl(); const { data, isError, error } = useCourseContainerChildren(downstreamBlockId, parent.length === 0); + // There is the case in which the item is removed, but it still exists + // in the library, for that case, we avoid bringing the children. const { data: libData, isError: isLibError, error: libError, - } = useContainerChildren(upstreamBlockId, true); + } = useContainerChildren(state === 'removed' ? undefined : upstreamBlockId, true); const { data: containerData, isError: isContainerTitleError, @@ -70,16 +72,7 @@ const CompareContainersWidgetInner = ({ return [undefined, undefined]; } - let libChildren = libData as ContainerChildBase[] || []; - if (state === 'removed') { - // There is the case in which the item is removed, but it still exists - // in the library, in that case the query will bring its children, - // but we must put it empty so that the status of all the children of - // the downstream are 'removed' - libChildren = []; - } - - return diffPreviewContainerChildren(data?.children || [], libChildren); + return diffPreviewContainerChildren(data?.children || [], libData as ContainerChildBase[] || []); }, [data, libData]); const renderBeforeChildren = useCallback(() => { From 541001cb8e4b9c3e6ed54452725d8a4c54e0a2fe Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 22 Oct 2025 15:40:00 -0500 Subject: [PATCH 5/5] style: Nits on the code --- src/container-comparison/data/apiHooks.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/container-comparison/data/apiHooks.ts b/src/container-comparison/data/apiHooks.ts index 898b2a695a..dd78e37a99 100644 --- a/src/container-comparison/data/apiHooks.ts +++ b/src/container-comparison/data/apiHooks.ts @@ -24,7 +24,7 @@ export const useCourseContainerChildren = (usageKey?: string, getUpstreamInfo?: useQuery({ enabled: !!usageKey, queryFn: () => getCourseContainerChildren(usageKey!, getUpstreamInfo), - // If we first get data with a valid `usageKey` and then the `usageKey` changes to undefinded, an error occurs. + // If we first get data with a valid `usageKey` and then the `usageKey` changes to undefined, an error occurs. queryKey: containerComparisonQueryKeys.container(getUpstreamInfo || false, usageKey), }) );