Skip to content

Fix TA mode for lecture lessons #3930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix TA mode for lecture lessons #3930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1910c07
Fix hover icon
leslieyip02 Jan 3, 2025
9438551
Make lectures TA-able
leslieyip02 Jan 3, 2025
3af7817
Add startTime and day to TaModulesConfig
leslieyip02 Jan 3, 2025
bc6eb5b
Update serialization
leslieyip02 Jan 3, 2025
ddec499
Update export types
leslieyip02 Jan 3, 2025
e56d83f
Fix tests
leslieyip02 Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions export/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,24 @@ export type ThemeState = Readonly<{
}>;
export type ColorScheme = 'LIGHT_COLOR_SCHEME' | 'DARK_COLOR_SCHEME';
export type Semester = number;
export type ClassNo = string; // E.g. "1", "A"
export type LessonType = string; // E.g. "Lecture", "Tutorial"
export type ModuleCode = string; // E.g. "CS3216"
export type LessonType = string; // E.g. "Lecture", "Tutorial"
export type ClassNo = string; // E.g. "1", "A"
export type StartTime = string;
export type DayText = string;
export type SemTimetableConfig = {
[moduleCode: ModuleCode]: ModuleLessonConfig;
};
export interface ModuleLessonConfig {
[lessonType: LessonType]: ClassNo;
}
export type TaModulesConfig = {
[moduleCode: ModuleCode]: [lessonType: LessonType, classNo: ClassNo][];
[moduleCode: ModuleCode]: [
lessonType: LessonType,
classNo: ClassNo,
startTime: StartTime,
day: DayText,
][];
Comment on lines +27 to +32
Copy link
Member

@zwliew zwliew Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TaModulesConfig is starting to get quite big. I'm starting to think that it would be better to store this state differently. With this PR the state would look like:

timetable: {
	lessons: {
		1: {
			// note: the "CS2103T" keyed object is completely useless since the user is a TA for CS2103T.
			CS2103T: {
				Lecture: "G01"
			}
		}
	},
	ta: {
		1: {
			CS2103T: [["Lecture", "G01", "0800", "Monday"]]
		}
	}
}

Instead, we could have addTaLesson actions overwrite the "lessons" keyed object like so:

timetable: {
	lessons: {
		1: {
			CS2103T: {
				// note: values are now arrays of arrays.
				Lecture: [["G01", "0800", "Monday"]]
			}
		}
	},
	ta: {
		1: ["CS2103T"]
	}
}

Note that we would have to change the type of the values for ModuleLessonConfig to an array.
Coincidentally (or not), this change causes the state schema for "ta" and "hidden" to match -- they are both arrays of ModuleCodes.

For readability, we can go even further and replace ["G01", "0800", "Monday"] with:

{
	classNo: "G01",
	startTime: "0800",
	day: "Monday"
}

Hence, the resulting state would look like:

timetable: {
	lessons: {
		1: {
			CS2103T: {
				Lecture: [
					{
						classNo: "G01",
						startTime: "0800",
						day: "Monday"
					}
				]
			}
		}
	},
	ta: {
		1: ["CS2103T"]
	}
}

As for the serialized state, it changes from CS2103T=LEC:G01&ta=CS2103T(LEC:G01:0800:Monday) to CS2103T=LEC:G01:0800:Monday&ta=CS2103T.

State transitions should be pretty straightforward in theory. When switching from TA to non-TA mode, we should also pick the first lesson for each type and discard the rest.

Let me know what you think, and if you'd like to work on this! This would be a pretty big change since it modifies the existing schema for modules, so feel free to break it up into more easily reviewable chunks, i.e.

  1. Modify the schema for modules to accept an array of lessons, along with startTime and day
  2. Modify the schema for TaModuleConfig

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think it makes sense to update the schema. I'll start working on those PRs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing I'm not sure how to handle is that there are some mods with lessons with identical startTime, endTime and day (e.g. HSI1000). The only thing differentiating the lessons are the weeks and venue. In this case, should we add weeks/venue to the schema as well? I feel that it would be introducing bloat for such a rare scenario though.

Copy link
Member

@zwliew zwliew Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a problem. Can we hold off on the addition of startTime and endTime and any other distinguishing identifiers for now? Let's make it possible to add lectures to TA in a separate PR, and spend some more time figuring out what we really need to make this feature possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that makes sense to me.

Copy link
Member Author

@leslieyip02 leslieyip02 Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, I'll also try to look for the edge cases that aren't covered so we can try to figure out what the minimum identifying info is:

  • Same classNo, different days: CG1111A
  • Same classNo, same day, start time and end time, different weeks: HSI1000
  • Same classNo, same lesson type at different times: CS2103T
  • 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

So far, it looks like we need day, start time, end time, weeks and lesson type.

I think weeks might be a problem for serialization because it can either be an array or JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But TBH if the lesson indices do changes, the timetable would need to be manually updated by the user anyways since new lesson options are available/existing lessons have changed/been removed.

Yeah... Furthermore, currently things fail silently, so I think it would make sense to notify users when any discrepancies are found. It would be a separate issue though

We would just need to handle the case where the index is out-of-bounds.

Yeah, that's part of the validation. currently I am handling it naively such that when it finds a non-valid lesson in normal mode it would also try to recover by finding lessons with the same classNo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leslieyip02 For the 4th example, suppose there were 2 classes, 4 lessons

LEC:01 <- currently selected
TUT:01 <- currently selected
LEC:02
TUT:02

Do we need to make it such selecting LEC:02 will also switch the tutorial lesson type to TUT:02?

Copy link
Contributor

@zehata zehata Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zwliew @leslieyip02 From #3930 (comment)

When switching from TA to non-TA mode, we should also pick the first lesson for each type and discard the rest.

I think it would be better to take into account the lessons that the user currently have selected in TA mode, it's also not too difficult to implement

Recording.2025-07-09.192301.1.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zwliew @leslieyip02 Since this discussion has veered from the original PR review, I have created a discussion which I think would be a more appropriate place to talk about the proposed changes and discuss the implementation
#4098

};

