Merge pull request #3242 from edx/will/studio-import-private-fix

Allow import of native XBlocks in draft

cms/djangoapps/contentstore/tests/test_import_pure_xblock.py

+"""
+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:
+        <stubxblock test_field="this is only a test" />
+    """
+    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)

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,33 +495,14 @@ 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()
-            )
-
-        module.location = module.location.replace(
+        _update_module_location(
+            module,
+            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:
         #
@@ -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()

common/test/data/pure_xblock_draft/chapter/150311f8087e47f4858f7db44df32ee5.xml

+<chapter display_name="Test Chapter">
+  <sequential url_name="b68bb50c4f1f41a3abeec1dac309087d"/>
+</chapter>

common/test/data/pure_xblock_draft/course.xml

+<course url_name="2012_Fall" org="edX" course="pure_xblock_draft"/>

common/test/data/pure_xblock_draft/course/2012_Fall.xml

+<course display_name="Test Course" start="2014-01-01T00:00:00Z">
+  <chapter url_name="150311f8087e47f4858f7db44df32ee5"/>
+</course>

common/test/data/pure_xblock_draft/drafts/vertical/5e8c6159aebf497db24c6eaea9d4982b.xml

+<vertical display_name="Test" parent_sequential_url="i4x://edX/pure_xblock_draft/sequential/b68bb50c4f1f41a3abeec1dac309087d" index_in_children_list="0">
+    <stubxblock test_field="set by xml" url_name="xblock_test" />
+</vertical>

common/test/data/pure_xblock_draft/sequential/b68bb50c4f1f41a3abeec1dac309087d.xml

+<sequential display_name="Test Section"/>

common/test/data/pure_xblock_public/course.xml

+<course org="edX" course="pure_xblock_public" url_name="2012_Fall">
+    <stubxblock test_field="set by xml" url_name="xblock_test" />
+</course>