-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[BD-24] [BB-2726] [TNL-7330] Added Course Membership API function #25843
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
Changes from all commits
0e81a41
6d968f9
4b7d2cd
adaeacf
d6c7706
328792d
d832029
13b9343
616b9f4
9be2543
29ceeb7
5f54435
82eafde
ce92c0c
8317eaf
c8e0169
abfd325
b677bb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| Course API | ||
| """ | ||
| import logging | ||
| from collections import defaultdict | ||
|
|
||
| import search | ||
| from django.conf import settings | ||
|
|
@@ -12,7 +13,7 @@ | |
| from opaque_keys.edx.django.models import CourseKeyField | ||
| from rest_framework.exceptions import PermissionDenied | ||
|
|
||
| from common.djangoapps.student.models import CourseAccessRole | ||
| from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment | ||
| from common.djangoapps.student.roles import GlobalStaff | ||
| from lms.djangoapps.courseware.access import has_access | ||
| from lms.djangoapps.courseware.courses import ( | ||
|
|
@@ -25,6 +26,7 @@ | |
| from xmodule.modulestore.django import modulestore | ||
| from xmodule.modulestore.exceptions import ItemNotFoundError | ||
|
|
||
| from .exceptions import OverEnrollmentLimitException | ||
| from .permissions import can_view_courses_for_username | ||
|
|
||
| logger = logging.getLogger(__name__) # pylint: disable=invalid-name | ||
|
|
@@ -266,3 +268,101 @@ def get_course_run_url(request, course_id): | |
| """ | ||
| course_run_url = reverse('openedx.course_experience.course_home', args=[course_id]) | ||
| return request.build_absolute_uri(course_run_url) | ||
|
|
||
|
|
||
| def get_course_members(course_key): | ||
| """ | ||
| Returns a dict containing all users with access to a course through CourseEnrollment | ||
| and CourseAccessRole models. | ||
|
|
||
| User information includes id, email, username, name, enrollment mode and role list. | ||
|
|
||
| This API is limited and will only work for courses with less than a configurable number | ||
| of active enrollments (managed through `settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT`, | ||
| and the default value is 1000). More than that and the method will raise a | ||
| `OverEnrollmentLimitException` exception. | ||
|
|
||
| This method works by querying the `CourseEnrollment` and `CourseAccessRole` models, | ||
| prefetching user information and *then joining results in Python using dictionaries*. | ||
| This approach was choosen to avoid database heavy queries (such as DISTINCT and COUNT) in | ||
| the `CourseEnrollment` table, which would take too long to complete in a request lifecycle. | ||
|
|
||
| The main concern with this approach is the dataset size and resource usage since this method | ||
| returns all enrollments without pagination. We're using a conservative number on the | ||
| `COURSE_MEMBER_API_ENROLLMENT_LIMIT` setting to avoid any issues. | ||
|
|
||
| Examples: | ||
| - Get all course members: | ||
| get_course_members(course_key) | ||
|
|
||
| Arguments: | ||
| course_key (CourseKey): CourseKey to retrieve student data. | ||
|
|
||
| Returns: | ||
| dict: A dictionary with the following format: | ||
| { | ||
| "user_id": { | ||
| "id": 12, | ||
| "username": "jonh5000", | ||
| "email": "jonh@example.com", | ||
| "name": "Jonh Doe", | ||
| "enrollment_mode": "verified", | ||
| "roles": [ | ||
| "student", | ||
| "instructor", | ||
| "staff", | ||
| ] | ||
| } | ||
| } | ||
| """ | ||
| def make_user_info_dict(user, enrollment_mode=None): | ||
| """ | ||
| Utility function to extract user information from model. | ||
| """ | ||
| return { | ||
| "id": user.id, | ||
| "username": user.username, | ||
| "email": user.email, | ||
| "name": user.profile.name, | ||
| "enrollment_mode": enrollment_mode, | ||
| } | ||
|
|
||
| # Raise error if trying to retrieve user list from a course with more than | ||
| # settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT active enrollments. The fastest way | ||
| # to do this is to query for the 1st item after `COURSE_MEMBER_API_ENROLLMENT_LIMIT`. | ||
| over_limit = CourseEnrollment.get_active_enrollments_in_course( | ||
| course_key | ||
| )[settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT:][:1] | ||
| if over_limit.exists(): | ||
| raise OverEnrollmentLimitException( | ||
| f"Can't retrieve course members for {course_key} since it has more than " | ||
| f"{settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT} active enrollments. " | ||
| f"This limit is stored on `settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT`." | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... not that this sort of thing has ever happened to us... 🤧
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add the course key to this error message? Or will it be obvious from some other source in the logs?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nedbat Yes, good idea! The log output now look like this: |
||
|
|
||
| # Python dicts where we're going to manually combine the data from the two querysets | ||
| user_roles = defaultdict(list) | ||
| user_info = {} | ||
|
|
||
| # Retrieve all active enrollments in course and prefetch user information | ||
| enrollments = CourseEnrollment.get_active_enrollments_in_course(course_key) | ||
|
|
||
| # Retrieve all course access roles and prefetch user information | ||
| access_roles = CourseAccessRole.access_roles_in_course(course_key) | ||
|
|
||
| # Evaluates querysets and parses data from the two querysets | ||
| # into `user_info` and `user_roles` dictionaries. | ||
| for enrollment in enrollments: | ||
| user_roles[enrollment.user_id].append('student') | ||
| user_info[enrollment.user_id] = make_user_info_dict(enrollment.user, enrollment.mode) | ||
|
|
||
| for access_role in access_roles: | ||
| user_roles[access_role.user_id].append(access_role.role) | ||
| if access_role.user_id not in user_info: | ||
| user_info[access_role.user_id] = make_user_info_dict(access_role.user) | ||
|
|
||
| # Merge user role information with `user_info` | ||
| for user_id in user_info: | ||
| user_info[user_id]['roles'] = user_roles[user_id] | ||
|
|
||
| return user_info | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| """ | ||
| Course API custom exceptions | ||
| """ | ||
|
|
||
|
|
||
| class OverEnrollmentLimitException(Exception): | ||
| """ | ||
| Exception used by `get_course_members` to signal when a | ||
| course has more enrollments than the limit specified on | ||
| `settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT`. | ||
| """ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import pytest | ||
| from django.contrib.auth.models import AnonymousUser | ||
| from django.http import Http404 | ||
| from django.test import override_settings | ||
| from opaque_keys.edx.keys import CourseKey | ||
| from rest_framework.exceptions import PermissionDenied | ||
| from rest_framework.request import Request | ||
|
|
@@ -19,7 +20,8 @@ | |
| from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase | ||
| from xmodule.modulestore.tests.factories import ItemFactory, check_mongo_calls | ||
|
|
||
| from ..api import UNKNOWN_BLOCK_DISPLAY_NAME, course_detail, get_due_dates, list_courses | ||
| from ..api import UNKNOWN_BLOCK_DISPLAY_NAME, course_detail, get_due_dates, list_courses, get_course_members | ||
| from ..exceptions import OverEnrollmentLimitException | ||
| from .mixins import CourseApiFactoryMixin | ||
|
|
||
|
|
||
|
|
@@ -305,3 +307,89 @@ def test_get_due_dates_error_fetching_block(self): | |
| ] | ||
| actual_due_dates = get_due_dates(request, self.course.id, self.staff_user) | ||
| assert expected_due_dates == actual_due_dates | ||
|
|
||
|
|
||
| class TestGetCourseMembers(CourseApiTestMixin, SharedModuleStoreTestCase): | ||
| """ | ||
| Test get_course_members function | ||
| """ | ||
| @classmethod | ||
| def setUpClass(cls): | ||
| super(TestGetCourseMembers, cls).setUpClass() | ||
| cls.course = cls.create_course() | ||
| cls.honor = cls.create_user('honor', is_staff=False) | ||
| cls.staff = cls.create_user('staff', is_staff=True) | ||
| cls.instructor = cls.create_user('instructor', is_staff=True) | ||
|
|
||
| # Attach honor to course with enrollment | ||
| cls.create_enrollment(user=cls.honor, course_id=cls.course.id) | ||
| # Attach instructor to course with both enrollment and course access role | ||
| cls.create_enrollment(user=cls.instructor, course_id=cls.course.id) | ||
| cls.create_courseaccessrole(user=cls.instructor, course_id=cls.course.id, role='instructor') | ||
| # Attach staff to course using only course access role | ||
| cls.create_courseaccessrole(user=cls.staff, course_id=cls.course.id, role='staff') | ||
|
|
||
| def test_get_course_members(self): | ||
| """ | ||
| Test all different possible filtering | ||
| """ | ||
| with self.assertNumQueries(3): | ||
| members = get_course_members(self.course.id) | ||
|
|
||
| self.assertEqual(len(members), 3) | ||
|
|
||
| # Check parameters for all users | ||
| expected_properties = ['id', 'username', 'email', 'name', 'enrollment_mode', 'roles'] | ||
| for user_id in members: | ||
| self.assertCountEqual(members[user_id], expected_properties) | ||
|
|
||
| # Check that users have correct roles | ||
| # Honor should be only a student and have the enrollment mode set | ||
| self.assertEqual(members[self.honor.id]['roles'], ['student']) | ||
| self.assertEqual(members[self.honor.id]['enrollment_mode'], 'audit') | ||
| # Instructor should have both roles and enrollment_mode set | ||
| self.assertEqual(members[self.instructor.id]['roles'], ['student', 'instructor']) | ||
| self.assertEqual(members[self.instructor.id]['enrollment_mode'], 'audit') | ||
| # Staff should only have the staff role | ||
| self.assertEqual(members[self.staff.id]['roles'], ['staff']) | ||
| self.assertEqual(members[self.staff.id]['enrollment_mode'], None) | ||
|
|
||
| def test_same_result_with_csa_or_enrollment(self): | ||
| """ | ||
| Checks that the API returns the same result regardless if a user | ||
| comes from CourseAccessRoles or CourseEnrollments table. | ||
| """ | ||
| # Create new user | ||
| user = TestGetCourseMembers.create_user('test_use', is_staff=True) | ||
|
|
||
| # Attach with course enrollment | ||
| enrollment = TestGetCourseMembers.create_enrollment( | ||
| user=user, | ||
| course_id=self.course.id | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
| members_enrollments = get_course_members(self.course.id) | ||
| enrollment.delete() | ||
|
|
||
| # Attach with course enrollment | ||
| enrollment = TestGetCourseMembers.create_courseaccessrole( | ||
| user=user, | ||
| course_id=self.course.id, | ||
| role='staff', | ||
| ) | ||
| members_courseaccessroles = get_course_members(self.course.id) | ||
|
|
||
| # Check properties (except the ones that change depending on role) | ||
| for item in ['id', 'username', 'email', 'name']: | ||
| self.assertEqual( | ||
| members_courseaccessroles[user.id][item], | ||
| members_enrollments[user.id][item] | ||
| ) | ||
|
|
||
| @override_settings(COURSE_MEMBER_API_ENROLLMENT_LIMIT=1) | ||
| def test_course_members_fails_overlimit(self): | ||
| """ | ||
| Check if trying to retrieve more than settings.COURSE_MEMBER_API_ENROLLMENT_LIMIT | ||
| fails. | ||
| """ | ||
| with self.assertRaises(OverEnrollmentLimitException): | ||
| get_course_members(self.course.id) | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
The docstring should clearly indicate the too-large-course error case, and answer the following: