diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py index ebe94f4a79..39ab626638 100644 --- a/cms/djangoapps/contentstore/features/video.py +++ b/cms/djangoapps/contentstore/features/video.py @@ -138,7 +138,7 @@ def xml_only_video(step): course = world.scenario_dict['COURSE'] store = modulestore() - parent_location = store.get_items(course.id, category='vertical')[0].location + parent_location = store.get_items(course.id, qualifiers={'category': 'vertical'})[0].location youtube_id = 'ABCDEFG' world.scenario_dict['YOUTUBE_ID'] = youtube_id diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py index 45bbcad78a..874a2b9a4c 100644 --- a/cms/djangoapps/contentstore/management/commands/check_course.py +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -58,7 +58,7 @@ class Command(BaseCommand): discussion_items = _get_discussion_items(course) # now query all discussion items via get_items() and compare with the tree-traversal - queried_discussion_items = store.get_items(course_key=course_key, category='discussion',) + queried_discussion_items = store.get_items(course_key=course_key, qualifiers={'category': 'discussion'}) for item in queried_discussion_items: if item.location not in discussion_items: diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 659bd9eca5..7fea7df3e2 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -28,7 +28,7 @@ from xmodule.exceptions import NotFoundError, InvalidVersionError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation, CourseLocator from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.xml_exporter import export_to_xml @@ -47,6 +47,7 @@ from student.roles import CourseCreatorRole, CourseInstructorRole from opaque_keys import InvalidKeyError from contentstore.tests.utils import get_url from course_action_state.models import CourseRerunState, CourseRerunUIStateManager +from course_action_state.managers import CourseActionStateItemNotFoundError TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -94,7 +95,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): store.update_item(course, self.user.id) # just pick one vertical - descriptor = store.get_items(course.id, category='vertical',) + descriptor = store.get_items(course.id, qualifiers={'category': 'vertical'}) resp = self.client.get_html(get_url('container_handler', descriptor[0].location)) self.assertEqual(resp.status_code, 200) @@ -127,7 +128,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): """Verifies the editing HTML in all the verticals in the given test course""" _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', [test_course_name]) - items = self.store.get_items(course_items[0].id, category='vertical') + items = self.store.get_items(course_items[0].id, qualifiers={'category': 'vertical'}) self._check_verticals(items) def test_edit_unit_toy(self): @@ -289,7 +290,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_key = course_items[0].id - items = self.store.get_items(course_key, category='poll_question') + items = self.store.get_items(course_key, qualifiers={'category': 'poll_question'}) found = len(items) > 0 self.assertTrue(found) @@ -643,7 +644,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): filesystem = OSFS(root_dir / 'test_export') self.assertTrue(filesystem.exists(dirname)) - items = store.get_items(course_id, category=category_name) + items = store.get_items(course_id, qualifiers={'category': category_name}) for item in items: filesystem = OSFS(root_dir / ('test_export/' + dirname)) @@ -742,7 +743,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): # create a new video module and add it as a child to a vertical # this re-creates a bug whereby since the video template doesn't have # anything in 'data' field, the export was blowing up - verticals = self.store.get_items(course_id, category='vertical') + verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) self.assertGreater(len(verticals), 0) @@ -768,7 +769,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): import_from_xml(self.store, self.user.id, 'common/test/data/', ['word_cloud']) course_id = SlashSeparatedCourseKey('HarvardX', 'ER22x', '2013_Spring') - verticals = self.store.get_items(course_id, category='vertical') + verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) self.assertGreater(len(verticals), 0) @@ -795,7 +796,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') - verticals = self.store.get_items(course_id, category='vertical') + verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) self.assertGreater(len(verticals), 0) @@ -916,8 +917,10 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): items = self.store.get_items( course_id, - category='sequential', - name='vertical_sequential' + qualifiers={ + 'category': 'sequential', + 'name': 'vertical_sequential', + } ) self.assertEqual(len(items), 1) @@ -1115,8 +1118,7 @@ class ContentStoreTest(ContentStoreTestCase): def test_create_course_with_bad_organization(self): """Test new course creation - error path for bad organization name""" self.course_data['org'] = 'University of California, Berkeley' - self.assert_course_creation_failed( - r"(?s)Unable to create course 'Robot Super Course'.*: Invalid characters in u'University of California, Berkeley'") + self.assert_course_creation_failed(r"(?s)Unable to create course 'Robot Super Course'.*") def test_create_course_with_course_creation_disabled_staff(self): """Test new course creation -- course creation disabled, but staff access.""" @@ -1401,7 +1403,7 @@ class ContentStoreTest(ContentStoreTestCase): _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course = course_items[0] - verticals = self.store.get_items(course.id, category='vertical') + verticals = self.store.get_items(course.id, qualifiers={'category': 'vertical'}) # let's assert on the metadata_inheritance on an existing vertical for vertical in verticals: @@ -1572,31 +1574,35 @@ class RerunCourseTest(ContentStoreTestCase): 'display_name': 'Robot Super Course', 'run': '2013_Spring' } - self.destination_course_key = _get_course_id(self.destination_course_data) - def post_rerun_request(self, source_course_key, response_code=200): + def post_rerun_request( + self, source_course_key, destination_course_data=None, response_code=200, expect_error=False + ): """Create and send an ajax post for the rerun request""" # create data to post rerun_course_data = {'source_course_key': unicode(source_course_key)} - rerun_course_data.update(self.destination_course_data) + if not destination_course_data: + destination_course_data = self.destination_course_data + rerun_course_data.update(destination_course_data) + destination_course_key = _get_course_id(destination_course_data) # post the request - course_url = get_url('course_handler', self.destination_course_key, 'course_key_string') + course_url = get_url('course_handler', destination_course_key, 'course_key_string') response = self.client.ajax_post(course_url, rerun_course_data) # verify response self.assertEqual(response.status_code, response_code) - if response_code == 200: - self.assertNotIn('ErrMsg', parse_json(response)) + if not expect_error: + json_resp = parse_json(response) + self.assertNotIn('ErrMsg', json_resp) + destination_course_key = CourseKey.from_string(json_resp['destination_course_key']) - def create_course_listing_html(self, course_key): - """Creates html fragment that is created for the given course_key in the course listing section""" - return ' split + split_store = copy.deepcopy(mongo_store) + # update the ENGINE and NAME fields + split_store['ENGINE'] = 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore' + split_store['NAME'] = 'split' + # add split to the end of the list + mixed_stores.append(split_store) + return module_store_setting diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index aad4c29a00..b508056099 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -37,10 +37,11 @@ from xblock.fields import Scope, ScopeIds, Reference, ReferenceList, ReferenceVa from xmodule.modulestore import ModuleStoreWriteBase, ModuleStoreEnum from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES from opaque_keys.edx.locations import Location -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, ReferentialIntegrityError +from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore from xblock.core import XBlock from opaque_keys.edx.locations import SlashSeparatedCourseKey +from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.keys import UsageKey, CourseKey from xmodule.exceptions import HeartbeatFailure @@ -354,8 +355,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): """ A Mongodb backed ModuleStore """ - reference_type = SlashSeparatedCourseKey - # TODO (cpennington): Enable non-filesystem filestores # pylint: disable=C0103 # pylint: disable=W0201 @@ -716,7 +715,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): for item in items ] - def get_courses(self): + def get_courses(self, **kwargs): ''' Returns a list of course descriptors. ''' @@ -751,7 +750,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): raise ItemNotFoundError(location) return item - def get_course(self, course_key, depth=0): + def make_course_key(self, org, course, run): + """ + Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore + that matches the supplied `org`, `course`, and `run`. + + This key may represent a course that doesn't exist in this modulestore. + """ + return CourseLocator(org, course, run, deprecated=True) + + def get_course(self, course_key, depth=0, **kwargs): """ Get the course with the given courseid (org/course/run) """ @@ -763,7 +771,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): except ItemNotFoundError: return None - def has_course(self, course_key, ignore_case=False): + def has_course(self, course_key, ignore_case=False, **kwargs): """ Returns the course_id of the course if it was found, else None Note: we return the course_id instead of a boolean here since the found course may have @@ -838,7 +846,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): for key in ('tag', 'org', 'course', 'category', 'name', 'revision') ]) - def get_items(self, course_id, settings=None, content=None, key_revision=MongoRevisionKey.published, **kwargs): + def get_items( + self, + course_id, + settings=None, + content=None, + key_revision=MongoRevisionKey.published, + qualifiers=None, + **kwargs + ): """ Returns: list of XModuleDescriptor instances for the matching items within the course with @@ -853,15 +869,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): Args: course_id (CourseKey): the course identifier settings (dict): fields to look for which have settings scope. Follows same syntax - and rules as kwargs below + and rules as qualifiers below content (dict): fields to look for which have content scope. Follows same syntax and - rules as kwargs below. + rules as qualifiers below. key_revision (str): the revision of the items you're looking for. MongoRevisionKey.draft - only returns drafts MongoRevisionKey.published (equates to None) - only returns published If you want one of each matching xblock but preferring draft to published, call this same method on the draft modulestore with ModuleStoreEnum.RevisionOption.draft_preferred. - kwargs (key=value): what to look for within the course. + qualifiers (dict): what to look for within the course. Common qualifiers are ``category`` or any field name. if the target field is a list, then it searches for the given value in the list not list equivalence. Substring matching pass a regex object. @@ -869,20 +885,21 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): This modulestore does not allow searching dates by comparison or edited_by, previous_version, update_version info. """ + qualifiers = qualifiers.copy() if qualifiers else {} # copy the qualifiers (destructively manipulated here) query = self._course_key_to_son(course_id) query['_id.revision'] = key_revision for field in ['category', 'name']: - if field in kwargs: - query['_id.' + field] = kwargs.pop(field) + if field in qualifiers: + query['_id.' + field] = qualifiers.pop(field) for key, value in (settings or {}).iteritems(): query['metadata.' + key] = value for key, value in (content or {}).iteritems(): query['definition.data.' + key] = value - if 'children' in kwargs: - query['definition.children'] = kwargs.pop('children') + if 'children' in qualifiers: + query['definition.children'] = qualifiers.pop('children') - query.update(kwargs) + query.update(qualifiers) items = self.collection.find( query, sort=[SORT_REVISION_FAVOR_DRAFT], @@ -919,10 +936,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): ]) courses = self.collection.find(course_search_location, fields=('_id')) if courses.count() > 0: - raise InvalidLocationError( - "There are already courses with the given org and course id: {}".format([ - course['_id'] for course in courses - ])) + raise DuplicateCourseError(course_id, courses[0]['_id']) location = course_id.make_usage_key('course', course_id.run) course = self.create_xmodule( @@ -1253,7 +1267,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): """ return ModuleStoreEnum.Type.mongo - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Return an array of all of the locations for orphans in the course. """ @@ -1274,7 +1288,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): item_locs -= all_reachable return [course_key.make_usage_key_from_deprecated_string(item_loc) for item_loc in item_locs] - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index d853516c05..bafb2cbd07 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -47,7 +47,7 @@ class DraftModuleStore(MongoModuleStore): This module also includes functionality to promote DRAFT modules (and their children) to published modules. """ - def get_item(self, usage_key, depth=0, revision=None): + def get_item(self, usage_key, depth=0, revision=None, **kwargs): """ Returns an XModuleDescriptor instance for the item at usage_key. @@ -155,7 +155,7 @@ class DraftModuleStore(MongoModuleStore): course_query = self._course_key_to_son(course_key) self.collection.remove(course_query, multi=True) - def clone_course(self, source_course_id, dest_course_id, user_id, fields=None): + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): """ Only called if cloning within this store or if env doesn't set up mixed. * copy the courseware @@ -331,11 +331,6 @@ class DraftModuleStore(MongoModuleStore): returns only Published items if the branch setting is ModuleStoreEnum.Branch.draft_preferred, returns either Draft or Published, preferring Draft items. - kwargs (key=value): what to look for within the course. - Common qualifiers are ``category`` or any field name. if the target field is a list, - then it searches for the given value in the list not list equivalence. - Substring matching pass a regex object. - ``name`` is another commonly provided key (Location based stores) """ def base_get_items(key_revision): return super(DraftModuleStore, self).get_items(course_key, key_revision=key_revision, **kwargs) @@ -439,7 +434,7 @@ class DraftModuleStore(MongoModuleStore): # convert the subtree using the original item as the root self._breadth_first(convert_item, [location]) - def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False): + def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False, **kwargs): """ See superclass doc. In addition to the superclass's behavior, this method converts the unit to draft if it's not @@ -616,7 +611,7 @@ class DraftModuleStore(MongoModuleStore): else: return False - def publish(self, location, user_id): + def publish(self, location, user_id, **kwargs): """ Publish the subtree rooted at location to the live course and remove the drafts. Such publishing may cause the deletion of previously published but subsequently deleted @@ -690,7 +685,7 @@ class DraftModuleStore(MongoModuleStore): self.collection.remove({'_id': {'$in': to_be_deleted}}) return self.get_item(as_published(location)) - def unpublish(self, location, user_id): + def unpublish(self, location, user_id, **kwargs): """ Turn the published version into a draft, removing the published version. diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 5d5712939b..8fff1be717 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -25,7 +25,9 @@ class SplitMigrator(object): self.split_modulestore = split_modulestore self.source_modulestore = source_modulestore - def migrate_mongo_course(self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None): + def migrate_mongo_course( + self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None, **kwargs + ): """ Create a new course in split_mongo representing the published and draft versions of the course from the original mongo store. And return the new CourseLocator @@ -43,7 +45,7 @@ class SplitMigrator(object): # locations are in location, children, conditionals, course.tab # create the course: set fields to explicitly_set for each scope, id_root = new_course_locator, master_branch = 'production' - original_course = self.source_modulestore.get_course(source_course_key) + original_course = self.source_modulestore.get_course(source_course_key, **kwargs) if new_org is None: new_org = source_course_key.org @@ -60,17 +62,20 @@ class SplitMigrator(object): new_org, new_course, new_run, user_id, fields=new_fields, master_branch=ModuleStoreEnum.BranchName.published, + **kwargs ) with self.split_modulestore.bulk_write_operations(new_course.id): - self._copy_published_modules_to_course(new_course, original_course.location, source_course_key, user_id) + self._copy_published_modules_to_course( + new_course, original_course.location, source_course_key, user_id, **kwargs + ) # create a new version for the drafts with self.split_modulestore.bulk_write_operations(new_course.id): - self._add_draft_modules_to_course(new_course.location, source_course_key, user_id) + self._add_draft_modules_to_course(new_course.location, source_course_key, user_id, **kwargs) return new_course.id - def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id): + def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id, **kwargs): """ Copy all of the modules from the 'direct' version of the course to the new split course. """ @@ -79,7 +84,7 @@ class SplitMigrator(object): # iterate over published course elements. Wildcarding rather than descending b/c some elements are orphaned (e.g., # course about pages, conditionals) for module in self.source_modulestore.get_items( - source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only + source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs ): # don't copy the course again. if module.location != old_course_loc: @@ -95,7 +100,8 @@ class SplitMigrator(object): fields=self._get_fields_translate_references( module, course_version_locator, new_course.location.block_id ), - continue_version=True + continue_version=True, + **kwargs ) # after done w/ published items, add version for DRAFT pointing to the published structure index_info = self.split_modulestore.get_course_index_info(course_version_locator) @@ -107,7 +113,7 @@ class SplitMigrator(object): # children which meant some pointers were to non-existent locations in 'direct' self.split_modulestore.internal_clean_children(course_version_locator) - def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id): + def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id, **kwargs): """ update each draft. Create any which don't exist in published and attach to their parents. """ @@ -117,11 +123,13 @@ class SplitMigrator(object): # to prevent race conditions of grandchilden being added before their parents and thus having no parent to # add to awaiting_adoption = {} - for module in self.source_modulestore.get_items(source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only): + for module in self.source_modulestore.get_items( + source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only, **kwargs + ): new_locator = new_draft_course_loc.make_usage_key(module.category, module.location.block_id) if self.split_modulestore.has_item(new_locator): # was in 'direct' so draft is a new version - split_module = self.split_modulestore.get_item(new_locator) + split_module = self.split_modulestore.get_item(new_locator, **kwargs) # need to remove any no-longer-explicitly-set values and add/update any now set values. for name, field in split_module.fields.iteritems(): if field.is_set_on(split_module) and not module.fields[name].is_set_on(module): @@ -131,7 +139,7 @@ class SplitMigrator(object): ).iteritems(): field.write_to(split_module, value) - _new_module = self.split_modulestore.update_item(split_module, user_id) + _new_module = self.split_modulestore.update_item(split_module, user_id, **kwargs) else: # only a draft version (aka, 'private'). _new_module = self.split_modulestore.create_item( @@ -140,22 +148,23 @@ class SplitMigrator(object): block_id=new_locator.block_id, fields=self._get_fields_translate_references( module, new_draft_course_loc, published_course_usage_key.block_id - ) + ), + **kwargs ) awaiting_adoption[module.location] = new_locator for draft_location, new_locator in awaiting_adoption.iteritems(): parent_loc = self.source_modulestore.get_parent_location( - draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred + draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred, **kwargs ) if parent_loc is None: log.warn(u'No parent found in source course for %s', draft_location) continue - old_parent = self.source_modulestore.get_item(parent_loc) + old_parent = self.source_modulestore.get_item(parent_loc, **kwargs) split_parent_loc = new_draft_course_loc.make_usage_key( parent_loc.category, parent_loc.block_id if parent_loc.category != 'course' else published_course_usage_key.block_id ) - new_parent = self.split_modulestore.get_item(split_parent_loc) + new_parent = self.split_modulestore.get_item(split_parent_loc, **kwargs) # this only occurs if the parent was also awaiting adoption: skip this one, go to next if any(new_locator == child.version_agnostic() for child in new_parent.children): continue diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index a2733d35b0..d9b74c5bde 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -53,7 +53,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): self.default_class = default_class self.local_modules = {} - def _load_item(self, block_id, course_entry_override=None): + def _load_item(self, block_id, course_entry_override=None, **kwargs): if isinstance(block_id, BlockUsageLocator): if isinstance(block_id.block_id, LocalId): try: @@ -77,7 +77,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): raise ItemNotFoundError(block_id) class_ = self.load_block_type(json_data.get('category')) - return self.xblock_from_json(class_, block_id, json_data, course_entry_override) + return self.xblock_from_json(class_, block_id, json_data, course_entry_override, **kwargs) # xblock's runtime does not always pass enough contextual information to figure out # which named container (course x branch) or which parent is requesting an item. Because split allows @@ -90,7 +90,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # low; thus, the course_entry is most likely correct. If the thread is looking at > 1 named container # pointing to the same structure, the access is likely to be chunky enough that the last known container # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id. - def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None): + def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None, **kwargs): if course_entry_override is None: course_entry_override = self.course_entry else: @@ -126,6 +126,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): definition, converted_fields, json_data.get('_inherited_settings'), + **kwargs ) field_data = KvsFieldData(kvs) @@ -151,6 +152,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem): edit_info = json_data.get('edit_info', {}) module.edited_by = edit_info.get('edited_by') module.edited_on = edit_info.get('edited_on') + module.subtree_edited_by = None # TODO - addressed with LMS-11183 + module.subtree_edited_on = None # TODO - addressed with LMS-11183 + module.published_by = None # TODO - addressed with LMS-11184 + module.published_date = None # TODO - addressed with LMS-11184 module.previous_version = edit_info.get('previous_version') module.update_version = edit_info.get('update_version') module.source_version = edit_info.get('source_version', None) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 886b790f67..3d067fb66e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -63,7 +63,7 @@ from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict from xmodule.errortracker import null_error_tracker from opaque_keys.edx.locator import ( BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, - LocalId, Locator + LocalId, ) from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \ DuplicateCourseError @@ -77,7 +77,6 @@ from .caching_descriptor_system import CachingDescriptorSystem from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo -import types from _collections import defaultdict @@ -111,7 +110,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ SCHEMA_VERSION = 1 - reference_type = Locator # a list of field names to store in course index search_targets. Note, this will # only record one value per key. If branches disagree, the last one set wins. # It won't recompute the value on operations such as update_course_index (e.g., to revert to a prev @@ -214,7 +212,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): system.module_data.update(new_module_data) return system.module_data - def _load_items(self, course_entry, block_ids, depth=0, lazy=True): + def _load_items(self, course_entry, block_ids, depth=0, lazy=True, **kwargs): ''' Load & cache the given blocks from the course. Prefetch down to the given depth. Load the definitions into each block if lazy is False; @@ -248,7 +246,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): branch=course_entry.get('branch'), ) self.cache_items(system, block_ids, course_key, depth, lazy) - return [system.load_item(block_id, course_entry) for block_id in block_ids] + return [system.load_item(block_id, course_entry, **kwargs) for block_id in block_ids] def _get_cache(self, course_version_guid): """ @@ -333,7 +331,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } return envelope - def get_courses(self, branch, qualifiers=None): + def get_courses(self, branch, qualifiers=None, **kwargs): ''' Returns a list of course descriptors matching any given qualifiers. @@ -373,12 +371,21 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'structure': entry, } root = entry['root'] - course_list = self._load_items(envelope, [root], 0, lazy=True) + course_list = self._load_items(envelope, [root], 0, lazy=True, **kwargs) if not isinstance(course_list[0], ErrorDescriptor): result.append(course_list[0]) return result - def get_course(self, course_id, depth=0): + def make_course_key(self, org, course, run): + """ + Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore + that matches the supplied `org`, `course`, and `run`. + + This key may represent a course that doesn't exist in this modulestore. + """ + return CourseLocator(org, course, run) + + def get_course(self, course_id, depth=0, **kwargs): ''' Gets the course descriptor for the course identified by the locator ''' @@ -388,10 +395,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course_entry = self._lookup_course(course_id) root = course_entry['structure']['root'] - result = self._load_items(course_entry, [root], 0, lazy=True) + result = self._load_items(course_entry, [root], 0, lazy=True, **kwargs) return result[0] - def has_course(self, course_id, ignore_case=False): + def has_course(self, course_id, ignore_case=False, **kwargs): ''' Does this course exist in this modulestore. This method does not verify that the branch &/or version in the course_id exists. Use get_course_index_info to check that. @@ -423,7 +430,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return self._get_block_from_structure(course_structure, usage_key.block_id) is not None - def get_item(self, usage_key, depth=0): + def get_item(self, usage_key, depth=0, **kwargs): """ depth (int): An argument that some module stores may use to prefetch descendants of the queried modules for more efficient results later @@ -437,14 +444,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): raise ItemNotFoundError(usage_key) course = self._lookup_course(usage_key) - items = self._load_items(course, [usage_key.block_id], depth, lazy=True) + items = self._load_items(course, [usage_key.block_id], depth, lazy=True, **kwargs) if len(items) == 0: raise ItemNotFoundError(usage_key) elif len(items) > 1: log.debug("Found more than one item for '{}'".format(usage_key)) return items[0] - def get_items(self, course_locator, settings=None, content=None, **kwargs): + def get_items(self, course_locator, settings=None, content=None, qualifiers=None, **kwargs): """ Returns: list of XModuleDescriptor instances for the matching items within the course with @@ -455,10 +462,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Args: course_locator (CourseLocator): the course identifier settings (dict): fields to look for which have settings scope. Follows same syntax - and rules as kwargs below + and rules as qualifiers below content (dict): fields to look for which have content scope. Follows same syntax and - rules as kwargs below. - kwargs (key=value): what to look for within the course. + rules as qualifiers below. + qualifiers (dict): what to look for within the course. Common qualifiers are ``category`` or any field name. if the target field is a list, then it searches for the given value in the list not list equivalence. For substring matching pass a regex object. @@ -467,6 +474,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ course = self._lookup_course(course_locator) items = [] + qualifiers = qualifiers.copy() if qualifiers else {} # copy the qualifiers (destructively manipulated here) def _block_matches_all(block_json): """ @@ -474,7 +482,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ # do the checks which don't require loading any additional data if ( - self._block_matches(block_json, kwargs) and + self._block_matches(block_json, qualifiers) and self._block_matches(block_json.get('fields', {}), settings) ): if content: @@ -485,23 +493,23 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if settings is None: settings = {} - if 'name' in kwargs: + if 'name' in qualifiers: # odd case where we don't search just confirm - block_id = kwargs.pop('name') + block_id = qualifiers.pop('name') block = course['structure']['blocks'].get(block_id) if _block_matches_all(block): - return self._load_items(course, [block_id], lazy=True) + return self._load_items(course, [block_id], lazy=True, **kwargs) else: return [] # don't expect caller to know that children are in fields - if 'children' in kwargs: - settings['children'] = kwargs.pop('children') + if 'children' in qualifiers: + settings['children'] = qualifiers.pop('children') for block_id, value in course['structure']['blocks'].iteritems(): if _block_matches_all(value): items.append(block_id) if len(items) > 0: - return self._load_items(course, items, 0, lazy=True) + return self._load_items(course, items, 0, lazy=True, **kwargs) else: return [] @@ -523,7 +531,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block_id=decode_key_from_mongo(parent_id), ) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Return an array of all of the orphans in the course. """ @@ -820,6 +828,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): the course id'd by version_guid but instead in one w/ a new version_guid. Ensure in this case that you get the new version_guid from the locator in the returned object! """ + # split handles all the fields in one dict not separated by scope + fields = fields or {} + fields.update(kwargs.pop('metadata', {}) or {}) + fields.update(kwargs.pop('definition_data', {}) or {}) + # find course_index entry if applicable and structures entry index_entry = self._get_index_if_valid(course_key, force, continue_version) structure = self._lookup_course(course_key)['structure'] @@ -940,18 +953,20 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # don't need to update the index b/c create_item did it for this version return xblock - def clone_course(self, source_course_id, dest_course_id, user_id, fields=None): + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): """ See :meth: `.ModuleStoreWrite.clone_course` for documentation. In split, other than copying the assets, this is cheap as it merely creates a new version of the existing course. """ - super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields) + super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs) source_index = self.get_course_index_info(source_course_id) + if source_index is None: + raise ItemNotFoundError("Cannot find a course at {0}. Aborting".format(source_course_id)) return self.create_course( dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=fields, - versions_dict=source_index['versions'], search_targets=source_index['search_targets'] + versions_dict=source_index['versions'], search_targets=source_index['search_targets'], **kwargs ) def create_course( @@ -1087,10 +1102,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self._update_search_targets(index_entry, fields) self.db_connection.insert_course_index(index_entry) # expensive hack to persist default field values set in __init__ method (e.g., wiki_slug) - course = self.get_course(locator) - return self.update_item(course, user_id) + course = self.get_course(locator, **kwargs) + return self.update_item(course, user_id, **kwargs) - def update_item(self, descriptor, user_id, allow_not_found=False, force=False): + def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): """ Save the descriptor's fields. it doesn't descend the course dag to save the children. Return the new descriptor (updated location). @@ -1161,12 +1176,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # fetch and return the new item--fetching is unnecessary but a good qc step new_locator = descriptor.location.map_into_course(course_key) - return self.get_item(new_locator) + return self.get_item(new_locator, **kwargs) else: # nothing changed, just return the one sent in return descriptor - def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None): + def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None, **kwargs): """ This method instantiates the correct subclass of XModuleDescriptor based on the contents of json_data. It does not persist it and can create one which @@ -1193,7 +1208,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if field_name in fields: json_data['_inherited_settings'][field_name] = fields[field_name] - new_block = runtime.xblock_from_json(xblock_class, block_id, json_data) + new_block = runtime.xblock_from_json(xblock_class, block_id, json_data, **kwargs) if parent_xblock is not None: parent_xblock.children.append(new_block.scope_ids.usage_id) # decache pending children field settings @@ -1844,6 +1859,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): destination_block['edit_info']['previous_version'] = previous_version destination_block['edit_info']['update_version'] = destination_version destination_block['edit_info']['edited_by'] = user_id + destination_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) else: destination_block = self._new_block( user_id, new_block['category'], @@ -1939,7 +1955,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): for entry in entries ] - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 85344f6478..1759c547c1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -48,16 +48,22 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS item = super(DraftVersioningModuleStore, self).create_course( org, course, run, user_id, master_branch=master_branch, **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item - def get_courses(self): + def get_courses(self, **kwargs): """ - Returns all the courses on the Draft branch (which is a superset of the courses on the Published branch). + Returns all the courses on the Draft or Published branch depending on the branch setting. """ - return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft) + branch_setting = self.get_branch_setting() + if branch_setting == ModuleStoreEnum.Branch.draft_preferred: + return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft, **kwargs) + elif branch_setting == ModuleStoreEnum.Branch.published_only: + return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.published, **kwargs) + else: + raise InsufficientSpecificationError() - def _auto_publish_no_children(self, location, category, user_id): + def _auto_publish_no_children(self, location, category, user_id, **kwargs): """ Publishes item if the category is DIRECT_ONLY. This assumes another method has checked that location points to the head of the branch and ignores the version. If you call this in any @@ -66,16 +72,17 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ if location.branch == ModuleStoreEnum.BranchName.draft and category in DIRECT_ONLY_CATEGORIES: # version_agnostic b/c of above assumption in docstring - self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL) + self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) - def update_item(self, descriptor, user_id, allow_not_found=False, force=False): + def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): item = super(DraftVersioningModuleStore, self).update_item( descriptor, user_id, allow_not_found=allow_not_found, - force=force + force=force, + **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item def create_item( @@ -88,7 +95,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS definition_locator=definition_locator, fields=fields, force=force, continue_version=continue_version, **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item def create_child( @@ -99,7 +106,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, continue_version=continue_version, **kwargs ) - self._auto_publish_no_children(parent_usage_key, item.location.category, user_id) + self._auto_publish_no_children(parent_usage_key, item.location.category, user_id, **kwargs) return item def delete_item(self, location, user_id, revision=None, **kwargs): @@ -134,8 +141,8 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS for branch in branches_to_delete: branched_location = location.for_branch(branch) parent_loc = self.get_parent_location(branched_location) - SplitMongoModuleStore.delete_item(self, branched_location, user_id, **kwargs) - self._auto_publish_no_children(parent_loc, parent_loc.category, user_id) + SplitMongoModuleStore.delete_item(self, branched_location, user_id) + self._auto_publish_no_children(parent_loc, parent_loc.category, user_id, **kwargs) def _map_revision_to_branch(self, key, revision=None): """ @@ -157,25 +164,20 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS usage_key = self._map_revision_to_branch(usage_key, revision=revision) return super(DraftVersioningModuleStore, self).has_item(usage_key) - def get_item(self, usage_key, depth=0, revision=None): + def get_item(self, usage_key, depth=0, revision=None, **kwargs): """ Returns the item identified by usage_key and revision. """ usage_key = self._map_revision_to_branch(usage_key, revision=revision) - return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth) + return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth, **kwargs) - def get_items(self, course_locator, settings=None, content=None, revision=None, **kwargs): + def get_items(self, course_locator, revision=None, **kwargs): """ Returns a list of XModuleDescriptor instances for the matching items within the course with the given course_locator. """ course_locator = self._map_revision_to_branch(course_locator, revision=revision) - return super(DraftVersioningModuleStore, self).get_items( - course_locator, - settings=settings, - content=content, - **kwargs - ) + return super(DraftVersioningModuleStore, self).get_items(course_locator, **kwargs) def get_parent_location(self, location, revision=None, **kwargs): ''' @@ -200,14 +202,18 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :param xblock: the block to check :return: True if the draft and published versions differ """ - # TODO for better performance: lookup the courses and get the block entry, don't create the instances - draft = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.draft)) - try: - published = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.published)) - except ItemNotFoundError: + def get_block(branch_name): + course_structure = self._lookup_course(xblock.location.for_branch(branch_name))['structure'] + return self._get_block_from_structure(course_structure, xblock.location.block_id) + + draft_block = get_block(ModuleStoreEnum.BranchName.draft) + published_block = get_block(ModuleStoreEnum.BranchName.published) + if not published_block: return True - return draft.update_version != published.source_version + # check if the draft has changed since the published was created + return self._get_version(draft_block) != self._get_version(published_block) + def publish(self, location, user_id, blacklist=None, **kwargs): """ @@ -224,15 +230,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS [location], blacklist=blacklist ) - return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published)) + return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published), **kwargs) - def unpublish(self, location, user_id): + def unpublish(self, location, user_id, **kwargs): """ Deletes the published version of the item. Returns the newly unpublished item. """ self.delete_item(location, user_id, revision=ModuleStoreEnum.RevisionOption.published_only) - return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft)) + return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft), **kwargs) def revert_to_published(self, location, user_id): """ @@ -255,24 +261,13 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS PublishState.public - published exists and is the same as draft PublishState.private - no published version exists """ - def get_head(branch): - course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure'] - return self._get_block_from_structure(course_structure, xblock.location.block_id) - - def get_version(block): - """ - Return the version of the given database representation of a block. - """ - #TODO: make this method a more generic helper - return block['edit_info'].get('source_version', block['edit_info']['update_version']) - - draft_head = get_head(ModuleStoreEnum.BranchName.draft) - published_head = get_head(ModuleStoreEnum.BranchName.published) + draft_head = self._get_head(xblock, ModuleStoreEnum.BranchName.draft) + published_head = self._get_head(xblock, ModuleStoreEnum.BranchName.published) if not published_head: # published version does not exist return PublishState.private - elif get_version(draft_head) == get_version(published_head): + elif self._get_version(draft_head) == self._get_version(published_head): # published and draft versions are equal return PublishState.public else: @@ -287,3 +282,13 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ # This is a no-op in Split since a draft version of the data always remains pass + + def _get_head(self, xblock, branch): + course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure'] + return self._get_block_from_structure(course_structure, xblock.location.block_id) + + def _get_version(self, block): + """ + Return the version of the given database representation of a block. + """ + return block['edit_info'].get('source_version', block['edit_info']['update_version']) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index d3fca2d8b0..dae355972e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -15,45 +15,52 @@ class SplitMongoKVS(InheritanceKeyValueStore): known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, definition, fields, inherited_settings): + def __init__(self, definition, initial_values, inherited_settings, **kwargs): """ :param definition: either a lazyloader or definition id for the definition - :param fields: a dictionary of the locally set fields + :param initial_values: a dictionary of the locally set values :param inherited_settings: the json value of each inheritable field from above this. Note, local fields may override and disagree w/ this b/c this says what the value should be if the field is undefined. """ # deepcopy so that manipulations of fields does not pollute the source - super(SplitMongoKVS, self).__init__(copy.deepcopy(fields), inherited_settings) + super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values), inherited_settings) self._definition = definition # either a DefinitionLazyLoader or the db id of the definition. # if the db id, then the definition is presumed to be loaded into _fields + # a decorator function for field values (to be called when a field is accessed) + self.field_decorator = kwargs.get('field_decorator', lambda x: x) + def get(self, key): - # simplest case, field is directly set + # load the field, if needed + if key.field_name not in self._fields: + # parent undefined in editing runtime (I think) + if key.scope == Scope.parent: + # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None + return None + if key.scope == Scope.children: + # didn't find children in _fields; so, see if there's a default + raise KeyError() + elif key.scope == Scope.settings: + # get default which may be the inherited value + raise KeyError() + elif key.scope == Scope.content: + if isinstance(self._definition, DefinitionLazyLoader): + self._load_definition() + else: + raise KeyError() + else: + raise InvalidScopeError(key) + if key.field_name in self._fields: - return self._fields[key.field_name] + field_value = self._fields[key.field_name] - # parent undefined in editing runtime (I think) - if key.scope == Scope.parent: - # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None - return None - if key.scope == Scope.children: - # didn't find children in _fields; so, see if there's a default - raise KeyError() - elif key.scope == Scope.settings: - # get default which may be the inherited value - raise KeyError() - elif key.scope == Scope.content: - if isinstance(self._definition, DefinitionLazyLoader): - self._load_definition() - if key.field_name in self._fields: - return self._fields[key.field_name] + # return the "decorated" field value + return self.field_decorator(field_value) - raise KeyError() - else: - raise InvalidScopeError(key) + return None def set(self, key, value): # handle any special cases diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_contentstore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_contentstore.py index 1ce5265a17..cda981e329 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_contentstore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_contentstore.py @@ -9,7 +9,8 @@ from tempfile import mkdtemp import path import shutil -from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation +from opaque_keys.edx.locator import CourseLocator, AssetLocator +from opaque_keys.edx.keys import AssetKey from xmodule.tests import DATA_DIR from xmodule.contentstore.mongo import MongoContentStore from xmodule.contentstore.content import StaticContent @@ -41,13 +42,13 @@ class TestContentstore(unittest.TestCase): Restores deprecated values """ if cls.asset_deprecated is not None: - setattr(AssetLocation, 'deprecated', cls.asset_deprecated) + setattr(AssetLocator, 'deprecated', cls.asset_deprecated) else: - delattr(AssetLocation, 'deprecated') + delattr(AssetLocator, 'deprecated') if cls.ssck_deprecated is not None: - setattr(SlashSeparatedCourseKey, 'deprecated', cls.ssck_deprecated) + setattr(CourseLocator, 'deprecated', cls.ssck_deprecated) else: - delattr(SlashSeparatedCourseKey, 'deprecated') + delattr(CourseLocator, 'deprecated') return super(TestContentstore, cls).tearDownClass() def set_up_assets(self, deprecated): @@ -59,11 +60,11 @@ class TestContentstore(unittest.TestCase): self.contentstore = MongoContentStore(HOST, DB, port=PORT) self.addCleanup(self.contentstore._drop_database) # pylint: disable=protected-access - setattr(AssetLocation, 'deprecated', deprecated) - setattr(SlashSeparatedCourseKey, 'deprecated', deprecated) + setattr(AssetLocator, 'deprecated', deprecated) + setattr(CourseLocator, 'deprecated', deprecated) - self.course1_key = SlashSeparatedCourseKey('test', 'asset_test', '2014_07') - self.course2_key = SlashSeparatedCourseKey('test', 'asset_test2', '2014_07') + self.course1_key = CourseLocator('test', 'asset_test', '2014_07') + self.course2_key = CourseLocator('test', 'asset_test2', '2014_07') self.course1_files = ['contains.sh', 'picture1.jpg', 'picture2.jpg'] self.course2_files = ['picture1.jpg', 'picture3.jpg', 'door_2.ogg'] @@ -154,13 +155,13 @@ class TestContentstore(unittest.TestCase): course1_assets, count = self.contentstore.get_all_content_for_course(self.course1_key) self.assertEqual(count, len(self.course1_files), course1_assets) for asset in course1_assets: - parsed = AssetLocation.from_deprecated_string(asset['filename']) + parsed = AssetKey.from_string(asset['filename']) self.assertIn(parsed.name, self.course1_files) course1_assets, __ = self.contentstore.get_all_content_for_course(self.course1_key, 1, 1) self.assertEqual(len(course1_assets), 1, course1_assets) - fake_course = SlashSeparatedCourseKey('test', 'fake', 'non') + fake_course = CourseLocator('test', 'fake', 'non') course_assets, count = self.contentstore.get_all_content_for_course(fake_course) self.assertEqual(count, 0) self.assertEqual(course_assets, []) @@ -183,7 +184,7 @@ class TestContentstore(unittest.TestCase): copy_all_course_assets """ self.set_up_assets(deprecated) - dest_course = SlashSeparatedCourseKey('test', 'destination', 'copy') + dest_course = CourseLocator('test', 'destination', 'copy') self.contentstore.copy_all_course_assets(self.course1_key, dest_course) for filename in self.course1_files: asset_key = self.course1_key.make_asset_key('asset', filename) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 07166e1739..3dc1c64b4b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,6 +1,7 @@ import pymongo from uuid import uuid4 import ddt +import itertools from importlib import import_module from collections import namedtuple import unittest @@ -8,7 +9,6 @@ import datetime from pytz import UTC from xmodule.tests import DATA_DIR -from opaque_keys.edx.locations import Location from xmodule.modulestore import ModuleStoreEnum, PublishState from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.exceptions import InvalidVersionError @@ -21,6 +21,7 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from django.conf import settings from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.search import path_to_location +from xmodule.modulestore.exceptions import DuplicateCourseError if not settings.configured: settings.configure() from xmodule.modulestore.mixed import MixedModuleStore @@ -90,7 +91,7 @@ class TestMixedModuleStore(unittest.TestCase): """ AssertEqual replacement for CourseLocator """ - if loc1.version_agnostic() != loc2.version_agnostic(): + if loc1.for_branch(None) != loc2.for_branch(None): self.fail(self._formatMessage(msg, u"{} != {}".format(unicode(loc1), unicode(loc2)))) def setUp(self): @@ -124,13 +125,13 @@ class TestMixedModuleStore(unittest.TestCase): # create course self.course = self.store.create_course(course_key.org, course_key.course, course_key.run, self.user_id) if isinstance(self.course.id, CourseLocator): - self.course_locations[self.MONGO_COURSEID] = self.course.location.version_agnostic() + self.course_locations[self.MONGO_COURSEID] = self.course.location else: self.assertEqual(self.course.id, course_key) # create chapter chapter = self.store.create_child(self.user_id, self.course.location, 'chapter', block_id='Overview') - self.writable_chapter_location = chapter.location.version_agnostic() + self.writable_chapter_location = chapter.location def _create_block_hierarchy(self): """ @@ -175,13 +176,13 @@ class TestMixedModuleStore(unittest.TestCase): def create_sub_tree(parent, block_info): block = self.store.create_child( - self.user_id, parent.location.version_agnostic(), + self.user_id, parent.location, block_info.category, block_id=block_info.display_name, fields={'display_name': block_info.display_name}, ) for tree in block_info.sub_tree: create_sub_tree(block, tree) - setattr(self, block_info.field_name, block.location.version_agnostic()) + setattr(self, block_info.field_name, block.location) for tree in trees: create_sub_tree(self.course, tree) @@ -192,6 +193,10 @@ class TestMixedModuleStore(unittest.TestCase): """ return self.course_locations[string].course_key + def _initialize_mixed(self): + self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options) + self.addCleanup(self.store.close_all_connections) + def initdb(self, default): """ Initialize the database and create one test course in it @@ -203,8 +208,7 @@ class TestMixedModuleStore(unittest.TestCase): if index > 0: store_configs[index], store_configs[0] = store_configs[0], store_configs[index] break - self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options) - self.addCleanup(self.store.close_all_connections) + self._initialize_mixed() # convert to CourseKeys self.course_locations = { @@ -216,12 +220,9 @@ class TestMixedModuleStore(unittest.TestCase): course_id: course_key.make_usage_key('course', course_key.run) for course_id, course_key in self.course_locations.iteritems() # pylint: disable=maybe-no-member } - if default == 'split': - self.fake_location = CourseLocator( - 'foo', 'bar', 'slowly', branch=ModuleStoreEnum.BranchName.draft - ).make_usage_key('vertical', 'baz') - else: - self.fake_location = Location('foo', 'bar', 'slowly', 'vertical', 'baz') + + self.fake_location = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'fake') + self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace( category='chapter', name='Overview' ) @@ -248,6 +249,23 @@ class TestMixedModuleStore(unittest.TestCase): SlashSeparatedCourseKey('foo', 'bar', '2012_Fall')), mongo_ms_type ) + @ddt.data(*itertools.product( + (ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split), + (True, False) + )) + @ddt.unpack + def test_duplicate_course_error(self, default_ms, reset_mixed_mappings): + """ + Make sure we get back the store type we expect for given mappings + """ + self._initialize_mixed() + with self.store.default_store(default_ms): + self.store.create_course('org_x', 'course_y', 'run_z', self.user_id) + if reset_mixed_mappings: + self.store.mappings = {} + with self.assertRaises(DuplicateCourseError): + self.store.create_course('org_x', 'course_y', 'run_z', self.user_id) + # split has one lookup for the course and then one for the course items @ddt.data(('draft', 1, 0), ('split', 2, 0)) @ddt.unpack @@ -308,7 +326,7 @@ class TestMixedModuleStore(unittest.TestCase): course_locn = self.course_locations[self.XML_COURSEID1] # NOTE: use get_course if you just want the course. get_items is expensive - modules = self.store.get_items(course_locn.course_key, category='course') + modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'course'}) self.assertEqual(len(modules), 1) self.assertEqual(modules[0].location, course_locn) @@ -316,7 +334,7 @@ class TestMixedModuleStore(unittest.TestCase): course_locn = self.course_locations[self.MONGO_COURSEID] with check_mongo_calls(mongo_store, max_find, max_send): # NOTE: use get_course if you just want the course. get_items is expensive - modules = self.store.get_items(course_locn.course_key, category='problem') + modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'}) self.assertEqual(len(modules), 6) # verify that an error is raised when the revision is not valid @@ -368,7 +386,7 @@ class TestMixedModuleStore(unittest.TestCase): # Create dummy direct only xblocks chapter = self.store.create_item( self.user_id, - test_course.id.version_agnostic(), + test_course.id, 'chapter', block_id='vertical_container' ) @@ -389,7 +407,7 @@ class TestMixedModuleStore(unittest.TestCase): # Create a dummy component to test against xblock = self.store.create_item( self.user_id, - test_course.id.version_agnostic(), + test_course.id, 'vertical', block_id='test_vertical' ) @@ -504,7 +522,7 @@ class TestMixedModuleStore(unittest.TestCase): revision=ModuleStoreEnum.RevisionOption.draft_preferred ) - self.store.publish(private_vert.location.version_agnostic(), self.user_id) + self.store.publish(private_vert.location, self.user_id) private_leaf.display_name = 'change me' private_leaf = self.store.update_item(private_leaf, self.user_id) mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) @@ -512,7 +530,7 @@ class TestMixedModuleStore(unittest.TestCase): with check_mongo_calls(mongo_store, max_find, max_send): self.store.delete_item(private_leaf.location, self.user_id) - @ddt.data(('draft', 3, 0), ('split', 6, 0)) + @ddt.data(('draft', 2, 0), ('split', 3, 0)) @ddt.unpack def test_get_courses(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -520,16 +538,19 @@ class TestMixedModuleStore(unittest.TestCase): mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) with check_mongo_calls(mongo_store, max_find, max_send): courses = self.store.get_courses() - course_ids = [ - course.location.version_agnostic() - if hasattr(course.location, 'version_agnostic') else course.location - for course in courses - ] + course_ids = [course.location for course in courses] self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids)) self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + draft_courses = self.store.get_courses(remove_branch=True) + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only): + published_courses = self.store.get_courses(remove_branch=True) + self.assertEquals([c.id for c in draft_courses], [c.id for c in published_courses]) + + def test_xml_get_courses(self): """ Test that the xml modulestore only loaded the courses from the maps. @@ -604,7 +625,7 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() # publish the course - self.course = self.store.publish(self.course.location.version_agnostic(), self.user_id) + self.course = self.store.publish(self.course.location, self.user_id) # make drafts of verticals self.store.convert_to_draft(self.vertical_x1a, self.user_id) @@ -630,7 +651,7 @@ class TestMixedModuleStore(unittest.TestCase): ]) # publish the course again - self.store.publish(self.course.location.version_agnostic(), self.user_id) + self.store.publish(self.course.location, self.user_id) self.verify_get_parent_locations_results([ (child_to_move_location, new_parent_location, None), (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), @@ -870,7 +891,7 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) block = self.store.create_item( self.user_id, - self.course.location.version_agnostic().course_key, + self.course.location.course_key, 'problem' ) self.assertEqual(self.user_id, block.edited_by) @@ -881,7 +902,7 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) block = self.store.create_item( self.user_id, - self.course.location.version_agnostic().course_key, + self.course.location.course_key, 'problem' ) self.assertEqual(self.user_id, block.subtree_edited_by) @@ -926,7 +947,7 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() # publish - self.store.publish(self.course.location.version_agnostic(), self.user_id) + self.store.publish(self.course.location, self.user_id) published_xblock = self.store.get_item( self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.published_only @@ -962,7 +983,7 @@ class TestMixedModuleStore(unittest.TestCase): # start off as Private item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state') - item_location = item.location.version_agnostic() + item_location = item.location mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) with check_mongo_calls(mongo_store, max_find, max_send): self.assertEquals(self.store.compute_publish_state(item), PublishState.private) @@ -1012,13 +1033,13 @@ class TestMixedModuleStore(unittest.TestCase): test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) self.assertEqual(self.store.compute_publish_state(test_course), PublishState.public) - test_course_key = test_course.id.version_agnostic() + test_course_key = test_course.id # test create_item of direct-only category to make sure we are autopublishing chapter = self.store.create_item(self.user_id, test_course_key, 'chapter', 'Overview') self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) - chapter_location = chapter.location.version_agnostic() + chapter_location = chapter.location # test create_child of direct-only category to make sure we are autopublishing sequential = self.store.create_child(self.user_id, chapter_location, 'sequential', 'Sequence') @@ -1189,6 +1210,59 @@ class TestMixedModuleStore(unittest.TestCase): # there should be no published problems with the old name assertNumProblems(problem_original_name, 0) + def verify_default_store(self, store_type): + # verify default_store property + self.assertEquals(self.store.default_modulestore.get_modulestore_type(), store_type) + + # verify internal helper method + store = self.store._get_modulestore_for_courseid() + self.assertEquals(store.get_modulestore_type(), store_type) + + # verify store used for creating a course + try: + course = self.store.create_course("org", "course{}".format(uuid4().hex[:3]), "run", self.user_id) + self.assertEquals(course.system.modulestore.get_modulestore_type(), store_type) + except NotImplementedError: + self.assertEquals(store_type, ModuleStoreEnum.Type.xml) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.xml) + def test_default_store(self, default_ms): + """ + Test the default store context manager + """ + # initialize the mixed modulestore + self._initialize_mixed() + + with self.store.default_store(default_ms): + self.verify_default_store(default_ms) + + def test_default_store_nested(self): + """ + Test the default store context manager, nested within one another + """ + # initialize the mixed modulestore + self._initialize_mixed() + + with self.store.default_store(ModuleStoreEnum.Type.mongo): + self.verify_default_store(ModuleStoreEnum.Type.mongo) + with self.store.default_store(ModuleStoreEnum.Type.split): + self.verify_default_store(ModuleStoreEnum.Type.split) + with self.store.default_store(ModuleStoreEnum.Type.xml): + self.verify_default_store(ModuleStoreEnum.Type.xml) + self.verify_default_store(ModuleStoreEnum.Type.split) + self.verify_default_store(ModuleStoreEnum.Type.mongo) + + def test_default_store_fake(self): + """ + Test the default store context manager, asking for a fake store + """ + # initialize the mixed modulestore + self._initialize_mixed() + + fake_store = "fake" + with self.assertRaisesRegexp(Exception, "Cannot find store of type {}".format(fake_store)): + with self.store.default_store(fake_store): + pass # pragma: no cover #============================================================================================================= # General utils for not using django settings diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py index a0b63d4691..166bf22327 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py @@ -45,7 +45,6 @@ class ModuleStoreSettingsMigration(TestCase): "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore", "OPTIONS": { "mappings": {}, - "reference_type": "Location", "stores": { "an_old_mongo_store": { "DOC_STORE_CONFIG": {}, @@ -77,15 +76,47 @@ class ModuleStoreSettingsMigration(TestCase): } } + ALREADY_UPDATED_MIXED_CONFIG = { + 'default': { + 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', + 'OPTIONS': { + 'mappings': {}, + 'stores': [ + { + 'NAME': 'split', + 'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore', + 'DOC_STORE_CONFIG': {}, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': "fs_root", + 'render_template': 'edxmako.shortcuts.render_to_string', + } + }, + { + 'NAME': 'draft', + 'ENGINE': 'xmodule.modulestore.mongo.draft.DraftModuleStore', + 'DOC_STORE_CONFIG': {}, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': "fs_root", + 'render_template': 'edxmako.shortcuts.render_to_string', + } + }, + ] + } + } + } + + def _get_mixed_stores(self, mixed_setting): """ - Helper for accessing stores in a configuration setting for the Mixed modulestore + Helper for accessing stores in a configuration setting for the Mixed modulestore. """ return mixed_setting["default"]["OPTIONS"]["stores"] def assertStoreValuesEqual(self, store_setting1, store_setting2): """ - Tests whether the fields in the given store_settings are equal + Tests whether the fields in the given store_settings are equal. """ store_fields = ["OPTIONS", "DOC_STORE_CONFIG"] for field in store_fields: @@ -108,26 +139,56 @@ class ModuleStoreSettingsMigration(TestCase): return new_mixed_setting, new_stores[0] + def is_split_configured(self, mixed_setting): + """ + Tests whether the split module store is configured in the given setting. + """ + stores = self._get_mixed_stores(mixed_setting) + split_settings = [store for store in stores if store['ENGINE'].endswith('.DraftVersioningModuleStore')] + if len(split_settings): + # there should only be one setting for split + self.assertEquals(len(split_settings), 1) + # verify name + self.assertEquals(split_settings[0]['NAME'], 'split') + # verify split config settings equal those of mongo + self.assertStoreValuesEqual( + split_settings[0], + next((store for store in stores if 'DraftModuleStore' in store['ENGINE']), None) + ) + return len(split_settings) > 0 + def test_convert_into_mixed(self): old_setting = self.OLD_CONFIG - _, new_default_store_setting = self.assertMigrated(old_setting) + new_mixed_setting, new_default_store_setting = self.assertMigrated(old_setting) self.assertStoreValuesEqual(new_default_store_setting, old_setting["default"]) self.assertEqual(new_default_store_setting["ENGINE"], old_setting["default"]["ENGINE"]) + self.assertFalse(self.is_split_configured(new_mixed_setting)) def test_convert_from_old_mongo_to_draft_store(self): old_setting = self.OLD_CONFIG_WITH_DIRECT_MONGO - _, new_default_store_setting = self.assertMigrated(old_setting) + new_mixed_setting, new_default_store_setting = self.assertMigrated(old_setting) self.assertStoreValuesEqual(new_default_store_setting, old_setting["default"]) self.assertEqual(new_default_store_setting["ENGINE"], "xmodule.modulestore.mongo.draft.DraftModuleStore") + self.assertTrue(self.is_split_configured(new_mixed_setting)) def test_convert_from_dict_to_list(self): old_mixed_setting = self.OLD_MIXED_CONFIG_WITH_DICT new_mixed_setting, new_default_store_setting = self.assertMigrated(old_mixed_setting) self.assertEqual(new_default_store_setting["ENGINE"], "the_default_store") + self.assertTrue(self.is_split_configured(new_mixed_setting)) + + # exclude split when comparing old and new, since split was added as part of the migration + new_stores = [store for store in self._get_mixed_stores(new_mixed_setting) if store['NAME'] != 'split'] + old_stores = self._get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT) # compare each store configured in mixed - old_stores = self._get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT) - new_stores = self._get_mixed_stores(new_mixed_setting) self.assertEqual(len(new_stores), len(old_stores)) - for new_store_setting in self._get_mixed_stores(new_mixed_setting): - self.assertStoreValuesEqual(new_store_setting, old_stores[new_store_setting['NAME']]) + for new_store in new_stores: + self.assertStoreValuesEqual(new_store, old_stores[new_store['NAME']]) + + def test_no_conversion(self): + # make sure there is no migration done on an already updated config + old_mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG + new_mixed_setting, new_default_store_setting = self.assertMigrated(old_mixed_setting) + self.assertTrue(self.is_split_configured(new_mixed_setting)) + self.assertEquals(old_mixed_setting, new_mixed_setting) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index c92e817533..835375ae7e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py @@ -151,7 +151,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): # grab the detached items to compare they should be in both published and draft for category in ['conditional', 'about', 'course_info', 'static_tab']: - for conditional in presplit.get_items(self.old_course_key, category=category): + for conditional in presplit.get_items(self.old_course_key, qualifiers={'category': category}): locator = new_course_key.make_usage_key(category, conditional.location.block_id) self.compare_dags(presplit, conditional, self.split_mongo.get_item(locator), published) 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 8e64247c80..466251f418 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -895,18 +895,18 @@ class SplitModuleItemTests(SplitModuleTest): self.assertEqual(len(matches), 6) matches = modulestore().get_items(locator) self.assertEqual(len(matches), 6) - matches = modulestore().get_items(locator, category='chapter') + matches = modulestore().get_items(locator, qualifiers={'category': 'chapter'}) self.assertEqual(len(matches), 3) - matches = modulestore().get_items(locator, category='garbage') + matches = modulestore().get_items(locator, qualifiers={'category': 'garbage'}) self.assertEqual(len(matches), 0) matches = modulestore().get_items( locator, - category='chapter', + qualifiers={'category': 'chapter'}, settings={'display_name': re.compile(r'Hera')}, ) self.assertEqual(len(matches), 2) - matches = modulestore().get_items(locator, children='chapter2') + matches = modulestore().get_items(locator, qualifiers={'children': 'chapter2'}) self.assertEqual(len(matches), 1) self.assertEqual(matches[0].location.block_id, 'head12345') @@ -1324,7 +1324,7 @@ class TestItemCrud(SplitModuleTest): reusable_location = course.id.version_agnostic().for_branch(BRANCH_NAME_DRAFT) # delete a leaf - problems = modulestore().get_items(reusable_location, category='problem') + problems = modulestore().get_items(reusable_location, qualifiers={'category': 'problem'}) locn_to_del = problems[0].location new_course_loc = modulestore().delete_item(locn_to_del, self.user_id) deleted = locn_to_del.version_agnostic() @@ -1336,7 +1336,7 @@ class TestItemCrud(SplitModuleTest): self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid) # delete a subtree - nodes = modulestore().get_items(reusable_location, category='chapter') + nodes = modulestore().get_items(reusable_location, qualifiers={'category': 'chapter'}) new_course_loc = modulestore().delete_item(nodes[0].location, self.user_id) # check subtree diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 5c71518364..b329fdc32a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -24,6 +24,7 @@ from xmodule.modulestore import ModuleStoreEnum, ModuleStoreReadBase from xmodule.tabs import CourseTabList from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location +from opaque_keys.edx.locator import CourseLocator from xblock.field_data import DictFieldData from xblock.runtime import DictKeyValueStore, IdGenerator @@ -403,7 +404,6 @@ class XMLModuleStore(ModuleStoreReadBase): self.default_class = class_ self.parent_trackers = defaultdict(ParentTracker) - self.reference_type = Location # All field data will be stored in an inheriting field data. self.field_data = inheriting_field_data(kvs=DictKeyValueStore()) @@ -700,7 +700,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return usage_key in self.modules[usage_key.course_key] - def get_item(self, usage_key, depth=0): + def get_item(self, usage_key, depth=0, **kwargs): """ Returns an XBlock instance for the item for this UsageKey. @@ -717,7 +717,7 @@ class XMLModuleStore(ModuleStoreReadBase): except KeyError: raise ItemNotFoundError(usage_key) - def get_items(self, course_id, settings=None, content=None, revision=None, **kwargs): + def get_items(self, course_id, settings=None, content=None, revision=None, qualifiers=None, **kwargs): """ Returns: list of XModuleDescriptor instances for the matching items within the course with @@ -729,10 +729,10 @@ class XMLModuleStore(ModuleStoreReadBase): Args: course_id (CourseKey): the course identifier settings (dict): fields to look for which have settings scope. Follows same syntax - and rules as kwargs below + and rules as qualifiers below content (dict): fields to look for which have content scope. Follows same syntax and - rules as kwargs below. - kwargs (key=value): what to look for within the course. + rules as qualifiers below. + qualifiers (dict): what to look for within the course. Common qualifiers are ``category`` or any field name. if the target field is a list, then it searches for the given value in the list not list equivalence. Substring matching pass a regex object. @@ -747,8 +747,9 @@ class XMLModuleStore(ModuleStoreReadBase): items = [] - category = kwargs.pop('category', None) - name = kwargs.pop('name', None) + qualifiers = qualifiers.copy() if qualifiers else {} # copy the qualifiers (destructively manipulated here) + category = qualifiers.pop('category', None) + name = qualifiers.pop('name', None) def _block_matches_all(mod_loc, module): if category and mod_loc.category != category: @@ -757,7 +758,7 @@ class XMLModuleStore(ModuleStoreReadBase): return False return all( self._block_matches(module, fields or {}) - for fields in [settings, content, kwargs] + for fields in [settings, content, qualifiers] ) for mod_loc, module in self.modules[course_id].iteritems(): @@ -766,7 +767,16 @@ class XMLModuleStore(ModuleStoreReadBase): return items - def get_courses(self, depth=0): + def make_course_key(self, org, course, run): + """ + Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore + that matches the supplied `org`, `course`, and `run`. + + This key may represent a course that doesn't exist in this modulestore. + """ + return CourseLocator(org, course, run, deprecated=True) + + def get_courses(self, depth=0, **kwargs): """ Returns a list of course descriptors. If there were errors on loading, some of these may be ErrorDescriptors instead. @@ -780,7 +790,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return dict((k, self.errored_courses[k].errors) for k in self.errored_courses) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Get all of the xblocks in the given course which have no parents and are not of types which are usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't @@ -806,7 +816,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return ModuleStoreEnum.Type.xml - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 60cb5f6700..7d5342a3b3 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -106,7 +106,7 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): # and index here since the XML modulestore cannot load draft modules draft_verticals = modulestore.get_items( course_key, - category='vertical', + qualifiers={'category': 'vertical'}, revision=ModuleStoreEnum.RevisionOption.draft_only ) if len(draft_verticals) > 0: @@ -144,7 +144,7 @@ def _export_field_content(xblock_item, item_dir): def export_extra_content(export_fs, modulestore, course_key, category_type, dirname, file_suffix=''): - items = modulestore.get_items(course_key, category=category_type) + items = modulestore.get_items(course_key, qualifiers={'category': category_type}) if len(items) > 0: item_dir = export_fs.makeopendir(dirname) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 84527ce8ba..f7bdd97091 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -17,7 +17,7 @@ from .store_utilities import rewrite_nonportable_content_links import xblock from xmodule.tabs import CourseTabList from xmodule.modulestore.django import ASSET_IGNORE_REGEX -from xmodule.modulestore.exceptions import InvalidLocationError +from xmodule.modulestore.exceptions import DuplicateCourseError from xmodule.modulestore.mongo.base import MongoRevisionKey from xmodule.modulestore import ModuleStoreEnum @@ -174,8 +174,9 @@ def import_from_xml( if create_new_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True): try: store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id) - except InvalidLocationError: + except DuplicateCourseError: # course w/ same org and course exists + # The Mongo modulestore checks *with* the run in has_course, but not in create_course. log.debug( "Skipping import of course with id, {0}," "since it collides with an existing one".format(dest_course_id) diff --git a/lms/djangoapps/courseware/tests/test_draft_modulestore.py b/lms/djangoapps/courseware/tests/test_draft_modulestore.py index d3c6163c55..991c18cc13 100644 --- a/lms/djangoapps/courseware/tests/test_draft_modulestore.py +++ b/lms/djangoapps/courseware/tests/test_draft_modulestore.py @@ -13,7 +13,7 @@ class TestDraftModuleStore(TestCase): store = modulestore() # fix was to allow get_items() to take the course_id parameter - store.get_items(SlashSeparatedCourseKey('a', 'b', 'c'), category='vertical') + store.get_items(SlashSeparatedCourseKey('a', 'b', 'c'), qualifiers={'category': 'vertical'}) # test success is just getting through the above statement. # The bug was that 'course_id' argument was diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index e3e13ef9a9..dbb17f061f 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -163,7 +163,7 @@ class TestDraftModuleStore(ModuleStoreTestCase): store = modulestore() # fix was to allow get_items() to take the course_id parameter - store.get_items(SlashSeparatedCourseKey('abc', 'def', 'ghi'), category='vertical') + store.get_items(SlashSeparatedCourseKey('abc', 'def', 'ghi'), qualifiers={'category': 'vertical'}) # test success is just getting through the above statement. # The bug was that 'course_id' argument was diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 8c23a0512b..70d6dc0d9c 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -463,7 +463,7 @@ def jump_to_id(request, course_id, module_id): passed in. This assumes that id is unique within the course_id namespace """ course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - items = modulestore().get_items(course_key, name=module_id) + items = modulestore().get_items(course_key, qualifiers={'name': module_id}) if len(items) == 0: raise Http404( @@ -937,7 +937,7 @@ def get_course_lti_endpoints(request, course_id): anonymous_user = AnonymousUser() anonymous_user.known = False # make these "noauth" requests like module_render.handle_xblock_callback_noauth - lti_descriptors = modulestore().get_items(course.id, category='lti') + lti_descriptors = modulestore().get_items(course.id, qualifiers={'category': 'lti'}) lti_noauth_modules = [ get_module_for_descriptor( diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 1c912de7c0..8ec578b956 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -57,7 +57,7 @@ def has_forum_access(uname, course_id, rolename): def _get_discussion_modules(course): - all_modules = modulestore().get_items(course.id, category='discussion') + all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'}) def has_required_keys(module): for key in ('discussion_id', 'discussion_category', 'discussion_target'): diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index ec98c5e1c1..c24c0540a6 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/lms/djangoapps/open_ended_grading/views.py @@ -92,7 +92,7 @@ def find_peer_grading_module(course): problem_url = "" # Get the peer grading modules currently in the course. Explicitly specify the course id to avoid issues with different runs. - items = modulestore().get_items(course.id, category='peergrading') + items = modulestore().get_items(course.id, qualifiers={'category': 'peergrading'}) # See if any of the modules are centralized modules (ie display info from multiple problems) items = [i for i in items if not getattr(i, "use_for_single_location", True)] # Loop through all potential peer grading modules, and find the first one that has a path to it. diff --git a/lms/envs/bok_choy.auth.json b/lms/envs/bok_choy.auth.json index a0429f4159..a094274a5b 100644 --- a/lms/envs/bok_choy.auth.json +++ b/lms/envs/bok_choy.auth.json @@ -51,7 +51,6 @@ "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore", "OPTIONS": { "mappings": {}, - "reference_type": "Location", "stores": [ { "NAME": "draft", diff --git a/lms/envs/common.py b/lms/envs/common.py index 7ae3f8346b..a667fe246e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -504,7 +504,6 @@ MODULESTORE = { 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { 'mappings': {}, - 'reference_type': 'Location', 'stores': [ { 'NAME': 'draft', @@ -524,6 +523,16 @@ MODULESTORE = { 'default_class': 'xmodule.hidden_module.HiddenDescriptor', } }, + { + 'NAME': 'split', + 'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': DATA_DIR, + 'render_template': 'edxmako.shortcuts.render_to_string', + } + }, ] } }