From 2a8a58ae6b566dcd5262ae08d9c1173890e1c57c Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 3 Jun 2021 19:38:13 +0600 Subject: [PATCH] feat: add Course Membership API 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 --- common/djangoapps/student/models.py | 26 +++++ lms/djangoapps/course_api/api.py | 102 +++++++++++++++++++- lms/djangoapps/course_api/exceptions.py | 11 +++ lms/djangoapps/course_api/tests/mixins.py | 16 ++- lms/djangoapps/course_api/tests/test_api.py | 90 ++++++++++++++++- lms/envs/common.py | 11 +++ 6 files changed, 253 insertions(+), 3 deletions(-) create mode 100644 lms/djangoapps/course_api/exceptions.py diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 48b05287f5..9b20142039 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -2239,6 +2239,20 @@ class CourseEnrollment(models.Model): """ cache[(user_id, course_key)] = enrollment_state + @classmethod + def get_active_enrollments_in_course(cls, course_key): + """ + Retrieves active CourseEnrollments for a given course and + prefetches user information. + """ + return cls.objects.filter( + course_id=course_key, + is_active=True, + ).select_related( + 'user', + 'user__profile', + ) + @python_2_unicode_compatible class FBEEnrollmentExclusion(models.Model): @@ -2448,6 +2462,18 @@ class CourseAccessRole(models.Model): """ return (self.role, self.org, self.course_id, self.user_id) + @classmethod + def access_roles_in_course(cls, course_key): + """ + Returns all CourseAccessRole for a given course and prefetches user information. + """ + return cls.objects.filter( + course_id=course_key, + ).select_related( + 'user', + 'user__profile' + ) + def __eq__(self, other): """ Overriding eq b/c the django impl relies on the primary key which requires fetch. sometimes we diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index d43aa3f53d..122c637881 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -2,6 +2,7 @@ Course API """ import logging +from collections import defaultdict import search from django.conf import settings @@ -12,7 +13,7 @@ from edx_when.api import get_dates_for_course 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 openedx.core.lib.api.view_utils import LazySequence 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`." + ) + + # 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 diff --git a/lms/djangoapps/course_api/exceptions.py b/lms/djangoapps/course_api/exceptions.py new file mode 100644 index 0000000000..53065c286b --- /dev/null +++ b/lms/djangoapps/course_api/exceptions.py @@ -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`. + """ diff --git a/lms/djangoapps/course_api/tests/mixins.py b/lms/djangoapps/course_api/tests/mixins.py index 4ab243ee4f..614b56743b 100644 --- a/lms/djangoapps/course_api/tests/mixins.py +++ b/lms/djangoapps/course_api/tests/mixins.py @@ -5,7 +5,7 @@ Common mixins for Course API Tests from datetime import datetime -from common.djangoapps.student.tests.factories import UserFactory +from common.djangoapps.student.tests.factories import UserFactory, CourseEnrollmentFactory, CourseAccessRoleFactory from xmodule.modulestore.tests.factories import ToyCourseFactory TEST_PASSWORD = 'edx' @@ -41,3 +41,17 @@ class CourseApiFactoryMixin: password=TEST_PASSWORD, is_staff=is_staff ) + + @staticmethod + def create_enrollment(**kwargs): + """ + Create a CourseEnrollment to use in tests. + """ + return CourseEnrollmentFactory(**kwargs) + + @staticmethod + def create_courseaccessrole(**kwargs): + """ + Create a CourseAccessRole to use in tests. + """ + return CourseAccessRoleFactory(**kwargs) diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index b0af5bb37d..7f838a1b7e 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -9,6 +9,7 @@ from unittest import mock 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.exceptions import ItemNotFoundError 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 @@ class TestGetCourseDates(CourseDetailTestMixin, SharedModuleStoreTestCase): ] 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 + ) + 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) diff --git a/lms/envs/common.py b/lms/envs/common.py index 6946d5c8a8..fa04bbe486 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4029,6 +4029,17 @@ PROFILE_IMAGE_SIZES_MAP = { # If set to None, all courses will be listed on the homepage HOMEPAGE_COURSE_MAX = None +# .. setting_name: COURSE_MEMBER_API_ENROLLMENT_LIMIT +# .. setting_implementation: DjangoSetting +# .. setting_default: 1000 +# .. setting_description: This limits the response size of the `get_course_members` API, throwing an exception +# if the number of Enrolled users is greater than this number. This is needed to limit the dataset size +# since the API does most of the calculation in Python to avoid expensive database queries. +# .. setting_use_cases: open_edx +# .. setting_creation_date: 2021-05-18 +# .. setting_tickets: https://openedx.atlassian.net/browse/TNL-7330 +COURSE_MEMBER_API_ENROLLMENT_LIMIT = 1000 + ################################ Settings for Credit Courses ################################ # Initial delay used for retrying tasks. # Additional retries use longer delays.