Skip to content

E
edx-platform
Commit a59f8494 by Ben McMorran
Options

Adds subtree_edited_on, subtree_edited_by, and fixes has_changes

parent 220a028b
Showing with 257 additions and 46 deletions
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
......
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