Merge pull request #4878 from edx/dhm/split_branch_agnostic

Make it branch agnostic

cms/djangoapps/contentstore/tests/test_clone_course.py

@@ -28,14 +28,14 @@ class CloneCourseTest(CourseTestCase):
         # 3. clone course (mongo -> split)
         with self.store.default_store(ModuleStoreEnum.Type.split):
             split_course3_id = CourseLocator(
-                org="edx3", course="split3", run="2013_Fall", branch=ModuleStoreEnum.BranchName.draft
+                org="edx3", course="split3", run="2013_Fall"
             )
             self.store.clone_course(mongo_course2_id, split_course3_id, self.user.id)
             self.assertCoursesEqual(mongo_course2_id, split_course3_id)
 
             # 4. clone course (split -> split)
             split_course4_id = CourseLocator(
-                org="edx4", course="split4", run="2013_Fall", branch=ModuleStoreEnum.BranchName.draft
+                org="edx4", course="split4", run="2013_Fall"
             )
             self.store.clone_course(split_course3_id, split_course4_id, self.user.id)
             self.assertCoursesEqual(split_course3_id, split_course4_id)

cms/djangoapps/contentstore/tests/utils.py

@@ -257,13 +257,14 @@ class CourseTestCase(ModuleStoreTestCase):
             )
 
         for course1_item in course1_items:
-            course2_item_location = course1_item.location.map_into_course(course2_id)
-            if course1_item.location.category == 'course':
+            course1_item_loc = course1_item.location
+            course2_item_loc = course2_id.make_usage_key(course1_item_loc.block_type, course1_item_loc.block_id)
+            if course1_item_loc.block_type == 'course':
                 # mongo uses the run as the name, split uses 'course'
                 store = self.store._get_modulestore_for_courseid(course2_id)  # pylint: disable=protected-access
-                new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_location.run
-                course2_item_location = course2_item_location.replace(name=new_name)
-            course2_item = self.store.get_item(course2_item_location)
+                new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_loc.run
+                course2_item_loc = course2_item_loc.replace(name=new_name)
+            course2_item = self.store.get_item(course2_item_loc)
 
             try:
                 # compare published state
@@ -278,7 +279,7 @@ class CourseTestCase(ModuleStoreTestCase):
                     c1_state,
                     c2_state,
                     "Publish states not equal: course item {} in state {} != course item {} in state {}".format(
-                        course1_item.location, c1_state, course2_item.location, c2_state
+                        course1_item_loc, c1_state, course2_item.location, c2_state
                     )
                 )
 
@@ -296,11 +297,9 @@ class CourseTestCase(ModuleStoreTestCase):
                 expected_children = []
                 for course1_item_child in course1_item.children:
                     expected_children.append(
-                        course1_item_child.map_into_course(course2_id)
+                        course2_id.make_usage_key(course1_item_child.block_type, course1_item_child.block_id)
                     )
-                # also process course2_children just in case they have version guids
-                course2_children = [child.version_agnostic() for child in course2_item.children]
-                self.assertEqual(expected_children, course2_children)
+                self.assertEqual(expected_children, course2_item.children)
 
         # compare assets
         content_store = self.store.contentstore

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -46,7 +46,7 @@ def strip_key(func):
 
         # remove version and branch, by default
         rem_vers = kwargs.pop('remove_version', True)
-        rem_branch = kwargs.pop('remove_branch', False)
+        rem_branch = kwargs.pop('remove_branch', True)
 
         # helper function for stripping individual values
         def strip_key_func(val):

common/lib/xmodule/xmodule/modulestore/search.py

@@ -95,10 +95,10 @@ def path_to_location(modulestore, usage_key):
             category = path[path_index].block_type
             if category == 'sequential' or category == 'videosequence':
                 section_desc = modulestore.get_item(path[path_index])
-                child_locs = [c.location.version_agnostic() for c in section_desc.get_children()]
+                child_locs = [c.location.block_id for c in section_desc.get_children()]
                 # positions are 1-indexed, and should be strings to be consistent with
                 # url parsing.
-                position_list.append(str(child_locs.index(path[path_index + 1]) + 1))
+                position_list.append(str(child_locs.index(path[path_index + 1].block_id) + 1))
         position = "_".join(position_list)
 
     return (course_id, chapter, section, position)

common/lib/xmodule/xmodule/modulestore/split_migrator.py

@@ -168,17 +168,16 @@ class SplitMigrator(object):
             )
             new_parent = self.split_modulestore.get_item(split_parent_loc, **kwargs)
             # this only occurs if the parent was also awaiting adoption: skip this one, go to next
-            if any(new_locator == child.version_agnostic() for child in new_parent.children):
+            if any(new_locator.block_id == child.block_id for child in new_parent.children):
                 continue
             # find index for module: new_parent may be missing quite a few of old_parent's children
             new_parent_cursor = 0
             for old_child_loc in old_parent.children:
-                if old_child_loc == draft_location:
+                if old_child_loc.block_id == draft_location.block_id:
                     break  # moved cursor enough, insert it here
