diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index e25ae735eb..5a75385551 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -5,7 +5,6 @@ by reversing group name formats. import random from chrono import Timer -from django.contrib.auth.models import Group from django.test import RequestFactory from contentstore.views.course import _accessible_courses_list, _accessible_courses_list_from_groups @@ -32,8 +31,11 @@ class TestCourseListing(ModuleStoreTestCase): """ super(TestCourseListing, self).setUp() # create and log in a staff user. - self.user = UserFactory(is_staff=True) # pylint: disable=no-member + # create and log in a non-staff user + self.user = UserFactory() self.factory = RequestFactory() + self.request = self.factory.get('/course') + self.request.user = self.user self.client = AjaxEnabledTestClient() self.client.login(username=self.user.username, password='test') @@ -64,18 +66,15 @@ class TestCourseListing(ModuleStoreTestCase): """ Test getting courses with new access group format e.g. 'instructor_edx.course.run' """ - request = self.factory.get('/course/') - request.user = self.user - course_location = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_location, self.user) # get courses through iterating all courses - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 1) # get courses by reversing group name formats - courses_list_by_groups = _accessible_courses_list_from_groups(request) + courses_list_by_groups = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 1) # check both course lists have same courses self.assertEqual(courses_list, courses_list_by_groups) @@ -84,18 +83,15 @@ class TestCourseListing(ModuleStoreTestCase): """ Test getting courses with invalid course location (course deleted from modulestore). """ - request = self.factory.get('/course') - request.user = self.user - course_key = SlashSeparatedCourseKey('Org', 'Course', 'Run') self._create_course_with_access_groups(course_key, self.user) # get courses through iterating all courses - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 1) # get courses by reversing group name formats - courses_list_by_groups = _accessible_courses_list_from_groups(request) + courses_list_by_groups = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 1) # check both course lists have same courses self.assertEqual(courses_list, courses_list_by_groups) @@ -106,25 +102,15 @@ class TestCourseListing(ModuleStoreTestCase): CourseInstructorRole(course_key).add_users(self.user) # test that get courses through iterating all courses now returns no course - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 0) - # now test that get courses by reversing group name formats gives 'ItemNotFoundError' - with self.assertRaises(ItemNotFoundError): - _accessible_courses_list_from_groups(request) - def test_course_listing_performance(self): """ Create large number of courses and give access of some of these courses to the user and compare the time to fetch accessible courses for the user through traversing all courses and reversing django groups """ - # create and log in a non-staff user - self.user = UserFactory() - request = self.factory.get('/course') - request.user = self.user - self.client.login(username=self.user.username, password='test') - # create list of random course numbers which will be accessible to the user user_course_ids = random.sample(range(TOTAL_COURSES_COUNT), USER_COURSES_COUNT) @@ -141,22 +127,22 @@ class TestCourseListing(ModuleStoreTestCase): # time the get courses by iterating through all courses with Timer() as iteration_over_courses_time_1: - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) # time again the get courses by iterating through all courses with Timer() as iteration_over_courses_time_2: - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) # time the get courses by reversing django groups with Timer() as iteration_over_groups_time_1: - courses_list = _accessible_courses_list_from_groups(request) + courses_list = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) # time again the get courses by reversing django groups with Timer() as iteration_over_groups_time_2: - courses_list = _accessible_courses_list_from_groups(request) + courses_list = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list), USER_COURSES_COUNT) # test that the time taken by getting courses through reversing django groups is lower then the time @@ -169,21 +155,15 @@ class TestCourseListing(ModuleStoreTestCase): Test getting courses with same id but with different name case. Then try to delete one of them and check that it is properly deleted and other one is accessible """ - # create and log in a non-staff user - self.user = UserFactory() - request = self.factory.get('/course') - request.user = self.user - self.client.login(username=self.user.username, password='test') - course_location_caps = SlashSeparatedCourseKey('Org', 'COURSE', 'Run') self._create_course_with_access_groups(course_location_caps, self.user) # get courses through iterating all courses - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 1) # get courses by reversing group name formats - courses_list_by_groups = _accessible_courses_list_from_groups(request) + courses_list_by_groups = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 1) # check both course lists have same courses self.assertEqual(courses_list, courses_list_by_groups) @@ -193,22 +173,22 @@ class TestCourseListing(ModuleStoreTestCase): self._create_course_with_access_groups(course_location_camel, self.user) # test that get courses through iterating all courses returns both courses - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 2) # test that get courses by reversing group name formats returns both courses - courses_list_by_groups = _accessible_courses_list_from_groups(request) + courses_list_by_groups = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 2) # now delete first course (course_location_caps) and check that it is no longer accessible delete_course_and_groups(course_location_caps, commit=True) # test that get courses through iterating all courses now returns one course - courses_list = _accessible_courses_list(request) + courses_list = _accessible_courses_list(self.request) self.assertEqual(len(courses_list), 1) # test that get courses by reversing group name formats also returns one course - courses_list_by_groups = _accessible_courses_list_from_groups(request) + courses_list_by_groups = _accessible_courses_list_from_groups(self.request) self.assertEqual(len(courses_list_by_groups), 1) # now check that deleted course is not accessible @@ -220,3 +200,29 @@ class TestCourseListing(ModuleStoreTestCase): outline_url = reverse_course_url('course_handler', course_location_camel) response = self.client.get(outline_url, HTTP_ACCEPT='application/json') self.assertEqual(response.status_code, 200) + + def test_course_listing_errored_deleted_courses(self): + """ + Create good courses, courses that won't load, and deleted courses which still have + roles. Test course listing. + """ + course_location = SlashSeparatedCourseKey('testOrg', 'testCourse', 'RunBabyRun') + self._create_course_with_access_groups(course_location, self.user) + + course_location = SlashSeparatedCourseKey('testOrg', 'doomedCourse', 'RunBabyRun') + self._create_course_with_access_groups(course_location, self.user) + modulestore().delete_course(course_location) + + course_location = SlashSeparatedCourseKey('testOrg', 'erroredCourse', 'RunBabyRun') + course = self._create_course_with_access_groups(course_location, self.user) + course_db_record = modulestore()._find_one(course.location) + course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki" }) + modulestore().collection.update( + {'_id': course_db_record['_id']}, + {'$set': { + 'metadata.tabs': course_db_record['metadata']['tabs'], + }}, + ) + + courses_list = _accessible_courses_list_from_groups(self.request) + self.assertEqual(len(courses_list), 1, courses_list) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4f7667db72..5393a3c55f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -189,9 +189,9 @@ def _accessible_courses_list_from_groups(request): course_key = course_access.course_id if course_key not in courses_list: course = modulestore('direct').get_course(course_key) - if course is None: - raise ItemNotFoundError(course_key) - courses_list[course_key] = course + if course is not None and not isinstance(course, ErrorDescriptor): + # ignore deleted or errored courses + courses_list[course_key] = course return courses_list.values() diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index cbffc85d89..7799a685d8 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -208,7 +208,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module.save() return module except: - log.warning("Failed to load descriptor", exc_info=True) + log.warning("Failed to load descriptor from %s", json_data, exc_info=True) return ErrorDescriptor.from_json( json_data, self,