Merge pull request #4273 from edx/benmcmorran/subtree-edited-info

Adds subtree_edited_on, subtree_edited_by, and fixes has_changes

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

@@ -230,7 +230,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
                 # migrate published_by and published_date if edit_info isn't present
                 if not edit_info:
-                    module.edited_by = module.edited_on = module.published_date = None
+                    module.edited_by = module.edited_on = module.subtree_edited_on = \
+                        module.subtree_edited_by = module.published_date = None
                     # published_date was previously stored as a list of time components instead of a datetime
                     if metadata.get('published_date'):
                         module.published_date = datetime(*metadata.get('published_date')[0:6]).replace(tzinfo=UTC)
@@ -239,6 +240,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 else:
                     module.edited_by = edit_info.get('edited_by')
                     module.edited_on = edit_info.get('edited_on')
+                    module.subtree_edited_on = edit_info.get('subtree_edited_on')
+                    module.subtree_edited_by = edit_info.get('subtree_edited_by')
                     module.published_date = edit_info.get('published_date')
                     module.published_by = edit_info.get('published_by')
 
@@ -981,7 +984,17 @@ class MongoModuleStore(ModuleStoreWriteBase):
         if result['n'] == 0:
             raise ItemNotFoundError(location)
 
-    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False, isPublish=False):
+    def _update_ancestors(self, location, update):
+        """
+        Recursively applies update to all the ancestors of location
+        """
+        parent = self._get_raw_parent_location(as_published(location), ModuleStoreEnum.RevisionOption.draft_preferred)
+        if parent:
+            self._update_single_item(parent, update)
+            self._update_ancestors(parent, update)
+
+    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False, isPublish=False,
+                    is_publish_root=True):
         """
         Update the persisted version of xblock to reflect its current values.
 
@@ -991,30 +1004,42 @@ class MongoModuleStore(ModuleStoreWriteBase):
         force: force is meaningless for this modulestore
         isPublish: an internal parameter that indicates whether this update is due to a Publish operation, and
           thus whether the item's published information should be updated.
+        is_publish_root: when publishing, this indicates whether xblock is the root of the publish and should
+          therefore propagate subtree edit info up the tree
         """
         try:
             definition_data = self._convert_reference_fields_to_strings(
                 xblock,
                 xblock.get_explicitly_set_fields_by_scope()
             )
+            now = datetime.now(UTC)
             payload = {
                 'definition.data': definition_data,
                 'metadata': self._convert_reference_fields_to_strings(xblock, own_metadata(xblock)),
-                'edit_info': {
-                    'edited_on': datetime.now(UTC),
-                    'edited_by': user_id,
-                }
+                'edit_info.edited_on': now,
+                'edit_info.edited_by': user_id,
+                'edit_info.subtree_edited_on': now,
+                'edit_info.subtree_edited_by': user_id,
             }
 
             if isPublish:
-                payload['edit_info']['published_date'] = datetime.now(UTC)
-                payload['edit_info']['published_by'] = user_id
+                payload['edit_info.published_date'] = now
+                payload['edit_info.published_by'] = user_id
 
             if xblock.has_children:
                 children = self._convert_reference_fields_to_strings(xblock, {'children': xblock.children})
                 payload.update({'definition.children': children['children']})
             self._update_single_item(xblock.scope_ids.usage_id, payload)
 
+            # update subtree edited info for ancestors
+            # don't update the subtree info for descendants of the publish root for efficiency
+            if (not isPublish) or (isPublish and is_publish_root):
+                ancestor_payload = {
+                    'edit_info.subtree_edited_on': now,
+                    'edit_info.subtree_edited_by': user_id
+                }
+                self._update_ancestors(xblock.scope_ids.usage_id, ancestor_payload)
+
             # recompute (and update) the metadata inheritance tree which is cached
             self.refresh_cached_metadata_inheritance_tree(xblock.scope_ids.usage_id.course_key, xblock.runtime)
             # fire signal that we've written to DB
@@ -1045,20 +1070,10 @@ class MongoModuleStore(ModuleStoreWriteBase):
                         value[key] = subvalue.to_deprecated_string()
         return jsonfields
 
