-
Notifications
You must be signed in to change notification settings - Fork 12
feat(programs): Add unique hashed ID for requirements #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
384b064 to
a7c6013
Compare
a9fb47e to
696b2bc
Compare
696b2bc to
32d1ff6
Compare
32d1ff6 to
70d29ad
Compare
70d29ad to
a10d47d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work thus far. This will need rigorous testing to verify due the sensitive nature of the problem.
This is not SemVer breaking, by the way, so I removed that from the PR description.
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
apps/data-pipeline/degreeworks-scraper/src/components/AuditParser.ts
Outdated
Show resolved
Hide resolved
laggycomputer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit messages (and therefore PR titles) must obey Conventional Commits.
a10d47d to
e75ffed
Compare
6aa5494 to
2517795
Compare
laggycomputer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Architecture is sane.
No new comments, but previous review comments have been updated and should be addressed first.
2517795 to
a41874f
Compare
| } | ||
|
|
||
| generateRequirementId(rule: Rule, childRequirementBlockIdentifier: string): string { | ||
| generateRequirementId(requirementType: string, coursesSalt: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also can't call this a "courses salt" because it's sometimes made up of requirement IDs instead!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laggycomputer Which of these seems best?
- ContentSalt
- RequirementSalt
- RequirementBlockSalt
Description
Due to needing a unique id for individual requirements, and a unique id not being served by DegreeWorks, a hash consisting of a requirement's features was added.
The unique id is served as requirementId in the requirements column of
major,minor,specialization, andcollege_requirementtables.Additionally, due to jsonb's being too large for the unique index in
college_requirement, arequirements_hashcolumn was added to enforce uniqueness on.Related Issue
#185
Motivation and Context
Reason for unique id comes from issue.
How Has This Been Tested?
Scraper has been ran locally, database updates verified, and API calls tested for both rest and graphql
Screenshots (if appropriate):
N/A
Types of changes
Checklist: