diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 1741f8285c..7829f2a7eb 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 0509404a53..a3a14775d4 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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' diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 708fd6be7d..160e2f8ce6 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index e1dc791ae7..c215a7a490 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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'] diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index b55d0f971c..e9876dd1db 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 49b4c55a6b..5906327203 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index 0e7ee14b7b..3262a060c8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/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() diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index 6e723c2ee6..fabc73a10f 100644 --- a/lms/djangoapps/branding/__init__.py +++ b/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: