Skip to content

E
edx-platform
Commit db0f9dfc by Nimisha Asthagiri
Options

LMS-11354 Make Split has_changes check subtree.

parent 7eb3a22d
Showing with 212 additions and 166 deletions
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):
"""
......
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
......
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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment