Update exam data schema, add course ID to each fulfillment option #584

benjamin-shen · 2021-11-12T02:48:50Z

Summary

This pull request is task 1 in the AP/IB Infra Update #576.

updated exam data schema and exams taken type
deprecated old types and functions
implemented function to generate exam courses
updated tests
removed NO_EQUIVALENT_COURSES_COURSE_ID from requirements json

Remaining TODOs:

update variable name NO_EQUIVALENT_COURSES_COURSE_ID => NO_FULFILLMENT_COURSE_ID
refactor 'AP' | 'IB' everywhere as ExamType

Test Plan

npm run test exam-credit (or check CI)
Also check that AP/IB exam functionality is unchanged.

Notes

The initial plan was to give each exam a single course ID.

After the updated schema, the new plan implemented in this PR is to give each fulfillment option a course ID. (Each exam so far has 1-2 fulfillment options that are chosen based on the user's score.) This is so we don't need to encode the fulfillment option logic into the requirements algorithm past the exam course generation.

To handle more nuanced criteria (such as majors excluded, college-specific equivalent courses within each fulfillment option, etc), we can encode the conditions/criteria into the requirements json as suggested by @SamChou19815. For example, each requirement may have an additional field:

{
  ...otherRequirementJSONData,
  conditions?: {
    [course_id]: { // theoretically key can also be course code, but not for ap/ib conditions
      colleges?: ['EN'], // if the user is not in the EN college, course_id cannot fulfill the requirement
      excludedMajors?: ['CS'] // if the user is a CS major, course_id cannot fulfill the requirement
    }
  }
}

Open to comments and other suggestions.

…nto ben/exam-courses

dti-github-bot · 2021-11-12T02:49:07Z

[diff-counting] Significant lines: 415.

github-actions · 2021-11-12T02:50:33Z

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

https://cornelldti-courseplan-dev--pr584-ben-exam-courses-r0jgfjuy.web.app

_{(expires Tue, 23 Nov 2021 20:16:58 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

hahnbeelee

LGTM! Other than adding a comment to make things more clear, despite this being my first time really looking at this code everything made a lot of sense to me. Although I also left some clarifying questions just so I'm more aware of what's going on. :)

I'm kinda going to assume AP/IB functionality is consistent based on the passing test cases. I know that Will found a bug for AP/IB and I'm not 100% sure I can properly diagnose that AP/IB is working as it did before because I didn't interact with it a lot/ idk what the expected behavior is.

hahnbeelee · 2021-11-16T05:52:27Z

src/requirements/data/exams/ExamCredit.ts

        courseEquivalents: {
          DEFAULT: [351265], // CHEM 2070
          EN: [359187], // CHEM 2090
        },


Isn't this already similar to having the proposed conditional?

{ ...otherRequirementJSONData, conditions?: { [course_id]: { // theoretically key can also be course code, but not for ap/ib conditions colleges?: ['EN'], // if the user is not in the EN college, course_id cannot fulfill the requirement excludedMajors?: ['CS'] // if the user is a CS major, course_id cannot fulfill the requirement } } }

Yes, thanks for bringing this up! They should look similar because the conditions (in the requirements json) will be derived from the exam fulfillments (in the exam data).

The exam fulfillments data is currently being used for generating courseTakens from courseEquivalents, which was part of the old infra. For the new infra, it will be used to determine the conditions.

There are a few subtle differences, though:

the exam fulfillments are properties of the exams, while the conditions are properties of the requirements

colleges is a list of colleges, while the exam data uses the college as the key

DEFAULT becomes a list of colleges

Here's a more concrete example if it helps. With the current data, AP CHEM has one equivalent course: CHEM 2090 for EN, and CHEM 2070 for DEFAULT (non-EN). This will cause the script to draw edges from an AP CHEM exam course with course id 102 to the requirements that CHEM 2070 and CHEM 2090 fulfill. This is the same thing as adding the course ids to the requirements json courses lists.
Say CHEM 2070 fulfills req1 and req2, and CHEM 2090 fulfills req1 and req3. Then, the exam course fulfills req1, req2, and req3.

For req1, conditions will be undefined (or at least wouldn't have a key of 102) because CHEM 2070 and CHEM 2090 (all colleges) fulfill req1.

For req2, conditions for course id 102 will be something like colleges: [all colleges except 'EN'] because CHEM 2070 (non-EN colleges) fulfills req2.

For req3, conditions for course id 102 will be something like colleges: ['EN'] because CHEM 2090 (EN college only) fulfills req3.

O i see so the checkers for defining which courses an exam fulfills are more complex than just defining the school which is why we need more infrastructure than what we already have here.

Do you mean the checkers for defining which requirements an exam fulfills are more complex than just defining the course ids? If so, yes. We need the user data (college and major), which we can get at runtime but not when the requirements json is being generated.

src/requirements/data/exams/ExamCredit.ts

src/requirements/data/constants.ts

hahnbeelee · 2021-11-16T06:08:54Z

src/requirements/requirement-exam-utils.ts

+  return fulfillment;
+};
+
+export const examsTakenToExamCourses = (exams: ExamsTaken): CourseTaken[] => {


So does this mean that an exam can only have one course equivalent?

One exam can have multiple course equivalents, but it will only be converted to a single exam course.

I think you might be conflating course equivalents and courseTakens -- this does mean that in the new infra, an exam is represented by only one courseTaken.

Gotcha, so this is only infra for getting up to the CourseTaken part, and later when you refactor the graph it'll do the calculations for course equivalents?

And just to clarify - courseTaken is just the "middle ground"/connecting point between AP/IB and courses so that they're all represented by the same thing?

Yeah, later it'll calculate the exam course's edges based on course equivalents.

courseTaken is the "courses" part. Ultimately, we want courseTakens to be used in the requirements computation. Eg. semester courses are also converted to courseTakens for the computation.

The middle ground would be course equivalents. In the old infra, we generated one courseTaken per course equivalent (that basically represented the same course as the course equivalent). In the new infra, we'll generate one courseTaken per exam (but still use the course equivalents to determine the edges / connected requirements).

hahnbeelee

Am I allowed to approve twice? LOL LGTM!!! ship it

benjamin-shen added 10 commits November 7, 2021 21:30

add courseid to exam data

8fb3f03

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

8fc3a32

…nto ben/exam-courses

update schema

b20c51c

fix tests

7272756

update schema again, update requirements json, userExamsToExamCourses

10d8f4b

Fix tests

73d5769

Merge branch 'master' of https://github.com/cornell-dti/course-plan i…

d37bb7c

…nto ben/exam-courses

Fix lint issues

e8a7b3e

Implement userDataToExamCourses

1a30714

Fix lint errors

26030ad

benjamin-shen marked this pull request as ready for review November 13, 2021 16:41

benjamin-shen requested a review from a team as a code owner November 13, 2021 16:41

hahnbeelee approved these changes Nov 16, 2021

View reviewed changes

revert deprecated fws, add exam fulfillment comment

5c253b2

hahnbeelee approved these changes Nov 16, 2021

View reviewed changes

benjamin-shen merged commit 9757850 into master Nov 16, 2021

benjamin-shen deleted the ben/exam-courses branch November 16, 2021 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update exam data schema, add course ID to each fulfillment option #584

Update exam data schema, add course ID to each fulfillment option #584

benjamin-shen commented Nov 12, 2021 •

edited

Loading

dti-github-bot commented Nov 12, 2021 •

edited

Loading

github-actions bot commented Nov 12, 2021 •

edited

Loading

hahnbeelee left a comment •

edited

Loading

hahnbeelee Nov 16, 2021

benjamin-shen Nov 16, 2021

hahnbeelee Nov 16, 2021 •

edited

Loading

benjamin-shen Nov 16, 2021

hahnbeelee Nov 16, 2021

benjamin-shen Nov 16, 2021

hahnbeelee Nov 16, 2021

hahnbeelee Nov 16, 2021

benjamin-shen Nov 16, 2021 •

edited

Loading

hahnbeelee left a comment

Update exam data schema, add course ID to each fulfillment option #584

Update exam data schema, add course ID to each fulfillment option #584

Conversation

benjamin-shen commented Nov 12, 2021 • edited Loading

Summary

Test Plan

Notes

dti-github-bot commented Nov 12, 2021 • edited Loading

github-actions bot commented Nov 12, 2021 • edited Loading

hahnbeelee left a comment • edited Loading

Choose a reason for hiding this comment

hahnbeelee Nov 16, 2021

Choose a reason for hiding this comment

benjamin-shen Nov 16, 2021

Choose a reason for hiding this comment

hahnbeelee Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

benjamin-shen Nov 16, 2021

Choose a reason for hiding this comment

hahnbeelee Nov 16, 2021

Choose a reason for hiding this comment

benjamin-shen Nov 16, 2021

Choose a reason for hiding this comment

hahnbeelee Nov 16, 2021

Choose a reason for hiding this comment

hahnbeelee Nov 16, 2021

Choose a reason for hiding this comment

benjamin-shen Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

hahnbeelee left a comment

Choose a reason for hiding this comment

benjamin-shen commented Nov 12, 2021 •

edited

Loading

dti-github-bot commented Nov 12, 2021 •

edited

Loading

github-actions bot commented Nov 12, 2021 •

edited

Loading

hahnbeelee left a comment •

edited

Loading

hahnbeelee Nov 16, 2021 •

edited

Loading

benjamin-shen Nov 16, 2021 •

edited

Loading