fix(degreeworks): Provide Polymorphic requirements for majors with specializations#310
fix(degreeworks): Provide Polymorphic requirements for majors with specializations#310
Conversation
…id of specialization table
…that points to seperate major_requirement table
laggycomputer
left a comment
There was a problem hiding this comment.
You also accidentally checked two testing files in, no big deal
| majorCode: string; | ||
| specCode?: string; | ||
| }; | ||
| // 'majorName;specCode'. If no specialization is taken, 'majorName;undefined' |
There was a problem hiding this comment.
Maybe just drop the semicolon and everything after it if there's no specialization. Explicitly leaving "undefined" in a string may be mistaken for a bug.
apps/api/src/schema/programs.ts
Outdated
| }), | ||
| specializationId: programIdBase.optional().openapi({ | ||
| description: | ||
| "fetch major requirements when taking the specialization with this ID, if provided; providing no specialization when one is required returns the 'base' major requirements", |
There was a problem hiding this comment.
providing no specialization when one is required returns the 'base' major requirements"
Too nice of a claim; we can't really say anything about the meaning of the major requirements when a specialization is required, but none is provided...
apps/api/src/rest/routes/programs.ts
Outdated
| { | ||
| ok: false, | ||
| message: "Couldn't find this major; check your ID?", | ||
| message: "Couldn't find this major associated with this specialization; check your ID?", |
There was a problem hiding this comment.
Grammar is a bit funky here...
apps/api/src/schema/programs.ts
Outdated
| }), | ||
| specializationId: programIdBase.optional().openapi({ | ||
| description: | ||
| "fetch major requirements when taking the specialization with this ID, if provided; providing no specialization when one is required results in unspecified behavior and is deprecated", |
There was a problem hiding this comment.
| "fetch major requirements when taking the specialization with this ID, if provided; providing no specialization when one is required results in unspecified behavior and is deprecated", | |
| "If provided, fetch major requirements given this specialization; providing no specialization when one is required has unspecified behavior", |
apps/data-pipeline/degreeworks-scraper/src/components/DegreeworksClient.ts
Show resolved
Hide resolved
| `${schoolCode}-MAJOR-${majorCode}-${degreeCode}`, | ||
| majorAudit, | ||
| ), | ||
| specCode: specCode, |
| const inMap = this.parsedPrograms.get( | ||
| this.asMajorSpecId("Major in Chemistry"), | ||
| ) as MajorProgram; |
There was a problem hiding this comment.
This was my goof as we discussed, but you can't as MajorProgram because this is an inappropriate assumption of infallibility in the map get.
Co-authored-by: Dante Dam <laggycomputer@yahoo.com>
| degree: string, | ||
| school: string, | ||
| majorCode: string, | ||
| specCode?: string, |
There was a problem hiding this comment.
More than one optional argument gets a little messy, especially if we add/reorder them. Do you want to pass in some kind of object instead? We could also leave this in if you don't care enough
| // "optional american chemical society certification" | ||
| const inMap = this.parsedPrograms.get("Major in Chemistry") as MajorProgram; | ||
| return inMap ? [inMap[1]] : []; | ||
| const inMap = this.parsedPrograms.get(this.asMajorSpecId("Major in Chemistry")); |
There was a problem hiding this comment.
I'd prefer the specialization argument to asMajorSpecId be nullable instead of optional, but that's an opinion.
| majorId: varchar("major_id") | ||
| .notNull() | ||
| .references(() => major.id), | ||
| specId: varchar("spec_id").references(() => specialization.id), |
There was a problem hiding this comment.
Should maybe be specialization_id for consistency
| .primaryKey() | ||
| .generatedAlwaysAs(sql`jsonb_hash_extended(requirements, 0)`), | ||
| }, | ||
| (table) => [], |
There was a problem hiding this comment.
Remove this if we're not going to add any indices, etc
Description
We currently save one requirement for a major. However, when majors are taken with a specialization the original major block may change. For example, the Applied and Computational Math major has 2 optional specializations. The major requirement has a "Requirements for Pure Applied and Computation Math" subset, which is removed when specializations are taken.
We now run a major scrape for each major, spec pair. The
major_spec_pair_to_requirementtable refers to a major code and an optional potentially null spec code, and has arequirement_idthat points to another table,major_requirement. To enforce uniqueness on major_requirement, arequirements_hashcolumn is used as the json for requirements is to large to put a unique constrain on.The
Major RequirementEndpoint has been changed to take an optionalspecializationId(ex = 'BS-201A) along with theprogramId(BS-201). It is an error to give specializationId asnullor to give a specializationID that is not associated with the given majorId.Related Issue
#263
Motivation and Context
How Has This Been Tested?
Locally tested on graphql and rest endpoints.
Screenshots (if appropriate):
Types of changes
Checklist: