Patch for #4225, with additional checks to prevent overwriting user config if error encountered and allow recovery for malformed configs

website/src/actions/timetables.test.ts

@@ -4,7 +4,14 @@ import { SemTimetableConfig, LessonWithIndex, TimetableConfigV1 } from 'types/ti
 import lessons from '__mocks__/lessons-array.json';
 import { CS1010A, CS1010S, CS3216 } from '__mocks__/modules';
 
-import { TaModulesMapV1, ModuleBank, TimetablesState } from 'types/reducers';
+import {
+  TaModulesMapV1,
+  ModuleBank,
+  TimetablesState,
+  SemesterColorMap,
+  HiddenModulesMap,
+  ColorMapping,
+} from 'types/reducers';
 import { defaultTimetableState } from 'reducers/timetables';
 import * as actions from './timetables';
 
@@ -135,7 +142,7 @@ describe('fillTimetableBlanks', () => {
   });
   const action = actions.validateTimetable(semester);
 
-  test('do nothing if timetable is already full', () => {
+  test('do nothing if timetable is already full', async () => {
     const timetable = {
       CS1010S: {
         Lecture: [0],
@@ -146,12 +153,11 @@ describe('fillTimetableBlanks', () => {
 
     const state: any = { timetables: timetablesState(timetable), moduleBank };
     const dispatch = jest.fn();
-    action(dispatch, () => state);
-
+    await expect(action(dispatch, () => state)).resolves.not.toThrow(Error);
     expect(dispatch).not.toHaveBeenCalled();
   });
 
-  test('fill missing lessons with randomly generated modules', () => {
+  test('fill missing lessons with randomly generated modules', async () => {
     const timetable = {
       CS1010S: {
         Lecture: [0],
@@ -161,13 +167,11 @@ describe('fillTimetableBlanks', () => {
     };
     const state: any = { timetables: timetablesState(timetable), moduleBank };
     const dispatch = jest.fn();
-
-    action(dispatch, () => state);
-
+    await expect(action(dispatch, () => state)).resolves.not.toThrow(Error);
     expect(dispatch).toHaveBeenCalledTimes(2);
 
     const [[firstAction], [secondAction]] = dispatch.mock.calls;
-    expect(firstAction).toMatchObject({
+    expect(firstAction).toEqual({
       type: actions.SET_LESSON_CONFIG,
       payload: {
         semester,
@@ -180,7 +184,7 @@ describe('fillTimetableBlanks', () => {
       },
     });
 
-    expect(secondAction).toMatchObject({
+    expect(secondAction).toEqual({
       type: actions.SET_LESSON_CONFIG,
       payload: {
         semester,
@@ -192,9 +196,13 @@ describe('fillTimetableBlanks', () => {
     });
   });
 
-  test('migrate v1 config', () => {
+  test('migrate v1 config', async () => {
+    const colors: ColorMapping = {
+      CS1010S: 0,
+      CS3216: 1,
+    };
+    const hiddenModules: ModuleCode[] = [];
     const timetables = {
-      ...initialState,
       lessons: {
         [semester]: {
           CS1010S: {
@@ -207,6 +215,12 @@ describe('fillTimetableBlanks', () => {
           },
         } as TimetableConfigV1,
       },
+      colors: {
+        [semester]: colors,
+      } as SemesterColorMap,
+      hidden: {
+        [semester]: hiddenModules,
+      } as HiddenModulesMap,
       ta: {
         [semester]: {
           CS1010S: [
@@ -220,29 +234,35 @@ describe('fillTimetableBlanks', () => {
 
     const state: any = { timetables, moduleBank };
     const dispatch = jest.fn();
-    action(dispatch, () => state);
+    await expect(action(dispatch, () => state)).resolves.not.toThrow(Error);
     expect(dispatch).toHaveBeenCalledTimes(1);
     const [[firstAction]] = dispatch.mock.calls;
-    expect(firstAction).toMatchObject({
-      type: 'SET_TIMETABLES',
+
+    const migratedTimetable: SemTimetableConfig = {
+      CS1010S: {
+        Lecture: [0],
+        Recitation: [3],
+        Tutorial: [21],
+      },
+      CS3216: {
+        Lecture: [0],
+      },
+    };
+    const migratedTaModules: ModuleCode[] = ['CS1010S'];
+
+    expect(firstAction).toEqual({
+      type: 'SET_TIMETABLE',
       payload: {
-        lessons: {
-          [semester]: {
-            CS1010S: {
-              Lecture: [0],
-              Tutorial: [21],
-              Recitation: [3],
-            },
-          },
-        },
-        taModules: {
-          [semester]: ['CS1010S'],
-        },
+        semester,
+        timetable: migratedTimetable,
+        colors,
+        hiddenModules,
+        taModules: migratedTaModules,
       },
     });
   });
 
-  test('should not error when module cannot be found', () => {
+  test('should not error when module cannot be found', async () => {
     const timetable = {
       CS1010S: {
         Lecture: [0],
@@ -260,8 +280,25 @@ describe('fillTimetableBlanks', () => {
       moduleBank: moduleBankWithoutModule,
     };
     const dispatch = jest.fn();
-    action(dispatch, () => state);
+    await expect(action(dispatch, () => state)).resolves.not.toThrow(Error);
+    expect(dispatch).not.toThrow(TypeError);
+  });
 
+  test('should not error when timetable configs are malformed', async () => {
+    const timetable = {
+      CS1010S: {
+        Lecture: [undefined],
+        Tutorial: ['1'],
+        Recitation: [null],
+      },
+    };
+    const timetables = {
+      ...initialState,
+      lessons: { [semester]: timetable },
+    };
+    const state: any = { timetables, moduleBank };
+    const dispatch = jest.fn();
+    await expect(action(dispatch, () => state)).resolves.not.toThrow(Error);
     expect(dispatch).not.toThrow(TypeError);
   });
 });

website/src/actions/timetables.ts

@@ -1,4 +1,4 @@
-import { each, flatMap } from 'lodash';
+import { each, flatMap, get } from 'lodash';
 
 import type {
   ColorIndex,
@@ -18,27 +18,19 @@ import { getModuleCondensed } from 'selectors/moduleBank';
 import {
   getClosestLessonConfig,
   makeLessonIndicesMap,
-  migrateTimetableConfigs,
+  migrateSemTimetableConfig,
   randomModuleLessonConfig,
   validateModuleLessons,
   validateTimetableModules,
 } from 'utils/timetables';
 import { getModuleSemesterData, getModuleTimetable } from 'utils/modules';
 
 // Actions that should not be used directly outside of thunks
-export const SET_TIMETABLES = 'SET_TIMETABLES' as const;
 export const SET_TIMETABLE = 'SET_TIMETABLE' as const;
 export const ADD_MODULE = 'ADD_MODULE' as const;
 export const SET_HIDDEN_IMPORTED = 'SET_HIDDEN_IMPORTED' as const;
 export const SET_TA_IMPORTED = 'SET_TA_IMPORTED' as const;
 export const Internal = {
-  setTimetables(lessons: TimetableConfig, taModules: TaModulesMap) {
-    return {
-      type: SET_TIMETABLES,
-      payload: { lessons, taModules },
-    };
-  },
-
   setTimetable(
     semester: Semester,
     timetable: SemTimetableConfig | undefined,
@@ -229,29 +221,47 @@ export function setTimetable(
  * @param semester Semester of the timetable config to validate
  */
 export function validateTimetable(semester: Semester) {
-  return (dispatch: Dispatch, getState: GetState) => {
+  return async (dispatch: Dispatch, getState: GetState) => {
     const { timetables, moduleBank } = getState();
 
-    const { lessons, ta, alreadyMigrated } = migrateTimetableConfigs(
-      timetables.lessons as TimetableConfig | TimetableConfigV1,
-      timetables.ta as TaModulesMap | TaModulesMapV1,
-      moduleBank.modules,
-    );
+    const timetableConfig = timetables.lessons as TimetableConfig | TimetableConfigV1;
+    const semTimetableConfig = timetableConfig[semester];
 
-    if (!alreadyMigrated) dispatch(Internal.setTimetables(lessons, ta));
+    const taTimetableConfig = timetables.ta as TaModulesMap | TaModulesMapV1;
+    const taModulesConfig = get(taTimetableConfig, semester, {});
 
-    // Extract the timetable and the modules for the semester
-    const timetable = lessons[semester];
-    if (!timetable) return;
-    const taModules = ta[semester];
+    const getModuleSemesterTimetable = (moduleCode: ModuleCode) =>
+      moduleBank.modules[moduleCode]
+        ? getModuleTimetable(moduleBank.modules[moduleCode], semester)
+        : [];
+
+    const {
+      migratedSemTimetableConfig: timetable,
+      migratedTaModulesConfig: ta,
+      alreadyMigrated,
+    } = await Promise.resolve(
+      migrateSemTimetableConfig(semTimetableConfig, taModulesConfig, getModuleSemesterTimetable),
+    );
+
+    if (!alreadyMigrated) {
+      dispatch(
+        Internal.setTimetable(
+          semester,
+          timetable,
+          timetables.colors[semester],
+          timetables.hidden[semester],
+          ta,
+        ),
+      );
+    }
 
     // Check that all lessons for each module are valid. If they are not, we update it
     // such that they are
     each(timetable, (lessonConfig: ModuleLessonConfig, moduleCode: ModuleCode) => {
       const module = moduleBank.modules[moduleCode];
       if (!module) return;
 
-      const isTa = taModules?.includes(moduleCode);
+      const isTa = ta?.includes(moduleCode);
 
       const { validatedLessonConfig, valid } = validateModuleLessons(
         semester,

website/src/reducers/timetables.test.ts

@@ -363,15 +363,3 @@ describe('import timetable', () => {
     });
   });
 });
-
-describe('migrate v1 config', () => {
-  test('should migrate config to new format', () => {
-    expect(
-      reducer(initialState, Internal.setTimetables({ [1]: { CS1010S: {} } }, { [1]: ['CS1010S'] })),
-    ).toStrictEqual({
-      ...initialState,
-      lessons: { [1]: { CS1010S: {} } },
-      ta: { [1]: ['CS1010S'] },
-    });
-  });
-});

website/src/reducers/timetables.ts

@@ -24,7 +24,6 @@ import {
   SET_TIMETABLE,
   SHOW_LESSON_IN_TIMETABLE,
   REMOVE_TA_MODULE,
-  SET_TIMETABLES,
 } from 'actions/timetables';
 import { getNewColor } from 'utils/colors';
 import { SET_EXPORTED_DATA } from 'actions/constants';
@@ -250,15 +249,6 @@ function timetables(
   }
 
   switch (action.type) {
-    case SET_TIMETABLES: {
-      const { lessons, taModules } = action.payload;
-      return {
-        ...state,
-        lessons,
-        ta: taModules,
-      };
-    }
-
     case SET_TIMETABLE: {
       const { semester, timetable, colors, hiddenModules, taModules } = action.payload;
 

website/src/utils/timetables.test.ts

@@ -10,6 +10,7 @@ import {
   TimetableArrangement,
   TimetableDayArrangement,
   TimetableDayFormat,
+  ModuleLessonConfigV1,
 } from 'types/timetables';
 import {
   LessonType,
@@ -942,7 +943,7 @@ describe('v1 config migration', () => {
     });
   });
 
-  test('should not error if ta module config is mismatched', () => {
+  test('should not error if ta module config was migrated but module lesson config was not', () => {
     const migrationResult = migrateModuleLessonConfig(
       moduleLessonConfig,
       [],
@@ -957,25 +958,41 @@ describe('v1 config migration', () => {
     });
   });
 
-  test('should ignore invalid classNo', () => {
-    const invalidTaModuleConfig = {
+  test('should error if migration is missing data to migrate from the old config', () => {
+    const taModuleConfig = {
+      CS1010S: [['Lecture', '1']],
+    } as TaModulesConfigV1;
+    expect(() =>
+      migrateModuleLessonConfig(moduleLessonConfig, taModuleConfig, 'CS1010S', []),
+    ).toThrow(Error('Lesson indices missing'));
+  });
+
+  test('should error if migration cannot find the lesson indices for non-ta module classNo', () => {
+    const invalidModuleLessonConfig = {
+      Lecture: '2',
+    } as ModuleLessonConfigV1;
+    expect(() =>
+      migrateModuleLessonConfig(
+        invalidModuleLessonConfig,
+        {
+          CS1010S: [],
+        },
+        'CS1010S',
+        moduleTimetable,
+      ),
+    ).toThrow(Error('Lesson indices missing'));
+  });
+
+  test('should error if migration cannot find the lesson indices for ta module classNo', () => {
+    const taModuleConfig = {
       CS1010S: [
         ['Lecture', '1'],
         ['Lecture', '2'],
       ],
     } as TaModulesConfigV1;
-    const migrationResult = migrateModuleLessonConfig(
-      moduleLessonConfig,
-      invalidTaModuleConfig,
-      'CS1010S',
-      moduleTimetable,
-    );
-    expect(migrationResult).toEqual({
-      migratedModuleLessonConfig: {
-        Lecture: [0],
-      },
-      alreadyMigrated: false,
-    });
+    expect(() =>
+      migrateModuleLessonConfig(moduleLessonConfig, taModuleConfig, 'CS1010S', moduleTimetable),
+    ).toThrow(Error('Lesson indices missing'));
   });
 });