diff --git a/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py new file mode 100644 index 0000000000..c788ad1a67 --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py @@ -0,0 +1,82 @@ +""" +Integration tests for importing courses containing pure XBlocks. +""" + +from django.test.utils import override_settings + +from xblock.core import XBlock +from xblock.fields import String + +from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.django import modulestore +from contentstore.tests.modulestore_config import TEST_MODULESTORE + + +class StubXBlock(XBlock): + """ + Stub XBlock to use in tests. + + The default XBlock implementation will load this XBlock + from XML, using the lowercase version of the class + as an element name ("stubxblock") and the field names + as attributes of that element. + + Example: + + """ + test_field = String(default="default") + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class XBlockImportTest(ModuleStoreTestCase): + + def setUp(self): + self.store = modulestore('direct') + self.draft_store = modulestore('default') + + @XBlock.register_temp_plugin(StubXBlock) + def test_import_public(self): + self._assert_import( + 'pure_xblock_public', + 'i4x://edX/pure_xblock_public/stubxblock/xblock_test', + 'set by xml' + ) + + @XBlock.register_temp_plugin(StubXBlock) + def test_import_draft(self): + self._assert_import( + 'pure_xblock_draft', + 'i4x://edX/pure_xblock_draft/stubxblock/xblock_test@draft', + 'set by xml', + has_draft=True + ) + + def _assert_import(self, course_dir, expected_xblock_loc, expected_field_val, has_draft=False): + """ + Import a course from XML, then verify that the XBlock was loaded + with the correct field value. + + Args: + course_dir (str): The name of the course directory (relative to the test data directory) + expected_xblock_loc (str): The location of the XBlock in the course. + expected_field_val (str): The expected value of the XBlock's test field. + + Kwargs: + has_draft (bool): If true, check that a draft of the XBlock exists with + the expected field value set. + + """ + import_from_xml( + self.store, 'common/test/data', [course_dir], + draft_store=self.draft_store + ) + + xblock = self.store.get_item(expected_xblock_loc) + self.assertTrue(isinstance(xblock, StubXBlock)) + self.assertEqual(xblock.test_field, expected_field_val) + + if has_draft: + draft_xblock = self.draft_store.get_item(expected_xblock_loc) + self.assertTrue(isinstance(draft_xblock, StubXBlock)) + self.assertEqual(draft_xblock.test_field, expected_field_val) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 0e49581efb..b9dd91d09e 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -6,6 +6,7 @@ import json from .xml import XMLModuleStore, ImportSystem, ParentTracker from xmodule.modulestore import Location +from xblock.runtime import KvsFieldData, DictKeyValueStore from xmodule.x_module import XModuleDescriptor from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict from xmodule.contentstore.content import StaticContent @@ -366,7 +367,8 @@ def import_course_draft( error_tracker=errorlog.tracker, parent_tracker=ParentTracker(), load_error_modules=False, - field_data=None, + mixins=xml_module_store.xblock_mixins, + field_data=KvsFieldData(kvs=DictKeyValueStore()), ) # now walk the /vertical directory where each file in there @@ -440,7 +442,11 @@ def import_course_draft( for descriptor in drafts[key]: try: def _import_module(module): - module.location = module.location.replace(revision='draft') + # 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='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 @@ -489,34 +495,15 @@ def remap_namespace(module, target_location_namespace): # This looks a bit wonky as we need to also change the 'name' of the # imported course to be what the caller passed in if module.location.category != 'course': - - # Retrieve the content and settings fields that have been explicitly set - # to ensure that they are properly re-keyed in the XBlock field data. - if isinstance(module, XModuleDescriptor): - rekey_fields = [] - else: - rekey_fields = ( - module.get_explicitly_set_fields_by_scope(Scope.content).keys() + - module.get_explicitly_set_fields_by_scope(Scope.settings).keys() + _update_module_location( + module, + module.location.replace( + tag=target_location_namespace.tag, + org=target_location_namespace.org, + course=target_location_namespace.course ) - - module.location = module.location.replace( - tag=target_location_namespace.tag, - org=target_location_namespace.org, - course=target_location_namespace.course ) - # Native XBlocks store the field data in a key-value store - # in which one component of the key is the XBlock's location (equivalent to "scope_ids"). - # Since we've changed the XBlock's location, we need to re-save - # all the XBlock's fields so they will be stored using the new location in the key. - # However, since XBlocks only save "dirty" fields, we need to first - # explicitly set each field to its current value before triggering the save. - if len(rekey_fields) > 0: - for rekey_field_name in rekey_fields: - setattr(module, rekey_field_name, getattr(module, rekey_field_name)) - module.save() - else: # # module is a course module @@ -834,3 +821,42 @@ def perform_xlint( print("This course can be imported successfully.") return err_cnt + + +def _update_module_location(module, new_location): + """ + Update a module's location. + + If the module is a pure XBlock (not an XModule), then its field data + keys will need to be updated to include the new location. + + Args: + module (XModuleMixin): The module to update. + new_location (Location): The new location of the module. + + Returns: + None + + """ + # Retrieve the content and settings fields that have been explicitly set + # to ensure that they are properly re-keyed in the XBlock field data. + if isinstance(module, XModuleDescriptor): + rekey_fields = [] + else: + rekey_fields = ( + module.get_explicitly_set_fields_by_scope(Scope.content).keys() + + module.get_explicitly_set_fields_by_scope(Scope.settings).keys() + ) + + module.location = new_location + + # Pure XBlocks store the field data in a key-value store + # in which one component of the key is the XBlock's location (equivalent to "scope_ids"). + # Since we've changed the XBlock's location, we need to re-save + # all the XBlock's fields so they will be stored using the new location in the key. + # However, since XBlocks only save "dirty" fields, we need to first + # explicitly set each field to its current value before triggering the save. + if len(rekey_fields) > 0: + for rekey_field_name in rekey_fields: + setattr(module, rekey_field_name, getattr(module, rekey_field_name)) + module.save() diff --git a/common/test/data/pure_xblock_draft/chapter/150311f8087e47f4858f7db44df32ee5.xml b/common/test/data/pure_xblock_draft/chapter/150311f8087e47f4858f7db44df32ee5.xml new file mode 100644 index 0000000000..5ade9eab68 --- /dev/null +++ b/common/test/data/pure_xblock_draft/chapter/150311f8087e47f4858f7db44df32ee5.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/pure_xblock_draft/course.xml b/common/test/data/pure_xblock_draft/course.xml new file mode 100644 index 0000000000..db7c4e2676 --- /dev/null +++ b/common/test/data/pure_xblock_draft/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/pure_xblock_draft/course/2012_Fall.xml b/common/test/data/pure_xblock_draft/course/2012_Fall.xml new file mode 100644 index 0000000000..934629aa27 --- /dev/null +++ b/common/test/data/pure_xblock_draft/course/2012_Fall.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/pure_xblock_draft/drafts/vertical/5e8c6159aebf497db24c6eaea9d4982b.xml b/common/test/data/pure_xblock_draft/drafts/vertical/5e8c6159aebf497db24c6eaea9d4982b.xml new file mode 100644 index 0000000000..852fb9297b --- /dev/null +++ b/common/test/data/pure_xblock_draft/drafts/vertical/5e8c6159aebf497db24c6eaea9d4982b.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/pure_xblock_draft/sequential/b68bb50c4f1f41a3abeec1dac309087d.xml b/common/test/data/pure_xblock_draft/sequential/b68bb50c4f1f41a3abeec1dac309087d.xml new file mode 100644 index 0000000000..18a8c495cd --- /dev/null +++ b/common/test/data/pure_xblock_draft/sequential/b68bb50c4f1f41a3abeec1dac309087d.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/pure_xblock_public/course.xml b/common/test/data/pure_xblock_public/course.xml new file mode 100644 index 0000000000..600bf96ce6 --- /dev/null +++ b/common/test/data/pure_xblock_public/course.xml @@ -0,0 +1,3 @@ + + +