Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/library-authoring/containers/ContainerEditableTitle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,26 @@ import messages from './messages';

interface EditableTitleProps {
containerId: string;
readOnly?: boolean;
textClassName?: string;
// In some cases, the title is already available, but it's retrieved in a list of containers.
// In these cases, it's necessary to use this `ContainerEditableTitle` for the optimistic update to work.
// By using `placeHolderText`, we can give the illusion that the data
// has already been loaded before using the real data.
placeHolderText?: string;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really understand why we need this.
When I removed it in 8576925 and tested, it's still working fine (the optimistic update works without it).

it's retrieved in a list of containers.

Where is that happening? Before you added this placeHolderText, the only place I see this component getting the title from is useContainer(containerId) which is not a list of containers. Maybe we should be getting it from a list though, as it may be a little inefficient loading each container separately if there's a lot in the parent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Where is that happening?

In LibraryContainerChildren, you get the containers in a list using useContainerChildren

The optimistic update is implemented by overwriting the data of the useContainer query. I feel like doing the same for the useContainerChildren query is going to be more complicated.

as it may be a little inefficient loading each container separately if there's a lot in the parent

This is seen visually if we do not use the placeHolderText:

https://www.loom.com/share/0090987109b44902809f55b15271090d?sid=3666ee1c-02e1-4655-8713-d3aca877454e

If you'd like, I can try implementing an optimistic update overriding useContainerChildren. This problem also exists in the component listing on the unit page, it is a separate implementation, but follows the same logic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you'd like, I can try implementing an optimistic update overriding useContainerChildren. This problem also exists in the component listing on the unit page, it is a separate implementation, but follows the same logic.

I think that would be better if you don't mind giving it a try!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works! It's ready for a review

}

