-
Notifications
You must be signed in to change notification settings - Fork 11
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
Requirement Migrations #874
Conversation
[diff-counting] Significant lines: 157. |
Visit the preview URL for this PR (updated for commit f13632a): https://cornelldti-courseplan-dev--pr874-requirement-migratio-9em0y109.web.app (expires Mon, 01 Jan 2024 15:58:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
…nerator to include all fields of migrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!! Can you add some more inline documentation? You'll need to remove "WIP" from the PR title for the pending check to pass.
Great work Nidhi! I think this is a great step for adding support to changing majors and minors. Documentation of this code looks great, I couldn't find any immediate problem with the code written. Excellent use of maps and filters. In the future have we considered adding support of this feature for changing college requirements? The ILR school changed their college requirements in 2022, it would be useful to be able to adopt similar method to encounter this case in the future. |
Looks really good Nidhi! I'm curious about what we want to do with the requirements migrations after everyone that the former requirement applies to graduates. They don't necessarily take up a lot of space, but we might want to think about cleaning them up in our annual update, or at least sorting them by year so we can easily keep track of which migrations exist and are applicable. |
Tested according to test plan and works great, well done Nidhi. Not doing a detailed review on account of all the feedback you received earlier. |
* Began working on migration structure with ECE core course requirements * formatting * All updates with requirement migrations but error with fieldOfStudyReqs function type * JSON Updated with decorated migrations - still need to change json generator to include all fields of migrations * Working requirement migrations with ECE core courses migrations * formatting * Maintained order of requirements when migrating, fixed format * Added helper * Added more inline comments
* Began working on migration structure with ECE core course requirements * formatting * All updates with requirement migrations but error with fieldOfStudyReqs function type * JSON Updated with decorated migrations - still need to change json generator to include all fields of migrations * Working requirement migrations with ECE core courses migrations * formatting * Maintained order of requirements when migrating, fixed format * Added helper * Added more inline comments
Summary
Test Plan
Notes
Documentation in Notion describing how to add requirement migrations: https://www.notion.so/courseplan/Requirement-Migrations-126ffdc4c2274e15a9e40bb8432b791b?pvs=4