-                sibling_loc = new_draft_course_loc.make_usage_key(old_child_loc.category, old_child_loc.block_id)
                 # sibling may move cursor
                 for idx in range(new_parent_cursor, len(new_parent.children)):
-                    if new_parent.children[idx].version_agnostic() == sibling_loc:
+                    if new_parent.children[idx].block_id == old_child_loc.block_id:
                         new_parent_cursor = idx + 1
                         break  # skipped sibs enough, pick back up scan
             new_parent.children.insert(new_parent_cursor, new_locator)

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

@@ -52,6 +52,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         course_id = self._map_revision_to_branch(course_id)
         return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs)
 
+    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, revision=None, **kwargs):
+        """
+        See :py:meth: xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.clone_course
+        """
+        dest_course_id = self._map_revision_to_branch(dest_course_id, revision=revision)
+        return super(DraftVersioningModuleStore, self).clone_course(
+            source_course_id, dest_course_id, user_id, fields=fields, **kwargs
+        )
+
     def get_courses(self, **kwargs):
         """
         Returns all the courses on the Draft or Published branch depending on the branch setting.
@@ -76,6 +85,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
 
     def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs):
+        descriptor.location = self._map_revision_to_branch(descriptor.location)
         item = super(DraftVersioningModuleStore, self).update_item(
             descriptor,
             user_id,

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

@@ -472,7 +472,6 @@ class TestMixedModuleStore(unittest.TestCase):
         )
 
         # verify pre delete state (just to verify that the test is valid)
-        self.assertTrue(hasattr(private_vert, 'is_draft') or private_vert.location.branch == ModuleStoreEnum.BranchName.draft)
         if hasattr(private_vert.location, 'version_guid'):
             # change to the HEAD version
             vert_loc = private_vert.location.for_version(private_leaf.location.version_guid)
@@ -699,20 +698,22 @@ class TestMixedModuleStore(unittest.TestCase):
         Make sure that path_to_location works
         """
         self.initdb(default_ms)
-        self._create_block_hierarchy()
 
         course_key = self.course_locations[self.MONGO_COURSEID].course_key
-        should_work = (
-            (self.problem_x1a_2,
-             (course_key, u"Chapter_x", u"Sequential_x1", '1')),
-            (self.chapter_x,
-             (course_key, "Chapter_x", None, None)),
-        )
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            self._create_block_hierarchy()
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        for location, expected in should_work:
-            with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends):
-                self.assertEqual(path_to_location(self.store, location), expected)
+            should_work = (
+                (self.problem_x1a_2,
+                 (course_key, u"Chapter_x", u"Sequential_x1", '1')),
+                (self.chapter_x,
+                 (course_key, "Chapter_x", None, None)),
+            )
+
+            mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
+            for location, expected in should_work:
+                with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends):
+                    self.assertEqual(path_to_location(self.store, location), expected)
 
         not_found = (
             course_key.make_usage_key('video', 'WelcomeX'),

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

@@ -104,40 +104,44 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir):
         policy = {'course/' + course.location.name: own_metadata(course)}
         course_policy.write(dumps(policy, cls=EdxJSONEncoder))
 
-    # NOTE: this code assumes that verticals are the top most draftable container
-    # should we change the application, then this assumption will no longer be valid
-    # NOTE: we need to explicitly implement the logic for setting the vertical's parent
-    # and index here since the XML modulestore cannot load draft modules
-    draft_verticals = modulestore.get_items(
-        course_key,
-        qualifiers={'category': 'vertical'},
-        revision=ModuleStoreEnum.RevisionOption.draft_only
-    )
-    if len(draft_verticals) > 0:
-        draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
-        for draft_vertical in draft_verticals:
-            parent_loc = modulestore.get_parent_location(
-                draft_vertical.location,
-                revision=ModuleStoreEnum.RevisionOption.draft_preferred
+    #### DRAFTS ####
+    # xml backed courses don't support drafts!
+    if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml:
+        # NOTE: this code assumes that verticals are the top most draftable container
+        # should we change the application, then this assumption will no longer be valid
+        # NOTE: we need to explicitly implement the logic for setting the vertical's parent
+        # and index here since the XML modulestore cannot load draft modules
+        with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+            draft_verticals = modulestore.get_items(
+                course_key,
+                qualifiers={'category': 'vertical'},
+                revision=ModuleStoreEnum.RevisionOption.draft_only
             )
-            # Don't try to export orphaned items.
-            if parent_loc is not None:
-                logging.debug('parent_loc = {0}'.format(parent_loc))
-                if parent_loc.category in DIRECT_ONLY_CATEGORIES:
-                    draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string()
-                    sequential = modulestore.get_item(parent_loc)
-                    index = sequential.children.index(draft_vertical.location)
-                    draft_vertical.xml_attributes['index_in_children_list'] = str(index)
-                draft_vertical.runtime.export_fs = draft_course_dir
-                adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir)
-                node = lxml.etree.Element('unknown')
-                draft_vertical.add_xml_to_node(node)
+
+            if len(draft_verticals) > 0:
+                draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
+                for draft_vertical in draft_verticals:
+                    parent_loc = modulestore.get_parent_location(
+                        draft_vertical.location,
+                        revision=ModuleStoreEnum.RevisionOption.draft_preferred
+                    )
+                    # Don't try to export orphaned items.
+                    if parent_loc is not None:
+                        logging.debug('parent_loc = {0}'.format(parent_loc))
+                        if parent_loc.category in DIRECT_ONLY_CATEGORIES:
+                            draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string()
+                            sequential = modulestore.get_item(parent_loc)
+                            index = sequential.children.index(draft_vertical.location)
+                            draft_vertical.xml_attributes['index_in_children_list'] = str(index)
+                        draft_vertical.runtime.export_fs = draft_course_dir
+                        adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir)
+                        node = lxml.etree.Element('unknown')
+                        draft_vertical.add_xml_to_node(node)
 
 
 def adapt_references(subtree, destination_course_key, export_fs):
     """
-    Map every reference in the subtree into destination_course_key and set it back into the xblock fields.
-    Make sure every runtime knows where the export_fs is.
+    Map every reference in the subtree into destination_course_key and set it back into the xblock fields
     """
     subtree.runtime.export_fs = export_fs  # ensure everything knows where it's going!
     for field_name, field in subtree.fields.iteritems():
