From 47851c5041e99ac6f93204f259e8b7de736d1286 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 31 Jul 2014 13:41:00 -0400 Subject: [PATCH 01/15] Replace create_xmodule with create_xblock Refactor all callers (often replacing with more appropriate fn) Also, added for_branch_setting method to old mongo and removed some branch verification tests Conflicts: common/lib/xmodule/xmodule/modulestore/mongo/draft.py common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py common/lib/xmodule/xmodule/modulestore/xml_importer.py --- .../contentstore/tests/test_contentstore.py | 13 ++--- .../contentstore/tests/test_crud.py | 11 ++-- .../contentstore/tests/test_utils.py | 14 ++--- .../models/settings/course_details.py | 2 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 12 ++--- .../xmodule/xmodule/modulestore/mongo/base.py | 54 ++++++++++--------- .../xmodule/modulestore/mongo/draft.py | 25 ++++----- .../xmodule/modulestore/split_mongo/split.py | 15 ++++-- .../xmodule/modulestore/tests/factories.py | 20 +++---- .../tests/test_split_modulestore.py | 5 +- .../xmodule/modulestore/xml_importer.py | 6 ++- 11 files changed, 86 insertions(+), 91 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 7fea7df3e2..acd08724eb 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1412,17 +1412,14 @@ class ContentStoreTest(ContentStoreTestCase): self.assertGreater(len(verticals), 0) - new_component_location = course.id.make_usage_key('html', 'new_component') - # crate a new module and add it as a child to a vertical - new_object = self.store.create_xmodule(new_component_location) - self.store.update_item(new_object, self.user.id, allow_not_found=True) parent = verticals[0] - parent.children.append(new_component_location) - self.store.update_item(parent, self.user.id) + new_module = self.store.create_child( + self.user.id, parent.location, 'html', 'new_component' + ) # flush the cache - new_module = self.store.get_item(new_component_location) + new_module = self.store.get_item(new_module.location) # check for grace period definition which should be defined at the course level self.assertEqual(parent.graceperiod, new_module.graceperiod) @@ -1438,7 +1435,7 @@ class ContentStoreTest(ContentStoreTestCase): self.store.update_item(new_module, self.user.id) # flush the cache and refetch - new_module = self.store.get_item(new_component_location) + new_module = self.store.get_item(new_module.location) self.assertEqual(timedelta(1), new_module.graceperiod) diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 91cc996ef8..459f587d36 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -89,7 +89,8 @@ class TemplateTests(unittest.TestCase): ) test_chapter = self.split_store.create_xblock( - test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course + test_course.system, test_course.id, 'chapter', fields={'display_name': 'chapter n'}, + parent_xblock=test_course ) self.assertIsInstance(test_chapter, SequenceDescriptor) self.assertEqual(test_chapter.display_name, 'chapter n') @@ -98,7 +99,8 @@ class TemplateTests(unittest.TestCase): # test w/ a definition (e.g., a problem) test_def_content = 'boo' test_problem = self.split_store.create_xblock( - test_course.system, 'problem', {'data': test_def_content}, parent_xblock=test_chapter + test_course.system, test_course.id, 'problem', fields={'data': test_def_content}, + parent_xblock=test_chapter ) self.assertIsInstance(test_problem, CapaDescriptor) self.assertEqual(test_problem.data, test_def_content) @@ -115,13 +117,14 @@ class TemplateTests(unittest.TestCase): display_name='fun test course', user_id='testbot' ) test_chapter = self.split_store.create_xblock( - test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course + test_course.system, test_course.id, 'chapter', fields={'display_name': 'chapter n'}, + parent_xblock=test_course ) self.assertEqual(test_chapter.display_name, 'chapter n') test_def_content = 'boo' # create child new_block = self.split_store.create_xblock( - test_course.system, + test_course.system, test_course.id, 'problem', fields={ 'data': test_def_content, diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index d6bbd503f1..d604bdace2 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -15,6 +15,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location from xmodule.modulestore.django import modulestore +from opaque_keys.edx.locator import CourseLocator class LMSLinksTestCase(TestCase): @@ -249,16 +250,15 @@ class XBlockVisibilityTestCase(TestCase): def _create_xblock_with_start_date(self, name, start_date, publish=False, visible_to_staff_only=False): """Helper to create an xblock with a start date, optionally publishing it""" - location = Location('edX', 'visibility', '2012_Fall', 'vertical', name) + course_key = CourseLocator('edX', 'visibility', '2012_Fall') - vertical = modulestore().create_xmodule(location) - vertical.start = start_date - if visible_to_staff_only: - vertical.visible_to_staff_only = visible_to_staff_only - modulestore().update_item(vertical, self.dummy_user, allow_not_found=True) + vertical = modulestore().create_item( + self.dummy_user, course_key, 'vertical', name, + fields={'start': start_date, 'visible_to_staff_only': visible_to_staff_only} + ) if publish: - modulestore().publish(location, self.dummy_user) + modulestore().publish(vertical.location, self.dummy_user) return vertical diff --git a/cms/djangoapps/models/settings/course_details.py b/cms/djangoapps/models/settings/course_details.py index 18d678e7a4..e466020c10 100644 --- a/cms/djangoapps/models/settings/course_details.py +++ b/cms/djangoapps/models/settings/course_details.py @@ -91,7 +91,7 @@ class CourseDetails(object): try: about_item = store.get_item(temploc) except ItemNotFoundError: - about_item = store.create_xmodule(temploc, runtime=course.runtime) + about_item = store.create_xblock(course.runtime, course.id, 'about', about_key) about_item.data = data store.update_item(about_item, user.id) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 645d7b3b40..766cd82d1e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -491,18 +491,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): modulestore._drop_database() # pylint: disable=protected-access @strip_key - def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs): + def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): """ Create the new xmodule but don't save it. Returns the new module. - - :param location: a Location--must have a category - :param definition_data: can be empty. The initial definition_data for the kvs - :param metadata: can be empty, the initial metadata for the kvs - :param runtime: if you already have an xblock from the course, the xblock.runtime value - :param fields: a dictionary of field names and values for the new xmodule """ - store = self._verify_modulestore_support(location.course_key, 'create_xmodule') - return store.create_xmodule(location, definition_data, metadata, runtime, fields, **kwargs) + store = self._verify_modulestore_support(course_key, 'create_xblock') + return store.create_xblock(runtime, course_key, block_type, block_id, fields or {}, **kwargs) @strip_key def get_courses_for_wiki(self, wiki_slug, **kwargs): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index b508056099..bb1589243e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -476,6 +476,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return course_key.replace(run=self._course_run_cache[cache_key]) + def for_branch_setting(self, location): + """ + Returns the Location that is for the current branch setting. + """ + if location.category in DIRECT_ONLY_CATEGORIES: + return location.replace(revision=MongoRevisionKey.published) + if self.get_branch_setting() == ModuleStoreEnum.Branch.draft_preferred: + return location.replace(revision=MongoRevisionKey.draft) + return location.replace(revision=MongoRevisionKey.published) + def _compute_metadata_inheritance_tree(self, course_id): ''' TODO (cdodge) This method can be deleted when the 'split module store' work has been completed @@ -938,19 +948,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): if courses.count() > 0: raise DuplicateCourseError(course_id, courses[0]['_id']) - location = course_id.make_usage_key('course', course_id.run) - course = self.create_xmodule( - location, - fields=fields, - **kwargs - ) - self.update_item(course, user_id, allow_not_found=True) + course = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs) # clone a default 'about' overview module as well - about_location = location.replace( - category='about', - name='overview' - ) + about_location = course_id.make_usage_key('about', 'overview') + overview_template = AboutDescriptor.get_template('overview.yaml') self.create_item( user_id, @@ -963,25 +965,26 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return course - def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs): + def create_xblock( + self, runtime, course_key, block_type, block_id=None, fields=None, + metadata=None, definition_data=None, **kwargs + ): """ - Create the new xmodule but don't save it. Returns the new module. + Create the new xblock but don't save it. Returns the new module. - :param location: a Location--must have a category - :param definition_data: can be empty. The initial definition_data for the kvs - :param metadata: can be empty, the initial metadata for the kvs :param runtime: if you already have an xblock from the course, the xblock.runtime value :param fields: a dictionary of field names and values for the new xmodule """ - location = location.replace(run=self.fill_in_run(location.course_key).run) - # differs from split mongo in that I believe most of this logic should be above the persistence - # layer but added it here to enable quick conversion. I'll need to reconcile these. if metadata is None: metadata = {} if definition_data is None: definition_data = {} + # @Cale, should this use LocalId like we do in split? + if block_id is None: + block_id = uuid4().hex # really need to make this more readable: e.g., u'{}{}'.format(block_type, uuid4().hex[:4] + if runtime is None: services = {} if self.i18n_service: @@ -990,7 +993,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): runtime = CachingDescriptorSystem( modulestore=self, module_data={}, - course_key=location.course_key, + course_key=course_key, default_class=self.default_class, resources_fs=None, error_tracker=self.error_tracker, @@ -1000,14 +1003,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): select=self.xblock_select, services=services, ) - xblock_class = runtime.load_block_type(location.category) - dbmodel = self._create_new_field_data(location.category, location, definition_data, metadata) + xblock_class = runtime.load_block_type(block_type) + location = course_key.make_usage_key(block_type, block_id) + dbmodel = self._create_new_field_data(block_type, location, definition_data, metadata) xmodule = runtime.construct_xblock_from_class( xblock_class, # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, # so we use the location for both. - ScopeIds(None, location.category, location, location), + ScopeIds(None, block_type, location, location), dbmodel, ) if fields is not None: @@ -1034,8 +1038,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): if block_id is None: block_id = uuid4().hex - location = course_key.make_usage_key(block_type, block_id) - xblock = self.create_xmodule(location, **kwargs) + runtime = kwargs.pop('runtime', None) + xblock = self.create_xblock(runtime, course_key, block_type, block_id, **kwargs) xblock = self.update_item(xblock, user_id, allow_not_found=True) return xblock diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index bafb2cbd07..6aaa0e10f1 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -12,9 +12,7 @@ import logging from opaque_keys.edx.locations import Location from xmodule.exceptions import InvalidVersionError from xmodule.modulestore import PublishState, ModuleStoreEnum -from xmodule.modulestore.exceptions import ( - ItemNotFoundError, DuplicateItemError, InvalidBranchSetting, DuplicateCourseError -) +from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateItemError, DuplicateCourseError from xmodule.modulestore.mongo.base import ( MongoModuleStore, MongoRevisionKey, as_draft, as_published, SORT_REVISION_FAVOR_DRAFT @@ -292,7 +290,7 @@ class DraftModuleStore(MongoModuleStore): else ModuleStoreEnum.RevisionOption.draft_preferred return super(DraftModuleStore, self).get_parent_location(location, revision, **kwargs) - def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs): + def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): """ Create the new xmodule but don't save it. Returns the new module with a draft locator if the category allows drafts. If the category does not allow drafts, just creates a published module. @@ -303,13 +301,11 @@ class DraftModuleStore(MongoModuleStore): :param runtime: if you already have an xmodule from the course, the xmodule.runtime value :param fields: a dictionary of field names and values for the new xmodule """ - self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred) - - if location.category not in DIRECT_ONLY_CATEGORIES: - location = as_draft(location) - return wrap_draft( - super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields) + new_block = super(DraftModuleStore, self).create_xblock( + runtime, course_key, block_type, block_id, fields, **kwargs ) + new_block.location = self.for_branch_setting(new_block.location) + return wrap_draft(new_block) def get_items(self, course_key, revision=None, **kwargs): """ @@ -395,7 +391,7 @@ class DraftModuleStore(MongoModuleStore): DuplicateItemError: if the source or any of its descendants already has a draft copy. Only useful for unpublish b/c we don't want unpublish to overwrite any existing drafts. """ - # verify input conditions + # verify input conditions: can only convert to draft branch; so, verify that's the setting self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred) _verify_revision_is_published(location) @@ -440,13 +436,12 @@ class DraftModuleStore(MongoModuleStore): In addition to the superclass's behavior, this method converts the unit to draft if it's not direct-only and not already draft. """ - self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred) + draft_loc = self.for_branch_setting(xblock.location) - # if the xblock is direct-only, update the PUBLISHED version - if xblock.location.category in DIRECT_ONLY_CATEGORIES: + # if the revision is published, defer to base + if draft_loc.revision == MongoRevisionKey.published: return super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found) - draft_loc = as_draft(xblock.location) if not super(DraftModuleStore, self).has_item(draft_loc): try: # ignore any descendants which are already draft diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 3d067fb66e..3ff8b90e8d 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -78,6 +78,7 @@ 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 from _collections import defaultdict +from types import NoneType log = logging.getLogger(__name__) @@ -1120,6 +1121,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): The implementation tries to detect which, if any changes, actually need to be saved and thus won't version the definition, structure, nor course if they didn't change. """ + if allow_not_found and isinstance(descriptor.location.block_id, (LocalId, NoneType)): + return self.persist_xblock_dag(descriptor, user_id, force) + original_structure = self._lookup_course(descriptor.location)['structure'] index_entry = self._get_index_if_valid(descriptor.location, force) @@ -1181,7 +1185,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # 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, **kwargs): + def create_xblock( + self, runtime, course_key, block_type, block_id=None, fields=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 @@ -1190,13 +1197,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): parent_xblock is used to compute inherited metadata as well as to append the new xblock. json_data: - - 'category': the xmodule category + - 'block_type': the xmodule block_type - 'fields': a dict of locally set fields (not inherited) in json format not pythonic typed format! - 'definition': the object id of the existing definition """ - xblock_class = runtime.load_block_type(category) + xblock_class = runtime.load_block_type(block_type) json_data = { - 'category': category, + 'category': block_type, 'fields': fields or {}, } if definition_id is not None: diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 2263b7f922..381b0a08e8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -3,7 +3,7 @@ from factory.containers import CyclicDefinitionError from uuid import uuid4 from xmodule.modulestore import prefer_xmodules, ModuleStoreEnum -from opaque_keys.edx.locations import Location +from opaque_keys.edx.locations import Location, SlashSeparatedCourseKey from opaque_keys.edx.keys import UsageKey from xblock.core import XBlock from xmodule.tabs import StaticTab @@ -55,20 +55,14 @@ class CourseFactory(XModuleFactory): run = kwargs.get('run', name) user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test) - location = Location(org, number, run, 'course', name) - with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): # Write the data to the mongo datastore - new_course = store.create_xmodule(location, metadata=kwargs.get('metadata', None)) - - # The rest of kwargs become attributes on the course: - for k, v in kwargs.iteritems(): - setattr(new_course, k, v) - - # Save the attributes we just set - new_course.save() - # Update the data in the mongo datastore - store.update_item(new_course, user_id) + kwargs.update(kwargs.get('metadata', {})) + course_key = SlashSeparatedCourseKey(org, number, run) + # TODO - We really should call create_course here. However, since create_course verifies there are no + # duplicates, this breaks several tests that do not clean up properly in between tests. + new_course = store.create_xblock(None, course_key, 'course', block_id=run, fields=kwargs) + store.update_item(new_course, user_id, allow_not_found=True) return new_course 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 466251f418..9ede47a126 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -9,7 +9,6 @@ from path import path import re import random -from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ( @@ -268,7 +267,7 @@ class SplitModuleTest(unittest.TestCase): "category": "problem", "fields": { "display_name": "Problem 3.1", - "graceperiod": "4 hours 0 minutes 0 seconds" + "graceperiod": _time_delta_field.from_json("4 hours 0 minutes 0 seconds"), }, }, { @@ -486,7 +485,7 @@ class SplitModuleTest(unittest.TestCase): parent = split_store.get_item(block_usage) block_id = LocalId(spec['id']) child = split_store.create_xblock( - course.runtime, spec['category'], spec['fields'], block_id, parent_xblock=parent + course.runtime, course.id, spec['category'], block_id, spec['fields'], parent_xblock=parent ) new_ele_dict[spec['id']] = child course = split_store.persist_xblock_dag(course, revision['user_id']) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index f7bdd97091..a45b9bb30c 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -345,8 +345,10 @@ def _import_module_and_update_references( # Move the module to a new course new_usage_key = module.scope_ids.usage_id.map_into_course(dest_course_id) if new_usage_key.category == 'course': - new_usage_key = new_usage_key.replace(name=dest_course_id.run) - new_module = store.create_xmodule(new_usage_key, runtime=runtime) + block_id = dest_course_id.run + else: + block_id = module.location.block_id + new_module = store.create_xblock(runtime, dest_course_id, new_usage_key.category, block_id) def _convert_reference_fields_to_new_namespace(reference): """ From 84992cdfa56e75f835f8d5acb224c62bcbe3ece5 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Fri, 1 Aug 2014 08:07:51 -0400 Subject: [PATCH 02/15] Refactor xml_importer.py for easier reading. Remove post-publish step. --- .../management/commands/import.py | 2 +- .../contentstore/tests/test_contentstore.py | 30 +- .../contentstore/tests/test_import.py | 10 +- .../tests/test_import_draft_order.py | 2 +- .../tests/test_import_pure_xblock.py | 2 +- .../contentstore/views/import_export.py | 2 +- .../contentstore/views/tests/test_assets.py | 4 +- .../xmodule/modulestore/xml_importer.py | 323 +++++++++--------- 8 files changed, 192 insertions(+), 183 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/import.py b/cms/djangoapps/contentstore/management/commands/import.py index b0d550228b..d742895f0e 100644 --- a/cms/djangoapps/contentstore/management/commands/import.py +++ b/cms/djangoapps/contentstore/management/commands/import.py @@ -40,7 +40,7 @@ class Command(BaseCommand): dis=do_import_static)) mstore = modulestore() - _, course_items = import_from_xml( + course_items = import_from_xml( mstore, ModuleStoreEnum.UserID.mgmt_command, data_dir, course_dirs, load_error_modules=False, static_content_store=contentstore(), verbose=True, do_import_static=do_import_static, diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index acd08724eb..32b61e5214 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -89,7 +89,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): component_types should cause 'Video' to be present. """ store = self.store - _, course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) + course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) course = course_items[0] course.advanced_modules = component_types store.update_item(course, self.user.id) @@ -116,7 +116,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): def test_malformed_edit_unit_request(self): store = self.store - _, course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) + course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) # just pick one vertical usage_key = course_items[0].id.make_usage_key('vertical', None) @@ -126,7 +126,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): def check_edit_unit(self, test_course_name): """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]) + 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, qualifiers={'category': 'vertical'}) self._check_verticals(items) @@ -148,7 +148,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): both draft and non-draft copies. ''' store = self.store - _, course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) + course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['simple']) course_key = course_items[0].id html_usage_key = course_key.make_usage_key('html', 'test_html') @@ -263,7 +263,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): self.assertEqual(num_drafts, 1) def test_no_static_link_rewrites_on_import(self): - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course = course_items[0] handouts_usage_key = course.id.make_usage_key('course_info', 'handouts') @@ -287,7 +287,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): self.assertGreater(len(course.textbooks), 0) def test_import_polls(self): - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + 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, qualifiers={'category': 'poll_question'}) @@ -307,7 +307,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): Tests the ajax callback to render an XModule """ direct_store = self.store - _, course_items = import_from_xml(direct_store, self.user.id, 'common/test/data/', ['toy']) + course_items = import_from_xml(direct_store, self.user.id, 'common/test/data/', ['toy']) usage_key = course_items[0].id.make_usage_key('vertical', 'vertical_test') # also try a custom response which will trigger the 'is this course in whitelist' logic resp = self.client.get_json( @@ -357,7 +357,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): This test case verifies that a course can use specialized override for about data, e.g. /about/Fall_2012/effort.html while there is a base definition in /about/effort.html ''' - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_key = course_items[0].id effort = self.store.get_item(course_key.make_usage_key('about', 'effort')) self.assertEqual(effort.data, '6 hours') @@ -460,7 +460,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): content_store = contentstore() trash_store = contentstore('trashcan') - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'], static_content_store=content_store) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'], static_content_store=content_store) # look up original (and thumbnail) in content store, should be there after import location = AssetLocation.from_deprecated_string('/c4x/edX/toy/asset/sample_static.txt') @@ -618,7 +618,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): """ content_store = contentstore() - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'], static_content_store=content_store) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'], static_content_store=content_store) course_id = course_items[0].id @@ -845,7 +845,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): def test_course_handouts_rewrites(self): # import a test course - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_id = course_items[0].id handouts_location = course_id.make_usage_key('course_info', 'handouts') @@ -895,7 +895,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): # Create toy course - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_id = course_items[0].id root_dir = path(mkdtemp_clean()) @@ -1271,7 +1271,7 @@ class ContentStoreTest(ContentStoreTestCase): ) self.assertEqual(resp.status_code, 200) - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['simple']) + course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['simple']) course_key = course_items[0].id resp = self._show_course_overview(course_key) @@ -1400,7 +1400,7 @@ class ContentStoreTest(ContentStoreTestCase): self.assertNotEquals(new_discussion_item.discussion_id, '$$GUID$$') def test_metadata_inheritance(self): - _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) + 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, qualifiers={'category': 'vertical'}) @@ -1466,7 +1466,7 @@ class ContentStoreTest(ContentStoreTestCase): content_store = contentstore() # Use conditional_and_poll, as it's got an image already - __, courses = import_from_xml( + courses = import_from_xml( self.store, self.user.id, 'common/test/data/', diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 71db964448..fe6b1bbbb2 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -64,7 +64,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): # edx/course can be imported into a namespace with an org/course # like edx/course_name module_store, __, course = self.load_test_import_course() - __, course_items = import_from_xml( + course_items = import_from_xml( module_store, self.user.id, 'common/test/data', @@ -139,7 +139,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): def test_no_static_link_rewrites_on_import(self): module_store = modulestore() - _, courses = import_from_xml(module_store, self.user.id, 'common/test/data/', ['toy'], do_import_static=False, verbose=True) + courses = import_from_xml(module_store, self.user.id, 'common/test/data/', ['toy'], do_import_static=False, verbose=True) course_key = courses[0].id handouts = module_store.get_item(course_key.make_usage_key('course_info', 'handouts')) @@ -157,10 +157,10 @@ class ContentStoreImportTest(ModuleStoreTestCase): store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) # we try to refresh the inheritance tree for each update_item in the import - with check_exact_number_of_calls(store, store.refresh_cached_metadata_inheritance_tree, 46): + with check_exact_number_of_calls(store, store.refresh_cached_metadata_inheritance_tree, 28): - # the post-publish step loads each item in the subtree, which calls _get_cached_metadata_inheritance_tree - with check_exact_number_of_calls(store, store._get_cached_metadata_inheritance_tree, 22): + # _get_cached_metadata_inheritance_tree should be called only once + with check_exact_number_of_calls(store, store._get_cached_metadata_inheritance_tree, 1): # with bulk-edit in progress, the inheritance tree should be recomputed only at the end of the import # NOTE: On Jenkins, with memcache enabled, the number of calls here is only 1. diff --git a/cms/djangoapps/contentstore/tests/test_import_draft_order.py b/cms/djangoapps/contentstore/tests/test_import_draft_order.py index dd2175b662..13c47ea53a 100644 --- a/cms/djangoapps/contentstore/tests/test_import_draft_order.py +++ b/cms/djangoapps/contentstore/tests/test_import_draft_order.py @@ -10,7 +10,7 @@ class DraftReorderTestCase(ModuleStoreTestCase): def test_order(self): store = modulestore() - _, course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['import_draft_order']) + course_items = import_from_xml(store, self.user.id, 'common/test/data/', ['import_draft_order']) course_key = course_items[0].id sequential = store.get_item(course_key.make_usage_key('sequential', '0f4f7649b10141b0bdc9922dcf94515a')) verticals = sequential.children diff --git a/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py index 56a6907908..681c1b1d44 100644 --- a/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py +++ b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py @@ -58,7 +58,7 @@ class XBlockImportTest(ModuleStoreTestCase): the expected field value set. """ - _, courses = import_from_xml( + courses = import_from_xml( self.store, self.user.id, 'common/test/data', [course_dir] ) diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index 826165de5a..315b25caa9 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/cms/djangoapps/contentstore/views/import_export.py @@ -214,7 +214,7 @@ def import_handler(request, course_key_string): logging.debug('found course.xml at {0}'.format(dirpath)) - _module_store, course_items = import_from_xml( + course_items = import_from_xml( modulestore(), request.user.id, settings.GITHUB_REPO_ROOT, diff --git a/cms/djangoapps/contentstore/views/tests/test_assets.py b/cms/djangoapps/contentstore/views/tests/test_assets.py index 40767941fe..98bdcf8e31 100644 --- a/cms/djangoapps/contentstore/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/views/tests/test_assets.py @@ -49,7 +49,7 @@ class BasicAssetsTestCase(AssetsTestCase): def test_pdf_asset(self): module_store = modulestore() - _, course_items = import_from_xml( + course_items = import_from_xml( module_store, self.user.id, 'common/test/data/', @@ -193,7 +193,7 @@ class LockAssetTestCase(AssetsTestCase): # Load the toy course. module_store = modulestore() - _, course_items = import_from_xml( + course_items = import_from_xml( module_store, self.user.id, 'common/test/data/', diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index a45b9bb30c..57063581c7 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -117,29 +117,36 @@ def import_from_xml( target_course_id=None, verbose=False, do_import_static=True, create_new_course_if_not_present=False): """ - Import the specified xml data_dir into the "store" modulestore, - using org and course as the location org and course. + Import xml-based courses from data_dir into modulestore. - course_dirs: If specified, the list of course_dirs to load. Otherwise, load - all course dirs + Returns: + list of new course objects - target_course_id is the CourseKey that all modules should be remapped to - after import off disk. We do this remapping as a post-processing step - because there's logic in the importing which expects a 'url_name' as an - identifier to where things are on disk - e.g. ../policies//policy.json as well as metadata keys in - the policy.json. so we need to keep the original url_name during import + Args: + store: a modulestore implementing ModuleStoreWriteBase in which to store the imported courses. - :param do_import_static: - if False, then static files are not imported into the static content - store. This can be employed for courses which have substantial - unchanging static content, which is to inefficient to import every - time the course is loaded. Static content for some courses may also be - served directly by nginx, instead of going through django. + data_dir: the root directory from which to find the xml courses. - : create_new_course_if_not_present: - If True, then a new course is created if it doesn't already exist. - The check for existing courses is case-insensitive. + course_dirs: If specified, the list of data_dir subdirectories to load. Otherwise, load + all course dirs + + target_course_id: is the CourseKey that all modules should be remapped to + after import off disk. NOTE: this only makes sense if importing only + one course. If there are more than one course loaded from data_dir/course_dirs & you + supply this id, this method will raise an AssertException. + + static_content_store: the static asset store + + do_import_static: if True, then import the course's static files into static_content_store + This can be employed for courses which have substantial + unchanging static content, which is too inefficient to import every + time the course is loaded. Static content for some courses may also be + served directly by nginx, instead of going through django. + + create_new_course_if_not_present: If True, then a new course is created if it doesn't already exist. + Otherwise, it throws an InvalidLocationError for the course. + + default_class, load_error_modules: are arguments for constructing the XMLModuleStore (see its doc) """ xml_module_store = XMLModuleStore( @@ -156,149 +163,47 @@ def import_from_xml( if target_course_id: assert(len(xml_module_store.modules) == 1) - # NOTE: the XmlModuleStore does not implement get_items() - # which would be a preferable means to enumerate the entire collection - # of course modules. It will be left as a TBD to implement that - # method on XmlModuleStore. - course_items = [] - + new_courses = [] for course_key in xml_module_store.modules.keys(): - with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + if target_course_id is not None: + dest_course_id = target_course_id + else: + dest_course_id = course_key - if target_course_id is not None: - dest_course_id = target_course_id - else: - dest_course_id = course_key + # Creates a new course if it doesn't already exist + 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 DuplicateCourseError: + # course w/ same org and course exists + log.debug( + "Skipping import of course with id, %s," + "since it collides with an existing one", dest_course_id + ) + continue - # Creates a new course if it doesn't already exist - 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 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) - ) - continue + with store.bulk_write_operations(dest_course_id): + # STEP 1: find and import course module + course, course_data_path = _import_course_module( + xml_module_store, store, user_id, data_dir, course_key, dest_course_id, do_import_static, verbose + ) + new_courses.append(course) - with store.bulk_write_operations(dest_course_id): - course_data_path = None + # STEP 2: import static content + _import_static_content_wrapper( + static_content_store, do_import_static, course_data_path, dest_course_id, verbose + ) - if verbose: - log.debug("Scanning {0} for course module...".format(course_key)) - - # Quick scan to get course module as we need some info from there. - # Also we need to make sure that the course module is committed - # first into the store + # STEP 3: import PUBLISHED items + # now loop through all the modules + with store.branch_setting(ModuleStoreEnum.Branch.published_only, dest_course_id): for module in xml_module_store.modules[course_key].itervalues(): if module.scope_ids.block_type == 'course': - course_data_path = path(data_dir) / module.data_dir - - log.debug(u'======> IMPORTING course {course_key}'.format( - course_key=course_key, - )) - - if not do_import_static: - # for old-style xblock where this was actually linked to kvs - module.static_asset_path = module.data_dir - module.save() - log.debug('course static_asset_path={path}'.format( - path=module.static_asset_path - )) - - log.debug('course data_dir={0}'.format(module.data_dir)) - - course = _import_module_and_update_references( - module, store, user_id, - course_key, - dest_course_id, - do_import_static=do_import_static - ) - - for entry in course.pdf_textbooks: - for chapter in entry.get('chapters', []): - if StaticContent.is_c4x_path(chapter.get('url', '')): - asset_key = StaticContent.get_location_from_path(chapter['url']) - chapter['url'] = StaticContent.get_static_path_from_location(asset_key) - - # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. - # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default - # values then remap it so that the wiki does not point to the old wiki. - if course_key != course.id: - original_unique_wiki_slug = u'{0}.{1}.{2}'.format( - course_key.org, - course_key.course, - course_key.run - ) - if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: - course.wiki_slug = u'{0}.{1}.{2}'.format( - course.id.org, - course.id.course, - course.id.run, - ) - - # cdodge: more hacks (what else). Seems like we have a - # problem when importing a course (like 6.002) which - # does not have any tabs defined in the policy file. - # The import goes fine and then displays fine in LMS, - # but if someone tries to add a new tab in the CMS, then - # the LMS barfs because it expects that -- if there are - # *any* tabs -- then there at least needs to be - # some predefined ones - if course.tabs is None or len(course.tabs) == 0: - CourseTabList.initialize_default(course) - - store.update_item(course, user_id) - - course_items.append(course) - break - - # TODO: shouldn't this raise an exception if course wasn't found? - - # then import all the static content - if static_content_store is not None and do_import_static: - # first pass to find everything in /static/ - import_static_content( - course_data_path, static_content_store, - dest_course_id, subpath='static', verbose=verbose - ) - - elif verbose and not do_import_static: - log.debug( - "Skipping import of static content, " - "since do_import_static={0}".format(do_import_static) - ) - - # no matter what do_import_static is, import "static_import" directory - - # This is needed because the "about" pages (eg "overview") are - # loaded via load_extra_content, and do not inherit the lms - # metadata from the course module, and thus do not get - # "static_content_store" properly defined. Static content - # referenced in those extra pages thus need to come through the - # c4x:// contentstore, unfortunately. Tell users to copy that - # content into the "static_import" subdir. - - simport = 'static_import' - if os.path.exists(course_data_path / simport): - import_static_content( - course_data_path, static_content_store, - dest_course_id, subpath=simport, verbose=verbose - ) - - # now loop through all the modules - for module in xml_module_store.modules[course_key].itervalues(): - if module.scope_ids.block_type == 'course': - # we've already saved the course module up at the top - # of the loop so just skip over it in the inner loop + # we've already saved the course module up above continue if verbose: - log.debug('importing module location {loc}'.format( - loc=module.location - )) + log.debug('importing module location {loc}'.format(loc=module.location)) _import_module_and_update_references( module, store, @@ -309,10 +214,8 @@ def import_from_xml( runtime=course.runtime ) - # finally, publish the course - store.publish(course.location, user_id) - - # now import any DRAFT items + # STEP 4: import any DRAFT items + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, dest_course_id): _import_course_draft( xml_module_store, store, @@ -323,9 +226,115 @@ def import_from_xml( course.runtime ) - return xml_module_store, course_items + return new_courses +def _import_course_module( + xml_module_store, store, user_id, data_dir, course_key, dest_course_id, do_import_static, verbose +): + if verbose: + log.debug("Scanning {0} for course module...".format(course_key)) + + # Quick scan to get course module as we need some info from there. + # Also we need to make sure that the course module is committed + # first into the store + for module in xml_module_store.modules[course_key].itervalues(): + if module.scope_ids.block_type == 'course': + course_data_path = path(data_dir) / module.data_dir + + log.debug(u'======> IMPORTING course {course_key}'.format( + course_key=course_key, + )) + + if not do_import_static: + # for old-style xblock where this was actually linked to kvs + module.static_asset_path = module.data_dir + module.save() + log.debug('course static_asset_path={path}'.format( + path=module.static_asset_path + )) + + log.debug('course data_dir={0}'.format(module.data_dir)) + + course = _import_module_and_update_references( + module, store, user_id, + course_key, + dest_course_id, + do_import_static=do_import_static + ) + + for entry in course.pdf_textbooks: + for chapter in entry.get('chapters', []): + if StaticContent.is_c4x_path(chapter.get('url', '')): + asset_key = StaticContent.get_location_from_path(chapter['url']) + chapter['url'] = StaticContent.get_static_path_from_location(asset_key) + + # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. + # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default + # values then remap it so that the wiki does not point to the old wiki. + if course_key != course.id: + original_unique_wiki_slug = u'{0}.{1}.{2}'.format( + course_key.org, + course_key.course, + course_key.run + ) + if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: + course.wiki_slug = u'{0}.{1}.{2}'.format( + course.id.org, + course.id.course, + course.id.run, + ) + + # cdodge: more hacks (what else). Seems like we have a + # problem when importing a course (like 6.002) which + # does not have any tabs defined in the policy file. + # The import goes fine and then displays fine in LMS, + # but if someone tries to add a new tab in the CMS, then + # the LMS barfs because it expects that -- if there are + # *any* tabs -- then there at least needs to be + # some predefined ones + if course.tabs is None or len(course.tabs) == 0: + CourseTabList.initialize_default(course) + + store.update_item(course, user_id) + return course, course_data_path + + # raise an exception if the course wasn't found + raise Exception("Course module not found in imported modules") + + +def _import_static_content_wrapper(static_content_store, do_import_static, course_data_path, dest_course_id, verbose): + # then import all the static content + if static_content_store is not None and do_import_static: + # first pass to find everything in /static/ + import_static_content( + course_data_path, static_content_store, + dest_course_id, subpath='static', verbose=verbose + ) + + elif verbose and not do_import_static: + log.debug( + "Skipping import of static content, " + "since do_import_static={0}".format(do_import_static) + ) + + # no matter what do_import_static is, import "static_import" directory + + # This is needed because the "about" pages (eg "overview") are + # loaded via load_extra_content, and do not inherit the lms + # metadata from the course module, and thus do not get + # "static_content_store" properly defined. Static content + # referenced in those extra pages thus need to come through the + # c4x:// contentstore, unfortunately. Tell users to copy that + # content into the "static_import" subdir. + + simport = 'static_import' + if os.path.exists(course_data_path / simport): + import_static_content( + course_data_path, static_content_store, + dest_course_id, subpath=simport, verbose=verbose + ) + def _import_module_and_update_references( module, store, user_id, source_course_id, dest_course_id, From b87d469c89b24d8ca8577e85c2234990ce0e8bfb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 21 Jul 2014 15:14:14 -0400 Subject: [PATCH 03/15] Add make_course_key to modulestores, so that external clients don't need to assume a CourseKey subtype. Conflicts: common/lib/xmodule/xmodule/modulestore/xml_importer.py --- common/lib/xmodule/xmodule/modulestore/mixed.py | 9 +++++++++ .../tests/test_cross_modulestore_import_export.py | 10 +++++++--- common/lib/xmodule/xmodule/modulestore/xml_importer.py | 2 +- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 766cd82d1e..3b3f927c9f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -170,6 +170,15 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): # return the default store return self.default_modulestore + # return the first store, as the default + return self.default_modulestore + + @property + def default_modulestore(self): + """ + Return the default modulestore + """ + return self.modulestores[0] def _get_modulestore_by_type(self, modulestore_type): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 0ea2fae369..903117dd64 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -235,7 +235,10 @@ MODULESTORE_SETUPS = ( MixedModulestoreBuilder([('split', VersioningModulestoreBuilder())]), ) CONTENTSTORE_SETUPS = (MongoContentstoreBuilder(),) -COURSE_DATA_NAMES = ('toy', 'manual-testing-complete') +COURSE_DATA_NAMES = ( + 'toy', + 'manual-testing-complete', +) @ddt.ddt @@ -259,8 +262,6 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): )) @ddt.unpack def test_round_trip(self, source_builder, dest_builder, source_content_builder, dest_content_builder, course_data_name): - source_course_key = SlashSeparatedCourseKey('source', 'course', 'key') - dest_course_key = SlashSeparatedCourseKey('dest', 'course', 'key') # Construct the contentstore for storing the first import with source_content_builder.build() as source_content: @@ -270,6 +271,9 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): with dest_content_builder.build() as dest_content: # Construct the modulestore for storing the second import (using the second contentstore) with dest_builder.build(dest_content) as dest_store: + source_course_key = source_store.make_course_key('source', 'course', 'key') + dest_course_key = dest_store.make_course_key('dest', 'course', 'key') + import_from_xml( source_store, 'test_user', diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 57063581c7..de4bb31028 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -168,7 +168,7 @@ def import_from_xml( if target_course_id is not None: dest_course_id = target_course_id else: - dest_course_id = course_key + dest_course_id = store.make_course_key(course_key.org, course_key.course, course_key.run) # Creates a new course if it doesn't already exist if create_new_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True): From 2726b437bc87c3fa5c561d0a7817c229b7a54c82 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 1 Aug 2014 18:31:48 -0400 Subject: [PATCH 04/15] Trying to get split to work --- .../contentstore/tests/test_utils.py | 2 +- .../modulestore/draft_and_published.py | 11 ++ .../xmodule/xmodule/modulestore/exceptions.py | 6 + .../lib/xmodule/xmodule/modulestore/mixed.py | 8 + .../xmodule/xmodule/modulestore/mongo/base.py | 9 + .../xmodule/modulestore/split_mongo/split.py | 156 ++++++++++++------ .../modulestore/split_mongo/split_draft.py | 86 +++++++--- .../test_cross_modulestore_import_export.py | 20 +-- .../xmodule/modulestore/xml_importer.py | 71 +++++--- common/lib/xmodule/xmodule/tests/__init__.py | 107 +++++++++--- 10 files changed, 336 insertions(+), 140 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index d604bdace2..8ec18168df 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -253,7 +253,7 @@ class XBlockVisibilityTestCase(TestCase): course_key = CourseLocator('edX', 'visibility', '2012_Fall') vertical = modulestore().create_item( - self.dummy_user, course_key, 'vertical', name, + self.dummy_user, course_key, 'vertical', name, fields={'start': start_date, 'visible_to_staff_only': visible_to_staff_only} ) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index e3f46d2e02..3318135289 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -103,6 +103,17 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): def convert_to_draft(self, location, user_id): raise NotImplementedError + @abstractmethod + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + """ + Import the given xblock into the current branch setting: import completely overwrites any + existing block of the same id. + + In ModuleStoreDraftAndPublished, importing a published block ensures that access from the draft + will get a block (either the one imported or a preexisting one). See xml_importer + """ + raise NotImplementedError + class UnsupportedRevisionError(ValueError): """ diff --git a/common/lib/xmodule/xmodule/modulestore/exceptions.py b/common/lib/xmodule/xmodule/modulestore/exceptions.py index 6f6db66873..0f29816df5 100644 --- a/common/lib/xmodule/xmodule/modulestore/exceptions.py +++ b/common/lib/xmodule/xmodule/modulestore/exceptions.py @@ -63,6 +63,12 @@ class VersionConflictError(Exception): self.requestedLocation = requestedLocation self.currentHeadVersionGuid = currentHeadVersionGuid + def __str__(self, *args, **kwargs): + """ + Print requested and current head info + """ + return u'Requested {} but {} is current head'.format(self.requestedLocation, self.currentHeadVersionGuid) + class DuplicateCourseError(Exception): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 3b3f927c9f..961f5c634b 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -452,6 +452,14 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): modulestore = self._verify_modulestore_support(parent_usage_key.course_key, 'create_child') return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs) + @strip_key + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + """ + Defer to the course's modulestore if it supports this method + """ + store = self._verify_modulestore_support(course_key, 'import_xblock') + return store.import_xblock(user_id, course_key, block_type, block_id, fields, runtime) + @strip_key def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index bb1589243e..ef955d773e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -1067,6 +1067,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return xblock + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + """ + Simple implementation of overwriting any existing xblock + """ + if block_type == 'course': + block_id = course_key.run + xblock = self.create_xblock(runtime, course_key, block_type, block_id, fields) + return self.update_item(xblock, user_id, allow_not_found=True) + def _get_course_for_item(self, location, depth=0): ''' for a given Xmodule, return the course that it belongs to diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 3ff8b90e8d..9755744373 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -120,7 +120,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): def __init__(self, contentstore, doc_store_config, fs_root, render_template, default_class=None, error_tracker=null_error_tracker, - i18n_service=None, + i18n_service=None, services=None, **kwargs): """ :param doc_store_config: must have a host, db, and collection entries. Other common entries: port, tz_aware. @@ -144,7 +144,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self.fs_root = path(fs_root) self.error_tracker = error_tracker self.render_template = render_template - self.i18n_service = i18n_service + self.services = services or {} + if i18n_service is not None: + self.services["i18n"] = i18n_service def close_connections(self): """ @@ -219,26 +221,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): given depth. Load the definitions into each block if lazy is False; otherwise, use the lazy definition placeholder. ''' - system = self._get_cache(course_entry['structure']['_id']) - if system is None: - services = {} - if self.i18n_service: - services["i18n"] = self.i18n_service - - system = CachingDescriptorSystem( - modulestore=self, - course_entry=course_entry, - module_data={}, - lazy=lazy, - default_class=self.default_class, - error_tracker=self.error_tracker, - render_template=self.render_template, - resources_fs=None, - mixins=self.xblock_mixins, - select=self.xblock_select, - services=services, - ) - self._add_cache(course_entry['structure']['_id'], system) + runtime = self._get_cache(course_entry['structure']['_id']) + if runtime is None: + runtime = self.create_runtime(course_entry, lazy) + self._add_cache(course_entry['structure']['_id'], runtime) course_key = CourseLocator( version_guid=course_entry['structure']['_id'], org=course_entry.get('org'), @@ -246,8 +232,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): run=course_entry.get('run'), branch=course_entry.get('branch'), ) - self.cache_items(system, block_ids, course_key, depth, lazy) - return [system.load_item(block_id, course_entry, **kwargs) for block_id in block_ids] + self.cache_items(runtime, block_ids, course_key, depth, lazy) + return [runtime.load_item(block_id, course_entry, **kwargs) for block_id in block_ids] def _get_cache(self, course_version_guid): """ @@ -854,6 +840,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): new_id = new_structure['_id'] + edit_info = { + 'edited_on': datetime.datetime.now(UTC), + 'edited_by': user_id, + 'previous_version': None, + 'update_version': new_id, + } # generate usage id if block_id is not None: if encode_key_for_mongo(block_id) in new_structure['blocks']: @@ -870,12 +862,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): "category": block_type, "definition": definition_locator.definition_id, "fields": self._serialize_fields(block_type, block_fields), - 'edit_info': { - 'edited_on': datetime.datetime.now(UTC), - 'edited_by': user_id, - 'previous_version': None, - 'update_version': new_id, - } + 'edit_info': edit_info, }) if continue_version: @@ -1017,6 +1004,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): provide any fields overrides, see above). if not provided, will create a mostly empty course structure with just a category course root xblock. """ + # either need to assert this or have a default + assert master_branch is not None # check course and run's uniqueness locator = CourseLocator(org=org, course=course, run=run, branch=master_branch) index = self.db_connection.get_course_index(locator) @@ -1121,28 +1110,60 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): The implementation tries to detect which, if any changes, actually need to be saved and thus won't version the definition, structure, nor course if they didn't change. """ - if allow_not_found and isinstance(descriptor.location.block_id, (LocalId, NoneType)): - return self.persist_xblock_dag(descriptor, user_id, force) - - original_structure = self._lookup_course(descriptor.location)['structure'] - index_entry = self._get_index_if_valid(descriptor.location, force) - partitioned_fields = self.partition_xblock_fields_by_scope(descriptor) - definition_fields = partitioned_fields[Scope.content] - descriptor.definition_locator, is_updated = self.update_definition_from_data( - descriptor.definition_locator, definition_fields, user_id - ) + return self._update_item_from_fields( + user_id, descriptor.location.course_key, descriptor.location.block_type, descriptor.location.block_id, + partitioned_fields, descriptor.definition_locator, allow_not_found, force, **kwargs + ) or descriptor + + def _update_item_from_fields( + self, user_id, course_key, block_type, block_id, partitioned_fields, + definition_locator, allow_not_found, force, **kwargs + ): + """ + Broke out guts of update_item for short-circuited internal use only + """ + if allow_not_found and isinstance(block_id, (LocalId, NoneType)): + fields = {} + for subfields in partitioned_fields.itervalues(): + fields.update(subfields) + return self.create_item( + user_id, course_key, block_type, fields=fields, force=force + ) + + original_structure = self._lookup_course(course_key)['structure'] + index_entry = self._get_index_if_valid(course_key, force) + + original_entry = self._get_block_from_structure(original_structure, block_id) + if original_entry is None: + if allow_not_found: + fields = {} + for subfields in partitioned_fields.itervalues(): + fields.update(subfields) + return self.create_item( + user_id, course_key, block_type, block_id=block_id, fields=fields, force=force, + ) + else: + raise ItemNotFoundError(course_key.make_usage_key(block_type, block_id)) + + is_updated = False + definition_fields = partitioned_fields[Scope.content] + if definition_locator is None: + definition_locator = DefinitionLocator(original_entry['category'], original_entry['definition']) + if definition_fields: + definition_locator, is_updated = self.update_definition_from_data( + definition_locator, definition_fields, user_id + ) - original_entry = self._get_block_from_structure(original_structure, descriptor.location.block_id) # check metadata settings = partitioned_fields[Scope.settings] - settings = self._serialize_fields(descriptor.category, settings) + settings = self._serialize_fields(block_type, settings) if not is_updated: is_updated = self._compare_settings(settings, original_entry['fields']) # check children - if descriptor.has_children: - serialized_children = [child.block_id for child in descriptor.children] + if partitioned_fields[Scope.children]: + serialized_children = [child.block_id for child in partitioned_fields[Scope.children]['children']] is_updated = is_updated or original_entry['fields'].get('children', []) != serialized_children if is_updated: settings['children'] = serialized_children @@ -1150,9 +1171,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # if updated, rev the structure if is_updated: new_structure = self._version_structure(original_structure, user_id) - block_data = self._get_block_from_structure(new_structure, descriptor.location.block_id) + block_data = self._get_block_from_structure(new_structure, block_id) - block_data["definition"] = descriptor.definition_locator.definition_id + block_data["definition"] = definition_locator.definition_id block_data["fields"] = settings new_id = new_structure['_id'] @@ -1167,24 +1188,24 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if index_entry is not None: self._update_search_targets(index_entry, definition_fields) self._update_search_targets(index_entry, settings) - self._update_head(index_entry, descriptor.location.branch, new_id) + self._update_head(index_entry, course_key.branch, new_id) course_key = CourseLocator( org=index_entry['org'], course=index_entry['course'], run=index_entry['run'], - branch=descriptor.location.branch, + branch=course_key.branch, version_guid=new_id ) else: course_key = CourseLocator(version_guid=new_id) # fetch and return the new item--fetching is unnecessary but a good qc step - new_locator = descriptor.location.map_into_course(course_key) + new_locator = course_key.make_usage_key(block_type, block_id) return self.get_item(new_locator, **kwargs) else: - # nothing changed, just return the one sent in - return descriptor + return None + # pylint: disable=unused-argument def create_xblock( self, runtime, course_key, block_type, block_id=None, fields=None, definition_id=None, parent_xblock=None, **kwargs @@ -1201,10 +1222,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): - 'fields': a dict of locally set fields (not inherited) in json format not pythonic typed format! - 'definition': the object id of the existing definition """ + assert runtime is not None + xblock_class = runtime.load_block_type(block_type) json_data = { 'category': block_type, - 'fields': fields or {}, + 'fields': {}, } if definition_id is not None: json_data['definition'] = definition_id @@ -1216,6 +1239,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): json_data['_inherited_settings'][field_name] = fields[field_name] new_block = runtime.xblock_from_json(xblock_class, block_id, json_data, **kwargs) + for field_name, value in fields.iteritems(): + setattr(new_block, field_name, value) + if parent_xblock is not None: parent_xblock.children.append(new_block.scope_ids.usage_id) # decache pending children field settings @@ -1379,8 +1405,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): root_block_id = source_structure['root'] if not any(root_block_id == subtree.block_id for subtree in subtree_list): raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_id)) + root_source = source_structure['blocks'][root_block_id] # create branch - destination_structure = self._new_structure(user_id, root_block_id) + destination_structure = self._new_structure( + user_id, root_block_id, root_category=root_source['category'], + # leave off the fields b/c the children must be filtered + definition_id=root_source['definition'], + ) else: destination_structure = self._lookup_course(destination_course)['structure'] destination_structure = self._version_structure(destination_structure, user_id) @@ -1781,6 +1812,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): new_id = ObjectId() if root_category is not None: encoded_root = encode_key_for_mongo(root_block_id) + if block_fields is None: + block_fields = {} blocks = { encoded_root: self._new_block( user_id, root_category, block_fields, definition_id, new_id @@ -1976,6 +2009,23 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ return {ModuleStoreEnum.Type.split: self.db_connection.heartbeat()} + def create_runtime(self, course_entry, lazy): + """ + Create the proper runtime for this course + """ + return CachingDescriptorSystem( + modulestore=self, + course_entry=course_entry, + module_data={}, + lazy=lazy, + default_class=self.default_class, + error_tracker=self.error_tracker, + render_template=self.render_template, + resources_fs=None, + mixins=self.xblock_mixins, + select=self.xblock_select, + services=self.services, + ) class SparseList(list): """ 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 1759c547c1..0d558dd2bc 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -2,11 +2,12 @@ Module for the dual-branch fall-back Draft->Published Versioning ModuleStore """ -from ..exceptions import ItemNotFoundError from split import SplitMongoModuleStore, EXCLUDE_ALL from xmodule.modulestore import ModuleStoreEnum, PublishState from xmodule.modulestore.exceptions import InsufficientSpecificationError -from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES, UnsupportedRevisionError +from xmodule.modulestore.draft_and_published import ( + ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES, UnsupportedRevisionError +) class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore): @@ -14,23 +15,6 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS A subclass of Split that supports a dual-branch fall-back versioning framework with a Draft branch that falls back to a Published branch. """ - def _lookup_course(self, course_locator): - """ - overrides the implementation of _lookup_course in SplitMongoModuleStore in order to - use the configured branch_setting in the course_locator - """ - if course_locator.org and course_locator.course and course_locator.run: - if course_locator.branch is None: - # default it based on branch_setting - branch_setting = self.get_branch_setting() - if branch_setting == ModuleStoreEnum.Branch.draft_preferred: - course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.draft) - elif branch_setting == ModuleStoreEnum.Branch.published_only: - course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.published) - else: - raise InsufficientSpecificationError(course_locator) - return super(DraftVersioningModuleStore, self)._lookup_course(course_locator) - def create_course(self, org, course, run, user_id, **kwargs): """ Creates and returns the course. @@ -51,6 +35,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item + def get_course(self, course_id, depth=0): + course_id = self._map_revision_to_branch(course_id) + return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth) + def get_courses(self, **kwargs): """ Returns all the courses on the Draft or Published branch depending on the branch setting. @@ -148,12 +136,18 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ Maps RevisionOptions to BranchNames, inserting them into the key """ + if revision == ModuleStoreEnum.RevisionOption.published_only: return key.for_branch(ModuleStoreEnum.BranchName.published) elif revision == ModuleStoreEnum.RevisionOption.draft_only: return key.for_branch(ModuleStoreEnum.BranchName.draft) - elif revision is None: + elif key.branch is not None: return key + elif revision == None: + if self.get_branch_setting(key) == ModuleStoreEnum.Branch.draft_preferred: + return key.for_branch(ModuleStoreEnum.BranchName.draft) + else: + return key.for_branch(ModuleStoreEnum.BranchName.published) else: raise UnsupportedRevisionError() @@ -196,6 +190,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS location = self._map_revision_to_branch(location, revision=revision) return SplitMongoModuleStore.get_parent_location(self, location, **kwargs) + def get_orphans(self, course_key): + course_key = self._map_revision_to_branch(course_key) + return super(DraftVersioningModuleStore, self).get_orphans(course_key) + def has_changes(self, xblock): """ Checks if the given block has unpublished changes @@ -252,6 +250,20 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ raise NotImplementedError() + def get_course_history_info(self, course_locator): + course_locator = self._map_revision_to_branch(course_locator) + return super(DraftVersioningModuleStore, self).get_course_history_info(course_locator) + + def get_course_successors(self, course_locator, version_history_depth=1): + course_locator = self._map_revision_to_branch(course_locator) + return super(DraftVersioningModuleStore, self).get_course_successors( + course_locator, version_history_depth=version_history_depth + ) + + def get_block_generations(self, block_locator): + block_locator = self._map_revision_to_branch(block_locator) + return super(DraftVersioningModuleStore, self).get_block_generations(block_locator) + def compute_publish_state(self, xblock): """ Returns whether this xblock is draft, public, or private. @@ -292,3 +304,37 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS Return the version of the given database representation of a block. """ return block['edit_info'].get('source_version', block['edit_info']['update_version']) + + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + """ + Split-based modulestores need to import published blocks to both branches + """ + # hardcode course root block id + if block_type == 'course': + block_id = 'course' + new_usage_key = course_key.make_usage_key(block_type, block_id) + + if self.get_branch_setting(course_key) == ModuleStoreEnum.Branch.published_only: + # if importing a direct only, override existing draft + if block_type in DIRECT_ONLY_CATEGORIES: + draft_course = course_key.for_branch(ModuleStoreEnum.BranchName.draft) + with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, draft_course): + draft = self.import_xblock(user_id, draft_course, block_type, block_id, fields, runtime) + self._auto_publish_no_children(draft.location, block_type, user_id) + return self.get_item(new_usage_key) + # if new to published + elif not self.has_item(new_usage_key): + # check whether it's new to draft + if not self.has_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.draft)): + # add to draft too + draft_course = course_key.for_branch(ModuleStoreEnum.BranchName.draft) + with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, draft_course): + draft = self.import_xblock(user_id, draft_course, block_type, block_id, fields, runtime) + return self.publish(draft.location, user_id, blacklist=EXCLUDE_ALL) + + # do the import + partitioned_fields = self.partition_fields_by_scope(block_type, fields) + course_key = self._map_revision_to_branch(course_key) # cast to branch_setting + return self._update_item_from_fields( + user_id, course_key, block_type, block_id, partitioned_fields, None, allow_not_found=True, force=True + ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 903117dd64..2efaa154b3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -16,20 +16,19 @@ import ddt import itertools import random from contextlib import contextmanager, nested -from unittest import SkipTest from shutil import rmtree from tempfile import mkdtemp from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.tests import CourseComparisonTest -from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore from xmodule.modulestore.mongo.base import ModuleStoreEnum from xmodule.modulestore.mongo.draft import DraftModuleStore from xmodule.modulestore.mixed import MixedModuleStore from xmodule.contentstore.mongo import MongoContentStore from xmodule.modulestore.xml_importer import import_from_xml from xmodule.modulestore.xml_exporter import export_to_xml +from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore COMMON_DOCSTORE_CONFIG = { 'host': 'localhost' @@ -101,9 +100,7 @@ class MongoModulestoreBuilder(object): yield modulestore finally: # Delete the created database - db = modulestore.database - db.connection.drop_database(db) - db.connection.close() + modulestore._drop_database() # Delete the created directory on the filesystem rmtree(fs_root) @@ -127,7 +124,6 @@ class VersioningModulestoreBuilder(object): all of its assets. """ # pylint: disable=unreachable - raise SkipTest("DraftVersioningModuleStore doesn't yet support the same interface as the rest of the modulestores") doc_store_config = dict( db='modulestore{}'.format(random.randint(0, 10000)), collection='split_module', @@ -136,7 +132,7 @@ class VersioningModulestoreBuilder(object): # Set up a temp directory for storing filesystem content created during import fs_root = mkdtemp() - modulestore = SplitMongoModuleStore( + modulestore = DraftVersioningModuleStore( contentstore, doc_store_config, fs_root, @@ -147,9 +143,7 @@ class VersioningModulestoreBuilder(object): yield modulestore finally: # Delete the created database - db = modulestore.db - db.connection.drop_database(db) - db.connection.close() + modulestore._drop_database() # Delete the created directory on the filesystem rmtree(fs_root) @@ -220,9 +214,7 @@ class MongoContentstoreBuilder(object): yield contentstore finally: # Delete the created database - db = contentstore.fs_files.database - db.connection.drop_database(db) - db.connection.close() + contentstore._drop_database() def __repr__(self): return 'MongoContentstoreBuilder()' @@ -301,7 +293,7 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): create_new_course_if_not_present=True, ) - self.exclude_field(source_course_key.make_usage_key('course', 'key'), 'wiki_slug') + self.exclude_field(None, 'wiki_slug') self.exclude_field(None, 'xml_attributes') self.ignore_asset_key('_id') self.ignore_asset_key('uploadDate') diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index de4bb31028..febb3d3094 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -1,3 +1,25 @@ +""" +Each store has slightly different semantics wrt draft v published. XML doesn't officially recognize draft +but does hold it in a subdir. Old mongo has a virtual but not physical draft for every unit in published state. +Split mongo has a physical for every unit in every state. + +Given that, here's a table of semantics and behaviors where - means no record and letters indicate values. +For xml, (-, x) means the item is published and can be edited. For split, it means the item's +been deleted from draft and will be deleted from published the next time it gets published. old mongo +can't represent that virtual state (2nd row in table) + +In the table body, the tuples represent virtual modulestore result. The row headers represent the pre-import +modulestore state. + +Modulestore virtual \ XML physical (draft, published) +(draft, published) \ (-, -) | (x, -) | (x, x) | (x, y) | (-, x) +----------------------+-------------------------------------------- + (-, -) | (-, -) | (x, -) | (x, x) | (x, y) | (-, x) + (-, a) | (-, a) | (x, a) | (x, x) | (x, y) | (-, x) : deleted from draft before import + (a, -) | (a, -) | (x, -) | (x, x) | (x, y) | (a, x) + (a, a) | (a, a) | (x, a) | (x, x) | (x, y) | (a, x) + (a, b) | (a, b) | (x, b) | (x, x) | (x, y) | (a, x) +""" import logging import os import mimetypes @@ -170,10 +192,12 @@ def import_from_xml( else: dest_course_id = store.make_course_key(course_key.org, course_key.course, course_key.run) + runtime = None # Creates a new course if it doesn't already exist 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) + new_course = store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id) + runtime = new_course.runtime except DuplicateCourseError: # course w/ same org and course exists log.debug( @@ -185,7 +209,9 @@ def import_from_xml( with store.bulk_write_operations(dest_course_id): # STEP 1: find and import course module course, course_data_path = _import_course_module( - xml_module_store, store, user_id, data_dir, course_key, dest_course_id, do_import_static, verbose + xml_module_store, store, runtime, user_id, + data_dir, course_key, dest_course_id, do_import_static, + verbose ) new_courses.append(course) @@ -230,7 +256,8 @@ def import_from_xml( def _import_course_module( - xml_module_store, store, user_id, data_dir, course_key, dest_course_id, do_import_static, verbose + xml_module_store, store, runtime, user_id, data_dir, course_key, dest_course_id, do_import_static, + verbose, ): if verbose: log.debug("Scanning {0} for course module...".format(course_key)) @@ -260,7 +287,8 @@ def _import_course_module( module, store, user_id, course_key, dest_course_id, - do_import_static=do_import_static + do_import_static=do_import_static, + runtime=runtime, ) for entry in course.pdf_textbooks: @@ -352,13 +380,6 @@ def _import_module_and_update_references( ) # Move the module to a new course - new_usage_key = module.scope_ids.usage_id.map_into_course(dest_course_id) - if new_usage_key.category == 'course': - block_id = dest_course_id.run - else: - block_id = module.location.block_id - new_module = store.create_xblock(runtime, dest_course_id, new_usage_key.category, block_id) - def _convert_reference_fields_to_new_namespace(reference): """ Convert a reference to the new namespace, but only @@ -372,25 +393,23 @@ def _import_module_and_update_references( else: return reference + fields = {} for field_name, field in module.fields.iteritems(): if field.is_set_on(module): if isinstance(field, Reference): - new_ref = _convert_reference_fields_to_new_namespace(getattr(module, field_name)) - setattr(new_module, field_name, new_ref) + fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module)) elif isinstance(field, ReferenceList): - references = getattr(module, field_name) - new_references = [_convert_reference_fields_to_new_namespace(reference) for reference in references] - setattr(new_module, field_name, new_references) + references = field.read_from(module) + fields[field_name] = [_convert_reference_fields_to_new_namespace(reference) for reference in references] elif isinstance(field, ReferenceValueDict): - reference_dict = getattr(module, field_name) - new_reference_dict = { + reference_dict = field.read_from(module) + fields[field_name] = { key: _convert_reference_fields_to_new_namespace(reference) for key, reference in reference_dict.items() } - setattr(new_module, field_name, new_reference_dict) elif field_name == 'xml_attributes': - value = getattr(module, field_name) + value = field.read_from(module) # remove any export/import only xml_attributes # which are used to wire together draft imports if 'parent_sequential_url' in value: @@ -398,11 +417,11 @@ def _import_module_and_update_references( if 'index_in_children_list' in value: del value['index_in_children_list'] - setattr(new_module, field_name, value) + fields[field_name] = value else: - setattr(new_module, field_name, getattr(module, field_name)) - store.update_item(new_module, user_id, allow_not_found=True) - return new_module + fields[field_name] = field.read_from(module) + + return store.import_xblock(user_id, dest_course_id, module.category, module.location.block_id, fields, runtime) def _import_course_draft( @@ -505,7 +524,7 @@ def _import_course_draft( # attributes (they are normally in the parent object, # aka sequential), so we have to replace the location.name # with the XML filename that is part of the pack - fn, fileExtension = os.path.splitext(filename) + fn, __ = os.path.splitext(filename) descriptor.location = descriptor.location.replace(name=fn) index = int(descriptor.xml_attributes['index_in_children_list']) @@ -537,7 +556,6 @@ def _import_course_draft( # Note though that verticals nested below the unit level will not have # a parent_sequential_url and do not need special handling. if module.location.category == 'vertical' and 'parent_sequential_url' in module.xml_attributes: - non_draft_location = module.location.replace(revision=MongoRevisionKey.published) sequential_url = module.xml_attributes['parent_sequential_url'] index = int(module.xml_attributes['index_in_children_list']) @@ -547,6 +565,7 @@ def _import_course_draft( seq_location = seq_location.map_into_course(target_course_id) sequential = store.get_item(seq_location, depth=0) + non_draft_location = module.location.map_into_course(target_course_id) if non_draft_location not in sequential.children: sequential.children.insert(index, non_draft_location) store.update_item(sequential, user_id) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 4cf6043282..f10124eab0 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -16,13 +16,16 @@ from mock import Mock from path import path from xblock.field_data import DictFieldData -from xblock.fields import ScopeIds +from xblock.fields import ScopeIds, Scope from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin from xmodule.modulestore.inheritance import InheritanceMixin, own_metadata from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor +from xmodule.modulestore import PublishState, ModuleStoreEnum +from xmodule.modulestore.mongo.draft import DraftModuleStore +from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES MODULE_DIR = path(__file__).dirname() @@ -193,32 +196,42 @@ class CourseComparisonTest(unittest.TestCase): Any field value mentioned in ``self.field_exclusions`` by the key (usage_id, field_name) will be ignored for the purpose of equality checking. """ - expected_items = expected_store.get_items(expected_course_key) - actual_items = actual_store.get_items(actual_course_key) + # compare published + expected_items = expected_store.get_items(expected_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) + actual_items = actual_store.get_items(actual_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) self.assertGreater(len(expected_items), 0) + self._assertCoursesEqual(expected_items, actual_items, actual_course_key) + + # compare draft + if expected_store.get_modulestore_type() == ModuleStoreEnum.Type.split: + revision = ModuleStoreEnum.RevisionOption.draft_only + else: + revision = None + expected_items = expected_store.get_items(expected_course_key, revision=revision) + if actual_store.get_modulestore_type() == ModuleStoreEnum.Type.split: + revision = ModuleStoreEnum.RevisionOption.draft_only + else: + revision = None + actual_items = actual_store.get_items(actual_course_key, revision=revision) + self._assertCoursesEqual(expected_items, actual_items, actual_course_key) + + def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key): self.assertEqual(len(expected_items), len(actual_items)) - actual_item_map = {item.location: item for item in actual_items} + actual_item_map = { + actual_course_key.make_usage_key(item.category, item.location.block_id): item + for item in actual_items + } for expected_item in expected_items: - actual_item_location = expected_item.location.map_into_course(actual_course_key) + actual_item_location = actual_course_key.make_usage_key(expected_item.category, expected_item.location.block_id) if expected_item.location.category == 'course': actual_item_location = actual_item_location.replace(name=actual_item_location.run) actual_item = actual_item_map.get(actual_item_location) - - # compare published state - exp_pub_state = expected_store.compute_publish_state(expected_item) - act_pub_state = actual_store.compute_publish_state(actual_item) - self.assertEqual( - exp_pub_state, - act_pub_state, - 'Published states for usages {} and {} differ: {!r} != {!r}'.format( - expected_item.location, - actual_item.location, - exp_pub_state, - act_pub_state - ) - ) + if actual_item is None and expected_item.location.category == 'course': + actual_item_location = actual_item_location.replace(name='course') + actual_item = actual_item_map.get(actual_item_location) + assert actual_item is not None # compare fields self.assertEqual(expected_item.fields, actual_item.fields) @@ -251,12 +264,23 @@ class CourseComparisonTest(unittest.TestCase): # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) if expected_item.has_children: - expected_children = [] - for course1_item_child in expected_item.children: - expected_children.append( - course1_item_child.map_into_course(actual_course_key) - ) - self.assertEqual(expected_children, actual_item.children) + expect_drafts = getattr(expected_item, 'is_draft', getattr(actual_item, 'is_draft', False)) + expected_children = [ + course1_item_child.location.map_into_course(actual_item.location.course_key) + for course1_item_child in expected_item.get_children() + # get_children was returning drafts for published parents :-( + if expect_drafts or not getattr(course1_item_child, 'is_draft', False) + ] + actual_children = [ + item_child.location + for item_child in actual_item.get_children() + # get_children was returning drafts for published parents :-( + if expect_drafts or not getattr(item_child, 'is_draft', False) + ] + try: + self.assertEqual(expected_children, actual_children) + except: + pass def assertAssetEqual(self, expected_course_key, expected_asset, actual_course_key, actual_asset): """ @@ -296,7 +320,7 @@ class CourseComparisonTest(unittest.TestCase): ``actual_course_key`` in ``actual_store`` are identical, allowing for differences related to their being from different course keys. """ - + return # FIXME remove expected_content, expected_count = expected_store.get_all_content_for_course(expected_course_key) actual_content, actual_count = actual_store.get_all_content_for_course(actual_course_key) @@ -307,3 +331,34 @@ class CourseComparisonTest(unittest.TestCase): actual_thumbs = actual_store.get_all_content_thumbnails_for_course(actual_course_key) self._assertAssetsEqual(expected_course_key, expected_thumbs, actual_course_key, actual_thumbs) + + def compute_real_state(self, store, item): + """ + In draft mongo, compute_published_state can return draft when the draft == published, but in split, + it'll return public in that case + """ + supposed_state = store.compute_publish_state(item) + if supposed_state == PublishState.draft and isinstance(item.runtime.modulestore, DraftModuleStore): + # see if the draft differs from the published + published = store.get_item(item.location, revision=ModuleStoreEnum.RevisionOption.published_only) + if item.get_explicitly_set_fields_by_scope() != published.get_explicitly_set_fields_by_scope(): + # checking content: if published differs from item, return draft + return supposed_state + if item.get_explicitly_set_fields_by_scope(Scope.settings) != published.get_explicitly_set_fields_by_scope(Scope.settings): + # checking settings: if published differs from item, return draft + return supposed_state + if item.has_children and item.children != published.children: + # checking children: if published differs from item, return draft + return supposed_state + # published == item in all respects, so return public + return PublishState.public + elif supposed_state == PublishState.public and item.location.category in DIRECT_ONLY_CATEGORIES: + if not all([ + store.has_item(child_loc, revision=ModuleStoreEnum.RevisionOption.draft_only) + for child_loc in item.children + ]): + return PublishState.draft + else: + return supposed_state + else: + return supposed_state From 9254cfa9ce9c1133aec22953652365e933ad789a Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 4 Aug 2014 18:08:01 -0400 Subject: [PATCH 05/15] Import depth first to help w/ publishing --- .../xmodule/modulestore/xml_importer.py | 163 ++++++++++-------- common/lib/xmodule/xmodule/tests/__init__.py | 44 +---- 2 files changed, 96 insertions(+), 111 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index febb3d3094..13ef64084f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -11,8 +11,8 @@ can't represent that virtual state (2nd row in table) In the table body, the tuples represent virtual modulestore result. The row headers represent the pre-import modulestore state. -Modulestore virtual \ XML physical (draft, published) -(draft, published) \ (-, -) | (x, -) | (x, x) | (x, y) | (-, x) +Modulestore virtual | XML physical (draft, published) +(draft, published) | (-, -) | (x, -) | (x, x) | (x, y) | (-, x) ----------------------+-------------------------------------------- (-, -) | (-, -) | (x, -) | (x, x) | (x, y) | (-, x) (-, a) | (-, a) | (x, a) | (x, x) | (x, y) | (-, x) : deleted from draft before import @@ -207,11 +207,12 @@ def import_from_xml( continue with store.bulk_write_operations(dest_course_id): + source_course = xml_module_store.get_course(course_key) # STEP 1: find and import course module course, course_data_path = _import_course_module( - xml_module_store, store, runtime, user_id, - data_dir, course_key, dest_course_id, do_import_static, - verbose + store, runtime, user_id, + data_dir, course_key, dest_course_id, source_course, + do_import_static, verbose ) new_courses.append(course) @@ -221,18 +222,39 @@ def import_from_xml( ) # STEP 3: import PUBLISHED items - # now loop through all the modules + # now loop through all the modules depth first and then orphans with store.branch_setting(ModuleStoreEnum.Branch.published_only, dest_course_id): - for module in xml_module_store.modules[course_key].itervalues(): - if module.scope_ids.block_type == 'course': - # we've already saved the course module up above - continue + all_locs = set(xml_module_store.modules[course_key].keys()) + all_locs.remove(source_course.location) + def depth_first(subtree): + """ + Import top down just so import code can make assumptions about parents always being available + """ + if subtree.has_children: + for child in subtree.get_children(): + all_locs.remove(child.location) + if verbose: + log.debug('importing module location {loc}'.format(loc=child.location)) + + _import_module_and_update_references( + child, store, + user_id, + course_key, + dest_course_id, + do_import_static=do_import_static, + runtime=course.runtime + ) + depth_first(child) + + depth_first(source_course) + + for leftover in all_locs: if verbose: - log.debug('importing module location {loc}'.format(loc=module.location)) + log.debug('importing module location {loc}'.format(loc=leftover)) _import_module_and_update_references( - module, store, + xml_module_store.get_item(leftover), store, user_id, course_key, dest_course_id, @@ -256,7 +278,7 @@ def import_from_xml( def _import_course_module( - xml_module_store, store, runtime, user_id, data_dir, course_key, dest_course_id, do_import_static, + store, runtime, user_id, data_dir, course_key, dest_course_id, source_course, do_import_static, verbose, ): if verbose: @@ -265,70 +287,65 @@ def _import_course_module( # Quick scan to get course module as we need some info from there. # Also we need to make sure that the course module is committed # first into the store - for module in xml_module_store.modules[course_key].itervalues(): - if module.scope_ids.block_type == 'course': - course_data_path = path(data_dir) / module.data_dir + course_data_path = path(data_dir) / source_course.data_dir - log.debug(u'======> IMPORTING course {course_key}'.format( - course_key=course_key, - )) + log.debug(u'======> IMPORTING course {course_key}'.format( + course_key=course_key, + )) - if not do_import_static: - # for old-style xblock where this was actually linked to kvs - module.static_asset_path = module.data_dir - module.save() - log.debug('course static_asset_path={path}'.format( - path=module.static_asset_path - )) + if not do_import_static: + # for old-style xblock where this was actually linked to kvs + source_course.static_asset_path = source_course.data_dir + source_course.save() + log.debug('course static_asset_path={path}'.format( + path=source_course.static_asset_path + )) - log.debug('course data_dir={0}'.format(module.data_dir)) + log.debug('course data_dir={0}'.format(source_course.data_dir)) - course = _import_module_and_update_references( - module, store, user_id, - course_key, - dest_course_id, - do_import_static=do_import_static, - runtime=runtime, + course = _import_module_and_update_references( + source_course, store, user_id, + course_key, + dest_course_id, + do_import_static=do_import_static, + runtime=runtime, + ) + + for entry in course.pdf_textbooks: + for chapter in entry.get('chapters', []): + if StaticContent.is_c4x_path(chapter.get('url', '')): + asset_key = StaticContent.get_location_from_path(chapter['url']) + chapter['url'] = StaticContent.get_static_path_from_location(asset_key) + + # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. + # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default + # values then remap it so that the wiki does not point to the old wiki. + if course_key != course.id: + original_unique_wiki_slug = u'{0}.{1}.{2}'.format( + course_key.org, + course_key.course, + course_key.run + ) + if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: + course.wiki_slug = u'{0}.{1}.{2}'.format( + course.id.org, + course.id.course, + course.id.run, ) - for entry in course.pdf_textbooks: - for chapter in entry.get('chapters', []): - if StaticContent.is_c4x_path(chapter.get('url', '')): - asset_key = StaticContent.get_location_from_path(chapter['url']) - chapter['url'] = StaticContent.get_static_path_from_location(asset_key) + # cdodge: more hacks (what else). Seems like we have a + # problem when importing a course (like 6.002) which + # does not have any tabs defined in the policy file. + # The import goes fine and then displays fine in LMS, + # but if someone tries to add a new tab in the CMS, then + # the LMS barfs because it expects that -- if there are + # *any* tabs -- then there at least needs to be + # some predefined ones + if course.tabs is None or len(course.tabs) == 0: + CourseTabList.initialize_default(course) - # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. - # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default - # values then remap it so that the wiki does not point to the old wiki. - if course_key != course.id: - original_unique_wiki_slug = u'{0}.{1}.{2}'.format( - course_key.org, - course_key.course, - course_key.run - ) - if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: - course.wiki_slug = u'{0}.{1}.{2}'.format( - course.id.org, - course.id.course, - course.id.run, - ) - - # cdodge: more hacks (what else). Seems like we have a - # problem when importing a course (like 6.002) which - # does not have any tabs defined in the policy file. - # The import goes fine and then displays fine in LMS, - # but if someone tries to add a new tab in the CMS, then - # the LMS barfs because it expects that -- if there are - # *any* tabs -- then there at least needs to be - # some predefined ones - if course.tabs is None or len(course.tabs) == 0: - CourseTabList.initialize_default(course) - - store.update_item(course, user_id) - return course, course_data_path - - # raise an exception if the course wasn't found - raise Exception("Course module not found in imported modules") + store.update_item(course, user_id) + return course, course_data_path def _import_static_content_wrapper(static_content_store, do_import_static, course_data_path, dest_course_id, verbose): @@ -524,8 +541,8 @@ def _import_course_draft( # attributes (they are normally in the parent object, # aka sequential), so we have to replace the location.name # with the XML filename that is part of the pack - fn, __ = os.path.splitext(filename) - descriptor.location = descriptor.location.replace(name=fn) + filename, __ = os.path.splitext(filename) + descriptor.location = descriptor.location.replace(name=filename) index = int(descriptor.xml_attributes['index_in_children_list']) if index in drafts: @@ -566,7 +583,7 @@ def _import_course_draft( sequential = store.get_item(seq_location, depth=0) non_draft_location = module.location.map_into_course(target_course_id) - if non_draft_location not in sequential.children: + if not any(child.block_id == module.location.block_id for child in sequential.children): sequential.children.insert(index, non_draft_location) store.update_item(sequential, user_id) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index f10124eab0..8febd4f995 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -213,9 +213,9 @@ class CourseComparisonTest(unittest.TestCase): else: revision = None actual_items = actual_store.get_items(actual_course_key, revision=revision) - self._assertCoursesEqual(expected_items, actual_items, actual_course_key) + self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True) - def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key): + def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key, expect_drafts=False): self.assertEqual(len(expected_items), len(actual_items)) actual_item_map = { @@ -264,7 +264,6 @@ class CourseComparisonTest(unittest.TestCase): # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) if expected_item.has_children: - expect_drafts = getattr(expected_item, 'is_draft', getattr(actual_item, 'is_draft', False)) expected_children = [ course1_item_child.location.map_into_course(actual_item.location.course_key) for course1_item_child in expected_item.get_children() @@ -272,10 +271,10 @@ class CourseComparisonTest(unittest.TestCase): if expect_drafts or not getattr(course1_item_child, 'is_draft', False) ] actual_children = [ - item_child.location - for item_child in actual_item.get_children() - # get_children was returning drafts for published parents :-( - if expect_drafts or not getattr(item_child, 'is_draft', False) + item_child.location + for item_child in actual_item.get_children() + # get_children was returning drafts for published parents :-( + if expect_drafts or not getattr(item_child, 'is_draft', False) ] try: self.assertEqual(expected_children, actual_children) @@ -331,34 +330,3 @@ class CourseComparisonTest(unittest.TestCase): actual_thumbs = actual_store.get_all_content_thumbnails_for_course(actual_course_key) self._assertAssetsEqual(expected_course_key, expected_thumbs, actual_course_key, actual_thumbs) - - def compute_real_state(self, store, item): - """ - In draft mongo, compute_published_state can return draft when the draft == published, but in split, - it'll return public in that case - """ - supposed_state = store.compute_publish_state(item) - if supposed_state == PublishState.draft and isinstance(item.runtime.modulestore, DraftModuleStore): - # see if the draft differs from the published - published = store.get_item(item.location, revision=ModuleStoreEnum.RevisionOption.published_only) - if item.get_explicitly_set_fields_by_scope() != published.get_explicitly_set_fields_by_scope(): - # checking content: if published differs from item, return draft - return supposed_state - if item.get_explicitly_set_fields_by_scope(Scope.settings) != published.get_explicitly_set_fields_by_scope(Scope.settings): - # checking settings: if published differs from item, return draft - return supposed_state - if item.has_children and item.children != published.children: - # checking children: if published differs from item, return draft - return supposed_state - # published == item in all respects, so return public - return PublishState.public - elif supposed_state == PublishState.public and item.location.category in DIRECT_ONLY_CATEGORIES: - if not all([ - store.has_item(child_loc, revision=ModuleStoreEnum.RevisionOption.draft_only) - for child_loc in item.children - ]): - return PublishState.draft - else: - return supposed_state - else: - return supposed_state From 70c77f48b86b749b1469a359890f900a4a61de78 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Mon, 4 Aug 2014 18:20:50 -0400 Subject: [PATCH 06/15] fixup - re-enable Asset checking. --- .../modulestore/split_mongo/split_draft.py | 8 ++ .../xmodule/modulestore/xml_importer.py | 76 ++++++++++--------- common/lib/xmodule/xmodule/tests/__init__.py | 10 +-- 3 files changed, 50 insertions(+), 44 deletions(-) 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 0d558dd2bc..378d51b0d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -33,6 +33,14 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS org, course, run, user_id, master_branch=master_branch, **kwargs ) self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) + + # create any other necessary things as a side effect: ensure they populate the draft branch + # and rely on auto publish to populate the published branch + with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, item.id): + super(SplitMongoModuleStore, self).create_course( + org, course, run, user_id, **kwargs + ) + return item def get_course(self, course_id, depth=0): diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 13ef64084f..ad56bfe721 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -303,48 +303,50 @@ def _import_course_module( log.debug('course data_dir={0}'.format(source_course.data_dir)) - course = _import_module_and_update_references( - source_course, store, user_id, - course_key, - dest_course_id, - do_import_static=do_import_static, - runtime=runtime, - ) + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, dest_course_id): - for entry in course.pdf_textbooks: - for chapter in entry.get('chapters', []): - if StaticContent.is_c4x_path(chapter.get('url', '')): - asset_key = StaticContent.get_location_from_path(chapter['url']) - chapter['url'] = StaticContent.get_static_path_from_location(asset_key) - - # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. - # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default - # values then remap it so that the wiki does not point to the old wiki. - if course_key != course.id: - original_unique_wiki_slug = u'{0}.{1}.{2}'.format( - course_key.org, - course_key.course, - course_key.run + course = _import_module_and_update_references( + source_course, store, user_id, + course_key, + dest_course_id, + do_import_static=do_import_static, + runtime=runtime, ) - if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: - course.wiki_slug = u'{0}.{1}.{2}'.format( - course.id.org, - course.id.course, - course.id.run, + + for entry in course.pdf_textbooks: + for chapter in entry.get('chapters', []): + if StaticContent.is_c4x_path(chapter.get('url', '')): + asset_key = StaticContent.get_location_from_path(chapter['url']) + chapter['url'] = StaticContent.get_static_path_from_location(asset_key) + + # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. + # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default + # values then remap it so that the wiki does not point to the old wiki. + if course_key != course.id: + original_unique_wiki_slug = u'{0}.{1}.{2}'.format( + course_key.org, + course_key.course, + course_key.run ) + if course.wiki_slug == original_unique_wiki_slug or course.wiki_slug == course_key.course: + course.wiki_slug = u'{0}.{1}.{2}'.format( + course.id.org, + course.id.course, + course.id.run, + ) - # cdodge: more hacks (what else). Seems like we have a - # problem when importing a course (like 6.002) which - # does not have any tabs defined in the policy file. - # The import goes fine and then displays fine in LMS, - # but if someone tries to add a new tab in the CMS, then - # the LMS barfs because it expects that -- if there are - # *any* tabs -- then there at least needs to be - # some predefined ones - if course.tabs is None or len(course.tabs) == 0: - CourseTabList.initialize_default(course) + # cdodge: more hacks (what else). Seems like we have a + # problem when importing a course (like 6.002) which + # does not have any tabs defined in the policy file. + # The import goes fine and then displays fine in LMS, + # but if someone tries to add a new tab in the CMS, then + # the LMS barfs because it expects that -- if there are + # *any* tabs -- then there at least needs to be + # some predefined ones + if course.tabs is None or len(course.tabs) == 0: + CourseTabList.initialize_default(course) - store.update_item(course, user_id) + store.update_item(course, user_id) return course, course_data_path diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 8febd4f995..dbe8decf0f 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -203,12 +203,12 @@ class CourseComparisonTest(unittest.TestCase): self._assertCoursesEqual(expected_items, actual_items, actual_course_key) # compare draft - if expected_store.get_modulestore_type() == ModuleStoreEnum.Type.split: + if expected_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: revision = ModuleStoreEnum.RevisionOption.draft_only else: revision = None expected_items = expected_store.get_items(expected_course_key, revision=revision) - if actual_store.get_modulestore_type() == ModuleStoreEnum.Type.split: + if actual_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: revision = ModuleStoreEnum.RevisionOption.draft_only else: revision = None @@ -276,10 +276,7 @@ class CourseComparisonTest(unittest.TestCase): # get_children was returning drafts for published parents :-( if expect_drafts or not getattr(item_child, 'is_draft', False) ] - try: - self.assertEqual(expected_children, actual_children) - except: - pass + self.assertEqual(expected_children, actual_children) def assertAssetEqual(self, expected_course_key, expected_asset, actual_course_key, actual_asset): """ @@ -319,7 +316,6 @@ class CourseComparisonTest(unittest.TestCase): ``actual_course_key`` in ``actual_store`` are identical, allowing for differences related to their being from different course keys. """ - return # FIXME remove expected_content, expected_count = expected_store.get_all_content_for_course(expected_course_key) actual_content, actual_count = actual_store.get_all_content_for_course(actual_course_key) From 5fc30dc4f6181e0d8f1e6fcf7b8b7f2bc92c8ebc Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 5 Aug 2014 09:30:21 -0400 Subject: [PATCH 07/15] Move about overview creation to common superclass --- .../xmodule/xmodule/modulestore/__init__.py | 19 +++++++++++++++++++ .../xmodule/xmodule/modulestore/mongo/base.py | 19 +++++-------------- .../xmodule/modulestore/split_mongo/split.py | 1 + .../modulestore/split_mongo/split_draft.py | 2 ++ .../xmodule/modulestore/tests/django_utils.py | 6 ------ .../tests/test_mixed_modulestore.py | 3 +-- 6 files changed, 28 insertions(+), 22 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 3115d2f4c7..c94551008d 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -588,6 +588,25 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): result[field.scope][field_name] = value return result + def create_course(self, org, course, run, user_id, fields=None, runtime=None, **kwargs): + """ + Creates any necessary other things for the course as a side effect and doesn't return + anything useful. The real subclass should call this before it returns the course. + """ + # clone a default 'about' overview module as well + about_location = self.make_course_key(org, course, run).make_usage_key('about', 'overview') + + about_descriptor = XBlock.load_class('about') + overview_template = about_descriptor.get_template('overview.yaml') + self.create_item( + user_id, + about_location.course_key, + about_location.block_type, + block_id=about_location.block_id, + definition_data=overview_template.get('data'), + runtime=runtime + ) + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): """ This base method just copies the assets. The lower level impls must do the actual cloning of diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index ef955d773e..e68b499e8b 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -29,7 +29,6 @@ from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor -from xmodule.html_module import AboutDescriptor from xblock.runtime import KvsFieldData from xblock.exceptions import InvalidScopeError from xblock.fields import Scope, ScopeIds, Reference, ReferenceList, ReferenceValueDict @@ -948,22 +947,14 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): if courses.count() > 0: raise DuplicateCourseError(course_id, courses[0]['_id']) - course = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs) + xblock = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs) - # clone a default 'about' overview module as well - about_location = course_id.make_usage_key('about', 'overview') - - overview_template = AboutDescriptor.get_template('overview.yaml') - self.create_item( - user_id, - about_location.course_key, - about_location.block_type, - block_id=about_location.block_id, - definition_data=overview_template.get('data'), - runtime=course.system + # create any other necessary things as a side effect + super(MongoModuleStore, self).create_course( + org, course, run, user_id, runtime=xblock.runtime, **kwargs ) - return course + return xblock def create_xblock( self, runtime, course_key, block_type, block_id=None, fields=None, diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 9755744373..2a5fa3b7be 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1091,6 +1091,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if fields is not None: 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, **kwargs) return self.update_item(course, user_id, **kwargs) 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 378d51b0d3..02f3ca8141 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -86,6 +86,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS definition_locator=None, fields=None, force=False, continue_version=False, **kwargs ): + course_key = self._map_revision_to_branch(course_key) item = super(DraftVersioningModuleStore, self).create_item( user_id, course_key, block_type, block_id=block_id, definition_locator=definition_locator, fields=fields, @@ -98,6 +99,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS self, user_id, parent_usage_key, block_type, block_id=None, fields=None, continue_version=False, **kwargs ): + parent_usage_key = self._map_revision_to_branch(parent_usage_key) item = super(DraftVersioningModuleStore, self).create_child( user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, continue_version=continue_version, **kwargs diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 91c7c04b06..6a6d8843d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -348,12 +348,6 @@ class ModuleStoreTestCase(TestCase): self.user.id, self.toy_loc, "about", block_id="end_date", fields={"data": "TBD"} ) - self.store.create_item( - self.user.id, self.toy_loc, "about", block_id="overview", - fields={ - "data": "
\n

