Merge pull request #7910 from edx/jeskew/PLAT_452_drafts_show_as_published

Fix issue PLAT-452: Draft block showing as published.

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -352,16 +352,21 @@ class EditInfo(object):
         self.original_usage = edit_info.get('original_usage', None)
         self.original_usage_version = edit_info.get('original_usage_version', None)
 
-    def __str__(self):
-        return ("EditInfo(previous_version={0.previous_version}, "
-                "update_version={0.update_version}, "
-                "source_version={0.source_version}, "
-                "edited_on={0.edited_on}, "
-                "edited_by={0.edited_by}, "
-                "original_usage={0.original_usage}, "
-                "original_usage_version={0.original_usage_version}, "
-                "_subtree_edited_on={0._subtree_edited_on}, "
-                "_subtree_edited_by={0._subtree_edited_by})").format(self)
+    def __repr__(self):
+        # pylint: disable=bad-continuation, redundant-keyword-arg
+        return ("{classname}(previous_version={self.previous_version}, "
+                "update_version={self.update_version}, "
+                "source_version={source_version}, "
+                "edited_on={self.edited_on}, "
+                "edited_by={self.edited_by}, "
+                "original_usage={self.original_usage}, "
+                "original_usage_version={self.original_usage_version}, "
+                "_subtree_edited_on={self._subtree_edited_on}, "
+                "_subtree_edited_by={self._subtree_edited_by})").format(
+            self=self,
+            classname=self.__class__.__name__,
+            source_version="UNSET" if self.source_version is None else self.source_version,
+        )  # pylint: disable=bad-continuation
 
 
 class BlockData(object):
@@ -408,13 +413,17 @@ class BlockData(object):
         # EditInfo object containing all versioning/editing data.
         self.edit_info = EditInfo(**block_data.get('edit_info', {}))
 
-    def __str__(self):
-        return ("BlockData(fields={0.fields}, "
-                "block_type={0.block_type}, "
-                "definition={0.definition}, "
-                "definition_loaded={0.definition_loaded}, "
-                "defaults={0.defaults}, "
-                "edit_info={0.edit_info})").format(self)
+    def __repr__(self):
+        # pylint: disable=bad-continuation, redundant-keyword-arg
+        return ("{classname}(fields={self.fields}, "
+                "block_type={self.block_type}, "
+                "definition={self.definition}, "
+                "definition_loaded={self.definition_loaded}, "
+                "defaults={self.defaults}, "
+                "edit_info={self.edit_info})").format(
+            self=self,
+            classname=self.__class__.__name__,
+        )  # pylint: disable=bad-continuation
 
 
 new_contract('BlockData', BlockData)

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -2831,6 +2831,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             # perhaps replace by fixing the views or Field Reference*.from_json to return a Key
             if isinstance(reference, basestring):
                 reference = BlockUsageLocator.from_string(reference)
+            elif isinstance(reference, BlockKey):
+                return reference
             return BlockKey.from_usage_key(reference)
 
         for field_name, value in fields.iteritems():

common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py

@@ -494,11 +494,14 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
                 block_id = self.DEFAULT_ROOT_LIBRARY_BLOCK_ID
             new_usage_key = course_key.make_usage_key(block_type, block_id)
 
+            # Only the course import process calls import_xblock(). If the branch setting is published_only,
+            # then the non-draft blocks are being imported.
             if self.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
-                # override existing draft (PLAT-297, PLAT-299). NOTE: this has the effect of removing
-                # any local changes w/ the import.
+                # Override any existing drafts (PLAT-297, PLAT-299). This import/publish step removes
+                # any local changes during the course import.
                 draft_course = course_key.for_branch(ModuleStoreEnum.BranchName.draft)
                 with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, draft_course):
