Make reverse matching robust to errored courses

Conflicts:
	cms/djangoapps/contentstore/tests/test_course_listing.py

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)

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()
 

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,