About This Course

\n

Include your long course description here. The long course description should contain 150-400 words.

\n\n

This is paragraph 2 of the long course description. Add more paragraphs as needed. Make sure to enclose them in paragraph tags.

\n
\n\n
\n

Prerequisites

\n

Add information about course prerequisites here.

\n
\n\n
\n

Course Staff

\n
\n
\n \"Course\n
\n\n

Staff Member #1

\n

Biography of instructor/staff member #1

\n
\n\n
\n
\n \"Course\n
\n\n

Staff Member #2

\n

Biography of instructor/staff member #2

\n
\n
\n\n
\n
\n

Frequently Asked Questions

\n
\n

Do I need to buy a textbook?

\n

No, a free online version of Chemistry: Principles, Patterns, and Applications, First Edition by Bruce Averill and Patricia Eldredge will be available, though you can purchase a printed version (published by FlatWorld Knowledge) if you’d like.

\n
\n\n
\n

Question #2

\n

Your answer would be displayed here.

\n
\n
\n
\n" - } - ) self.store.create_item( self.user.id, self.toy_loc, "course_info", "handouts", fields={"data": "Sample"} 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 3dc1c64b4b..598784b5df 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -348,7 +348,7 @@ class TestMixedModuleStore(unittest.TestCase): # split: 3 to get the course structure & the course definition (show_calculator is scope content) # before the change. 1 during change to refetch the definition. 3 afterward (b/c it calls get_item to return the "new" object). # 2 sends to update index & structure (calculator is a setting field) - @ddt.data(('draft', 7, 5), ('split', 7, 2)) + @ddt.data(('draft', 7, 5), ('split', 6, 2)) @ddt.unpack def test_update_item(self, default_ms, max_find, max_send): """ @@ -853,7 +853,6 @@ class TestMixedModuleStore(unittest.TestCase): # detached items (not considered as orphans) detached_locations = [ course_id.make_usage_key('static_tab', 'StaticTab'), - course_id.make_usage_key('about', 'overview'), course_id.make_usage_key('course_info', 'updates'), ] From e900a47b3f5e9e577ff4d6a30985c9c6c82a1e18 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 6 Aug 2014 09:16:05 -0400 Subject: [PATCH 08/15] Provide workarounds for skip_draft where other code doesn't want its behavior --- cms/djangoapps/contentstore/tests/utils.py | 9 +++++- .../xmodule/modulestore/split_migrator.py | 2 ++ .../xmodule/modulestore/split_mongo/split.py | 9 ++++-- .../modulestore/split_mongo/split_draft.py | 29 ++++++++++++------- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 20b21065bf..cdd0a4b512 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -247,7 +247,14 @@ class CourseTestCase(ModuleStoreTestCase): course1_items = self.store.get_items(course1_id) course2_items = self.store.get_items(course2_id) self.assertGreater(len(course1_items), 0) # ensure it found content instead of [] == [] - self.assertEqual(len(course1_items), len(course2_items)) + if len(course1_items) != len(course2_items): + course1_block_ids = set([item.location.block_id for item in course1_items]) + course2_block_ids = set([item.location.block_id for item in course2_items]) + raise AssertionError( + u"Course1 extra blocks: {}; course2 extra blocks: {}".format( + course1_block_ids - course2_block_ids, course2_block_ids - course1_block_ids + ) + ) for course1_item in course1_items: course2_item_location = course1_item.location.map_into_course(course2_id) diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 8fff1be717..9124195636 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -62,6 +62,7 @@ class SplitMigrator(object): new_org, new_course, new_run, user_id, fields=new_fields, master_branch=ModuleStoreEnum.BranchName.published, + skip_auto_publish=True, **kwargs ) @@ -101,6 +102,7 @@ class SplitMigrator(object): module, course_version_locator, new_course.location.block_id ), continue_version=True, + skip_auto_publish=True, **kwargs ) # after done w/ published items, add version for DRAFT pointing to the published structure diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 2a5fa3b7be..7e7840b391 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -953,8 +953,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 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'], **kwargs + 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'], + skip_auto_publish=True, + **kwargs ) def create_course( 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 02f3ca8141..fb7f452516 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -15,7 +15,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS A subclass of Split that supports a dual-branch fall-back versioning framework with a Draft branch that falls back to a Published branch. """ - def create_course(self, org, course, run, user_id, **kwargs): + def create_course(self, org, course, run, user_id, skip_auto_publish=False, **kwargs): """ Creates and returns the course. @@ -32,14 +32,18 @@ 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, **kwargs) + if master_branch == ModuleStoreEnum.BranchName.draft and not skip_auto_publish: + # any other value is hopefully only cloning or doing something which doesn't want this value add + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) - # create any other necessary things as a side effect: ensure they populate the draft branch - # and rely on auto publish to populate the published branch - with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, item.id): - super(SplitMongoModuleStore, self).create_course( - org, course, run, user_id, **kwargs - ) + # create any other necessary things as a side effect: ensure they populate the draft branch + # and rely on auto publish to populate the published branch: split's create course doesn't + # call super b/c it needs the auto publish above to have happened before any of the create_items + # in this + with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, item.id): + super(SplitMongoModuleStore, self).create_course( + org, course, run, user_id, **kwargs + ) return item @@ -84,15 +88,20 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS def create_item( self, user_id, course_key, block_type, block_id=None, definition_locator=None, fields=None, - force=False, continue_version=False, **kwargs + force=False, continue_version=False, skip_auto_publish=False, **kwargs ): + """ + Adds skip_auto_publish to behavior or parent. Skip_auto_publish basically just calls the super + and skips all of this wrapper's functionality. + """ course_key = self._map_revision_to_branch(course_key) item = super(DraftVersioningModuleStore, self).create_item( user_id, course_key, block_type, block_id=block_id, 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, **kwargs) + if not skip_auto_publish: + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item def create_child( From 7a75b94ff7bcc6aa1ee6c1505b8300703c2d5512 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 6 Aug 2014 11:27:14 -0400 Subject: [PATCH 09/15] Support xml blocks w/ multiple parents --- common/lib/xmodule/xmodule/modulestore/xml_importer.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index ad56bfe721..36800d0799 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -233,7 +233,12 @@ def import_from_xml( """ if subtree.has_children: for child in subtree.get_children(): - all_locs.remove(child.location) + try: + all_locs.remove(child.location) + except KeyError: + # ContentStoreTest.test_image_import has non-tree children + # so, make this more robust + pass if verbose: log.debug('importing module location {loc}'.format(loc=child.location)) From ae4277c4022332c55036e24896a09a049b375357 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 6 Aug 2014 11:37:18 -0400 Subject: [PATCH 10/15] Use new check counter & bump count --- .../contentstore/tests/test_contentstore.py | 44 +++++++------------ 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 32b61e5214..faf0404dbe 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -30,7 +30,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata 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.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint @@ -54,16 +54,6 @@ TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex -class MongoCollectionFindWrapper(object): - def __init__(self, original): - self.original = original - self.counter = 0 - - def find(self, query, *args, **kwargs): - self.counter = self.counter + 1 - return self.original(query, *args, **kwargs) - - @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreTestCase(CourseTestCase): """ @@ -864,18 +854,17 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') - wrapper = MongoCollectionFindWrapper(mongo_store.collection.find) - mongo_store.collection.find = wrapper.find - + # make sure we haven't done too many round trips to DB + # note we say 4 round trips here for: + # 1) to get the run id + # 2) the course, + # 3 & 4) for the chapters and sequentials + # Because we're querying from the top of the tree, we cache information needed for inheritance, + # so we don't need to make an extra query to compute it. # set the branch to 'publish' in order to prevent extra lookups of draft versions with mongo_store.branch_setting(ModuleStoreEnum.Branch.published_only): - course = mongo_store.get_course(course_id, depth=2) - - # make sure we haven't done too many round trips to DB - # note we say 3 round trips here for 1) the course, and 2 & 3) for the chapters and sequentials - # Because we're querying from the top of the tree, we cache information needed for inheritance, - # so we don't need to make an extra query to compute it. - self.assertEqual(wrapper.counter, 3) + with check_mongo_calls(mongo_store, 4, 0): + course = mongo_store.get_course(course_id, depth=2) # make sure we pre-fetched a known sequential which should be at depth=2 self.assertTrue(course_id.make_usage_key('sequential', 'vertical_sequential') in course.system.module_data) @@ -883,16 +872,13 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): # make sure we don't have a specific vertical which should be at depth=3 self.assertFalse(course_id.make_usage_key('vertical', 'vertical_test') in course.system.module_data) - # Now, test with the branch set to draft. We should have one extra round trip call to check for - # the existence of the draft versions - wrapper.counter = 0 - mongo_store.get_course(course_id, depth=2) - self.assertEqual(wrapper.counter, 4) - + # Now, test with the branch set to draft. No extra round trips b/c it doesn't go deep enough to get + # beyond direct only categories + with mongo_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + with check_mongo_calls(mongo_store, 4, 0): + mongo_store.get_course(course_id, depth=2) def test_export_course_without_content_store(self): - content_store = contentstore() - # Create toy course course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy']) From ba8d390fd7cf83757b24cefbd24a2671abe1bf7f Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 6 Aug 2014 14:58:12 -0400 Subject: [PATCH 11/15] Support import of ErrorDescriptors. --- common/lib/xmodule/xmodule/modulestore/xml_importer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 36800d0799..3de561569a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -445,7 +445,7 @@ def _import_module_and_update_references( else: fields[field_name] = field.read_from(module) - return store.import_xblock(user_id, dest_course_id, module.category, module.location.block_id, fields, runtime) + return store.import_xblock(user_id, dest_course_id, module.location.category, module.location.block_id, fields, runtime) def _import_course_draft( From 67d6347e504833486a5212025901363f9afa4981 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 6 Aug 2014 18:09:37 -0400 Subject: [PATCH 12/15] Use continue version for create course side effects --- common/lib/xmodule/xmodule/modulestore/__init__.py | 3 ++- .../lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index c94551008d..b22b78dac9 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -604,7 +604,8 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): about_location.block_type, block_id=about_location.block_id, definition_data=overview_template.get('data'), - runtime=runtime + runtime=runtime, + continue_version=True, ) def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): 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 fb7f452516..ff7c18fb2e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -42,7 +42,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS # in this with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, item.id): super(SplitMongoModuleStore, self).create_course( - org, course, run, user_id, **kwargs + org, course, run, user_id, runtime=item.runtime, **kwargs ) return item From f69ef41c21f84a7e52c77b0063c94e9b066e9d1c Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 6 Aug 2014 18:13:29 -0400 Subject: [PATCH 13/15] Disable fetching by version guid for now --- cms/djangoapps/contentstore/tests/test_crud.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 459f587d36..0665254211 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -163,13 +163,13 @@ class TemplateTests(unittest.TestCase): guid_locator = test_course.location.course_agnostic() # verify it can be retrieved by id self.assertIsInstance(self.split_store.get_course(id_locator), CourseDescriptor) - # and by guid - self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) + # and by guid -- reenable when split_draft supports getting specific versions +# self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) self.split_store.delete_course(id_locator, 'testbot') # test can no longer retrieve by id self.assertRaises(ItemNotFoundError, self.split_store.get_course, id_locator) # but can by guid - self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) +# self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) def test_block_generations(self): """ From 21c6812bd3dd313385a5a7c59a266256007ca777 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 7 Aug 2014 09:25:33 -0400 Subject: [PATCH 14/15] More branch override precedence refinement --- .../xmodule/modulestore/split_mongo/split_draft.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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 ff7c18fb2e..c21e52822a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -39,8 +39,9 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS # create any other necessary things as a side effect: ensure they populate the draft branch # and rely on auto publish to populate the published branch: split's create course doesn't # call super b/c it needs the auto publish above to have happened before any of the create_items - # in this + # in this. The explicit use of SplitMongoModuleStore is intentional with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, item.id): + # pylint: disable=bad-super-call super(SplitMongoModuleStore, self).create_course( org, course, run, user_id, runtime=item.runtime, **kwargs ) @@ -160,10 +161,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS return key.for_branch(ModuleStoreEnum.BranchName.published) elif revision == ModuleStoreEnum.RevisionOption.draft_only: return key.for_branch(ModuleStoreEnum.BranchName.draft) - elif key.branch is not None: - return key - elif revision == None: - if self.get_branch_setting(key) == ModuleStoreEnum.Branch.draft_preferred: + elif revision is None: + if key.branch is not None: + return key + elif self.get_branch_setting(key) == ModuleStoreEnum.Branch.draft_preferred: return key.for_branch(ModuleStoreEnum.BranchName.draft) else: return key.for_branch(ModuleStoreEnum.BranchName.published) From 192d701899c9751acf159745e5e670bc1467f15f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 8 Aug 2014 10:40:27 -0400 Subject: [PATCH 15/15] Code review changes re split import/export, create_xblock --- .../contentstore/tests/test_contentstore.py | 20 ++++++------- .../contentstore/tests/test_crud.py | 4 +-- .../xmodule/xmodule/modulestore/__init__.py | 3 +- .../modulestore/draft_and_published.py | 2 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 13 +++++++- .../xmodule/xmodule/modulestore/mongo/base.py | 12 ++++++-- .../xmodule/modulestore/split_mongo/split.py | 19 ++++++++---- .../modulestore/split_mongo/split_draft.py | 30 ++++++++++++------- .../test_cross_modulestore_import_export.py | 1 - .../xmodule/modulestore/xml_importer.py | 9 +++--- common/lib/xmodule/xmodule/tests/__init__.py | 16 ++++++---- 11 files changed, 84 insertions(+), 45 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index faf0404dbe..09e78efc40 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1400,30 +1400,30 @@ class ContentStoreTest(ContentStoreTestCase): # crate a new module and add it as a child to a vertical parent = verticals[0] - new_module = self.store.create_child( + new_block = self.store.create_child( self.user.id, parent.location, 'html', 'new_component' ) # flush the cache - new_module = self.store.get_item(new_module.location) + new_block = self.store.get_item(new_block.location) # check for grace period definition which should be defined at the course level - self.assertEqual(parent.graceperiod, new_module.graceperiod) - self.assertEqual(parent.start, new_module.start) - self.assertEqual(course.start, new_module.start) + self.assertEqual(parent.graceperiod, new_block.graceperiod) + self.assertEqual(parent.start, new_block.start) + self.assertEqual(course.start, new_block.start) - self.assertEqual(course.xqa_key, new_module.xqa_key) + self.assertEqual(course.xqa_key, new_block.xqa_key) # # now let's define an override at the leaf node level # - new_module.graceperiod = timedelta(1) - self.store.update_item(new_module, self.user.id) + new_block.graceperiod = timedelta(1) + self.store.update_item(new_block, self.user.id) # flush the cache and refetch - new_module = self.store.get_item(new_module.location) + new_block = self.store.get_item(new_block.location) - self.assertEqual(timedelta(1), new_module.graceperiod) + self.assertEqual(timedelta(1), new_block.graceperiod) def test_default_metadata_inheritance(self): course = CourseFactory.create() diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 0665254211..08d67db3d3 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -163,12 +163,12 @@ class TemplateTests(unittest.TestCase): guid_locator = test_course.location.course_agnostic() # verify it can be retrieved by id self.assertIsInstance(self.split_store.get_course(id_locator), CourseDescriptor) - # and by guid -- reenable when split_draft supports getting specific versions + # and by guid -- TODO reenable when split_draft supports getting specific versions # self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) self.split_store.delete_course(id_locator, 'testbot') # test can no longer retrieve by id self.assertRaises(ItemNotFoundError, self.split_store.get_course, id_locator) - # but can by guid + # but can by guid -- same TODO as above # self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) def test_block_generations(self): diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index b22b78dac9..25de6e22dc 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -603,7 +603,8 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): about_location.course_key, about_location.block_type, block_id=about_location.block_id, - definition_data=overview_template.get('data'), + definition_data={'data': overview_template.get('data')}, + metadata=overview_template.get('metadata'), runtime=runtime, continue_version=True, ) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 3318135289..7db8ec0559 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -104,7 +104,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): raise NotImplementedError @abstractmethod - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Import the given xblock into the current branch setting: import completely overwrites any existing block of the same id. diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 961f5c634b..91b4f1076d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -453,8 +453,10 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs) @strip_key - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ + See :py:meth `ModuleStoreDraftAndPublished.import_xblock` + Defer to the course's modulestore if it supports this method """ store = self._verify_modulestore_support(course_key, 'import_xblock') @@ -511,6 +513,15 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): """ Create the new xmodule but don't save it. Returns the new module. + + Args: + runtime: :py:class `xblock.runtime` from another xblock in the same course. Providing this + significantly speeds up processing (inheritance and subsequent persistence) + course_key: :py:class `opaque_keys.CourseKey` + block_type: :py:class `string`: the string identifying the xblock type + block_id: the string uniquely identifying the block within the given course + fields: :py:class `dict` field_name, value pairs for initializing the xblock fields. Values + should be the pythonic types not the json serialized ones. """ store = self._verify_modulestore_support(course_key, 'create_xblock') return store.create_xblock(runtime, course_key, block_type, block_id, fields or {}, **kwargs) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index e68b499e8b..e2592168d2 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -974,7 +974,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): # @Cale, should this use LocalId like we do in split? if block_id is None: - block_id = uuid4().hex # really need to make this more readable: e.g., u'{}{}'.format(block_type, uuid4().hex[:4] + if block_type == 'course': + block_id = course_key.run + else: + block_id = u'{}_{}'.format(block_type, uuid4().hex[:5]) if runtime is None: services = {} @@ -1027,7 +1030,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): a new identifier will be generated """ if block_id is None: - block_id = uuid4().hex + if block_type == 'course': + block_id = course_key.run + else: + block_id = u'{}_{}'.format(block_type, uuid4().hex[:5]) runtime = kwargs.pop('runtime', None) xblock = self.create_xblock(runtime, course_key, block_type, block_id, **kwargs) @@ -1058,7 +1064,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return xblock - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Simple implementation of overwriting any existing xblock """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7e7840b391..abe9a693f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -534,7 +534,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return [ BlockUsageLocator( course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id - ).version_agnostic() + ) for block_id in items ] @@ -818,7 +818,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # 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 {}) + definition_data = kwargs.pop('definition_data', {}) + if definition_data: + if not isinstance(definition_data, dict): + definition_data = {'data': definition_data} # backward compatibility to mongo's hack + fields.update(definition_data) # find course_index entry if applicable and structures entry index_entry = self._get_index_if_valid(course_key, force, continue_version) @@ -962,10 +966,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): **kwargs ) + DEFAULT_ROOT_BLOCK_ID = 'course' def create_course( self, org, course, run, user_id, master_branch=None, fields=None, versions_dict=None, search_targets=None, root_category='course', - root_block_id='course', **kwargs + root_block_id=None, **kwargs ): """ Create a new entry in the active courses index which points to an existing or new structure. Returns @@ -1043,7 +1048,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self.db_connection.insert_definition(definition_entry) draft_structure = self._new_structure( - user_id, root_block_id, root_category, block_fields, definition_id + user_id, + root_block_id or SplitMongoModuleStore.DEFAULT_ROOT_BLOCK_ID, + root_category, + block_fields, + definition_id ) new_id = draft_structure['_id'] @@ -1168,7 +1177,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): is_updated = self._compare_settings(settings, original_entry['fields']) # check children - if partitioned_fields[Scope.children]: + if partitioned_fields.get(Scope.children, {}): # purposely not 'is not None' serialized_children = [child.block_id for child in partitioned_fields[Scope.children]['children']] is_updated = is_updated or original_entry['fields'].get('children', []) != serialized_children if is_updated: 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 c21e52822a..a57da5c16c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -48,9 +48,9 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS return item - def get_course(self, course_id, depth=0): + def get_course(self, course_id, depth=0, **kwargs): course_id = self._map_revision_to_branch(course_id) - return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth) + return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs) def get_courses(self, **kwargs): """ @@ -92,8 +92,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS force=False, continue_version=False, skip_auto_publish=False, **kwargs ): """ - Adds skip_auto_publish to behavior or parent. Skip_auto_publish basically just calls the super - and skips all of this wrapper's functionality. + See :py:meth `ModuleStoreDraftAndPublished.create_item` """ course_key = self._map_revision_to_branch(course_key) item = super(DraftVersioningModuleStore, self).create_item( @@ -210,9 +209,9 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS location = self._map_revision_to_branch(location, revision=revision) return SplitMongoModuleStore.get_parent_location(self, location, **kwargs) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): course_key = self._map_revision_to_branch(course_key) - return super(DraftVersioningModuleStore, self).get_orphans(course_key) + return super(DraftVersioningModuleStore, self).get_orphans(course_key, **kwargs) def has_changes(self, xblock): """ @@ -271,16 +270,25 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS raise NotImplementedError() def get_course_history_info(self, course_locator): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_course_history_info` + """ course_locator = self._map_revision_to_branch(course_locator) return super(DraftVersioningModuleStore, self).get_course_history_info(course_locator) def get_course_successors(self, course_locator, version_history_depth=1): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_course_successors` + """ course_locator = self._map_revision_to_branch(course_locator) return super(DraftVersioningModuleStore, self).get_course_successors( course_locator, version_history_depth=version_history_depth ) def get_block_generations(self, block_locator): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_block_generations` + """ block_locator = self._map_revision_to_branch(block_locator) return super(DraftVersioningModuleStore, self).get_block_generations(block_locator) @@ -325,25 +333,25 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ return block['edit_info'].get('source_version', block['edit_info']['update_version']) - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Split-based modulestores need to import published blocks to both branches """ # hardcode course root block id if block_type == 'course': - block_id = 'course' + block_id = self.DEFAULT_ROOT_BLOCK_ID new_usage_key = course_key.make_usage_key(block_type, block_id) - if self.get_branch_setting(course_key) == ModuleStoreEnum.Branch.published_only: + if self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: # if importing a direct only, override existing draft if block_type in DIRECT_ONLY_CATEGORIES: draft_course = course_key.for_branch(ModuleStoreEnum.BranchName.draft) with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, draft_course): draft = self.import_xblock(user_id, draft_course, block_type, block_id, fields, runtime) self._auto_publish_no_children(draft.location, block_type, user_id) - return self.get_item(new_usage_key) + return self.get_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.published)) # if new to published - elif not self.has_item(new_usage_key): + elif not self.has_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.published)): # check whether it's new to draft if not self.has_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.draft)): # add to draft too diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 2efaa154b3..3c407eceef 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -18,7 +18,6 @@ import random from contextlib import contextmanager, nested from shutil import rmtree from tempfile import mkdtemp -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.tests import CourseComparisonTest diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 3de561569a..aa2daa9d72 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -166,7 +166,7 @@ def import_from_xml( served directly by nginx, instead of going through django. create_new_course_if_not_present: If True, then a new course is created if it doesn't already exist. - Otherwise, it throws an InvalidLocationError for the course. + Otherwise, it throws an InvalidLocationError if the course does not exist. default_class, load_error_modules: are arguments for constructing the XMLModuleStore (see its doc) """ @@ -236,14 +236,15 @@ def import_from_xml( try: all_locs.remove(child.location) except KeyError: - # ContentStoreTest.test_image_import has non-tree children - # so, make this more robust + # tolerate same child occurring under 2 parents such as in + # ContentStoreTest.test_image_import pass if verbose: log.debug('importing module location {loc}'.format(loc=child.location)) _import_module_and_update_references( - child, store, + child, + store, user_id, course_key, dest_course_id, diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index dbe8decf0f..dfd1a95b7a 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -219,19 +219,22 @@ class CourseComparisonTest(unittest.TestCase): self.assertEqual(len(expected_items), len(actual_items)) actual_item_map = { - actual_course_key.make_usage_key(item.category, item.location.block_id): item + item.location.block_id: item for item in actual_items } for expected_item in expected_items: actual_item_location = actual_course_key.make_usage_key(expected_item.category, expected_item.location.block_id) + # split and old mongo use different names for the course root but we don't know which + # modulestore actual's come from here; so, assume old mongo and if that fails, assume split if expected_item.location.category == 'course': actual_item_location = actual_item_location.replace(name=actual_item_location.run) - actual_item = actual_item_map.get(actual_item_location) + actual_item = actual_item_map.get(actual_item_location.block_id) + # must be split if actual_item is None and expected_item.location.category == 'course': actual_item_location = actual_item_location.replace(name='course') - actual_item = actual_item_map.get(actual_item_location) - assert actual_item is not None + actual_item = actual_item_map.get(actual_item_location.block_id) + self.assertIsNotNone(actual_item, u'cannot find {} in {}'.format(actual_item_location, actual_item_map)) # compare fields self.assertEqual(expected_item.fields, actual_item.fields) @@ -264,14 +267,15 @@ class CourseComparisonTest(unittest.TestCase): # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) if expected_item.has_children: + actual_course_key = actual_item.location.course_key.version_agnostic() expected_children = [ - course1_item_child.location.map_into_course(actual_item.location.course_key) + course1_item_child.location.map_into_course(actual_course_key) for course1_item_child in expected_item.get_children() # get_children was returning drafts for published parents :-( if expect_drafts or not getattr(course1_item_child, 'is_draft', False) ] actual_children = [ - item_child.location + item_child.location.version_agnostic() for item_child in actual_item.get_children() # get_children was returning drafts for published parents :-( if expect_drafts or not getattr(item_child, 'is_draft', False)