Skip to content

Commit 75ea750

Browse files
ChrisChVChrisChV
authored
fix: Rename optimistic update in children containers (#2141)
Fix: When you update the title of a unit/subsection in the subsection/section page, the text returns to the previous value for a while
1 parent 08c3d12 commit 75ea750

File tree

5 files changed

+95
-33
lines changed

5 files changed

+95
-33
lines changed

‎src/library-authoring/data/api.mocks.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,7 @@ mockXBlockFields.dataHtml = {
283283
metadata: { displayName: 'Introduction to Testing' },
284284
} satisfies api.XBlockFields;
285285
// Mock of another "regular" HTML (Text) block:
286-
mockXBlockFields.usageKey0 = 'lb:org1:Demo_course:html:text-0';
286+
mockXBlockFields.usageKey0 = 'lb:org1:Demo_course_generated:html:text-0';
287287
mockXBlockFields.dataHtml0 = {
288288
displayName: 'text block 0',
289289
data: '<p>This is a text component which uses <strong>HTML</strong>.</p>',
@@ -493,6 +493,17 @@ export async function mockGetContainerMetadata(containerId: string): Promise<api
493493
case mockGetContainerMetadata.subsectionIdEmpty:
494494
return Promise.resolve(mockGetContainerMetadata.subsectionData);
495495
default:
496+
if (containerId.startsWith('lct:org1:Demo_course_generated:')) {
497+
const lastPart = containerId.split(':').pop();
498+
if (lastPart) {
499+
const [name, idx] = lastPart.split('-');
500+
return Promise.resolve({
501+
...mockGetContainerMetadata.containerData,
502+
displayName: `${name} block ${idx}`,
503+
publishedDisplayName: `${name} block published ${idx}`,
504+
});
505+
}
506+
}
496507
return Promise.resolve(mockGetContainerMetadata.containerData);
497508
}
498509
}
@@ -575,19 +586,22 @@ export async function mockGetContainerChildren(containerId: string): Promise<api
575586
}
576587
let blockType = 'html';
577588
let name = 'text';
589+
let typeNamespace = 'lb';
578590
if (containerId.includes('subsection')) {
579591
blockType = 'unit';
580592
name = blockType;
593+
typeNamespace = 'lct';
581594
} else if (containerId.includes('section')) {
582595
blockType = 'subsection';
583596
name = blockType;
597+
typeNamespace = 'lct';
584598
}
585599
return Promise.resolve(
586600
Array(numChildren).fill(mockGetContainerChildren.childTemplate).map((child, idx) => (
587601
{
588602
...child,
589603
// Generate a unique ID for each child block to avoid "duplicate key" errors in tests
590-
id: `lb:org1:Demo_course:${blockType}:${name}-${idx}`,
604+
id: `${typeNamespace}:org1:Demo_course_generated:${blockType}:${name}-${idx}`,
591605
displayName: `${name} block ${idx}`,
592606
publishedDisplayName: `${name} block published ${idx}`,
593607
}

‎src/library-authoring/data/apiHooks.ts

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -611,25 +611,49 @@ export const useContainer = (containerId?: string) => (
611611
);
612612

613613
/**
614-
* Use this mutation to update the fields of a container in a library
614+
* Use this mutation to update the fields of a container in a library.
615+
*
616+
* Use `affectedParentContainerId` to enable the optimistic update when the container
617+
* is updated from a children list of a container
615618
*/
616-
export const useUpdateContainer = (containerId: string) => {
619+
export const useUpdateContainer = (containerId: string, affectedParentContainerId?: string) => {
617620
const libraryId = getLibraryId(containerId);
618621
const queryClient = useQueryClient();
619622
const containerQueryKey = libraryAuthoringQueryKeys.container(containerId);
620623
return useMutation({
621624
mutationFn: (data: api.UpdateContainerDataRequest) => api.updateContainerMetadata(containerId, data),
622625
onMutate: (data) => {
623626
const previousData = queryClient.getQueryData(containerQueryKey) as api.Container;
624-
queryClient.setQueryData(containerQueryKey, {
625-
...previousData,
626-
...data,
627-
});
628627

629-
return { previousData };
628+
if (previousData) {
629+
queryClient.setQueryData(containerQueryKey, {
630+
...previousData,
631+
...data,
632+
});
633+
}
634+
635+
let childrenPreviousData;
636+
if (affectedParentContainerId) {
637+
const childrenQueryKey = libraryAuthoringQueryKeys.containerChildren(affectedParentContainerId);
638+
childrenPreviousData = queryClient.getQueryData(childrenQueryKey) as api.Container[];
639+
if (childrenPreviousData) {
640+
queryClient.setQueryData(childrenQueryKey, childrenPreviousData.map(item => (
641+
item.id === containerId ? { ...item, ...data } : item
642+
)));
643+
}
644+
}
645+
646+
return { previousData, childrenPreviousData };
630647
},
631648
onError: (_err, _data, context) => {
632-
queryClient.setQueryData(containerQueryKey, context?.previousData);
649+
if (context?.previousData) {
650+
queryClient.setQueryData(containerQueryKey, context?.previousData);
651+
}
652+
653+
if (affectedParentContainerId && context?.childrenPreviousData) {
654+
const childrenQueryKey = libraryAuthoringQueryKeys.containerChildren(affectedParentContainerId);
655+
queryClient.setQueryData(childrenQueryKey, context?.childrenPreviousData);
656+
}
633657
},
634658
onSettled: () => {
635659
// NOTE: We invalidate the library query here because we need to update the library's

‎src/library-authoring/section-subsections/LibraryContainerChildren.tsx

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
ActionRow, Badge, Icon, Stack,
77
} from '@openedx/paragon';
88
import { Description } from '@openedx/paragon/icons';
9+
import { InplaceTextEditor } from '@src/generic/inplace-text-editor';
910
import DraggableList, { SortableItem } from '../../generic/DraggableList';
1011
import Loading from '../../generic/Loading';
1112
import ErrorAlert from '../../generic/alert-error';
@@ -19,7 +20,6 @@ import {
1920
import { messages, subsectionMessages, sectionMessages } from './messages';
2021
import containerMessages from '../containers/messages';
2122
import { Container } from '../data/api';
22-
import { InplaceTextEditor } from '../../generic/inplace-text-editor';
2323
import { ToastContext } from '../../generic/toast-context';
2424
import TagCount from '../../generic/tag-count';
2525
import { ContainerMenu } from '../components/ContainerCard';
@@ -40,10 +40,10 @@ interface ContainerRowProps extends LibraryContainerChildrenProps {
4040
container: LibraryContainerMetadataWithUniqueId;
4141
}
4242

43-
const ContainerRow = ({ container, readOnly }: ContainerRowProps) => {
43+
const ContainerRow = ({ containerKey, container, readOnly }: ContainerRowProps) => {
4444
const intl = useIntl();
4545
const { showToast } = useContext(ToastContext);
46-
const updateMutation = useUpdateContainer(container.originalId);
46+
const updateMutation = useUpdateContainer(container.originalId, containerKey);
4747
const { showOnlyPublished } = useLibraryContext();
4848

4949
const handleSaveDisplayName = async (newDisplayName: string) => {
@@ -59,12 +59,18 @@ const ContainerRow = ({ container, readOnly }: ContainerRowProps) => {
5959

6060
return (
6161
<>
62-
<InplaceTextEditor
63-
onSave={handleSaveDisplayName}
64-
text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
65-
textClassName="font-weight-bold small"
66-
readOnly={readOnly || showOnlyPublished}
67-
/>
62+
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
63+
<div
64+
// Prevent parent card from being clicked.
65+
onClick={(e) => e.stopPropagation()}
66+
>
67+
<InplaceTextEditor
68+
onSave={handleSaveDisplayName}
69+
text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
70+
textClassName="font-weight-bold small"
71+
readOnly={readOnly || showOnlyPublished}
72+
/>
73+
</div>
6874
<ActionRow.Spacer />
6975
<Stack
7076
direction="horizontal"

‎src/library-authoring/section-subsections/LibrarySectionSubsectionPage.test.tsx

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import userEvent from '@testing-library/user-event';
22
import type MockAdapter from 'axios-mock-adapter';
3+
import { QueryClient } from '@tanstack/react-query';
34

45
import { act } from 'react';
56
import {
@@ -31,6 +32,7 @@ const path = '/library/:libraryId/*';
3132
const libraryTitle = mockContentLibrary.libraryData.title;
3233

3334
let axiosMock: MockAdapter;
35+
let queryClient: QueryClient;
3436
let mockShowToast: (message: string, action?: ToastActionData | undefined) => void;
3537

3638
mockClipboardEmpty.applyMock();
@@ -67,7 +69,7 @@ jest.mock('../../generic/DraggableList/verticalSortableList', () => ({
6769

6870
describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
6971
beforeEach(() => {
70-
({ axiosMock, mockShowToast } = initializeMocks());
72+
({ axiosMock, mockShowToast, queryClient } = initializeMocks());
7173
});
7274

7375
afterEach(() => {
@@ -104,6 +106,10 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
104106
const childType = cType === ContainerType.Section
105107
? ContainerType.Subsection
106108
: ContainerType.Unit;
109+
let typeNamespace = 'lct';
110+
if (cType === ContainerType.Unit) {
111+
typeNamespace = 'lb';
112+
}
107113
it(`shows the spinner before the query is complete in ${cType} page`, async () => {
108114
// This mock will never return data about the collection (it loads forever):
109115
const cId = cType === ContainerType.Section
@@ -253,7 +259,8 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
253259
});
254260

255261
it(`should rename child by clicking edit icon besides name in ${cType} page`, async () => {
256-
const url = getLibraryContainerApiUrl(`lb:org1:Demo_course:${childType}:${childType}-0`);
262+
const mockSetQueryData = jest.spyOn(queryClient, 'setQueryData');
263+
const url = getLibraryContainerApiUrl(`${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-0`);
257264
axiosMock.onPatch(url).reply(200);
258265
renderLibrarySectionPage(undefined, undefined, cType);
259266

@@ -282,10 +289,11 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
282289
}));
283290
expect(textBox).not.toBeInTheDocument();
284291
expect(mockShowToast).toHaveBeenCalledWith('Container updated successfully.');
292+
expect(mockSetQueryData).toHaveBeenCalledTimes(1);
285293
});
286294

287295
it(`should show error while updating child name in ${cType} page`, async () => {
288-
const url = getLibraryContainerApiUrl(`lb:org1:Demo_course:${childType}:${childType}-0`);
296+
const url = getLibraryContainerApiUrl(`${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-0`);
289297
axiosMock.onPatch(url).reply(400);
290298
renderLibrarySectionPage(undefined, undefined, cType);
291299

@@ -326,7 +334,7 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
326334
.onPatch(getLibraryContainerChildrenApiUrl(cId))
327335
.reply(200);
328336
verticalSortableListCollisionDetection.mockReturnValue([{
329-
id: `lb:org1:Demo_course:${childType}:${childType}-1----1`,
337+
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
330338
}]);
331339
await act(async () => {
332340
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
@@ -344,7 +352,9 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
344352
axiosMock
345353
.onPatch(getLibraryContainerChildrenApiUrl(cId))
346354
.reply(200);
347-
verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]);
355+
verticalSortableListCollisionDetection.mockReturnValue([{
356+
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
357+
}]);
348358
await act(async () => {
349359
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
350360
});
@@ -361,7 +371,9 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
361371
axiosMock
362372
.onPatch(getLibraryContainerChildrenApiUrl(cId))
363373
.reply(500);
364-
verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]);
374+
verticalSortableListCollisionDetection.mockReturnValue([{
375+
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
376+
}]);
365377
await act(async () => {
366378
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
367379
});
@@ -372,9 +384,9 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
372384
it(`should open ${childType} page on double click`, async () => {
373385
renderLibrarySectionPage(undefined, undefined, cType);
374386
const child = await screen.findByText(`${childType} block 0`);
375-
// trigger double click
376-
userEvent.click(child.parentElement!, undefined, { clickCount: 2 });
377-
expect((await screen.findAllByText(new RegExp(`Test ${childType}`, 'i')))[0]).toBeInTheDocument();
387+
// Trigger double click. Find the child card as the parent element
388+
userEvent.click(child.parentElement!.parentElement!.parentElement!, undefined, { clickCount: 2 });
389+
expect((await screen.findAllByText(new RegExp(`${childType} block 0`, 'i')))[0]).toBeInTheDocument();
378390
expect(await screen.findByRole('button', { name: new RegExp(`${childType} Info`, 'i') })).toBeInTheDocument();
379391
});
380392
});

‎src/library-authoring/units/LibraryUnitPage.test.tsx

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ describe('<LibraryUnitPage />', () => {
217217
});
218218

219219
it('should rename component while clicking on name', async () => {
220-
const url = getXBlockFieldsApiUrl('lb:org1:Demo_course:html:text-0');
220+
const url = getXBlockFieldsApiUrl('lb:org1:Demo_course_generated:html:text-0');
221221
axiosMock.onPost(url).reply(200);
222222
renderLibraryUnitPage();
223223

@@ -251,7 +251,7 @@ describe('<LibraryUnitPage />', () => {
251251
});
252252

253253
it('should show error while updating component name', async () => {
254-
const url = getXBlockFieldsApiUrl('lb:org1:Demo_course:html:text-0');
254+
const url = getXBlockFieldsApiUrl('lb:org1:Demo_course_generated:html:text-0');
255255
axiosMock.onPost(url).reply(400);
256256
renderLibraryUnitPage();
257257

@@ -290,7 +290,9 @@ describe('<LibraryUnitPage />', () => {
290290
axiosMock
291291
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
292292
.reply(200);
293-
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
293+
verticalSortableListCollisionDetection.mockReturnValue([{
294+
id: 'lb:org1:Demo_course_generated:html:text-1----1',
295+
}]);
294296
await act(async () => {
295297
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
296298
});
@@ -304,7 +306,9 @@ describe('<LibraryUnitPage />', () => {
304306
axiosMock
305307
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
306308
.reply(200);
307-
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
309+
verticalSortableListCollisionDetection.mockReturnValue([{
310+
id: 'lb:org1:Demo_course_generated:html:text-1----1',
311+
}]);
308312
await act(async () => {
309313
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
310314
});
@@ -318,7 +322,9 @@ describe('<LibraryUnitPage />', () => {
318322
axiosMock
319323
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
320324
.reply(500);
321-
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
325+
verticalSortableListCollisionDetection.mockReturnValue([{
326+
id: 'lb:org1:Demo_course_generated:html:text-1----1',
327+
}]);
322328
await act(async () => {
323329
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
324330
});

0 commit comments

Comments
 (0)