Skip to content
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.

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

Fix TA mode for lecture lessons #3930

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

Conversation

leslieyip02
Copy link
Contributor

Context

Fixes #3929.

Implementation

Previously, TA mode will filter out all lectures. However, some mods (e.g. CS2103T) only have lecture lessons, so this PR makes all lessons togglable.

TaModulesConfig now stores [lessonType, classNo, startTime, day] instead of [lessonType, classNo].

export type TaModulesConfig = {
  [moduleCode: ModuleCode]: [
    lessonType: LessonType,
    classNo: ClassNo,
    startTime: StartTime,
    day: DayText,
  ][];
};

For the migration, I'm not sure on how startTime and day data can be pulled. Right now, I am wiping the existing TA config to avoid conflicts. Let me know if there's a better way to do this?

Hover icon should only appear if the cell can be modified
Some modules (e.g. CS2103T) map multiple lessons of the same type to a single class number. startTime and day allows each lesson slot to be uniquely identified.
Copy link

vercel bot commented Jan 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 2:31pm

Copy link

vercel bot commented Jan 3, 2025

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

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 35.84906% with 34 lines in your changes missing coverage. Please review.

Project coverage is 54.68%. Comparing base (988c6fd) to head (e56d83f).
Report is 59 commits behind head on master.

Files with missing lines Patch % Lines
website/src/views/timetable/TimetableContent.tsx 0.00% 22 Missing ⚠️
website/src/utils/timetables.ts 62.50% 6 Missing ⚠️
website/src/reducers/timetables.ts 55.55% 4 Missing ⚠️
website/src/views/timetable/TimetableCell.tsx 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3930      +/-   ##
==========================================
+ Coverage   54.52%   54.68%   +0.16%     
==========================================
  Files         274      276       +2     
  Lines        6076     6327     +251     
  Branches     1455     1548      +93     
==========================================
+ Hits         3313     3460     +147     
- Misses       2763     2867     +104     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +27 to +32
[moduleCode: ModuleCode]: [
lessonType: LessonType,
classNo: ClassNo,
startTime: StartTime,
day: DayText,
][];
Copy link
Member

@zwliew zwliew Jan 4, 2025

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
Contributor 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
Contributor 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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TA Mode doesn't work for some courses
2 participants