From 79d097d3c82820a330dbdc291f78ca29f3e5144f Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 29 Jan 2020 16:33:57 -0500 Subject: [PATCH] Remove the role parameter from the courses api. BOM-1228 The api is already not very performant and trying to limit it to only show courses where you are staff causes the code to iterate over almost all the courses and times out before it returns any results to the user. The plan is to build a different api for the thing we need that will just provide the course IDs for courses where you are staff and sholud be much faster. --- lms/djangoapps/course_api/api.py | 27 +----- lms/djangoapps/course_api/forms.py | 3 +- lms/djangoapps/course_api/tests/test_forms.py | 1 - lms/djangoapps/course_api/tests/test_views.py | 84 ------------------- lms/djangoapps/course_api/views.py | 9 -- 5 files changed, 2 insertions(+), 122 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index f86f6f5e0a..df27af994d 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -74,25 +74,6 @@ def course_detail(request, username, course_key): return overview -def _filter_by_role(course_queryset, user, roles): - """ - Filters a course queryset by the roles for which the user has access. - """ - # Global staff have access to all courses. Filter course roles for non-global staff only. - if not user.is_staff: - if roles: - for role in roles: - # Filter the courses again to return only the courses for which the user has each specified role. - course_queryset = LazySequence( - ( - course for course in course_queryset - if has_access(user, role, course.id) - ), - est_len=len(course_queryset) - ) - return course_queryset - - def _filter_by_search(course_queryset, search_term): """ Filters a course queryset by the specified search term. @@ -121,8 +102,7 @@ def _filter_by_search(course_queryset, search_term): ) -@function_trace('list_courses') -def list_courses(request, username, org=None, roles=None, filter_=None, search_term=None): +def list_courses(request, username, org=None, filter_=None, search_term=None): """ Yield all available courses. @@ -143,10 +123,6 @@ def list_courses(request, username, org=None, roles=None, filter_=None, search_t If specified, visible `CourseOverview` objects are filtered such that only those belonging to the organization with the provided org code (e.g., "HarvardX") are returned. Case-insensitive. - roles (list of strings): - If specified, visible `CourseOverview` objects are filtered - such that only those for which the user has the specified role(s) - are returned. Multiple role parameters can be specified. filter_ (dict): If specified, visible `CourseOverview` objects are filtered by the given key-value pairs. @@ -158,7 +134,6 @@ def list_courses(request, username, org=None, roles=None, filter_=None, search_t """ user = get_effective_user(request.user, username) course_qs = get_courses(user, org=org, filter_=filter_) - course_qs = _filter_by_role(course_qs, user, roles) course_qs = _filter_by_search(course_qs, search_term) return course_qs diff --git a/lms/djangoapps/course_api/forms.py b/lms/djangoapps/course_api/forms.py index f92d9658d7..cc6c3593fd 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -11,7 +11,7 @@ from django.forms import CharField, Form from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.util.forms import ExtendedNullBooleanField, MultiValueField +from openedx.core.djangoapps.util.forms import ExtendedNullBooleanField class UsernameValidatorMixin(object): @@ -53,7 +53,6 @@ class CourseListGetForm(UsernameValidatorMixin, Form): search_term = CharField(required=False) username = CharField(required=False) org = CharField(required=False) - role = MultiValueField(required=False) # white list of all supported filter fields filter_type = namedtuple('filter_type', ['param_name', 'field_name']) diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index bee4f1dc1e..faaf90e083 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -68,7 +68,6 @@ class TestCourseListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreT self.cleaned_data = { 'username': user.username, 'org': '', - 'role': set([]), 'mobile': None, 'search_term': '', 'filter_': None, diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 70264e0911..44b8b7d26b 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -23,9 +23,6 @@ from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig -from student.auth import add_users -from student.roles import CourseInstructorRole, CourseStaffRole -from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -162,7 +159,6 @@ class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreT self.url = reverse('course-list') self.staff_user = self.create_user(username='staff', is_staff=True) self.honor_user = self.create_user(username='honor', is_staff=False) - self.global_admin = AdminFactory() def test_filter_by_org(self): """Verify that CourseOverviews are filtered by the provided org key.""" @@ -210,86 +206,6 @@ class CourseListViewTestCaseMultipleCourses(CourseApiTestViewMixin, ModuleStoreT u"testing course_api.views.CourseListView with filter_={}".format(filter_), ) - def test_filter_by_roles_global_staff(self): - """ - Verify that global staff are always returned all courses irregardless of role filter. - """ - self.setup_user(self.staff_user) - - # Create a second course to be filtered out of queries. - alternate_course = self.create_course(org=md5(self.course.org.encode('utf-8')).hexdigest()) - - # Request the courses as the staff user with the different roles specified. - for roles in ('', 'staff', 'staff,instructor'): - filtered_response = self.verify_response(params={'username': self.staff_user.username, 'role': roles}) - # Both courses should be returned in the course list. - for org in [self.course.org, alternate_course.org]: - self.assertTrue( - any(course['org'] == org for course in filtered_response.data['results']) - ) - - def test_filter_by_roles_non_staff(self): - """ - Verify that a non-staff user can't access CourseOverviews by role. - """ - - # Requesting the courses as the non-staff user should *not* be allowed. - self.setup_user(self.honor_user) - filtered_response = self.verify_response(params={'username': self.honor_user.username, 'role': 'staff'}) - self.assertEqual(len(filtered_response.data['results']), 0) - - def test_filter_by_roles_course_staff(self): - """ - Verify that CourseOverviews are filtered by the provided roles. - """ - # Make this user a course staff user for the course. - course_staff_user = self.create_user(username='course_staff', is_staff=False) - add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user) - - # Create a second course to be filtered out of queries, along with an instructor user for it. - alternate_course = self.create_course(org=md5(self.course.org.encode('utf-8')).hexdigest()) - course_instructor_user = self.create_user(username='course_instructor', is_staff=False) - add_users(self.global_admin, CourseInstructorRole(alternate_course.id), course_instructor_user) - - # Requesting the courses for which the course staff user is staff should return *only* the single course. - self.setup_user(self.staff_user) - filtered_response = self.verify_response(params={ - 'username': course_staff_user.username, - 'role': 'staff' - }) - self.assertEqual(len(filtered_response.data['results']), 1) - self.assertEqual(filtered_response.data['results'][0]['org'], self.course.org) - - # The course staff user does *not* have the course instructor role on any courses. - filtered_response = self.verify_response(params={ - 'username': course_staff_user.username, - 'role': 'instructor' - }) - self.assertEqual(len(filtered_response.data['results']), 0) - - # The course instructor user only has the course instructor role on one course. - filtered_response = self.verify_response(params={ - 'username': course_instructor_user.username, - 'role': 'instructor' - }) - self.assertEqual(len(filtered_response.data['results']), 1) - self.assertEqual(filtered_response.data['results'][0]['org'], alternate_course.org) - - # The honor user does *not* have the course staff -or- instructor role on any courses. - filtered_response = self.verify_response(params={ - 'username': self.honor_user.username, - 'role': 'staff,instructor' - }) - self.assertEqual(len(filtered_response.data['results']), 0) - - # The course instructor user has the inferred course staff role on one course. - self.setup_user(course_instructor_user) - filtered_response = self.verify_response(params={ - 'username': course_instructor_user.username, - 'role': 'staff' - }) - self.assertEqual(len(filtered_response.data['results']), 1) - class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 7320fea86e..efed2458b3 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -191,12 +191,6 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): provided org code (e.g., "HarvardX") are returned. Case-insensitive. - role (optional): - If specified, visible `CourseOverview` objects are filtered - such that only those for which the user has the specified role - are returned. Multiple role parameters can be specified. - Case-insensitive. - **Returns** * 200 on success, with a list of course discovery objects as returned @@ -247,13 +241,10 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): if not form.is_valid(): raise ValidationError(form.errors) - set_custom_metric('query_param_roles', form.cleaned_data['role']) - return list_courses( self.request, form.cleaned_data['username'], org=form.cleaned_data['org'], - roles=form.cleaned_data['role'], filter_=form.cleaned_data['filter_'], search_term=form.cleaned_data['search_term'] )