Fix engineering liberal arts incorrect count issue

[SamChou19815]

Summary

Note: the requirement graph logic is solid. However, the algo to put courses into slots have some issues.

We still have some issue with perSlotMinCount logic. When minNumberOfSlots is not null, we cannot assume that each slot can be filled with at most one course. For example, consider the engineering liberal arts requirement where you have 6 courses. Right now we have 13 slots representing 13 different categories. It's perfectly fine to have multiple courses into one category. e.g. two SBA courses are OK. Therefore I tweaked the logic to take the min of the slot and the progress in that slot.

I also changed the minCount of each slot to be 6 for engineering. There the per slot minCount isn't meaning the min count. It's just a cap of how many courses can be put there. Therefore, it's good to set a high slot count to work for cases when we have multiple SBA courses.

Test Plan

After this change, I can finally fulfill all of my engineering and CS requirements:

[dti-github-bot]

[diff-counting] Significant lines: 7.

[github-actions]

Visit the preview URL for this PR (updated for commit 6162432):

https://cornelldti-courseplan-dev--pr412-fix-engineering-libe-2wy5jgv5.web.app

_{(expires Wed, 07 Apr 2021 03:59:13 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[SamChou19815]

@abcdefguan can you help confirm whether it fixes the linked issue?

Update: I don't #399 is related to this. It seems to be the same issue with AP/IB credits.

[willespencer]

Tested by adding all of my liberal studies courses, and I now able to completely fulfill the 2 liberal studies requirements (which I cannot do on the web app) 😄 ! However, the 70+ slots is really difficult to scroll through when the distribution req is expanded, is there any way to avoid showing all of them?

Also just wanted to say it's really exciting that we can now actually finish our engineering/cs requirements 😁

[tcho6319]

LGTM thanks Sam! This does fix the incorrect counting issue, which should be prioritized for launch. Post launch, I think we should def revisit the logic/display for putting courses into the right slots (esp multiple courses for a single slot). Since we're making the perSlotMinCount be 6 for each of the 13 slots, we get 78 different physical slots on the frontend, which could be overwhelming for the user.

On the frontend side of things, is there a way to at first show the 13 physical (i.e. frontend) slots

for each of the 13 distribution categories? Then while progress < perSlotMinCount[index], when progress increases (e.g. Course 2 physical slot is now completed)

we create another incomplete physical slot for that category.

Would this be feasible pre-launch?

+1. if it's not feasible pre-launch, i think we might want to make it temp 1 for the slots (even if it means less accurate requirements) bc i think the 78 slots will really confuse new users.
-ein

[SamChou19815]

@einc Actually I think the change in this PR is wrong, and we are simply writing the data in incorrect ways. In #407 you seem to merge the 6-course and 3-category requirement into one to cause the problem first... The original data actually looks quite reasonable to me. Is there a reason you changed it?

Ahh should I just change the liberal studies back into the original requirements? [I was trying to merge them bc ppl were saying the text was redundant and I couldn't fulfill all the liberal study courses and a different requirement. For example, INFO 1200 fulfills liberal studies and engineering communication but I couldn't do that previously]

[SamChou19815]

Closing since this is the wrong fix and the correct fix is already merged in #420.