Record and compute sub-requirement progress in a fine-grained way #344

SamChou19815 · 2021-03-04T21:00:50Z

Summary

Implemented the proposal in #343. Check that issue for the discussion on design decisions.

Test Plan

Added a test to ensure perSlotMinCount.length === checkers.length for all requirements,

Example showing A&S's PSB vs MQR requirement is correctly handled:

Engineering requirement is still correctly computed:

Notes

Info-sci concentrations are not done in this PR to avoid getting this PR too large.

dti-github-bot · 2021-03-04T21:01:12Z

[diff-counting] Significant lines: 930.

github-actions · 2021-03-04T21:03:43Z

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

https://cornelldti-courseplan-dev--pr344-fine-grained-sub-req-0np259g5.web.app

_{(expires Sat, 13 Mar 2021 21:41:12 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

hahnbeelee

see comments below
main concern:

modal showing that a course fulfills PBS/MQR when it is already fulfilled by another course.

hahnbeelee · 2021-03-06T02:07:19Z

OK - so I'm seeing why I was confused. If you already took 2 MQR courses & take a third: on the add modal it shows that it'll satisfy PBS & MQR (not as desired) but when you add the course it doesn't end up affecting the sidebar (as desired).

We should make it so that it doesn't show that it fulfills MQR or PBS on the add modal. How? Is there a way to check through the frontend that it will eventually not fulfill the requirement? Or is there a way to ensure that it won't show up at all from some calculation in the requirements logic? (I am asking these questions to scope if this is a problem I need to address or if it's better for u to address right now in this PR) Clearly, some computation is happening after the course is added instead of when the requirements are shown on the modal.

hahnbeelee · 2021-03-06T02:11:07Z

So, it seems as though now there is a hierarchy of requirement > sub-requirement > slot

Can you add definitions to all of these in the READ-ME in a future PR?

src/requirements/data/minors/dbme.ts

hahnbeelee · 2021-03-06T02:43:14Z

OK - so I'm seeing why I was confused. If you already took 2 MQR courses & take a third: on the add modal it shows that it'll satisfy PBS & MQR (not as desired) but when you add the course it doesn't end up affecting the sidebar (as desired).

We should make it so that it doesn't show that it fulfills MQR or PBS on the add modal. How? Is there a way to check through the frontend that it will eventually not fulfill the requirement? Or is there a way to ensure that it won't show up at all from some calculation in the requirements logic? (I am asking these questions to scope if this is a problem I need to address or if it's better for u to address right now in this PR) Clearly, some computation is happening after the course is added instead of when the requirements are shown on the modal.

maybe some more computation needs to happen in the requirement-frontend-computation.ts file?

hahnbeelee · 2021-03-06T02:48:59Z

Should I be able to run the tests somehow?

SamChou19815 · 2021-03-06T04:47:37Z

Should I be able to run the tests somehow?

@hahnbeelee npm run test

tcho6319

Thanks Sam! I left some comments about how the perSlotMinCount was determined for some of the reqs and requests for documentation in certain functions. The next sprint will involve the team looking through our requirements data and making sure it's as accurate as possible so perhaps more changes can be handled there, but I think some of the perSlotMinCount values were off. I also wanted to note an issue below:

I don't think the frontend is correctly handling the PBS and MQR courses requirement. It only registers the 3 slots and doesn't add another one to complete the requirement. Not sure if it would be best to address in this PR since it's getting large:

Screen.Recording.2021-03-05.at.2.31.49.PM.mov

src/requirements/requirement-frontend-computation.ts

src/requirements/data/majors/bme.ts

src/requirements/data/majors/bio.ts

src/requirements/data/checkers-common.ts

src/requirements/data/majors/bio.ts

src/requirements/data/majors/bme.ts

SamChou19815 · 2021-03-06T21:46:03Z

@tcho6319 I updated some incorrect requirement perSlotMinCount setup. They should be accurate now. It turns out the original setup is also wrong, but this setup now give them the expressive power to specify the requirement correctly now.

@hahnbeelee
In terms of the PBS/MQR requirement, we do need additional work to make them right. However, I think this PR is a good start. To ensure the add modal doesn't report that you can still use a MQR course to fulfill the requirement when all the slots are already used up, we can use the computeFulfillmentCoursesAndStatistics function to compute a new statistics after the new course is added, and only allow if the new stat is greater than the old stat.

@tcho6319
In terms of the 3 slot vs 4 slot issue in the PBS/MQR requirement, I believe we should actually use 3 slots, since logically there are 3 slots. Currently, it's slightly broken because the frontend code assumes that 1 slot can only be filled by 1 course. This is something we might want to change. Loop in @einc for potential design work.

hahnbeelee

gotcha, sounds good then!

einc · 2021-03-07T06:19:51Z

For the "3 slot vs 4 slot issue in the PBS/MQR requirement", I thought there were supposed to be 4 slots also?
Two for PBS classes, One for MQR, and One for MQR or PBS?

Is there only 3 slots right now because we adding one MQR class counts for both the MQR AND the (MQR or PBS) slot? Is the problem that we currently can't differentiate the (MQR or PBS) sub-requirement from the PBS and MQR subrequirements?

SamChou19815 · 2021-03-07T16:15:59Z

For the "3 slot vs 4 slot issue in the PBS/MQR requirement", I thought there were supposed to be 4 slots also?

Two for PBS classes, One for MQR, and One for MQR or PBS?

Is there only 3 slots right now because we adding one MQR class counts for both the MQR AND the (MQR or PBS) slot? Is the problem that we currently can't differentiate the (MQR or PBS) sub-requirement from the PBS and MQR subrequirements?

@einc It's currently 3 slots because the slots are PSB: 2 courses, MQR: 1 course, PSB/MQR: 1 course.

tcho6319

LGTM. Thanks Sam!

@tcho6319 In terms of the 3 slot vs 4 slot issue in the PBS/MQR requirement, I believe we should actually use 3 slots, since logically there are 3 slots. Currently, it's slightly broken because the frontend code assumes that 1 slot can only be filled by 1 course. This is something we might want to change. Loop in @einc for potential design work.

I think this frontend code fix should be done in a separate PR. I will make a Story item for it on Notion.

SamChou19815 requested a review from a team as a code owner March 4, 2021 21:00

SamChou19815 linked an issue Mar 4, 2021 that may be closed by this pull request

[RFC] Fine-grained sub-requirement progress calculation #343

Closed

SamChou19815 force-pushed the fine-grained-sub-req-progress branch from f53c11f to 2cc309b Compare March 4, 2021 21:02

SamChou19815 added the large label Mar 4, 2021

SamChou19815 force-pushed the fine-grained-sub-req-progress branch from 2cc309b to 50ed657 Compare March 4, 2021 21:29

hahnbeelee suggested changes Mar 6, 2021

View reviewed changes

hahnbeelee reviewed Mar 6, 2021

View reviewed changes

src/requirements/data/minors/dbme.ts Show resolved Hide resolved

tcho6319 requested changes Mar 6, 2021

View reviewed changes

SamChou19815 force-pushed the fine-grained-sub-req-progress branch from 50ed657 to 700164a Compare March 6, 2021 21:32

Record and compute sub-requirement progress in a fine-grained way

1896f55

SamChou19815 force-pushed the fine-grained-sub-req-progress branch from 700164a to 1896f55 Compare March 6, 2021 21:38

SamChou19815 requested review from hahnbeelee and tcho6319 March 6, 2021 21:46

SamChou19815 self-assigned this Mar 6, 2021

hahnbeelee approved these changes Mar 6, 2021

View reviewed changes

tcho6319 approved these changes Mar 7, 2021

View reviewed changes

SamChou19815 merged commit d6b9884 into master Mar 7, 2021

SamChou19815 deleted the fine-grained-sub-req-progress branch March 7, 2021 20:01

SamChou19815 mentioned this pull request Mar 7, 2021

Consider sub-requirement progress in add modal choices #351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record and compute sub-requirement progress in a fine-grained way #344

Record and compute sub-requirement progress in a fine-grained way #344

SamChou19815 commented Mar 4, 2021 •

edited

Loading

dti-github-bot commented Mar 4, 2021 •

edited

Loading

github-actions bot commented Mar 4, 2021 •

edited

Loading

hahnbeelee left a comment •

edited

Loading

hahnbeelee commented Mar 6, 2021 •

edited

Loading

hahnbeelee commented Mar 6, 2021

hahnbeelee commented Mar 6, 2021

hahnbeelee commented Mar 6, 2021

SamChou19815 commented Mar 6, 2021

tcho6319 left a comment •

edited

Loading

SamChou19815 commented Mar 6, 2021

hahnbeelee left a comment

einc commented Mar 7, 2021

SamChou19815 commented Mar 7, 2021

tcho6319 left a comment

Record and compute sub-requirement progress in a fine-grained way #344

Record and compute sub-requirement progress in a fine-grained way #344

Conversation

SamChou19815 commented Mar 4, 2021 • edited Loading

Summary

Test Plan

Notes

dti-github-bot commented Mar 4, 2021 • edited Loading

github-actions bot commented Mar 4, 2021 • edited Loading

hahnbeelee left a comment • edited Loading

Choose a reason for hiding this comment

hahnbeelee commented Mar 6, 2021 • edited Loading

hahnbeelee commented Mar 6, 2021

hahnbeelee commented Mar 6, 2021

hahnbeelee commented Mar 6, 2021

SamChou19815 commented Mar 6, 2021

tcho6319 left a comment • edited Loading

Choose a reason for hiding this comment

SamChou19815 commented Mar 6, 2021

hahnbeelee left a comment

Choose a reason for hiding this comment

einc commented Mar 7, 2021

SamChou19815 commented Mar 7, 2021

tcho6319 left a comment

Choose a reason for hiding this comment

SamChou19815 commented Mar 4, 2021 •

edited

Loading

dti-github-bot commented Mar 4, 2021 •

edited

Loading

github-actions bot commented Mar 4, 2021 •

edited

Loading

hahnbeelee left a comment •

edited

Loading

hahnbeelee commented Mar 6, 2021 •

edited

Loading

tcho6319 left a comment •

edited

Loading