-
Notifications
You must be signed in to change notification settings - Fork 0
feat: ✨ add filtering by GE and additional aggregation operations #88
Conversation
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.
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.
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.
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.
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.
LGTM!
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.