// `ExportData` is duplicated from `website/src/types/export.ts`.
Expand Down
18 changes: 15 additions & 3 deletions website/src/actions/timetables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@ import type {
} from 'types/timetables';
import type { Dispatch, GetState } from 'types/redux';
import type { ColorMapping } from 'types/reducers';
import type { ClassNo, LessonType, Module, ModuleCode, Semester } from 'types/modules';
import type {
ClassNo,
DayText,
LessonType,
Module,
ModuleCode,
Semester,
StartTime,
} from 'types/modules';

import { fetchModule } from 'actions/moduleBank';
import { openNotification } from 'actions/app';
Expand Down Expand Up @@ -279,10 +287,12 @@ export function addTaLessonInTimetable(
moduleCode: ModuleCode,
lessonType: LessonType,
classNo: ClassNo,
startTime: StartTime,
day: DayText,
) {
return {
type: ADD_TA_LESSON_IN_TIMETABLE,
payload: { semester, moduleCode, lessonType, classNo },
payload: { semester, moduleCode, lessonType, classNo, startTime, day },
};
}

Expand All @@ -292,10 +302,12 @@ export function removeTaLessonInTimetable(
moduleCode: ModuleCode,
lessonType: LessonType,
classNo: ClassNo,
startTime: StartTime,
day: DayText,
) {
return {
type: REMOVE_TA_LESSON_IN_TIMETABLE,
payload: { semester, moduleCode, lessonType, classNo },
payload: { semester, moduleCode, lessonType, classNo, startTime, day },
};
}

