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