Merge pull request #16173 from edx/noraiz/EDUCATOR-1424

use CourseSummary for course listing on studio for non-global staff users

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -16,7 +16,7 @@ from contentstore.tests.utils import AjaxEnabledTestClient
 from contentstore.utils import delete_course
 from contentstore.views.course import (
     AccessListFallback,
-    _accessible_courses_iter,
+    _accessible_courses_iter_for_tests,
     _accessible_courses_list_from_groups,
     _accessible_courses_summary_iter,
     get_courses_accessible_to_user
@@ -103,7 +103,7 @@ class TestCourseListing(ModuleStoreTestCase):
         self._create_course_with_access_groups(course_location, self.user)
 
         # get courses through iterating all courses
-        courses_iter, __ = _accessible_courses_iter(self.request)
+        courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
         courses_list = list(courses_iter)
         self.assertEqual(len(courses_list), 1)
 
@@ -115,7 +115,10 @@ class TestCourseListing(ModuleStoreTestCase):
         self.assertEqual(len(courses_list_by_groups), 1)
 
         # check both course lists have same courses
-        self.assertEqual(courses_list, courses_list_by_groups)
+        course_keys_in_course_list = [course.id for course in courses_list]
+        course_keys_in_courses_list_by_groups = [course.id for course in courses_list_by_groups]
+
+        self.assertEqual(course_keys_in_course_list, course_keys_in_courses_list_by_groups)
 
     def test_courses_list_with_ccx_courses(self):
         """
@@ -150,37 +153,11 @@ class TestCourseListing(ModuleStoreTestCase):
 
         # Verify that CCX courses are filtered out while iterating over all courses
         mocked_ccx_course = Mock(id=ccx_course_key)
-        with patch('xmodule.modulestore.mixed.MixedModuleStore.get_courses', return_value=[mocked_ccx_course]):
-            courses_iter, __ = _accessible_courses_iter(self.request)
+        with patch('xmodule.modulestore.mixed.MixedModuleStore.get_course_summaries', return_value=[mocked_ccx_course]):
+            courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
             self.assertEqual(len(list(courses_iter)), 0)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'),
-        (ModuleStoreEnum.Type.mongo, 'xmodule.modulestore.mongo.base.MongoKeyValueStore')
-    )
-    @ddt.unpack
-    def test_errored_course_global_staff(self, store, path_to_patch):
-        """
-        Test the course list for global staff when get_course returns an ErrorDescriptor
-        """
-        GlobalStaff().add_users(self.user)
-
-        with self.store.default_store(store):
-            course_key = self.store.make_course_key('Org1', 'Course1', 'Run1')
-            self._create_course_with_access_groups(course_key, self.user, store=store)
-
-            with patch(path_to_patch, Mock(side_effect=Exception)):
-                self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor)
-
-                # get courses through iterating all courses
-                courses_iter, __ = _accessible_courses_iter(self.request)
-                self.assertEqual(list(courses_iter), [])
-
-                # get courses by reversing group name formats
-                courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
-                self.assertEqual(courses_list_by_groups, [])
-
-    @ddt.data(
         (ModuleStoreEnum.Type.split, 3),
         (ModuleStoreEnum.Type.mongo, 2)
     )
@@ -212,36 +189,6 @@ class TestCourseListing(ModuleStoreTestCase):
         with check_mongo_calls(mongo_calls):
             list(_accessible_courses_summary_iter(self.request))
 
-    @ddt.data(
-        (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'),
-        (ModuleStoreEnum.Type.mongo, 'xmodule.modulestore.mongo.base.MongoKeyValueStore')
-    )
-    @ddt.unpack
-    def test_errored_course_regular_access(self, store, path_to_patch):
-        """
-        Test the course list for regular staff when get_course returns an ErrorDescriptor
-        """
-        GlobalStaff().remove_users(self.user)
-
-        with self.store.default_store(store):
-            CourseStaffRole(self.store.make_course_key('Non', 'Existent', 'Course')).add_users(self.user)
-
-            course_key = self.store.make_course_key('Org1', 'Course1', 'Run1')
-            self._create_course_with_access_groups(course_key, self.user, store)
-
-            with patch(path_to_patch, Mock(side_effect=Exception)):
-                self.assertIsInstance(self.store.get_course(course_key), ErrorDescriptor)
-
-                # get courses through iterating all courses
-                courses_iter, __ = _accessible_courses_iter(self.request)
-                courses_list = list(courses_iter)
-                self.assertEqual(courses_list, [])
-
-                # get courses by reversing group name formats
-                courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
-                self.assertEqual(courses_list_by_groups, [])
-                self.assertEqual(courses_list, courses_list_by_groups)
-
     @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
     def test_get_course_list_with_invalid_course_location(self, store):
         """
@@ -252,7 +199,7 @@ class TestCourseListing(ModuleStoreTestCase):
             self._create_course_with_access_groups(course_key, self.user, store)
 
         # get courses through iterating all courses
-        courses_iter, __ = _accessible_courses_iter(self.request)
+        courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
         courses_list = list(courses_iter)
         self.assertEqual(len(courses_list), 1)
 
@@ -268,16 +215,17 @@ class TestCourseListing(ModuleStoreTestCase):
         courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
         self.assertEqual(len(courses_list_by_groups), 1)
 
+        course_keys_in_course_list = [course.id for course in courses_list]
+        course_keys_in_courses_list_by_groups = [course.id for course in courses_list_by_groups]
         # check course lists have same courses
-        self.assertEqual(courses_list, courses_list_by_groups)
-
+        self.assertEqual(course_keys_in_course_list, course_keys_in_courses_list_by_groups)
         # now delete this course and re-add user to instructor group of this course
         delete_course(course_key, self.user.id)
 
         CourseInstructorRole(course_key).add_users(self.user)
 
         # Get courses through iterating all courses
-        courses_iter, __ = _accessible_courses_iter(self.request)
+        courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
 
         # Get course summaries by iterating all courses
         courses_summary_iter, __ = _accessible_courses_summary_iter(self.request)
@@ -292,8 +240,8 @@ class TestCourseListing(ModuleStoreTestCase):
         )
 
     @ddt.data(
-        (ModuleStoreEnum.Type.split, 4, 23),
-        (ModuleStoreEnum.Type.mongo, USER_COURSES_COUNT, 2)
+        (ModuleStoreEnum.Type.split, 3, 3),
+        (ModuleStoreEnum.Type.mongo, 2, 2)
     )
     @ddt.unpack
     def test_course_listing_performance(self, store, courses_list_from_group_calls, courses_list_calls):
@@ -319,12 +267,12 @@ class TestCourseListing(ModuleStoreTestCase):
 
         # time the get courses by iterating through all courses
         with Timer() as iteration_over_courses_time_1:
-            courses_iter, __ = _accessible_courses_iter(self.request)
+            courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
         self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT)
 
         # time again the get courses by iterating through all courses
         with Timer() as iteration_over_courses_time_2:
-            courses_iter, __ = _accessible_courses_iter(self.request)
+            courses_iter, __ = _accessible_courses_iter_for_tests(self.request)
         self.assertEqual(len(list(courses_iter)), USER_COURSES_COUNT)
 
         # time the get courses by reversing django groups
@@ -337,25 +285,18 @@ class TestCourseListing(ModuleStoreTestCase):
             courses_list, __ = _accessible_courses_list_from_groups(self.request)
         self.assertEqual(len(courses_list), USER_COURSES_COUNT)
 
-        # TODO (cdyer) : iteration over courses was optimized, and is now
-        # sometimes faster than iteration over groups. One of the following
-        # should be done to resolve this:
-        # * Iteration over groups should be sped up.
-        # * Iteration over groups should be removed, as it no longer saves time.
-        # * Or this part of the test should be removed.
-
         # Test that the time taken by getting courses through reversing django
         # groups is lower then the time taken by traversing through all courses
         # (if accessible courses are relatively small).
-        #self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed)
-        #self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed)
+        self.assertGreaterEqual(iteration_over_courses_time_1.elapsed, iteration_over_groups_time_1.elapsed)
+        self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed)
 
         # Now count the db queries
         with check_mongo_calls(courses_list_from_group_calls):
             _accessible_courses_list_from_groups(self.request)
 
         with check_mongo_calls(courses_list_calls):
