[WIP] Rebase lessonIndex drift

[leslieyip02]

Context

Fixes #4283.

Implementation

• Store the info of selected mods before refreshing the module bank
• When validating the timetable, check that the mod to lesson index mapping is still valid by comparison the stored index and the new index of the mod
• This is implemented by creating a signature for each mod (basically serializes all fields in a RawLesson) because the signature lets us look up the new index

Other Information

I've written unit tests but I haven't figured out how to actually test this.

[vercel]

@leslieyip02 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]

Codecov Report

❌ Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.93%. Comparing base (988c6fd) to head (bf1e0e3).
⚠️ Report is 164 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/views/AppShell.tsx	0.00%	4 Missing ⚠️
website/src/utils/timetables.ts	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4292      +/-   ##
==========================================
+ Coverage   54.52%   56.93%   +2.40%     
==========================================
  Files         274      297      +23     
  Lines        6076     6959     +883     
  Branches     1455     1678     +223     
==========================================
+ Hits         3313     3962     +649     
- Misses       2763     2997     +234     

☔ 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]

One of the potential issue I can see with continuing to use a index based schema is that configs will not remain stable across timetable changes. I am worried that this might cause issues at the beginning of the semesters when people are most likely going to be sharing timetables with each other.

If we stick with the current serialization method, and thus the current schema, we are always going to run into this problem with sharing timetables with the share link. While it is possible for us to create some kind of dialog that alerts the user that their previous config has become invalid, I think it will greatly affect the user experience if their shared link cannot automatically map lessons to the new ones when timetables are modified.

Move this logic to scraper?

If we choose to map lessons from the current timetable to the modified timetable, we might want to consider moving this logic into the scraper instead, and we can represent the configs entirely with a key that the scraper generates, either a uuid or a hash of the details.

A problem with this approach is that it is stateful, which would make it a bit more complicated and harder to maintain. I would also prefer not to touch the API, given that it is used by other apps as well.

	Non-modified lessons	Modified lessons
Hash	Easier to map, they are going to have the same hash	Harder to map, will need to be done on the client side, since the hash will change
Uuid	Loop through to find the same/similar lesson	Easier to map

Still, as a sidenote, in either case we are going to have to inform users that the lessons have been modified.

Custom lessons

Another thing that I am thinking about is custom lessons, which I know is not directly related to this, but is something I am considering too, since we would probably run into the question of how to serialize custom lessons in the future.

Separating serialization format and config schema?

We could potentially store the config schema by serializing all of the details of each lesson, and we serialize it differently from how we represent it internally. Personally, I think this is more trouble than it's worth, and we should probably change the schema to keep it consistent with the serialization format.

I realize it will be painful for us to introduce a yet another new config schema, since the previous migration caused quite a few issues, but I think it might better for us to rip the bandaid off and fix this once and for all, instead of retrofitting fixed atop of the current schema.

	Sample serialization string	Lookup on modified timetable	Complexity	String format readability, which affects maintainability	Custom lessons serialization
Hash lesson details	?PC2130=TUT:(fea8e9eb2298​16b08f08e4e9c​bbcf32a)	- Simple id lookup on modified timetable	- We incur some compute to calculate the hash, but I don't think it will be too much, since the longest lesson timetable array we get is only what, 90?	- Not readable	- Will require a different serialization format
Backend-generated unique lesson id	?PC2130=TUT:(131973c3-282​8-​4493-7cb-453465b41c12)	- Simple id lookup on modified timetable	- This will incur an API request to get the old timetable for each module - Mapping each lesson will incur some compute, but like above, probably not too significant	- Not readable	- Will require a different serialization format
Serialization of all details	?PC2130=TUT:(02/WE/09​00/1200/LT3/ODD)​&ta=(PC2130)	- Incurs complexity to map to modified timetable	- Serialization and deserialization should be quite straightforward and cheap	- Tedious but at least it's readable	- Same serialization format
Serialization of partial details	?PC2130=TUT:(02/WE/0900​)​&ta=(PC2130)	- Incurs complexity to map to modified timetable	- Serialization and deserialization will be quite complex - But will be human-readable	- Most readable, IMO	- Similar serialization format

Personally, I would favor the partial details approach, because it makes it a lot easier for debugging and thus improve maintainability. It should be considerably smaller than serializing all lesson details, but I think there are very few instances whereby we need every single one of the details of a lesson in order to disambiguate it.

My proposal is that we use the minimum partial details possible to disambiguate lessons

For non-TA modules, this will be effectively be the class number,
a sample serialized config will be ?CS1010E=TUT:(01)
This makes it trivial for serialization strings to work after modifications to the timetable

For TA modules, the simplest method is to identify each lesson with a minimum details required to separate it from other lessons
a sample serialized config will be ?CS1010E=TUT:(01/WE,01/TH)
Suppose one of the lesson was moved to friday, say, 01/THU, the wednesday lesson will be unaffected, and we just inform the user that no lessons can be found on thursday

Sidenote

Though I guess it's also possible to further reduce serialization string length if lessons in the config don't have to be disambiguated so the above would just be `?CS1010E=TUT:(01)` but we probably want to leave this for the future, since it's just further optimization

I thought about using a fixed set of details, which will make the logic straightforward, like class number/day of week/start time, but I realize this will run into issues:

• Some classes are held in 2 different venues concurrently
• Some classes are held on odd/even weeks, with otherwise the exact same details

[leslieyip02]

+1 on ripping off the band-aid. I don't think there's a good way to solve this without significant rewrites.

While I agree partial serialization would be good, I'm not confident that we can come up with a foolproof way of disambiguating mods:

1. There are too many mods
2. Would our disambiguation be future proof?

It's inelegant, but I personally feel a full serialization might be the safest way to implement this. The only downside I see is that the exported URLs would be verbose but most users probably wouldn't even notice.