From c86f235275595191afdba29b757e53741f03f5fc Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Mon, 5 Jan 2015 13:02:25 -0500
Subject: [PATCH] Test xml get_parent for a non-tree course TNL-1012

---
 .../xmodule/modulestore/tests/test_xml.py     | 33 ++++++++++++++++---
 common/lib/xmodule/xmodule/modulestore/xml.py | 12 +++++--
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py
index c72e91a1e0..688748d0b8 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py
@@ -102,14 +102,39 @@ class TestXMLModuleStore(unittest.TestCase):
         Test the branch setting context manager
         """
         store = XMLModuleStore(DATA_DIR, course_dirs=['toy'])
-        course_key = store.get_courses()[0]
+        course = store.get_courses()[0]
 
         # XML store allows published_only branch setting
-        with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
-            store.get_item(course_key.location)
+        with store.branch_setting(ModuleStoreEnum.Branch.published_only, course.id):
+            store.get_item(course.location)
 
         # XML store does NOT allow draft_preferred branch setting
         with self.assertRaises(ValueError):
-            with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+            with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course.id):
                 # verify that the above context manager raises a ValueError
                 pass  # pragma: no cover
+
+    @patch('xmodule.modulestore.xml.log')
+    def test_dag_course(self, mock_logging):
+        """
+        Test a course whose structure is not a tree.
+        """
+        store = XMLModuleStore(DATA_DIR, course_dirs=['xml_dag'])
+        course_key = store.get_courses()[0].id
+
+        mock_logging.warning.assert_called_with(
+            "%s has more than one definition", course_key.make_usage_key('discussion', 'duplicate_def')
+        )
+
+        shared_item_loc = course_key.make_usage_key('html', 'toyhtml')
+        shared_item = store.get_item(shared_item_loc)
+        parent = shared_item.get_parent()
+        self.assertIsNotNone(parent, "get_parent failed to return a value")
+        parent_loc = course_key.make_usage_key('vertical', 'vertical_test')
+        self.assertEqual(parent.location, parent_loc)
+        self.assertIn(shared_item, parent.get_children())
+        # ensure it's still a child of the other parent even tho it doesn't claim the other parent as its parent
+        other_parent_loc = course_key.make_usage_key('vertical', 'zeta')
+        other_parent = store.get_item(other_parent_loc)
+        # children rather than get_children b/c the instance returned by get_children != shared_item
+        self.assertIn(shared_item_loc, other_parent.children)
diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py
index 18c8ea4160..b8cfad7e53 100644
--- a/common/lib/xmodule/xmodule/modulestore/xml.py
+++ b/common/lib/xmodule/xmodule/modulestore/xml.py
@@ -205,12 +205,20 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
 
             descriptor.data_dir = course_dir
 
+            if descriptor.scope_ids.usage_id in xmlstore.modules[course_id]:
+                # keep the parent pointer if any but allow everything else to overwrite
+                other_copy = xmlstore.modules[course_id][descriptor.scope_ids.usage_id]
+                descriptor.parent = other_copy.parent
+                if descriptor != other_copy:
+                    log.warning("%s has more than one definition", descriptor.scope_ids.usage_id)
             xmlstore.modules[course_id][descriptor.scope_ids.usage_id] = descriptor
 
             if descriptor.has_children:
                 for child in descriptor.get_children():
-                    child.parent = descriptor.location
-                    child.save()
+                    # parent is alphabetically least
+                    if child.parent is None or child.parent > descriptor.scope_ids.usage_id:
+                        child.parent = descriptor.location
+                        child.save()
 
             # After setting up the descriptor, save any changes that we have
             # made to attributes on the descriptor to the underlying KeyValueStore.