-            list(_accessible_courses_iter(self.request))
+            list(_accessible_courses_iter_for_tests(self.request))
         # Calls:
         #    1) query old mongo
         #    2) get_more on old mongo
@@ -436,13 +377,12 @@ class TestCourseListing(ModuleStoreTestCase):
             )
 
         # verify return values
-        for method in (_accessible_courses_list_from_groups, _accessible_courses_iter):
-            def set_of_course_keys(course_list, key_attribute_name='id'):
-                """Returns a python set of course keys by accessing the key with the given attribute name."""
-                return set(getattr(c, key_attribute_name) for c in course_list)
-
-            found_courses, unsucceeded_course_actions = method(self.request)
-            self.assertSetEqual(set_of_course_keys(courses + courses_in_progress), set_of_course_keys(found_courses))
-            self.assertSetEqual(
-                set_of_course_keys(courses_in_progress), set_of_course_keys(unsucceeded_course_actions, 'course_key')
-            )
+        def _set_of_course_keys(course_list, key_attribute_name='id'):
+            """Returns a python set of course keys by accessing the key with the given attribute name."""
+            return set(getattr(c, key_attribute_name) for c in course_list)
+
+        found_courses, unsucceeded_course_actions = _accessible_courses_iter_for_tests(self.request)
+        self.assertSetEqual(_set_of_course_keys(courses + courses_in_progress), _set_of_course_keys(found_courses))
+        self.assertSetEqual(
+            _set_of_course_keys(courses_in_progress), _set_of_course_keys(unsucceeded_course_actions, 'course_key')
+        )

cms/djangoapps/contentstore/views/course.py

@@ -390,7 +390,6 @@ def _accessible_courses_summary_iter(request, org=None):
 def _accessible_courses_iter(request):
     """
     List all courses available to the logged in user by iterating through all the courses.
-    This method is only used by tests.
     """
     def course_filter(course):
         """
@@ -417,6 +416,35 @@ def _accessible_courses_iter(request):
     return courses, in_process_course_actions
 
 
+def _accessible_courses_iter_for_tests(request):
+    """
+    List all courses available to the logged in user by iterating through all the courses.
+    CourseSummary objects are used for listing purposes.
+    This method is only used by tests.
+    """
+    def course_filter(course):
+        """
+        Filter out unusable and inaccessible courses
+        """
+
+        # Custom Courses for edX (CCX) is an edX feature for re-using course content.
+        # CCXs cannot be edited in Studio (aka cms) and should not be shown in this dashboard.
+        if isinstance(course.id, CCXLocator):
+            return False
+
+        # pylint: disable=fixme
+        # TODO remove this condition when templates purged from db
+        if course.location.course == 'templates':
+            return False
+
+        return has_studio_read_access(request.user, course.id)
+
+    courses = six.moves.filter(course_filter, modulestore().get_course_summaries())
+
+    in_process_course_actions = get_in_process_course_actions(request)
+    return courses, in_process_course_actions
+
+
 def _accessible_courses_list_from_groups(request):
     """
     List all courses available to the logged in user by reversing access group names
@@ -425,39 +453,23 @@ def _accessible_courses_list_from_groups(request):
         """ CCXs cannot be edited in Studio and should not be shown in this dashboard """
         return not isinstance(course_access.course_id, CCXLocator)
 
-    courses_list = {}
-    in_process_course_actions = []
-
     instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role()
     staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role()
     all_courses = filter(filter_ccx, instructor_courses | staff_courses)
+    courses_list = []
+    course_keys = {}
 
     for course_access in all_courses:
-        course_key = course_access.course_id
-        if course_key is None:
-            # If the course_access does not have a course_id, it's an org-based role, so we fall back
+        if course_access.course_id is None:
             raise AccessListFallback
-        if course_key not in courses_list:
-            # check for any course action state for this course
-            in_process_course_actions.extend(
-                CourseRerunState.objects.find_all(
-                    exclude_args={'state': CourseRerunUIStateManager.State.SUCCEEDED},
-                    should_display=True,
-                    course_key=course_key,
-                )
-            )
-            # check for the course itself
-            try:
-                course = modulestore().get_course(course_key)
-            except ItemNotFoundError:
-                # If a user has access to a course that doesn't exist, don't do anything with that course
-                pass
+        course_keys[course_access.course_id] = course_access.course_id
+
+    course_keys = course_keys.values()
 