Expand Down
4 changes: 2 additions & 2 deletions website/src/reducers/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const exportData: ExportData = {
},
hidden: ['PC1222'],
ta: {
CS1010S: [['Tutorial', '1']],
CS1010S: [['Tutorial', '1', '0900', 'Monday']],
},
theme: {
id: 'google',
Expand Down Expand Up @@ -79,7 +79,7 @@ test('reducers should set export data state', () => {
hidden: { [1]: ['PC1222'] },
ta: {
[1]: {
CS1010S: [['Tutorial', '1']],
CS1010S: [['Tutorial', '1', '0900', 'Monday']],
},
},
academicYear: expect.any(String),
Expand Down
28 changes: 20 additions & 8 deletions website/src/reducers/timetables.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,28 +129,37 @@ describe('hidden module reducer', () => {
describe('TA module reducer', () => {
const withTaModules: TimetablesState = {
...initialState,
ta: { [1]: { CS1010S: [['Tutorial', '1']] }, [2]: { CS1010S: [['Tutorial', '1']] } },
ta: {
[1]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
[2]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
},
};

test('should update TA modules', () => {
expect(
reducer(initialState, addTaLessonInTimetable(1, 'CS3216', 'Tutorial', '1')),
).toHaveProperty('ta.1', { CS3216: [['Tutorial', '1']] });
reducer(initialState, addTaLessonInTimetable(1, 'CS3216', 'Tutorial', '1', '0900', 'Monday')),
).toHaveProperty('ta.1', { CS3216: [['Tutorial', '1', '0900', 'Monday']] });

expect(
reducer(initialState, removeTaLessonInTimetable(1, 'CS1010S', 'Tutorial', '1')),
reducer(
initialState,
removeTaLessonInTimetable(1, 'CS1010S', 'Tutorial', '1', '0900', 'Monday'),
),
).toMatchObject({
ta: {
[1]: {},
},
});

expect(
reducer(withTaModules, removeTaLessonInTimetable(1, 'CS1010S', 'Tutorial', '1')),
reducer(
withTaModules,
removeTaLessonInTimetable(1, 'CS1010S', 'Tutorial', '1', '0900', 'Monday'),
),
).toMatchObject({
ta: {
[1]: {},
[2]: { CS1010S: [['Tutorial', '1']] },
[2]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
},
});
});
Expand All @@ -160,14 +169,17 @@ describe('TA module reducer', () => {
reducer(
{
...initialState,
ta: { [1]: { CS1010S: [['Tutorial', '1']] }, [2]: { CS1010S: [['Tutorial', '1']] } },
ta: {
[1]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
[2]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
},
},
removeModule(1, 'CS1010S'),
),
).toMatchObject({
ta: {
[1]: {},
[2]: { CS1010S: [['Tutorial', '1']] },
[2]: { CS1010S: [['Tutorial', '1', '0900', 'Monday']] },
},
});
});
Expand Down
26 changes: 21 additions & 5 deletions website/src/reducers/timetables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,18 @@
// eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain
_persist: state?._persist!,
}),
3: (state) => ({

Check warning on line 52 in website/src/reducers/timetables.ts

View check run for this annotation

Codecov / codecov/patch

website/src/reducers/timetables.ts#L52 

Added line #L52 was not covered by tests
...state,
ta: {},
// FIXME: Remove the next line when _persist is optional again.
// Cause: https://github.com/rt2zz/redux-persist/pull/919
// Issue: https://github.com/rt2zz/redux-persist/pull/1170
// eslint-disable-next-line no-underscore-dangle, @typescript-eslint/no-non-null-assertion, @typescript-eslint/no-non-null-asserted-optional-chain
_persist: state?._persist!,
}),
}),
/* eslint-enable */
version: 2,
version: 3,

// Our own state reconciler archives old timetables if the acad year is different,
// otherwise use the persisted timetable state
Expand Down Expand Up @@ -190,20 +199,27 @@

switch (action.type) {
case ADD_TA_LESSON_IN_TIMETABLE: {
const { moduleCode, lessonType, classNo } = action.payload;
const { moduleCode, lessonType, classNo, startTime, day } = action.payload;
if (!(moduleCode && lessonType && classNo)) return state;
// Prevent duplicate lessons
if (moduleCode in state) {
const isDuplicate = state[moduleCode].some((existingConfig) =>
isEqual(existingConfig, [lessonType, classNo, startTime, day]),

Check warning on line 207 in website/src/reducers/timetables.ts

View check run for this annotation

Codecov / codecov/patch

website/src/reducers/timetables.ts#L206-L207 

Added lines #L206 - L207 were not covered by tests
);
if (isDuplicate) return state;

Check warning on line 209 in website/src/reducers/timetables.ts

View check run for this annotation

Codecov / codecov/patch

website/src/reducers/timetables.ts#L209 

Added line #L209 was not covered by tests
}
return {
...state,
[moduleCode]: [...(state[moduleCode] ?? []), [lessonType, classNo]],
[moduleCode]: [...(state[moduleCode] ?? []), [lessonType, classNo, startTime, day]],
};
}
case REMOVE_TA_LESSON_IN_TIMETABLE: {
const { moduleCode, lessonType, classNo } = action.payload;
const { moduleCode, lessonType, classNo, startTime, day } = action.payload;
if (!(moduleCode && lessonType && classNo)) return state;
return {
...state,
[moduleCode]: state[moduleCode]?.filter(
(lesson) => !isEqual(lesson, [lessonType, classNo]),
(lesson) => !isEqual(lesson, [lessonType, classNo, startTime, day]),
),
};
}
Expand Down
22 changes: 19 additions & 3 deletions website/src/types/timetables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import { ClassNo, LessonType, ModuleCode, ModuleTitle, RawLesson } from './modules';
import {
ClassNo,
DayText,
LessonType,
ModuleCode,
ModuleTitle,
RawLesson,
StartTime,
} from './modules';

// ModuleLessonConfig is a mapping of lessonType to ClassNo for a module.
export type ModuleLessonConfig = {
Expand All @@ -11,8 +19,14 @@ export type SemTimetableConfig = {
};

// TaModulesConfig is a mapping of moduleCode to the TA's lesson types.
// startTime and day are needed since some modules (e.g. CS2103T) map a single classNo to multiple lessons.
export type TaModulesConfig = {
[moduleCode: ModuleCode]: [lessonType: LessonType, classNo: ClassNo][];
[moduleCode: ModuleCode]: [
lessonType: LessonType,
classNo: ClassNo,
startTime: StartTime,
day: DayText,
][];
};

// ModuleLessonConfigWithLessons is a mapping of lessonType to an array of Lessons for a module.
Expand Down Expand Up @@ -67,9 +81,11 @@ export type TimetableArrangement = {
// Represents the lesson which the user is currently hovering over.
// Used to highlight lessons which have the same classNo
export type HoverLesson = {
readonly classNo: ClassNo;
readonly moduleCode: ModuleCode;
readonly lessonType: LessonType;
readonly classNo: ClassNo;
readonly startTime: StartTime;
readonly day: DayText;
};

export type ColorIndex = number;
2 changes: 1 addition & 1 deletion website/src/utils/ical.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ describe(iCalForTimetable, () => {
CS3216,
};
const actual = iCalForTimetable(1, mockTimetable, moduleData, [], {
CS1010S: [['Tutorial', '1']],
CS1010S: [['Tutorial', '1', '0800', 'Monday']],
});
// 5 lesson types for cs1010s, 1 for cs3216 (1 exam for cs1010s will be excluded)
expect(actual).toHaveLength(6);
Expand Down
12 changes: 9 additions & 3 deletions website/src/utils/timetables.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ test('hydrateTaModulesConfigWithLessons should replace ClassNo with lessons', ()
const modules: ModulesMap = { [moduleCode]: CS1010S };
const taModules: TaModulesConfig = {
[moduleCode]: [
['Tutorial', '1'],
['Tutorial', '8'],
['Recitation', '4'],
['Tutorial', '1', '0900', 'Monday'],
['Tutorial', '8', '1600', 'Monday'],
['Recitation', '4', '1700', 'Thursday'],
],
};

Expand All @@ -120,8 +120,14 @@ test('hydrateTaModulesConfigWithLessons should replace ClassNo with lessons', ()
sem,
);
expect(configWithLessons[moduleCode].Tutorial[0].classNo).toBe('1');
expect(configWithLessons[moduleCode].Tutorial[0].startTime).toBe('0900');
expect(configWithLessons[moduleCode].Tutorial[0].day).toBe('Monday');
expect(configWithLessons[moduleCode].Tutorial[1].classNo).toBe('8');
expect(configWithLessons[moduleCode].Tutorial[1].startTime).toBe('1600');
expect(configWithLessons[moduleCode].Tutorial[1].day).toBe('Monday');
expect(configWithLessons[moduleCode].Recitation[0].classNo).toBe('4');
expect(configWithLessons[moduleCode].Recitation[0].startTime).toBe('1700');
expect(configWithLessons[moduleCode].Recitation[0].day).toBe('Thursday');
expect(configWithLessons[moduleCode]).not.toHaveProperty('Lecture');
});

Expand Down
Loading