diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 349c05a5ba..86aa3c9d39 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -18,8 +18,10 @@ from lms.djangoapps.courseware.courses import ( get_courses, get_permission_for_course_about ) +from opaque_keys.edx.django.models import CourseKeyField from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.view_utils import LazySequence +from student.models import CourseAccessRole from student.roles import GlobalStaff from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -170,19 +172,43 @@ def list_course_keys(request, username, role): """ user = get_effective_user(request.user, username) - course_keys = CourseOverview.get_all_course_keys() + all_course_keys = CourseOverview.get_all_course_keys() # Global staff have access to all courses. Filter courses for non-global staff. if GlobalStaff().has_user(user): - return course_keys + return all_course_keys - return LazySequence( - ( - course_key for course_key in course_keys - if has_access(user, role, course_key) - ), - est_len=len(course_keys) - ) + if role == 'staff': + # This short-circuit implementation bypasses has_access() which we think is too slow for some users when + # evaluating staff-level course access for Insights. Various tickets have context on this issue: CR-2487, + # TNL-7448, DESUPPORT-416, and probably more. + # + # This is a simplified implementation that does not consider org-level access grants (e.g. when course_id is + # empty). + filtered_course_keys = ( + CourseAccessRole.objects.filter( + user=user, + # Having the instructor role implies staff access. + role__in=['staff', 'instructor'], + ) + # We need to check against CourseOverview so that we don't return any Libraries. + .extra(tables=['course_overviews_courseoverview'], where=['course_id = course_overviews_courseoverview.id']) + # For good measure, make sure we don't return empty course IDs. + .exclude(course_id=CourseKeyField.Empty) + .order_by('course_id') + .values_list('course_id', flat=True) + .distinct() + ) + else: + # This is the original implementation which still covers the case where role = "instructor": + filtered_course_keys = LazySequence( + ( + course_key for course_key in all_course_keys + if has_access(user, role, course_key) + ), + est_len=len(all_course_keys) + ) + return filtered_course_keys def get_due_dates(request, course_key, user): diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index efdd4678ad..8378481caf 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -22,6 +22,7 @@ from waffle.testutils import override_switch from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory +from opaque_keys.edx.locator import LibraryLocator from openedx.core.lib.api.view_utils import LazySequence from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig @@ -459,9 +460,15 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user) # Create a second course, along with an instructor user for it. - alternate_course = self.create_course(org='test') + alternate_course1 = self.create_course(org='test1') course_instructor_user = self.create_user(username='course_instructor', is_staff=False) - add_users(self.global_admin, CourseInstructorRole(alternate_course.id), course_instructor_user) + add_users(self.global_admin, CourseInstructorRole(alternate_course1.id), course_instructor_user) + + # Create a third course, along with an user that has both staff and instructor for it. + alternate_course2 = self.create_course(org='test2') + course_instructor_staff_user = self.create_user(username='course_instructor_staff', is_staff=False) + add_users(self.global_admin, CourseInstructorRole(alternate_course2.id), course_instructor_staff_user) + add_users(self.global_admin, CourseStaffRole(alternate_course2.id), course_instructor_staff_user) # Requesting the courses for which the course staff user is staff should return *only* the single course. self.setup_user(self.staff_user) @@ -485,7 +492,7 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): 'role': 'instructor' }) self.assertEqual(len(filtered_response.data['results']), 1) - self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org)) + self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course1.org)) # The course instructor user has the inferred course staff role on one course. self.setup_user(course_instructor_user) @@ -494,7 +501,38 @@ class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): 'role': 'staff' }) self.assertEqual(len(filtered_response.data['results']), 1) - self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org)) + self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course1.org)) + + # The user with both instructor AND staff on a course has the inferred course staff role on that one course. + self.setup_user(course_instructor_staff_user) + filtered_response = self.verify_response(params={ + 'username': course_instructor_staff_user.username, + 'role': 'staff' + }) + self.assertEqual(len(filtered_response.data['results']), 1) + self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course2.org)) + + def test_no_libraries(self): + """ + Verify that only Course IDs are returned, not anything else like libraries. + """ + # Make this user a course staff user for a course, AND a library. + course_staff_user = self.create_user(username='course_staff', is_staff=False) + add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user) + add_users( + self.global_admin, + CourseStaffRole(LibraryLocator.from_string('library-v1:library_org+library_name')), + course_staff_user, + ) + + # Requesting the courses should return *only* courses and not libraries. + 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.assertTrue(filtered_response.data['results'][0].startswith(self.course.org)) class LazyPageNumberPaginationTestCase(TestCase):