Merge pull request #6599 from edx/release

Release

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

@@ -1061,13 +1061,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             raise ItemNotFoundError(locator)
 
         course = self._lookup_course(locator.course_key)
-        parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course.structure)
-        if parent_id is None:
+        parent_ids = self._get_parents_from_structure(BlockKey.from_usage_key(locator), course.structure)
+        if len(parent_ids) == 0:
             return None
+        # find alphabetically least
+        parent_ids.sort(key=lambda parent: (parent.type, parent.id))
         return BlockUsageLocator.make_relative(
             locator,
-            block_type=parent_id.type,
-            block_id=parent_id.id,
+            block_type=parent_ids[0].type,
+            block_id=parent_ids[0].id,
         )
 
     def get_orphans(self, course_key, **kwargs):
@@ -2041,8 +2043,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             for subtree_root in subtree_list:
                 if BlockKey.from_usage_key(subtree_root) != source_structure['root']:
                     # find the parents and put root in the right sequence
-                    parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure)
-                    if parent is not None:  # may be a detached category xblock
+                    parents = self._get_parents_from_structure(BlockKey.from_usage_key(subtree_root), source_structure)
+                    for parent in parents:
                         if parent not in destination_blocks:
                             raise ItemNotFoundError(parent)
                         orphans.update(
@@ -2101,8 +2103,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             new_structure = self.version_structure(usage_locator.course_key, original_structure, user_id)
             new_blocks = new_structure['blocks']
             new_id = new_structure['_id']
-            parent_block_key = self._get_parent_from_structure(block_key, original_structure)
-            if parent_block_key:
+            parent_block_keys = self._get_parents_from_structure(block_key, original_structure)
+            for parent_block_key in parent_block_keys:
                 parent_block = new_blocks[parent_block_key]
                 parent_block['fields']['children'].remove(block_key)
                 parent_block['edit_info']['edited_on'] = datetime.datetime.now(UTC)
@@ -2626,15 +2628,16 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         }
 
     @contract(block_key=BlockKey)
-    def _get_parent_from_structure(self, block_key, structure):
+    def _get_parents_from_structure(self, block_key, structure):
         """
         Given a structure, find block_key's parent in that structure. Note returns
         the encoded format for parent
         """
-        for parent_block_key, value in structure['blocks'].iteritems():
-            if block_key in value['fields'].get('children', []):
-                return parent_block_key
-        return None
+        return [
+            parent_block_key
+            for parent_block_key, value in structure['blocks'].iteritems()
+            if block_key in value['fields'].get('children', [])
+        ]
 
     def _sync_children(self, source_parent, destination_parent, new_child):
         """
@@ -2733,7 +2736,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         """
         Delete the orphan and any of its descendants which no longer have parents.
         """
-        if self._get_parent_from_structure(orphan, structure) is None:
+        if len(self._get_parents_from_structure(orphan, structure)) == 0:
             for child in structure['blocks'][orphan]['fields'].get('children', []):
                 self._delete_if_true_orphan(BlockKey(*child), structure)
             del structure['blocks'][orphan]

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

@@ -1962,6 +1962,46 @@ class TestMixedModuleStore(CourseComparisonTest):
                 with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
                     self.assertTrue(self.store.has_item(vertical_loc))
 
+    @ddt.data(ModuleStoreEnum.Type.split)  # Need to fix and add ModuleStoreEnum.Type.mongo,
+    def test_delete_dag(self, default):
+        """
+        Test that deleting an element with more than one parent fully removes it from the course.
+        """
+        # set the default modulestore
+        with MongoContentstoreBuilder().build() as contentstore:
+            self.store = MixedModuleStore(
+                contentstore=contentstore,
+                create_modulestore_instance=create_modulestore_instance,
+                mappings={},
+                **self.OPTIONS
+            )
+            self.addCleanup(self.store.close_all_connections)
+            with self.store.default_store(default):
+                dest_course_key = self.store.make_course_key('a', 'course', 'course')
+                courses = import_from_xml(
+                    self.store, self.user_id, DATA_DIR, ['xml_dag'], load_error_modules=False,
+                    static_content_store=contentstore,
+                    target_course_id=dest_course_key,
+                    create_course_if_not_present=True,
+                )
+                course_id = courses[0].id
+                # ensure both parents point to the dag item
+                dag_item = course_id.make_usage_key('html', 'toyhtml')
+                one_parent = course_id.make_usage_key('vertical', 'vertical_test')
+                other_parent = course_id.make_usage_key('vertical', 'zeta')
+                with self.store.bulk_operations(course_id):
+                    # actually should test get_parent but it's not alphabetized yet
+                    self.assertEqual(self.store.get_parent_location(dag_item), one_parent)
+                    for parent_loc in [one_parent, other_parent]:
+                        parent = self.store.get_item(parent_loc)
+                        self.assertIn(dag_item, parent.children)
+                    # just testing draft branch assuming it doesn't matter which branch
+                    with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
+                        self.store.delete_item(dag_item, self.user_id)
+                        for parent_loc in [one_parent, other_parent]:
+                            parent = self.store.get_item(parent_loc)
+                            self.assertNotIn(dag_item, parent.children)
+
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_import_edit_import(self, default):
         """

common/test/data/xml_dag/README.md

@@ -0,0 +1,2 @@
+Minimal course for testing the handling of non-tree courses. The same html component occurs under more than one 
+vertical.

common/test/data/xml_dag/course.xml

@@ -0,0 +1 @@
+<course org="testX" course="dag" url_name="2012_Fall"/>

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

@@ -0,0 +1,13 @@
+<course course_image="just_a_test.jpg">
+  <chapter url_name="Overview">
+    <sequential>
+        <vertical filename="vertical_test" slug="vertical_test" />
+    </sequential>
+    <sequential>
+        <vertical url_name="zeta">
+            <html url_name="toyhtml"/>
+            <discussion id="duplicate_def" slug="duplicate_def" discussion_category="Lectures"/>
+        </vertical>
+    </sequential>
+  </chapter>
+</course>

common/test/data/xml_dag/html/toyhtml.html

@@ -0,0 +1 @@
+<a href='/static/handouts/sample_handout.txt'>Sample</a>

common/test/data/xml_dag/html/toyhtml.xml

@@ -0,0 +1 @@
+<html filename="toyhtml.html"/>

common/test/data/xml_dag/policies/2012_Fall.json

@@ -0,0 +1,8 @@
+{
+    "course/2012_Fall": {
+        "graceperiod": "2 days 5 hours 59 minutes 59 seconds",
+        "start": "2015-07-17T12:00",
+        "display_name": "Toy Course",
+        "graded": "true",
+    }
+}

common/test/data/xml_dag/vertical/vertical_test.xml

@@ -0,0 +1,4 @@
+<sequential>
+    <html url_name="toyhtml"/>
+    <discussion for="topic 0" id="duplicate_def" slug="duplicate_def" discussion_category="Lecture"/>
+</sequential>