-    def get_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs):
+    def _get_raw_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only):
         '''
-        Find the location that is the parent of this location in this course.
-
-        Returns: version agnostic location (revision always None) as per the rest of mongo.
-
-        Args:
-            revision:
-                ModuleStoreEnum.RevisionOption.published_only
-                    - return only the PUBLISHED parent if it exists, else returns None
-                ModuleStoreEnum.RevisionOption.draft_preferred
-                    - return either the DRAFT or PUBLISHED parent,
-                        preferring DRAFT, if parent(s) exists,
-                        else returns None
+        Helper for get_parent_location that finds the location that is the parent of this location in this course,
+        but does NOT return a version agnostic location.
         '''
         assert location.revision is None
         assert revision == ModuleStoreEnum.RevisionOption.published_only \
@@ -1096,7 +1111,27 @@ class MongoModuleStore(ModuleStoreWriteBase):
             # since we sorted by SORT_REVISION_FAVOR_DRAFT, the 0'th parent is the one we want
             found_id = parents[0]['_id']
             # don't disclose revision outside modulestore
-            return as_published(Location._from_deprecated_son(found_id, location.course_key.run))
+            return Location._from_deprecated_son(found_id, location.course_key.run)
+
+    def get_parent_location(self, location, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs):
+        '''
+        Find the location that is the parent of this location in this course.
+
+        Returns: version agnostic location (revision always None) as per the rest of mongo.
+
+        Args:
+            revision:
+                ModuleStoreEnum.RevisionOption.published_only
+                    - return only the PUBLISHED parent if it exists, else returns None
+                ModuleStoreEnum.RevisionOption.draft_preferred
+                    - return either the DRAFT or PUBLISHED parent,
+                        preferring DRAFT, if parent(s) exists,
+                        else returns None
+        '''
+        parent = self._get_raw_parent_location(location, revision)
+        if parent:
+            return as_published(parent)
+        return None
 
     def get_modulestore_type(self, course_key=None):
         """

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

@@ -371,7 +371,7 @@ class DraftModuleStore(MongoModuleStore):
                     raise
 
         xblock.location = draft_loc
-        super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found, isPublish)
+        super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found, isPublish=isPublish)
         return wrap_draft(xblock)
 
     def delete_item(self, location, user_id, revision=None, **kwargs):
@@ -493,27 +493,21 @@ class DraftModuleStore(MongoModuleStore):
 
     def has_changes(self, location):
         """
-        Check if the xblock has been changed since it was last published.
+        Check if the xblock or its children have been changed since the last publish.
         :param location: location to check
         :return: True if the draft and published versions differ
         """
 
-        # Direct only categories can never have changes because they can't have drafts
-        if location.category in DIRECT_ONLY_CATEGORIES:
-            return False
-
-        draft = self.get_item(location)
-
-        # If the draft was never published, then it clearly has unpublished changes
-        if not draft.published_date:
-            return True
-
-        # edited_on may be None if the draft was last edited before edit time tracking
-        # If the draft does not have an edit time, we play it safe and assume there are differences
-        if draft.edited_on:
-            return draft.edited_on > draft.published_date
+        # a non direct only xblock has changes if it is in a non public state
+        if location.category not in DIRECT_ONLY_CATEGORIES:
+            return self.compute_publish_state(self.get_item(location)) != PublishState.public
         else:
-            return True
+            item = self.get_item(location)
+            if item.has_children:
+                for child in item.children:
+                    if self.has_changes(child):
+                        return True
+            return False
 
     def publish(self, location, user_id):
         """
@@ -533,7 +527,7 @@ class DraftModuleStore(MongoModuleStore):
         # list of published ones.)
         to_be_deleted = []
 
-        def _internal_depth_first(item_location):
+        def _internal_depth_first(item_location, is_root):
             """
             Depth first publishing from the given location
             """
@@ -542,7 +536,7 @@ class DraftModuleStore(MongoModuleStore):
             # publish the children first
             if item.has_children:
                 for child_loc in item.children:
-                    _internal_depth_first(child_loc)
+                    _internal_depth_first(child_loc, False)
 
             if item_location.category in DIRECT_ONLY_CATEGORIES or not getattr(item, 'is_draft', False):
                 # ignore noop attempt to publish something that can't be or isn't currently draft
@@ -572,14 +566,14 @@ class DraftModuleStore(MongoModuleStore):
                                 # So, do not delete the child.  It will be published when the new parent is published.
                                 pass
 
-            super(DraftModuleStore, self).update_item(item, user_id, isPublish=True)
+            super(DraftModuleStore, self).update_item(item, user_id, isPublish=True, is_publish_root=is_root)
             to_be_deleted.append(as_draft(item_location).to_deprecated_son())
 
         # verify input conditions
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
         _verify_revision_is_published(location)
 
