Skip to content

Conversation

@zehata
Copy link
Contributor

@zehata zehata commented Dec 16, 2025

@leslieyip02 @jloh02 Patch for #4225, for the issue reported on the morning of 16 Dec by LH (https://t.me/NUSMods/13136)

Important Note: While this patch preserves the current behavior of the app, I think it is more prudent to add more error checking to the migration process, and to stop the migration if an error is encountered, because migrating the config is an inherently destructive process (it overwrites the old config). I am currently working on that. This patch should be considered superseded by #4272

  1. When the app fetches the timetable modules, it populates the lesson indices which allows the migration of the module config to the new schema

    case SUCCESS_KEY(FETCH_MODULE):
    return {
    ...state,
    modules: {
    ...state.modules,
    [action.payload.moduleCode]: {
    ...action.payload,
    timestamp: Date.now(),
    semesterData: map(action.payload.semesterData, (semesterData) => ({
    ...semesterData,
    timetable: map(semesterData.timetable, (lesson, lessonIndex) => ({
    ...lesson,
    lessonIndex,
    })),

  2. The issue was caused by

    function fetchTimetableModulesImpl(timetable: SemTimetableConfig, semester: Semester) {
    dispatch(fetchTimetableModulesAction([timetable]))
    .then(() => dispatch(validateTimetable(semester)))

    calling
    export function validateTimetable(semester: Semester) {
    return (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,
    );

When validateTimetable(1) is called when the app successfully fetches the module data for semester 1, it calls migrateTimetableConfigs which migrates the configs for all semesters, including semester 2. Since the request for semester 2 has not returned, the lesson indices has not been populated for semester 2 (see point 1). Trying to migrate the config for semester 2 will thus fail.

This patch fixes this issue by removing the migrateTimetableConfigs function and instead migrates each semester's timetable config as data is fetched for each of the semester.

…ble before state has been updated with lesson indices
@vercel
Copy link

vercel bot commented Dec 16, 2025

@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

@codecov
Copy link

codecov bot commented Dec 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.58%. Comparing base (988c6fd) to head (9b97038).
⚠️ Report is 152 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4271      +/-   ##
==========================================
+ Coverage   54.52%   56.58%   +2.05%     
==========================================
  Files         274      297      +23     
  Lines        6076     6917     +841     
  Branches     1455     1667     +212     
==========================================
+ Hits         3313     3914     +601     
- Misses       2763     3003     +240     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zehata zehata changed the title Fixed migration error caused by prematurely migrating semester timetable before the relevant module timetables have been loaded Patch for #4225 Dec 17, 2025
@zehata zehata marked this pull request as draft December 17, 2025 09:16
@zehata zehata closed this Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant