Merge pull request #7452 from edx/cdodge/get-courses-filter

do the ORG filtering as part of the mongo query. Also, don't trigger met...

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -829,7 +829,9 @@ class ModuleStoreRead(ModuleStoreAssetBase):
     def get_courses(self, **kwargs):
         '''
         Returns a list containing the top level XModuleDescriptors of the courses
-        in this modulestore.
+        in this modulestore. This method can take an optional argument 'org' which
+        will efficiently apply a filter so that only the courses of the specified
+        ORG in the CourseKey will be fetched.
         '''
         pass
 

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

@@ -74,6 +74,8 @@ BLOCK_TYPES_WITH_CHILDREN = list(set(
 # at module level, cache one instance of OSFS per filesystem root.
 _OSFS_INSTANCE = {}
 
+_DETACHED_CATEGORIES = [name for name, __ in XBlock.load_tagged_classes("detached")]
+
 
 class MongoRevisionKey(object):
     """
@@ -933,17 +935,36 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 course_key,
                 item,
                 data_cache,
-                apply_cached_metadata=(item['location']['category'] != 'course' or depth != 0),
-                using_descriptor_system=using_descriptor_system
+                using_descriptor_system=using_descriptor_system,
+                apply_cached_metadata=self._should_apply_cached_metadata(item, depth)
             )
             for item in items
         ]
 
+    def _should_apply_cached_metadata(self, item, depth):
+        """
+        Returns a boolean whether a particular query should trigger an application
+        of inherited metadata onto the item
+        """
+        category = item['location']['category']
+        apply_cached_metadata = category not in _DETACHED_CATEGORIES and \
+            not (category == 'course' and depth == 0)
+        return apply_cached_metadata
+
     @autoretry_read()
     def get_courses(self, **kwargs):
         '''
-        Returns a list of course descriptors.
+        Returns a list of course descriptors. This accepts an optional parameter of 'org' which
+        will apply an efficient filter to only get courses with the specified ORG
         '''
+
+        course_org_filter = kwargs.get('org')
+
+        if course_org_filter:
+            course_records = self.collection.find({'_id.category': 'course', '_id.org': course_org_filter})
+        else:
+            course_records = self.collection.find({'_id.category': 'course'})
+
         base_list = sum(
             [
                 self._load_items(
@@ -953,7 +974,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 for course
                 # I tried to add '$and': [{'_id.org': {'$ne': 'edx'}}, {'_id.course': {'$ne': 'templates'}}]
                 # but it didn't do the right thing (it filtered all edx and all templates out)
-                in self.collection.find({'_id.category': 'course'})
+                in course_records
                 if not (  # TODO kill this
                     course['_id']['org'] == 'edx' and
                     course['_id']['course'] == 'templates'

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

@@ -191,7 +191,7 @@ class MongoConnection(object):
             }
         return self.course_index.find_one(query)
 
-    def find_matching_course_indexes(self, branch=None, search_targets=None):
+    def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None):
         """
         Find the course_index matching particular conditions.
 
@@ -199,6 +199,8 @@ class MongoConnection(object):
             branch: If specified, this branch must exist in the returned courses
             search_targets: If specified, this must be a dictionary specifying field values
                 that must exist in the search_targets of the returned courses
+            org_target: If specified, this is an ORG filter so that only course_indexs are
+                returned for the specified ORG
         """
         query = {}
         if branch is not None:
@@ -208,6 +210,9 @@ class MongoConnection(object):
             for key, value in search_targets.iteritems():
                 query['search_targets.{}'.format(key)] = value
 
+        if org_target:
+            query['org'] = org_target
+
         return self.course_index.find(query)
 
     def insert_course_index(self, course_index):

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

@@ -486,14 +486,16 @@ 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):
+    def find_matching_course_indexes(self, branch=None, search_targets=None, org_target=None):
         """
-        Find the course_indexes which have the specified branch and search_targets.
+        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
+        that ORG.
 
         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)
+        indexes = self.db_connection.find_matching_course_indexes(branch, search_targets, org_target)
 
         def _replace_or_append_index(altered_index):
             """
@@ -519,6 +521,13 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 ):
                     continue
 
+            # if we've specified a filter by org,
+            # make sure we've honored that filter when
+            # integrating in-transit records
+            if org_target:
+                if record.index['org'] != org_target:
+                    continue
+
             if not hasattr(indexes, 'append'):  # Just in time conversion to list from cursor
                 indexes = list(indexes)
 
@@ -830,11 +839,20 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         # add it in the envelope for the structure.
         return CourseEnvelope(course_key.replace(version_guid=version_guid), entry)
 
-    def _get_structures_for_branch(self, branch):
+    def _get_structures_for_branch(self, branch, **kwargs):
         """
         Internal generator for fetching lists of courses, libraries, etc.
         """
-        matching_indexes = self.find_matching_course_indexes(branch)
+
+        # if we pass in a 'org' parameter that means to
+        # only get the course which match the passed in
+        # ORG
+
+        matching_indexes = self.find_matching_course_indexes(
+            branch,
+            search_targets=None,
+            org_target=kwargs.get('org')
+        )
 
         # collect ids and then query for those
         version_guids = []
@@ -858,7 +876,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         :param type locator_factory: Factory to create locator from structure info and branch
         """
         result = []
-        for entry, structure_info in self._get_structures_for_branch(branch):
+        for entry, structure_info in self._get_structures_for_branch(branch, **kwargs):
             locator = locator_factory(structure_info, branch)
             envelope = CourseEnvelope(locator, entry)
             root = entry['root']

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

@@ -4,6 +4,7 @@ Unit tests for the Mongo modulestore
 # pylint: disable=no-member
 # pylint: disable=protected-access
 # pylint: disable=no-name-in-module
+# pylint: disable=bad-continuation
 from nose.tools import assert_equals, assert_raises, \
     assert_not_equals, assert_false, assert_true, assert_greater, assert_is_instance, assert_is_none
 # pylint: enable=E0611
@@ -145,6 +146,18 @@ class TestMongoModuleStoreBase(unittest.TestCase):
             verbose=True
         )
 