@@ -162,6 +166,7 @@ def adapt_references(subtree, destination_course_key, export_fs):
                 )
 
 
+
 def _export_field_content(xblock_item, item_dir):
     """
     Export all fields related to 'xblock_item' other than 'metadata' and 'data' to json file in provided directory

common/lib/xmodule/xmodule/tests/__init__.py

@@ -212,23 +212,27 @@ class CourseComparisonTest(unittest.TestCase):
         will be ignored for the purpose of equality checking.
         """
         # compare published
-        expected_items = expected_store.get_items(expected_course_key, revision=ModuleStoreEnum.RevisionOption.published_only)
-        actual_items = actual_store.get_items(actual_course_key, revision=ModuleStoreEnum.RevisionOption.published_only)
-        self.assertGreater(len(expected_items), 0)
-        self._assertCoursesEqual(expected_items, actual_items, actual_course_key)
-
-        # compare draft
-        if expected_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split:
-            revision = ModuleStoreEnum.RevisionOption.draft_only
-        else:
-            revision = None
-        expected_items = expected_store.get_items(expected_course_key, revision=revision)
-        if actual_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split:
-            revision = ModuleStoreEnum.RevisionOption.draft_only
-        else:
-            revision = None
-        actual_items = actual_store.get_items(actual_course_key, revision=revision)
-        self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True)
+        with expected_store.branch_setting(ModuleStoreEnum.Branch.published_only, expected_course_key):
+            with actual_store.branch_setting(ModuleStoreEnum.Branch.published_only, actual_course_key):
+                expected_items = expected_store.get_items(expected_course_key, revision=ModuleStoreEnum.RevisionOption.published_only)
+                actual_items = actual_store.get_items(actual_course_key, revision=ModuleStoreEnum.RevisionOption.published_only)
+                self.assertGreater(len(expected_items), 0)
+                self._assertCoursesEqual(expected_items, actual_items, actual_course_key)
+
+        with expected_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, expected_course_key):
+            with actual_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, actual_course_key):
+                # compare draft
+                if expected_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split:
+                    revision = ModuleStoreEnum.RevisionOption.draft_only
+                else:
+                    revision = None
+                expected_items = expected_store.get_items(expected_course_key, revision=revision)
+                if actual_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split:
+                    revision = ModuleStoreEnum.RevisionOption.draft_only
+                else:
+                    revision = None
+                actual_items = actual_store.get_items(actual_course_key, revision=revision)
+                self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True)
 
     def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key, expect_drafts=False):
         self.assertEqual(len(expected_items), len(actual_items))
@@ -282,18 +286,15 @@ class CourseComparisonTest(unittest.TestCase):
             # compare children
             self.assertEqual(expected_item.has_children, actual_item.has_children)
             if expected_item.has_children:
-                actual_course_key = actual_item.location.course_key.version_agnostic()
                 expected_children = [
-                    course1_item_child.location.map_into_course(actual_course_key)
+                    (course1_item_child.location.block_type, course1_item_child.location.block_id)
+                    # get_children() rather than children to strip privates from public parents
                     for course1_item_child in expected_item.get_children()
-                    # get_children was returning drafts for published parents :-(
-                    if expect_drafts or not getattr(course1_item_child, 'is_draft', False)
                 ]
                 actual_children = [
-                    item_child.location.version_agnostic()
+                    (item_child.location.block_type, item_child.location.block_id)
+                    # get_children() rather than children to strip privates from public parents
                     for item_child in actual_item.get_children()
-                    # get_children was returning drafts for published parents :-(
-                    if expect_drafts or not getattr(item_child, 'is_draft', False)
                 ]
                 self.assertEqual(expected_children, actual_children)