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 92125eefe3..65cf16e9da 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 @@ -313,13 +313,14 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest): create_new_course_if_not_present=True, ) - export_to_xml( - dest_store, - dest_content, - dest_course_key, - self.export_dir, - 'exported_dest_course', - ) +# NOT CURRENTLY USED +# export_to_xml( +# dest_store, +# dest_content, +# dest_course_key, +# self.export_dir, +# 'exported_dest_course', +# ) self.exclude_field(None, 'wiki_slug') self.exclude_field(None, 'xml_attributes') diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 79304da535..31fd7bab92 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -492,6 +492,46 @@ def _import_course_draft( field_data=KvsFieldData(kvs=DictKeyValueStore()), ) + def _import_module(module): + # IMPORTANT: Be sure to update the module location in the NEW namespace + module_location = module.location.map_into_course(target_course_id) + # Update the module's location to DRAFT revision + # We need to call this method (instead of updating the location directly) + # to ensure that pure XBlock field data is updated correctly. + _update_module_location(module, module_location.replace(revision=MongoRevisionKey.draft)) + + # make sure our parent has us in its list of children + # this is to make sure private only verticals show up + # in the list of children since they would have been + # filtered out from the non-draft store export. + # 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: + sequential_url = module.xml_attributes['parent_sequential_url'] + index = int(module.xml_attributes['index_in_children_list']) + + course_key = descriptor.location.course_key + seq_location = course_key.make_usage_key_from_deprecated_string(sequential_url) + + # IMPORTANT: Be sure to update the sequential in the NEW namespace + 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 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) + + _import_module_and_update_references( + module, store, user_id, + source_course_id, + target_course_id, + runtime=mongo_runtime, + ) + for child in module.get_children(): + _import_module(child) + # now walk the /vertical directory where each file in there # will be a draft copy of the Vertical @@ -563,51 +603,11 @@ def _import_course_draft( logging.exception('Error while parsing course xml.') # For each index_in_children_list key, there is a list of vertical descriptors. + for key in sorted(drafts.iterkeys()): for descriptor in drafts[key]: - course_key = descriptor.location.course_key try: - def _import_module(module): - # IMPORTANT: Be sure to update the module location in the NEW namespace - module_location = module.location.map_into_course(target_course_id) - # Update the module's location to DRAFT revision - # We need to call this method (instead of updating the location directly) - # to ensure that pure XBlock field data is updated correctly. - _update_module_location(module, module_location.replace(revision=MongoRevisionKey.draft)) - - # make sure our parent has us in its list of children - # this is to make sure private only verticals show up - # in the list of children since they would have been - # filtered out from the non-draft store export. - # 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: - sequential_url = module.xml_attributes['parent_sequential_url'] - index = int(module.xml_attributes['index_in_children_list']) - - seq_location = course_key.make_usage_key_from_deprecated_string(sequential_url) - - # IMPORTANT: Be sure to update the sequential in the NEW namespace - 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 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) - - _import_module_and_update_references( - module, store, user_id, - source_course_id, - target_course_id, - runtime=mongo_runtime, - ) - for child in module.get_children(): - _import_module(child) - _import_module(descriptor) - except Exception: logging.exception('while importing draft descriptor %s', descriptor) diff --git a/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py b/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py index dc27f0c792..e922b4716f 100644 --- a/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py +++ b/common/lib/xmodule/xmodule/tests/xml/test_inheritance.py @@ -1,10 +1,10 @@ """ Test that inherited fields work correctly when parsing XML """ -from nose.tools import assert_equals # pylint: disable=no-name-in-module +from nose.tools import assert_equals, assert_in # pylint: disable=no-name-in-module from xmodule.tests.xml import XModuleXmlImportTest -from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory +from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory, XmlImportFactory class TestInheritedFieldParsing(XModuleXmlImportTest): @@ -27,3 +27,21 @@ class TestInheritedFieldParsing(XModuleXmlImportTest): problem = sequence.get_children()[0] assert_equals(None, problem.days_early_for_beta) + + def test_video_attr(self): + """ + Test that video's definition_from_xml handles unknown attrs w/o choking + """ + # Fixes LMS-11491 + root = CourseFactory.build() + sequence = SequenceFactory.build(parent=root) + video = XmlImportFactory( + parent=sequence, + tag='video', + attribs={ + 'parent_sequential_url': 'foo', 'garbage': 'asdlk', + 'download_video': 'true', + } + ) + video_block = self.process_xml(video) + assert_in('garbage', video_block.xml_attributes) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 2b0e3e9075..f65f5c194b 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -583,15 +583,14 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler # If the user has specified html5 sources, make sure we don't use the default video if youtube_id != '' or 'html5_sources' in field_data: field_data['youtube_id_{0}'.format(normalized_speed.replace('.', '_'))] = youtube_id + elif attr in conversions: + field_data[attr] = conversions[attr](value) + elif attr not in cls.fields: + field_data.setdefault('xml_attributes', {})[attr] = value else: - # Convert XML attrs into Python values. - if attr in conversions: - value = conversions[attr](value) - else: # We export values with json.dumps (well, except for Strings, but # for about a month we did it for Strings also). - value = deserialize_field(cls.fields[attr], value) - field_data[attr] = value + field_data[attr] = deserialize_field(cls.fields[attr], value) # For backwards compatibility: Add `source` if XML doesn't have `download_video` # attribute.