+        # also import a course under a different course_id (especially ORG)
+        import_course_from_xml(
+            draft_store,
+            999,
+            DATA_DIR,
+            ['test_import_course'],
+            static_content_store=content_store,
+            do_import_static=False,
+            verbose=True,
+            target_id=SlashSeparatedCourseKey('guestx', 'foo', 'bar')
+        )
+
         return content_store, draft_store
 
     @staticmethod
@@ -191,15 +204,29 @@ class TestMongoModuleStore(TestMongoModuleStoreBase):
     def test_get_courses(self):
         '''Make sure the course objects loaded properly'''
         courses = self.draft_store.get_courses()
-        assert_equals(len(courses), 6)
+
+        # note, the number of courses expected is really
+        # 6, but due to a lack of cache flushing between
+        # test case runs, we will get back 7.
+        # When we fix the caching issue, we should reduce this
+        # to 6 and remove the 'treexport_peer_component' course_id
+        # from the list below
+        assert_equals(len(courses), 7)  # pylint: disable=no-value-for-parameter
         course_ids = [course.id for course in courses]
+
         for course_key in [
 
             SlashSeparatedCourseKey(*fields)
             for fields in [
-                ['edX', 'simple', '2012_Fall'], ['edX', 'simple_with_draft', '2012_Fall'],
-                ['edX', 'test_import_course', '2012_Fall'], ['edX', 'test_unicode', '2012_Fall'],
-                ['edX', 'toy', '2012_Fall']
+                ['edX', 'simple', '2012_Fall'],
+                ['edX', 'simple_with_draft', '2012_Fall'],
+                ['edX', 'test_import_course', '2012_Fall'],
+                ['edX', 'test_unicode', '2012_Fall'],
+                ['edX', 'toy', '2012_Fall'],
+                ['guestx', 'foo', 'bar'],
+                # This course below is due to a caching issue in the modulestore
+                # which is not cleared between test runs. This means
+                ['edX', 'treeexport_peer_component', 'export_peer_component'],
             ]
         ]:
             assert_in(course_key, course_ids)
@@ -212,6 +239,48 @@ class TestMongoModuleStore(TestMongoModuleStoreBase):
             assert_false(self.draft_store.has_course(mix_cased))
             assert_true(self.draft_store.has_course(mix_cased, ignore_case=True))
 
