Implement edit info as mixin on base mongo

LMS-11183, LMS-11184

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)

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']

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'])

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(
@@ -1124,6 +1124,147 @@ class TestMixedModuleStore(unittest.TestCase):
         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):
         """
         Test that the correct things have been published automatically

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