[BD-24] [BB-2726] [TNL-7330] Added Course Membership API function - #25843
Conversation
|
Thanks for the pull request, @shimulch! I've created BLENDED-793 to keep track of it in Jira. More details are on the BD-24 project page. When this pull request is ready, tag your edX technical lead. |
openedx-webhooks
added
needs triage
open-source-contribution
bf44c78 to
b678721
Compare
giovannicimolin
left a comment
giovannicimolin
left a comment
There was a problem hiding this comment.
Left a few comments.
I'm not sure this API is in the best place though. It retrieves users for a course, so it makes sense to have it in the enrollment API no?
CC @nedbat
There was a problem hiding this comment.
@shimulch Is there a reason we'd avoid retrieving students enrolled? And if so, this parameters overlaps with access_roles, can we use a single parameter for this with (something like access_role=['student'] to filter students)?
There was a problem hiding this comment.
In my implementation, I've treated access_roles as valid values for CourseAccessRole. student is not a valid access_role. Students only have the CourseEnrollment instance but no CourseAccessRole instance. As it's an internal API, I wanted to be explicit about that.
There was a problem hiding this comment.
I think we can remove the prefetching here and do it in the viewset, inside the get_queryset method.
This needs to be a simple API to retrieve users enrolled into a course depending on their role.
There was a problem hiding this comment.
Yeah, we can. But in that case, we would get all the CourseEnrollment of the user, not only those related to the Course. In order to do that, we would need to import CourseEnrollment in lti_consumer.compat module. Should we do that and remove prefetching here?
There was a problem hiding this comment.
Improve the function name to match it's functionality: get_course_member_queryset.
| def get_course_members(course_key, include_students=True, access_roles=None, prefetch_accessroles=False, prefetch_enrollments=False): | |
| def get_course_member_queryset(course_key, include_students=True, access_roles=None, prefetch_accessroles=False, prefetch_enrollments=False): |
I've thought that too. But in this API, we need to check for both |
|
@shimulch Thank you for your contribution. Please let me know once it is ready for our review. |
There was a problem hiding this comment.
Please add an example of how to filter out all roles here (e.g pass an empty dict to exclude all roles (except students).
There was a problem hiding this comment.
Added example calls for different use cases.
There was a problem hiding this comment.
I'm wondering if we can get to a simplified version of this.
Can you squash prefetch_accessroles and prefetch_enrollments down to a single parameter (prefetch_user_role_data)?
There was a problem hiding this comment.
Moved into one single flag prefetch_user_course_roles.
There was a problem hiding this comment.
Run this before doing the prefetch?
I'm not sure it'll have any impact since django uses lazy querysets, but the code will look better.
There was a problem hiding this comment.
When querying with in relation to many to many tables, some matches multiple times (ex: both instructor and enrolled) and I've seen it returns duplicates. That's why this has been placed.
There was a problem hiding this comment.
@shimulch Yes, I got that. I just want you to run distinct before the prefetch operations. Just move this line to above the prefetch if statement.
openedx-webhooks
added
waiting on author
There is already code to deal with access roles in the enrollment API: In fact, it seems more generic - it uses a pluggable "enrollment data backend" and uses My question is: what is this API adding that isn't covered by existing APIs? I assume there is already an API to get all the staff, instructors, and TAs for a course, because that's needed for the instructor dashboard "staff" tool as well as the "course team" page on Studio. And there is of course already an API to list all the students in a course. So is this just providing a combined view of data that can already be retrieved through two existing API calls? |
Using
As you have already mentioned, the combined view of all
Yes, but it returns a |
Probably. At this point it might be a good idea to check the PRs / git-blame to see who made the enrollment API pluggable and get their input. I didn't even know it had a pluggable backend. But whoever did that or maintains it can probably offer more useful advice here.
I see, the goal here is adding a python API, even though there are similar REST APIs that provide this information. Your rationale makes total sense, and an |
|
The pluggable data API seem to introduced in this PR - https://github.com/edx/edx-platform/pull/5695 And from the PR description, it seems the goal was to make it easier for testing with fake data. If that's the only reason, I think we might not have to worry about data API here. |
Ah, ok. Sorry for focusing on that then. I can try to review this soon but it might be better if someone with more context on these APIs can weigh in. |
|
@shimulch Is this ready for edX review? |
|
@natabene This is also part of BD-24 as like https://github.com/edx/edx-platform/pull/25844 and was blocked due to some performance concerns. Since the ADR got accepted, we are now working back again on this one. |
natabene
changed the title
openedx-webhooks
added
blended
2d99a22 to
6779a03
Compare
giovannicimolin
left a comment
giovannicimolin
left a comment
There was a problem hiding this comment.
@shimulch I left a few comments. I tested this to be working, but there's a problem with the approach here, and I've suggested a few changes.
There was a problem hiding this comment.
@shimulch Yes, I got that. I just want you to run distinct before the prefetch operations. Just move this line to above the prefetch if statement.
There was a problem hiding this comment.
@shimulch I don't think the behavior here is the expected one. We always want the course_id filter to be applied here. I suggested the prefetching is just to avoid making repetitive queries when retrieving data from the related models, but I see that there's some filtering happening.
I think the best approach here is to:
- Remove the
prefetch_user_course_rolesconditional, leaving prefetching and filtering to always happen. - Change the function docstring to be explicitly clear that only course access roles and enrollments related to the queried course will be retrieved.
- Update tests to take this into account, and check that accessing the related objects doesn't trigger extra queries when using this API.
giovannicimolin
left a comment
giovannicimolin
left a comment
There was a problem hiding this comment.
Looks good so far, and it seems to be working as expected. Left a few minor comments since this is just a wip version.
There was a problem hiding this comment.
Can you add a ordering query here? Otherwise the paginator might yield inconsistent results.
I got this warning on my devstack:
2021-03-17 14:53:06,478 WARNING 300 [py.warnings] [user None] [ip None] warnings.py:109 - /edx/app/edxapp/edx-platform/lms/djangoapps/course_api/api.py:348: UnorderedObjectListWarning: Pagination may yield inconsistent results with an unordered object_list: <class 'django.contrib.auth.models.User'> QuerySet.
There was a problem hiding this comment.
This mentions "legacy" serializer, isn't there a more up-to-date one?
There was a problem hiding this comment.
I didn't find any other profile serializer other than AccountLegacyProfileSerializer. And this seems to be used while updating profile here.
|
@ormsbee I've implemented your suggestions and the end result is blazing fast now. 🚀
This didn't work, querysets evaluate as false if the start was bigger than the end in the index. This worked perfectly though: Here are the queries executed: This is ready for review. CC @nedbat |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Queries look good. Just a few requests on documentation, tests, and errors.
Also, when you make the final commit message for this, please be sure to write details about why this was implemented in the way it was. I think that it's a reasonable tradeoff, but it's going to look a little weird without context, and people may not immediately realize the performance issues or the complexity brought on by the fact that we're trying to combine two tables with users that can be in either or both places. Lacking that context could cause folks a lot of pain a few years down the line.
Thank you for working through this!
| "staff", | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
The docstring should clearly indicate the too-large-course error case, and answer the following:
- Where is the config value set?
- What exception is raised?
| course_key | ||
| )[settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT:][:1] | ||
| if over_limit.exists(): | ||
| raise Exception( |
There was a problem hiding this comment.
Please make a new error type that subclasses Exception, so callers can check for it explicitly.
| user_info[access_role.user_id] = get_user_info(enrollment.user) | ||
|
|
||
| # Merge user role information with `user_info` | ||
| for user in user_info: |
There was a problem hiding this comment.
Nit: user_id might be clearer here, since you're iterating over those, and not User model objects.
|
|
||
| # Merge user role information with `user_info` | ||
| for user in user_info: | ||
| user_info[user]['roles'] = user_roles.get(user, []) |
There was a problem hiding this comment.
Nit: user_roles is a defaultdict(list), so the .get with a default value shouldn't be necessary (i.e. user_roles[user] should work).
| raise Exception( | ||
| f"Can't retrieve course members for courses with more than" | ||
| f"{settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT} active enrollments" | ||
| ) |
There was a problem hiding this comment.
Nit: Say I got paged for this because a certain course that was dependent on this API grew beyond a certain size and suddenly started breaking... and then I looked in the logs and saw this message. Since the immediate short-term fix is going to be to bump up the limit, I think it'd be helpful to not just give the number, but also log which setting it's pulled from (so that the word COURSE_MEMBER_API_ENROLLMENT_LIMIT is in the log string and I can go from that log message to searching my config).
There was a problem hiding this comment.
... not that this sort of thing has ever happened to us... 🤧
There was a problem hiding this comment.
Should we add the course key to this error message? Or will it be obvious from some other source in the logs?
There was a problem hiding this comment.
@nedbat Yes, good idea! The log output now look like this:
Can't retrieve course members for COURSE_KEY since it has more than 1000 active enrollments. This limit is stored on `settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT`
| self.assertEqual( | ||
| members[self.honor.id]['roles'], | ||
| ['student'] | ||
| ) |
There was a problem hiding this comment.
We want to test that the combining of data works properly, so I think it would be useful to have test cases for people who exist only in enrollments (students), people who exist in both (course staff, beta testing students), and people who exist only in access_roles (data_researcher).
|
@nedbat: The performance issues that I was concerned about have been resolved. I left a review here tonight, but please feel free to take over the reviews/merging at your discretion. Thank you for looping me on in this! |
| @classmethod | ||
| def access_roles_in_course(cls, course_key): | ||
| """ | ||
| Returns all users that have a course access role in a given course. |
There was a problem hiding this comment.
It doesn't return all users but all CourseAccessRoles for a given course, with user information prefetched.
| Returns all users that have a course access role in a given course. | |
| Returns all CourseAccessRole for a given course and prefetches user information. |
|
jenkins run python |
shimulch
left a comment
shimulch
left a comment
There was a problem hiding this comment.
@ormsbee Thanks for all the suggestions. 😃
@giovannicimolin Thanks a lot for working on this. LGTM 👍🏽
|
@nedbat This is ready for review. |
| # Retrieve all active enrollments in course and prefetch user information | ||
| enrollments = CourseEnrollment.get_active_enrollments_in_course( | ||
| course_key | ||
| )[:settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT] |
There was a problem hiding this comment.
Why are we still limiting here? If the limit accomplishes anything, then we'd never reach this line of code, right?
There was a problem hiding this comment.
Yup, makes sense. I copied this from Dave's pseudo-implementation above but it has no effect (or at least it shouldn't).
| } | ||
| } | ||
| """ | ||
| def get_user_info(user, enrollment_mode=None): |
There was a problem hiding this comment.
Nit: this function doesn't feel like a "get_" to me, since it just reformats the data you pass to it. I would name it "make_user_info_dict" or something.
nedbat
left a comment
nedbat
left a comment
There was a problem hiding this comment.
I really like this pull request: it took a lot of discussion, but in the end, is good clean code.
|
Your PR has finished running tests. There were no failures. |
|
@nedbat I addressed your review comments :) |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
The get_course_members API returns a dict of users associated with a course. This is a potentially expensive operation on a large course, so there is a control in place to limit its cost. If a course has more than settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT enrollments, then the function raises an OverEnrollmentLimitException. This API was added to help implement the LTI 1.3 Names and Roles Provisioning service. Jira references: [BD-24] [BB-2726] [TNL-7330] Pull request: #25843 Co-authored-by: Giovanni Cimolin da Silva <giovannicimolin@gmail.com> (cherry picked from commit 2a8a58a)
|
I've cherry-picked this onto Lilac. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
The get_course_members API returns a dict of users associated with a course. This is a potentially expensive operation on a large course, so there is a control in place to limit its cost. If a course has more than settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT enrollments, then the function raises an OverEnrollmentLimitException. This API was added to help implement the LTI 1.3 Names and Roles Provisioning service. Jira references: [BD-24] [BB-2726] [TNL-7330] Pull request: openedx#25843 Co-authored-by: Giovanni Cimolin da Silva <giovannicimolin@gmail.com>
9 participants
openedx/xblock-lti-consumer#124 depends on this PR.
This PR adds a new function
get_course_memberswhich can be used to get all users related to a Course. This function can -{ 'count': total number of user instances matching specified filter, 'num_pages': total number of pages, 'current_page': current page number, 'result': [ { 'id': user id, 'email': user's email, 'username': user's username, 'profile': { 'name': user's name, 'profile_image': user's profile image, .... other profile information }, 'enrollments': [ { 'mode': enrollment mode } ], 'course_access_roles': [ { 'role': user's role in course } ] } ] }JIRA tickets:
Discussions: N/A
Dependencies: None
Screenshots: N/A
Sandbox URL: N/A
Merge deadline: ASAP - as it's blocking openedx/xblock-lti-consumer#124
Testing instructions:
python manage.py lms shell.from lms.djangoapps.course_api.api import get_course_members.CourseKey. Ex -get_course_memberswith available parameters.Author notes and concerns:
lms.djangoapps.course_api.apithe correct place for this function?SELECT COUNT(*) FROM (SELECT DISTINCT "auth_user"."id" AS Col1, "auth_user"."password" AS Col2, "auth_user"."last_login" AS Col3, "auth_user"."is_superuser" AS Col4, "auth_user"."username" AS Col5, "auth_user"."first_name" AS Col6, "auth_user"."last_name" AS Col7, "auth_user"."email" AS Col8, "auth_user"."is_staff" AS Col9, "auth_user"."is_active" AS Col10, "auth_user"."date_joined" AS Col11 FROM "auth_user" LEFT OUTER JOIN "student_courseaccessrole" ON ("auth_user"."id" = "student_courseaccessrole"."user_id") LEFT OUTER JOIN "student_courseenrollment" ON ("auth_user"."id" = "student_courseenrollment"."user_id") WHERE ("auth_user"."is_active" = 1 AND (("student_courseaccessrole"."course_id" = 'edX/toy/2012_Fall' AND "student_courseaccessrole"."role" IN ('staff', 'instructor', 'finance_admin', 'sales_admin', 'beta_testers', 'library_user', 'data_researcher', 'course_creator_group', 'support')) OR ("student_courseenrollment"."course_id" = 'edX/toy/2012_Fall' AND "student_courseenrollment"."is_active" = 1)))) subquery SELECT DISTINCT "auth_user"."id", "auth_user"."password", "auth_user"."last_login", "auth_user"."is_superuser", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."date_joined" FROM "auth_user" LEFT OUTER JOIN "student_courseaccessrole" ON ("auth_user"."id" = "student_courseaccessrole"."user_id") LEFT OUTER JOIN "student_courseenrollment" ON ("auth_user"."id" = "student_courseenrollment"."user_id") WHERE ("auth_user"."is_active" = 1 AND (("student_courseaccessrole"."course_id" = 'edX/toy/2012_Fall' AND "student_courseaccessrole"."role" IN ('staff', 'instructor', 'finance_admin', 'sales_admin', 'beta_testers', 'library_user', 'data_researcher', 'course_creator_group', 'support')) OR ("student_courseenrollment"."course_id" = 'edX/toy/2012_Fall' AND "student_courseenrollment"."is_active" = 1))) ORDER BY "auth_user"."id" ASC LIMIT 3 SELECT "auth_userprofile"."id", "auth_userprofile"."user_id", "auth_userprofile"."name", "auth_userprofile"."meta", "auth_userprofile"."courseware", "auth_userprofile"."language", "auth_userprofile"."location", "auth_userprofile"."year_of_birth", "auth_userprofile"."gender", "auth_userprofile"."level_of_education", "auth_userprofile"."mailing_address", "auth_userprofile"."city", "auth_userprofile"."country", "auth_userprofile"."state", "auth_userprofile"."goals", "auth_userprofile"."allow_certificate", "auth_userprofile"."bio", "auth_userprofile"."profile_image_uploaded_at", "auth_userprofile"."phone_number" FROM "auth_userprofile" WHERE "auth_userprofile"."user_id" IN (1, 2, 3) SELECT "student_courseaccessrole"."id", "student_courseaccessrole"."user_id", "student_courseaccessrole"."org", "student_courseaccessrole"."course_id", "student_courseaccessrole"."role" FROM "student_courseaccessrole" WHERE ("student_courseaccessrole"."course_id" = 'edX/toy/2012_Fall' AND "student_courseaccessrole"."role" IN ('staff', 'instructor', 'finance_admin', 'sales_admin', 'beta_testers', 'library_user', 'data_researcher', 'course_creator_group', 'support') AND "student_courseaccessrole"."user_id" IN (1, 2, 3)) SELECT "student_courseenrollment"."id", "student_courseenrollment"."user_id", "student_courseenrollment"."course_id", "student_courseenrollment"."created", "student_courseenrollment"."is_active", "student_courseenrollment"."mode" FROM "student_courseenrollment" WHERE ("student_courseenrollment"."course_id" = 'edX/toy/2012_Fall' AND "student_courseenrollment"."user_id" IN (1, 2, 3)) ORDER BY "student_courseenrollment"."user_id" ASC, "student_courseenrollment"."course_id" ASCReviewers