Skip to content
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

feat: add new db powered by UT_Grade_Parser #163

Merged
merged 9 commits into from
Mar 24, 2024
Merged

Conversation

doprz
Copy link
Collaborator

@doprz doprz commented Mar 14, 2024

This change is Reviewable

@Samathingamajig Samathingamajig self-assigned this Mar 23, 2024
@doprz doprz marked this pull request as ready for review March 23, 2024 19:24
@doprz doprz requested a review from Razboy20 March 23, 2024 19:24
@Samathingamajig Samathingamajig added the blocked Do not merge (yet) label Mar 23, 2024
@Samathingamajig
Copy link
Collaborator

something is wrong with the db, reviewing UT_Grade_Parser

@doprz doprz marked this pull request as draft March 23, 2024 20:27
@doprz
Copy link
Collaborator Author

doprz commented Mar 23, 2024

PR blocked until UT_Grade_Parser bug is resolved.

Copy link
Member

@Razboy20 Razboy20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @doprz and @Samathingamajig)


src/views/components/injected/CourseCatalogInjectedPopup/GradeDistribution.tsx line 0 at r2 (raw file):
Let's use imports rather than use React.

Code snippet:

React.useMemo -> useMemo
React.useState -> useState

src/views/lib/database/queryDistribution.ts line 11 at r2 (raw file):

    'grade_distributions_2021_2022',
    'grade_distributions_2022_2023',
] as const;

fyi, in the future let's maybe refactor this to be reactive to the items in the db rather than being explicit.

Code quote:

const allTables = [
    'grade_distributions_2019_2020',
    'grade_distributions_2020_2021',
    'grade_distributions_2021_2022',
    'grade_distributions_2022_2023',
] as const;

src/views/lib/database/queryDistribution.ts line 99 at r2 (raw file):

        where Department_Code = '${course.department}'
        and Course_Number = '${course.number}'
        ${semester ? `and Semester = '${semester.season} ${semester.year}'` : ''}

Can we sanitize this more? I noticed that the previous revision used %% wrappings, idk whether this is something we should add or not.

Code quote:

    const query = `
        select * from ${semester ? `grade_distributions_${semester.year + yearDelta}_${semester.year + yearDelta + 1}` : `(select * from ${allTables.join(' union all select * from ')})`}
        where Department_Code = '${course.department}'
        and Course_Number = '${course.number}'
        ${semester ? `and Semester = '${semester.season} ${semester.year}'` : ''}

src/views/lib/database/queryDistribution.ts line 139 at r2 (raw file):

        F: row.F,
        Other: row.Other,
    } satisfies Distribution;

Why satisfies? By virtue of having a return type, ts knows the type of the object.

doprz pushed a commit to doprz/UT_Grade_Parser that referenced this pull request Mar 24, 2024
* fix: grade columns in deterministic ordering

fixes #5, unblocks Longhorn-Developers/UT-Registration-Plus#163

* fix: grade columns into correct order
@doprz doprz marked this pull request as ready for review March 24, 2024 03:45
@doprz doprz removed blocked Do not merge (yet) wip labels Mar 24, 2024
Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @doprz and @Razboy20)


src/views/lib/database/queryDistribution.ts line 11 at r2 (raw file):

Previously, Razboy20 wrote…

fyi, in the future let's maybe refactor this to be reactive to the items in the db rather than being explicit.

yeah... I wanted to do that, but also didn't want to get every table name in case there are more in the future that aren't grade distributions (could've filtered by grade_distributions_\d+_\d+ but who cares)


src/views/lib/database/queryDistribution.ts line 99 at r2 (raw file):

Previously, Razboy20 wrote…

Can we sanitize this more? I noticed that the previous revision used %% wrappings, idk whether this is something we should add or not.

the %% wrapping wasn't sanitization, they're wildcards. it was something like Course_Number like '%314%' which would match "314H" (314 honors).

As for preventing SQL injections, 1. this is running completely locally, 2. I don't think there will ever be a department id, course id, or semester (e.g. "Summer 2022") with an apostrophe, but I do agree we should probably use a sanitizer just in case (especially if a field is ever added where that could be a problem) and to make cleaner code.

Copy link
Member

@Razboy20 Razboy20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r3, all commit messages.
Dismissed @Razboy20 from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @doprz)


src/views/lib/database/queryDistribution.ts line 99 at r2 (raw file):

Previously, Samathingamajig (Samuel Gunter) wrote…

the %% wrapping wasn't sanitization, they're wildcards. it was something like Course_Number like '%314%' which would match "314H" (314 honors).

As for preventing SQL injections, 1. this is running completely locally, 2. I don't think there will ever be a department id, course id, or semester (e.g. "Summer 2022") with an apostrophe, but I do agree we should probably use a sanitizer just in case (especially if a field is ever added where that could be a problem) and to make cleaner code.

Yeah I agree, sanitization is not a real concern, but just for posterity.

@Razboy20 Razboy20 merged commit 60d1f48 into main Mar 24, 2024
12 checks passed
@Razboy20 Razboy20 deleted the feature/update-db branch March 24, 2024 05:21
caseycharleston pushed a commit to caseycharleston/UT-Registration-Plus that referenced this pull request Mar 30, 2024
* feat: add new db powered by UT_Grade_Parser

* Merge branch 'main' of https://github.com/Longhorn-Developers/UT-Registration-Plus into feature/update-db

* feat: update db

* feat: update db handlers and types

Co-authored-by: Samuel Gunter <[email protected]>

* fix: type errors

* fix: add Other to grade dist

* fix: db with proper insertion order

* Merge branch 'main' of https://github.com/Longhorn-Developers/UT-Registration-Plus into feature/update-db

* chore: address PR comments

Co-Authored-By: Samuel Gunter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants