diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 8c6abc2cd5..14516b0ea3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -236,18 +236,34 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :param xblock: the block to check :return: True if the draft and published versions differ """ - def get_block(branch_name): - course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch_name))['structure'] - return self._get_block_from_structure(course_structure, xblock.location.block_id) + def get_course(branch_name): + return self._lookup_course(xblock.location.course_key.for_branch(branch_name))['structure'] - draft_block = get_block(ModuleStoreEnum.BranchName.draft) - published_block = get_block(ModuleStoreEnum.BranchName.published) - if not published_block: - return True + def get_block(course_structure, block_id): + return self._get_block_from_structure(course_structure, block_id) - # check if the draft has changed since the published was created - return self._get_version(draft_block) != self._get_version(published_block) + draft_course = get_course(ModuleStoreEnum.BranchName.draft) + published_course = get_course(ModuleStoreEnum.BranchName.published) + def has_changes_subtree(block_id): + draft_block = get_block(draft_course, block_id) + published_block = get_block(published_course, block_id) + if not published_block: + return True + + # check if the draft has changed since the published was created + if self._get_version(draft_block) != self._get_version(published_block): + return True + + # check the children in the draft + if 'children' in draft_block.setdefault('fields', {}): + return any( + [has_changes_subtree(child_block_id) for child_block_id in draft_block['fields']['children']] + ) + + return False + + return has_changes_subtree(xblock.location.block_id) def publish(self, location, user_id, blacklist=None, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 348295086b..2d6fbebdcc 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -158,8 +158,14 @@ class TestMixedModuleStore(unittest.TestCase): BlockInfo('problem_x1a_3', 'problem', 'Problem_x1a_3', []), BlockInfo('html_x1a_1', 'html', 'HTML_x1a_1', []), ] + ), + BlockInfo( + 'vertical_x1b', 'vertical', 'Vertical_x1b', [] ) ] + ), + BlockInfo( + 'sequential_x2', 'sequential', 'Sequential_x2', [] ) ] ), @@ -441,6 +447,184 @@ class TestMixedModuleStore(unittest.TestCase): component = self.store.publish(component.location, self.user_id) self.assertFalse(self.store.has_changes(component)) + def _has_changes(self, location): + return self.store.has_changes(self.store.get_item(location)) + + def setup_has_changes(self, default_ms): + """ + Common set up for has_changes tests below. + Returns a dictionary of useful location maps for testing. + """ + self.initdb(default_ms) + self._create_block_hierarchy() + + locations = { + 'grandparent': self.chapter_x, + 'parent_sibling': self.sequential_x2, + 'parent': self.sequential_x1, + 'child_sibling': self.vertical_x1b, + 'child': self.vertical_x1a, + } + + # Publish the vertical units + self.store.publish(locations['parent_sibling'], self.user_id) + self.store.publish(locations['parent'], self.user_id) + + return locations + + @ddt.data('draft', 'split') + def test_has_changes_ancestors(self, default_ms): + """ + Tests that has_changes() returns true on ancestors when a child is changed + """ + locations = self.setup_has_changes(default_ms) + + # Verify that there are no unpublished changes + for key in locations: + self.assertFalse(self._has_changes(locations[key])) + + # Change the child + child = self.store.get_item(locations['child']) + child.display_name = 'Changed Display Name' + self.store.update_item(child, self.user_id) + + # All ancestors should have changes, but not siblings + self.assertTrue(self._has_changes(locations['grandparent'])) + self.assertTrue(self._has_changes(locations['parent'])) + self.assertTrue(self._has_changes(locations['child'])) + self.assertFalse(self._has_changes(locations['parent_sibling'])) + self.assertFalse(self._has_changes(locations['child_sibling'])) + + # Publish the unit with changes + self.store.publish(locations['parent'], self.user_id) + + # Verify that there are no unpublished changes + for key in locations: + self.assertFalse(self._has_changes(locations[key])) + + @ddt.data('draft', 'split') + def test_has_changes_publish_ancestors(self, default_ms): + """ + Tests that has_changes() returns false after a child is published only if all children are unchanged + """ + locations = self.setup_has_changes(default_ms) + + # Verify that there are no unpublished changes + for key in locations: + self.assertFalse(self._has_changes(locations[key])) + + # Change both children + child = self.store.get_item(locations['child']) + child_sibling = self.store.get_item(locations['child_sibling']) + child.display_name = 'Changed Display Name' + child_sibling.display_name = 'Changed Display Name' + self.store.update_item(child, user_id=self.user_id) + self.store.update_item(child_sibling, user_id=self.user_id) + + # Verify that ancestors have changes + self.assertTrue(self._has_changes(locations['grandparent'])) + self.assertTrue(self._has_changes(locations['parent'])) + + # Publish one child + self.store.publish(locations['child_sibling'], self.user_id) + + # Verify that ancestors still have changes + self.assertTrue(self._has_changes(locations['grandparent'])) + self.assertTrue(self._has_changes(locations['parent'])) + + # Publish the other child + self.store.publish(locations['child'], self.user_id) + + # Verify that ancestors now have no changes + self.assertFalse(self._has_changes(locations['grandparent'])) + self.assertFalse(self._has_changes(locations['parent'])) + + @ddt.data('draft', 'split') + def test_has_changes_add_remove_child(self, default_ms): + """ + Tests that has_changes() returns true for the parent when a child with changes is added + and false when that child is removed. + """ + locations = self.setup_has_changes(default_ms) + + # Test that the ancestors don't have changes + self.assertFalse(self._has_changes(locations['grandparent'])) + self.assertFalse(self._has_changes(locations['parent'])) + + # Create a new child and attach it to parent + self.store.create_child( + self.user_id, + locations['parent'], + 'vertical', + block_id='new_child', + ) + + # Verify that the ancestors now have changes + self.assertTrue(self._has_changes(locations['grandparent'])) + self.assertTrue(self._has_changes(locations['parent'])) + + # Remove the child from the parent + parent = self.store.get_item(locations['parent']) + parent.children = [locations['child'], locations['child_sibling']] + self.store.update_item(parent, user_id=self.user_id) + + # Verify that ancestors now have no changes + self.assertFalse(self._has_changes(locations['grandparent'])) + self.assertFalse(self._has_changes(locations['parent'])) + + @ddt.data('draft', 'split') + def test_has_changes_non_direct_only_children(self, default_ms): + """ + Tests that has_changes() returns true after editing the child of a vertical (both not direct only categories). + """ + self.initdb(default_ms) + + parent = self.store.create_item( + self.user_id, + self.course.id, + 'vertical', + block_id='parent', + ) + child = self.store.create_child( + self.user_id, + parent.location, + 'html', + block_id='child', + ) + self.store.publish(parent.location, self.user_id) + + # Verify that there are no changes + self.assertFalse(self._has_changes(parent.location)) + self.assertFalse(self._has_changes(child.location)) + + # Change the child + child.display_name = 'Changed Display Name' + self.store.update_item(child, user_id=self.user_id) + + # Verify that both parent and child have changes + 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): + """ + 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) + + # Check the parent for changes should return True and not throw an exception + self.assertTrue(self.store.has_changes(parent)) + # TODO: LMS-11220: Document why split find count is 4 # TODO: LMS-11220: Document why split send count is 3 @ddt.data(('draft', 8, 2), ('split', 4, 3)) @@ -717,7 +901,7 @@ class TestMixedModuleStore(unittest.TestCase): # TODO: LMS-11220: Document why draft send count is 5 # TODO: LMS-11220: Document why draft find count is [19, 6] # TODO: LMS-11220: Document why split find count is [2, 2] - @ddt.data(('draft', [19, 6], 0), ('split', [2, 2], 0)) + @ddt.data(('draft', [21, 6], 0), ('split', [2, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): """ @@ -866,10 +1050,11 @@ class TestMixedModuleStore(unittest.TestCase): """ self.initdb(default_ms) self._create_block_hierarchy() + num_children = len(self.store.get_item(self.sequential_x1).children) self.store.revert_to_published(self.sequential_x1, self.user_id) reverted_parent = self.store.get_item(self.sequential_x1) # It does not discard the child vertical, even though that child is a draft (with no published version) - self.assertEqual(1, len(reverted_parent.children)) + self.assertEqual(num_children, len(reverted_parent.children)) @ddt.data(('draft', 1, 0), ('split', 2, 0)) @ddt.unpack diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 06f4a1ba76..031fd358a7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -505,25 +505,6 @@ class TestMongoModuleStore(unittest.TestCase): finally: shutil.rmtree(root_dir) - def test_has_changes_missing_child(self): - """ - Tests that has_changes() returns False when a published parent points to a child that doesn't exist. - """ - location = Location('edX', 'toy', '2012_Fall', 'sequential', 'parent') - - # Create the parent and point it to a fake child - parent = self.draft_store.create_item( - self.dummy_user, - location.course_key, - location.block_type, - block_id=location.block_id - ) - parent.children += [Location('edX', 'toy', '2012_Fall', 'vertical', 'does_not_exist')] - parent = self.draft_store.update_item(parent, self.dummy_user) - - # Check the parent for changes should return False and not throw an exception - self.assertFalse(self.draft_store.has_changes(parent)) - def _create_test_tree(self, name, user_id=None): """ Creates and returns a tree with the following structure: @@ -573,142 +554,6 @@ class TestMongoModuleStore(unittest.TestCase): return locations - def _has_changes(self, location): - """ Helper that returns True if location has changes, False otherwise """ - store = self.draft_store - return store.has_changes(store.get_item(location)) - - def test_has_changes_ancestors(self): - """ - Tests that has_changes() returns true on ancestors when a child is changed - """ - locations = self._create_test_tree('has_changes_ancestors') - - # Verify that there are no unpublished changes - for key in locations: - self.assertFalse(self._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=self.dummy_user) - - # All ancestors should have changes, but not siblings - self.assertTrue(self._has_changes(locations['grandparent'])) - self.assertTrue(self._has_changes(locations['parent'])) - self.assertTrue(self._has_changes(locations['child'])) - self.assertFalse(self._has_changes(locations['parent_sibling'])) - self.assertFalse(self._has_changes(locations['child_sibling'])) - - # Publish the unit with changes - self.draft_store.publish(locations['parent'], self.dummy_user) - - # Verify that there are no unpublished changes - for key in locations: - self.assertFalse(self._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 - """ - locations = self._create_test_tree('has_changes_publish_ancestors') - - # Verify that there are no unpublished changes - for key in locations: - self.assertFalse(self._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=self.dummy_user) - self.draft_store.update_item(child_sibling, user_id=self.dummy_user) - - # Verify that ancestors have changes - self.assertTrue(self._has_changes(locations['grandparent'])) - self.assertTrue(self._has_changes(locations['parent'])) - - # Publish one child - self.draft_store.publish(locations['child_sibling'], self.dummy_user) - - # Verify that ancestors still have changes - self.assertTrue(self._has_changes(locations['grandparent'])) - self.assertTrue(self._has_changes(locations['parent'])) - - # Publish the other child - self.draft_store.publish(locations['child'], self.dummy_user) - - # Verify that ancestors now have no changes - self.assertFalse(self._has_changes(locations['grandparent'])) - self.assertFalse(self._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. - """ - locations = self._create_test_tree('has_changes_add_remove_child') - - # Test that the ancestors don't have changes - self.assertFalse(self._has_changes(locations['grandparent'])) - self.assertFalse(self._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_child( - self.dummy_user, - locations['parent'], - new_child_location.block_type, - block_id=new_child_location.block_id - ) - - # Verify that the ancestors now have changes - self.assertTrue(self._has_changes(locations['grandparent'])) - self.assertTrue(self._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=self.dummy_user) - - # Verify that ancestors now have no changes - self.assertFalse(self._has_changes(locations['grandparent'])) - self.assertFalse(self._has_changes(locations['parent'])) - - def test_has_changes_non_direct_only_children(self): - """ - Tests that has_changes() returns true after editing the child of a vertical (both not direct only categories). - """ - parent_location = Location('edX', 'toy', '2012_Fall', 'vertical', 'parent') - child_location = Location('edX', 'toy', '2012_Fall', 'html', 'child') - - parent = self.draft_store.create_item( - self.dummy_user, - parent_location.course_key, - parent_location.block_type, - block_id=parent_location.block_id - ) - child = self.draft_store.create_child( - self.dummy_user, - parent_location, - child_location.block_type, - block_id=child_location.block_id - ) - self.draft_store.publish(parent_location, self.dummy_user) - - # Verify that there are no changes - self.assertFalse(self._has_changes(parent_location)) - self.assertFalse(self._has_changes(child_location)) - - # Change the child - child.display_name = 'Changed Display Name' - self.draft_store.update_item(child, user_id=self.dummy_user) - - # Verify that both parent and child have changes - self.assertTrue(self._has_changes(parent_location)) - self.assertTrue(self._has_changes(child_location)) - def test_migrate_published_info(self): """ Tests that blocks that were storing published_date and published_by through CMSBlockMixin are loaded correctly