From 1ea2e06d4e1a8eebe49da6aed905d14736e9e60d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 1 Aug 2024 15:21:10 -0700 Subject: [PATCH 01/19] feat: make the text editor work with content libraries --- src/editors/data/redux/app/selectors.js | 3 +-- src/editors/data/redux/thunkActions/app.js | 11 +++++++++-- src/editors/data/services/cms/urls.js | 6 +----- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/editors/data/redux/app/selectors.js b/src/editors/data/redux/app/selectors.js index 9976eee19f..d1c7ffd666 100644 --- a/src/editors/data/redux/app/selectors.js +++ b/src/editors/data/redux/app/selectors.js @@ -42,10 +42,9 @@ export const returnUrl = createSelector( export const isInitialized = createSelector( [ - module.simpleSelectors.unitUrl, module.simpleSelectors.blockValue, ], - (unitUrl, blockValue) => !!(unitUrl && blockValue), + (blockValue) => !!(blockValue), ); export const displayTitle = createSelector( diff --git a/src/editors/data/redux/thunkActions/app.js b/src/editors/data/redux/thunkActions/app.js index fa50c91a06..0bd1a070af 100644 --- a/src/editors/data/redux/thunkActions/app.js +++ b/src/editors/data/redux/thunkActions/app.js @@ -89,7 +89,9 @@ export const initialize = (data) => (dispatch) => { const editorType = data.blockType; dispatch(actions.app.initialize(data)); dispatch(module.fetchBlock()); - dispatch(module.fetchUnit()); + if (data.blockId.startsWith('block-v1:')) { + dispatch(module.fetchUnit()); + } switch (editorType) { case 'problem': dispatch(module.fetchImages({ pageNumber: 0 })); @@ -100,7 +102,12 @@ export const initialize = (data) => (dispatch) => { dispatch(module.fetchCourseDetails()); break; case 'html': - dispatch(module.fetchImages({ pageNumber: 0 })); + if (data.learningContextId.startsWith('lib:')) { + // eslint-disable-next-line no-console + console.log('Not fetching image assets - not implemented yet for content libraries.'); + } else { + dispatch(module.fetchImages({ pageNumber: 0 })); + } break; default: break; diff --git a/src/editors/data/services/cms/urls.js b/src/editors/data/services/cms/urls.js index de4e9f158a..d101ff9241 100644 --- a/src/editors/data/services/cms/urls.js +++ b/src/editors/data/services/cms/urls.js @@ -38,11 +38,7 @@ export const blockAncestor = ({ studioEndpointUrl, blockId }) => { if (blockId.includes('block-v1')) { return `${block({ studioEndpointUrl, blockId })}?fields=ancestorInfo`; } - // this url only need to get info to build the return url, which isn't used by V2 blocks - // (temporary) don't throw error, just return empty url. it will fail it's network connection but otherwise - // the app will run - // throw new Error('Block ancestor not available (and not needed) for V2 blocks'); - return ''; + throw new Error('Block ancestor not available (and not needed) for V2 blocks'); }; export const blockStudioView = ({ studioEndpointUrl, blockId }) => ( From db477c7e44c1ade10900727df6e2c5253c02daca Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 31 Jul 2024 14:58:09 -0700 Subject: [PATCH 02/19] feat: In libraries, allow opening the editor for text (html) components --- src/CourseAuthoringRoutes.jsx | 2 +- src/editors/EditorContainer.jsx | 28 --------- src/editors/EditorContainer.test.jsx | 2 +- src/editors/EditorContainer.tsx | 42 ++++++++++++++ src/library-authoring/LibraryLayout.tsx | 57 +++++++++++++++++-- .../components/ComponentCard.tsx | 24 ++++++-- src/library-authoring/data/apiHooks.ts | 25 +++++++- 7 files changed, 136 insertions(+), 44 deletions(-) delete mode 100644 src/editors/EditorContainer.jsx create mode 100644 src/editors/EditorContainer.tsx diff --git a/src/CourseAuthoringRoutes.jsx b/src/CourseAuthoringRoutes.jsx index 51599317e6..0c9d2a1680 100644 --- a/src/CourseAuthoringRoutes.jsx +++ b/src/CourseAuthoringRoutes.jsx @@ -88,7 +88,7 @@ const CourseAuthoringRoutes = () => { /> } + element={} /> { - const { blockType, blockId } = useParams(); - return ( -
- -
- ); -}; -EditorContainer.propTypes = { - courseId: PropTypes.string.isRequired, -}; - -export default EditorContainer; diff --git a/src/editors/EditorContainer.test.jsx b/src/editors/EditorContainer.test.jsx index a6186050ae..d57d14c6b1 100644 --- a/src/editors/EditorContainer.test.jsx +++ b/src/editors/EditorContainer.test.jsx @@ -10,7 +10,7 @@ jest.mock('react-router', () => ({ }), })); -const props = { courseId: 'cOuRsEId' }; +const props = { learningContextId: 'cOuRsEId' }; describe('Editor Container', () => { describe('snapshots', () => { diff --git a/src/editors/EditorContainer.tsx b/src/editors/EditorContainer.tsx new file mode 100644 index 0000000000..aaa8680b61 --- /dev/null +++ b/src/editors/EditorContainer.tsx @@ -0,0 +1,42 @@ +/* eslint-disable react/require-default-props */ +import React from 'react'; +import { useParams } from 'react-router-dom'; +import { getConfig } from '@edx/frontend-platform'; + +import EditorPage from './EditorPage'; + +interface Props { + /** Course ID or Library ID */ + learningContextId: string; + /** Event handler for when user cancels out of the editor page */ + onClose?: () => void; + /** Event handler for when user saves changes using an editor */ + onSave?: () => (newData: Record) => void; +} + +const EditorContainer: React.FC = ({ + learningContextId, + onClose, + onSave, +}) => { + const { blockType, blockId } = useParams(); + if (blockType === undefined || blockId === undefined) { + // This shouldn't be possible; it's just here to satisfy the type checker. + return
Error: missing URL parameters
; + } + return ( +
+ +
+ ); +}; + +export default EditorContainer; diff --git a/src/library-authoring/LibraryLayout.tsx b/src/library-authoring/LibraryLayout.tsx index 95d829606f..65ad125b92 100644 --- a/src/library-authoring/LibraryLayout.tsx +++ b/src/library-authoring/LibraryLayout.tsx @@ -1,11 +1,58 @@ import React from 'react'; +import { + Route, + Routes, + useNavigate, + useParams, +} from 'react-router-dom'; +import { PageWrap } from '@edx/frontend-platform/react'; +import { useQueryClient } from '@tanstack/react-query'; + +import EditorContainer from '../editors/EditorContainer'; import LibraryAuthoringPage from './LibraryAuthoringPage'; import { LibraryProvider } from './common/context'; +import { invalidateComponentData } from './data/apiHooks'; + +const LibraryLayout = () => { + const { libraryId } = useParams(); + const queryClient = useQueryClient(); + + if (libraryId === undefined) { + throw new Error('Error: route is missing libraryId.'); // Should never happen + } + + const navigate = useNavigate(); + const goBack = React.useCallback(() => { + if (window.history.length > 1) { + navigate(-1); // go back + } else { + navigate(`/library/${libraryId}`); + } + // The following function is called only if changes are saved: + return ({ id: usageKey }) => { + // invalidate any queries that involve this XBlock: + invalidateComponentData(queryClient, libraryId, usageKey); + }; + }, []); -const LibraryLayout = () => ( - - - -); + return ( + + + + + + )} + /> + } + /> + + + ); +}; export default LibraryLayout; diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index f460bc3ba4..aca18c4521 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -1,5 +1,5 @@ import React, { useContext, useMemo, useState } from 'react'; -import { useIntl } from '@edx/frontend-platform/i18n'; +import { FormattedMessage, useIntl } from '@edx/frontend-platform/i18n'; import { ActionRow, Card, @@ -10,6 +10,7 @@ import { Stack, } from '@openedx/paragon'; import { MoreVert } from '@openedx/paragon/icons'; +import { Link, useParams } from 'react-router-dom'; import { getItemIcon, getComponentStyleColor } from '../../generic/block-type-utils'; import { updateClipboard } from '../../generic/data/api'; @@ -27,6 +28,9 @@ type ComponentCardProps = { export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { const intl = useIntl(); + // Get the block type (e.g. "html") from a lib block usage key string like "lb:org:lib:block_type:id" + const blockType: string = usageKey.split(':')[3] ?? 'unknown'; + const { libraryId } = useParams(); const { showToast } = useContext(ToastContext); const [clipboardBroadcastChannel] = useState(() => new BroadcastChannel(STUDIO_CLIPBOARD_CHANNEL)); const updateClipboardClick = () => { @@ -50,14 +54,22 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { data-testid="component-card-menu-toggle" /> - - {intl.formatMessage(messages.menuEdit)} - + { + blockType === 'html' ? ( + + + + ) : ( + + + + ) + } - {intl.formatMessage(messages.menuCopyToClipboard)} + - {intl.formatMessage(messages.menuAddToCollection)} + diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index a48832554f..f101bdd59c 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -1,6 +1,10 @@ import { camelCaseObject } from '@edx/frontend-platform'; import { - useQuery, useMutation, useQueryClient, type Query, + useQuery, + useMutation, + useQueryClient, + type Query, + type QueryClient, } from '@tanstack/react-query'; import { @@ -61,6 +65,22 @@ export const libraryAuthoringQueryKeys = { ], }; +/** + * Tell react-query to refresh its cache of any data related to the given + * component (XBlock). + * + * Note that technically it's possible to derive the library key from the + * usageKey, so we could refactor this to only require the usageKey. + * + * @param queryClient The query client - get it via useQueryClient() + * @param contentLibraryId The ID of library that holds the XBlock ("lib:...") + * @param usageKey The usage ID of the XBlock ("lb:...") + */ +export function invalidateComponentData(queryClient: QueryClient, contentLibraryId: string, usageKey: string) { + queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.xblockFields(contentLibraryId, usageKey) }); + queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, contentLibraryId) }); +} + /** * Hook to fetch a content library by its ID. */ @@ -205,8 +225,7 @@ export const useUpdateXBlockFields = (contentLibraryId: string, usageKey: string ); }, onSettled: () => { - queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.xblockFields(contentLibraryId, usageKey) }); - queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, contentLibraryId) }); + invalidateComponentData(queryClient, contentLibraryId, usageKey); }, }); }; From 42e50da1fd3c78835c8a9da7d657cf936e8a3118 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 4 Sep 2024 12:17:37 -0700 Subject: [PATCH 03/19] refactor: utility method for getting the "Edit" URL of a component --- .../component-info/ComponentInfo.tsx | 17 ++++-- .../components/ComponentCard.tsx | 11 ++-- .../components/utils.test.ts | 52 +++++++++++++++++++ src/library-authoring/components/utils.ts | 30 +++++++++++ 4 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 src/library-authoring/components/utils.test.ts create mode 100644 src/library-authoring/components/utils.ts diff --git a/src/library-authoring/component-info/ComponentInfo.tsx b/src/library-authoring/component-info/ComponentInfo.tsx index 4234722687..56f1835c13 100644 --- a/src/library-authoring/component-info/ComponentInfo.tsx +++ b/src/library-authoring/component-info/ComponentInfo.tsx @@ -6,7 +6,9 @@ import { Tabs, Stack, } from '@openedx/paragon'; +import { Link } from 'react-router-dom'; +import { getEditUrl } from '../components/utils'; import { ComponentMenu } from '../components'; import messages from './messages'; @@ -16,13 +18,22 @@ interface ComponentInfoProps { const ComponentInfo = ({ usageKey } : ComponentInfoProps) => { const intl = useIntl(); + const editUrl = getEditUrl(usageKey); return (
- + { + editUrl ? ( + + ) : ( + + ) + } diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index aca18c4521..0131185c32 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -10,7 +10,7 @@ import { Stack, } from '@openedx/paragon'; import { MoreVert } from '@openedx/paragon/icons'; -import { Link, useParams } from 'react-router-dom'; +import { Link } from 'react-router-dom'; import { getItemIcon, getComponentStyleColor } from '../../generic/block-type-utils'; import { updateClipboard } from '../../generic/data/api'; @@ -20,6 +20,7 @@ import { type ContentHit, Highlight } from '../../search-manager'; import { LibraryContext } from '../common/context'; import messages from './messages'; import { STUDIO_CLIPBOARD_CHANNEL } from '../../constants'; +import { getEditUrl } from './utils'; type ComponentCardProps = { contentHit: ContentHit, @@ -28,9 +29,7 @@ type ComponentCardProps = { export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { const intl = useIntl(); - // Get the block type (e.g. "html") from a lib block usage key string like "lb:org:lib:block_type:id" - const blockType: string = usageKey.split(':')[3] ?? 'unknown'; - const { libraryId } = useParams(); + const editUrl = getEditUrl(usageKey); const { showToast } = useContext(ToastContext); const [clipboardBroadcastChannel] = useState(() => new BroadcastChannel(STUDIO_CLIPBOARD_CHANNEL)); const updateClipboardClick = () => { @@ -55,8 +54,8 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { /> { - blockType === 'html' ? ( - + editUrl ? ( + ) : ( diff --git a/src/library-authoring/components/utils.test.ts b/src/library-authoring/components/utils.test.ts new file mode 100644 index 0000000000..496ced3cfe --- /dev/null +++ b/src/library-authoring/components/utils.test.ts @@ -0,0 +1,52 @@ +import { getBlockType, getEditUrl, getLibraryId } from './utils'; + +describe('component utils', () => { + describe('getBlockType', () => { + for (const [input, expected] of [ + ['lb:org:lib:html:id', 'html'], + ['lb:OpenCraftX:ALPHA:html:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'html'], + ['lb:Axim:beta:problem:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'problem'], + ['not a key', 'unknown'], + ]) { + it(`returns '${expected}' for usage key '${input}'`, () => { + expect(getBlockType(input)).toStrictEqual(expected); + }); + } + }); + describe('getLibraryId', () => { + for (const [input, expected] of [ + ['lb:org:lib:html:id', 'lib:org:lib'], + ['lb:OpenCraftX:ALPHA:html:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'lib:OpenCraftX:ALPHA'], + ['lb:Axim:beta:problem:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'lib:Axim:beta'], + ['not a key', 'lib:unknown:unknown'], + ]) { + it(`returns '${expected}' for usage key '${input}'`, () => { + expect(getLibraryId(input)).toStrictEqual(expected); + }); + } + }); + + describe('getEditUrl', () => { + it('returns the right URL for an HTML (Text) block', () => { + const usageKey = 'lb:org:ALPHA:html:571fe018-f3ce-45c9-8f53-5dafcb422fdd'; + expect(getEditUrl(usageKey)).toStrictEqual(`/library/lib:org:ALPHA/editor/html/${usageKey}`); + }); + it('doesn\'t yet allow editing a problem block', () => { + const usageKey = 'lb:org:beta:problem:571fe018-f3ce-45c9-8f53-5dafcb422fdd'; + expect(getEditUrl(usageKey)).toBeUndefined(); + }); + it('doesn\'t yet allow editing a video block', () => { + const usageKey = 'lb:org:beta:video:571fe018-f3ce-45c9-8f53-5dafcb422fdd'; + expect(getEditUrl(usageKey)).toBeUndefined(); + }); + it('doesn\'t yet allow editing a drag-and-drop-v2 block', () => { + const usageKey = 'lb:org:beta:drag-and-drop-v2:571fe018-f3ce-45c9-8f53-5dafcb422fdd'; + expect(getEditUrl(usageKey)).toBeUndefined(); + }); + it('returns undefined for an invalid key', () => { + expect(getEditUrl('foobar')).toBeUndefined(); + expect(getEditUrl('')).toBeUndefined(); + expect(getEditUrl('lb:unknown:unknown:unknown')).toBeUndefined(); + }); + }); +}); diff --git a/src/library-authoring/components/utils.ts b/src/library-authoring/components/utils.ts new file mode 100644 index 0000000000..ded0ec8290 --- /dev/null +++ b/src/library-authoring/components/utils.ts @@ -0,0 +1,30 @@ +/** + * Given a usage key like `lb:org:lib:html:id`, get the type (e.g. `html`) + * @param usageKey e.g. `lb:org:lib:html:id` + * @returns The block type as a string, or 'unknown' + */ +export function getBlockType(usageKey: string): string { + return usageKey.split(':')[3] ?? 'unknown'; +} + +/** + * Given a usage key like `lb:org:lib:html:id`, get the library key + * @param usageKey e.g. `lb:org:lib:html:id` + * @returns The library key, e.g. `lib:org:lib` + */ +export function getLibraryId(usageKey: string): string { + const org = usageKey.split(':')[1] ?? 'unknown'; + const lib = usageKey.split(':')[2] ?? 'unknown'; + return `lib:${org}:${lib}`; +} + +export function getEditUrl(usageKey: string): string | undefined { + const blockType = getBlockType(usageKey); + const libraryId = getLibraryId(usageKey); + + const mfeEditorTypes = ['html']; + if (mfeEditorTypes.includes(blockType)) { + return `/library/${libraryId}/editor/${blockType}/${usageKey}`; + } + return undefined; +} From 49a903ae4f6ad24c93e221c0445f5ede71a28ba2 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 4 Sep 2024 14:16:46 -0700 Subject: [PATCH 04/19] test: update tests --- src/editors/data/redux/app/selectors.test.js | 11 ++++------- src/editors/data/redux/thunkActions/app.js | 4 ++-- src/editors/data/redux/thunkActions/app.test.js | 7 ++++++- src/editors/data/services/cms/urls.test.js | 11 +++-------- 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/editors/data/redux/app/selectors.test.js b/src/editors/data/redux/app/selectors.test.js index 33a022e0b6..6a9fd2c94c 100644 --- a/src/editors/data/redux/app/selectors.test.js +++ b/src/editors/data/redux/app/selectors.test.js @@ -78,23 +78,20 @@ describe('app selectors unit tests', () => { }); }); describe('isInitialized selector', () => { - it('is memoized based on unitUrl, editorInitialized, and blockValue', () => { + it('is memoized based on editorInitialized and blockValue', () => { expect(selectors.isInitialized.preSelectors).toEqual([ - simpleSelectors.unitUrl, simpleSelectors.blockValue, ]); }); - it('returns true iff unitUrl, blockValue, and editorInitialized are all truthy', () => { + it('returns true iff blockValue and editorInitialized are truthy', () => { const { cb } = selectors.isInitialized; const truthy = { - url: { url: 'data' }, blockValue: { block: 'value' }, }; [ - [[null, truthy.blockValue], false], - [[truthy.url, null], false], - [[truthy.url, truthy.blockValue], true], + [[truthy.blockValue], true], + [[null], false], ].map(([args, expected]) => expect(cb(...args)).toEqual(expected)); }); }); diff --git a/src/editors/data/redux/thunkActions/app.js b/src/editors/data/redux/thunkActions/app.js index 0bd1a070af..2e210bb80d 100644 --- a/src/editors/data/redux/thunkActions/app.js +++ b/src/editors/data/redux/thunkActions/app.js @@ -89,7 +89,7 @@ export const initialize = (data) => (dispatch) => { const editorType = data.blockType; dispatch(actions.app.initialize(data)); dispatch(module.fetchBlock()); - if (data.blockId.startsWith('block-v1:')) { + if (data.blockId?.startsWith('block-v1:')) { dispatch(module.fetchUnit()); } switch (editorType) { @@ -102,7 +102,7 @@ export const initialize = (data) => (dispatch) => { dispatch(module.fetchCourseDetails()); break; case 'html': - if (data.learningContextId.startsWith('lib:')) { + if (data.learningContextId?.startsWith('lib:')) { // eslint-disable-next-line no-console console.log('Not fetching image assets - not implemented yet for content libraries.'); } else { diff --git a/src/editors/data/redux/thunkActions/app.test.js b/src/editors/data/redux/thunkActions/app.test.js index 2c962b2853..5cf52f7941 100644 --- a/src/editors/data/redux/thunkActions/app.test.js +++ b/src/editors/data/redux/thunkActions/app.test.js @@ -187,7 +187,6 @@ describe('app thunkActions', () => { expect(dispatch.mock.calls).toEqual([ [actions.app.initialize(testValue)], [thunkActions.fetchBlock()], - [thunkActions.fetchUnit()], ]); thunkActions.fetchBlock = fetchBlock; thunkActions.fetchUnit = fetchUnit; @@ -216,6 +215,8 @@ describe('app thunkActions', () => { const data = { ...testValue, blockType: 'html', + blockId: 'block-v1:UniversityX+PHYS+1+type@problem+block@123', + learningContextId: 'course-v1:UniversityX+PHYS+1', }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ @@ -251,6 +252,8 @@ describe('app thunkActions', () => { const data = { ...testValue, blockType: 'problem', + blockId: 'block-v1:UniversityX+PHYS+1+type@problem+block@123', + learningContextId: 'course-v1:UniversityX+PHYS+1', }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ @@ -286,6 +289,8 @@ describe('app thunkActions', () => { const data = { ...testValue, blockType: 'video', + blockId: 'block-v1:UniversityX+PHYS+1+type@problem+block@123', + learningContextId: 'course-v1:UniversityX+PHYS+1', }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ diff --git a/src/editors/data/services/cms/urls.test.js b/src/editors/data/services/cms/urls.test.js index bbaf1cbcff..94b39828c7 100644 --- a/src/editors/data/services/cms/urls.test.js +++ b/src/editors/data/services/cms/urls.test.js @@ -95,14 +95,9 @@ describe('cms url methods', () => { expect(blockAncestor({ studioEndpointUrl, blockId })) .toEqual(`${block({ studioEndpointUrl, blockId })}?fields=ancestorInfo`); }); - // This test will probably be used in the future - // it('throws error with studioEndpointUrl, v2 blockId and ancestor query', () => { - // expect(() => { blockAncestor({ studioEndpointUrl, blockId: v2BlockId }); }) - // .toThrow('Block ancestor not available (and not needed) for V2 blocks'); - // }); - it('returns blank url with studioEndpointUrl, v2 blockId and ancestor query', () => { - expect(blockAncestor({ studioEndpointUrl, blockId: v2BlockId })) - .toEqual(''); + it('throws error with studioEndpointUrl, v2 blockId and ancestor query', () => { + expect(() => { blockAncestor({ studioEndpointUrl, blockId: v2BlockId }); }) + .toThrow('Block ancestor not available (and not needed) for V2 blocks'); }); }); describe('blockStudioView', () => { From e21adef3365786d8879f6381cff53aa64720d102 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 4 Sep 2024 11:46:18 -0700 Subject: [PATCH 05/19] feat: developer info panel --- .../component-info/ComponentDeveloperInfo.tsx | 33 ++++++++++++++++ .../component-info/ComponentInfo.tsx | 5 +++ .../component-info/ComponentInfoHeader.tsx | 4 +- src/library-authoring/data/api.ts | 14 ++++++- src/library-authoring/data/apiHooks.ts | 38 ++++++++++++++----- 5 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 src/library-authoring/component-info/ComponentDeveloperInfo.tsx diff --git a/src/library-authoring/component-info/ComponentDeveloperInfo.tsx b/src/library-authoring/component-info/ComponentDeveloperInfo.tsx new file mode 100644 index 0000000000..983b2034ea --- /dev/null +++ b/src/library-authoring/component-info/ComponentDeveloperInfo.tsx @@ -0,0 +1,33 @@ +/* istanbul ignore file */ +/* eslint-disable import/prefer-default-export */ +// This file doesn't need test coverage nor i18n because it's only seen by devs +import React from 'react'; +import { LoadingSpinner } from '../../generic/Loading'; +import { useXBlockOLX } from '../data/apiHooks'; + +interface Props { + usageKey: string; +} + +export const ComponentDeveloperInfo: React.FC = ({ usageKey }) => { + const { data: olx, isLoading: isOLXLoading } = useXBlockOLX(usageKey); + return ( + <> +
+

Developer Component Details

+

(This panel is only visible in development builds.)

+
+
Usage key
+
{usageKey}
+
OLX
+
+ { + olx ? {olx} : // eslint-disable-line + isOLXLoading ? : // eslint-disable-line + Error + } +
+
+ + ); +}; diff --git a/src/library-authoring/component-info/ComponentInfo.tsx b/src/library-authoring/component-info/ComponentInfo.tsx index 56f1835c13..fa6db58296 100644 --- a/src/library-authoring/component-info/ComponentInfo.tsx +++ b/src/library-authoring/component-info/ComponentInfo.tsx @@ -10,6 +10,7 @@ import { Link } from 'react-router-dom'; import { getEditUrl } from '../components/utils'; import { ComponentMenu } from '../components'; +import { ComponentDeveloperInfo } from './ComponentDeveloperInfo'; import messages from './messages'; interface ComponentInfoProps { @@ -52,6 +53,10 @@ const ComponentInfo = ({ usageKey } : ComponentInfoProps) => { Details tab placeholder + + { + (process.env.NODE_ENV === 'development' ? : null) + } diff --git a/src/library-authoring/component-info/ComponentInfoHeader.tsx b/src/library-authoring/component-info/ComponentInfoHeader.tsx index 8f576fe1be..205acecfd4 100644 --- a/src/library-authoring/component-info/ComponentInfoHeader.tsx +++ b/src/library-authoring/component-info/ComponentInfoHeader.tsx @@ -24,9 +24,9 @@ const ComponentInfoHeader = ({ library, usageKey }: ComponentInfoHeaderProps) => const { data: xblockFields, - } = useXBlockFields(library.id, usageKey); + } = useXBlockFields(usageKey); - const updateMutation = useUpdateXBlockFields(library.id, usageKey); + const updateMutation = useUpdateXBlockFields(usageKey); const { showToast } = useContext(ToastContext); const handleSaveDisplayName = useCallback( diff --git a/src/library-authoring/data/api.ts b/src/library-authoring/data/api.ts index 323f57de93..1fb99baea0 100644 --- a/src/library-authoring/data/api.ts +++ b/src/library-authoring/data/api.ts @@ -25,9 +25,13 @@ export const getCommitLibraryChangesUrl = (libraryId: string) => `${getApiBaseUr */ export const getLibraryPasteClipboardUrl = (libraryId: string) => `${getApiBaseUrl()}/api/libraries/v2/${libraryId}/paste_clipboard/`; /** - * Get the URL for the xblock metadata API. + * Get the URL for the xblock fields/metadata API. */ export const getXBlockFieldsApiUrl = (usageKey: string) => `${getApiBaseUrl()}/api/xblock/v2/xblocks/${usageKey}/fields/`; +/** + * Get the URL for the xblock OLX API + */ +export const getXBlockOLXApiUrl = (usageKey: string) => `${getApiBaseUrl()}/api/libraries/v2/blocks/${usageKey}/olx/`; export interface ContentLibrary { id: string; @@ -236,3 +240,11 @@ export async function updateXBlockFields(usageKey:string, xblockData: UpdateXBlo const client = getAuthenticatedHttpClient(); await client.post(getXBlockFieldsApiUrl(usageKey), xblockData); } + +/** + * Fetch the OLX for the given XBlock. + */ +export async function getXBlockOLX(usageKey: string): Promise { + const { data } = await getAuthenticatedHttpClient().get(getXBlockOLXApiUrl(usageKey)); + return data.olx; +} diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index f101bdd59c..1ffe488774 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -7,6 +7,7 @@ import { type QueryClient, } from '@tanstack/react-query'; +import { getLibraryId } from '../components/utils'; import { type GetLibrariesV2CustomParams, type ContentLibrary, @@ -22,6 +23,7 @@ import { libraryPasteClipboard, getXBlockFields, updateXBlockFields, + getXBlockOLX, } from './api'; const libraryQueryPredicate = (query: Query, libraryId: string): boolean => { @@ -56,12 +58,21 @@ export const libraryAuthoringQueryKeys = { 'content', 'libraryBlockTypes', ], - xblockFields: (contentLibraryId: string, usageKey: string) => [ + xblockFields: (usageKey: string) => [ ...libraryAuthoringQueryKeys.all, - ...libraryAuthoringQueryKeys.contentLibrary(contentLibraryId), + ...libraryAuthoringQueryKeys.contentLibrary(getLibraryId(usageKey)), + 'content', + 'xblock', + usageKey, + 'fields', + ], + xblockOLX: (usageKey: string) => [ + ...libraryAuthoringQueryKeys.all, + ...libraryAuthoringQueryKeys.contentLibrary(getLibraryId(usageKey)), 'content', - 'xblockFields', + 'xblock', usageKey, + 'OLX', ], }; @@ -77,7 +88,7 @@ export const libraryAuthoringQueryKeys = { * @param usageKey The usage ID of the XBlock ("lb:...") */ export function invalidateComponentData(queryClient: QueryClient, contentLibraryId: string, usageKey: string) { - queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.xblockFields(contentLibraryId, usageKey) }); + queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.xblockFields(usageKey) }); queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, contentLibraryId) }); } @@ -188,20 +199,21 @@ export const useLibraryPasteClipboard = () => { }); }; -export const useXBlockFields = (contentLibrayId: string, usageKey: string) => ( +export const useXBlockFields = (usageKey: string) => ( useQuery({ - queryKey: libraryAuthoringQueryKeys.xblockFields(contentLibrayId, usageKey), + queryKey: libraryAuthoringQueryKeys.xblockFields(usageKey), queryFn: () => getXBlockFields(usageKey), enabled: !!usageKey, }) ); -export const useUpdateXBlockFields = (contentLibraryId: string, usageKey: string) => { +export const useUpdateXBlockFields = (usageKey: string) => { + const contentLibraryId = getLibraryId(usageKey); const queryClient = useQueryClient(); return useMutation({ mutationFn: (data: UpdateXBlockFieldsRequest) => updateXBlockFields(usageKey, data), onMutate: async (data) => { - const queryKey = libraryAuthoringQueryKeys.xblockFields(contentLibraryId, usageKey); + const queryKey = libraryAuthoringQueryKeys.xblockFields(usageKey); const previousBlockData = queryClient.getQueriesData(queryKey)[0][1] as XBlockFields; const formatedData = camelCaseObject(data); @@ -220,7 +232,7 @@ export const useUpdateXBlockFields = (contentLibraryId: string, usageKey: string }, onError: (_err, _data, context) => { queryClient.setQueryData( - libraryAuthoringQueryKeys.xblockFields(contentLibraryId, usageKey), + libraryAuthoringQueryKeys.xblockFields(usageKey), context?.previousBlockData, ); }, @@ -229,3 +241,11 @@ export const useUpdateXBlockFields = (contentLibraryId: string, usageKey: string }, }); }; + +export const useXBlockOLX = (usageKey: string) => ( + useQuery({ + queryKey: libraryAuthoringQueryKeys.xblockOLX(usageKey), + queryFn: () => getXBlockOLX(usageKey), + enabled: !!usageKey, + }) +); From 9eec8a4c092a621f75f1cd997a3d01b53b959d53 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 6 Sep 2024 11:38:38 -0700 Subject: [PATCH 06/19] fix: sidebar needs to stick to the screen properly --- src/library-authoring/LibraryAuthoringPage.scss | 5 +++++ src/library-authoring/LibraryAuthoringPage.tsx | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.scss b/src/library-authoring/LibraryAuthoringPage.scss index eaf87428b8..03a167b370 100644 --- a/src/library-authoring/LibraryAuthoringPage.scss +++ b/src/library-authoring/LibraryAuthoringPage.scss @@ -14,4 +14,9 @@ min-width: 300px; max-width: map-get($grid-breakpoints, "sm"); z-index: 1001; // to appear over header + position: sticky; + top: 0; + right: 0; + height: 100vh; + overflow-y: auto; } diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 7ec5e63a8a..9c5d85c629 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -153,8 +153,8 @@ const LibraryAuthoringPage = () => { }; return ( -
-
+
+
Date: Fri, 6 Sep 2024 11:57:05 -0700 Subject: [PATCH 07/19] feat: open the editor when creating a new component --- .../add-content/AddContentContainer.tsx | 15 ++++++++++++--- src/library-authoring/data/api.ts | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/library-authoring/add-content/AddContentContainer.tsx b/src/library-authoring/add-content/AddContentContainer.tsx index 421c81be68..f13023dac3 100644 --- a/src/library-authoring/add-content/AddContentContainer.tsx +++ b/src/library-authoring/add-content/AddContentContainer.tsx @@ -16,16 +16,19 @@ import { ContentPaste, } from '@openedx/paragon/icons'; import { v4 as uuid4 } from 'uuid'; -import { useParams } from 'react-router-dom'; +import { useNavigate, useParams } from 'react-router-dom'; + import { ToastContext } from '../../generic/toast-context'; import { useCopyToClipboard } from '../../generic/clipboard'; import { getCanEdit } from '../../course-unit/data/selectors'; import { useCreateLibraryBlock, useLibraryPasteClipboard } from '../data/apiHooks'; +import { getEditUrl } from '../components/utils'; import messages from './messages'; const AddContentContainer = () => { const intl = useIntl(); + const navigate = useNavigate(); const { libraryId } = useParams(); const createBlockMutation = useCreateLibraryBlock(); const pasteClipboardMutation = useLibraryPasteClipboard(); @@ -100,8 +103,14 @@ const AddContentContainer = () => { libraryId, blockType, definitionId: `${uuid4()}`, - }).then(() => { - showToast(intl.formatMessage(messages.successCreateMessage)); + }).then((data) => { + const editUrl = getEditUrl(data.id); + if (editUrl) { + navigate(editUrl); + } else { + // We can't start editing this right away so just show a toast message: + showToast(intl.formatMessage(messages.successCreateMessage)); + } }).catch(() => { showToast(intl.formatMessage(messages.errorCreateMessage)); }); diff --git a/src/library-authoring/data/api.ts b/src/library-authoring/data/api.ts index 1fb99baea0..e5ca502c56 100644 --- a/src/library-authoring/data/api.ts +++ b/src/library-authoring/data/api.ts @@ -160,7 +160,7 @@ export async function createLibraryBlock({ definition_id: definitionId, }, ); - return data; + return camelCaseObject(data); } /** From 7fddc77878c07b45b6ef96c296399d131139d81d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 6 Sep 2024 17:13:08 -0700 Subject: [PATCH 08/19] test: simplify the AddContentContainer tests --- src/generic/data/api.mock.ts | 40 +++++ src/generic/data/api.ts | 21 ++- .../add-content/AddContentContainer.test.tsx | 138 ++++++------------ 3 files changed, 106 insertions(+), 93 deletions(-) create mode 100644 src/generic/data/api.mock.ts diff --git a/src/generic/data/api.mock.ts b/src/generic/data/api.mock.ts new file mode 100644 index 0000000000..a6ec5bf47a --- /dev/null +++ b/src/generic/data/api.mock.ts @@ -0,0 +1,40 @@ +/* istanbul ignore file */ +import * as api from './api'; + +/** + * Mock for `getClipboard()` that simulates an empty clipboard + */ +export async function mockClipboardEmpty(): Promise { + return { + content: null, + sourceUsageKey: '', + sourceContextTitle: '', + sourceEditUrl: '', + }; +} +mockClipboardEmpty.applyMock = () => jest.spyOn(api, 'getClipboard').mockImplementation(mockClipboardEmpty); +mockClipboardEmpty.applyMockOnce = () => jest.spyOn(api, 'getClipboard').mockImplementationOnce(mockClipboardEmpty); + +/** + * Mock for `getClipboard()` that simulates a copied HTML component + */ +export async function mockClipboardHtml(): Promise { + return { + content: { + id: 69, + userId: 3, + created: '2024-01-16T13:33:21.314439Z', + purpose: 'clipboard', + status: 'ready', + blockType: 'html', + blockTypeDisplay: 'Text', + olxUrl: 'http://localhost:18010/api/content-staging/v1/staged-content/69/olx', + displayName: 'Blank HTML Page', + }, + sourceUsageKey: 'block-v1:edX+DemoX+Demo_Course+type@html+block@html1', + sourceContextTitle: 'Demonstration Course', + sourceEditUrl: 'http://localhost:18010/container/block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical1', + }; +} +mockClipboardHtml.applyMock = () => jest.spyOn(api, 'getClipboard').mockImplementation(mockClipboardHtml); +mockClipboardHtml.applyMockOnce = () => jest.spyOn(api, 'getClipboard').mockImplementationOnce(mockClipboardHtml); diff --git a/src/generic/data/api.ts b/src/generic/data/api.ts index fbec8b55bf..5fc5802eaa 100644 --- a/src/generic/data/api.ts +++ b/src/generic/data/api.ts @@ -47,10 +47,27 @@ export async function createOrRerunCourse(courseData: Object): Promise return camelCaseObject(data); } +export interface ClipboardStatus { + content: { + id: number; + userId: number; + created: string; // e.g. '2024-08-28T19:02:08.272192Z' + purpose: 'clipboard'; + status: 'ready' | 'loading' | 'expired' | 'error'; + blockType: string; + blockTypeDisplay: string; + olxUrl: string; + displayName: string; + } | null; + sourceUsageKey: string; // May be an empty string + sourceContextTitle: string; // May be an empty string + sourceEditUrl: string; // May be an empty string +} + /** * Retrieves user's clipboard. */ -export async function getClipboard(): Promise { +export async function getClipboard(): Promise { const { data } = await getAuthenticatedHttpClient() .get(getClipboardUrl()); @@ -60,7 +77,7 @@ export async function getClipboard(): Promise { /** * Updates user's clipboard. */ -export async function updateClipboard(usageKey: string): Promise { +export async function updateClipboard(usageKey: string): Promise { const { data } = await getAuthenticatedHttpClient() .post(getClipboardUrl(), { usage_key: usageKey }); diff --git a/src/library-authoring/add-content/AddContentContainer.test.tsx b/src/library-authoring/add-content/AddContentContainer.test.tsx index 6db80f15b5..69648180c3 100644 --- a/src/library-authoring/add-content/AddContentContainer.test.tsx +++ b/src/library-authoring/add-content/AddContentContainer.test.tsx @@ -1,38 +1,16 @@ -import React from 'react'; -import { initializeMockApp } from '@edx/frontend-platform'; -import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { - render, screen, fireEvent, waitFor, -} from '@testing-library/react'; -import { AppProvider } from '@edx/frontend-platform/react'; -import MockAdapter from 'axios-mock-adapter'; -import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; -import AddContentContainer from './AddContentContainer'; -import initializeStore from '../../store'; + fireEvent, + render, + waitFor, + initializeMocks, +} from '../../testUtils'; +import { mockContentLibrary } from '../data/api.mocks'; import { getCreateLibraryBlockUrl, getLibraryPasteClipboardUrl } from '../data/api'; -import { getClipboardUrl } from '../../generic/data/api'; - -import { clipboardXBlock } from '../../__mocks__'; - -const mockUseParams = jest.fn(); -let axiosMock; - -jest.mock('react-router-dom', () => ({ - ...jest.requireActual('react-router-dom'), // use actual for all non-hook parts - useParams: () => mockUseParams(), -})); - -const libraryId = '1'; -let store; +import { mockClipboardEmpty, mockClipboardHtml } from '../../generic/data/api.mock'; +import AddContentContainer from './AddContentContainer'; -const queryClient = new QueryClient({ - defaultOptions: { - queries: { - retry: false, - }, - }, -}); +const { libraryId } = mockContentLibrary; +const renderOpts = { path: '/library/:libraryId/*', params: { libraryId } }; const clipboardBroadcastChannelMock = { postMessage: jest.fn(), @@ -41,101 +19,79 @@ const clipboardBroadcastChannelMock = { (global as any).BroadcastChannel = jest.fn(() => clipboardBroadcastChannelMock); -const RootWrapper = () => ( - - - - - - - -); - describe('', () => { - beforeEach(() => { - initializeMockApp({ - authenticatedUser: { - userId: 3, - username: 'abc123', - administrator: true, - roles: [], - }, - }); - store = initializeStore(); - axiosMock = new MockAdapter(getAuthenticatedHttpClient()); - mockUseParams.mockReturnValue({ libraryId }); - }); - - afterEach(() => { - jest.clearAllMocks(); - }); - it('should render content buttons', () => { - render(); - expect(screen.getByRole('button', { name: /collection/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /text/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /problem/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /open reponse/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /drag drop/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /video/i })).toBeInTheDocument(); - expect(screen.getByRole('button', { name: /advanced \/ other/i })).toBeInTheDocument(); - expect(screen.queryByRole('button', { name: /copy from clipboard/i })).not.toBeInTheDocument(); + initializeMocks(); + mockClipboardEmpty.applyMock(); + const doc = render(); + expect(doc.getByRole('button', { name: /collection/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /text/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /problem/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /open reponse/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /drag drop/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /video/i })).toBeInTheDocument(); + expect(doc.getByRole('button', { name: /advanced \/ other/i })).toBeInTheDocument(); + expect(doc.queryByRole('button', { name: /copy from clipboard/i })).not.toBeInTheDocument(); }); it('should create a content', async () => { + const { axiosMock } = initializeMocks(); + mockClipboardEmpty.applyMock(); const url = getCreateLibraryBlockUrl(libraryId); axiosMock.onPost(url).reply(200); - render(); + const doc = render(, renderOpts); - const textButton = screen.getByRole('button', { name: /text/i }); + const textButton = doc.getByRole('button', { name: /text/i }); fireEvent.click(textButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(url)); }); it('should render paste button if clipboard contains pastable xblock', async () => { - const url = getClipboardUrl(); - axiosMock.onGet(url).reply(200, clipboardXBlock); - - render(); - - await waitFor(() => expect(axiosMock.history.get[0].url).toEqual(url)); - - expect(screen.getByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument(); + initializeMocks(); + // Simulate having an HTML block in the clipboard: + const getClipboardSpy = mockClipboardHtml.applyMock(); + const doc = render(, renderOpts); + expect(getClipboardSpy).toHaveBeenCalled(); // Hmm, this is getting called three times! Refactor to use react-query. + await waitFor(() => expect(doc.queryByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument()); }); it('should paste content', async () => { - const clipboardUrl = getClipboardUrl(); - axiosMock.onGet(clipboardUrl).reply(200, clipboardXBlock); + const { axiosMock } = initializeMocks(); + // Simulate having an HTML block in the clipboard: + const getClipboardSpy = mockClipboardHtml.applyMock(); const pasteUrl = getLibraryPasteClipboardUrl(libraryId); axiosMock.onPost(pasteUrl).reply(200); - render(); + const doc = render(, renderOpts); - await waitFor(() => expect(axiosMock.history.get[0].url).toEqual(clipboardUrl)); + expect(getClipboardSpy).toHaveBeenCalled(); // Hmm, this is getting called four times! Refactor to use react-query. - const pasteButton = screen.getByRole('button', { name: /paste from clipboard/i }); + await waitFor(() => expect(doc.queryByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument()); + const pasteButton = doc.getByRole('button', { name: /paste from clipboard/i }); fireEvent.click(pasteButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(pasteUrl)); }); - it('should fail pasting content', async () => { - const clipboardUrl = getClipboardUrl(); - axiosMock.onGet(clipboardUrl).reply(200, clipboardXBlock); + it('should handle failure to paste content', async () => { + const { axiosMock } = initializeMocks(); + // Simulate having an HTML block in the clipboard: + mockClipboardHtml.applyMock(); const pasteUrl = getLibraryPasteClipboardUrl(libraryId); axiosMock.onPost(pasteUrl).reply(400); - render(); + const doc = render(, renderOpts); - await waitFor(() => expect(axiosMock.history.get[0].url).toEqual(clipboardUrl)); - - const pasteButton = screen.getByRole('button', { name: /paste from clipboard/i }); + await waitFor(() => expect(doc.queryByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument()); + const pasteButton = doc.getByRole('button', { name: /paste from clipboard/i }); fireEvent.click(pasteButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(pasteUrl)); + + // TODO: check that an actual error message is shown?! }); }); From 637e4a37cbd53dc47aac8c9c3f3c464d269ef207 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 9 Sep 2024 09:46:27 -0700 Subject: [PATCH 09/19] test: workflow test for editor --- src/generic/data/api.mock.ts | 9 ++ .../LibraryAuthoringPage.test.tsx | 21 ++--- .../add-content/AddContentContainer.test.tsx | 11 +-- .../add-content/AddContentWorkflow.test.tsx | 89 +++++++++++++++++++ src/library-authoring/data/api.mocks.ts | 38 +++++++- src/search-manager/data/api.mock.ts | 42 +++++++++ 6 files changed, 184 insertions(+), 26 deletions(-) create mode 100644 src/library-authoring/add-content/AddContentWorkflow.test.tsx create mode 100644 src/search-manager/data/api.mock.ts diff --git a/src/generic/data/api.mock.ts b/src/generic/data/api.mock.ts index a6ec5bf47a..cc01da38fd 100644 --- a/src/generic/data/api.mock.ts +++ b/src/generic/data/api.mock.ts @@ -38,3 +38,12 @@ export async function mockClipboardHtml(): Promise { } mockClipboardHtml.applyMock = () => jest.spyOn(api, 'getClipboard').mockImplementation(mockClipboardHtml); mockClipboardHtml.applyMockOnce = () => jest.spyOn(api, 'getClipboard').mockImplementationOnce(mockClipboardHtml); + +/** Mock the DOM `BroadcastChannel` API which the clipboard code uses */ +export function mockBroadcastChannel() { + const clipboardBroadcastChannelMock = { + postMessage: jest.fn(), + close: jest.fn(), + }; + (global as any).BroadcastChannel = jest.fn(() => clipboardBroadcastChannelMock); +} diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 6a5cac0912..31747b505a 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -7,15 +7,18 @@ import { waitFor, within, } from '../testUtils'; -import { getContentSearchConfigUrl } from '../search-manager/data/api'; import mockResult from './__mocks__/library-search.json'; import mockEmptyResult from '../search-modal/__mocks__/empty-search-result.json'; import { mockContentLibrary, mockLibraryBlockTypes, mockXBlockFields } from './data/api.mocks'; +import { mockContentSearchConfig } from '../search-manager/data/api.mock'; +import { mockBroadcastChannel } from '../generic/data/api.mock'; import { LibraryLayout } from '.'; +mockContentSearchConfig.applyMock(); mockContentLibrary.applyMock(); mockLibraryBlockTypes.applyMock(); mockXBlockFields.applyMock(); +mockBroadcastChannel(); const searchEndpoint = 'http://mock.meilisearch.local/multi-search'; @@ -55,26 +58,12 @@ const returnLowNumberResults = (_url, req) => { return newMockResult; }; -const clipboardBroadcastChannelMock = { - postMessage: jest.fn(), - close: jest.fn(), -}; - -(global as any).BroadcastChannel = jest.fn(() => clipboardBroadcastChannelMock); - const path = '/library/:libraryId/*'; const libraryTitle = mockContentLibrary.libraryData.title; describe('', () => { beforeEach(() => { - const { axiosMock } = initializeMocks(); - - // The API method to get the Meilisearch connection details uses Axios: - axiosMock.onGet(getContentSearchConfigUrl()).reply(200, { - url: 'http://mock.meilisearch.local', - index_name: 'studio', - api_key: 'test-key', - }); + initializeMocks(); // The Meilisearch client-side API uses fetch, not Axios. fetchMock.post(searchEndpoint, (_url, req) => { diff --git a/src/library-authoring/add-content/AddContentContainer.test.tsx b/src/library-authoring/add-content/AddContentContainer.test.tsx index 69648180c3..5a386f01aa 100644 --- a/src/library-authoring/add-content/AddContentContainer.test.tsx +++ b/src/library-authoring/add-content/AddContentContainer.test.tsx @@ -6,19 +6,14 @@ import { } from '../../testUtils'; import { mockContentLibrary } from '../data/api.mocks'; import { getCreateLibraryBlockUrl, getLibraryPasteClipboardUrl } from '../data/api'; -import { mockClipboardEmpty, mockClipboardHtml } from '../../generic/data/api.mock'; +import { mockBroadcastChannel, mockClipboardEmpty, mockClipboardHtml } from '../../generic/data/api.mock'; import AddContentContainer from './AddContentContainer'; +mockBroadcastChannel(); + const { libraryId } = mockContentLibrary; const renderOpts = { path: '/library/:libraryId/*', params: { libraryId } }; -const clipboardBroadcastChannelMock = { - postMessage: jest.fn(), - close: jest.fn(), -}; - -(global as any).BroadcastChannel = jest.fn(() => clipboardBroadcastChannelMock); - describe('', () => { it('should render content buttons', () => { initializeMocks(); diff --git a/src/library-authoring/add-content/AddContentWorkflow.test.tsx b/src/library-authoring/add-content/AddContentWorkflow.test.tsx new file mode 100644 index 0000000000..555174e2fd --- /dev/null +++ b/src/library-authoring/add-content/AddContentWorkflow.test.tsx @@ -0,0 +1,89 @@ +/** + * Test the whole workflow of adding content, editing it, saving it + */ +import { snakeCaseObject } from '@edx/frontend-platform'; +import { + fireEvent, + render, + waitFor, + screen, + initializeMocks, +} from '../../testUtils'; +import mockResult from '../__mocks__/library-search.json'; +import editorCmsApi from '../../editors/data/services/cms/api'; +import * as textEditorHooks from '../../editors/containers/TextEditor/hooks'; +import { + mockContentLibrary, + mockCreateLibraryBlock, + mockLibraryBlockTypes, + mockXBlockFields, +} from '../data/api.mocks'; +import { mockBroadcastChannel, mockClipboardEmpty } from '../../generic/data/api.mock'; +import { mockContentSearchConfig, mockSearchResult } from '../../search-manager/data/api.mock'; +import LibraryLayout from '../LibraryLayout'; + +mockContentSearchConfig.applyMock(); +mockLibraryBlockTypes.applyMock(); +mockClipboardEmpty.applyMock(); +mockBroadcastChannel(); +mockContentLibrary.applyMock(); +mockCreateLibraryBlock.applyMock(); +mockSearchResult(mockResult); +// Mocking the redux APIs in the src/editors/ folder is a bit more involved: +jest.spyOn(editorCmsApi as any, 'fetchBlockById').mockImplementation( + async (args: { blockId: string }) => ( + { status: 200, data: snakeCaseObject(await mockXBlockFields(args.blockId)) } + ), +); +jest.spyOn(textEditorHooks, 'getContent').mockImplementation(() => () => '

Edited HTML content

'); +jest.mock('frontend-components-tinymce-advanced-plugins', () => ({ a11ycheckerCss: '' })); + +const { libraryId } = mockContentLibrary; +const renderOpts = { + // Mount the on this route, to simulate how it's mounted in the real app: + path: '/library/:libraryId/*', + // And set the current URL to the following: + routerProps: { initialEntries: [`/library/${libraryId}/components`] }, +}; + +describe('AddContentWorkflow test', () => { + beforeEach(() => { + initializeMocks(); + }); + + it('can create an HTML component', async () => { + render(, renderOpts); + + // Click "New [Component]" + const newComponentButton = await screen.findByRole('button', { name: /New/ }); + fireEvent.click(newComponentButton); + + // Click "Text" to create a text component + fireEvent.click(await screen.findByRole('button', { name: /Text/ })); + + // Then the editor should open + expect(await screen.findByRole('heading', { name: /New Text Component/ })).toBeInTheDocument(); + + // Edit the title + fireEvent.click(screen.getByRole('button', { name: /Edit Title/ })); + const titleInput = screen.getByPlaceholderText('Title'); + fireEvent.change(titleInput, { target: { value: 'A customized title' } }); + fireEvent.blur(titleInput); + await waitFor(() => expect(screen.queryByRole('heading', { name: /New Text Component/ })).not.toBeInTheDocument()); + expect(screen.getByRole('heading', { name: /A customized title/ })); + + // Note that TinyMCE doesn't really load properly in our test environment + // so we can't really edit the text, but we have getContent() mocked to simulate + // using TinyMCE to enter some new HTML. + + // Mock the save() REST API method: + const saveSpy = jest.spyOn(editorCmsApi as any, 'saveBlock').mockImplementationOnce(async () => ({ + status: 200, data: { id: mockXBlockFields.usageKeyNewHtml }, + })); + + // Click Save + const saveButton = screen.getByLabelText('Save changes and return to learning context'); + fireEvent.click(saveButton); + expect(saveSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/library-authoring/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index 863d25607c..cb744173b8 100644 --- a/src/library-authoring/data/api.mocks.ts +++ b/src/library-authoring/data/api.mocks.ts @@ -104,7 +104,31 @@ mockContentLibrary.libraryIdReadOnly = 'lib:Axim:readOnly'; mockContentLibrary.libraryIdThatNeverLoads = 'lib:Axim:infiniteLoading'; mockContentLibrary.library404 = 'lib:Axim:error404'; mockContentLibrary.library500 = 'lib:Axim:error500'; -mockContentLibrary.applyMock = () => { jest.spyOn(api, 'getContentLibrary').mockImplementation(mockContentLibrary); }; +mockContentLibrary.applyMock = () => jest.spyOn(api, 'getContentLibrary').mockImplementation(mockContentLibrary); + +/** + * Mock for `createLibraryBlock()` + */ +export async function mockCreateLibraryBlock( + args: api.CreateBlockDataRequest, +): ReturnType { + if (args.blockType === 'html' && args.libraryId === mockContentLibrary.libraryId) { + return mockCreateLibraryBlock.newHtmlData; + } + throw new Error(`mockCreateLibraryBlock doesn't know how to mock ${JSON.stringify(args)}`); +} +mockCreateLibraryBlock.newHtmlData = { + id: 'lb:Axim:TEST:html:123', + defKey: '123', + blockType: 'html', + displayName: 'New Text Component', + hasUnpublishedChanges: true, + tagsCount: 0, +} satisfies api.CreateBlockDataResponse; +/** Apply this mock. Returns a spy object that can tell you if it's been called. */ +mockCreateLibraryBlock.applyMock = () => ( + jest.spyOn(api, 'createLibraryBlock').mockImplementation(mockCreateLibraryBlock) +); /** * Mock for `getXBlockFields()` @@ -117,13 +141,23 @@ export async function mockXBlockFields(usageKey: string): PromiseThis is a text component which uses HTML.

', metadata: { displayName: 'Introduction to Testing' }, } satisfies api.XBlockFields; -mockXBlockFields.applyMock = () => { jest.spyOn(api, 'getXBlockFields').mockImplementation(mockXBlockFields); }; +// Mock of a blank/new HTML (Text) block: +mockXBlockFields.usageKeyNewHtml = 'lb:Axim:TEST:html:123'; +mockXBlockFields.dataNewHtml = { + displayName: 'New Text Component', + data: '', + metadata: { displayName: 'New Text Component' }, +} satisfies api.XBlockFields; +/** Apply this mock. Returns a spy object that can tell you if it's been called. */ +mockXBlockFields.applyMock = () => jest.spyOn(api, 'getXBlockFields').mockImplementation(mockXBlockFields); diff --git a/src/search-manager/data/api.mock.ts b/src/search-manager/data/api.mock.ts new file mode 100644 index 0000000000..dfcc9584ae --- /dev/null +++ b/src/search-manager/data/api.mock.ts @@ -0,0 +1,42 @@ +/* istanbul ignore file */ +// eslint-disable-next-line import/no-extraneous-dependencies +import fetchMock from 'fetch-mock-jest'; +import type { MultiSearchResponse } from 'meilisearch'; +import * as api from './api'; + +/** + * Mock getContentSearchConfig() + */ +export async function mockContentSearchConfig(): ReturnType { + return { + url: 'http://mock.meilisearch.local', + indexName: 'studio', + apiKey: 'test-key', + }; +} +mockContentSearchConfig.searchEndpointUrl = 'http://mock.meilisearch.local/multi-search'; +mockContentSearchConfig.applyMock = () => ( + jest.spyOn(api, 'getContentSearchConfig').mockImplementation(mockContentSearchConfig) +); + +/** + * Mock all future Meilisearch searches with the given response. + * + * For a given test suite, this mock will stay in effect until you call it with + * a different mock response, or you call `fetchMock.mockReset()` + */ +export function mockSearchResult(mockResponse: MultiSearchResponse) { + fetchMock.post(mockContentSearchConfig.searchEndpointUrl, (_url, req) => { + const requestData = JSON.parse(req.body?.toString() ?? ''); + const query = requestData?.queries[0]?.q ?? ''; + // We have to replace the query (search keywords) in the mock results with the actual query, + // because otherwise Instantsearch will update the UI and change the query, + // leading to unexpected results in the test cases. + const newMockResponse = { ...mockResponse }; + newMockResponse.results[0].query = query; + // And fake the required '_formatted' fields; it contains the highlighting ... around matched words + // eslint-disable-next-line no-underscore-dangle, no-param-reassign + mockResponse.results[0]?.hits.forEach((hit) => { hit._formatted = { ...hit }; }); + return newMockResponse; + }, { overwriteRoutes: true }); +} From 51470b3f582aa2c750f33fd8a5c160924bdc4a5e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 10 Sep 2024 10:35:18 -0700 Subject: [PATCH 10/19] test: update test coverage --- src/editors/EditorContainer.tsx | 2 +- src/generic/toast-context/index.tsx | 4 +-- .../LibraryAuthoringPage.tsx | 1 + src/library-authoring/LibraryLayout.tsx | 3 ++- .../add-content/AddContentWorkflow.test.tsx | 23 ++++++++++++++--- .../component-info/ComponentDeveloperInfo.tsx | 1 + src/library-authoring/data/api.mocks.ts | 11 ++++++++ src/library-authoring/data/apiHooks.ts | 1 + src/testUtils.tsx | 25 ++++++++++++++++--- 9 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/editors/EditorContainer.tsx b/src/editors/EditorContainer.tsx index aaa8680b61..8d26f24901 100644 --- a/src/editors/EditorContainer.tsx +++ b/src/editors/EditorContainer.tsx @@ -21,7 +21,7 @@ const EditorContainer: React.FC = ({ }) => { const { blockType, blockId } = useParams(); if (blockType === undefined || blockId === undefined) { - // This shouldn't be possible; it's just here to satisfy the type checker. + // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker. return
Error: missing URL parameters
; } return ( diff --git a/src/generic/toast-context/index.tsx b/src/generic/toast-context/index.tsx index f4fd2aa332..40145068fa 100644 --- a/src/generic/toast-context/index.tsx +++ b/src/generic/toast-context/index.tsx @@ -16,11 +16,11 @@ export interface ToastProviderProps { * Global context to keep track of popup message(s) that appears to user after * they take an action like creating or deleting something. */ -export const ToastContext = React.createContext({ +export const ToastContext = React.createContext({ toastMessage: null, showToast: () => {}, closeToast: () => {}, -} as ToastContextData); +}); /** * React component to provide `ToastContext` to the app diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 9c5d85c629..6e5bbc647e 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -120,6 +120,7 @@ const LibraryAuthoringPage = () => { const { libraryId } = useParams(); if (!libraryId) { + // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker. throw new Error('Rendered without libraryId URL parameter'); } const { data: libraryData, isLoading } = useContentLibrary(libraryId); diff --git a/src/library-authoring/LibraryLayout.tsx b/src/library-authoring/LibraryLayout.tsx index 65ad125b92..f956f96533 100644 --- a/src/library-authoring/LibraryLayout.tsx +++ b/src/library-authoring/LibraryLayout.tsx @@ -18,7 +18,8 @@ const LibraryLayout = () => { const queryClient = useQueryClient(); if (libraryId === undefined) { - throw new Error('Error: route is missing libraryId.'); // Should never happen + // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker. + throw new Error('Error: route is missing libraryId.'); } const navigate = useNavigate(); diff --git a/src/library-authoring/add-content/AddContentWorkflow.test.tsx b/src/library-authoring/add-content/AddContentWorkflow.test.tsx index 555174e2fd..5a77c92953 100644 --- a/src/library-authoring/add-content/AddContentWorkflow.test.tsx +++ b/src/library-authoring/add-content/AddContentWorkflow.test.tsx @@ -47,11 +47,8 @@ const renderOpts = { }; describe('AddContentWorkflow test', () => { - beforeEach(() => { - initializeMocks(); - }); - it('can create an HTML component', async () => { + initializeMocks(); render(, renderOpts); // Click "New [Component]" @@ -86,4 +83,22 @@ describe('AddContentWorkflow test', () => { fireEvent.click(saveButton); expect(saveSpy).toHaveBeenCalledTimes(1); }); + + it('can create a Problem component', async () => { + const { mockShowToast } = initializeMocks(); + render(, renderOpts); + + // Click "New [Component]" + const newComponentButton = await screen.findByRole('button', { name: /New/ }); + fireEvent.click(newComponentButton); + + // Pre-condition - this is NOT shown yet: + expect(screen.queryByText('Content created successfully.')).not.toBeInTheDocument(); + + // Click "Problem" to create a capa problem component + fireEvent.click(await screen.findByRole('button', { name: /Problem/ })); + + // We haven't yet implemented the problem editor, so we expect only a toast to appear + await waitFor(() => expect(mockShowToast).toHaveBeenCalledWith('Content created successfully.')); + }); }); diff --git a/src/library-authoring/component-info/ComponentDeveloperInfo.tsx b/src/library-authoring/component-info/ComponentDeveloperInfo.tsx index 983b2034ea..430d9a7636 100644 --- a/src/library-authoring/component-info/ComponentDeveloperInfo.tsx +++ b/src/library-authoring/component-info/ComponentDeveloperInfo.tsx @@ -9,6 +9,7 @@ interface Props { usageKey: string; } +/* istanbul ignore next */ export const ComponentDeveloperInfo: React.FC = ({ usageKey }) => { const { data: olx, isLoading: isOLXLoading } = useXBlockOLX(usageKey); return ( diff --git a/src/library-authoring/data/api.mocks.ts b/src/library-authoring/data/api.mocks.ts index cb744173b8..6b35bac8b5 100644 --- a/src/library-authoring/data/api.mocks.ts +++ b/src/library-authoring/data/api.mocks.ts @@ -115,6 +115,9 @@ export async function mockCreateLibraryBlock( if (args.blockType === 'html' && args.libraryId === mockContentLibrary.libraryId) { return mockCreateLibraryBlock.newHtmlData; } + if (args.blockType === 'problem' && args.libraryId === mockContentLibrary.libraryId) { + return mockCreateLibraryBlock.newProblemData; + } throw new Error(`mockCreateLibraryBlock doesn't know how to mock ${JSON.stringify(args)}`); } mockCreateLibraryBlock.newHtmlData = { @@ -125,6 +128,14 @@ mockCreateLibraryBlock.newHtmlData = { hasUnpublishedChanges: true, tagsCount: 0, } satisfies api.CreateBlockDataResponse; +mockCreateLibraryBlock.newProblemData = { + id: 'lb:Axim:TEST:problem:prob1', + defKey: 'prob1', + blockType: 'problem', + displayName: 'New Problem', + hasUnpublishedChanges: true, + tagsCount: 0, +} satisfies api.CreateBlockDataResponse; /** Apply this mock. Returns a spy object that can tell you if it's been called. */ mockCreateLibraryBlock.applyMock = () => ( jest.spyOn(api, 'createLibraryBlock').mockImplementation(mockCreateLibraryBlock) diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index 1ffe488774..983a197819 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -242,6 +242,7 @@ export const useUpdateXBlockFields = (usageKey: string) => { }); }; +/* istanbul ignore next */ // This is only used in developer builds, and the associated UI doesn't work in test or prod export const useXBlockOLX = (usageKey: string) => ( useQuery({ queryKey: libraryAuthoringQueryKeys.xblockOLX(usageKey), diff --git a/src/testUtils.tsx b/src/testUtils.tsx index d306dfb1a7..283e341f48 100644 --- a/src/testUtils.tsx +++ b/src/testUtils.tsx @@ -6,6 +6,7 @@ */ import React from 'react'; import { AxiosError } from 'axios'; +import { jest } from '@jest/globals'; import { initializeMockApp } from '@edx/frontend-platform'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import { IntlProvider } from '@edx/frontend-platform/i18n'; @@ -20,6 +21,7 @@ import { Routes, } from 'react-router-dom'; +import { ToastContext, type ToastContextData } from './generic/toast-context'; import initializeReduxStore from './store'; /** @deprecated Use React Query and/or regular React Context instead of redux */ @@ -27,6 +29,13 @@ let reduxStore; let queryClient; let axiosMock: MockAdapter; +/** To use this: `const { mockShowToast } = initializeMocks()` and `expect(mockShowToast).toHaveBeenCalled()` */ +let mockToastContext: ToastContextData = { + showToast: jest.fn(), + closeToast: jest.fn(), + toastMessage: null, +}; + export interface RouteOptions { /** The URL path, like '/libraries/:libraryId' */ path?: string; @@ -106,9 +115,11 @@ function makeWrapper({ ...routeArgs }: RouteOptions) { - - {children} - + + + {children} + + @@ -152,9 +163,17 @@ export function initializeMocks({ user = defaultUser } = {}) { }); axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + // Reset `mockToastContext` for this current test + mockToastContext = { + showToast: jest.fn(), + closeToast: jest.fn(), + toastMessage: null, + }; + return { reduxStore, axiosMock, + mockShowToast: mockToastContext.showToast, }; } From 7c7dee3c0ecd2441bbff5b5a468a0dd20edc34d1 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 09:51:17 -0700 Subject: [PATCH 11/19] test: cleanups to AddContentContainer test (from review comments) --- .../add-content/AddContentContainer.test.tsx | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/library-authoring/add-content/AddContentContainer.test.tsx b/src/library-authoring/add-content/AddContentContainer.test.tsx index 5a386f01aa..f0f721c393 100644 --- a/src/library-authoring/add-content/AddContentContainer.test.tsx +++ b/src/library-authoring/add-content/AddContentContainer.test.tsx @@ -1,6 +1,7 @@ import { fireEvent, render, + screen, waitFor, initializeMocks, } from '../../testUtils'; @@ -18,15 +19,15 @@ describe('', () => { it('should render content buttons', () => { initializeMocks(); mockClipboardEmpty.applyMock(); - const doc = render(); - expect(doc.getByRole('button', { name: /collection/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /text/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /problem/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /open reponse/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /drag drop/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /video/i })).toBeInTheDocument(); - expect(doc.getByRole('button', { name: /advanced \/ other/i })).toBeInTheDocument(); - expect(doc.queryByRole('button', { name: /copy from clipboard/i })).not.toBeInTheDocument(); + render(); + expect(screen.getByRole('button', { name: /collection/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /text/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /problem/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /open reponse/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /drag drop/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /video/i })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /advanced \/ other/i })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /copy from clipboard/i })).not.toBeInTheDocument(); }); it('should create a content', async () => { @@ -35,9 +36,9 @@ describe('', () => { const url = getCreateLibraryBlockUrl(libraryId); axiosMock.onPost(url).reply(200); - const doc = render(, renderOpts); + render(, renderOpts); - const textButton = doc.getByRole('button', { name: /text/i }); + const textButton = screen.getByRole('button', { name: /text/i }); fireEvent.click(textButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(url)); @@ -60,12 +61,11 @@ describe('', () => { const pasteUrl = getLibraryPasteClipboardUrl(libraryId); axiosMock.onPost(pasteUrl).reply(200); - const doc = render(, renderOpts); + render(, renderOpts); expect(getClipboardSpy).toHaveBeenCalled(); // Hmm, this is getting called four times! Refactor to use react-query. - await waitFor(() => expect(doc.queryByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument()); - const pasteButton = doc.getByRole('button', { name: /paste from clipboard/i }); + const pasteButton = await screen.findByRole('button', { name: /paste from clipboard/i }); fireEvent.click(pasteButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(pasteUrl)); @@ -79,10 +79,9 @@ describe('', () => { const pasteUrl = getLibraryPasteClipboardUrl(libraryId); axiosMock.onPost(pasteUrl).reply(400); - const doc = render(, renderOpts); + render(, renderOpts); - await waitFor(() => expect(doc.queryByRole('button', { name: /paste from clipboard/i })).toBeInTheDocument()); - const pasteButton = doc.getByRole('button', { name: /paste from clipboard/i }); + const pasteButton = await screen.findByRole('button', { name: /paste from clipboard/i }); fireEvent.click(pasteButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(pasteUrl)); From cdd2b63f70c4dfa47faa9396ecdd8df62567a446 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 10:00:45 -0700 Subject: [PATCH 12/19] refactor: cleanups from code review --- src/editors/EditorContainer.tsx | 9 ++++----- src/library-authoring/LibraryLayout.tsx | 9 +++------ .../component-info/ComponentInfo.tsx | 18 +++++++----------- .../components/ComponentCard.tsx | 16 ++++------------ 4 files changed, 18 insertions(+), 34 deletions(-) diff --git a/src/editors/EditorContainer.tsx b/src/editors/EditorContainer.tsx index 8d26f24901..692382ecca 100644 --- a/src/editors/EditorContainer.tsx +++ b/src/editors/EditorContainer.tsx @@ -1,4 +1,3 @@ -/* eslint-disable react/require-default-props */ import React from 'react'; import { useParams } from 'react-router-dom'; import { getConfig } from '@edx/frontend-platform'; @@ -10,14 +9,14 @@ interface Props { learningContextId: string; /** Event handler for when user cancels out of the editor page */ onClose?: () => void; - /** Event handler for when user saves changes using an editor */ - onSave?: () => (newData: Record) => void; + /** Event handler called after when user saves their changes using an editor */ + afterSave?: () => (newData: Record) => void; } const EditorContainer: React.FC = ({ learningContextId, onClose, - onSave, + afterSave, }) => { const { blockType, blockId } = useParams(); if (blockType === undefined || blockId === undefined) { @@ -33,7 +32,7 @@ const EditorContainer: React.FC = ({ studioEndpointUrl={getConfig().STUDIO_BASE_URL} lmsEndpointUrl={getConfig().LMS_BASE_URL} onClose={onClose} - returnFunction={onSave} + returnFunction={afterSave} />
); diff --git a/src/library-authoring/LibraryLayout.tsx b/src/library-authoring/LibraryLayout.tsx index f956f96533..960f8814f1 100644 --- a/src/library-authoring/LibraryLayout.tsx +++ b/src/library-authoring/LibraryLayout.tsx @@ -24,11 +24,8 @@ const LibraryLayout = () => { const navigate = useNavigate(); const goBack = React.useCallback(() => { - if (window.history.length > 1) { - navigate(-1); // go back - } else { - navigate(`/library/${libraryId}`); - } + // Go back to the library + navigate(`/library/${libraryId}`); // The following function is called only if changes are saved: return ({ id: usageKey }) => { // invalidate any queries that involve this XBlock: @@ -43,7 +40,7 @@ const LibraryLayout = () => { path="editor/:blockType/:blockId?" element={( - + )} /> diff --git a/src/library-authoring/component-info/ComponentInfo.tsx b/src/library-authoring/component-info/ComponentInfo.tsx index fa6db58296..f17dbff3e4 100644 --- a/src/library-authoring/component-info/ComponentInfo.tsx +++ b/src/library-authoring/component-info/ComponentInfo.tsx @@ -24,17 +24,13 @@ const ComponentInfo = ({ usageKey } : ComponentInfoProps) => { return (
- { - editUrl ? ( - - ) : ( - - ) - } + diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index 0131185c32..80b93c48ed 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -29,7 +29,7 @@ type ComponentCardProps = { export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { const intl = useIntl(); - const editUrl = getEditUrl(usageKey); + const editUrl = usageKey && getEditUrl(usageKey); const { showToast } = useContext(ToastContext); const [clipboardBroadcastChannel] = useState(() => new BroadcastChannel(STUDIO_CLIPBOARD_CHANNEL)); const updateClipboardClick = () => { @@ -53,17 +53,9 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { data-testid="component-card-menu-toggle" /> - { - editUrl ? ( - - - - ) : ( - - - - ) - } + + + From b0f83d6efa3c6a51793b16d95b251633674f1831 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 10:27:15 -0700 Subject: [PATCH 13/19] refactor: better validation for usageKey utils --- .../components/utils.test.ts | 15 +++++++-- src/library-authoring/components/utils.ts | 31 ++++++++++++++----- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/library-authoring/components/utils.test.ts b/src/library-authoring/components/utils.test.ts index 496ced3cfe..8d09c43986 100644 --- a/src/library-authoring/components/utils.test.ts +++ b/src/library-authoring/components/utils.test.ts @@ -6,24 +6,35 @@ describe('component utils', () => { ['lb:org:lib:html:id', 'html'], ['lb:OpenCraftX:ALPHA:html:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'html'], ['lb:Axim:beta:problem:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'problem'], - ['not a key', 'unknown'], ]) { it(`returns '${expected}' for usage key '${input}'`, () => { expect(getBlockType(input)).toStrictEqual(expected); }); } + + for (const input of ['', undefined, null, 'not a key', 'lb:foo']) { + it(`throws an exception for usage key '${input}'`, () => { + expect(() => getBlockType(input as any)).toThrow(`Invalid usageKey: ${input}`); + }); + } }); + describe('getLibraryId', () => { for (const [input, expected] of [ ['lb:org:lib:html:id', 'lib:org:lib'], ['lb:OpenCraftX:ALPHA:html:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'lib:OpenCraftX:ALPHA'], ['lb:Axim:beta:problem:571fe018-f3ce-45c9-8f53-5dafcb422fdd', 'lib:Axim:beta'], - ['not a key', 'lib:unknown:unknown'], ]) { it(`returns '${expected}' for usage key '${input}'`, () => { expect(getLibraryId(input)).toStrictEqual(expected); }); } + + for (const input of ['', undefined, null, 'not a key', 'lb:foo']) { + it(`throws an exception for usage key '${input}'`, () => { + expect(() => getLibraryId(input as any)).toThrow(`Invalid usageKey: ${input}`); + }); + } }); describe('getEditUrl', () => { diff --git a/src/library-authoring/components/utils.ts b/src/library-authoring/components/utils.ts index ded0ec8290..095d956a00 100644 --- a/src/library-authoring/components/utils.ts +++ b/src/library-authoring/components/utils.ts @@ -1,10 +1,16 @@ /** * Given a usage key like `lb:org:lib:html:id`, get the type (e.g. `html`) * @param usageKey e.g. `lb:org:lib:html:id` - * @returns The block type as a string, or 'unknown' + * @returns The block type as a string */ export function getBlockType(usageKey: string): string { - return usageKey.split(':')[3] ?? 'unknown'; + if (usageKey && usageKey.startsWith('lb:')) { + const blockType = usageKey.split(':')[3]; + if (blockType) { + return blockType; + } + } + throw new Error(`Invalid usageKey: ${usageKey}`); } /** @@ -13,14 +19,25 @@ export function getBlockType(usageKey: string): string { * @returns The library key, e.g. `lib:org:lib` */ export function getLibraryId(usageKey: string): string { - const org = usageKey.split(':')[1] ?? 'unknown'; - const lib = usageKey.split(':')[2] ?? 'unknown'; - return `lib:${org}:${lib}`; + if (usageKey && usageKey.startsWith('lb:')) { + const org = usageKey.split(':')[1]; + const lib = usageKey.split(':')[2]; + if (org && lib) { + return `lib:${org}:${lib}`; + } + } + throw new Error(`Invalid usageKey: ${usageKey}`); } export function getEditUrl(usageKey: string): string | undefined { - const blockType = getBlockType(usageKey); - const libraryId = getLibraryId(usageKey); + let blockType: string; + let libraryId: string; + try { + blockType = getBlockType(usageKey); + libraryId = getLibraryId(usageKey); + } catch { + return undefined; + } const mfeEditorTypes = ['html']; if (mfeEditorTypes.includes(blockType)) { From 9e62bc05d79887c185186d8ce2124c5974e52eaf Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 10:42:47 -0700 Subject: [PATCH 14/19] refactor: avoid footgun with EditorContainer --- src/editors/EditorContainer.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/editors/EditorContainer.tsx b/src/editors/EditorContainer.tsx index 692382ecca..fc6fa417c1 100644 --- a/src/editors/EditorContainer.tsx +++ b/src/editors/EditorContainer.tsx @@ -23,6 +23,16 @@ const EditorContainer: React.FC = ({ // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker. return
Error: missing URL parameters
; } + if (!!onClose !== !!afterSave) { + /* istanbul ignore next */ + throw new Error('You must specify both onClose and afterSave or neither.'); + // These parameters are a bit messy so I'm trying to help make it more + // consistent here. For example, if you specify onClose, then returnFunction + // is only called if the save is successful. But if you leave onClose + // undefined, then returnFunction is called in either case, and with + // different arguments. The underlying EditorPage should be refactored to + // have more clear events like onCancel and onSaveSuccess + } return (
Date: Wed, 11 Sep 2024 10:50:12 -0700 Subject: [PATCH 15/19] fix: console warning when TextEditor is initialized --- src/editors/containers/TextEditor/index.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/editors/containers/TextEditor/index.jsx b/src/editors/containers/TextEditor/index.jsx index 522436ffa7..f27ecf83f3 100644 --- a/src/editors/containers/TextEditor/index.jsx +++ b/src/editors/containers/TextEditor/index.jsx @@ -111,7 +111,7 @@ TextEditor.propTypes = { initializeEditor: PropTypes.func.isRequired, showRawEditor: PropTypes.bool.isRequired, blockFinished: PropTypes.bool, - learningContextId: PropTypes.string.isRequired, + learningContextId: PropTypes.string, // This should be required but is NULL when the store is in initial state :/ images: PropTypes.shape({}).isRequired, isLibrary: PropTypes.bool.isRequired, // inject From b5e754a48c004c862dda188882d4b2f1f6c699e7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 10:53:02 -0700 Subject: [PATCH 16/19] fix: a11y - add alt text for "exit" button in the editors --- .../EditorContainer/__snapshots__/index.test.jsx.snap | 2 ++ src/editors/containers/EditorContainer/index.jsx | 1 + src/editors/containers/EditorContainer/messages.js | 5 +++++ 3 files changed, 8 insertions(+) diff --git a/src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap b/src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap index 49598b47ee..02c89e55d7 100644 --- a/src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap +++ b/src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap @@ -51,6 +51,7 @@ exports[`EditorContainer component render snapshot: initialized. enable save and />
diff --git a/src/editors/containers/EditorContainer/messages.js b/src/editors/containers/EditorContainer/messages.js index b8301ca810..a6f1754fb2 100644 --- a/src/editors/containers/EditorContainer/messages.js +++ b/src/editors/containers/EditorContainer/messages.js @@ -12,6 +12,11 @@ const messages = defineMessages({ defaultMessage: 'Are you sure you want to exit the editor? Any unsaved changes will be lost.', description: 'Description text for modal confirming cancellation', }, + exitButtonAlt: { + id: 'authoring.editorContainer.exitButton.alt', + defaultMessage: 'Exit the editor', + description: 'Alt text for the Exit button', + }, okButtonLabel: { id: 'authoring.editorContainer.okButton.label', defaultMessage: 'OK', From f5796aca6a9c5788ab51f2d8daf5164966dcb0bd Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 11:11:07 -0700 Subject: [PATCH 17/19] refactor: separate xblock queryKeys from library queryKeys --- src/library-authoring/data/apiHooks.ts | 38 ++++++++++++-------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/library-authoring/data/apiHooks.ts b/src/library-authoring/data/apiHooks.ts index 983a197819..365bd8d1e6 100644 --- a/src/library-authoring/data/apiHooks.ts +++ b/src/library-authoring/data/apiHooks.ts @@ -58,22 +58,18 @@ export const libraryAuthoringQueryKeys = { 'content', 'libraryBlockTypes', ], - xblockFields: (usageKey: string) => [ - ...libraryAuthoringQueryKeys.all, - ...libraryAuthoringQueryKeys.contentLibrary(getLibraryId(usageKey)), - 'content', - 'xblock', - usageKey, - 'fields', - ], - xblockOLX: (usageKey: string) => [ - ...libraryAuthoringQueryKeys.all, - ...libraryAuthoringQueryKeys.contentLibrary(getLibraryId(usageKey)), - 'content', - 'xblock', - usageKey, - 'OLX', - ], +}; + +export const xblockQueryKeys = { + all: ['xblock'], + /** + * Base key for data specific to a xblock + */ + xblock: (usageKey?: string) => [...xblockQueryKeys.all, usageKey], + /** Fields (i.e. the content, display name, etc.) of an XBlock */ + xblockFields: (usageKey: string) => [...xblockQueryKeys.xblock(usageKey), 'fields'], + /** OLX (XML representation of the fields/content) */ + xblockOLX: (usageKey: string) => [...xblockQueryKeys.xblock(usageKey), 'OLX'], }; /** @@ -88,7 +84,7 @@ export const libraryAuthoringQueryKeys = { * @param usageKey The usage ID of the XBlock ("lb:...") */ export function invalidateComponentData(queryClient: QueryClient, contentLibraryId: string, usageKey: string) { - queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.xblockFields(usageKey) }); + queryClient.invalidateQueries({ queryKey: xblockQueryKeys.xblockFields(usageKey) }); queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, contentLibraryId) }); } @@ -201,7 +197,7 @@ export const useLibraryPasteClipboard = () => { export const useXBlockFields = (usageKey: string) => ( useQuery({ - queryKey: libraryAuthoringQueryKeys.xblockFields(usageKey), + queryKey: xblockQueryKeys.xblockFields(usageKey), queryFn: () => getXBlockFields(usageKey), enabled: !!usageKey, }) @@ -213,7 +209,7 @@ export const useUpdateXBlockFields = (usageKey: string) => { return useMutation({ mutationFn: (data: UpdateXBlockFieldsRequest) => updateXBlockFields(usageKey, data), onMutate: async (data) => { - const queryKey = libraryAuthoringQueryKeys.xblockFields(usageKey); + const queryKey = xblockQueryKeys.xblockFields(usageKey); const previousBlockData = queryClient.getQueriesData(queryKey)[0][1] as XBlockFields; const formatedData = camelCaseObject(data); @@ -232,7 +228,7 @@ export const useUpdateXBlockFields = (usageKey: string) => { }, onError: (_err, _data, context) => { queryClient.setQueryData( - libraryAuthoringQueryKeys.xblockFields(usageKey), + xblockQueryKeys.xblockFields(usageKey), context?.previousBlockData, ); }, @@ -245,7 +241,7 @@ export const useUpdateXBlockFields = (usageKey: string) => { /* istanbul ignore next */ // This is only used in developer builds, and the associated UI doesn't work in test or prod export const useXBlockOLX = (usageKey: string) => ( useQuery({ - queryKey: libraryAuthoringQueryKeys.xblockOLX(usageKey), + queryKey: xblockQueryKeys.xblockOLX(usageKey), queryFn: () => getXBlockOLX(usageKey), enabled: !!usageKey, }) From bb051e69d6b35a04711bd71b09e052cebe14039d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 11:27:49 -0700 Subject: [PATCH 18/19] test: simplify flaky test --- src/library-authoring/LibraryAuthoringPage.test.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 31747b505a..873f05720b 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -393,9 +393,8 @@ describe('', () => { expect(mockResult0.display_name).toStrictEqual(displayName); await renderLibraryPage(); - // Click on the first component - waitFor(() => expect(screen.queryByText(displayName)).toBeInTheDocument()); - fireEvent.click(screen.getAllByText(displayName)[0]); + // Click on the first component. It should appear twice, in both "Recently Modified" and "Components" + fireEvent.click((await screen.findAllByText(displayName))[0]); const sidebar = screen.getByTestId('library-sidebar'); From be9d01bdb2ae0438dc2c687825fc697a2a3e6a00 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 11 Sep 2024 13:55:34 -0700 Subject: [PATCH 19/19] fix: propTypes validation warning on Toast content string --- .../__snapshots__/index.test.jsx.snap | 30 ++++--------------- src/editors/containers/TextEditor/index.jsx | 4 +-- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/src/editors/containers/TextEditor/__snapshots__/index.test.jsx.snap b/src/editors/containers/TextEditor/__snapshots__/index.test.jsx.snap index c7a8f48e8d..32b7e20300 100644 --- a/src/editors/containers/TextEditor/__snapshots__/index.test.jsx.snap +++ b/src/editors/containers/TextEditor/__snapshots__/index.test.jsx.snap @@ -24,11 +24,7 @@ exports[`TextEditor snapshots block failed to load, Toast is shown 1`] = ` onClose={[MockFunction hooks.nullMethod]} show={true} > - + Error: Could Not Load Text Content - + Error: Could Not Load Text Content - + Error: Could Not Load Text Content
- + Error: Could Not Load Text Content - + Error: Could Not Load Text Content
- + { intl.formatMessage(messages.couldNotLoadTextContext) } {(!blockFinished)