Merge pull request #6524 from edx/hotfix/2014-01-09

Handle children pointers to non-existent children in has_changes

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -624,6 +624,9 @@ class DraftModuleStore(MongoModuleStore):
             return True
         # if this block doesn't have changes, then check its children
         elif xblock.has_children:
+            # fix a bug where dangling pointers should imply a change
+            if len(xblock.children) > len(xblock.get_children()):
+                return True
             return any([self.has_changes(child) for child in xblock.get_children()])
         # otherwise there are no changes
         else:

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

@@ -268,8 +268,10 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
 
         def has_changes_subtree(block_key):
             draft_block = get_block(draft_course, block_key)
+            if draft_block is None:  # temporary fix for bad pointers TNL-1141
+                return True
             published_block = get_block(published_course, block_key)
-            if not published_block:
+            if published_block is None:
                 return True
 
             # check if the draft has changed since the published was created

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

@@ -663,25 +663,30 @@ class TestMixedModuleStore(CourseComparisonTest):
         self.assertTrue(self._has_changes(parent.location))
         self.assertTrue(self._has_changes(child.location))
 
-    @ddt.data('draft', 'split')
-    def test_has_changes_missing_child(self, default_ms):
+    @ddt.data(*itertools.product(
+        ('draft', 'split'),
+        (ModuleStoreEnum.Branch.draft_preferred, ModuleStoreEnum.Branch.published_only)
+    ))
+    @ddt.unpack
+    def test_has_changes_missing_child(self, default_ms, default_branch):
         """
         Tests that has_changes() does not throw an exception when a child doesn't exist.
         """
         self.initdb(default_ms)
 
-        # Create the parent and point it to a fake child
-        parent = self.store.create_item(
-            self.user_id,
-            self.course.id,
-            'vertical',
-            block_id='parent',
-        )
-        parent.children += [self.course.id.make_usage_key('vertical', 'does_not_exist')]
-        parent = self.store.update_item(parent, self.user_id)
+        with self.store.branch_setting(default_branch, self.course.id):
+            # Create the parent and point it to a fake child
+            parent = self.store.create_item(
+                self.user_id,
+                self.course.id,
+                'vertical',
+                block_id='parent',
+            )
+            parent.children += [self.course.id.make_usage_key('vertical', 'does_not_exist')]
+            parent = self.store.update_item(parent, self.user_id)
 
-        # Check the parent for changes should return True and not throw an exception
-        self.assertTrue(self.store.has_changes(parent))
+            # Check the parent for changes should return True and not throw an exception
+            self.assertTrue(self.store.has_changes(parent))
 
     # Draft
     #   Find: find parents (definition.children query), get parent, get course (fill in run?),