fix: implement correct grouping for engineering liberal studies #962
Conversation
|
[diff-counting] Significant lines: 75. |
|
Visit the preview URL for this PR (updated for commit 089c723): https://cornelldti-courseplan-dev--pr962-fix-eng-lib-studies-8ds9adrf.web.app (expires Fri, 03 Jan 2025 05:48:16 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
There was a problem hiding this comment.
Hey Eric, good work catching this consistency issue. Hopefully not too many existing Engineering users were affected! There were a couple of code style things I commented on, but mainly nitpicking — great job and should be good to merge (ping or message me) once changes are in place.
| ]; | ||
| const engineeringLiberalArtsGroups: Record<string, string[]> = { | ||
| 'Group 1': [ | ||
| 'CA-AAP', |
There was a problem hiding this comment.
Hmmm I'm not sure if it is strictly necessary to go and add in the content after the hyphen for all of these categories. hasCategory checks for a substring match, and all the CAs are in Group 1, all the HAs in Group 2 ... (they are disjoint sets) so such granularity might not be needed. @nidhi-mylavarapu not sure if you have any thoughts?
There was a problem hiding this comment.
I tried the groups without the after-hyphen content and it didn't filter correctly...not sure if it's something else though?
There was a problem hiding this comment.
Alright we can probably stick with just this then, I don't anticipate the categories changing much.
Summary
Currently, the engineering liberal studies requirement is represented incorrectly on CoursePlan. For the "courses from three separate groups" requirement listed on the liberal studies policy, it's currently set up such that courses from any three separate liberal studies categories (not groups) check off the requirement. For example, adding SHUM 2208 (LA), ARTH 2600 (CA), and AMST 3981 (ALC) checks off the requirement, despite LA, CA, and ALC all being members of "Group 1" and thus only counting for one of the three groups under the requirement.
This pull request implements the liberal studies group structure to correct this (I'm not sure if I implemented it correctly though, there seems to be many more line deletions than additions in the requirements json). Currently, the code checks all categories (LA, CA, ALC, HA, etc.) separately.
In addition, the liberal studies groups were rearranged for Cornell engineers entering in Fall 2024, so I will make another PR soon to implement a migration for that.
CC @nidhi-mylavarapu