Requirement builder with the new data format #588
Conversation
|
[diff-counting] Significant lines: 655. |
|
Visit the preview URL for this PR (updated for commit 8373560): https://cornelldti-courseplan-dev--pr588-requirement-graph-bu-lxmxr55u.web.app (expires Fri, 10 Dec 2021 17:00:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
SamChou19815
force-pushed
the
requirement-graph-builder
branch
from
3f6b9c0 to
59616ae
Compare
SamChou19815
force-pushed
the
delete-unused-ap-ib-override
branch
from
cc36672 to
3a671c8
Compare
SamChou19815
force-pushed
the
requirement-graph-builder
branch
from
59616ae to
8b47015
Compare
SamChou19815
force-pushed
the
delete-unused-ap-ib-override
branch
from
3a671c8 to
64422dd
Compare
SamChou19815
force-pushed
the
requirement-graph-builder
branch
from
8b47015 to
fe125f1
Compare
SamChou19815
force-pushed
the
delete-unused-ap-ib-override
branch
from
64422dd to
a4728a9
Compare
SamChou19815
force-pushed
the
requirement-graph-builder
branch
from
fe125f1 to
64747ee
Compare
SamChou19815
force-pushed
the
requirement-graph-builder
branch
from
64747ee to
7265ffa
Compare
|
When I dragged CS 4121 for the CS Practicum requirement it fulfilled both the CS Elective and CS Practicum requirement. This doesn't happen on prod. |
|
When I added a course from the add modal from the req bar for tech elec it got counted as tech elec and cs elec
|
|
Weird bug where I added CS 3300 as an external spec from the req bar, but it didn't actually get added? The course card isn't in the semester view, and it isn't being counted for actually fulfilling the requirement (0/3) I refreshed and it completely disappeared. Did it again and it was fine. Maybe something weird just happened |
|
Fulfilled course cards are not showing for self-select (specifically the CS major tech elec) - notice 2/3 but no courses listed |
|
local vs. prod. |
| @@ -90,7 +90,7 @@ export const addCourseToSemester = ( | |||
| year: number, | |||
| season: FirestoreSemesterSeason, | |||
| newCourse: FirestoreSemesterCourse, | |||
| requirementID?: string, | |||
| choiceUpdater: (choice: FirestoreCourseOptInOptOutChoices) => FirestoreCourseOptInOptOutChoices, | |||
There was a problem hiding this comment.
How does choiceUpdater work?
There was a problem hiding this comment.
It's a function that takes in the current choice as parameter and return the updated choice.
### Fix dragged-from-req-bar courses double counting issue: Filtering out from a full list of all potentially eligible requirements instead of filter from nothing. ### Self-check double counting issue Same as above, need to add all eligible courses to opt-out to prevent double counting. ### Fulfilled self-check course not showing up Use requirement graph instead of now unused `derivedSelectableRequirementData` as data source to find fulfilled courses. ### Self-check choices not showing up in add modal
|
@hahnbeelee all the above-mentioned issues should be fixed now. How I fixed them is explain in the commit message. |
@hahnbeelee Should be fixed in the last commit. Turns out a filter condition is inverted 😅 |
| @@ -3,7 +3,7 @@ import { checkNotNull } from '../../utilities'; | |||
| import { | |||
| semestersCollection, | |||
| toggleableRequirementChoicesCollection, | |||
| selectableRequirementChoicesCollection, | |||
There was a problem hiding this comment.
Are we completely deprecating selectableRequirementChoicesCollection?
There was a problem hiding this comment.
if so should we delete it from firebase-admin-config.ts and store.ts?
There was a problem hiding this comment.
All migrated away
Summary
Depends on #587.
This PR switches the requirement building algorithm to use the new data format proposed in #522. It only builds the first graph (the graph that keeps all the edges with constraint violations) for now, since the current data enforces that there are no constraint violations.
Test Plan
Load it the first time and compare your requirement progress against staging. It should be mostly the same. There might be some discrepancies with edges from requirement with warnings to AP/IB credits, since the new implementation fixes the bug in the old system (See #553 (comment) when I first write the migration script).
After that, you can play around with adding courses and requirements from both the semester view and the requirement bar, with both regular and self-check requirements. Everything should work fine.
I would like to get more eyes on this since it's a pretty significant change.