From 37c873db90efbf2d189168e59efeedcf503856c8 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 29 Jul 2014 17:12:01 -0400 Subject: [PATCH] Serialize using to_json on fields --- .../xmodule/xmodule/modulestore/__init__.py | 6 +- .../xmodule/modulestore/split_migrator.py | 57 +++++-------------- .../xmodule/modulestore/split_mongo/split.py | 30 ++++++++-- .../tests/test_split_modulestore.py | 16 ++---- 4 files changed, 46 insertions(+), 63 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index f21507d185..2a38f4e677 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -568,10 +568,10 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): :param category: the xblock category :param fields: the dictionary of {fieldname: value} """ - if fields is None: - return {} - cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules)) result = collections.defaultdict(dict) + if fields is None: + return result + cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules)) for field_name, value in fields.iteritems(): field = getattr(cls, field_name) result[field.scope][field_name] = value diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 1f3d66a123..5d5712939b 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -53,7 +53,7 @@ class SplitMigrator(object): new_run = source_course_key.run new_course_key = CourseLocator(new_org, new_course, new_run, branch=ModuleStoreEnum.BranchName.published) - new_fields = self._get_json_fields_translate_references(original_course, new_course_key, None) + new_fields = self._get_fields_translate_references(original_course, new_course_key, None) if fields: new_fields.update(fields) new_course = self.split_modulestore.create_course( @@ -92,7 +92,7 @@ class SplitMigrator(object): course_version_locator, module.location.block_type, block_id=module.location.block_id, - fields=self._get_json_fields_translate_references( + fields=self._get_fields_translate_references( module, course_version_locator, new_course.location.block_id ), continue_version=True @@ -127,7 +127,7 @@ class SplitMigrator(object): if field.is_set_on(split_module) and not module.fields[name].is_set_on(module): field.delete_from(split_module) for field, value in self._get_fields_translate_references( - module, new_draft_course_loc, published_course_usage_key.block_id + module, new_draft_course_loc, published_course_usage_key.block_id, field_names=False ).iteritems(): field.write_to(split_module, value) @@ -138,7 +138,7 @@ class SplitMigrator(object): user_id, new_draft_course_loc, new_locator.block_type, block_id=new_locator.block_id, - fields=self._get_json_fields_translate_references( + fields=self._get_fields_translate_references( module, new_draft_course_loc, published_course_usage_key.block_id ) ) @@ -173,43 +173,13 @@ class SplitMigrator(object): new_parent.children.insert(new_parent_cursor, new_locator) new_parent = self.split_modulestore.update_item(new_parent, user_id) - def _get_json_fields_translate_references(self, xblock, new_course_key, course_block_id): - """ - Return the json repr for explicitly set fields but convert all references to their Locators - """ - def get_translation(location): - """ - Convert the location - """ - return new_course_key.make_usage_key( - location.category, - location.block_id if location.category != 'course' else course_block_id - ) - - result = {} - for field_name, field in xblock.fields.iteritems(): - if field.is_set_on(xblock): - field_value = getattr(xblock, field_name) - if isinstance(field, Reference) and field_value is not None: - result[field_name] = get_translation(field_value) - elif isinstance(field, ReferenceList): - result[field_name] = [ - get_translation(ele) for ele in field_value - ] - elif isinstance(field, ReferenceValueDict): - result[field_name] = { - key: get_translation(subvalue) - for key, subvalue in field_value.iteritems() - } - else: - result[field_name] = field.read_json(xblock) - - return result - - def _get_fields_translate_references(self, xblock, new_course_key, course_block_id): + def _get_fields_translate_references(self, xblock, new_course_key, course_block_id, field_names=True): """ Return a dictionary of field: value pairs for explicitly set fields but convert all references to their BlockUsageLocators + Args: + field_names: if Truthy, the dictionary keys are the field names. If falsey, the keys are the + field objects. """ def get_translation(location): """ @@ -223,19 +193,20 @@ class SplitMigrator(object): result = {} for field_name, field in xblock.fields.iteritems(): if field.is_set_on(xblock): - field_value = getattr(xblock, field_name) + field_value = field.read_from(xblock) + field_key = field_name if field_names else field if isinstance(field, Reference) and field_value is not None: - result[field] = get_translation(field_value) + result[field_key] = get_translation(field_value) elif isinstance(field, ReferenceList): - result[field] = [ + result[field_key] = [ get_translation(ele) for ele in field_value ] elif isinstance(field, ReferenceValueDict): - result[field] = { + result[field_key] = { key: get_translation(subvalue) for key, subvalue in field_value.iteritems() } else: - result[field] = field_value + result[field_key] = field_value return result diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 6683acd8e0..886b790f67 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -77,6 +77,8 @@ from .caching_descriptor_system import CachingDescriptorSystem from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo +import types +from _collections import defaultdict log = logging.getLogger(__name__) @@ -1006,7 +1008,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): raise DuplicateCourseError(locator, index) partitioned_fields = self.partition_fields_by_scope(root_category, fields) - block_fields = partitioned_fields.setdefault(Scope.settings, {}) + block_fields = partitioned_fields[Scope.settings] if Scope.children in partitioned_fields: block_fields.update(partitioned_fields[Scope.children]) definition_fields = self._serialize_fields(root_category, partitioned_fields.get(Scope.content, {})) @@ -1106,14 +1108,15 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): original_structure = self._lookup_course(descriptor.location)['structure'] index_entry = self._get_index_if_valid(descriptor.location, force) - definition_fields = descriptor.get_explicitly_set_fields_by_scope(Scope.content) + 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 ) original_entry = self._get_block_from_structure(original_structure, descriptor.location.block_id) # check metadata - settings = descriptor.get_explicitly_set_fields_by_scope(Scope.settings) + settings = partitioned_fields[Scope.settings] settings = self._serialize_fields(descriptor.category, settings) if not is_updated: is_updated = self._compare_settings(settings, original_entry['fields']) @@ -1235,7 +1238,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): def _persist_subdag(self, xblock, user_id, structure_blocks, new_id): # persist the definition if persisted != passed - new_def_data = self._serialize_fields(xblock.category, xblock.get_explicitly_set_fields_by_scope(Scope.content)) + partitioned_fields = self.partition_xblock_fields_by_scope(xblock) + new_def_data = self._serialize_fields(xblock.category, partitioned_fields[Scope.content]) is_updated = False if xblock.definition_locator is None or isinstance(xblock.definition_locator.definition_id, LocalId): xblock.definition_locator = self.create_definition_from_data( @@ -1270,7 +1274,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): children.append(child.block_id) is_updated = is_updated or structure_blocks[encoded_block_id]['fields']['children'] != children - block_fields = xblock.get_explicitly_set_fields_by_scope(Scope.settings) + block_fields = partitioned_fields[Scope.settings] block_fields = self._serialize_fields(xblock.category, block_fields) if not is_new and not is_updated: is_updated = self._compare_settings(block_fields, structure_blocks[encoded_block_id]['fields']) @@ -1697,6 +1701,18 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): index_entry['versions'][branch] = new_id self.db_connection.update_course_index(index_entry) + def partition_xblock_fields_by_scope(self, xblock): + """ + Return a dictionary of scopes mapped to this xblock's explicitly set fields w/o any conversions + """ + # explicitly_set_fields_by_scope converts to json; so, avoiding it + # the existing partition_fields_by_scope works on a dict not an xblock + result = defaultdict(dict) + for field in xblock.fields.itervalues(): + if field.is_set_on(xblock): + result[field.scope][field.name] = field.read_from(xblock) + return result + def _serialize_fields(self, category, fields): """ Convert any references to their serialized form. @@ -1708,6 +1724,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): assert isinstance(fields, dict) xblock_class = XBlock.load_class(category, self.default_class) xblock_class = self.mixologist.mix(xblock_class) + for field_name, value in fields.iteritems(): if value: if isinstance(xblock_class.fields[field_name], Reference): @@ -1719,6 +1736,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): elif isinstance(xblock_class.fields[field_name], ReferenceValueDict): for key, subvalue in value.iteritems(): value[key] = subvalue.block_id + # should this recurse down dicts and lists just in case they contain datetime? + elif not isinstance(value, datetime.datetime): # don't convert datetimes! + fields[field_name] = xblock_class.fields[field_name].to_json(value) # I think these are obsolete conditions; so, I want to confirm that. Thus the warnings if 'location' in fields: 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 a97ae4d264..8e64247c80 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1464,19 +1464,11 @@ class TestCourseCreation(SplitModuleTest): original_locator = CourseLocator(org='guestx', course='contender', run="run", branch=BRANCH_NAME_DRAFT) original = modulestore().get_course(original_locator) original_index = modulestore().get_course_index_info(original_locator) - fields = {} - for field in original.fields.values(): - value = getattr(original, field.name) - if not isinstance(value, datetime.datetime): - json_value = field.to_json(value) - else: - json_value = value - if field.scope == Scope.content and field.name != 'location': - fields[field.name] = json_value - elif field.scope == Scope.settings: - fields[field.name] = json_value + fields = { + 'grading_policy': original.grading_policy, + 'display_name': 'Derivative', + } fields['grading_policy']['GRADE_CUTOFFS'] = {'A': .9, 'B': .8, 'C': .65} - fields['display_name'] = 'Derivative' new_draft = modulestore().create_course( 'counter', 'leech', 'leech_run', 'leech_master', BRANCH_NAME_DRAFT, versions_dict={BRANCH_NAME_DRAFT: original_index['versions'][BRANCH_NAME_DRAFT]},