From 6b2ba6e063285f3678a592ccf9f2e40275972847 Mon Sep 17 00:00:00 2001 From: Diana Olarte Date: Wed, 26 Feb 2025 05:28:42 +1100 Subject: [PATCH] feat: cancellable block creation/edit workflow in libraries v2 (#1574) Before, clicking "new problem" (etc) would create a new block, then launch the editor. Now it launches the editor and then creates the new block only on save. This makes the "Cancel" button work as expected. Only affects libraries so far, not courses. --- .env.test | 3 +- .../containers/EditorContainer/hooks.test.jsx | 29 +++++ .../containers/EditorContainer/hooks.ts | 25 ++++- .../containers/EditorContainer/index.test.tsx | 33 ++++-- .../containers/EditorContainer/index.tsx | 17 +++ .../containers/ProblemEditor/index.test.tsx | 11 +- .../containers/ProblemEditor/index.tsx | 6 +- src/editors/containers/TextEditor/index.jsx | 3 +- .../containers/TextEditor/index.test.jsx | 4 +- src/editors/containers/VideoEditor/index.tsx | 3 +- src/editors/data/constants/problem.ts | 4 + src/editors/data/constants/requests.ts | 2 + src/editors/data/redux/app/reducer.ts | 1 - src/editors/data/redux/app/selectors.test.ts | 26 ++++- src/editors/data/redux/app/selectors.ts | 24 +++- src/editors/data/redux/index.ts | 10 +- src/editors/data/redux/requests/reducer.js | 1 + src/editors/data/redux/thunkActions/app.js | 61 +++++++++- .../data/redux/thunkActions/app.test.js | 83 +++++++++++++- .../data/redux/thunkActions/problem.ts | 2 +- .../data/redux/thunkActions/requests.js | 68 +++++++++++- .../data/redux/thunkActions/requests.test.js | 96 +++++++++++++++- src/editors/data/redux/thunkActions/video.js | 4 +- src/editors/hooks.test.jsx | 50 +++++++++ src/editors/hooks.ts | 25 ++++- .../ImageUploadModal/index.jsx | 2 +- .../add-content/AddContentContainer.test.tsx | 72 +++++------- .../add-content/AddContentContainer.tsx | 104 ++++++++++-------- .../add-content/AddContentWorkflow.test.tsx | 37 ++++--- .../common/context/LibraryContext.tsx | 15 +-- .../components/ComponentEditorModal.tsx | 8 +- 31 files changed, 663 insertions(+), 166 deletions(-) diff --git a/.env.test b/.env.test index 7ee23dd9fb..0be671946d 100644 --- a/.env.test +++ b/.env.test @@ -39,4 +39,5 @@ INVITE_STUDENTS_EMAIL_TO="someone@domain.com" ENABLE_HOME_PAGE_COURSE_API_V2=true ENABLE_CHECKLIST_QUALITY=true ENABLE_GRADING_METHOD_IN_PROBLEMS=false -LIBRARY_SUPPORTED_BLOCKS="problem,video,html,drag-and-drop-v2" +# "other" is used to test the workflow for creating blocks that aren't supported by the built-in editors +LIBRARY_SUPPORTED_BLOCKS="problem,video,html,drag-and-drop-v2,other" diff --git a/src/editors/containers/EditorContainer/hooks.test.jsx b/src/editors/containers/EditorContainer/hooks.test.jsx index 815a4cc12a..8434685941 100644 --- a/src/editors/containers/EditorContainer/hooks.test.jsx +++ b/src/editors/containers/EditorContainer/hooks.test.jsx @@ -16,6 +16,7 @@ jest.mock('../../data/redux', () => ({ app: { isInitialized: (state) => ({ isInitialized: state }), images: (state) => ({ images: state }), + shouldCreateBlock: (state) => ({ shouldCreateBlock: state }), }, requests: { isFailed: (...args) => ({ requestFailed: args }), @@ -26,6 +27,7 @@ jest.mock('../../hooks', () => ({ ...jest.requireActual('../../hooks'), navigateCallback: jest.fn((args) => ({ navigateCallback: args })), saveBlock: jest.fn((args) => ({ saveBlock: args })), + createBlock: jest.fn((args) => ({ createBlock: args })), })); const dispatch = jest.fn(); @@ -53,6 +55,8 @@ describe('EditorContainer hooks', () => { const getContent = () => 'myTestContentValue'; const setAssetToStaticUrl = () => 'myTestContentValue'; const validateEntry = () => 'vaLIdAteENTry'; + reactRedux.useSelector.mockReturnValue(false); + const output = hooks.handleSaveClicked({ getContent, images: { @@ -73,6 +77,31 @@ describe('EditorContainer hooks', () => { validateEntry, }); }); + it('returns callback to createBlock with dispatch and content if shouldCreateBlock is true', () => { + const getContent = () => 'myTestContentValue'; + const setAssetToStaticUrl = () => 'myTestContentValue'; + const validateEntry = () => 'vaLIdAteENTry'; + reactRedux.useSelector.mockReturnValue(true); + + const output = hooks.handleSaveClicked({ + getContent, + images: { + portableUrl: '/static/sOmEuiMAge.jpeg', + displayName: 'sOmEuiMAge', + }, + destination: 'testDEsTURL', + analytics: 'soMEanALytics', + dispatch, + validateEntry, + }); + output(); + expect(appHooks.createBlock).toHaveBeenCalledWith({ + content: setAssetToStaticUrl(reactRedux.useSelector(selectors.app.images), getContent), + destination: reactRedux.useSelector(selectors.app.returnUrl), + analytics: reactRedux.useSelector(selectors.app.analytics), + dispatch, + }); + }); }); describe('cancelConfirmModalToggle', () => { diff --git a/src/editors/containers/EditorContainer/hooks.ts b/src/editors/containers/EditorContainer/hooks.ts index 935e3ad89d..bb0b6d53e3 100644 --- a/src/editors/containers/EditorContainer/hooks.ts +++ b/src/editors/containers/EditorContainer/hooks.ts @@ -9,9 +9,11 @@ import * as appHooks from '../../hooks'; export const { clearSaveError, + clearCreateError, navigateCallback, nullMethod, saveBlock, + createBlock, } = appHooks; export const state = StrictDict({ @@ -27,10 +29,20 @@ export const handleSaveClicked = ({ }) => { // eslint-disable-next-line react-hooks/rules-of-hooks const returnUrl = useSelector(selectors.app.returnUrl); + // eslint-disable-next-line react-hooks/rules-of-hooks + const createBlockOnSave = useSelector(selectors.app.shouldCreateBlock); const destination = returnFunction ? '' : returnUrl; // eslint-disable-next-line react-hooks/rules-of-hooks const analytics = useSelector(selectors.app.analytics); - + if (createBlockOnSave) { + return () => createBlock({ + analytics, + content: getContent({ dispatch }), + destination, + dispatch, + returnFunction, + }); + } return () => saveBlock({ analytics, content: getContent({ dispatch }), @@ -79,3 +91,14 @@ export const isInitialized = () => useSelector(selectors.app.isInitialized); export const saveFailed = () => useSelector((rootState) => ( selectors.requests.isFailed(rootState, { requestKey: RequestKeys.saveBlock }) )); + +export const createFailed = () => ({ + // eslint-disable-next-line react-hooks/rules-of-hooks + createFailed: useSelector((rootState) => ( + selectors.requests.isFailed(rootState, { requestKey: RequestKeys.createBlock }) + )), + // eslint-disable-next-line react-hooks/rules-of-hooks + createFailedError: useSelector((rootState) => ( + selectors.requests.error(rootState, { requestKey: RequestKeys.createBlock }) + )), +}); diff --git a/src/editors/containers/EditorContainer/index.test.tsx b/src/editors/containers/EditorContainer/index.test.tsx index a35e4d74b8..f61730626c 100644 --- a/src/editors/containers/EditorContainer/index.test.tsx +++ b/src/editors/containers/EditorContainer/index.test.tsx @@ -9,6 +9,7 @@ import { import editorCmsApi from '../../data/services/cms/api'; import EditorPage from '../../EditorPage'; +import * as hooks from './hooks'; // Mock this plugins component: jest.mock('frontend-components-tinymce-advanced-plugins', () => ({ a11ycheckerCss: '' })); @@ -17,17 +18,6 @@ jest.spyOn(editorCmsApi, 'fetchCourseImages').mockImplementation(async () => ( / { data: { assets: [], start: 0, end: 0, page: 0, pageSize: 50, totalCount: 0 } } )); // Mock out the 'get ancestors' API: -jest.spyOn(editorCmsApi, 'fetchByUnitId').mockImplementation(async () => ({ - status: 200, - data: { - ancestors: [{ - id: 'block-v1:Org+TS100+24+type@vertical+block@parent', - display_name: 'You-Knit? The Test Unit', - category: 'vertical', - has_children: true, - }], - }, -})); const isDirtyMock = jest.fn(); jest.mock('../TextEditor/hooks', () => ({ @@ -60,6 +50,17 @@ describe('EditorContainer', () => { jest.spyOn(window, 'removeEventListener'); jest.spyOn(mockEvent, 'preventDefault'); Object.defineProperty(mockEvent, 'returnValue', { writable: true }); + jest.spyOn(editorCmsApi, 'fetchByUnitId').mockImplementation(async () => ({ + status: 200, + data: { + ancestors: [{ + id: 'block-v1:Org+TS100+24+type@vertical+block@parent', + display_name: 'You-Knit? The Test Unit', + category: 'vertical', + has_children: true, + }], + }, + })); }); afterEach(() => { @@ -165,4 +166,14 @@ describe('EditorContainer', () => { expect(mockEvent.preventDefault).toHaveBeenCalled(); expect(mockEvent.returnValue).toBe(true); }); + test('should display an alert when is an error creating a new block', async () => { + jest.spyOn(hooks, 'createFailed').mockImplementation(() => ({ createFailed: true, createFailedError: 'error' })); + render(); + expect(await screen.findByText(/There was an error creating the content/i)).toBeInTheDocument(); + }); + test('should display an alert when is an error saving the changes', async () => { + jest.spyOn(hooks, 'saveFailed').mockImplementation(() => true); + render(); + expect(await screen.findByText(/Error: Content save failed. Please check recent changes and try again later./i)).toBeInTheDocument(); + }); }); diff --git a/src/editors/containers/EditorContainer/index.tsx b/src/editors/containers/EditorContainer/index.tsx index bc22c360da..6fa3cfc9ba 100644 --- a/src/editors/containers/EditorContainer/index.tsx +++ b/src/editors/containers/EditorContainer/index.tsx @@ -18,6 +18,9 @@ import { useEditorContext } from '../../EditorContext'; import TitleHeader from './components/TitleHeader'; import * as hooks from './hooks'; import messages from './messages'; +import { parseErrorMsg } from '../../../library-authoring/add-content/AddContentContainer'; +import libraryMessages from '../../../library-authoring/add-content/messages'; + import './index.scss'; import usePromptIfDirty from '../../../generic/promptIfDirty/usePromptIfDirty'; import CancelConfirmModal from './components/CancelConfirmModal'; @@ -81,9 +84,12 @@ const EditorContainer: React.FC = ({ const isInitialized = hooks.isInitialized(); const { isCancelConfirmOpen, openCancelConfirmModal, closeCancelConfirmModal } = hooks.cancelConfirmModalToggle(); const handleCancel = hooks.handleCancel({ onClose, returnFunction }); + const { createFailed, createFailedError } = hooks.createFailed(); const disableSave = !isInitialized; const saveFailed = hooks.saveFailed(); const clearSaveFailed = hooks.clearSaveError({ dispatch }); + const clearCreateFailed = hooks.clearCreateError({ dispatch }); + const handleSave = hooks.handleSaveClicked({ dispatch, getContent, @@ -113,6 +119,16 @@ const EditorContainer: React.FC = ({ }; return ( + {createFailed && ( + + {parseErrorMsg( + intl, + createFailedError, + libraryMessages.errorCreateMessageWithDetail, + libraryMessages.errorCreateMessage, + )} + + )} {saveFailed && ( @@ -126,6 +142,7 @@ const EditorContainer: React.FC = ({ if (returnFunction) { closeCancelConfirmModal(); } + dispatch({ type: 'resetEditor' }); }} /> diff --git a/src/editors/containers/ProblemEditor/index.test.tsx b/src/editors/containers/ProblemEditor/index.test.tsx index ab3d37095a..7f58db69c7 100644 --- a/src/editors/containers/ProblemEditor/index.test.tsx +++ b/src/editors/containers/ProblemEditor/index.test.tsx @@ -27,6 +27,7 @@ jest.mock('../../data/redux', () => ({ selectors: { app: { blockValue: jest.fn(state => ({ blockValue: state })), + shouldCreateBlock: jest.fn(state => ({ shouldCreateBlock: state })), }, problem: { problemType: jest.fn(state => ({ problemType: state })), @@ -104,12 +105,18 @@ describe('ProblemEditor', () => { test('blockFinished from requests.isFinished', () => { expect( mapStateToProps(testState).blockFinished, - ).toEqual(selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchBlock })); + ).toEqual( + selectors.app.shouldCreateBlock(testState) + || selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchBlock }), + ); }); test('advancedSettingsFinished from requests.isFinished', () => { expect( mapStateToProps(testState).advancedSettingsFinished, - ).toEqual(selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchAdvancedSettings })); + ).toEqual( + selectors.app.shouldCreateBlock(testState) + || selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchAdvancedSettings }), + ); }); }); describe('mapDispatchToProps', () => { diff --git a/src/editors/containers/ProblemEditor/index.tsx b/src/editors/containers/ProblemEditor/index.tsx index d8154c7e87..8f30ff9498 100644 --- a/src/editors/containers/ProblemEditor/index.tsx +++ b/src/editors/containers/ProblemEditor/index.tsx @@ -65,11 +65,13 @@ const ProblemEditor: React.FC = ({ }; export const mapStateToProps = (state) => ({ - blockFinished: selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchBlock }), + blockFinished: selectors.app.shouldCreateBlock(state) + || selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchBlock }), blockFailed: selectors.requests.isFailed(state, { requestKey: RequestKeys.fetchBlock }), problemType: selectors.problem.problemType(state), blockValue: selectors.app.blockValue(state), - advancedSettingsFinished: selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchAdvancedSettings }), + advancedSettingsFinished: selectors.app.shouldCreateBlock(state) + || selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchAdvancedSettings }), }); export const mapDispatchToProps = { diff --git a/src/editors/containers/TextEditor/index.jsx b/src/editors/containers/TextEditor/index.jsx index 0546e65b04..10bab16dbd 100644 --- a/src/editors/containers/TextEditor/index.jsx +++ b/src/editors/containers/TextEditor/index.jsx @@ -132,7 +132,8 @@ export const mapStateToProps = (state) => ({ blockFailed: selectors.requests.isFailed(state, { requestKey: RequestKeys.fetchBlock }), blockId: selectors.app.blockId(state), showRawEditor: selectors.app.showRawEditor(state), - blockFinished: selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchBlock }), + blockFinished: selectors.app.shouldCreateBlock(state) + || selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchBlock }), learningContextId: selectors.app.learningContextId(state), images: selectors.app.images(state), isLibrary: selectors.app.isLibrary(state), diff --git a/src/editors/containers/TextEditor/index.test.jsx b/src/editors/containers/TextEditor/index.test.jsx index ea3bffc945..2b386b3bb8 100644 --- a/src/editors/containers/TextEditor/index.test.jsx +++ b/src/editors/containers/TextEditor/index.test.jsx @@ -55,6 +55,7 @@ jest.mock('../../data/redux', () => ({ selectors: { app: { blockValue: jest.fn(state => ({ blockValue: state })), + shouldCreateBlock: jest.fn(state => ({ shouldCreateBlock: state })), lmsEndpointUrl: jest.fn(state => ({ lmsEndpointUrl: state })), studioEndpointUrl: jest.fn(state => ({ studioEndpointUrl: state })), showRawEditor: jest.fn(state => ({ showRawEditor: state })), @@ -126,7 +127,8 @@ describe('TextEditor', () => { test('blockFinished from requests.isFinished', () => { expect( mapStateToProps(testState).blockFinished, - ).toEqual(selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchBlock })); + ).toEqual(selectors.app.shouldCreateBlock(testState) + || selectors.requests.isFinished(testState, { requestKey: RequestKeys.fetchBlock })); }); test('learningContextId from app.learningContextId', () => { expect( diff --git a/src/editors/containers/VideoEditor/index.tsx b/src/editors/containers/VideoEditor/index.tsx index f2ad7c670f..0b3018812d 100644 --- a/src/editors/containers/VideoEditor/index.tsx +++ b/src/editors/containers/VideoEditor/index.tsx @@ -23,6 +23,7 @@ const VideoEditor: React.FC = ({ (state) => selectors.requests.isFinished(state, { requestKey: RequestKeys.fetchStudioView }), ); const isLibrary = useSelector(selectors.app.isLibrary) as boolean; + const isCreateWorkflow = useSelector(selectors.app.shouldCreateBlock) as boolean; const { error, validateEntry, @@ -36,7 +37,7 @@ const VideoEditor: React.FC = ({ returnFunction={returnFunction} validateEntry={validateEntry} > - {studioViewFinished ? ( + {(isCreateWorkflow || studioViewFinished) ? (
diff --git a/src/editors/data/constants/problem.ts b/src/editors/data/constants/problem.ts index 0c26a45f04..71989d3d8b 100644 --- a/src/editors/data/constants/problem.ts +++ b/src/editors/data/constants/problem.ts @@ -239,3 +239,7 @@ export const ignoredOlxAttributes = [ '@_url_name', '@_x-is-pointer-node', ] as const; + +// Useful for the block creation workflow. +export const problemTitles = new Set([...Object.values(ProblemTypes).map((problem) => problem.title), + ...Object.values(AdvanceProblems).map((problem) => problem.title)]); diff --git a/src/editors/data/constants/requests.ts b/src/editors/data/constants/requests.ts index 561965af86..f9f907bedc 100644 --- a/src/editors/data/constants/requests.ts +++ b/src/editors/data/constants/requests.ts @@ -13,6 +13,7 @@ export const RequestKeys = StrictDict({ fetchImages: 'fetchImages', fetchUnit: 'fetchUnit', fetchStudioView: 'fetchStudioView', + createBlock: 'createBlock', saveBlock: 'saveBlock', uploadVideo: 'uploadVideo', allowThumbnailUpload: 'allowThumbnailUpload', @@ -25,6 +26,7 @@ export const RequestKeys = StrictDict({ checkTranscriptsForImport: 'checkTranscriptsForImport', importTranscript: 'importTranscript', uploadAsset: 'uploadAsset', + batchUploadAssets: 'batchUploadAssets', fetchAdvancedSettings: 'fetchAdvancedSettings', fetchVideoFeatures: 'fetchVideoFeatures', getHandlerUrl: 'getHandlerUrl', diff --git a/src/editors/data/redux/app/reducer.ts b/src/editors/data/redux/app/reducer.ts index d53579250c..91ce052564 100644 --- a/src/editors/data/redux/app/reducer.ts +++ b/src/editors/data/redux/app/reducer.ts @@ -54,7 +54,6 @@ const app = createSlice({ images: { ...state.images, ...payload.images }, imageCount: payload.imageCount, }), - resetImages: (state) => ({ ...state, images: {}, imageCount: 0 }), setVideos: (state, { payload }) => ({ ...state, videos: payload }), setCourseDetails: (state, { payload }) => ({ ...state, courseDetails: payload }), setShowRawEditor: (state, { payload }) => ({ diff --git a/src/editors/data/redux/app/selectors.test.ts b/src/editors/data/redux/app/selectors.test.ts index b380deca4f..aab64b080a 100644 --- a/src/editors/data/redux/app/selectors.test.ts +++ b/src/editors/data/redux/app/selectors.test.ts @@ -88,6 +88,7 @@ describe('app selectors unit tests', () => { simpleSelectors.unitUrl, simpleSelectors.blockValue, selectors.isLibrary, + selectors.shouldCreateBlock, ]); }); describe('for library blocks', () => { @@ -98,8 +99,8 @@ describe('app selectors unit tests', () => { }; [ - [[null, truthy.blockValue, true] as [any, any, any], true] as const, - [[null, null, true] as [any, any, any], false] as const, + [[null, truthy.blockValue, true, false] as [any, any, any, any], true] as const, + [[null, null, true, false] as [any, any, any, any], false] as const, ].map(([args, expected]) => expect(cb(...args)).toEqual(expected)); }); }); @@ -112,9 +113,19 @@ describe('app selectors unit tests', () => { }; [ - [[null, truthy.blockValue, false] as [any, any, any], false] as const, - [[truthy.unitUrl, null, false] as [any, any, any], false] as const, - [[truthy.unitUrl, truthy.blockValue, false] as [any, any, any], true] as const, + [[null, truthy.blockValue, false, false] as [any, any, any, any], false] as const, + [[truthy.unitUrl, null, false, false] as [any, any, any, any], false] as const, + [[truthy.unitUrl, truthy.blockValue, false, false] as [any, any, any, any], true] as const, + ].map(([args, expected]) => expect(cb(...args)).toEqual(expected)); + }); + }); + describe('component creation workflow', () => { + it('returns true if is shouldCreateBlock is truthy', () => { + const { resultFunc: cb } = selectors.isInitialized; + + [ + [[null, null, true, true] as [any, any, any, any], true] as const, + [[null, null, true, true] as [any, any, any, any], true] as const, ].map(([args, expected]) => expect(cb(...args)).toEqual(expected)); }); }); @@ -184,4 +195,9 @@ describe('app selectors unit tests', () => { }); }); }); + describe('shouldCreateBlock', () => { + it('should return false if the editor is initialized with a blockId', () => { + expect(selectors.shouldCreateBlock.resultFunc('block-v1:', 'text')).toEqual(false); + }); + }); }); diff --git a/src/editors/data/redux/app/selectors.ts b/src/editors/data/redux/app/selectors.ts index 3b7d151023..b2fd6d6ae7 100644 --- a/src/editors/data/redux/app/selectors.ts +++ b/src/editors/data/redux/app/selectors.ts @@ -1,7 +1,7 @@ import { createSelector } from 'reselect'; import type { EditorState } from '..'; import { blockTypes } from '../../constants/app'; -import { isLibraryV1Key } from '../../../../generic/key-utils'; +import { isLibraryKey, isLibraryV1Key } from '../../../../generic/key-utils'; import * as urls from '../../services/cms/urls'; export const appSelector = (state: EditorState) => state.app; @@ -47,7 +47,19 @@ export const isLibrary = createSelector( if (isLibraryV1Key(learningContextId)) { return true; } - if (blockId && blockId.startsWith('lb:')) { + if ((blockId && blockId.startsWith('lb:')) || isLibraryKey(learningContextId)) { + return true; + } + return false; + }, +); + +export const shouldCreateBlock = createSelector( + [simpleSelectors.blockId, + simpleSelectors.blockType, + ], + (blockId, blockType) => { + if (blockId === '' && blockType) { return true; } return false; @@ -59,8 +71,13 @@ export const isInitialized = createSelector( simpleSelectors.unitUrl, simpleSelectors.blockValue, isLibrary, + shouldCreateBlock, ], - (unitUrl, blockValue, isLibraryBlock) => { + (unitUrl, blockValue, isLibraryBlock, initCreateWorkflow) => { + if (initCreateWorkflow) { + return true; + } + if (isLibraryBlock) { return !!blockValue; } @@ -105,4 +122,5 @@ export default { displayTitle, analytics, isLibrary, + shouldCreateBlock, }; diff --git a/src/editors/data/redux/index.ts b/src/editors/data/redux/index.ts index 7758662965..3606f99aa2 100644 --- a/src/editors/data/redux/index.ts +++ b/src/editors/data/redux/index.ts @@ -12,7 +12,7 @@ import { AdvancedProblemType, ProblemType } from '../constants/problem'; export { default as thunkActions } from './thunkActions'; -const rootReducer = combineReducers({ +const editorReducer = combineReducers({ app: app.reducer, requests: requests.reducer, video: video.reducer, @@ -20,6 +20,14 @@ const rootReducer = combineReducers({ game: game.reducer, }); +const rootReducer = (state: any, action: any) => { + if (action.type === 'resetEditor') { + return editorReducer(undefined, action); + } + + return editorReducer(state, action); +}; + const actions = StrictDict({ app: app.actions, requests: requests.actions, diff --git a/src/editors/data/redux/requests/reducer.js b/src/editors/data/redux/requests/reducer.js index 583c390648..4383be7a65 100644 --- a/src/editors/data/redux/requests/reducer.js +++ b/src/editors/data/redux/requests/reducer.js @@ -8,6 +8,7 @@ const initialState = { [RequestKeys.fetchUnit]: { status: RequestStates.inactive }, [RequestKeys.fetchBlock]: { status: RequestStates.inactive }, [RequestKeys.fetchStudioView]: { status: RequestStates.inactive }, + [RequestKeys.createBlock]: { status: RequestStates.inactive }, [RequestKeys.saveBlock]: { status: RequestStates.inactive }, [RequestKeys.uploadAsset]: { status: RequestStates.inactive }, [RequestKeys.allowThumbnailUpload]: { status: RequestStates.inactive }, diff --git a/src/editors/data/redux/thunkActions/app.js b/src/editors/data/redux/thunkActions/app.js index 09ab9eb61b..8da9f24b71 100644 --- a/src/editors/data/redux/thunkActions/app.js +++ b/src/editors/data/redux/thunkActions/app.js @@ -1,12 +1,11 @@ import { StrictDict, camelizeKeys } from '../../../utils'; -import { isLibraryKey } from '../../../../generic/key-utils'; import * as requests from './requests'; // This 'module' self-import hack enables mocking during tests. // See src/editors/decisions/0005-internal-editor-testability-decisions.md. The whole approach to how hooks are tested // should be re-thought and cleaned up to avoid this pattern. // eslint-disable-next-line import/no-self-import import * as module from './app'; -import { actions as appActions } from '../app'; +import { actions as appActions, selectors } from '../app'; import { actions as requestsActions } from '../requests'; import { RequestKeys } from '../../constants/requests'; @@ -88,7 +87,14 @@ export const fetchCourseDetails = () => (dispatch) => { */ export const initialize = (data) => (dispatch) => { const editorType = data.blockType; + dispatch({ type: 'resetEditor' }); dispatch(actions.app.initialize(data)); + + if (data.blockId === '' && editorType) { + dispatch(actions.app.initializeEditor()); + return; + } + dispatch(module.fetchBlock()); if (data.blockId?.startsWith('block-v1:')) { dispatch(module.fetchUnit()); @@ -103,7 +109,6 @@ export const initialize = (data) => (dispatch) => { dispatch(module.fetchCourseDetails()); break; case 'html': - if (isLibraryKey(data.learningContextId)) { dispatch(actions.app.resetImages()); } dispatch(module.fetchImages({ pageNumber: 0 })); break; default: @@ -125,7 +130,54 @@ export const saveBlock = (content, returnToUnit) => (dispatch) => { })); }; -export const uploadAsset = ({ file, setSelection }) => (dispatch) => { +/** + * @param {func} onSuccess + */ +export const createBlock = (content, returnToUnit) => (dispatch, getState) => { + dispatch(requests.createBlock({ + onSuccess: (response) => { + dispatch(actions.app.setBlockId(response.id)); + const newImages = Object.values(selectors.images(getState())).map((image) => image.file); + + if (newImages.length === 0) { + dispatch(saveBlock(content, returnToUnit)); + return; + } + dispatch(requests.batchUploadAssets({ + assets: newImages, + content, + onSuccess: (updatedContent) => dispatch(saveBlock(updatedContent, returnToUnit)), + onFailure: (error) => dispatch(actions.requests.failRequest({ + requestKey: RequestKeys.batchUploadAssets, + error, + })), + })); + }, + onFailure: (error) => dispatch(actions.requests.failRequest({ + requestKey: RequestKeys.createBlock, + error, + })), + })); +}; + +export const uploadAsset = ({ file, setSelection }) => (dispatch, getState) => { + if (selectors.shouldCreateBlock(getState())) { + const tempFileURL = URL.createObjectURL(file); + const tempImage = { + displayName: file.name, + url: tempFileURL, + externalUrl: tempFileURL, + portableUrl: tempFileURL, + thumbnail: tempFileURL, + id: file.name, + locked: false, + file, + }; + setSelection(tempImage); + dispatch(appActions.setImages({ images: { [file.name]: tempImage }, imageCount: 1 })); + return; + } + dispatch(requests.uploadAsset({ asset: file, onSuccess: (response) => setSelection(camelizeKeys(response.data.asset)), @@ -142,4 +194,5 @@ export default StrictDict({ saveBlock, fetchImages, uploadAsset, + createBlock, }); diff --git a/src/editors/data/redux/thunkActions/app.test.js b/src/editors/data/redux/thunkActions/app.test.js index 5cf52f7941..35debc7f3c 100644 --- a/src/editors/data/redux/thunkActions/app.test.js +++ b/src/editors/data/redux/thunkActions/app.test.js @@ -1,14 +1,17 @@ /* eslint-disable no-import-assign */ import { actions } from '..'; import { camelizeKeys } from '../../../utils'; +import { mockImageData } from '../../constants/mockData'; import { RequestKeys } from '../../constants/requests'; import * as thunkActions from './app'; jest.mock('./requests', () => ({ fetchBlock: (args) => ({ fetchBlock: args }), fetchUnit: (args) => ({ fetchUnit: args }), + createBlock: (args) => ({ createBlock: args }), saveBlock: (args) => ({ saveBlock: args }), uploadAsset: (args) => ({ uploadAsset: args }), + batchUploadAssets: (args) => ({ batchUploadAssets: args }), fetchStudioView: (args) => ({ fetchStudioView: args }), fetchImages: (args) => ({ fetchImages: args }), fetchVideos: (args) => ({ fetchVideos: args }), @@ -30,8 +33,10 @@ const testValue = { describe('app thunkActions', () => { let dispatch; let dispatchedAction; + let getState; beforeEach(() => { dispatch = jest.fn((action) => ({ dispatch: action })); + getState = jest.fn().mockImplementation(() => ({ app: { blockId: 'blockId', images: {} } })); }); describe('fetchBlock', () => { beforeEach(() => { @@ -185,6 +190,7 @@ describe('app thunkActions', () => { thunkActions.fetchCourseDetails = () => 'fetchCourseDetails'; thunkActions.initialize(testValue)(dispatch); expect(dispatch.mock.calls).toEqual([ + [{ type: 'resetEditor' }], [actions.app.initialize(testValue)], [thunkActions.fetchBlock()], ]); @@ -196,6 +202,22 @@ describe('app thunkActions', () => { thunkActions.fetchCourseDetails = fetchCourseDetails; }); }); + describe('initialize without block id but block type defined', () => { + it('dispatches actions.app.initialize, and then fetches both block and unit', () => { + const data = { + ...testValue, + blockType: 'html', + blockId: '', + learningContextId: 'course-v1:UniversityX+PHYS+1', + }; + thunkActions.initialize(data)(dispatch); + expect(dispatch.mock.calls).toEqual([ + [{ type: 'resetEditor' }], + [actions.app.initialize(data)], + [actions.app.initializeEditor()], + ]); + }); + }); describe('initialize with block type html', () => { it('dispatches actions.app.initialize, and then fetches both block and unit', () => { const { @@ -220,6 +242,7 @@ describe('app thunkActions', () => { }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ + [{ type: 'resetEditor' }], [actions.app.initialize(data)], [thunkActions.fetchBlock()], [thunkActions.fetchUnit()], @@ -257,6 +280,7 @@ describe('app thunkActions', () => { }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ + [{ type: 'resetEditor' }], [actions.app.initialize(data)], [thunkActions.fetchBlock()], [thunkActions.fetchUnit()], @@ -294,6 +318,7 @@ describe('app thunkActions', () => { }; thunkActions.initialize(data)(dispatch); expect(dispatch.mock.calls).toEqual([ + [{ type: 'resetEditor' }], [actions.app.initialize(data)], [thunkActions.fetchBlock()], [thunkActions.fetchUnit()], @@ -333,21 +358,73 @@ describe('app thunkActions', () => { expect(returnToUnit).toHaveBeenCalled(); }); }); - describe('uploadAsset', () => { - const setSelection = jest.fn(); + describe('createBlock', () => { + let returnToUnit; beforeEach(() => { - thunkActions.uploadAsset({ file: testValue, setSelection })(dispatch); + returnToUnit = jest.fn(); + thunkActions.createBlock(testValue, returnToUnit)(dispatch, getState); [[dispatchedAction]] = dispatch.mock.calls; }); + it('dispatches createBlock', () => { + expect(dispatchedAction.createBlock).not.toBe(undefined); + }); + test('onSuccess: calls setBlockId and dispatches saveBlock', () => { + const data = { id: 'block-id' }; + dispatchedAction.createBlock.onSuccess(data); + expect(dispatch).toHaveBeenCalledWith(actions.app.setBlockId(data.id)); + expect(dispatch.mock.calls.length).toBe(3); + }); + test('onFailure: dispatches failRequest', () => { + const error = new Error('fail create a new component'); + dispatchedAction.createBlock.onFailure(error); + expect(dispatch).toHaveBeenCalledWith(actions.requests.failRequest({ + requestKey: RequestKeys.createBlock, + error, + })); + }); + test('should call batchUploadAssets if the block has images', () => { + mockImageData.map(image => ({ ...image, file: 'file' })); + getState.mockReturnValueOnce({ app: { blockId: '', images: mockImageData } }); + const data = { id: 'block-id' }; + dispatchedAction.createBlock.onSuccess(data); + expect(dispatch).toHaveBeenCalledWith(actions.app.setBlockId(data.id)); + expect(dispatch.mock.calls[2][0]).toEqual({ + [RequestKeys.batchUploadAssets]: { + assets: mockImageData.map(image => image.file), + content: { data: expect.any(Object) }, + onSuccess: expect.any(Function), + onFailure: expect.any(Function), + }, + }); + }); + }); + describe('uploadAsset', () => { + const setSelection = jest.fn(); + it('dispatches uploadAsset action', () => { + thunkActions.uploadAsset({ file: testValue, setSelection })(dispatch, getState); + [[dispatchedAction]] = dispatch.mock.calls; expect(dispatchedAction.uploadAsset).not.toBe(undefined); }); test('passes file as image prop', () => { + thunkActions.uploadAsset({ file: testValue, setSelection })(dispatch, getState); + [[dispatchedAction]] = dispatch.mock.calls; expect(dispatchedAction.uploadAsset.asset).toEqual(testValue); }); test('onSuccess: calls setSelection with camelized response.data.asset', () => { + thunkActions.uploadAsset({ file: testValue, setSelection })(dispatch, getState); + [[dispatchedAction]] = dispatch.mock.calls; dispatchedAction.uploadAsset.onSuccess({ data: { asset: testValue } }); expect(setSelection).toHaveBeenCalledWith(camelizeKeys(testValue)); }); + test('should save a new image temporary when is create workflow', () => { + getState.mockImplementation(() => ({ app: { blockId: '', blockType: 'html' } })); + global.URL.createObjectURL = jest.fn(); + thunkActions.uploadAsset({ file: testValue, setSelection })(dispatch, getState); + [[dispatchedAction]] = dispatch.mock.calls; + expect(URL.createObjectURL).toHaveBeenCalledWith(testValue); + expect(setSelection).toHaveBeenCalled(); + expect(dispatch).toHaveBeenCalledWith(actions.app.setImages({ images: expect.any(Object), imageCount: 1 })); + }); }); }); diff --git a/src/editors/data/redux/thunkActions/problem.ts b/src/editors/data/redux/thunkActions/problem.ts index 79916195e4..cd9567d407 100644 --- a/src/editors/data/redux/thunkActions/problem.ts +++ b/src/editors/data/redux/thunkActions/problem.ts @@ -84,7 +84,7 @@ export const fetchAdvancedSettings = ({ rawOLX, rawSettings }) => (dispatch) => }; export const initializeProblem = (blockValue) => (dispatch, getState) => { - const rawOLX = _.get(blockValue, 'data.data', {}); + const rawOLX = _.get(blockValue, 'data.data', ''); const rawSettings = _.get(blockValue, 'data.metadata', {}); const learningContextId = selectors.app.learningContextId(getState()); if (isLibraryKey(learningContextId)) { diff --git a/src/editors/data/redux/thunkActions/requests.js b/src/editors/data/redux/thunkActions/requests.js index 3dd4663477..335e98c60d 100644 --- a/src/editors/data/redux/thunkActions/requests.js +++ b/src/editors/data/redux/thunkActions/requests.js @@ -1,3 +1,5 @@ +import { v4 as uuid4 } from 'uuid'; + import { StrictDict, parseLibraryImageData, getLibraryImageAssets } from '../../../utils'; import { RequestKeys } from '../../constants/requests'; @@ -12,7 +14,10 @@ import { selectors as videoSelectors } from '../video'; // eslint-disable-next-line import/no-self-import import * as module from './requests'; import { isLibraryKey } from '../../../../generic/key-utils'; +import { createLibraryBlock } from '../../../../library-authoring/data/api'; import { acceptedImgKeys } from '../../../sharedComponents/ImageUploadModal/SelectImageModal/utils'; +import { blockTypes } from '../../constants/app'; +import { problemTitles } from '../../constants/problem'; // Similar to `import { actions, selectors } from '..';` but avoid circular imports: const actions = { requests: requestsActions }; @@ -123,9 +128,69 @@ export const saveBlock = ({ content, ...rest }) => (dispatch, getState) => { ...rest, })); }; + +/** + * Tracked createBlock api method. Tracked to the `createBlock` request key. + * @param {[func]} onSuccess - onSuccess method ((response) => { ... }) + * @param {[func]} onFailure - onFailure method ((error) => { ... }) + */ +export const createBlock = ({ ...rest }) => (dispatch, getState) => { + const blockTitle = selectors.app.blockTitle(getState()); + const blockType = selectors.app.blockType(getState()); + // Remove any special character, a slug should be created with unicode letters, numbers, underscores or hyphens. + const cleanTitle = blockTitle?.toLowerCase().replace(/[^a-zA-Z0-9_\s-]/g, '').trim(); + let definitionId; + // Validates if the title has been assigned by the user, if not a UUID is returned as the key. + if (!cleanTitle || (blockType === blockTypes.problem && problemTitles.has(blockTitle))) { + definitionId = `${uuid4()}`; + } else { + // add a short random suffix to prevent conflicting IDs. + const suffix = uuid4().split('-')[4]; + definitionId = `${cleanTitle.replaceAll(/\s+/g, '-')}-${suffix}`; + } + dispatch(module.networkRequest({ + requestKey: RequestKeys.createBlock, + promise: createLibraryBlock({ + libraryId: selectors.app.learningContextId(getState()), + blockType, + definitionId, + }), + ...rest, + })); +}; + +// exportend only for test +export const removeTemporalLink = (response, asset, content, resolve) => { + const imagePath = `/${response.data.asset.portableUrl}`; + const reader = new FileReader(); + reader.addEventListener('load', () => { + const imageBS64 = reader.result.toString(); + const parsedContent = typeof content === 'string' ? content.replace(imageBS64, imagePath) : { ...content, olx: content.olx.replace(imageBS64, imagePath) }; + URL.revokeObjectURL(asset); + resolve(parsedContent); + }); + reader.readAsDataURL(asset); +}; + +export const batchUploadAssets = ({ assets, content, ...rest }) => (dispatch) => { + const promises = assets.reduce((promiseChain, asset) => promiseChain + .then((parsedContent) => new Promise((resolve) => { + dispatch(module.uploadAsset({ + asset, + onSuccess: (response) => removeTemporalLink(response, asset, parsedContent, resolve), + })); + })), Promise.resolve(content)); + + dispatch(module.networkRequest({ + requestKey: RequestKeys.batchUploadAssets, + promise: promises, + ...rest, + })); +}; + export const uploadAsset = ({ asset, ...rest }) => (dispatch, getState) => { const learningContextId = selectors.app.learningContextId(getState()); - dispatch(module.networkRequest({ + return dispatch(module.networkRequest({ requestKey: RequestKeys.uploadAsset, promise: api.uploadAsset({ learningContextId, @@ -424,6 +489,7 @@ export default StrictDict({ fetchBlock, fetchStudioView, fetchUnit, + createBlock, saveBlock, fetchImages, fetchVideos, diff --git a/src/editors/data/redux/thunkActions/requests.test.js b/src/editors/data/redux/thunkActions/requests.test.js index 27f288685c..82bf74cfcc 100644 --- a/src/editors/data/redux/thunkActions/requests.test.js +++ b/src/editors/data/redux/thunkActions/requests.test.js @@ -3,6 +3,7 @@ import { RequestKeys } from '../../constants/requests'; import api from '../../services/cms/api'; import * as requests from './requests'; import { actions, selectors } from '../index'; +import { createLibraryBlock } from '../../../../library-authoring/data/api'; const testState = { some: 'data', @@ -18,7 +19,7 @@ jest.mock('../app/selectors', () => ({ blockId: (state) => ({ blockId: state }), blockType: (state) => ({ blockType: state }), learningContextId: (state) => ({ learningContextId: state }), - blockTitle: (state) => ({ title: state }), + blockTitle: (state) => state.some, isLibrary: (state) => (state.isLibrary), })); @@ -51,6 +52,10 @@ jest.mock('../../services/cms/api', () => ({ uploadVideo: (args) => args, })); +jest.mock('../../../../library-authoring/data/api', () => ({ + createLibraryBlock: ({ id, url }) => ({ id, url }), +})); + jest.mock('../../../utils', () => ({ ...jest.requireActual('../../../utils'), parseLibraryImageData: jest.fn(), @@ -360,6 +365,22 @@ describe('requests thunkActions module', () => { }, }); }); + describe('createBlock', () => { + testNetworkRequestAction({ + action: requests.createBlock, + args: { ...fetchParams }, + expectedString: 'with create promise', + expectedData: { + ...fetchParams, + requestKey: RequestKeys.createBlock, + promise: createLibraryBlock({ + libraryId: selectors.app.learningContextId(testState), + blockType: selectors.app.blockType(testState), + }), + }, + }); + }); + describe('uploadAsset', () => { const asset = 'SoME iMage CoNtent As String'; let uploadAsset; @@ -418,6 +439,79 @@ describe('requests thunkActions module', () => { }); }); + describe('batchUploadAssets', () => { + const assets = [new Blob(['file1']), new Blob(['file2'])]; + testNetworkRequestAction({ + action: requests.batchUploadAssets, + args: { ...fetchParams, assets }, + expectedString: 'with upload batch assets promise', + expectedData: { + ...fetchParams, + requestKey: RequestKeys.batchUploadAssets, + promise: assets.reduce((acc, asset) => acc.then(() => requests.uploadAsset({ + asset, + learningContextId: selectors.app.learningContextId(testState), + studioEndpointUrl: selectors.app.studioEndpointUrl(testState), + })), Promise.resolve()), + }, + }); + describe('removeTemporalLink', () => { + let response; + let asset; + let content; + + beforeEach(() => { + response = { + data: { + asset: { + portableUrl: 'image/test.jpg', + }, + }, + }; + + asset = new Blob(['image data'], { type: 'image/jpeg' }); + content = 'Content with an image: data:image/jpeg;base64,TESTBASE64'; + }); + + it('should replace the base64 image with the image path in the content', (done) => { + /* const onLoad = jest.fn(); + const readAsDataURLMock = jest.fn(() => { + this.result = 'data:image/jpeg;base64,TESTBASE64'; + onLoad(); + }); */ + class FileReaderMock { + constructor() { + this.result = ''; + this.onload = null; + } + + addEventListener(event, callback) { + if (event === 'load') { + this.onLoad = callback; + } + } + + readAsDataURL() { + this.result = 'data:image/jpeg;base64,TESTBASE64'; + this.onLoad(); + } + } + global.FileReader = FileReaderMock; + global.URL.revokeObjectURL = jest.fn(); + const resolveMock = jest.fn((parsedContent) => { + // Assert that the content has been replaced correctly + expect(parsedContent).toEqual('Content with an image: /image/test.jpg'); + done(); + }); + + requests.removeTemporalLink(response, asset, content, resolveMock); + + // expect(global.FileReader.readAsDataURLMock).toHaveBeenCalledWith(asset); + expect(global.URL.revokeObjectURL).toHaveBeenCalledWith(asset); + }); + }); + }); + describe('uploadThumbnail', () => { const thumbnail = 'SoME tHumbNAil CoNtent As String'; const videoId = 'SoME VidEOid CoNtent As String'; diff --git a/src/editors/data/redux/thunkActions/video.js b/src/editors/data/redux/thunkActions/video.js index 0ea4d84cea..c84a3913c4 100644 --- a/src/editors/data/redux/thunkActions/video.js +++ b/src/editors/data/redux/thunkActions/video.js @@ -17,8 +17,8 @@ const selectors = { app: appSelectors, video: videoSelectors }; export const loadVideoData = (selectedVideoId, selectedVideoUrl) => (dispatch, getState) => { const state = getState(); - const blockValueData = state.app.blockValue.data; - let rawVideoData = blockValueData.metadata ? blockValueData.metadata : {}; + const blockValueData = state.app?.blockValue?.data; + let rawVideoData = blockValueData?.metadata ? blockValueData.metadata : {}; const rawVideos = Object.values(selectors.app.videos(state)); if (selectedVideoId !== undefined && selectedVideoId !== null) { const selectedVideo = _.find(rawVideos, video => { diff --git a/src/editors/hooks.test.jsx b/src/editors/hooks.test.jsx index 103577a98a..d1a2f11209 100644 --- a/src/editors/hooks.test.jsx +++ b/src/editors/hooks.test.jsx @@ -25,6 +25,7 @@ jest.mock('./data/redux', () => ({ app: { initialize: (args) => ({ initializeApp: args }), saveBlock: (args) => ({ saveBlock: args }), + createBlock: (args) => ({ createBlock: args }), }, }, })); @@ -165,6 +166,45 @@ describe('hooks', () => { }); }); + describe('createBlock', () => { + const navigateCallback = (args) => ({ navigateCallback: args }); + const dispatch = jest.fn(); + const destination = 'uRLwhENsAved'; + const analytics = 'dATAonEveNT'; + + beforeEach(() => { + jest.clearAllMocks(); + jest.spyOn(hooks, hookKeys.navigateCallback).mockImplementationOnce(navigateCallback); + }); + it('returns null when content is null', () => { + const content = null; + const expected = hooks.createBlock({ + content, + destination, + analytics, + dispatch, + }); + expect(expected).toEqual(undefined); + }); + it('dispatches thunkActions.app.createBlock with navigateCallback, and passed content', () => { + const content = 'myContent'; + hooks.createBlock({ + content, + destination, + analytics, + dispatch, + }); + expect(dispatch).toHaveBeenCalledWith(thunkActions.app.createBlock( + content, + navigateCallback({ + destination, + analyticsEvent: analyticsEvt.editorSaveClick, + analytics, + }), + )); + }); + }); + describe('clearSaveError', () => { it('dispatches actions.requests.clearRequest with saveBlock requestKey', () => { const dispatch = jest.fn(); @@ -174,4 +214,14 @@ describe('hooks', () => { })); }); }); + + describe('clearCreateError', () => { + it('dispatches actions.requests.clearRequest with createBlock requestKey', () => { + const dispatch = jest.fn(); + hooks.clearCreateError({ dispatch })(); + expect(dispatch).toHaveBeenCalledWith(actions.requests.clearRequest({ + requestKey: RequestKeys.createBlock, + })); + }); + }); }); diff --git a/src/editors/hooks.ts b/src/editors/hooks.ts index c32d608008..8874675348 100644 --- a/src/editors/hooks.ts +++ b/src/editors/hooks.ts @@ -65,7 +65,30 @@ export const saveBlock = ({ )); } }; - +export const createBlock = ({ + analytics, + content, + destination, + dispatch, + returnFunction, +}) => { + if (!content) { + return; + } + dispatch(thunkActions.app.createBlock( + content, + navigateCallback({ + destination, + analyticsEvent: analyticsEvt.editorSaveClick, + analytics, + returnFunction, + }), + )); +}; export const clearSaveError = ({ dispatch, }) => () => dispatch(actions.requests.clearRequest({ requestKey: RequestKeys.saveBlock })); + +export const clearCreateError = ({ + dispatch, +}) => () => dispatch(actions.requests.clearRequest({ requestKey: RequestKeys.createBlock })); diff --git a/src/editors/sharedComponents/ImageUploadModal/index.jsx b/src/editors/sharedComponents/ImageUploadModal/index.jsx index fea13225c8..e72f4a56d0 100644 --- a/src/editors/sharedComponents/ImageUploadModal/index.jsx +++ b/src/editors/sharedComponents/ImageUploadModal/index.jsx @@ -200,7 +200,7 @@ ImageUploadModal.propTypes = { images: PropTypes.shape({}).isRequired, lmsEndpointUrl: PropTypes.string.isRequired, editorType: PropTypes.string, - isLibrary: PropTypes.string, + isLibrary: PropTypes.bool, }; export const ImageUploadModalInternal = ImageUploadModal; // For testing only diff --git a/src/library-authoring/add-content/AddContentContainer.test.tsx b/src/library-authoring/add-content/AddContentContainer.test.tsx index 1f8a785f74..63b42895ba 100644 --- a/src/library-authoring/add-content/AddContentContainer.test.tsx +++ b/src/library-authoring/add-content/AddContentContainer.test.tsx @@ -1,5 +1,4 @@ import MockAdapter from 'axios-mock-adapter/types'; -import { snakeCaseObject } from '@edx/frontend-platform'; import { fireEvent, render as baseRender, @@ -7,9 +6,10 @@ import { waitFor, initializeMocks, } from '../../testUtils'; -import { mockContentLibrary } from '../data/api.mocks'; +import { mockContentLibrary, mockXBlockFields } from '../data/api.mocks'; import { getContentLibraryApiUrl, getCreateLibraryBlockUrl, getLibraryCollectionComponentApiUrl, getLibraryPasteClipboardUrl, + getXBlockFieldsApiUrl, } from '../data/api'; import { mockBroadcastChannel, mockClipboardEmpty, mockClipboardHtml } from '../../generic/data/api.mock'; import { LibraryProvider } from '../common/context/LibraryContext'; @@ -17,8 +17,10 @@ import AddContentContainer from './AddContentContainer'; import { ComponentEditorModal } from '../components/ComponentEditorModal'; import editorCmsApi from '../../editors/data/services/cms/api'; import { ToastActionData } from '../../generic/toast-context'; +import * as textEditorHooks from '../../editors/containers/TextEditor/hooks'; mockBroadcastChannel(); +// mockCreateLibraryBlock.applyMock(); // Mocks for ComponentEditorModal to work in tests. jest.mock('frontend-components-tinymce-advanced-plugins', () => ({ a11ycheckerCss: '' })); @@ -48,6 +50,7 @@ describe('', () => { axiosMock = mocks.axiosMock; mockShowToast = mocks.mockShowToast; axiosMock.onGet(getContentLibraryApiUrl(libraryId)).reply(200, {}); + jest.spyOn(textEditorHooks, 'getContent').mockImplementation(() => () => '

Edited HTML content

'); }); afterEach(() => { jest.restoreAllMocks(); @@ -61,11 +64,11 @@ describe('', () => { expect(screen.queryByRole('button', { name: /open reponse/i })).not.toBeInTheDocument(); // Excluded from MVP expect(screen.queryByRole('button', { name: /drag drop/i })).toBeInTheDocument(); expect(screen.queryByRole('button', { name: /video/i })).toBeInTheDocument(); - expect(screen.queryByRole('button', { name: /advanced \/ other/i })).not.toBeInTheDocument(); // Excluded from MVP + expect(screen.queryByRole('button', { name: /advanced \/ other/i })).toBeInTheDocument(); // To test not editor supported blocks expect(screen.queryByRole('button', { name: /copy from clipboard/i })).not.toBeInTheDocument(); }); - it('should create a content', async () => { + it('should open the editor modal to create a content when the block is supported', async () => { mockClipboardEmpty.applyMock(); const url = getCreateLibraryBlockUrl(libraryId); axiosMock.onPost(url).reply(200); @@ -74,7 +77,16 @@ describe('', () => { const textButton = screen.getByRole('button', { name: /text/i }); fireEvent.click(textButton); + expect(await screen.findByRole('heading', { name: /Text/ })).toBeInTheDocument(); + }); + it('should create a component when the block is not supported by the editor', async () => { + mockClipboardEmpty.applyMock(); + const url = getCreateLibraryBlockUrl(libraryId); + axiosMock.onPost(url).reply(200); + render(); + const textButton = screen.getByRole('button', { name: /other/i }); + fireEvent.click(textButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(url)); await waitFor(() => expect(axiosMock.history.patch.length).toEqual(0)); }); @@ -87,13 +99,14 @@ describe('', () => { libraryId, collectionId, ); - // having id of block which is not video, html or problem will not trigger editor. + axiosMock.onPost(url).reply(200, { id: 'some-component-id' }); axiosMock.onPatch(collectionComponentUrl).reply(200); render(collectionId); - const textButton = screen.getByRole('button', { name: /text/i }); + // Select a block that is not supported by the editor should create the component + const textButton = screen.getByRole('button', { name: /other/i }); fireEvent.click(textButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(url)); @@ -105,6 +118,8 @@ describe('', () => { mockClipboardEmpty.applyMock(); const collectionId = 'some-collection-id'; const url = getCreateLibraryBlockUrl(libraryId); + const usageKey = mockXBlockFields.usageKeyNewHtml; + const updateBlockUrl = getXBlockFieldsApiUrl(usageKey); const collectionComponentUrl = getLibraryCollectionComponentApiUrl( libraryId, collectionId, @@ -113,39 +128,20 @@ describe('', () => { jest.spyOn(editorCmsApi, 'fetchCourseImages').mockImplementation(async () => ( // eslint-disable-next-line { data: { assets: [], start: 0, end: 0, page: 0, pageSize: 50, totalCount: 0 } } )); - jest.spyOn(editorCmsApi, 'fetchByUnitId').mockImplementation(async () => ({ - status: 200, - data: { - ancestors: [{ - id: 'block-v1:Org+TS100+24+type@vertical+block@parent', - display_name: 'You-Knit? The Test Unit', - category: 'vertical', - has_children: true, - }], - }, - })); - axiosMock.onPost(url).reply(200, { - id: 'lb:OpenedX:CSPROB2:html:1a5efd56-4ee5-4df0-b466-44f08fbbf567', + id: usageKey, }); - const fieldsHtml = { - displayName: 'Introduction to Testing', - data: '

This is a text component which uses HTML.

', - metadata: { displayName: 'Introduction to Testing' }, - }; - jest.spyOn(editorCmsApi, 'fetchBlockById').mockImplementationOnce(async () => ( - { status: 200, data: snakeCaseObject(fieldsHtml) } - )); - axiosMock.onPatch(collectionComponentUrl).reply(200); + axiosMock.onPost(updateBlockUrl).reply(200, mockXBlockFields.dataHtml); + axiosMock.onPatch(collectionComponentUrl).reply(200); render(collectionId); const textButton = screen.getByRole('button', { name: /text/i }); fireEvent.click(textButton); - // Component should be linked to Collection on closing editor. - const closeButton = await screen.findByRole('button', { name: 'Exit the editor' }); - fireEvent.click(closeButton); + // Component should be linked to Collection on saving the changes in the editor. + const saveButton = screen.getByLabelText('Save changes and return to learning context'); + fireEvent.click(saveButton); await waitFor(() => expect(axiosMock.history.post[0].url).toEqual(url)); await waitFor(() => expect(axiosMock.history.patch.length).toEqual(1)); await waitFor(() => expect(axiosMock.history.patch[0].url).toEqual(collectionComponentUrl)); @@ -259,20 +255,6 @@ describe('', () => { expectedError: 'There was an error pasting the content: library cannot have more than 100000 components', buttonName: /paste from clipboard/i, }, - { - label: 'should handle failure to create content', - mockUrl: getCreateLibraryBlockUrl(libraryId), - mockResponse: undefined, - expectedError: 'There was an error creating the content.', - buttonName: /text/i, - }, - { - label: 'should show detailed error in toast on create failure', - mockUrl: getCreateLibraryBlockUrl(libraryId), - mockResponse: 'library cannot have more than 100000 components', - expectedError: 'There was an error creating the content: library cannot have more than 100000 components', - buttonName: /text/i, - }, ])('$label', async ({ mockUrl, mockResponse, buttonName, expectedError, }) => { diff --git a/src/library-authoring/add-content/AddContentContainer.tsx b/src/library-authoring/add-content/AddContentContainer.tsx index 1a2e28b1a9..d47f33e7e1 100644 --- a/src/library-authoring/add-content/AddContentContainer.tsx +++ b/src/library-authoring/add-content/AddContentContainer.tsx @@ -26,8 +26,8 @@ import { useCopyToClipboard } from '../../generic/clipboard'; import { getCanEdit } from '../../course-unit/data/selectors'; import { useCreateLibraryBlock, useLibraryPasteClipboard, useAddComponentsToCollection } from '../data/apiHooks'; import { useLibraryContext } from '../common/context/LibraryContext'; -import { canEditComponent } from '../components/ComponentEditorModal'; import { PickLibraryContentModal } from './PickLibraryContentModal'; +import { blockTypes } from '../../editors/data/constants/app'; import messages from './messages'; @@ -62,7 +62,23 @@ const AddContentButton = ({ contentType, onCreateContent } : AddContentButtonPro ); }; - +export const parseErrorMsg = ( + intl, + error: any, + detailedMessage: MessageDescriptor, + defaultMessage: MessageDescriptor, +) => { + try { + const { response: { data } } = error; + const detail = data && (Array.isArray(data) ? data.join() : String(data)); + if (detail) { + return intl.formatMessage(detailedMessage, { detail }); + } + } catch (_err) { + // ignore + } + return intl.formatMessage(defaultMessage); +}; const AddContentContainer = () => { const intl = useIntl(); const { @@ -72,8 +88,8 @@ const AddContentContainer = () => { openComponentEditor, componentPicker, } = useLibraryContext(); - const createBlockMutation = useCreateLibraryBlock(); const updateComponentsMutation = useAddComponentsToCollection(libraryId, collectionId); + const createBlockMutation = useCreateLibraryBlock(); const pasteClipboardMutation = useLibraryPasteClipboard(); const { showToast } = useContext(ToastContext); const canEdit = useSelector(getCanEdit); @@ -81,23 +97,6 @@ const AddContentContainer = () => { const [isAddLibraryContentModalOpen, showAddLibraryContentModal, closeAddLibraryContentModal] = useToggle(); - const parseErrorMsg = ( - error: any, - detailedMessage: MessageDescriptor, - defaultMessage: MessageDescriptor, - ) => { - try { - const { response: { data } } = error; - const detail = data && (Array.isArray(data) ? data.join() : String(data)); - if (detail) { - return intl.formatMessage(detailedMessage, { detail }); - } - } catch (_err) { - // ignore - } - return intl.formatMessage(defaultMessage); - }; - const isBlockTypeEnabled = (blockType: string) => getConfig().LIBRARY_SUPPORTED_BLOCKS.includes(blockType); const collectionButtonData = { @@ -184,35 +183,36 @@ const AddContentContainer = () => { showToast(intl.formatMessage(messages.successPasteClipboardMessage)); }).catch((error) => { showToast(parseErrorMsg( + intl, error, messages.errorPasteClipboardMessageWithDetail, messages.errorPasteClipboardMessage, )); }); }; - const onCreateBlock = (blockType: string) => { - createBlockMutation.mutateAsync({ - libraryId, - blockType, - definitionId: `${uuid4()}`, - }).then((data) => { - const hasEditor = canEditComponent(data.id); - if (hasEditor) { - // linkComponent on editor close. - openComponentEditor(data.id, () => linkComponent(data.id)); - } else { + const suportedEditorTypes = Object.values(blockTypes); + if (suportedEditorTypes.includes(blockType)) { + // linkComponent on editor close. + openComponentEditor('', (data) => data && linkComponent(data.id), blockType); + } else { + createBlockMutation.mutateAsync({ + libraryId, + blockType, + definitionId: `${uuid4()}`, + }).then((data) => { // We can't start editing this right away so just show a toast message: showToast(intl.formatMessage(messages.successCreateMessage)); linkComponent(data.id); - } - }).catch((error) => { - showToast(parseErrorMsg( - error, - messages.errorCreateMessageWithDetail, - messages.errorCreateMessage, - )); - }); + }).catch((error) => { + showToast(parseErrorMsg( + intl, + error, + messages.errorCreateMessageWithDetail, + messages.errorCreateMessage, + )); + }); + } }; const onCreateContent = (blockType: string) => { @@ -237,7 +237,10 @@ const AddContentContainer = () => { {collectionId ? ( componentPicker && ( <> - + { ) ) : ( - - )} -
- {/* Note: for MVP we are hiding the unuspported types, not just disabling them. */} - {contentTypes.filter(ct => !ct.disabled).map((contentType) => ( - ))} + )} +
+ {/* Note: for MVP we are hiding the unuspported types, not just disabling them. */} + {contentTypes + .filter((ct) => !ct.disabled) + .map((contentType) => ( + + ))} ); }; diff --git a/src/library-authoring/add-content/AddContentWorkflow.test.tsx b/src/library-authoring/add-content/AddContentWorkflow.test.tsx index b019700a56..f12dc0d91a 100644 --- a/src/library-authoring/add-content/AddContentWorkflow.test.tsx +++ b/src/library-authoring/add-content/AddContentWorkflow.test.tsx @@ -60,20 +60,17 @@ describe('AddContentWorkflow test', () => { const newComponentButton = await screen.findByRole('button', { name: /New/ }); fireEvent.click(newComponentButton); - // Click "Text" to create a text component + // Click "Text" to open the editor in creation mode fireEvent.click(await screen.findByRole('button', { name: /Text/ })); - - // Then the editor should open - expect(await screen.findByRole('heading', { name: /New Text Component/ })).toBeInTheDocument(); + expect(await screen.findByRole('heading', { name: /Text/ })).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/ })); - + await waitFor(() => expect(screen.queryByRole('heading', { name: /Text/ })).not.toBeInTheDocument()); + expect(screen.getByRole('heading', { name: /A customized title/ })).toBeInTheDocument(); // 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. @@ -83,10 +80,12 @@ describe('AddContentWorkflow test', () => { status: 200, data: { id: mockXBlockFields.usageKeyNewHtml }, })); - // Click Save + // Click Save should create the component and then save the content const saveButton = screen.getByLabelText('Save changes and return to learning context'); fireEvent.click(saveButton); - expect(saveSpy).toHaveBeenCalledTimes(1); + await waitFor(() => { + expect(saveSpy).toHaveBeenCalledTimes(1); + }); }); it('can create a Problem component', async () => { @@ -96,10 +95,8 @@ describe('AddContentWorkflow test', () => { const newComponentButton = await screen.findByRole('button', { name: /New/ }); fireEvent.click(newComponentButton); - // Click "Problem" to create a capa problem component + // Click "Problem" to create a capa problem component in the editor fireEvent.click(await screen.findByRole('button', { name: /Problem/ })); - - // Then the editor should open expect(await screen.findByRole('heading', { name: /Select problem type/ })).toBeInTheDocument(); // Select the type: Numerical Input @@ -117,10 +114,12 @@ describe('AddContentWorkflow test', () => { status: 200, data: { id: mockXBlockFields.usageKeyNewProblem }, })); - // Click Save + // Click Save should create the component and then save the content const saveButton = screen.getByLabelText('Save changes and return to learning context'); fireEvent.click(saveButton); - expect(saveSpy).toHaveBeenCalledTimes(1); + await waitFor(() => { + expect(saveSpy).toHaveBeenCalledTimes(1); + }); }); it('can create a Video component', async () => { @@ -133,8 +132,8 @@ describe('AddContentWorkflow test', () => { // Click "Video" to create a video component fireEvent.click(await screen.findByRole('button', { name: /Video/ })); - // Then the editor should open - this is the default title of a blank video in our mock - expect(await screen.findByRole('heading', { name: /New Video/ })).toBeInTheDocument(); + // Then the editor should open + expect(await screen.findByRole('heading', { name: /Video/ })).toBeInTheDocument(); // Enter the video URL const urlInput = await screen.findByRole('textbox', { name: 'Video URL' }); @@ -146,9 +145,11 @@ describe('AddContentWorkflow test', () => { status: 200, data: { id: mockXBlockFields.usageKeyNewVideo }, })); - // Click Save + // Click Save should create the component and then save the content const saveButton = screen.getByLabelText('Save changes and return to learning context'); fireEvent.click(saveButton); - expect(saveSpy).toHaveBeenCalledTimes(1); + await waitFor(() => { + expect(saveSpy).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/src/library-authoring/common/context/LibraryContext.tsx b/src/library-authoring/common/context/LibraryContext.tsx index d1abb40783..2e5e9526e0 100644 --- a/src/library-authoring/common/context/LibraryContext.tsx +++ b/src/library-authoring/common/context/LibraryContext.tsx @@ -15,7 +15,8 @@ import { useComponentPickerContext } from './ComponentPickerContext'; export interface ComponentEditorInfo { usageKey: string; - onClose?: () => void; + blockType?:string + onClose?: (data?:any) => void; } export type LibraryContextData = { @@ -38,8 +39,8 @@ export type LibraryContextData = { /** If the editor is open and the user is editing some component, this is the component being edited. */ componentBeingEdited: ComponentEditorInfo | undefined; /** If an onClose callback is provided, it will be called when the editor is closed. */ - openComponentEditor: (usageKey: string, onClose?: () => void) => void; - closeComponentEditor: () => void; + openComponentEditor: (usageKey: string, onClose?: (data?:any) => void, blockType?:string) => void; + closeComponentEditor: (data?:any) => void; componentPicker?: typeof ComponentPicker; }; @@ -79,14 +80,14 @@ export const LibraryProvider = ({ }: LibraryProviderProps) => { const [isCreateCollectionModalOpen, openCreateCollectionModal, closeCreateCollectionModal] = useToggle(false); const [componentBeingEdited, setComponentBeingEdited] = useState(); - const closeComponentEditor = useCallback(() => { + const closeComponentEditor = useCallback((data) => { setComponentBeingEdited((prev) => { - prev?.onClose?.(); + prev?.onClose?.(data); return undefined; }); }, []); - const openComponentEditor = useCallback((usageKey: string, onClose?: () => void) => { - setComponentBeingEdited({ usageKey, onClose }); + const openComponentEditor = useCallback((usageKey: string, onClose?: () => void, blockType?:string) => { + setComponentBeingEdited({ usageKey, onClose, blockType }); }, []); const { data: libraryData, isLoading: isLoadingLibraryData } = useContentLibrary(libraryId); diff --git a/src/library-authoring/components/ComponentEditorModal.tsx b/src/library-authoring/components/ComponentEditorModal.tsx index e139a8c409..0938fa7ccb 100644 --- a/src/library-authoring/components/ComponentEditorModal.tsx +++ b/src/library-authoring/components/ComponentEditorModal.tsx @@ -25,10 +25,10 @@ export const ComponentEditorModal: React.FC> = () => { if (componentBeingEdited === undefined) { return null; } - const blockType = getBlockType(componentBeingEdited.usageKey); + const blockType = componentBeingEdited.blockType || getBlockType(componentBeingEdited.usageKey); - const onClose = () => { - closeComponentEditor(); + const onClose = (data?:any) => { + closeComponentEditor(data); invalidateComponentData(queryClient, libraryId, componentBeingEdited.usageKey); }; @@ -40,7 +40,7 @@ export const ComponentEditorModal: React.FC> = () => { studioEndpointUrl={getConfig().STUDIO_BASE_URL} lmsEndpointUrl={getConfig().LMS_BASE_URL} onClose={onClose} - returnFunction={() => { onClose(); return () => {}; }} + returnFunction={() => onClose} fullScreen={false} /> );