Miscellaneous small refactorings (#74)

* Normalizing “courseInfo” back into “course”

Splitting it out denormalizes the data and introduces potential data inconsistencies.

* Name component JSX files with the name of the component.

* Normalizing some module exports/naming.

* Moving alerts into a sub-directory.

* DRYing up alert hook creation into reusable useAlert hook.

* Adding some comments about ‘courses’ hydration.

src/access-expiration-alert/hooks.js

@@ -1,28 +0,0 @@
-/* eslint-disable import/prefer-default-export */
-import { useContext, useState, useEffect } from 'react';
-import { UserMessagesContext } from '../user-messages';
-import { useModel } from '../model-store';
-
-export function useAccessExpirationAlert(courseId) {
-  const course = useModel('courses', courseId);
-  const { add, remove } = useContext(UserMessagesContext);
-  const [alertId, setAlertId] = useState(null);
-  const rawHtml = (course && course.courseExpiredMessage) || null;
-  useEffect(() => {
-    if (rawHtml && alertId === null) {
-      setAlertId(add({
-        code: 'clientAccessExpirationAlert',
-        topic: 'course',
-        rawHtml,
-      }));
-    } else if (!rawHtml && alertId !== null) {
-      remove(alertId);
-      setAlertId(null);
-    }
-    return () => {
-      if (alertId !== null) {
-        remove(alertId);
-      }
-    };
-  }, [alertId, courseId, rawHtml]);
-}

src/alerts/access-expiration-alert/AccessExpirationAlert.jsx

@@ -1,12 +1,12 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 
-import { Alert } from '../user-messages';
+import { Alert } from '../../user-messages';
 
-function AccessExpirationAlert(props) {
+function AccessExpirationAlert({ payload }) {
   const {
     rawHtml,
-  } = props;
+  } = payload;
   return rawHtml && (
     <Alert type="info">
       <div dangerouslySetInnerHTML={{ __html: rawHtml }} />
@@ -15,7 +15,9 @@ function AccessExpirationAlert(props) {
 }
 
 AccessExpirationAlert.propTypes = {
-  rawHtml: PropTypes.string.isRequired,
+  payload: PropTypes.shape({
+    rawHtml: PropTypes.string.isRequired,
+  }).isRequired,
 };
 
 export default AccessExpirationAlert;

src/alerts/access-expiration-alert/hooks.js

@@ -0,0 +1,15 @@
+/* eslint-disable import/prefer-default-export */
+import { useModel } from '../../model-store';
+import { useAlert } from '../../user-messages';
+
+export function useAccessExpirationAlert(courseId) {
+  const course = useModel('courses', courseId);
+  const rawHtml = (course && course.courseExpiredMessage) || null;
+  const isVisible = !!rawHtml; // If it exists, show it.
+
+  useAlert(isVisible, {
+    code: 'clientAccessExpirationAlert',
+    topic: 'course',
+    payload: { rawHtml },
+  });
+}

src/alerts/enrollment-alert/EnrollmentAlert.jsx

@@ -5,7 +5,7 @@ import { Button } from '@edx/paragon';
 
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import { faSpinner } from '@fortawesome/free-solid-svg-icons';
-import { Alert } from '../user-messages';
+import { Alert } from '../../user-messages';
 
 import messages from './messages';
 import { useEnrollClickHandler } from './hooks';

src/alerts/enrollment-alert/StaffEnrollmentAlert.jsx

@@ -5,7 +5,7 @@ import { Button } from '@edx/paragon';
 
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import { faSpinner } from '@fortawesome/free-solid-svg-icons';
-import { Alert } from '../user-messages';
+import { Alert } from '../../user-messages';
 
 import messages from './messages';
 import { useEnrollClickHandler } from './hooks';

src/alerts/enrollment-alert/hooks.js

@@ -0,0 +1,40 @@
+/* eslint-disable import/prefer-default-export */
+import {
+  useContext, useState, useCallback,
+} from 'react';
+import { UserMessagesContext, ALERT_TYPES, useAlert } from '../../user-messages';
+import { useModel } from '../../model-store';
+import { postCourseEnrollment } from './data/api';
+
+
+export function useEnrollmentAlert(courseId) {
+  const course = useModel('courses', courseId);
+  const code = course.isStaff ? 'clientStaffEnrollmentAlert' : 'clientEnrollmentAlert';
+  const isVisible = course && course.isEnrolled !== undefined && !course.isEnrolled;
+
+  useAlert(isVisible, {
+    code,
+    topic: 'course',
+  });
+}
+
+export function useEnrollClickHandler(courseId, successText) {
+  const [loading, setLoading] = useState(false);
+  const { addFlash } = useContext(UserMessagesContext);
+  const enrollClickHandler = useCallback(() => {
+    setLoading(true);
+    postCourseEnrollment(courseId).then(() => {
+      addFlash({
+        dismissible: true,
+        flash: true,
+        text: successText,
+        type: ALERT_TYPES.SUCCESS,
+        topic: 'course',
+      });
+      setLoading(false);
+      global.location.reload();
+    });
+  }, [courseId]);
+
+  return { enrollClickHandler, loading };
+}

src/alerts/logistration-alert/LogistrationAlert.jsx

@@ -3,7 +3,7 @@ import { getConfig } from '@edx/frontend-platform';
 import { injectIntl, intlShape, FormattedMessage } from '@edx/frontend-platform/i18n';
 import { getLoginRedirectUrl } from '@edx/frontend-platform/auth';
 
-import { Alert } from '../user-messages';
+import { Alert } from '../../user-messages';
 import messages from './messages';
 
 function LogistrationAlert({ intl }) {

src/alerts/logistration-alert/hooks.js

@@ -0,0 +1,17 @@
+/* eslint-disable import/prefer-default-export */
+import { useContext } from 'react';
+import { AppContext } from '@edx/frontend-platform/react';
+import { ALERT_TYPES, useAlert } from '../../user-messages';
+
+
+export function useLogistrationAlert() {
+  const { authenticatedUser } = useContext(AppContext);
+  const isVisible = authenticatedUser === null;
+
+  useAlert(isVisible, {
+    code: 'clientLogistrationAlert',
+    topic: 'course',
+    dismissible: false,
+    type: ALERT_TYPES.ERROR,
+  });
+}

src/alerts/offer-alert/OfferAlert.jsx

@@ -1,12 +1,12 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 
-import { Alert } from '../user-messages';
+import { Alert } from '../../user-messages';
 
-function OfferAlert(props) {
+function OfferAlert({ payload }) {
   const {
     rawHtml,
-  } = props;
+  } = payload;
   return rawHtml && (
     <Alert type="info">
       <div dangerouslySetInnerHTML={{ __html: rawHtml }} />
@@ -15,7 +15,9 @@ function OfferAlert(props) {
 }
 
 OfferAlert.propTypes = {
-  rawHtml: PropTypes.string.isRequired,
+  payload: PropTypes.shape({
+    rawHtml: PropTypes.string.isRequired,
+  }).isRequired,
 };
 
 export default OfferAlert;

src/alerts/offer-alert/hooks.js

@@ -0,0 +1,15 @@
+/* eslint-disable import/prefer-default-export */
+import { useModel } from '../../model-store';
+import { useAlert } from '../../user-messages';
+
+export function useOfferAlert(courseId) {
+  const course = useModel('courses', courseId);
+  const rawHtml = (course && course.offerHtml) || null;
+  const isVisible = !!rawHtml; // if it exists, show it.
+
+  useAlert(isVisible, {
+    code: 'clientOfferAlert',
+    topic: 'course',
+    payload: { rawHtml },
+  });
+}

src/course-home/CourseHome.jsx

@@ -13,8 +13,8 @@ import { useModel } from '../model-store';
 // This is because React.lazy() requires that we import() from a file with a Component as its
 // default export.
 // See React.lazy docs here: https://reactjs.org/docs/code-splitting.html#reactlazy
-const { EnrollmentAlert, StaffEnrollmentAlert } = React.lazy(() => import('../enrollment-alert'));
-const LogistrationAlert = React.lazy(() => import('../logistration-alert'));
+const { EnrollmentAlert, StaffEnrollmentAlert } = React.lazy(() => import('../alerts/enrollment-alert'));
+const LogistrationAlert = React.lazy(() => import('../alerts/logistration-alert'));
 
 export default function CourseHome() {
   const {

src/courseware/course/Course.jsx

@@ -2,25 +2,25 @@ import React from 'react';
 import PropTypes from 'prop-types';
 
 import { AlertList } from '../../user-messages';
-import { useAccessExpirationAlert } from '../../access-expiration-alert';
-import { useOfferAlert } from '../../offer-alert';
+import { useAccessExpirationAlert } from '../../alerts/access-expiration-alert';
+import { useOfferAlert } from '../../alerts/offer-alert';
 
 import Sequence from './sequence';
 
 import CourseBreadcrumbs from './CourseBreadcrumbs';
 import CourseSock from './course-sock';
-import ContentTools from './tools/ContentTools';
+import ContentTools from './content-tools';
 import { useModel } from '../../model-store';
 
 // Note that we import from the component files themselves in the enrollment-alert package.
 // This is because Reacy.lazy() requires that we import() from a file with a Component as it's
 // default export.
 // See React.lazy docs here: https://reactjs.org/docs/code-splitting.html#reactlazy
-const AccessExpirationAlert = React.lazy(() => import('../../access-expiration-alert/AccessExpirationAlert'));
-const EnrollmentAlert = React.lazy(() => import('../../enrollment-alert/EnrollmentAlert'));
-const StaffEnrollmentAlert = React.lazy(() => import('../../enrollment-alert/StaffEnrollmentAlert'));
-const LogistrationAlert = React.lazy(() => import('../../logistration-alert'));
-const OfferAlert = React.lazy(() => import('../../offer-alert/OfferAlert'));
+const AccessExpirationAlert = React.lazy(() => import('../../alerts/access-expiration-alert/AccessExpirationAlert'));
+const EnrollmentAlert = React.lazy(() => import('../../alerts/enrollment-alert/EnrollmentAlert'));
+const StaffEnrollmentAlert = React.lazy(() => import('../../alerts/enrollment-alert/StaffEnrollmentAlert'));
+const LogistrationAlert = React.lazy(() => import('../../alerts/logistration-alert'));
+const OfferAlert = React.lazy(() => import('../../alerts/offer-alert/OfferAlert'));
 
 function Course({
   courseId,

src/courseware/course/content-tools/ContentTools.jsx

@@ -2,8 +2,8 @@ import React from 'react';
 import PropTypes from 'prop-types';
 
 import Calculator from './calculator';
-import NotesVisibility from './notes/NotesVisibility';
-import './tools.scss';
+import NotesVisibility from './notes-visibility';
+import './contentTools.scss';
 
 export default function ContentTools({
   course,

src/courseware/course/content-tools/calculator/index.js

@@ -0,0 +1,3 @@
+import Calculator from './Calculator';
+
+export default Calculator;

src/courseware/course/content-tools/index.js

@@ -0,0 +1,3 @@
+import ContentTools from './ContentTools';
+
+export default ContentTools;

src/courseware/course/content-tools/notes-visibility/NotesVisibility.jsx

@@ -8,9 +8,12 @@ import {
 } from '@edx/frontend-platform/i18n';
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import { faPencilAlt } from '@fortawesome/free-solid-svg-icons';
-import toggleNotes from '../data/api';
 import messages from './messages';
 
+function toggleNotes() {
+  const iframe = document.getElementById('unit-iframe');
+  iframe.contentWindow.postMessage('tools.toggleNotes', getConfig().LMS_BASE_URL);
+}
 
 class NotesVisibility extends Component {
   constructor(props) {

src/courseware/course/content-tools/notes-visibility/index.js

@@ -0,0 +1,3 @@
+import NotesVisibility from './NotesVisibility';
+
+export default NotesVisibility;

src/courseware/course/course-sock/index.js

@@ -0,0 +1,3 @@
+import CourseSock from './CourseSock';
+
+export default CourseSock;

src/courseware/course/tools/data/api.js

@@ -1,6 +0,0 @@
-import { getConfig } from '@edx/frontend-platform';
-
-export default function toggleNotes() {
-  const iframe = document.getElementById('unit-iframe');
-  iframe.contentWindow.postMessage('tools.toggleNotes', getConfig().LMS_BASE_URL);
-}

src/data/thunks.js

@@ -35,17 +35,6 @@ export function fetchCourse(courseId) {
           modelType: 'courses',
           model: courseMetadataResult.value,
         }));
-        dispatch(addModel({
-          modelType: 'courseInfo',
-          model: {
-            id: courseMetadataResult.value.id,
-            isStaff: courseMetadataResult.value.isStaff,
-            number: courseMetadataResult.value.number,
-            org: courseMetadataResult.value.org,
-            tabs: courseMetadataResult.value.tabs,
-            title: courseMetadataResult.value.title,
-          },
-        }));
       }
 
       if (courseBlocksResult.status === 'fulfilled') {
@@ -53,6 +42,7 @@ export function fetchCourse(courseId) {
           courses, sections, sequences, units,
         } = courseBlocksResult.value;
 
+        // This updates the course with a sectionIds array from the blocks data.
         dispatch(updateModelsMap({
           modelType: 'courses',
           modelsMap: courses,
@@ -124,16 +114,14 @@ export function fetchTab(courseId, tab, version) {
       const fetchedTabData = tabDataResult.status === 'fulfilled';
 
       if (fetchedMetadata) {
+        /*
+         * NOTE: The "courses" models created by this thunk do not include an array of sectionIds.
+         * If that data is required for some use case, then fetchTab will need to call
+         * getCourseBlocks as well.  See fetchCourse above.
+         */
         dispatch(addModel({
-          modelType: 'courseInfo',
-          model: {
-            id: courseMetadataResult.value.id,
-            isStaff: courseMetadataResult.value.isStaff,
-            number: courseMetadataResult.value.number,
-            org: courseMetadataResult.value.org,
-            tabs: courseMetadataResult.value.tabs,
-            title: courseMetadataResult.value.title,
-          },
+          modelType: 'courses',
+          model: courseMetadataResult.value,
         }));
       } else {
         logError(courseMetadataResult.reason);

src/enrollment-alert/hooks.js

@@ -1,54 +0,0 @@
-/* eslint-disable import/prefer-default-export */
-import {
-  useContext, useState, useEffect, useCallback,
-} from 'react';
-import { UserMessagesContext, ALERT_TYPES } from '../user-messages';
-import { useModel } from '../model-store';
-import { postCourseEnrollment } from './data/api';
-
-export function useEnrollmentAlert(courseId) {
-  const course = useModel('courses', courseId);
-  const { add, remove } = useContext(UserMessagesContext);
-  const [alertId, setAlertId] = useState(null);
-  const isEnrolled = course && course.isEnrolled;
-  useEffect(() => {
-    if (course && course.isEnrolled !== undefined) {
-      if (!course.isEnrolled && alertId === null) {
-        const code = course.isStaff ? 'clientStaffEnrollmentAlert' : 'clientEnrollmentAlert';
-        setAlertId(add({
-          code,
-          topic: 'course',
-        }));
-      } else if (course.isEnrolled && alertId !== null) {
-        remove(alertId);
-        setAlertId(null);
-      }
-    }
-    return () => {
-      if (alertId !== null) {
-        remove(alertId);
-      }
-    };
-  }, [course, isEnrolled]);
-}
-
-export function useEnrollClickHandler(courseId, successText) {
-  const [loading, setLoading] = useState(false);
-  const { addFlash } = useContext(UserMessagesContext);
-  const enrollClickHandler = useCallback(() => {
-    setLoading(true);
-    postCourseEnrollment(courseId).then(() => {
-      addFlash({
-        dismissible: true,
-        flash: true,
-        text: successText,
-        type: ALERT_TYPES.SUCCESS,
-        topic: 'course',
-      });
-      setLoading(false);
-      global.location.reload();
-    });
-  }, [courseId]);
-
-  return { enrollClickHandler, loading };
-}

src/logistration-alert/hooks.js

@@ -1,28 +0,0 @@
-/* eslint-disable import/prefer-default-export */
-import { useContext, useState, useEffect } from 'react';
-import { AppContext } from '@edx/frontend-platform/react';
-import { UserMessagesContext, ALERT_TYPES } from '../user-messages';
-
-export function useLogistrationAlert() {
-  const { authenticatedUser } = useContext(AppContext);
-  const { add, remove } = useContext(UserMessagesContext);
-  const [alertId, setAlertId] = useState(null);
-  useEffect(() => {
-    if (authenticatedUser === null && alertId === null) {
-      setAlertId(add({
-        code: 'clientLogistrationAlert',
-        dismissible: false,
-        type: ALERT_TYPES.ERROR,
-        topic: 'course',
-      }));
-    } else if (authenticatedUser !== null && alertId !== null) {
-      remove(alertId);
-      setAlertId(null);
-    }
-    return () => {
-      if (alertId !== null) {
-        remove(alertId);
-      }
-    };
-  }, [authenticatedUser]);
-}

src/offer-alert/hooks.js

@@ -1,28 +0,0 @@
-/* eslint-disable import/prefer-default-export */
-import { useContext, useState, useEffect } from 'react';
-import { UserMessagesContext } from '../user-messages';
-import { useModel } from '../model-store';
-
-export function useOfferAlert(courseId) {
-  const course = useModel('courses', courseId);
-  const { add, remove } = useContext(UserMessagesContext);
-  const [alertId, setAlertId] = useState(null);
-  const rawHtml = (course && course.offerHtml) || null;
-  useEffect(() => {
-    if (rawHtml && alertId === null) {
-      setAlertId(add({
-        code: 'clientOfferAlert',
-        topic: 'course',
-        rawHtml,
-      }));
-    } else if (!rawHtml && alertId !== null) {
-      remove(alertId);
-      setAlertId(null);
-    }
-    return () => {
-      if (alertId !== null) {
-        remove(alertId);
-      }
-    };
-  }, [alertId, courseId, rawHtml]);
-}

src/tab-page/LoadedTabPage.jsx

@@ -3,7 +3,7 @@ import PropTypes from 'prop-types';
 
 import { Header, CourseTabsNavigation } from '../course-header';
 import { useModel } from '../model-store';
-import { useEnrollmentAlert } from '../enrollment-alert';
+import { useEnrollmentAlert } from '../alerts/enrollment-alert';
 import InstructorToolbar from '../courseware/course/InstructorToolbar';
 
 function LoadedTabPage({
@@ -20,7 +20,7 @@ function LoadedTabPage({
     org,
     tabs,
     title,
-  } = useModel('courseInfo', courseId);
+  } = useModel('courses', courseId);
 
   return (
     <>

src/tab-page/TabPage.jsx

@@ -3,7 +3,7 @@ import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
 import { useSelector } from 'react-redux';
 
 import { Header } from '../course-header';
-import { useLogistrationAlert } from '../logistration-alert';
+import { useLogistrationAlert } from '../alerts/logistration-alert';
 import PageLoading from '../PageLoading';
 
 import messages from './messages';

src/user-messages/AlertList.jsx

@@ -28,7 +28,7 @@ export default function AlertList({
               type={message.type}
               dismissible={message.dismissible}
               onDismiss={() => remove(message.id)}
-              rawHtml={message.rawHtml}
+              payload={message.payload}
               {...customProps}
             >
               {message.text}

src/user-messages/UserMessagesProvider.jsx

@@ -82,11 +82,11 @@ export default function UserMessagesProvider({ children }) {
 
   function add(message) {
     const {
-      code, dismissible, text, type, topic, ...others
+      code, dismissible, text, type, topic, payload, ...others
     } = message;
     const id = refId.current;
     setMessages(currentMessages => [...currentMessages, {
-      code, dismissible, text, type, topic, ...others, id,
+      code, dismissible, text, type, topic, payload, ...others, id,
     }]);
     refId.current += 1;
     setNextId(refId.current);

src/user-messages/hooks.js

@@ -0,0 +1,26 @@
+/* eslint-disable import/prefer-default-export */
+import { useContext, useState, useEffect } from 'react';
+import UserMessagesContext from './UserMessagesContext';
+
+export function useAlert(isVisible, {
+  code, text, topic, type, payload, dismissible,
+}) {
+  const { add, remove } = useContext(UserMessagesContext);
+  const [alertId, setAlertId] = useState(null);
+
+  useEffect(() => {
+    if (isVisible && alertId === null) {
+      setAlertId(add({
+        code, text, topic, type, payload, dismissible,
+      }));
+    } else if (!isVisible && alertId !== null) {
+      remove(alertId);
+      setAlertId(null);
+    }
+    return () => {
+      if (alertId !== null) {
+        remove(alertId);
+      }
+    };
+  }, [alertId, isVisible, code, text, topic, type, dismissible, payload]);
+}

src/user-messages/index.js

@@ -2,3 +2,4 @@ export { default as UserMessagesProvider, ALERT_TYPES } from './UserMessagesProv
 export { default as UserMessagesContext } from './UserMessagesContext';
 export { default as AlertList } from './AlertList';
 export { default as Alert } from './Alert';
+export { useAlert } from './hooks';