export const ContainerEditableTitle = ({ containerId, textClassName }: EditableTitleProps) => {
export const ContainerEditableTitle = ({
containerId,
readOnly,
textClassName,
placeHolderText,
}: EditableTitleProps) => {
const intl = useIntl();

const { readOnly, showOnlyPublished } = useLibraryContext();
const { readOnly: libReadOnly, showOnlyPublished } = useLibraryContext();

const { data: container } = useContainer(containerId);
const { data: container, isLoading } = useContainer(containerId);

const updateMutation = useUpdateContainer(containerId);
const { showToast } = useContext(ToastContext);
Expand All @@ -32,16 +43,20 @@ export const ContainerEditableTitle = ({ containerId, textClassName }: EditableT
}
};

// istanbul ignore if: this should never happen
if (!container) {
return null;
let textTitle;
if (isLoading && placeHolderText) {
textTitle = placeHolderText;
} else if (isLoading || !container) {
textTitle = '';
} else {
textTitle = showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName;
}

return (
<InplaceTextEditor
onSave={handleSaveDisplayName}
text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
readOnly={readOnly}
text={textTitle}
readOnly={readOnly || libReadOnly}
textClassName={textClassName}
/>
);
Expand Down
18 changes: 16 additions & 2 deletions src/library-authoring/data/api.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ mockXBlockFields.dataHtml = {
metadata: { displayName: 'Introduction to Testing' },
} satisfies api.XBlockFields;
// Mock of another "regular" HTML (Text) block:
mockXBlockFields.usageKey0 = 'lb:org1:Demo_course:html:text-0';
mockXBlockFields.usageKey0 = 'lb:org1:Demo_course_generated:html:text-0';
mockXBlockFields.dataHtml0 = {
displayName: 'text block 0',
data: '<p>This is a text component which uses <strong>HTML</strong>.</p>',
Expand Down Expand Up @@ -493,6 +493,17 @@ export async function mockGetContainerMetadata(containerId: string): Promise<api
case mockGetContainerMetadata.subsectionIdEmpty:
return Promise.resolve(mockGetContainerMetadata.subsectionData);
default:
if (containerId.startsWith('lct:org1:Demo_course_generated:')) {
const lastPart = containerId.split(':').pop();
if (lastPart) {
const [name, idx] = lastPart.split('-');
return Promise.resolve({
...mockGetContainerMetadata.containerData,
displayName: `${name} block ${idx}`,
publishedDisplayName: `${name} block published ${idx}`,
});
}
}
return Promise.resolve(mockGetContainerMetadata.containerData);
}
}
Expand Down Expand Up @@ -575,19 +586,22 @@ export async function mockGetContainerChildren(containerId: string): Promise<api
}
let blockType = 'html';
let name = 'text';
let typeNamespace = 'lb';
if (containerId.includes('subsection')) {
blockType = 'unit';
name = blockType;
typeNamespace = 'lct';
} else if (containerId.includes('section')) {
blockType = 'subsection';
name = blockType;
typeNamespace = 'lct';
}
return Promise.resolve(
Array(numChildren).fill(mockGetContainerChildren.childTemplate).map((child, idx) => (
{
...child,
// Generate a unique ID for each child block to avoid "duplicate key" errors in tests
id: `lb:org1:Demo_course:${blockType}:${name}-${idx}`,
id: `${typeNamespace}:org1:Demo_course_generated:${blockType}:${name}-${idx}`,
displayName: `${name} block ${idx}`,
publishedDisplayName: `${name} block published ${idx}`,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,11 @@ import { ContainerType, getBlockType } from '../../generic/key-utils';
import { useLibraryContext } from '../common/context/LibraryContext';
import {
useContainerChildren,
useUpdateContainer,
useUpdateContainerChildren,
} from '../data/apiHooks';
import { messages, subsectionMessages, sectionMessages } from './messages';
import containerMessages from '../containers/messages';
import { ContainerEditableTitle } from '../containers/ContainerEditableTitle';
import { Container } from '../data/api';
import { InplaceTextEditor } from '../../generic/inplace-text-editor';
import { ToastContext } from '../../generic/toast-context';
import TagCount from '../../generic/tag-count';
import { ContainerMenu } from '../components/ContainerCard';
Expand All @@ -41,29 +39,17 @@ interface ContainerRowProps extends LibraryContainerChildrenProps {
}

const ContainerRow = ({ container, readOnly }: ContainerRowProps) => {
const intl = useIntl();
const { showToast } = useContext(ToastContext);
const updateMutation = useUpdateContainer(container.originalId);
const { showOnlyPublished } = useLibraryContext();

const handleSaveDisplayName = async (newDisplayName: string) => {
try {
await updateMutation.mutateAsync({
displayName: newDisplayName,
});
showToast(intl.formatMessage(containerMessages.updateContainerSuccessMsg));
} catch (err) {
showToast(intl.formatMessage(containerMessages.updateContainerErrorMsg));
}
};

return (
<>
<InplaceTextEditor
onSave={handleSaveDisplayName}
text={showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName}
textClassName="font-weight-bold small"
<ContainerEditableTitle
containerId={container.originalId}
readOnly={readOnly || showOnlyPublished}
textClassName="font-weight-bold small"
placeHolderText={
showOnlyPublished ? (container.publishedDisplayName ?? container.displayName) : container.displayName
}
/>
<ActionRow.Spacer />
<Stack
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
const childType = cType === ContainerType.Section
? ContainerType.Subsection
: ContainerType.Unit;
let typeNamespace = 'lct';
if (cType === ContainerType.Unit) {
typeNamespace = 'lb';
}
it(`shows the spinner before the query is complete in ${cType} page`, async () => {
// This mock will never return data about the collection (it loads forever):
const cId = cType === ContainerType.Section
Expand Down Expand Up @@ -253,7 +257,7 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
});

it(`should rename child by clicking edit icon besides name in ${cType} page`, async () => {
const url = getLibraryContainerApiUrl(`lb:org1:Demo_course:${childType}:${childType}-0`);
const url = getLibraryContainerApiUrl(`${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-0`);
axiosMock.onPatch(url).reply(200);
renderLibrarySectionPage(undefined, undefined, cType);

Expand Down Expand Up @@ -285,7 +289,7 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
});

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

Expand Down Expand Up @@ -326,7 +330,7 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
.onPatch(getLibraryContainerChildrenApiUrl(cId))
.reply(200);
verticalSortableListCollisionDetection.mockReturnValue([{
id: `lb:org1:Demo_course:${childType}:${childType}-1----1`,
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
Expand All @@ -344,7 +348,9 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
axiosMock
.onPatch(getLibraryContainerChildrenApiUrl(cId))
.reply(200);
verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]);
verticalSortableListCollisionDetection.mockReturnValue([{
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
});
Expand All @@ -361,7 +367,9 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
axiosMock
.onPatch(getLibraryContainerChildrenApiUrl(cId))
.reply(500);
verticalSortableListCollisionDetection.mockReturnValue([{ id: `lb:org1:Demo_course:${childType}:${childType}-1----1` }]);
verticalSortableListCollisionDetection.mockReturnValue([{
id: `${typeNamespace}:org1:Demo_course_generated:${childType}:${childType}-1----1`,
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
});
Expand All @@ -374,7 +382,7 @@ describe('<LibrarySectionPage / LibrarySubsectionPage />', () => {
const child = await screen.findByText(`${childType} block 0`);
// trigger double click
userEvent.click(child.parentElement!, undefined, { clickCount: 2 });
expect((await screen.findAllByText(new RegExp(`Test ${childType}`, 'i')))[0]).toBeInTheDocument();
expect((await screen.findAllByText(new RegExp(`${childType} block 0`, 'i')))[0]).toBeInTheDocument();
expect(await screen.findByRole('button', { name: new RegExp(`${childType} Info`, 'i') })).toBeInTheDocument();
});
});
Expand Down
16 changes: 11 additions & 5 deletions src/library-authoring/units/LibraryUnitPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ describe('<LibraryUnitPage />', () => {
});

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

Expand Down Expand Up @@ -251,7 +251,7 @@ describe('<LibraryUnitPage />', () => {
});

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

Expand Down Expand Up @@ -290,7 +290,9 @@ describe('<LibraryUnitPage />', () => {
axiosMock
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
.reply(200);
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
verticalSortableListCollisionDetection.mockReturnValue([{
id: 'lb:org1:Demo_course_generated:html:text-1----1',
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
});
Expand All @@ -304,7 +306,9 @@ describe('<LibraryUnitPage />', () => {
axiosMock
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
.reply(200);
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
verticalSortableListCollisionDetection.mockReturnValue([{
id: 'lb:org1:Demo_course_generated:html:text-1----1',
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
});
Expand All @@ -318,7 +322,9 @@ describe('<LibraryUnitPage />', () => {
axiosMock
.onPatch(getLibraryContainerChildrenApiUrl(mockGetContainerMetadata.unitId))
.reply(500);
verticalSortableListCollisionDetection.mockReturnValue([{ id: 'lb:org1:Demo_course:html:text-1----1' }]);
verticalSortableListCollisionDetection.mockReturnValue([{
id: 'lb:org1:Demo_course_generated:html:text-1----1',
}]);
await act(async () => {
fireEvent.keyDown(firstDragHandle, { code: 'Space' });
});
Expand Down