-            if course is not None and not isinstance(course, ErrorDescriptor):
-                # ignore deleted, errored or ccx courses
-                courses_list[course_key] = course
+    if course_keys:
+        courses_list = modulestore().get_course_summaries(course_keys=course_keys)
 
-    return courses_list.values(), in_process_course_actions
+    return courses_list, []
 
 
 def _accessible_libraries_iter(user, org=None):

cms/djangoapps/contentstore/views/tests/test_course_index.py

@@ -392,8 +392,8 @@ class TestCourseIndexArchived(CourseTestCase):
 
     @ddt.data(
         # Staff user has course staff access
-        (True, 'staff', None, 4, 21),
-        (False, 'staff', None, 4, 21),
+        (True, 'staff', None, 3, 17),
+        (False, 'staff', None, 3, 17),
         # Base user has global staff access
         (True, 'user', ORG, 3, 17),
         (False, 'user', ORG, 3, 17),

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -1010,11 +1010,23 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 if field in course['metadata']
             }
 
-        course_org_filter = kwargs.get('org')
+        course_records = []
         query = {'_id.category': 'course'}
-
-        if course_org_filter:
-            query['_id.org'] = course_org_filter
+        course_org_filter = kwargs.get('org')
+        course_keys = kwargs.get('course_keys')
+
+        if course_keys:
+            course_queries = []
+            for course_key in course_keys:
+                course_query = {
+                    '_id.{}'.format(value_attr): getattr(course_key, key_attr)
+                    for key_attr, value_attr in {'org': 'org', 'course': 'course', 'run': 'name'}.iteritems()
+                }
+                course_query.update(query)
+                course_queries.append(course_query)
+            query = {'$or': course_queries}
+        elif course_org_filter:
+                query['_id.org'] = course_org_filter
 
         course_records = self.collection.find(query, {'metadata': True})
 
@@ -1028,6 +1040,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 courses_summaries.append(
                     CourseSummary(locator, **course_summary)
                 )
+
         return courses_summaries
 
     @autoretry_read()

common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py

@@ -451,7 +451,15 @@ class MongoConnection(object):
                 }
             return self.course_index.find_one(query)
 
-    def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None, course_context=None):
+    def find_matching_course_indexes(
+            self,
+            branch=None,
+            search_targets=None,
+            org_target=None,
+            course_context=None,
+            course_keys=None
+
+    ):
         """
         Find the course_index matching particular conditions.
 
@@ -464,18 +472,41 @@ class MongoConnection(object):
         """
         with TIMER.timer("find_matching_course_indexes", course_context):
             query = {}
-            if branch is not None:
-                query['versions.{}'.format(branch)] = {'$exists': True}
+            if course_keys:
+                courses_queries = self._generate_query_from_course_keys(branch, course_keys)
+                query['$or'] = courses_queries
+            else:
+                if branch is not None:
+                    query['versions.{}'.format(branch)] = {'$exists': True}
 
-            if search_targets:
-                for key, value in search_targets.iteritems():
-                    query['search_targets.{}'.format(key)] = value
+                if search_targets:
+                    for key, value in search_targets.iteritems():
+                        query['search_targets.{}'.format(key)] = value
 
-            if org_target:
-                query['org'] = org_target
+                if org_target:
+                    query['org'] = org_target
 
             return self.course_index.find(query)
 
+    def _generate_query_from_course_keys(self, branch, course_keys):
+        """
+        Generate query for courses using course keys
+        """
+        courses_queries = []
+        query = {}
+        if branch:
+            query = {'versions.{}'.format(branch): {'$exists': True}}
+
+        for course_key in course_keys:
+            course_query = {
+                key_attr: getattr(course_key, key_attr)
+                for key_attr in ('org', 'course', 'run')
+            }
+            course_query.update(query)
+            courses_queries.append(course_query)
+
+        return courses_queries
+
     def insert_course_index(self, course_index, course_context=None):
         """
         Create the course_index in the db

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -497,7 +497,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
             block_data.edit_info.original_usage = original_usage
             block_data.edit_info.original_usage_version = original_usage_version
 
-    def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None):
+    def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None, course_keys=None):
         """
         Find the course_indexes which have the specified branch and search_targets. An optional org_target
         can be specified to apply an ORG filter to return only the courses that are part of
