From a65771df93c9c618bd83dcc487ed23a45dcba951 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 27 Aug 2014 16:36:32 -0400 Subject: [PATCH] Implement edit info as mixin on base mongo LMS-11183, LMS-11184 --- .../xmodule/xmodule/modulestore/mongo/base.py | 87 +++++++---- .../split_mongo/caching_descriptor_system.py | 2 +- .../modulestore/split_mongo/split_draft.py | 5 +- .../tests/test_mixed_modulestore.py | 143 +++++++++++++++++- .../xmodule/modulestore/tests/test_mongo.py | 105 +------------ 5 files changed, 205 insertions(+), 137 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index e0e39a28e5..c23ffcc6a5 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -44,6 +44,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.keys import UsageKey, CourseKey from xmodule.exceptions import HeartbeatFailure +from xmodule.modulestore.edit_info import EditInfoRuntimeMixin log = logging.getLogger(__name__) @@ -137,7 +138,7 @@ class MongoKeyValueStore(InheritanceKeyValueStore): return False -class CachingDescriptorSystem(MakoDescriptorSystem): +class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): """ A system that has a cache of module json that it will use to load modules from, with a backup of calling to the underlying modulestore for more data @@ -233,25 +234,16 @@ class CachingDescriptorSystem(MakoDescriptorSystem): metadata_to_inherit = self.cached_metadata.get(unicode(non_draft_loc), {}) inherit_metadata(module, metadata_to_inherit) - edit_info = json_data.get('edit_info') + module._edit_info = json_data.get('edit_info') # migrate published_by and published_on if edit_info isn't present - if not edit_info: - module.edited_by = module.edited_on = module.subtree_edited_on = \ - module.subtree_edited_by = module.published_on = None + if module._edit_info is None: + module._edit_info = {} raw_metadata = json_data.get('metadata', {}) # published_on was previously stored as a list of time components instead of a datetime if raw_metadata.get('published_date'): - module.published_on = datetime(*raw_metadata.get('published_date')[0:6]).replace(tzinfo=UTC) - module.published_by = raw_metadata.get('published_by') - # otherwise restore the stored editing information - 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_on = edit_info.get('published_date') - module.published_by = edit_info.get('published_by') + module._edit_info['published_date'] = datetime(*raw_metadata.get('published_date')[0:6]).replace(tzinfo=UTC) + module._edit_info['published_by'] = raw_metadata.get('published_by') # decache any computed pending field settings module.save() @@ -316,6 +308,42 @@ class CachingDescriptorSystem(MakoDescriptorSystem): return json + def get_edited_by(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('edited_by') + + def get_edited_on(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('edited_on') + + def get_subtree_edited_by(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('subtree_edited_by') + + def get_subtree_edited_on(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('subtree_edited_on') + + def get_published_by(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('published_by') + + def get_published_on(self, xblock): + """ + See :class: cms.lib.xblock.runtime.EditInfoRuntimeMixin + """ + return xblock._edit_info.get('published_date') + # The only thing using this w/ wildcards is contentstore.mongo for asset retrieval def location_to_query(location, wildcard=True, tag='i4x'): @@ -1153,15 +1181,20 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): payload = { 'definition.data': definition_data, 'metadata': self._serialize_scope(xblock, Scope.settings), - 'edit_info.edited_on': now, - 'edit_info.edited_by': user_id, - 'edit_info.subtree_edited_on': now, - 'edit_info.subtree_edited_by': user_id, + 'edit_info': { + 'edited_on': now, + 'edited_by': user_id, + 'subtree_edited_on': now, + 'subtree_edited_by': user_id, + } } if isPublish: - payload['edit_info.published_date'] = now - payload['edit_info.published_by'] = user_id + payload['edit_info']['published_date'] = now + payload['edit_info']['published_by'] = user_id + elif 'published_date' in getattr(xblock, '_edit_info', {}): + payload['edit_info']['published_date'] = xblock._edit_info['published_date'] + payload['edit_info']['published_by'] = xblock._edit_info['published_by'] if xblock.has_children: children = self._serialize_scope(xblock, Scope.children) @@ -1181,17 +1214,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): self._update_ancestors(xblock.scope_ids.usage_id, ancestor_payload) # update the edit info of the instantiated xblock - xblock.edited_on = now - xblock.edited_by = user_id - xblock.subtree_edited_on = now - xblock.subtree_edited_by = user_id - if not hasattr(xblock, 'published_on'): - xblock.published_on = None - if not hasattr(xblock, 'published_by'): - xblock.published_by = None - if isPublish: - xblock.published_on = now - xblock.published_by = user_id + xblock._edit_info = payload['edit_info'] # 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) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index fee8f4ad2b..ab261dee96 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -276,7 +276,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): for child in json_data.get('fields', {}).get('children', []): child_data = self.get_module_data(child, course_key) if '_subtree_edited_on' not in json_data.setdefault('edit_info', {}): - self._compute_subtree_edited_internal(child, child_data) + self._compute_subtree_edited_internal(child, child_data, course_key) if child_data['edit_info']['_subtree_edited_on'] > max_date: max_date = child_data['edit_info']['_subtree_edited_on'] max_by = child_data['edit_info']['_subtree_edited_by'] 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 4a935bd319..6fd7caf5ec 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -393,5 +393,6 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS Get the published branch and find when it was published if it was. Cache the results in the xblock """ published_block = self._get_head(xblock, ModuleStoreEnum.BranchName.published) - setattr(xblock, '_published_by', published_block['edit_info']['edited_by']) - setattr(xblock, '_published_on', published_block['edit_info']['edited_on']) + if published_block is not None: + setattr(xblock, '_published_by', published_block['edit_info']['edited_by']) + setattr(xblock, '_published_on', published_block['edit_info']['edited_on']) 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 f1498ee63b..141f479e9b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -999,7 +999,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(self.user_id, block.edited_by) self.assertGreater(datetime.datetime.now(UTC), block.edited_on) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_create_item_populates_subtree_edited_info(self, default_ms): self.initdb(default_ms) block = self.store.create_item( @@ -1123,6 +1123,147 @@ class TestMixedModuleStore(unittest.TestCase): self.assertTrue(self.store.has_changes(item)) self.assertTrue(self.store.has_published_version(item)) + @ddt.data('draft', 'split') + def test_update_edit_info_ancestors(self, default_ms): + """ + Tests that edited_on, edited_by, subtree_edited_on, and subtree_edited_by are set correctly during update + """ + self.initdb(default_ms) + + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + + 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.store.get_item(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) + + # Create a dummy vertical & html to test against + component = self.store.create_child( + self.user_id, + test_course.location, + 'vertical', + block_id='test_vertical' + ) + child = self.store.create_child( + self.user_id, + component.location, + 'html', + block_id='test_html' + ) + sibling = self.store.create_child( + self.user_id, + component.location, + 'html', + block_id='test_html_no_change' + ) + + after_create = datetime.datetime.now(UTC) + # Verify that all nodes were last edited in the past by create_user + [ + check_node(block.location, None, after_create, self.user_id, None, after_create, self.user_id) + for block in [component, child, sibling] + ] + + # Change the component, then check that there now are changes + component.display_name = 'Changed Display Name' + + editing_user = self.user_id - 2 + component = self.store.update_item(component, editing_user) + after_edit = datetime.datetime.now(UTC) + check_node(component.location, after_create, after_edit, editing_user, after_create, after_edit, editing_user) + # but child didn't change + check_node(child.location, None, after_create, self.user_id, None, after_create, self.user_id) + + + # Change the child + child = self.store.get_item(child.location) + child.display_name = 'Changed Display Name' + self.store.update_item(child, user_id=editing_user) + + after_edit = datetime.datetime.now(UTC) + + # Verify that child was last edited between after_create and after_edit by edit_user + check_node(child.location, after_create, after_edit, editing_user, after_create, after_edit, editing_user) + + # Verify that ancestors edit info is unchanged, but their subtree edit info matches child + check_node(test_course.location, None, after_create, self.user_id, after_create, after_edit, editing_user) + + # Verify that others have unchanged edit info + check_node(sibling.location, None, after_create, self.user_id, None, after_create, self.user_id) + + @ddt.data('draft', 'split') + def test_update_edit_info(self, default_ms): + """ + Tests that edited_on and edited_by are set correctly during an update + """ + self.initdb(default_ms) + + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + + # Create a dummy component to test against + component = self.store.create_child( + self.user_id, + test_course.location, + 'vertical', + ) + + # Store the current edit time and verify that user created the component + self.assertEqual(component.edited_by, self.user_id) + old_edited_on = component.edited_on + + edit_user = self.user_id - 2 + # Change the component + component.display_name = 'Changed' + self.store.update_item(component, edit_user) + updated_component = self.store.get_item(component.location) + + # Verify the ordering of edit times and that dummy_user made the edit + self.assertLess(old_edited_on, updated_component.edited_on) + self.assertEqual(updated_component.edited_by, edit_user) + + @ddt.data('draft', 'split') + def test_update_published_info(self, default_ms): + """ + Tests that published_on and published_by are set correctly + """ + self.initdb(default_ms) + + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + + publish_user = 456 + + # Create a dummy component to test against + component = self.store.create_child( + self.user_id, + test_course.location, + 'vertical', + ) + + # Store the current time, then publish + old_time = datetime.datetime.now(UTC) + self.store.publish(component.location, publish_user) + updated_component = self.store.get_item(component.location) + + # Verify the time order and that publish_user caused publication + self.assertLessEqual(old_time, updated_component.published_on) + self.assertEqual(updated_component.published_by, publish_user) + + # Verify that changing the item doesn't unset the published info + updated_component.display_name = 'changed' + self.store.update_item(updated_component, self.user_id) + updated_component = self.store.get_item(updated_component.location) + self.assertLessEqual(old_time, updated_component.published_on) + self.assertEqual(updated_component.published_by, publish_user) + @ddt.data('draft', 'split') def test_auto_publish(self, default_ms): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 84d845eea2..06f4a1ba76 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -37,6 +37,7 @@ from git.test.lib.asserts import assert_not_none from xmodule.x_module import XModuleMixin from xmodule.modulestore.mongo.base import as_draft from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST +from xmodule.modulestore.edit_info import EditInfoMixin log = logging.getLogger(__name__) @@ -105,7 +106,9 @@ class TestMongoModuleStore(unittest.TestCase): content_store, doc_store_config, FS_ROOT, RENDER_TEMPLATE, default_class=DEFAULT_CLASS, - branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred + branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred, + xblock_mixins=(EditInfoMixin,) + ) import_from_xml( draft_store, @@ -706,106 +709,6 @@ class TestMongoModuleStore(unittest.TestCase): self.assertTrue(self._has_changes(parent_location)) self.assertTrue(self._has_changes(child_location)) - 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 - """ - location = Location('edX', 'toy', '2012_Fall', 'html', 'test_html') - - # Create a dummy component to test against - self.draft_store.create_item( - self.dummy_user, - location.course_key, - location.block_type, - block_id=location.block_id - ) - - # Store the current edit time and verify that dummy_user created the component - component = self.draft_store.get_item(location) - self.assertEqual(component.edited_by, self.dummy_user) - old_edited_on = component.edited_on - - # Change the component - component.display_name = component.display_name + ' Changed' - self.draft_store.update_item(component, self.dummy_user) - updated_component = self.draft_store.get_item(location) - - # Verify the ordering of edit times and that dummy_user made the edit - self.assertLess(old_edited_on, updated_component.edited_on) - self.assertEqual(updated_component.edited_by, self.dummy_user) - - def test_update_published_info(self): - """ - Tests that published_on and published_by are set correctly - """ - location = Location('edX', 'toy', '2012_Fall', 'html', 'test_html') - create_user = 123 - publish_user = 456 - - # Create a dummy component to test against - self.draft_store.create_item( - create_user, - location.course_key, - location.block_type, - block_id=location.block_id - ) - - # Store the current time, then publish - old_time = datetime.now(UTC) - self.draft_store.publish(location, publish_user) - updated_component = self.draft_store.get_item(location) - - # Verify the time order and that publish_user caused publication - self.assertLessEqual(old_time, updated_component.published_on) - self.assertEqual(updated_component.published_by, publish_user) - def test_migrate_published_info(self): """ Tests that blocks that were storing published_date and published_by through CMSBlockMixin are loaded correctly