Fix Timetable Config Drift and Notify User When Faculty Changes Timetable

[zehata]

This discussion discusses fixes for #4283 and follows from #4292.

Schema and Serialization

Instead of storing lesson indices, we store LessonIds

export type ModuleLessonConfig = {
--  [lessonType: LessonType]: LessonIndex[];
++  [lessonType: LessonType]: LessonId[];
};

LessonIds are serialization of all a lesson details, except its module info and lesson type, please refer to #4292 (comment) for why this was chosen.

        {
          "classNo": "22",
          "lessonType": "Tutorial", // not serialized
          "weeks": [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13],
          "day": "Wednesday",
          "startTime": "0900",
          "endTime": "1000",
          "venue": "S16-0518"
        },

would serialize to
22|TUE|0900|1000|S16-0518|2_3_4_5_6_7_8_9_10_11_12_13.

Serialization and deserialization can be quite expensive. So we calculate and store a map of the LessonType to LessonIds to lessons in SemesterData.

// Semester-specific information of a module.
export type SemesterData = {
  // ...
  readonly lessonMap: LessonMap<RawLesson>;
};

Storing lesson details like this is stable across timetable changes, but when the lesson itself changes, this id will change too, so we implement recovery based on the serialized lesson details.

Recovery and Validation

For non-TA modules, we simply pick all lessons with the classNo and compare the set of lesson keys.
For TA modules, we can either:

• Get all lessons with the same classNo and add it to the config
• Do not recover, warn the user that the lesson has been modified

My preference is to keep it as is and notify the user and have them fix the config themselves

Notifications

I experimented with a few different ways of notifying users of lesson changes. The text messages are just placeholders, do suggest what you think would be clearer.

This warning shows up before recovery occurs and for lessons in TA modules
A notification that appears when there are lessons that got modified, which opens to a modal
An announcement that shows up in today and timetables when lessons are modified

Tasks

I am still cleaning things up and splitting them up into smaller PRs

• Update schema
  • I have split up my changes into smaller (hopefully) more reviewable PRs, you can see the current PRs stack at https://github.com/zehata/nusmods/tree/lesson-id-split.
  • Refactoring timetable utils before updating schema Refactor timetable utils into smaller files #4389
  • Removing dead code from Change representation of lesson configs from classNo to lesson indices #4225 so that I do not have to update the code for v3 and then delete it afterwards Cleanup dead code remaining that was missed in #4225 #4420
  • Refactor serialization/deserialization tests for better organization Refactor serialization/deserialization tests #4421
• Fix non-clonable actions so that modification notifications can be added Non-clonable Actions are Broken #4383
• Notify user when such changes have been made
  • Refactor timetable cells and add warning to timetable cells
  • Refactor existing notification or announcements to add notification or announcements

[jloh02]

Thanks for the suggestions. While they all look good, it seems counter-UX in the sense that these were never a problem to users who have never touched the TA feature? Doing seems to increase friction of simply using the platform. Perhaps, we should think of a more robust fix that does not break UX for majority of users.

A minor note on a higher level discussion:
(Personal take that does not necessarily reflect all maintainers)
If we are unable to resolve this inconvenience, I'm in favor of reverting back the entire feature of customizing specific timeslots for TA-ing and simply just marking an entire lessonType. While useful, this feature seems to benefit a very small community - TAs of multi-slot same lesson type courses. Using pure estimation, we have roughly 40 tutors for CS2103/T, and 20+ for CS2113. Not all of them may even use the TA feature on NUSMods. Compared to the thousands of students who use NUSMods, this inconvenience feels unnecessary

[zehata]

@leslieyip02 So, I have been working on it, and the further I go, the more I think it might be a bad idea to use a union type for ModuleLessonConfig. Do you think it's ok if we store non TA modules as

{
    Tutorial: ["1"]. // i.e. the ClassNo
}

and for TA modules

{
    Tutorial: ["1|WED|0900|blablabla", "1|FRI|0900|blablabla"]. // i.e. the LessonIds
}

Trying to use v1 lesson config for non TA modules complicates the logic more than I initially expected because of all the guards required at every turn and I really really think it will add a lot of technical debt, even before accounting for migration and serialization. It's still WIP but I can push it if you want to take a look at the complexity for dealing with a heterogenous ModuleLessonConfig.

[leslieyip02]

Hmm, I see what you mean. I was thinking that TA modules config would be stored in a separate object that is separate from ModuleLessonConfig, but I think you are right since just using LessonId[] would simplify the logic by a lot. Let's stick to LessonId in that case.

[zehata]

Ah ok... hmm you mean like going back to when we were storing TA module configs separately. I will have to think about it.

I do understand your concern about using LessonId though. Let's see if using ClassNo as LessonId will be ok, because I don't think it would be too complex?

[zehata]

just an update: it's done I'm just really slow writing tests.

[zehata]

Self reviewing...