+    def test_get_org_courses(self):
+        """
+        Make sure that we can query for a filtered list of courses for a given ORG
+        """
+
+        courses = self.draft_store.get_courses(org='guestx')
+        assert_equals(len(courses), 1)  # pylint: disable=no-value-for-parameter
+        course_ids = [course.id for course in courses]
+
+        for course_key in [
+            SlashSeparatedCourseKey(*fields)
+            for fields in [
+                ['guestx', 'foo', 'bar']
+            ]
+        ]:
+            assert_in(course_key, course_ids)  # pylint: disable=no-value-for-parameter
+
+        courses = self.draft_store.get_courses(org='edX')
+        # note, the number of courses expected is really
+        # 5, but due to a lack of cache flushing between
+        # test case runs, we will get back 6.
+        # When we fix the caching issue, we should reduce this
+        # to 6 and remove the 'treexport_peer_component' course_id
+        # from the list below
+        assert_equals(len(courses), 6)  # pylint: disable=no-value-for-parameter
+        course_ids = [course.id for course in courses]
+
+        for course_key in [
+            SlashSeparatedCourseKey(*fields)
+            for fields in [
+                ['edX', 'simple', '2012_Fall'],
+                ['edX', 'simple_with_draft', '2012_Fall'],
+                ['edX', 'test_import_course', '2012_Fall'],
+                ['edX', 'test_unicode', '2012_Fall'],
+                ['edX', 'toy', '2012_Fall'],
+                # This course below is due to a caching issue in the modulestore
+                # which is not cleared between test runs. This means
+                ['edX', 'treeexport_peer_component', 'export_peer_component'],
+            ]
+        ]:
+            assert_in(course_key, course_ids)  # pylint: disable=no-value-for-parameter
+
     def test_no_such_course(self):
         """
         Test get_course and has_course with ids which don't exist

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

@@ -617,6 +617,23 @@ class SplitModuleCourseTests(SplitModuleTest):
         self.assertEqual(course.edited_by, "testassist@edx.org")
         self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.45})
 
+    def test_get_org_courses(self):
+        courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='guestx')
+
+        # should have gotten 1 draft courses
+        self.assertEqual(len(courses), 1)
+
+        courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='testx')
+
+        # should have gotten 2 draft courses
+        self.assertEqual(len(courses), 2)
+
+        # although this is already covered in other tests, let's
+        # also not pass in org= parameter to make sure we get back
+        # 3 courses
+        courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT)
+        self.assertEqual(len(courses), 3)
+
     def test_branch_requests(self):
         # query w/ branch qualifier (both draft and published)
         def _verify_published_course(courses_published):
@@ -1232,6 +1249,37 @@ class TestItemCrud(SplitModuleTest):
             self.assertEqual(refetch_course.previous_version, course_block_update_version)
             self.assertEqual(refetch_course.update_version, transaction_guid)
 
+    def test_bulk_ops_org_filtering(self):
+        """
+        Make sure of proper filtering when using bulk operations and
+        calling get_courses with an 'org' filter
+        """
+
+        # start transaction w/ simple creation
+        user = random.getrandbits(32)
+        course_key = CourseLocator('test_org', 'test_transaction', 'test_run')
+        with modulestore().bulk_operations(course_key):
+            modulestore().create_course('test_org', 'test_transaction', 'test_run', user, BRANCH_NAME_DRAFT)
+
+            courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='test_org')
+            self.assertEqual(len(courses), 1)
+            self.assertEqual(courses[0].id.org, course_key.org)
+            self.assertEqual(courses[0].id.course, course_key.course)
+            self.assertEqual(courses[0].id.run, course_key.run)
+
+            courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='other_org')
+            self.assertEqual(len(courses), 0)
+
+        # re-assert after the end of the with scope
+        courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='test_org')
+        self.assertEqual(len(courses), 1)
+        self.assertEqual(courses[0].id.org, course_key.org)
+        self.assertEqual(courses[0].id.course, course_key.course)
+        self.assertEqual(courses[0].id.run, course_key.run)
+
+        courses = modulestore().get_courses(branch=BRANCH_NAME_DRAFT, org='other_org')
+        self.assertEqual(len(courses), 0)
+
     def test_update_metadata(self):
         """
         test updating an items metadata ensuring the definition doesn't version but the course does if it should

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

@@ -274,9 +274,10 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin):
     def test_no_bulk_find_matching_course_indexes(self):
         branch = Mock(name='branch')
         search_targets = MagicMock(name='search_targets')
+        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))
+        self.assertConnCalls(call.find_matching_course_indexes(branch, search_targets, org_targets))
         self.assertEqual(result, self.conn.find_matching_course_indexes.return_value)
         self.assertCacheNotCleared()
 

lms/djangoapps/branding/__init__.py

@@ -10,7 +10,10 @@ def get_visible_courses():
     """
     Return the set of CourseDescriptors that should be visible in this branded instance
     """
-    _courses = modulestore().get_courses()
+
+    filtered_by_org = microsite.get_value('course_org_filter')
+
+    _courses = modulestore().get_courses(org=filtered_by_org)
 
     courses = [c for c in _courses
                if isinstance(c, CourseDescriptor)]
@@ -25,8 +28,6 @@ def get_visible_courses():
     if hasattr(settings, 'COURSE_LISTINGS') and subdomain in settings.COURSE_LISTINGS and not settings.DEBUG:
         filtered_visible_ids = frozenset([SlashSeparatedCourseKey.from_deprecated_string(c) for c in settings.COURSE_LISTINGS[subdomain]])
 
-    filtered_by_org = microsite.get_value('course_org_filter')
-
     if filtered_by_org:
         return [course for course in courses if course.location.org == filtered_by_org]
     if filtered_visible_ids: