Filter courses by assignment IDs #6435
Conversation
marcospri
force-pushed
the
course-assignment-filter
branch
from
c4fac34 to
7e73977
Compare
| ) -> Select[tuple[Course]]: | ||
| """Get a list of unique courses. | ||
|
|
||
| :param organization: organization the courses belong to. | ||
| :param h_userid: only courses this user has access to. | ||
| :param instructor_h_userid: return only courses where instructor_h_userid is an instructor. | ||
| :param h_userids: return only courses where these users are members. |
There was a problem hiding this comment.
What does a user have to do to qualify as a member of a course? Be a student who has launched at least one assignment in the course?
There was a problem hiding this comment.
Yes, for now everything is based on "what we've seen" instead of querying LMS API for more data.
| LTIRole.scope == RoleScope.COURSE, | ||
| LTIRole.type == RoleType.INSTRUCTOR, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
To check my understanding - we don't actually store a user's role per course, instead we store the role per assignment. This means that to find courses where the user is an instructor, we find all courses where the user was identified as an instructor in at least one assignment launch?
There was a problem hiding this comment.
Yes exactly.
Ideally we store that info at the GroupingMembership level and not at the assignment level.
I don't want to tackle a big data migration & refactor & dashboard at the same time but we should move in that direction, store the data at the level it's more meaningful.
| :param h_userid: only courses this user has access to. | ||
| :param instructor_h_userid: return only courses where instructor_h_userid is an instructor. | ||
| :param h_userids: return only courses where these users are members. | ||
| :param assignment_ids: return only the courses these assignments belong to. |
There was a problem hiding this comment.
The Assignment.course property has a note that an assignment can only belong to one course, but there can be multiple rows in the grouping table representing the same course. In what circumstances does that happen and does that matter here - ie. if there are multiple rows representing the course, do we need to pick one?
There was a problem hiding this comment.
Courses are duplicated once per application instance, we take the last updated row but they represent the same entity in the LMS.
| h_userids = fields.List(fields.Str()) | ||
| """Return metrics for these users only.""" | ||
|
|
||
| assignment_ids = fields.List(fields.Integer()) |
There was a problem hiding this comment.
In the dashboard's user APIs we validate several ID fields are integers >= 1. It would be useful I think to have a common field definition that is used by query string parameters that expect integer IDs.
|
|
||
| location = "querystring" | ||
|
|
||
| h_userids = fields.List(fields.Str()) |
There was a problem hiding this comment.
Maybe worth adding validation on the format of the user IDs here (acct:[chars]@[authority_chars]).
There was a problem hiding this comment.
Making a note about this one to apply to all the filters.
| @@ -156,6 +165,34 @@ def get_courses( | |||
| # Only select the ID of the deduplicated courses | |||
| ).with_entities(Course.id) | |||
|
|
|||
| if instructor_h_userid: | |||
There was a problem hiding this comment.
We don't need to change anything now, but I note that I'm expecting we will be asked to make some subset of the dashboard functionality available for students in future.
There was a problem hiding this comment.
Yes, this could be changed to a filter that takes an arbitrary tuple of (h_userid, role_type, role_scope) to make it more flexible, at least at this level.
He have to sort teacher and admins out first so let's revisit this when we have a better understand on what data we'd show for students.
marcospri
force-pushed
the
course-assignment-filter
branch
from
ef0269c to
b2d0324
Compare
For:
Same as: #6430 but for courses.
Testing
http://localhost:8001/api/dashboard/organizations/local.lms.org.LDtcl7EUTeW2UERPJLAVtA/courses?assignment_ids=397