-        _internal_depth_first(location)
+        _internal_depth_first(location, True)
         if len(to_be_deleted) > 0:
             self.collection.remove({'_id': {'$in': to_be_deleted}})
         return self.get_item(as_published(location))

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

@@ -487,7 +487,7 @@ class TestMongoModuleStore(unittest.TestCase):
 
     def test_has_changes_direct_only(self):
         """
-        Tests that has_changes() returns false when an xblock in a direct only category is checked
+        Tests that has_changes() returns false when a new xblock in a direct only category is checked
         """
         course_location = Location('edx', 'direct', '2012_Fall', 'course', 'test_course')
         chapter_location = Location('edx', 'direct', '2012_Fall', 'chapter', 'test_chapter')
@@ -528,6 +528,186 @@ class TestMongoModuleStore(unittest.TestCase):
         self.draft_store.publish(location, dummy_user)
         self.assertFalse(self.draft_store.has_changes(location))
 
+    def _create_test_tree(self, name, user_id=123):
+        """
+        Creates and returns a tree with the following structure:
+        Grandparent
+            Parent Sibling
+            Parent
+                Child
+                Child Sibling
+
+        """
+        locations = {
+            'grandparent': Location('edX', 'tree', name, 'chapter', 'grandparent'),
+            'parent_sibling': Location('edX', 'tree', name, 'sequential', 'parent_sibling'),
+            'parent': Location('edX', 'tree', name, 'sequential', 'parent'),
+            'child_sibling': Location('edX', 'tree', name, 'vertical', 'child_sibling'),
+            'child': Location('edX', 'tree', name, 'vertical', 'child'),
+        }
+
+        for key in locations:
+            self.draft_store.create_and_save_xmodule(locations[key], user_id=user_id)
+
+        grandparent = self.draft_store.get_item(locations['grandparent'])
+        grandparent.children += [locations['parent_sibling'], locations['parent']]
+        self.draft_store.update_item(grandparent, user_id=user_id)
+
+        parent = self.draft_store.get_item(locations['parent'])
+        parent.children += [locations['child_sibling'], locations['child']]
+        self.draft_store.update_item(parent, user_id=user_id)
+
+        self.draft_store.publish(locations['parent'], user_id)
+        self.draft_store.publish(locations['parent_sibling'], user_id)
+
+        return locations
+
+    def test_has_changes_ancestors(self):
+        """
+        Tests that has_changes() returns true on ancestors when a child is changed
+        """
+        dummy_user = 123
+        locations = self._create_test_tree('has_changes_ancestors')
+
+        # Verify that there are no unpublished changes
+        for key in locations:
+            self.assertFalse(self.draft_store.has_changes(locations[key]))
+
+        # Change the child
+        child = self.draft_store.get_item(locations['child'])
+        child.display_name = 'Changed Display Name'
+        self.draft_store.update_item(child, user_id=dummy_user)
+
+        # All ancestors should have changes, but not siblings
+        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
+        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+        self.assertTrue(self.draft_store.has_changes(locations['child']))
+        self.assertFalse(self.draft_store.has_changes(locations['parent_sibling']))
+        self.assertFalse(self.draft_store.has_changes(locations['child_sibling']))
+
+        # Publish the unit with changes
+        self.draft_store.publish(locations['parent'], dummy_user)
+
+        # Verify that there are no unpublished changes
+        for key in locations:
+            self.assertFalse(self.draft_store.has_changes(locations[key]))
+
+    def test_has_changes_publish_ancestors(self):
+        """
+        Tests that has_changes() returns false after a child is published only if all children are unchanged
+        """
+        dummy_user = 123
+        locations = self._create_test_tree('has_changes_publish_ancestors')
+
+        # Verify that there are no unpublished changes
+        for key in locations:
+            self.assertFalse(self.draft_store.has_changes(locations[key]))
+
+        # Change both children
+        child = self.draft_store.get_item(locations['child'])
+        child_sibling = self.draft_store.get_item(locations['child_sibling'])
+        child.display_name = 'Changed Display Name'
+        child_sibling.display_name = 'Changed Display Name'
+        self.draft_store.update_item(child, user_id=dummy_user)
+        self.draft_store.update_item(child_sibling, user_id=dummy_user)
+
+        # Verify that ancestors have changes
+        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
+        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+
+        # Publish one child
+        self.draft_store.publish(locations['child_sibling'], dummy_user)
+
+        # Verify that ancestors still have changes
+        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
+        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+
+        # Publish the other child
+        self.draft_store.publish(locations['child'], dummy_user)
+
+        # Verify that ancestors now have no changes
+        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
+        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+
+    def test_has_changes_add_remove_child(self):
+        """
+        Tests that has_changes() returns true for the parent when a child with changes is added
+        and false when that child is removed.
+        """
+        dummy_user = 123
+        locations = self._create_test_tree('has_changes_add_remove_child')
+
+        # Test that the ancestors don't have changes
+        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
+        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+
+        # Create a new child and attach it to parent
+        new_child_location = Location('edX', 'tree', 'has_changes_add_remove_child', 'vertical', 'new_child')
+        self.draft_store.create_and_save_xmodule(new_child_location, user_id=dummy_user)
+        parent = self.draft_store.get_item(locations['parent'])
+        parent.children += [new_child_location]
+        self.draft_store.update_item(parent, user_id=dummy_user)
+
+        # Verify that the ancestors now have changes
+        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
+        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+
+        # Remove the child from the parent
+        parent = self.draft_store.get_item(locations['parent'])
+        parent.children = [locations['child'], locations['child_sibling']]
+        self.draft_store.update_item(parent, user_id=dummy_user)
+
+        # Verify that ancestors now have no changes
+        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
+        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+
+    def test_update_edit_info_ancestors(self):
+        """
+        Tests that edited_on, edited_by, subtree_edited_on, and subtree_edited_by are set correctly during update
+        """
+        create_user = 123
+        edit_user = 456
+        locations = self._create_test_tree('update_edit_info_ancestors', create_user)
+
+        def check_node(location_key, after, before, edited_by, subtree_after, subtree_before, subtree_by):
+            """
+            Checks that the node given by location_key matches the given edit_info constraints.
+            """
+            node = self.draft_store.get_item(locations[location_key])
+            if after:
+                self.assertLess(after, node.edited_on)
+            self.assertLess(node.edited_on, before)
+            self.assertEqual(node.edited_by, edited_by)
+            if subtree_after:
+                self.assertLess(subtree_after, node.subtree_edited_on)
+            self.assertLess(node.subtree_edited_on, subtree_before)
+            self.assertEqual(node.subtree_edited_by, subtree_by)
+
+        after_create = datetime.now(UTC)
+        # Verify that all nodes were last edited in the past by create_user
+        for key in locations:
+            check_node(key, None, after_create, create_user, None, after_create, create_user)
+
+        # Change the child
+        child = self.draft_store.get_item(locations['child'])
+        child.display_name = 'Changed Display Name'
+        self.draft_store.update_item(child, user_id=edit_user)
+
+        after_edit = datetime.now(UTC)
+        ancestors = ['parent', 'grandparent']
+        others = ['child_sibling', 'parent_sibling']
+
+        # Verify that child was last edited between after_create and after_edit by edit_user
+        check_node('child', after_create, after_edit, edit_user, after_create, after_edit, edit_user)
+
+        # Verify that ancestors edit info is unchanged, but their subtree edit info matches child
+        for key in ancestors:
+            check_node(key, None, after_create, create_user, after_create, after_edit, edit_user)
+
+        # Verify that others have unchanged edit info
+        for key in others:
+            check_node(key, None, after_create, create_user, None, after_create, create_user)
+
     def test_update_edit_info(self):
         """
         Tests that edited_on and edited_by are set correctly during an update

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

@@ -19,7 +19,7 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         # Should be 1 to verify course unique, 11 parent fetches,
         # and n per _create_item where n is the size of the course tree non-leaf nodes
         # for inheritance computation (which is 7*4 + sum(1..4) = 38) (max_finds)
-        with check_mongo_calls(self.draft_mongo, 70, 27):
+        with check_mongo_calls(self.draft_mongo, 71, 27):
             with check_mongo_calls(self.old_mongo, 70, 27):
                 super(TestPublish, self)._create_course(split=False)
 
@@ -67,7 +67,9 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         item = self.draft_mongo.get_item(location, 2)
         # Vert1 has 3 children; so, publishes 4 nodes which may mean 4 inserts & 1 bulk remove
         # 25-June-2014 find calls are 19. Probably due to inheritance recomputation?
-        with check_mongo_calls(self.draft_mongo, 19, 5):
+        # 02-July-2014 send calls are 7. 5 from above, plus 2 for updating subtree edit info for Chapter1 and course
+        #              find calls are 22. 19 from above, plus 3 for finding the parent of Vert1, Chapter1, and course
+        with check_mongo_calls(self.draft_mongo, 22, 7):
             self.draft_mongo.publish(item.location, self.userid)
 
         # verify status