+                    # Importing the block and publishing the block links the draft & published blocks' version history.
                     draft_block = self.import_xblock(user_id, draft_course, block_type, block_id, fields, runtime)
                     return self.publish(draft_block.location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
 

common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py

@@ -10,15 +10,19 @@ import itertools
 import mimetypes
 from unittest import skip
 from uuid import uuid4
+from contextlib import contextmanager
 
 # Mixed modulestore depends on django, so we'll manually configure some django settings
 # before importing the module
 # TODO remove this import and the configuration -- xmodule should not depend on django!
 from django.conf import settings
+# This import breaks this test file when run separately. Needs to be fixed! (PLAT-449)
 from mock_django import mock_signal_receiver
 from nose.plugins.attrib import attr
 import pymongo
 from pytz import UTC
+from shutil import rmtree
+from tempfile import mkdtemp
 
 from xmodule.x_module import XModuleMixin
 from xmodule.modulestore.edit_info import EditInfoMixin
@@ -27,6 +31,7 @@ from xmodule.modulestore.tests.test_cross_modulestore_import_export import Mongo
 from xmodule.contentstore.content import StaticContent
 from opaque_keys.edx.keys import CourseKey
 from xmodule.modulestore.xml_importer import import_course_from_xml
+from xmodule.modulestore.xml_exporter import export_course_to_xml
 from xmodule.modulestore.django import SignalHandler
 
 if not settings.configured:
@@ -49,9 +54,7 @@ from xmodule.tests import DATA_DIR, CourseComparisonTest
 log = logging.getLogger(__name__)
 
 
-@ddt.ddt
-@attr('mongo')
-class TestMixedModuleStore(CourseComparisonTest):
+class CommonMixedModuleStoreSetup(CourseComparisonTest):
     """
     Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and
     Location-based dbs)
@@ -126,7 +129,7 @@ class TestMixedModuleStore(CourseComparisonTest):
         """
         Set up the database for testing
         """
-        super(TestMixedModuleStore, self).setUp()
+        super(CommonMixedModuleStoreSetup, self).setUp()
 
         self.exclude_field(None, 'wiki_slug')
         self.exclude_field(None, 'xml_attributes')
@@ -241,6 +244,12 @@ class TestMixedModuleStore(CourseComparisonTest):
         """
         return self.course_locations[string].course_key
 
+    def _has_changes(self, location):
+        """
+        Helper function that loads the item before calling has_changes
+        """
+        return self.store.has_changes(self.store.get_item(location))
+
     # pylint: disable=dangerous-default-value
     def _initialize_mixed(self, mappings=MAPPINGS, contentstore=None):
         """
@@ -285,6 +294,13 @@ class TestMixedModuleStore(CourseComparisonTest):
         )
         self._create_course(self.course_locations[self.MONGO_COURSEID].course_key)
 
+
+@ddt.ddt
+@attr('mongo')
+class TestMixedModuleStore(CommonMixedModuleStoreSetup):
+    """
+    Tests of the MixedModulestore interface methods.
+    """
     @ddt.data('draft', 'split')
     def test_get_modulestore_type(self, default_ms):
         """
@@ -506,12 +522,6 @@ class TestMixedModuleStore(CourseComparisonTest):
         component = self.store.publish(component.location, self.user_id)
         self.assertFalse(self.store.has_changes(component))
 
-    def _has_changes(self, location):
-        """
-        Helper function that loads the item before calling has_changes
-        """
-        return self.store.has_changes(self.store.get_item(location))
-
     def setup_has_changes(self, default_ms):
         """
         Common set up for has_changes tests below.
@@ -2244,3 +2254,331 @@ class TestMixedModuleStore(CourseComparisonTest):
                         self.store.update_item(unit, self.user_id)
                         self.assertEqual(receiver.call_count, 0)
                     self.assertEqual(receiver.call_count, 0)
+
+
+@ddt.ddt
+@attr('mongo')
+class TestPublishOverExportImport(CommonMixedModuleStoreSetup):
+    """
+    Tests which publish (or don't publish) items - and then export/import the course,
+    checking the state of the imported items.
+    """
+    def setUp(self):
+        """
+        Set up the database for testing
+        """
+        super(TestPublishOverExportImport, self).setUp()
+
+        self.user_id = ModuleStoreEnum.UserID.test
+        self.export_dir = mkdtemp()
+        self.addCleanup(rmtree, self.export_dir, ignore_errors=True)
+
+    def _export_import_course_round_trip(self, modulestore, contentstore, source_course_key, export_dir):
+        """
+        Export the course from a modulestore and then re-import the course.
+        """
+        top_level_export_dir = 'exported_source_course'
+        export_course_to_xml(
+            modulestore,
+            contentstore,
+            source_course_key,
+            export_dir,
+            top_level_export_dir,
+        )
+
+        import_course_from_xml(
+            modulestore,
+            'test_user',
+            export_dir,
+            source_dirs=[top_level_export_dir],
+            static_content_store=contentstore,
+            target_id=source_course_key,
+            create_if_not_present=True,
+            raise_on_failure=True,
+        )
+
+    @contextmanager
+    def _build_store(self, default_ms):
+        """
+        Perform the modulestore-building and course creation steps for a mixed modulestore test.
+        """
+        with MongoContentstoreBuilder().build() as contentstore:
+            # initialize the mixed modulestore
+            self._initialize_mixed(contentstore=contentstore, mappings={})
+            with self.store.default_store(default_ms):
+                source_course_key = self.store.make_course_key("org.source", "course.source", "run.source")
+                self._create_course(source_course_key)
+                yield contentstore, source_course_key
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_draft_has_changes_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an unpublished unit remains with no changes across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # Create a dummy component to test against and don't publish it.
+            draft_xblock = self.store.create_item(
+                self.user_id,
+                self.course.id,
+                'vertical',
+                block_id='test_vertical'
+            )
+            # Not yet published, so changes are present
+            self.assertTrue(self._has_changes(draft_xblock.location))
+
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Verify that the imported block still is a draft, i.e. has changes.
+            self.assertTrue(self._has_changes(draft_xblock.location))
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_published_has_changes_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an published unit remains published across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # Create a dummy component to test against and publish it.
+            published_xblock = self.store.create_item(
+                self.user_id,
+                self.course.id,
+                'vertical',
+                block_id='test_vertical'
+            )
+            self.store.publish(published_xblock.location, self.user_id)
+
+            # Retrieve the published block and make sure it's published.
+            self.assertFalse(self._has_changes(published_xblock.location))
+
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Get the published xblock from the imported course.
+            # Verify that it still is published, i.e. has no changes.
+            self.assertFalse(self._has_changes(published_xblock.location))
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_changed_published_has_changes_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an published unit with an unpublished draft remains published across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # Create a dummy component to test against and publish it.
+            published_xblock = self.store.create_item(
+                self.user_id,
+                self.course.id,
+                'vertical',
+                block_id='test_vertical'
+            )
+            self.store.publish(published_xblock.location, self.user_id)
+
+            # Retrieve the published block and make sure it's published.
+            self.assertFalse(self._has_changes(published_xblock.location))
+
+            updated_display_name = 'Changed Display Name'
+            component = self.store.get_item(published_xblock.location)
+            component.display_name = updated_display_name
+            component = self.store.update_item(component, self.user_id)
+            self.assertTrue(self.store.has_changes(component))
+
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Get the published xblock from the imported course.
+            # Verify that the published block still has a draft block, i.e. has changes.
+            self.assertTrue(self._has_changes(published_xblock.location))
+
+            # Verify that the changes in the draft vertical still exist.
+            with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, source_course_key):
+                component = self.store.get_item(published_xblock.location)
+                self.assertEqual(component.display_name, updated_display_name)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_seq_with_unpublished_vertical_has_changes_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an published unit with an unpublished draft remains published across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # create chapter
+            chapter = self.store.create_child(
+                self.user_id, self.course.location, 'chapter', block_id='section_one'
+            )
+            self.store.publish(chapter.location, self.user_id)
+
+            # create sequential
+            sequential = self.store.create_child(
+                self.user_id, chapter.location, 'sequential', block_id='subsection_one'
+            )
+            self.store.publish(sequential.location, self.user_id)
+
+            # create vertical - don't publish it!
+            vertical = self.store.create_child(
+                self.user_id, sequential.location, 'vertical', block_id='moon_unit'
+            )
+
+            # Retrieve the published block and make sure it's published.
+            # Chapter is published - but the changes in vertical below means it "has_changes".
+            self.assertTrue(self._has_changes(chapter.location))
+            # Sequential is published - but the changes in vertical below means it "has_changes".
+            self.assertTrue(self._has_changes(sequential.location))
+            # Vertical is unpublished - so it "has_changes".
+            self.assertTrue(self._has_changes(vertical.location))
+
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Get the published xblock from the imported course.
+            # Verify that the published block still has a draft block, i.e. has changes.
+            self.assertTrue(self._has_changes(chapter.location))
+            self.assertTrue(self._has_changes(sequential.location))
+            self.assertTrue(self._has_changes(vertical.location))
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_vertical_with_draft_and_published_unit_has_changes_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an published unit with an unpublished draft remains published across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # create chapter
+            chapter = self.store.create_child(
+                self.user_id, self.course.location, 'chapter', block_id='section_one'
+            )
+            self.store.publish(chapter.location, self.user_id)
+
+            # create sequential
+            sequential = self.store.create_child(
+                self.user_id, chapter.location, 'sequential', block_id='subsection_one'
+            )
+            self.store.publish(sequential.location, self.user_id)
+
+            # create vertical
+            vertical = self.store.create_child(
+                self.user_id, sequential.location, 'vertical', block_id='moon_unit'
+            )
+            # Vertical has changes until it is actually published.
+            self.assertTrue(self._has_changes(vertical.location))
+            self.store.publish(vertical.location, self.user_id)
+            self.assertFalse(self._has_changes(vertical.location))
+
+            # create unit
+            unit = self.store.create_child(
+                self.user_id, vertical.location, 'html', block_id='html_unit'
+            )
+            # Vertical has a new child -and- unit is unpublished. So both have changes.
+            self.assertTrue(self._has_changes(vertical.location))
+            self.assertTrue(self._has_changes(unit.location))
+
+            # Publishing the vertical also publishes its unit child.
+            self.store.publish(vertical.location, self.user_id)
+            self.assertFalse(self._has_changes(vertical.location))
+            self.assertFalse(self._has_changes(unit.location))
+
+            # Publishing the unit separately has no effect on whether it has changes - it's already published.
+            self.store.publish(unit.location, self.user_id)
+            self.assertFalse(self._has_changes(vertical.location))
+            self.assertFalse(self._has_changes(unit.location))
+
+            # Retrieve the published block and make sure it's published.
+            self.store.publish(chapter.location, self.user_id)
+            self.assertFalse(self._has_changes(chapter.location))
+            self.assertFalse(self._has_changes(sequential.location))
+            self.assertFalse(self._has_changes(vertical.location))
+            self.assertFalse(self._has_changes(unit.location))
+
+            # Now make changes to the unit - but don't publish them.
+            component = self.store.get_item(unit.location)
+            updated_display_name = 'Changed Display Name'
+            component.display_name = updated_display_name
+            component = self.store.update_item(component, self.user_id)
+            self.assertTrue(self._has_changes(component.location))
+
+            # Export the course - then import the course export.
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Get the published xblock from the imported course.
+            # Verify that the published block still has a draft block, i.e. has changes.
+            self.assertTrue(self._has_changes(chapter.location))
+            self.assertTrue(self._has_changes(sequential.location))
+            self.assertTrue(self._has_changes(vertical.location))
+            self.assertTrue(self._has_changes(unit.location))
+
+            # Verify that the changes in the draft unit still exist.
+            with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, source_course_key):
+                component = self.store.get_item(unit.location)
+                self.assertEqual(component.display_name, updated_display_name)
+
+            # Verify that the draft changes don't exist in the published unit - it still uses the default name.
+            with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, source_course_key):
+                component = self.store.get_item(unit.location)
+                self.assertEqual(component.display_name, 'Text')
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_vertical_with_published_unit_remains_published_before_export_and_after_import(self, default_ms):
+        """
+        Tests that an published unit remains published across export and re-import.
+        """
+        with self._build_store(default_ms) as (contentstore, source_course_key):
+
+            # create chapter
+            chapter = self.store.create_child(
+                self.user_id, self.course.location, 'chapter', block_id='section_one'
+            )
+            self.store.publish(chapter.location, self.user_id)
+
+            # create sequential
+            sequential = self.store.create_child(
+                self.user_id, chapter.location, 'sequential', block_id='subsection_one'
+            )
+            self.store.publish(sequential.location, self.user_id)
+
+            # create vertical
+            vertical = self.store.create_child(
+                self.user_id, sequential.location, 'vertical', block_id='moon_unit'
+            )
+            # Vertical has changes until it is actually published.
+            self.assertTrue(self._has_changes(vertical.location))
+            self.store.publish(vertical.location, self.user_id)
+            self.assertFalse(self._has_changes(vertical.location))
+
+            # create unit
+            unit = self.store.create_child(
+                self.user_id, vertical.location, 'html', block_id='html_unit'
+            )
+            # Now make changes to the unit.
+            updated_display_name = 'Changed Display Name'
+            unit.display_name = updated_display_name
+            unit = self.store.update_item(unit, self.user_id)
+            self.assertTrue(self._has_changes(unit.location))
+
+            # Publishing the vertical also publishes its unit child.
+            self.store.publish(vertical.location, self.user_id)
+            self.assertFalse(self._has_changes(vertical.location))
+            self.assertFalse(self._has_changes(unit.location))
+
+            # Export the course - then import the course export.
+            self._export_import_course_round_trip(
+                self.store, contentstore, source_course_key, self.export_dir
+            )
+
+            # Get the published xblock from the imported course.
+            # Verify that the published block still has a draft block, i.e. has changes.
+            self.assertFalse(self._has_changes(chapter.location))
+            self.assertFalse(self._has_changes(sequential.location))
+            self.assertFalse(self._has_changes(vertical.location))
+            self.assertFalse(self._has_changes(unit.location))
+
+            # Verify that the published changes exist in the published unit.
+            with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, source_course_key):
+                component = self.store.get_item(unit.location)
+                self.assertEqual(component.display_name, updated_display_name)

common/lib/xmodule/xmodule/modulestore/tests/test_xml_importer.py

@@ -9,7 +9,7 @@ from xmodule.x_module import XModuleMixin
 from opaque_keys.edx.locations import Location
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.inheritance import InheritanceMixin
-from xmodule.modulestore.xml_importer import _import_module_and_update_references
+from xmodule.modulestore.xml_importer import _update_and_import_module
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from xmodule.tests import DATA_DIR
@@ -144,7 +144,7 @@ class RemapNamespaceTest(ModuleStoreNoSettings):
 
         # Move to different runtime w/ different course id
         target_location_namespace = SlashSeparatedCourseKey("org", "course", "run")
-        new_version = _import_module_and_update_references(
+        new_version = _update_and_import_module(
             self.xblock,
             modulestore(),
             999,
@@ -181,7 +181,7 @@ class RemapNamespaceTest(ModuleStoreNoSettings):
 
         # Remap the namespace
         target_location_namespace = Location("org", "course", "run", "category", "stubxblock")
-        new_version = _import_module_and_update_references(
+        new_version = _update_and_import_module(
             self.xblock,
             modulestore(),
             999,
@@ -213,7 +213,7 @@ class RemapNamespaceTest(ModuleStoreNoSettings):
 
         # Remap the namespace
         target_location_namespace = Location("org", "course", "run", "category", "stubxblock")
-        new_version = _import_module_and_update_references(
+        new_version = _update_and_import_module(
             self.xblock,
             modulestore(),
             999,

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -42,6 +42,9 @@ def _export_drafts(modulestore, course_key, export_fs, xml_centric_course_key):
             qualifiers={'category': {'$nin': DIRECT_ONLY_CATEGORIES}},
             revision=ModuleStoreEnum.RevisionOption.draft_only
         )
+        # Check to see if the returned draft modules have changes w.r.t. the published module.
+        # Only modules with changes will be exported into the /drafts directory.
+        draft_modules = [module for module in draft_modules if modulestore.has_changes(module)]
 
         if draft_modules:
             draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
@@ -153,20 +156,15 @@ class ExportManager(object):
         Perform the export given the parameters handed to this class at init.
         """
         with self.modulestore.bulk_operations(self.courselike_key):
-            # depth = None: Traverses down the entire course structure.
-            # lazy = False: Loads and caches all block definitions during traversal for fast access later
-            #               -and- to eliminate many round-trips to read individual definitions.
-            # Why these parameters? Because a course export needs to access all the course block information
-            # eventually. Accessing it all now at the beginning increases performance of the export.
-            fsm = OSFS(self.root_dir)
-            courselike = self.get_courselike()
-            export_fs = courselike.runtime.export_fs = fsm.makeopendir(self.target_dir)
-            root_courselike_dir = self.root_dir + '/' + self.target_dir
 
+            fsm = OSFS(self.root_dir)
             root = lxml.etree.Element('unknown')  # pylint: disable=no-member
 
             # export only the published content
             with self.modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, self.courselike_key):
+                courselike = self.get_courselike()
+                export_fs = courselike.runtime.export_fs = fsm.makeopendir(self.target_dir)
+
                 # change all of the references inside the course to use the xml expected key type w/o version & branch
                 xml_centric_courselike_key = self.get_key()
                 adapt_references(courselike, xml_centric_courselike_key, export_fs)
@@ -176,6 +174,7 @@ class ExportManager(object):
             self.process_root(root, export_fs)
 
             # Process extra items-- drafts, assets, etc
+            root_courselike_dir = self.root_dir + '/' + self.target_dir
             self.process_extra(root, courselike, root_courselike_dir, xml_centric_courselike_key, export_fs)
 
             # Any last pass adjustments
@@ -192,6 +191,11 @@ class CourseExportManager(ExportManager):
         )
 
     def get_courselike(self):
