Refactor admin panel (#2983)

* Refactor AdminPanel code structure

* Refactor data flow logic in assessment config panel

* Create WithImperativeApi utility type
* Refactor components

* Fix state bug

Author: RichDom2185

src/commons/utils/TypeHelper.ts

@@ -52,11 +52,22 @@ export type RemoveIndex<T> = {
     : K]: T[K];
 };
 
-/** A true intersection of the properties of types `A` and `B`, unlike the confusingly named
+/**
+ * A true intersection of the properties of types `A` and `B`, unlike the confusingly named
  * "Intersection Types" in TypeScript which uses the `&` operator and are actually unions.
- * This also excludes the index signature from both `A` and `B` automatically. */
+ * This also excludes the index signature from both `A` and `B` automatically.
+ */
 export type SharedProperties<A, B> = Pick<A, Extract<keyof RemoveIndex<A>, keyof RemoveIndex<B>>>;
 
+/**
+ * A type that represents a React component that has an imperative API
+ * (i.e. a ref that can be used to call methods on the component). Such
+ * components are typically wrapped in `forwardRef` and `useImperativeHandle`.
+ */
+export type WithImperativeApi<T, C extends React.ComponentType<any> = React.FC> = React.FC<
+  React.ComponentProps<C> & React.RefAttributes<T>
+>;
+
 /* =========================================
  * Utility types for tuple type manipulation
  * ========================================= */

src/pages/academy/adminPanel/AdminPanel.tsx

@@ -2,11 +2,9 @@ import 'ag-grid-community/styles/ag-grid.css';
 import 'ag-grid-community/styles/ag-theme-balham.css';
 
 import { Button, Divider, H1, Intent, Tab, Tabs } from '@blueprintjs/core';
-import { cloneDeep } from 'lodash';
-import React from 'react';
+import React, { useCallback, useEffect, useRef, useState } from 'react';
 import { useDispatch } from 'react-redux';
