Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

feat: ✨ add filtering by GE and additional aggregation operations #88

Merged
merged 23 commits into from
Oct 2, 2023

Conversation

ecxyzzy
Copy link
Member

@ecxyzzy ecxyzzy commented Aug 28, 2023

Summary

Add filtering by GE category for all endpoints, as well as the aggregate grouped operation. The section listing for raw and aggregate grades also include the section's GE categor(y/ies).

The aggregate grouped operation returns a list containing the aggregated grade distribution for each (course, instructor) pair.

Notes

This PR used to implement only the latter feature (hence the branch name), but the first change was later determined to be in scope for this PR and so was built upon this PR.

Issues

Closes #87; closes #94.

@ecxyzzy ecxyzzy temporarily deployed to staging-88 August 28, 2023 00:39 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs August 28, 2023 00:39 — with GitHub Actions Inactive
@github-actions github-actions bot changed the title feat: add operation for aggregating grades by course/instructor pair feat: ✨ add operation for aggregating grades by course/instructor pair Aug 28, 2023
@ecxyzzy ecxyzzy changed the title feat: ✨ add operation for aggregating grades by course/instructor pair feat(grades): ✨ aggregate grades by course and instructor Aug 28, 2023
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs August 31, 2023 04:47 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 August 31, 2023 04:48 — with GitHub Actions Inactive
@github-actions github-actions bot changed the title feat(grades): ✨ aggregate grades by course and instructor feat(grades): ✨ aggregate grades by course and instructor Aug 31, 2023
@ecxyzzy ecxyzzy temporarily deployed to staging-88 August 31, 2023 04:49 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs August 31, 2023 04:49 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 August 31, 2023 04:53 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs August 31, 2023 04:53 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy marked this pull request as ready for review August 31, 2023 04:58
@ecxyzzy ecxyzzy requested a review from MinhxNguyen7 August 31, 2023 04:58
@ecxyzzy ecxyzzy temporarily deployed to staging-88 August 31, 2023 05:02 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs August 31, 2023 05:02 — with GitHub Actions Inactive
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

I haven't looked at the code yet, so I'm only going to comment on functionality.

This is good for when the user is looking at a department or a particular course. However, a (arguably more) common use case is selecting a GE with the highest GPA. This feature does not support that use case more than the current one does.

I'm not sure what you can do about that. Perhaps implementing selection by GE is as easy as by department; I don't know how your database is structured.

@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 5, 2023 06:46 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 5, 2023 06:46 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 5, 2023 06:47 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 5, 2023 06:47 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 7, 2023 04:35 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 7, 2023 04:35 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 7, 2023 05:02 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 7, 2023 05:02 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 7, 2023 16:30 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 7, 2023 16:30 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 7, 2023 18:07 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 7, 2023 18:07 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy changed the title feat: ✨ add filtering by GE and aggregate grouped operation feat: ✨ add filtering by GE and additional aggregation operations Sep 7, 2023
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 7, 2023 22:40 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 7, 2023 22:40 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 14, 2023 05:55 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 14, 2023 05:55 — with GitHub Actions Inactive
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

When querying aggregateByCourse, there's an option to put instructor in the metadata, but that results in an unhelpful error: "Cannot return null for non-nullable field AggregateGradeByOffering.instructor". It doesn't make sense to aggregate by instructor, but that is an option, so it should either be removed or at least given a more helpful message.

Everything else seems fine. I assume, considering you scaled up the DB, that you are positively sure that the GPA PR on AA won't destroy PP.

I can't speak much to the goodness of the code. It looks alright I guess.

apps/api/v1/rest/grades/src/lib.ts Show resolved Hide resolved
@ecxyzzy ecxyzzy temporarily deployed to staging-88-docs September 18, 2023 01:50 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-88 September 18, 2023 01:50 — with GitHub Actions Inactive
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ecxyzzy ecxyzzy merged commit 874872a into main Oct 2, 2023
5 checks passed
@ecxyzzy ecxyzzy deleted the aggregate-grouped branch October 2, 2023 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants