nusmods icon indicating copy to clipboard operation
nusmods copied to clipboard

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

Open zehata opened this issue 4 weeks ago • 2 comments

This is an extension to #4271, details of which I have copied over. @leslieyip02 @jloh02 Patch for #4225, for the issue reported on the morning of 16 Dec by LH (https://t.me/NUSMods/13136)

Post-mortem

  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 https://github.com/nusmodifications/nusmods/blob/d31261e060e45bcd79b07d251f0ffe2403905dee/website/src/reducers/moduleBank.ts#L47-L60

  2. The issue was caused by https://github.com/nusmodifications/nusmods/blob/d31261e060e45bcd79b07d251f0ffe2403905dee/website/src/views/AppShell.tsx#L64-L66 calling https://github.com/nusmodifications/nusmods/blob/d31261e060e45bcd79b07d251f0ffe2403905dee/website/src/actions/timetables.ts#L231-L239

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.

Fix

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.

It adds checks to prevent overwriting the user's config if any error occurs during the migration process.

It also adds code to create a random config to overwrite malformed configs caused by the migration failure.

zehata avatar Dec 17 '25 09:12 zehata

@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.

vercel[bot] avatar Dec 17 '25 09:12 vercel[bot]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 56.62%. Comparing base (988c6fd) to head (2986fbe). :warning: Report is 153 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4272      +/-   ##
==========================================
+ Coverage   54.52%   56.62%   +2.10%     
==========================================
  Files         274      297      +23     
  Lines        6076     6924     +848     
  Branches     1455     1671     +216     
==========================================
+ Hits         3313     3921     +608     
- Misses       2763     3003     +240     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Dec 17 '25 09:12 codecov[bot]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
nusmods-export Ready Ready Preview, Comment Dec 20, 2025 8:50pm
nusmods-website Ready Ready Preview, Comment Dec 20, 2025 8:50pm

vercel[bot] avatar Dec 20 '25 20:12 vercel[bot]

Also, while doing testing, I realized that the error occurred before writing to state, so users' configs were actually not overwritten. Nonetheless, I think it would make more sense to add checks to explicitly ensure that users' configs are not overwritten when errors are encountered.

zehata avatar Dec 21 '25 08:12 zehata