@@ -506,19 +506,44 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         Returns:
             a Cursor if there are no changes in flight or a list if some have changed in current bulk op
         """
-        indexes = self.db_connection.find_matching_course_indexes(branch, search_targets, org_target)
+        indexes = self.db_connection.find_matching_course_indexes(
+            branch,
+            search_targets,
+            org_target,
+            course_keys=course_keys)
+
+        indexes = self._add_indexes_from_active_records(
+            indexes,
+            branch,
+            search_targets,
+            org_target,
+            course_keys=course_keys
+        )
 
+        return indexes
+
+    def _add_indexes_from_active_records(
+            self,
+            course_indexes,
+            branch=None,
+            search_targets=None,
+            org_target=None,
+            course_keys=None
+    ):
+        """
+
+        Add any being built but not yet persisted or in the process of being updated
+        """
         def _replace_or_append_index(altered_index):
             """
             If the index is already in indexes, replace it. Otherwise, append it.
             """
-            for index, existing in enumerate(indexes):
+            for index, existing in enumerate(course_indexes):
                 if all(existing[attr] == altered_index[attr] for attr in ['org', 'course', 'run']):
-                    indexes[index] = altered_index
+                    course_indexes[index] = altered_index
                     return
-            indexes.append(altered_index)
+            course_indexes.append(altered_index)
 
-        # add any being built but not yet persisted or in the process of being updated
         for _, record in self._active_records:
             if branch and branch not in record.index.get('versions', {}):
                 continue
@@ -531,7 +556,6 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                     for field, value in search_targets.iteritems()
                 ):
                     continue
-
             # if we've specified a filter by org,
             # make sure we've honored that filter when
             # integrating in-transit records
@@ -539,12 +563,22 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 if record.index['org'] != org_target:
                     continue
 
-            if not hasattr(indexes, 'append'):  # Just in time conversion to list from cursor
-                indexes = list(indexes)
+            if course_keys:
+                index_exists_in_active_records = False
+                for course_key in course_keys:
+                    if all(record.index[key_attr] == getattr(course_key, key_attr)
+                           for key_attr in ['org', 'course', 'run']):
+                        index_exists_in_active_records = True
+                        break
+                if not index_exists_in_active_records:
+                    continue
+
+            if not hasattr(course_indexes, 'append'):  # Just in time conversion to list from cursor
+                course_indexes = list(course_indexes)
 
             _replace_or_append_index(record.index)
 
-        return indexes
+        return course_indexes
 
     def find_course_blocks_by_id(self, ids):
         """
@@ -905,15 +939,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
     def collect_ids_from_matching_indexes(self, branch, **kwargs):
         """
-        Find the course_indexes which have the specified branch. if `kwargs` contains `org`
-        to apply an ORG filter to return only the courses that are part of that ORG. Extract `version_guids`
+        Find the course_indexes which have the specified branch. Extract `version_guids`
         from the course_indexes.
 
         """
         matching_indexes = self.find_matching_course_indexes(
             branch,
             search_targets=None,
-            org_target=kwargs.get('org')
+            org_target=kwargs.get('org'),
+            course_keys=kwargs.get('course_keys')
         )
 
         # collect ids and then query for those

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py

@@ -302,7 +302,13 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin):
         org_targets = None
         self.conn.find_matching_course_indexes.return_value = [Mock(name='result')]
         result = self.bulk.find_matching_course_indexes(branch, search_targets)
-        self.assertConnCalls(call.find_matching_course_indexes(branch, search_targets, org_targets))
+        self.assertConnCalls(call.find_matching_course_indexes(
+            branch,
+            search_targets,
+            org_targets,
+            course_keys=None
+        )
+        )
         self.assertEqual(result, self.conn.find_matching_course_indexes.return_value)
         self.assertCacheNotCleared()