feat: Enable capa problem editor for components in libraries (#1290)

* feat: enable the problem editor for library components

* fix: don't try to load "advanced settings" when editing problem in library

* fix: don't fetch images when editing problem in library

* docs: add a note about plans for the editor modal

* fix: choosing a problem type then cancelling resulted in an error

* chore: remove unused mockApi, clean up problematic 'module' self import

* test: update workflow test to test problem editor

* feat: show capa content summary on cards in library search results

* docs: fix comment typos found in code review

* refactor: add 'key-utils' to consolidate opaque key logic

Author: bradenmacdonald

src/editors/EditorContainer.tsx

@@ -7,32 +7,29 @@ import EditorPage from './EditorPage';
 interface Props {
   /** Course ID or Library ID */
   learningContextId: string;
-  /** Event handler for when user cancels out of the editor page */
+  /** Event handler sometimes called 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;
+  /**
+   * Event handler called after when user saves their changes using an editor
+   * and sometimes called when user cancels the editor, instead of onClose.
+   * If changes are saved, newData will be present, and if it was cancellation,
+   * newData will be undefined.
+   * TODO: clean this up so there are separate onCancel and onSave callbacks,
+   * and they are used consistently instead of this mess.
+   */
+  returnFunction?: () => (newData: Record<string, any> | undefined) => void;
 }
 
 const EditorContainer: React.FC<Props> = ({
   learningContextId,
   onClose,
-  afterSave,
+  returnFunction,
 }) => {
   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
@@ -42,7 +39,7 @@ const EditorContainer: React.FC<Props> = ({
         studioEndpointUrl={getConfig().STUDIO_BASE_URL}
         lmsEndpointUrl={getConfig().LMS_BASE_URL}
         onClose={onClose}
-        returnFunction={afterSave}
+        returnFunction={returnFunction}
       />
     </div>
   );

src/editors/data/constants/requests.js renamed to src/editors/data/constants/requests.ts

@@ -5,7 +5,7 @@ export const RequestStates = StrictDict({
   pending: 'pending',
   completed: 'completed',
   failed: 'failed',
-});
+} as const);
 
 export const RequestKeys = StrictDict({
   fetchVideos: 'fetchVideos',
@@ -27,4 +27,4 @@ export const RequestKeys = StrictDict({
   uploadAsset: 'uploadAsset',
   fetchAdvancedSettings: 'fetchAdvancedSettings',
   fetchVideoFeatures: 'fetchVideoFeatures',
-});
+} as const);

src/editors/data/redux/app/selectors.js

@@ -1,5 +1,6 @@
 import { createSelector } from 'reselect';
 import { blockTypes } from '../../constants/app';
+import { isLibraryV1Key } from '../../../../generic/key-utils';
 import * as urls from '../../services/cms/urls';
 // 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
@@ -46,7 +47,7 @@ export const isLibrary = createSelector(
     module.simpleSelectors.blockId,
   ],
   (learningContextId, blockId) => {
-    if (learningContextId && learningContextId.startsWith('library-v1')) {
+    if (isLibraryV1Key(learningContextId)) {
       return true;
     }
     if (blockId && blockId.startsWith('lb:')) {

src/editors/data/redux/thunkActions/app.js

@@ -1,4 +1,5 @@
 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
@@ -102,7 +103,7 @@ export const initialize = (data) => (dispatch) => {
       dispatch(module.fetchCourseDetails());
       break;
     case 'html':
-      if (data.learningContextId?.startsWith('lib:')) {
+      if (isLibraryKey(data.learningContextId)) {
         // eslint-disable-next-line no-console
         console.log('Not fetching image assets - not implemented yet for content libraries.');
       } else {

src/editors/data/redux/thunkActions/problem.test.js renamed to src/editors/data/redux/thunkActions/problem.test.ts

@@ -45,14 +45,17 @@ describe('problem thunkActions', () => {
     getState = jest.fn(() => ({
       problem: {
       },
+      app: {
+        learningContextId: 'course-v1:org+course+run',
+      },
     }));
   });
 
   afterEach(() => {
     jest.restoreAllMocks();
   });
   test('initializeProblem visual Problem :', () => {
-    initializeProblem(blockValue)(dispatch);
+    initializeProblem(blockValue)(dispatch, getState);
     expect(dispatch).toHaveBeenCalled();
   });
   test('switchToAdvancedEditor visual Problem', () => {

src/editors/data/redux/thunkActions/problem.js renamed to src/editors/data/redux/thunkActions/problem.ts

@@ -1,16 +1,20 @@
 import _ from 'lodash';
 import { actions as problemActions } from '../problem';
+import { actions as requestActions } from '../requests';
+import { selectors as appSelectors } from '../app';
 import * as requests from './requests';
+import { isLibraryKey } from '../../../../generic/key-utils';
 import { OLXParser } from '../../../containers/ProblemEditor/data/OLXParser';
 import { parseSettings } from '../../../containers/ProblemEditor/data/SettingsParser';
 import { ProblemTypeKeys } from '../../constants/problem';
 import ReactStateOLXParser from '../../../containers/ProblemEditor/data/ReactStateOLXParser';
-import { blankProblemOLX } from '../../../containers/ProblemEditor/data/mockData/olxTestData';
 import { camelizeKeys } from '../../../utils';
 import { fetchEditorContent } from '../../../containers/ProblemEditor/components/EditProblemView/hooks';
+import { RequestKeys } from '../../constants/requests';
 
 // Similar to `import { actions, selectors } from '..';` but avoid circular imports:
-const actions = { problem: problemActions };
+const actions = { problem: problemActions, requests: requestActions };
+const selectors = { app: appSelectors };
 
 export const switchToAdvancedEditor = () => (dispatch, getState) => {
   const state = getState();
@@ -21,7 +25,7 @@ export const switchToAdvancedEditor = () => (dispatch, getState) => {
 };
 
 export const isBlankProblem = ({ rawOLX }) => {
-  if (rawOLX.replace(/\s/g, '') === blankProblemOLX.rawOLX) {
+  if (['<problem></problem>', '<problem/>'].includes(rawOLX.replace(/\s/g, ''))) {
     return true;
   }
   return false;
@@ -67,7 +71,7 @@ export const fetchAdvancedSettings = ({ rawOLX, rawSettings }) => (dispatch) =>
   dispatch(requests.fetchAdvancedSettings({
     onSuccess: (response) => {
       const defaultSettings = {};
-      Object.entries(response.data).forEach(([key, value]) => {
+      Object.entries(response.data as Record<string, any>).forEach(([key, value]) => {
         if (advancedProblemSettingKeys.includes(key)) {
           defaultSettings[key] = value.value;
         }
@@ -79,10 +83,20 @@ export const fetchAdvancedSettings = ({ rawOLX, rawSettings }) => (dispatch) =>
   }));
 };
 
-export const initializeProblem = (blockValue) => (dispatch) => {
+export const initializeProblem = (blockValue) => (dispatch, getState) => {
   const rawOLX = _.get(blockValue, 'data.data', {});
   const rawSettings = _.get(blockValue, 'data.metadata', {});
-  dispatch(fetchAdvancedSettings({ rawOLX, rawSettings }));
+  const learningContextId = selectors.app.learningContextId(getState());
+  if (isLibraryKey(learningContextId)) {
+    // Content libraries don't yet support defaults for fields like max_attempts, showanswer, etc.
+    // So proceed with loading the problem.
+    // Though first we need to fake the request or else the problem type selection UI won't display:
+    dispatch(actions.requests.completeRequest({ requestKey: RequestKeys.fetchAdvancedSettings, response: {} }));
+    dispatch(loadProblem({ rawOLX, rawSettings, defaultSettings: {} }));
+  } else {
+    // Load the defaults (for max_attempts, etc.) from the course's advanced settings, then proceed:
+    dispatch(fetchAdvancedSettings({ rawOLX, rawSettings }));
+  }
 };
 
 export default { initializeProblem, switchToAdvancedEditor, fetchAdvancedSettings };

src/editors/data/services/cms/api.test.js renamed to src/editors/data/services/cms/api.test.ts

@@ -1,19 +1,7 @@
-/* eslint-disable no-import-assign */
-import * as utils from '../../../utils';
 import * as api from './api';
-import * as mockApi from './mockApi';
 import * as urls from './urls';
 import { get, post, deleteObject } from './utils';
 
-jest.mock('../../../utils', () => {
-  const camelizeMap = (obj) => ({ ...obj, camelized: true });
-  return {
-    ...jest.requireActual('../../../utils'),
-    camelize: camelizeMap,
-    camelizeKeys: (list) => list.map(camelizeMap),
-  };
-});
-
 jest.mock('./urls', () => ({
   block: jest.fn().mockReturnValue('urls.block'),
   blockAncestor: jest.fn().mockReturnValue('urls.blockAncestor'),
@@ -40,8 +28,6 @@ jest.mock('./utils', () => ({
   deleteObject: jest.fn().mockName('deleteObject'),
 }));
 
-const { camelize } = utils;
-
 const { apiMethods } = api;
 
 const blockId = 'block-v1-coursev1:2uX@4345432';
@@ -200,7 +186,7 @@ describe('cms api', () => {
           licenseType: 'LiCeNsETYpe',
           licenseDetails: 'liCENSeDetAIls',
         };
-        const html5Sources = 'hTML5souRCES';
+        const html5Sources = ['hTML5souRCES'];
         const edxVideoId = 'eDXviDEOid';
         const youtubeId = 'yOUtUBeid';
         const license = 'LiCEnsE';
@@ -241,6 +227,7 @@ describe('cms api', () => {
         jest.restoreAllMocks();
       });
       test('throw error for invalid blockType', () => {
+        // @ts-expect-error because we're not passing 'blockId' or other parameters
         expect(() => { apiMethods.normalizeContent({ blockType: 'somethingINVALID' }); })
           .toThrow(TypeError);
       });
@@ -271,7 +258,7 @@ describe('cms api', () => {
     });
 
     describe('uploadAsset', () => {
-      const asset = { photo: 'dAta' };
+      const asset = new Blob(['data'], { type: 'image/jpeg' });
       it('should call post with urls.courseAssets and imgdata', () => {
         const mockFormdata = new FormData();
         mockFormdata.append('file', asset);
@@ -318,18 +305,17 @@ describe('cms api', () => {
         { id: ids[0], some: 'data' },
         { id: ids[1], other: 'data' },
         { id: ids[2], some: 'DATA' },
-        { id: ids[3], other: 'DATA' },
+        { id: ids[3], other_data: 'DATA' },
       ];
-      const oldLoadImage = api.loadImage;
-      api.loadImage = (imageData) => ({ loadImage: imageData });
+      const spy = jest.spyOn(api, 'loadImage').mockImplementation((imageData) => ({ loadImage: imageData }));
       const out = api.loadImages(testData);
       expect(out).toEqual({
-        [ids[0]]: api.loadImage(camelize(testData[0])),
-        [ids[1]]: api.loadImage(camelize(testData[1])),
-        [ids[2]]: api.loadImage(camelize(testData[2])),
-        [ids[3]]: api.loadImage(camelize(testData[3])),
+        [ids[0]]: api.loadImage(testData[0]),
+        [ids[1]]: api.loadImage(testData[1]),
+        [ids[2]]: api.loadImage(testData[2]),
+        [ids[3]]: api.loadImage({ id: ids[3], otherData: 'DATA' }), // Verify its 'other_data' key was camelized
       });
-      api.loadImage = oldLoadImage;
+      spy.mockClear();
     });
   });
   describe('uploadThumbnail', () => {
@@ -382,7 +368,7 @@ describe('cms api', () => {
       });
     });
     describe('uploadTranscript', () => {
-      const transcript = { transcript: 'dAta' };
+      const transcript = new Blob(['dAta']);
       it('should call post with urls.videoTranscripts and transcript data', () => {
         const mockFormdata = new FormData();
         mockFormdata.append('file', transcript);
@@ -606,31 +592,6 @@ describe('cms api', () => {
       expect(api.processLicense(licenseType, licenseDetails)).toEqual('all-rights-reserved');
     });
   });
-  describe('checkMockApi', () => {
-    const envTemp = process.env;
-    beforeEach(() => {
-      jest.resetModules();
-      process.env = { ...envTemp };
-    });
-    afterEach(() => {
-      process.env = envTemp;
-    });
-    describe('if REACT_APP_DEVGALLERY is true', () => {
-      it('should return the mockApi version of a call when it exists', () => {
-        process.env.REACT_APP_DEVGALLERY = true;
-        expect(api.checkMockApi('fetchBlockById')).toEqual(mockApi.fetchBlockById);
-      });
-      it('should return an empty mock when the call does not exist', () => {
-        process.env.REACT_APP_DEVGALLERY = true;
-        expect(api.checkMockApi('someRAnDomThINg')).toEqual(mockApi.emptyMock);
-      });
-    });
-    describe('if REACT_APP_DEVGALLERY is not true', () => {
-      it('should return the appropriate call', () => {
-        expect(api.checkMockApi('fetchBlockById')).toEqual(apiMethods.fetchBlockById);
-      });
-    });
-  });
   describe('fetchVideoFeatures', () => {
     it('should call get with url.videoFeatures', () => {
       const args = { studioEndpointUrl };