From a59f8494a6f531f9b30c02af2b11a4294d91e9c0 Mon Sep 17 00:00:00 2001 From: Ben McMorran Date: Thu, 26 Jun 2014 17:44:41 -0400 Subject: [PATCH] Adds subtree_edited_on, subtree_edited_by, and fixes has_changes --- .../xmodule/xmodule/modulestore/mongo/base.py | 79 +++++--- .../xmodule/modulestore/mongo/draft.py | 36 ++-- .../xmodule/modulestore/tests/test_mongo.py | 182 +++++++++++++++++- .../xmodule/modulestore/tests/test_publish.py | 6 +- 4 files changed, 257 insertions(+), 46 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 1eddc7afd7..7ae03cc332 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 99bda91cbe..d94fcb68ba 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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)) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 313740fd98..76725a9bbd 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index fb9dade4c3..26e832d4e1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/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