feat: edit Text components within content libraries [FC-0062] (#1240)

Author: bradenmacdonald

src/CourseAuthoringRoutes.jsx

@@ -88,7 +88,7 @@ const CourseAuthoringRoutes = () => {
         />
         <Route
           path="editor/:blockType/:blockId?"
-          element={<PageWrap><EditorContainer courseId={courseId} /></PageWrap>}
+          element={<PageWrap><EditorContainer learningContextId={courseId} /></PageWrap>}
         />
         <Route
           path="settings/details"

src/editors/EditorContainer.jsx

@@ -10,7 +10,7 @@ jest.mock('react-router', () => ({
   }),
 }));
 
-const props = { courseId: 'cOuRsEId' };
+const props = { learningContextId: 'cOuRsEId' };
 
 describe('Editor Container', () => {
   describe('snapshots', () => {

src/editors/EditorContainer.test.jsx

@@ -0,0 +1,51 @@
+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 called after when user saves their changes using an editor */
+  afterSave?: () => (newData: Record<string, any>) => void;
+}
+
+const EditorContainer: React.FC<Props> = ({
+  learningContextId,
+  onClose,
+  afterSave,
+}) => {
+  const { blockType, blockId } = useParams();
+  if (blockType === undefined || blockId === undefined) {
+    // istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker.
+    return <div>Error: missing URL parameters</div>;
+  }
+  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 (
+    <div className="editor-page">
+      <EditorPage
+        courseId={learningContextId}
+        blockType={blockType}
+        blockId={blockId}
+        studioEndpointUrl={getConfig().STUDIO_BASE_URL}
+        lmsEndpointUrl={getConfig().LMS_BASE_URL}
+        onClose={onClose}
+        returnFunction={afterSave}
+      />
+    </div>
+  );
+};
+
+export default EditorContainer;

src/editors/EditorContainer.tsx

@@ -51,6 +51,7 @@ exports[`EditorContainer component render snapshot: initialized. enable save and
         />
       </h2>
       <IconButton
+        alt="Exit the editor"
         iconAs="Icon"
         onClick={[MockFunction openCancelConfirmModal]}
         src={[MockFunction icons.Close]}
@@ -132,6 +133,7 @@ exports[`EditorContainer component render snapshot: not initialized. disable sav
         />
       </h2>
       <IconButton
+        alt="Exit the editor"
         iconAs="Icon"
         onClick={[MockFunction openCancelConfirmModal]}
         src={[MockFunction icons.Close]}

src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap

@@ -63,6 +63,7 @@ const EditorContainer = ({
             src={Close}
             iconAs={Icon}
             onClick={openCancelConfirmModal}
+            alt={intl.formatMessage(messages.exitButtonAlt)}
           />
         </div>
       </ModalDialog.Header>

src/editors/containers/EditorContainer/index.jsx

@@ -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',

src/editors/containers/EditorContainer/messages.js

@@ -24,11 +24,7 @@ exports[`TextEditor snapshots block failed to load, Toast is shown 1`] = `
       onClose={[MockFunction hooks.nullMethod]}
       show={true}
     >
-      <FormattedMessage
-        defaultMessage="Error: Could Not Load Text Content"
-        description="Error Message Dispayed When HTML content fails to Load"
-        id="authoring.texteditor.load.error"
-      />
+      Error: Could Not Load Text Content
     </Toast>
     <TinyMceWidget
       disabled={false}
@@ -81,11 +77,7 @@ exports[`TextEditor snapshots loaded, raw editor 1`] = `
       onClose={[MockFunction hooks.nullMethod]}
       show={false}
     >
-      <FormattedMessage
-        defaultMessage="Error: Could Not Load Text Content"
-        description="Error Message Dispayed When HTML content fails to Load"
-        id="authoring.texteditor.load.error"
-      />
+      Error: Could Not Load Text Content
     </Toast>
     <RawEditor
       content={
@@ -132,11 +124,7 @@ exports[`TextEditor snapshots not yet loaded, Spinner appears 1`] = `
       onClose={[MockFunction hooks.nullMethod]}
       show={false}
     >
-      <FormattedMessage
-        defaultMessage="Error: Could Not Load Text Content"
-        description="Error Message Dispayed When HTML content fails to Load"
-        id="authoring.texteditor.load.error"
-      />
+      Error: Could Not Load Text Content
     </Toast>
     <div
       className="text-center p-6"
@@ -175,11 +163,7 @@ exports[`TextEditor snapshots renders as expected with default behavior 1`] = `
       onClose={[MockFunction hooks.nullMethod]}
       show={false}
     >
-      <FormattedMessage
-        defaultMessage="Error: Could Not Load Text Content"
-        description="Error Message Dispayed When HTML content fails to Load"
-        id="authoring.texteditor.load.error"
-      />
+      Error: Could Not Load Text Content
     </Toast>
     <TinyMceWidget
       disabled={false}
@@ -232,11 +216,7 @@ exports[`TextEditor snapshots renders static images with relative paths 1`] = `
       onClose={[MockFunction hooks.nullMethod]}
       show={false}
     >
-      <FormattedMessage
-        defaultMessage="Error: Could Not Load Text Content"
-        description="Error Message Dispayed When HTML content fails to Load"
-        id="authoring.texteditor.load.error"
-      />
+      Error: Could Not Load Text Content
     </Toast>
     <TinyMceWidget
       disabled={false}

src/editors/containers/TextEditor/__snapshots__/index.test.jsx.snap

@@ -6,7 +6,7 @@ import {
   Spinner,
   Toast,
 } from '@openedx/paragon';
-import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n';
+import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
 
 import { actions, selectors } from '../../data/redux';
 import { RequestKeys } from '../../data/constants/requests';
@@ -78,7 +78,7 @@ const TextEditor = ({
     >
       <div className="editor-body h-75 overflow-auto">
         <Toast show={blockFailed} onClose={hooks.nullMethod}>
-          <FormattedMessage {...messages.couldNotLoadTextContext} />
+          { intl.formatMessage(messages.couldNotLoadTextContext) }
         </Toast>
 
         {(!blockFinished)
@@ -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

src/editors/containers/TextEditor/index.jsx

@@ -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(

src/editors/data/redux/app/selectors.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));
     });
   });

src/editors/data/redux/app/selectors.test.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;

src/editors/data/redux/thunkActions/app.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([

src/editors/data/redux/thunkActions/app.test.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 }) => (

src/editors/data/services/cms/urls.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', () => {