Cleanups for the video editor [FC-0062] (#1318)

* refactor: cleanups to video editor code

* test: ignore coverage of blank default data

Author: bradenmacdonald

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

@@ -0,0 +1,24 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`VideoEditor snapshots renders as expected with default behavior 1`] = `
+<ErrorContext.Provider
+  value="hooks.errorsHook.error"
+>
+  <EditorContainer
+    onClose={[MockFunction props.onClose]}
+    validateEntry={[MockFunction validateEntry]}
+  >
+    <div
+      className="video-editor"
+    >
+      <VideoEditorModal
+        isLibrary={
+          {
+            "useSelector": [MockFunction],
+          }
+        }
+      />
+    </div>
+  </EditorContainer>
+</ErrorContext.Provider>
+`;

src/editors/containers/VideoEditor/__snapshots__/index.test.tsx.snap

@@ -1,10 +1,12 @@
 import React from 'react';
 import { useDispatch, useSelector } from 'react-redux';
-import PropTypes from 'prop-types';
 import * as appHooks from '../../../hooks';
 import { thunkActions, selectors } from '../../../data/redux';
 import VideoSettingsModal from './VideoSettingsModal';
-// import SelectVideoModal from './SelectVideoModal';
+
+interface Props {
+  isLibrary: boolean;
+}
 
 export const {
   navigateTo,
@@ -21,9 +23,7 @@ export const hooks = {
   },
 };
 
