Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove the role parameter from the courses api.

Browse Source 
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.
This commit is contained in:
Feanil Patel
2020-01-29 16:33:57 -05:00
parent 4bbdf2e20b
commit 79d097d3c8
5 changed files with 2 additions and 122 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
lms/djangoapps/course_api/api.py
View File

@@ -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

3
lms/djangoapps/course_api/forms.py
View File

@@ -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'])

1
lms/djangoapps/course_api/tests/test_forms.py
View File

@@ -68,7 +68,6 @@ class TestCourseListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreT
self.cleaned_data = {
'username': user.username,
'org': '',
'role': set([]),
'mobile': None,
'search_term': '',
'filter_': None,

84
lms/djangoapps/course_api/tests/test_views.py
View File

@@ -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):
"""

9
lms/djangoapps/course_api/views.py
View File

@@ -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']
)