Add to the 120 credits on repeated courses #280
Conversation
|
[diff-counting] Significant lines: 145. |
Resolve #141. Implemented according to my proposal during the meeting: - Lifting the total credits requirement out of the generated requirement json and special case it on frontend - As a result, it will not become part of requirement graph as desired. - In addition, the requirement json dropped from 2.5M to 800k!
SamChou19815
force-pushed
the
repeated-courses-for-total-credits
branch
from
291307b to
87fc51d
Compare
| @@ -9,7 +9,6 @@ Array [ | |||
| "College-AG-Physical and Life Sciences", | |||
| "College-AG-Quantitative Literacy", | |||
| "College-AG-Social Sciences and Humanities", | |||
| "College-AG-Total Academic Credits", | |||
There was a problem hiding this comment.
snapshot is updated as expected, since there are no longer part of the requirement json any more. Since it's before launch, no migration is necessary. For more background, see #222
|
Visit the preview URL for this PR (updated for commit 916f68e): https://cornelldti-courseplan-dev--pr280-repeated-courses-for-43nh79v5.web.app (expires Sat, 20 Feb 2021 01:49:03 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
There was a problem hiding this comment.
Thank you so much Sam! I requested some smaller changes but everything looks mostly good. To note: I'm not sure if we can say that #141 would be completely resolved by this PR though, because the issue discusses how if the user repeats a course, the min total number of academic credits would increase by the number of credits of that course. Though the credits of repeated courses are taken into account for the total academic credits, the number still remains at 120.
| const requirementCommon = { | ||
| sourceType: 'College', | ||
| sourceSpecificName: college, | ||
| name: 'Total Academic Credits', |
There was a problem hiding this comment.
Would the name field be how the frontend would special case the 120 total academic credits?
There was a problem hiding this comment.
No, it's handled by completely remove them from the requirement json, and inject it as the first college requirement in the requirement menu.
@tcho6319 I'm not sure if I understand the question. Is there an counterexample that shows this PR doesn't solve everything in that issue. |
For example, since I have CS 1110 twice in my schedule, that would mean that I repeated the course. Based on the policy re: repeated courses, the total number of academic credits that I would need to graduate should increase from 120 to 124 because CS 1110 is 4 credits. In the screenshot, we see that the minimum number of credits remains 120. Also, this is more related to the second point about 10xx level courses, but the required number of credits I need to graduate is 121 total academic credits because I took ENGRG 1094 (a supplemental class for lin alg), which is 1 credit. |
There was a problem hiding this comment.
LGTM! Thanks Sam. I shared some examples of why I think this PR might not fully address #141 above but I think this PR is good on its own. I think tackling changing the Total Academic Credits minimum might be better handled in a separate PR.
|
It turns out that handling the increase isn't hard, so I just included it there. |
Summary
Resolve #141.
Implemented according to my proposal during the meeting:
Test Plan
Repeated courses now correctly count towards 120 credits:
AAP: no college req as expected.
A&S: correctly counted
Business school: correctly counted
CALS: correctly counted
Engineering: no such requirement as expected
Human ecology: correctly counted
ILR: correctly counted