Store lessons by their index in semester data timetable, replace lessons-by-classNo with Lesson Groups

[zehata]

Opening this discussion to talk about proposed changes originating from #3930

Lesson groups are meant to replace lessons-by-classNo, or rather, serves as an evolution
Lesson groups store lesson indices as an array
Lessons in a lesson group must have the same classNo
No lessons in a lesson group may overlap with another

• Lessons in a lesson group may overlap in terms of day and startTime and endTime provided that the weeks are separate (Added, thanks @leslieyip02)
• If there are overlaps, first try to differentiate them by venue
• If there are still overlaps, try to differentiate them by weeks
• If there are still overlaps, the lesson group is necessarily unitary, return only the lesson itself

This is meant to resolve ambiguous lessons with the same classNo and day and startTime and endTime like the ones in HSI1000

The state structure under timetable will be

timetable: {
    lessons: [
        CS2103T: {
            Lecture: [15],
        }
    ],
    ta: ["CS2103T"],
}

When changing lessons in normal mode, lesson groups are used to show the hints on hover, and when the user selects the lesson, we can write the lesson indices in the lesson group into state

When adding lessons in TA mode, lesson groups are not used

When disabling TA mode for a module, we use the lesson indices in the state to determine which lesson group should be written into state, depending on which lesson group has the most matching lessons when the user disables TA mode.

[zehata]

@leslieyip02 Well, the bug with HSI1000 is fixed. But uhhh... we need to think of the UI now:

[zehata]

Don't worry about the identifier code, it's just for testing

[leslieyip02]

I'm honestly not too bothered by it, the UX is consistent with current behavior:

[zehata]

The basics work now, but still WIP on things like backward comp and serialization

Recording.2025-07-10.222140.1.mp4

[leslieyip02]

Good progress, but I think the implementation might be a bit wrong because in normal mode (non-TA), adding HSI1000 should add both slots of the classNo?

[zehata]

@leslieyip02 Oh I didn't realize those are actually 1 class, didn't see the weeks

[zehata]

Thanks, @leslieyip02
Fixed it. I didn't account for the fact that lessons in one lesson group could be held at different venues on different weeks

Recording.2025-07-11.175724.mp4

[zehata]

Progress update: finished the serialization. Working on backward comps now.

[zehata]

Progress update: Finished backward comps.

Time to learn golang to port optimizer over 😮‍💨

[zehata]

@leslieyip02 Can I clarify something from: #3930 (comment)?

Same classNo, different lesson types at different times: FST2102B

• There's only 1 lab and 1 tut slot, but because it's grouped together, you must have both
• The current logic blocks selecting options if there's only 1 possible slot for that lesson type
• This might be out-of-scope

I am not sure I quite follow the issue described, what is expected behaviour here?

[leslieyip02]

There's a tut slot and a lab slot, but you can't toggle either of them off because there's only 1 option for each class type:

But technically it could be possible that you are only TA-ing one of the lessons, so it would be good if it they could be toggled even if there's only one option?

[zehata]

Progress: porting optimiser is done. Working on tests.

[zehata]

It just occurred to me I should probably ask for comments about this implementation before fixing the test cases
https://github.com/zehata/nusmods/tree/grouped-lessons-with-lesson-index

There are a bunch of hacks I used client side to augment what is meant to be data returned from server side, those will be removed of course.

I am looking for comments about the general implementation. There is still quite a bit of refactoring that I still want to do.

[zehata]

Diff to nusmods/master: https://github.com/nusmodifications/nusmods/compare/master...zehata:nusmods:grouped-lessons-with-lesson-index?expand=1

[zehata]

Also yes this will naturally need to be split into multiple PRs

[leslieyip02]

It looks quite solid to me so far. I don't have any comments on the general implementation, the disambiguation logic and migration logic makes sense to me.

For the optimiser, I think this can be deprioritised since it's still unstable and in beta. We can revisit it once it's more stable. Otherwise, great work!

[zehata]

I think it will be better structure to store information such as lesson indices, module codes, etc..., since it is clearer to future maintainers that the values inside are unique, and the order of these data should not be relevant to their equality, However, this will also require quite a bit of added code to transform arrays as parsed from the API response JSON. What do yous think?

[leslieyip02]

Do you mean storing it on RawLesson? If so, I think maybe we could leave RawLesson alone since it mirrors the RawLesson used by the scraper.

The injected fields (module code, lesson index, etc) are only needed on the client side, so I think it's ok to have separate types.

[zehata]

@leslieyip02 Oh yes, what I meant is only on the client side. Though I am having second thoughts considering that it would be quite a hassle to transform the data parsed from state and back to arrays when storing the data.

[zehata]

@leslieyip02
I have been looking into this again: #3930 (comment)
Actually, aren't these 2 classes correct based on class number alone? They look like they are overlapping but they are actually the same class as you mentioned here: #4098 (reply in thread)

[zehata]

Ok, I'm done with old tests and coverage. Just merging conflicts now.

[zehata]

Not much conflicts. Working on JSDocs now.

[zehata]

Done: #4225