-const VideoEditorModal = ({
-  close,
-  isOpen,
+const VideoEditorModal: React.FC<Props> = ({
   isLibrary,
 }) => {
   const dispatch = useDispatch();
@@ -34,8 +34,6 @@ const VideoEditorModal = ({
   hooks.initialize(dispatch, selectedVideoId, selectedVideoUrl);
   return (
     <VideoSettingsModal {...{
-      close,
-      isOpen,
       onReturn,
       isLibrary,
     }}
@@ -44,11 +42,4 @@ const VideoEditorModal = ({
   // TODO: add logic to show SelectVideoModal if no selection
 };
 
-VideoEditorModal.defaultProps = {
-};
-VideoEditorModal.propTypes = {
-  close: PropTypes.func.isRequired,
-  isOpen: PropTypes.bool.isRequired,
-  isLibrary: PropTypes.bool.isRequired,
-};
 export default VideoEditorModal;

src/editors/containers/VideoEditor/components/VideoEditorModal.jsx renamed to src/editors/containers/VideoEditor/components/VideoEditorModal.tsx

@@ -2,9 +2,9 @@ import 'CourseAuthoring/editors/setupEditorTest';
 import React from 'react';
 import { shallow } from '@edx/react-unit-test-utils';
 
-import * as module from './ErrorSummary';
+import * as mod from './ErrorSummary';
 
-const { ErrorSummary } = module;
+const { ErrorSummary } = mod;
 
 describe('ErrorSummary', () => {
   const errors = {
@@ -19,30 +19,30 @@ describe('ErrorSummary', () => {
       jest.spyOn(React, 'useContext').mockReturnValueOnce({});
     });
     test('snapshots: renders as expected when there are no errors', () => {
-      jest.spyOn(module, 'showAlert').mockReturnValue(false);
+      jest.spyOn(mod, 'showAlert').mockReturnValue(false);
       expect(shallow(<ErrorSummary />).snapshot).toMatchSnapshot();
     });
     test('snapshots: renders as expected when there are errors', () => {
-      jest.spyOn(module, 'showAlert').mockReturnValue(true);
+      jest.spyOn(mod, 'showAlert').mockReturnValue(true);
       expect(shallow(<ErrorSummary />).snapshot).toMatchSnapshot();
     });
   });
   describe('hasNoError', () => {
     it('returns true', () => {
-      expect(module.hasNoError(errors.widgetWithError)).toEqual(false);
+      expect(mod.hasNoError(errors.widgetWithError)).toEqual(false);
     });
     it('returns false', () => {
-      expect(module.hasNoError(errors.widgetWithNoError)).toEqual(true);
+      expect(mod.hasNoError(errors.widgetWithNoError)).toEqual(true);
     });
   });
   describe('showAlert', () => {
     it('returns true', () => {
-      jest.spyOn(module, 'hasNoError').mockReturnValue(false);
-      expect(module.showAlert(errors)).toEqual(true);
+      jest.spyOn(mod, 'hasNoError').mockReturnValue(false);
+      expect(mod.showAlert(errors)).toEqual(true);
     });
     it('returns false', () => {
-      jest.spyOn(module, 'hasNoError').mockReturnValue(true);
-      expect(module.showAlert(errors)).toEqual(false);
+      jest.spyOn(mod, 'hasNoError').mockReturnValue(true);
+      expect(mod.showAlert(errors)).toEqual(false);
     });
   });
 });

src/editors/containers/VideoEditor/components/VideoSettingsModal/ErrorSummary.test.jsx renamed to src/editors/containers/VideoEditor/components/VideoSettingsModal/ErrorSummary.test.tsx

@@ -6,22 +6,17 @@ import { InfoOutline } from '@openedx/paragon/icons';
 
 import messages from './components/messages';
 import { ErrorContext } from '../../hooks';
-// 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 './ErrorSummary';
 
 export const hasNoError = (error) => Object.keys(error[0]).length === 0;
 
-export const showAlert = (errors) => !Object.values(errors).every(module.hasNoError);
+export const showAlert = (errors) => !Object.values(errors).every(hasNoError);
 
 export const ErrorSummary = () => {
   const errors = React.useContext(ErrorContext);
   return (
     <Alert
       icon={InfoOutline}
-      show={module.showAlert(errors)}
+      show={showAlert(errors)}
       variant="danger"
     >
       <Alert.Heading>

src/editors/containers/VideoEditor/components/VideoSettingsModal/ErrorSummary.jsx renamed to src/editors/containers/VideoEditor/components/VideoSettingsModal/ErrorSummary.tsx

@@ -1,11 +1,7 @@
 import React from 'react';
-import PropTypes from 'prop-types';
 import { Button, Icon } from '@openedx/paragon';
 import { ArrowBackIos } from '@openedx/paragon/icons';
-import {
-  FormattedMessage,
-  injectIntl,
-} from '@edx/frontend-platform/i18n';
+import { FormattedMessage } from '@edx/frontend-platform/i18n';
 
 // import VideoPreview from './components/VideoPreview';
 import { ErrorSummary } from './ErrorSummary';
@@ -20,7 +16,12 @@ import './index.scss';
 import SocialShareWidget from './components/SocialShareWidget';
 import messages from '../../messages';
 
-const VideoSettingsModal = ({
+interface Props {
+  onReturn: () => void;
+  isLibrary: boolean;
+}
+
+const VideoSettingsModal: React.FC<Props> = ({
   onReturn,
   isLibrary,
 }) => (
@@ -54,9 +55,4 @@ const VideoSettingsModal = ({
   </>
 );
 
-VideoSettingsModal.propTypes = {
-  onReturn: PropTypes.func.isRequired,
-  isLibrary: PropTypes.func.isRequired,
-};
-
-export default injectIntl(VideoSettingsModal);
+export default VideoSettingsModal;

src/editors/containers/VideoEditor/components/VideoSettingsModal/__snapshots__/ErrorSummary.test.jsx.snap renamed to src/editors/containers/VideoEditor/components/VideoSettingsModal/__snapshots__/ErrorSummary.test.tsx.snap

@@ -1,12 +1,7 @@
-import { dispatch } from 'react-redux';
 import { thunkActions } from '../../data/redux';
 import { MockUseState } from '../../testUtils';
 
-// 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 './hooks';
+import * as hooks from './hooks';
 
 jest.mock('react', () => ({
   ...jest.requireActual('react'),
@@ -15,14 +10,12 @@ jest.mock('react', () => ({
   useCallback: (cb, prereqs) => ({ cb, prereqs }),
 }));
 
-jest.mock('react-redux', () => {
-  const dispatchFn = jest.fn();
-  return {
-    ...jest.requireActual('react-redux'),
-    dispatch: dispatchFn,
-    useDispatch: jest.fn(() => dispatchFn),
-  };
-});
+const mockDispatchFn = jest.fn();
+jest.mock('react-redux', () => ({
+  ...jest.requireActual('react-redux'),
+  dispatch: mockDispatchFn,
+  useDispatch: jest.fn(() => mockDispatchFn),
+}));
 
 jest.mock('../../data/redux', () => ({
   thunkActions: {
@@ -32,7 +25,7 @@ jest.mock('../../data/redux', () => ({
   },
 }));
 
-const state = new MockUseState(module);
+const state = new MockUseState(hooks);
 
 let hook;
 
@@ -62,47 +55,47 @@ describe('VideoEditorHooks', () => {
       field2: 'field2msg',
     };
     test('error: state values', () => {
-      expect(module.errorsHook().error.duration).toEqual([
+      expect(hooks.errorsHook().error.duration).toEqual([
         state.stateVals[state.keys.durationErrors],
         state.setState[state.keys.durationErrors],
       ]);
-      expect(module.errorsHook().error.handout).toEqual([
+      expect(hooks.errorsHook().error.handout).toEqual([
         state.stateVals[state.keys.handoutErrors],
         state.setState[state.keys.handoutErrors],
       ]);
-      expect(module.errorsHook().error.license).toEqual([
+      expect(hooks.errorsHook().error.license).toEqual([
         state.stateVals[state.keys.licenseErrors],
         state.setState[state.keys.licenseErrors],
       ]);
-      expect(module.errorsHook().error.thumbnail).toEqual([
+      expect(hooks.errorsHook().error.thumbnail).toEqual([
         state.stateVals[state.keys.thumbnailErrors],
         state.setState[state.keys.thumbnailErrors],
       ]);
-      expect(module.errorsHook().error.transcripts).toEqual([
+      expect(hooks.errorsHook().error.transcripts).toEqual([
         state.stateVals[state.keys.transcriptsErrors],
         state.setState[state.keys.transcriptsErrors],
       ]);
-      expect(module.errorsHook().error.videoSource).toEqual([
+      expect(hooks.errorsHook().error.videoSource).toEqual([
         state.stateVals[state.keys.videoSourceErrors],
         state.setState[state.keys.videoSourceErrors],
       ]);
     });
     describe('validateEntry', () => {
       test('validateEntry: returns true if all validation calls are true', () => {
-        hook = module.errorsHook();
+        hook = hooks.errorsHook();
         expect(hook.validateEntry()).toEqual(true);
       });
       test('validateEntry: returns false if any validation calls are false', () => {
         state.mockVal(state.keys.durationErrors, fakeDurationError);
-        hook = module.errorsHook();
+        hook = hooks.errorsHook();
         expect(hook.validateEntry()).toEqual(false);
       });
     });
   });
   describe('fetchVideoContent', () => {
     it('equals dispatch(thunkActions.video.saveVideoData())', () => {
-      hook = module.fetchVideoContent()({ dispatch });
-      expect(hook).toEqual(dispatch(thunkActions.video.saveVideoData()));
+      hook = hooks.fetchVideoContent()({ dispatch: mockDispatchFn });
+      expect(hook).toEqual(mockDispatchFn(thunkActions.video.saveVideoData()));
     });
   });
 });