-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add schedule query to Graphql #56
Conversation
This is just the intial commit. Adds the schedule query and updates the course offering type. Also creates a new helper file to query websoc. There are still additional changes needed, such as error handling, input validation, and nested query optimizations.
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.
So this is a start for the schedule
query. There is still a decent amount that needs to be done before it is merged. I've noted some of my TODO items on the PR.
A few other things I wanted to mention:
-
We should consider downloading old websoc data and putting it in a sqlite db.
Since the data from previous terms are static and won't change, there is no need for us to query websoc every time. We should store that locally. Having it in a local db would also allow us to do more complex lookups, such as showing every course a professor has taught (currently queries are restricted to just one year/quarter). Idk if clients would want that info but maybe it would be worth the effort ¯\(ツ)/¯ -
This scheme file is getting BIG!
After this PR, I think it's time to split the schemas up into separate files.
// TODO: only return one error for a query, instead of one per item in the list | ||
throw new Error(`Accessing a course's offerings from a nested list is not allowed. Please use the schedules query`) | ||
} |
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.
Right now it will throw an error for each item in a nested list. So a client could end up with several dozen of the same errors lol. It's good that we have this in place because we don't want to send websoc several dozen requests. But I think we could clean up how we're presenting the error to the client.
TODO: only send one error back to the client.
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.
Honestly, just gonna leave this for now. Haven't been able to find a way to do this.
graphql/schema.js
Outdated
num_section_enrolled: { type: GraphQLFloat }, | ||
num_total_enrolled: { type: GraphQLFloat }, | ||
num_new_only_reserved: { type: GraphQLFloat }, | ||
num_on_waitlist: { type: GraphQLFloat }, | ||
num_requested: { type: GraphQLFloat }, |
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.
Not sure if I'm happy with these field names. They seem really long...
TODO: find better names
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.
@devsdevsdevs @Maybe14:
Can y'all take a look at the courseOfferingType fields and see if the names make sense to you?
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.
What exactly is the difference between section_enrolled
and total_enrolled
? I'm assuming section_enrolled
is for the, well, section LOL, like a lab. And total_enrolled
is the total amount enrolled in the course (Like ICS 31 taught by Professor Kay).
Must provide year and quarter Must provide ge, department, section codes, or instructor
If it is n/a, we return null. I thought about returning 0, but there is actually an important distinction between the waitlist being open and no waitlist at all.
For This was the query I was testing it on:
Just wondering if this is the intended behavior. It's definitely better to not have a user write the same query over again, so I do agree, but it was unclear to me that this was the case. It could be mentioned in the documentation for graphql playground or on the generated graphql docs. Also, I was looking over the schema.js file and 100% agree the file is way too large and hard to read, and would be very helpful if separated. |
The arguments are completely ignored for offerings under the schedule queries. I agree that's confusing... This happens because both queries share the same |
Course's id, Instructor's ucinetid, and Schedule's year/quarter args are now required.
Regarding including in Documentation, I can look into explicitly stating it on GraphQL playground. I think having more guidance on the playground (like a header made up of comments) was a feature previously requested. But other than that, the only other place we can state this behavior is on the MkDocs |
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.
naming looks good!
graphql/schema.js
Outdated
num_section_enrolled: { type: GraphQLFloat }, | ||
num_total_enrolled: { type: GraphQLFloat }, | ||
num_new_only_reserved: { type: GraphQLFloat }, | ||
num_on_waitlist: { type: GraphQLFloat }, | ||
num_requested: { type: GraphQLFloat }, |
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.
What exactly is the difference between section_enrolled
and total_enrolled
? I'm assuming section_enrolled
is for the, well, section LOL, like a lab. And total_enrolled
is the total amount enrolled in the course (Like ICS 31 taught by Professor Kay).
This PR implements the
schedule
query and establishes theCourseOffering
type.Test Plan
Verify that the following queries work as intended
Verify that the following queries throw an error. We do not support offerings on large nested lists from allCourses or an instructor's history