-import { Role } from 'src/commons/application/ApplicationTypes';
-import { useTypedSelector } from 'src/commons/utils/Hooks';
+import { useSession } from 'src/commons/utils/Hooks';
 import {
   addNewStoriesUsersToCourse,
   addNewUsersToCourse
@@ -19,65 +17,48 @@ import {
   fetchAssessmentConfigs,
   fetchCourseConfig,
   fetchNotificationConfigs,
-  setAssessmentConfigurations,
   updateAssessmentConfigs,
   updateCourseConfig,
   updateUserRole
 } from '../../../commons/application/actions/SessionActions';
 import { UpdateCourseConfiguration } from '../../../commons/application/types/SessionTypes';
-import { AssessmentConfiguration } from '../../../commons/assessment/AssessmentTypes';
 import ContentDisplay from '../../../commons/ContentDisplay';
-import AddStoriesUserPanel, { NameUsernameRole } from './subcomponents/AddStoriesUserPanel';
-import AddUserPanel, { UsernameRoleGroup } from './subcomponents/AddUserPanel';
-import AssessmentConfigPanel from './subcomponents/assessmentConfigPanel/AssessmentConfigPanel';
+import AddStoriesUserPanel from './subcomponents/AddStoriesUserPanel';
+import AddUserPanel from './subcomponents/AddUserPanel';
+import AssessmentConfigPanel, {
+  ImperativeAssessmentConfigPanel
+} from './subcomponents/assessmentConfigPanel/AssessmentConfigPanel';
 import CourseConfigPanel from './subcomponents/CourseConfigPanel';
 import NotificationConfigPanel from './subcomponents/NotificationConfigPanel';
 import UserConfigPanel from './subcomponents/userConfigPanel/UserConfigPanel';
 
-const AdminPanel: React.FC = () => {
-  const [hasChangesCourseConfig, setHasChangesCourseConfig] = React.useState(false);
-  const [hasChangesAssessmentConfig, setHasChangesAssessmentConfig] = React.useState(false);
+const defaultCourseConfig: UpdateCourseConfiguration = {
+  courseName: '',
+  courseShortName: '',
+  viewable: true,
+  enableGame: true,
+  enableAchievements: true,
+  enableSourcecast: true,
+  enableStories: false,
+  moduleHelpText: ''
+};
 
-  const [courseConfiguration, setCourseConfiguration] = React.useState<UpdateCourseConfiguration>({
-    courseName: '',
-    courseShortName: '',
-    viewable: true,
-    enableGame: true,
-    enableAchievements: true,
-    enableSourcecast: true,
-    enableStories: false,
-    moduleHelpText: ''
-  });
+const AdminPanel: React.FC = () => {
+  const [hasChangesCourseConfig, setHasChangesCourseConfig] = useState(false);
+  const [hasChangesAssessmentConfig, setHasChangesAssessmentConfig] = useState(false);
+  const [courseConfiguration, setCourseConfiguration] = useState(defaultCourseConfig);
 
   const dispatch = useDispatch();
+  const session = useSession();
 
-  const session = useTypedSelector(state => state.session);
-
-  /**
-   * Mutable ref to track the assessment configuration form state instead of useState. This is
-   * because ag-grid does not update the cellRendererParams whenever there is an update in rowData,
-   * leading to a stale closure problem where the handlers in AssessmentConfigPanel capture the old
-   * value of assessmentConfig.
-   *
-   * Also, useState causes a flicker in ag-grid during rerenders. Thus we use this mutable ref and
-   * ag-grid's API to update cell values instead.
-   */
-  const assessmentConfig = React.useRef(session.assessmentConfigurations);
-
-  // Tracks the assessment configurations to be deleted in the backend when the save button is clicked
-  const [assessmentConfigsToDelete, setAssessmentConfigsToDelete] = React.useState<
-    AssessmentConfiguration[]
-  >([]);
-
-  React.useEffect(() => {
+  useEffect(() => {
     dispatch(fetchCourseConfig());
     dispatch(fetchAssessmentConfigs());
     dispatch(fetchAdminPanelCourseRegistrations());
     dispatch(fetchNotificationConfigs());
   }, [dispatch]);
 
-  // After updated configs have been loaded from the backend, put them into local React state
-  React.useEffect(() => {
+  useEffect(() => {
     setCourseConfiguration({
       courseName: session.courseName,
       courseShortName: session.courseShortName,
@@ -88,10 +69,22 @@ const AdminPanel: React.FC = () => {
       enableStories: session.enableStories,
       moduleHelpText: session.moduleHelpText
     });
-
-    // IMPT: To prevent mutation of props
-    assessmentConfig.current = cloneDeep(session.assessmentConfigurations);
-  }, [session]);
+  }, [
+    session.assessmentConfigurations,
+    session.courseName,
+    session.courseShortName,
+    session.enableAchievements,
+    session.enableGame,
+    session.enableSourcecast,
+    session.enableStories,
+    session.moduleHelpText,
+    session.viewable
+  ]);
+
+  const tableRef = useRef<ImperativeAssessmentConfigPanel>(null);
+  useEffect(() => {
+    tableRef.current?.resetData();
+  }, [session.assessmentConfigurations]);
 
   const courseConfigPanelProps = {
     courseConfiguration: courseConfiguration,
@@ -101,67 +94,31 @@ const AdminPanel: React.FC = () => {
     }
   };
 
-  const assessmentConfigPanelProps = React.useMemo(() => {
-    return {
-      // Would have been loaded by the useEffect above
-      assessmentConfig: assessmentConfig as React.MutableRefObject<AssessmentConfiguration[]>,
-      setAssessmentConfig: (val: AssessmentConfiguration[]) => {
-        assessmentConfig.current = val;
-        setHasChangesAssessmentConfig(true);
-      },
-      setAssessmentConfigsToDelete: (deletedElement: AssessmentConfiguration) => {
-        // If it is not a newly created row that is yet to be persisted in the backend
-        if (deletedElement.assessmentConfigId !== -1) {
-          const temp = [...assessmentConfigsToDelete];
-          temp.push(deletedElement);
-          setAssessmentConfigsToDelete(temp);
-        }
-      },
-      setHasChangesAssessmentConfig: setHasChangesAssessmentConfig
-    };
-  }, [assessmentConfigsToDelete]);
-
-  const userConfigPanelProps = {
-    courseRegId: session.courseRegId,
-    userCourseRegistrations: session.userCourseRegistrations,
-    handleUpdateUserRole: (courseRegId: number, role: Role) =>
-      dispatch(updateUserRole(courseRegId, role)),
-    handleDeleteUserFromCourse: (courseRegId: number) =>
-      dispatch(deleteUserCourseRegistration(courseRegId))
-  };
-
-  const addUserPanelProps = {
-    handleAddNewUsersToCourse: (users: UsernameRoleGroup[], provider: string) =>
-      dispatch(addNewUsersToCourse(users, provider))
-  };
-
-  const addStoriesUserPanelProps = {
-    handleAddNewUsersToCourse: (users: NameUsernameRole[], provider: string) =>
-      dispatch(addNewStoriesUsersToCourse(users, provider))
-  };
-
   // Handler to submit changes to Course Configration and Assessment Configuration to the backend.
   // Changes made to users are handled separately.
-  const submitHandler = () => {
+  const submitHandler = useCallback(() => {
     if (hasChangesCourseConfig) {
       dispatch(updateCourseConfig(courseConfiguration));
       setHasChangesCourseConfig(false);
     }
-    if (assessmentConfigsToDelete.length > 0) {
-      assessmentConfigsToDelete.forEach(assessmentConfig => {
-        dispatch(deleteAssessmentConfig(assessmentConfig));
-      });
-      setAssessmentConfigsToDelete([]);
-    }
+    const tableState = tableRef.current?.getData() ?? [];
+    const currentConfigs = session.assessmentConfigurations ?? [];
+    const currentIds = new Set(tableState.map(config => config.assessmentConfigId));
+    const configsToDelete = currentConfigs.filter(
+      config => !currentIds.has(config.assessmentConfigId)
+    );
+    configsToDelete.forEach(config => dispatch(deleteAssessmentConfig(config)));
     if (hasChangesAssessmentConfig) {
-      // Reset the store first so that old props do not propagate down and cause a flicker
-      dispatch(setAssessmentConfigurations([]));
-
-      // assessmentConfig.current will exist after the first load
-      dispatch(updateAssessmentConfigs(assessmentConfig.current!));
+      dispatch(updateAssessmentConfigs(tableState));
       setHasChangesAssessmentConfig(false);
     }
-  };
+  }, [
+    courseConfiguration,
+    dispatch,
+    hasChangesAssessmentConfig,
+    hasChangesCourseConfig,
+    session.assessmentConfigurations
+  ]);
 
   const data = (
     <div className="admin-panel">
@@ -175,9 +132,14 @@ const AdminPanel: React.FC = () => {
             <>
               <CourseConfigPanel {...courseConfigPanelProps} />
               <Divider />
-              <AssessmentConfigPanel {...assessmentConfigPanelProps} />
+              <AssessmentConfigPanel
+                ref={tableRef}
+                initialConfigs={session.assessmentConfigurations ?? []}
+                setHasChangesAssessmentConfig={setHasChangesAssessmentConfig}
+              />
               <Button
                 text="Save"
+                disabled={!hasChangesCourseConfig && !hasChangesAssessmentConfig}
                 style={{ marginTop: '15px' }}
                 intent={
                   hasChangesCourseConfig || hasChangesAssessmentConfig
@@ -189,12 +151,43 @@ const AdminPanel: React.FC = () => {
             </>
           }
         />
-        <Tab id="users" title="Users" panel={<UserConfigPanel {...userConfigPanelProps} />} />
-        <Tab id="add-users" title="Add Users" panel={<AddUserPanel {...addUserPanelProps} />} />
+        <Tab
+          id="users"
+          title="Users"
+          panel={
+            <UserConfigPanel
+              courseRegId={session.courseRegId}
+              userCourseRegistrations={session.userCourseRegistrations}
+              handleUpdateUserRole={(courseRegId, role) =>
+                dispatch(updateUserRole(courseRegId, role))
+              }
+              handleDeleteUserFromCourse={(courseRegId: number) =>
+                dispatch(deleteUserCourseRegistration(courseRegId))
+              }
+            />
+          }
+        />
+        <Tab
+          id="add-users"
+          title="Add Users"
+          panel={
+            <AddUserPanel
+              handleAddNewUsersToCourse={(users, provider) =>
+                dispatch(addNewUsersToCourse(users, provider))
+              }
+            />
+          }
+        />
         <Tab
           id="add-stories-users"
           title="Add Stories Users"
-          panel={<AddStoriesUserPanel {...addStoriesUserPanelProps} />}
+          panel={
+            <AddStoriesUserPanel
+              handleAddNewUsersToCourse={(users, provider) =>
+                dispatch(addNewStoriesUsersToCourse(users, provider))
+              }
+            />
+          }
         />
         <Tab id="notification-config" title="Notifications" panel={<NotificationConfigPanel />} />
       </Tabs>