+        # depth = None: Traverses down the entire course structure.
+        # lazy = False: Loads and caches all block definitions during traversal for fast access later
+        #               -and- to eliminate many round-trips to read individual definitions.
+        # Why these parameters? Because a course export needs to access all the course block information
+        # eventually. Accessing it all now at the beginning increases performance of the export.
         return self.modulestore.get_course(self.courselike_key, depth=None, lazy=False)
 
     def process_root(self, root, export_fs):

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -316,7 +316,7 @@ class ImportManager(object):
         log.debug('course data_dir=%s', source_courselike.data_dir)
 
         with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, dest_id):
-            course = _import_module_and_update_references(
+            course = _update_and_import_module(
                 source_courselike, self.store, self.user_id,
                 courselike_key,
                 dest_id,
@@ -352,12 +352,19 @@ class ImportManager(object):
         raise NotImplementedError
 
     @abstractmethod
-    def import_children(self, source_courselike, courselike, courselike_key, data_path, dest_id):
+    def import_children(self, source_courselike, courselike, courselike_key, dest_id):
         """
         To be overloaded with a method that installs the child items into self.store.
         """
         raise NotImplementedError
 
+    @abstractmethod
+    def import_drafts(self, courselike, courselike_key, data_path, dest_id):
+        """
+        To be overloaded with a method that installs the draft items into self.store.
+        """
+        raise NotImplementedError
+
     def recursive_build(self, source_courselike, courselike, courselike_key, dest_id):
         """
         Recursively imports all child blocks from the temporary modulestore into the
@@ -381,7 +388,7 @@ class ImportManager(object):
                     if self.verbose:
                         log.debug('importing module location %s', child.location)
 
-                    _import_module_and_update_references(
+                    _update_and_import_module(
                         child,
                         self.store,
                         self.user_id,
@@ -399,7 +406,7 @@ class ImportManager(object):
             if self.verbose:
                 log.debug('importing module location %s', leftover)
 
-            _import_module_and_update_references(
+            _update_and_import_module(
                 self.xml_module_store.get_item(leftover),
                 self.store,
                 self.user_id,
@@ -419,8 +426,12 @@ class ImportManager(object):
                 dest_id, runtime = self.get_dest_id(courselike_key)
             except DuplicateCourseError:
                 continue
+
+            # This bulk operation wraps all the operations to populate the published branch.
             with self.store.bulk_operations(dest_id):
+                # Retrieve the course itself.
                 source_courselike, courselike, data_path = self.get_courselike(courselike_key, runtime, dest_id)
+
                 # Import all static pieces.
                 self.import_static(data_path, dest_id)
 
@@ -428,7 +439,17 @@ class ImportManager(object):
                 self.import_asset_metadata(data_path, dest_id)
 
                 # Import all children
-                self.import_children(source_courselike, courselike, courselike_key, data_path, dest_id)
+                self.import_children(source_courselike, courselike, courselike_key, dest_id)
+
+            # This bulk operation wraps all the operations to populate the draft branch with any items
+            # from the /drafts subdirectory.
+            # Drafts must be imported in a separate bulk operation from published items to import properly,
+            # due to the recursive_build() above creating a draft item for each course block
+            # and then publishing it.
+            with self.store.bulk_operations(dest_id):
+                # Import all draft items into the courselike.
+                courselike = self.import_drafts(courselike, courselike_key, data_path, dest_id)
+
             yield courselike
 
 
@@ -520,13 +541,19 @@ class CourseImportManager(ImportManager):
         if course.tabs is None or len(course.tabs) == 0:
             CourseTabList.initialize_default(course)
 
-    def import_children(self, source_courselike, courselike, courselike_key, data_path, dest_id):
+    def import_children(self, source_courselike, courselike, courselike_key, dest_id):
         """
         Imports all children into the desired store.
         """
+        # The branch setting of published_only forces an overwrite of all draft modules
+        # during the course import.
         with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, dest_id):
             self.recursive_build(source_courselike, courselike, courselike_key, dest_id)
 
+    def import_drafts(self, courselike, courselike_key, data_path, dest_id):
+        """
+        Imports all drafts into the desired store.
+        """
         # Import any draft items
         with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, dest_id):
             _import_course_draft(
@@ -539,6 +566,11 @@ class CourseImportManager(ImportManager):
                 courselike.runtime
             )
 
+        # Importing the drafts potentially triggered a new structure version.
+        # If so, the HEAD version_guid of the passed-in courselike will be out-of-date.
+        # Fetch the course to return the most recent course version.
+        return self.store.get_course(courselike.id.replace(branch=None, version_guid=None))
+
 
 class LibraryImportManager(ImportManager):
     """
@@ -597,12 +629,18 @@ class LibraryImportManager(ImportManager):
         """
         pass
 
-    def import_children(self, source_courselike, courselike, courselike_key, data_path, dest_id):
+    def import_children(self, source_courselike, courselike, courselike_key, dest_id):
         """
         Imports all children into the desired store.
         """
         self.recursive_build(source_courselike, courselike, courselike_key, dest_id)
 
+    def import_drafts(self, courselike, courselike_key, data_path, dest_id):
+        """
+        Imports all drafts into the desired store.
+        """
+        return courselike
+
 
 def import_course_from_xml(*args, **kwargs):
     """
@@ -620,24 +658,21 @@ def import_library_from_xml(*args, **kwargs):
     return list(manager.run_imports())
 
 
-def _import_module_and_update_references(
+def _update_and_import_module(
         module, store, user_id,
         source_course_id, dest_course_id,
         do_import_static=True, runtime=None):
-
+    """
+    Update all the module reference fields to the destination course id,
+    then import the module into the destination course.
+    """
     logging.debug(u'processing import of module %s...', unicode(module.location))
 
-    if do_import_static and 'data' in module.fields and isinstance(module.fields['data'], xblock.fields.String):
-        # we want to convert all 'non-portable' links in the module_data
-        # (if it is a string) to portable strings (e.g. /static/)
-        module.data = rewrite_nonportable_content_links(
-            source_course_id,
-            dest_course_id,
-            module.data
-        )
-
-    # Move the module to a new course
-    def _convert_reference_fields_to_new_namespace(reference):
+    def _update_module_references(module, source_course_id, dest_course_id):
+        """
+        Move the module to a new course.
+        """
+        def _convert_ref_fields_to_new_namespace(reference):  # pylint: disable=invalid-name
             """
             Convert a reference to the new namespace, but only
             if the original namespace matched the original course.
@@ -658,14 +693,14 @@ def _import_module_and_update_references(
                     if value is None:
                         fields[field_name] = None
                     else:
-                    fields[field_name] = _convert_reference_fields_to_new_namespace(field.read_from(module))
+                        fields[field_name] = _convert_ref_fields_to_new_namespace(field.read_from(module))
                 elif isinstance(field, ReferenceList):
                     references = field.read_from(module)
-                fields[field_name] = [_convert_reference_fields_to_new_namespace(reference) for reference in references]
+                    fields[field_name] = [_convert_ref_fields_to_new_namespace(reference) for reference in references]
                 elif isinstance(field, ReferenceValueDict):
                     reference_dict = field.read_from(module)
                     fields[field_name] = {
-                    key: _convert_reference_fields_to_new_namespace(reference)
+                        key: _convert_ref_fields_to_new_namespace(reference)
                         for key, reference
                         in reference_dict.iteritems()
                     }
@@ -683,6 +718,18 @@ def _import_module_and_update_references(
                     fields[field_name] = value
                 else:
                     fields[field_name] = field.read_from(module)
+        return fields
+
+    if do_import_static and 'data' in module.fields and isinstance(module.fields['data'], xblock.fields.String):
+        # we want to convert all 'non-portable' links in the module_data
+        # (if it is a string) to portable strings (e.g. /static/)
+        module.data = rewrite_nonportable_content_links(
+            source_course_id,
+            dest_course_id,
+            module.data
+        )
+
+    fields = _update_module_references(module, source_course_id, dest_course_id)
 
     return store.import_xblock(
         user_id, dest_course_id, module.location.category,
@@ -699,14 +746,13 @@ def _import_course_draft(
         target_id,
         mongo_runtime
 ):
-    '''
-    This will import all the content inside of the 'drafts' folder, if it exists
-    NOTE: This is not a full course import, basically in our current
-    application only verticals (and downwards) can be in draft.
-    Therefore, we need to use slightly different call points into
-    the import process_xml as we can't simply call XMLModuleStore() constructor
-    (like we do for importing public content)
-    '''
+    """
+    This method will import all the content inside of the 'drafts' folder, if content exists.
+    NOTE: This is not a full course import! In our current application, only verticals
+    (and blocks beneath) can be in draft. Therefore, different call points into the import
+    process_xml are used as the XMLModuleStore() constructor cannot simply be called
+    (as is done for importing public content).
+    """
     draft_dir = course_data_path + "/drafts"
     if not os.path.exists(draft_dir):
         return
@@ -720,7 +766,9 @@ def _import_course_draft(
     # Whether or not data_dir ends with a "/" differs in production vs. test.
     if not data_dir.endswith("/"):
         data_dir += "/"
+    # Remove absolute path, leaving relative <course_name>/drafts.
     draft_course_dir = draft_dir.replace(data_dir, '', 1)
+
     system = ImportSystem(
         xmlstore=xml_module_store,
         course_id=source_course_id,
@@ -761,7 +809,7 @@ def _import_course_draft(
                 parent.children.insert(index, non_draft_location)
                 store.update_item(parent, user_id)
 
-        _import_module_and_update_references(
+        _update_and_import_module(
             module, store, user_id,
             source_course_id,
             target_id,
@@ -770,52 +818,23 @@ def _import_course_draft(
         for child in module.get_children():
             _import_module(child)
 
-    # Now walk the /vertical directory.
+    # Now walk the /drafts directory.
     # Each file in the directory will be a draft copy of the vertical.
 
     # First it is necessary to order the draft items by their desired index in the child list,
     # since the order in which os.walk() returns the files is not guaranteed.
     drafts = []
-    for dirname, _dirnames, filenames in os.walk(draft_dir):
+    for rootdir, __, filenames in os.walk(draft_dir):
         for filename in filenames:
-            module_path = os.path.join(dirname, filename)
-            with open(module_path, 'r') as f:
-                try:
-                    # note, on local dev it seems like OSX will put
-                    # some extra files in the directory with "quarantine"
-                    # information. These files are binary files and will
-                    # throw exceptions when we try to parse the file
-                    # as an XML string. Let's make sure we're
-                    # dealing with a string before ingesting
-                    data = f.read()
-
-                    try:
-                        xml = data.decode('utf-8')
-                    except UnicodeDecodeError, err:
-                        # seems like on OSX localdev, the OS is making
-                        # quarantine files in the unzip directory
-                        # when importing courses so if we blindly try to
-                        # enumerate through the directory, we'll try
-                        # to process a bunch of binary quarantine files
-                        # (which are prefixed with a '._' character which
-                        # will dump a bunch of exceptions to the output,
-                        # although they are harmless.
-                        #
-                        # Reading online docs there doesn't seem to be
-                        # a good means to detect a 'hidden' file that works
-                        # well across all OS environments. So for now, I'm using
-                        # OSX's utilization of a leading '.' in the filename
-                        # to indicate a system hidden file.
-                        #
-                        # Better yet would be a way to figure out if this is
-                        # a binary file, but I haven't found a good way
-                        # to do this yet.
             if filename.startswith('._'):
+                # Skip any OSX quarantine files, prefixed with a '._'.
                 continue
-                        # Not a 'hidden file', then re-raise exception
-                        raise err
+            module_path = os.path.join(rootdir, filename)
+            with open(module_path, 'r') as f:
+                try:
+                    xml = f.read().decode('utf-8')
 
-                    # process_xml call below recursively processes all descendants. If
+                    # The process_xml() call below recursively processes all descendants. If
                     # we call this on all verticals in a course with verticals nested below
                     # the unit level, we try to import the same content twice, causing naming conflicts.
                     # Therefore only process verticals at the unit level, assuming that any other
@@ -838,13 +857,12 @@ def _import_course_draft(
                         draft = draft_node_constructor(
                             module=descriptor, url=draft_url, parent_url=parent_url, index=index
                         )
-
                         drafts.append(draft)
 
                 except Exception:  # pylint: disable=broad-except
-                    logging.exception('Error while parsing course xml.')
+                    logging.exception('Error while parsing course drafts xml.')
 
-    # sort drafts by `index_in_children_list` attribute
+    # Sort drafts by `index_in_children_list` attribute.
     drafts.sort(key=lambda x: x.index)
 
     for draft in get_draft_subtree_roots(drafts):
@@ -864,11 +882,11 @@ def allowed_metadata_by_category(category):
 
 
 def check_module_metadata_editability(module):
-    '''
+    """
     Assert that there is no metadata within a particular module that
     we can't support editing. However we always allow 'display_name'
     and 'xml_attributes'
-    '''
+    """
     allowed = allowed_metadata_by_category(module.location.category)
     if '*' in allowed:
         # everything is allowed