Merge pull request #4060 from edx/benmcmorran/edit-tracking

Tracks last edited date and user, and adds has_changes for blocks

cms/djangoapps/contentstore/course_info_model.py

@@ -239,6 +239,6 @@ def save_course_update_items(location, course_updates, course_update_items, user
     course_updates.data = _get_html(course_update_items)
 
     # update db record
-    modulestore('direct').update_item(course_updates, user)
+    modulestore('direct').update_item(course_updates, user.id)
 
     return course_updates

cms/djangoapps/contentstore/views/tests/test_course_updates.py

@@ -135,7 +135,7 @@ class CourseUpdateTest(CourseTestCase):
         update_content = u"Hello world!"
         update_data = u"<ol><li><h2>" + update_date + "</h2>" + update_content + "</li></ol>"
         course_updates.data = update_data
-        modulestore('direct').update_item(course_updates, self.user)
+        modulestore('direct').update_item(course_updates, self.user.id)
 
         # test getting all updates list
         course_update_url = self.create_update_url()

cms/djangoapps/models/settings/course_metadata.py

 from xblock.fields import Scope
 
 from contentstore.utils import get_modulestore
-from cms.lib.xblock.mixin import CmsBlockMixin
 
 
 class CourseMetadata(object):
@@ -33,9 +32,6 @@ class CourseMetadata(object):
         result = {}
 
         for field in descriptor.fields.values():
-            if field.name in CmsBlockMixin.fields:
-                continue
-
             if field.scope != Scope.settings:
                 continue
 

cms/envs/common.py

@@ -33,7 +33,6 @@ from lms.envs.common import (
 from path import path
 
 from lms.lib.xblock.mixin import LmsBlockMixin
-from cms.lib.xblock.mixin import CmsBlockMixin
 from dealer.git import git
 
 ############################ FEATURE CONFIGURATION #############################
@@ -238,7 +237,7 @@ from xmodule.x_module import XModuleMixin
 
 # This should be moved into an XBlock Runtime/Application object
 # once the responsibility of XBlock creation is moved out of modulestore - cpennington
-XBLOCK_MIXINS = (LmsBlockMixin, CmsBlockMixin, InheritanceMixin, XModuleMixin)
+XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin)
 
 # Allow any XBlock in Studio
 # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that

cms/lib/xblock/mixin.py

@@ -19,11 +19,3 @@ class DateTuple(Field):
             return None
 
         return list(value.timetuple())
-
-
-class CmsBlockMixin(XBlockMixin):
-    """
-    Mixin with fields common to all blocks in Studio
-    """
-    published_date = DateTuple(help="Date when the module was published", scope=Scope.settings)
-    published_by = Integer(help="Id of the user who published this module", scope=Scope.settings)

common/lib/xmodule/xmodule/error_module.py

@@ -13,6 +13,7 @@ from xmodule.x_module import XModule, XModuleDescriptor
 from xmodule.errortracker import exc_info_to_str
 from xblock.fields import String, Scope, ScopeIds
 from xblock.field_data import DictFieldData
+from xmodule.modulestore.xml_exporter import EdxJSONEncoder
 
 
 log = logging.getLogger(__name__)
@@ -121,7 +122,7 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
     def from_json(cls, json_data, system, location, error_msg='Error not available'):
         return cls._construct(
             system,
-            json.dumps(json_data, skipkeys=False, indent=4),
+            json.dumps(json_data, skipkeys=False, indent=4, cls=EdxJSONEncoder),
             error_msg,
             location=location
         )

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -21,6 +21,8 @@ import re
 from bson.son import SON
 from fs.osfs import OSFS
 from path import path
+from datetime import datetime
+from pytz import UTC
 
 from importlib import import_module
 from xmodule.errortracker import null_error_tracker, exc_info_to_str
@@ -207,6 +209,23 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                     # to python values
                     metadata_to_inherit = self.cached_metadata.get(non_draft_loc.to_deprecated_string(), {})
                     inherit_metadata(module, metadata_to_inherit)
+
+                edit_info = json_data.get('edit_info')
+
+                # 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
+                    # 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)
+                    module.published_by = 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.published_date = edit_info.get('published_date')
+                    module.published_by = edit_info.get('published_by')
+
                 # decache any computed pending field settings
                 module.save()
                 return module
@@ -827,7 +846,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         return xmodule
 
     def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None,
-                                fields={}):
+                                fields={}, user_id=None):
         """
         Create the new xmodule and save it. Does not return the new module because if the caller
         will insert it as a child, it's inherited metadata will completely change. The difference
@@ -838,12 +857,13 @@ class MongoModuleStore(ModuleStoreWriteBase):
         :param definition_data: can be empty. The initial definition_data for the kvs
         :param metadata: can be empty, the initial metadata for the kvs
         :param system: if you already have an xblock from the course, the xblock.runtime value
+        :param user_id: the user that created the xblock
         """
         # differs from split mongo in that I believe most of this logic should be above the persistence
         # layer but added it here to enable quick conversion. I'll need to reconcile these.
         new_object = self.create_xmodule(location, definition_data, metadata, system, fields)
         location = new_object.scope_ids.usage_id
-        self.update_item(new_object, allow_not_found=True)
+        self.update_item(new_object, allow_not_found=True, user_id=user_id)
 
         # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
         # if we add one then we need to also add it to the policy information (i.e. metadata)
@@ -857,7 +877,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
                     url_slug=new_object.scope_ids.usage_id.name,
                 )
             )
-            self.update_item(course)
+            self.update_item(course, user_id=user_id)
 
         return new_object
 
@@ -889,7 +909,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         if result['n'] == 0:
             raise ItemNotFoundError(location)
 
-    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False):
+    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False, isPublish=False):
         """
         Update the persisted version of xblock to reflect its current values.
 
@@ -903,7 +923,16 @@ class MongoModuleStore(ModuleStoreWriteBase):
             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,
+                }
             }
+
+            if isPublish:
+                payload['edit_info']['published_date'] = datetime.now(UTC)
+                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']})

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -157,7 +157,7 @@ class DraftModuleStore(MongoModuleStore):
 
         return wrap_draft(self._load_items(source_location.course_key, [original])[0])
 
-    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False):
+    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False, isPublish=False):
         """
         See superclass doc.
         In addition to the superclass's behavior, this method converts the unit to draft if it's not
@@ -175,7 +175,7 @@ class DraftModuleStore(MongoModuleStore):
                 raise
 
         xblock.location = draft_loc
-        super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found)
+        super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found, isPublish)
         # don't allow locations to truly represent themselves as draft outside of this file
         xblock.location = as_published(xblock.location)
 
@@ -194,6 +194,30 @@ class DraftModuleStore(MongoModuleStore):
 
         return
 
+    def has_changes(self, location):
+        """
+        Check if the xblock has been changed since it was last published.
+        :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
+        else:
+            return True
+
     def publish(self, location, published_by_id):
         """
         Save a current draft to the underlying modulestore
@@ -209,8 +233,6 @@ class DraftModuleStore(MongoModuleStore):
 
         draft = self.get_item(location)
 
-        draft.published_date = datetime.now(UTC)
-        draft.published_by = published_by_id
         if draft.has_children:
             if original_published is not None:
                 # see if children were deleted. 2 reasons for children lists to differ:
@@ -221,7 +243,7 @@ class DraftModuleStore(MongoModuleStore):
                         rents = self.get_parent_locations(child)
                         if (len(rents) == 1 and rents[0] == location):  # the 1 is this original_published
                             self.delete_item(child, True)
-        super(DraftModuleStore, self).update_item(draft, '**replace_user**')
+        super(DraftModuleStore, self).update_item(draft, published_by_id, isPublish=True)
         self.delete_item(location)
 
     def unpublish(self, location):

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -371,6 +371,20 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         return self._get_block_from_structure(course_structure, usage_key.block_id) is not None
 
+    def has_changes(self, usage_key):
+        """
+        Checks if the given block has unpublished changes
+        :param usage_key: the block to check
+        :return: True if the draft and published versions differ
+        """
+        draft = self.get_item(usage_key.for_branch("draft"))
+        try:
+            published = self.get_item(usage_key.for_branch("published"))
+        except ItemNotFoundError:
+            return True
+
+        return draft.update_version != published.update_version
+
     def get_item(self, usage_key, depth=0):
         """
         depth (int): An argument that some module stores may use to prefetch

common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py

@@ -8,6 +8,8 @@ import logging
 import shutil
 from tempfile import mkdtemp
 from uuid import uuid4
+from datetime import datetime
+from pytz import UTC
 import unittest
 from xblock.core import XBlock
 
@@ -21,6 +23,7 @@ from opaque_keys.edx.locations import Location
 from xmodule.modulestore import MONGO_MODULESTORE_TYPE
 from xmodule.modulestore.mongo import MongoModuleStore, MongoKeyValueStore
 from xmodule.modulestore.draft import DraftModuleStore
+from xmodule.modulestore.mongo.draft import as_draft
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation
 from xmodule.modulestore.xml_exporter import export_to_xml
 from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint
@@ -471,6 +474,120 @@ class TestMongoModuleStore(unittest.TestCase):
         finally:
             shutil.rmtree(root_dir)
 
+    def test_has_changes_direct_only(self):
+        """
+        Tests that has_changes() returns false when an 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')
+        dummy_user = 123
+
+        # Create dummy direct only xblocks
+        self.draft_store.create_and_save_xmodule(course_location, user_id=dummy_user)
+        self.draft_store.create_and_save_xmodule(chapter_location, user_id=dummy_user)
+
+        # Check that neither xblock has changes
+        self.assertFalse(self.draft_store.has_changes(course_location))
+        self.assertFalse(self.draft_store.has_changes(chapter_location))
+
+    def test_has_changes(self):
+        """
+        Tests that has_changes() only returns true when changes are present
+        """
+        location = Location('edX', 'changes', '2012_Fall', 'vertical', 'test_vertical')
+        dummy_user = 123
+
+        # Create a dummy component to test against
+        self.draft_store.create_and_save_xmodule(location, user_id=dummy_user)
+
+        # Not yet published, so changes are present
+        self.assertTrue(self.draft_store.has_changes(location))
+
+        # Publish and verify that there are no unpublished changes
+        self.draft_store.publish(location, dummy_user)
+        self.assertFalse(self.draft_store.has_changes(location))
+
+        # Change the component, then check that there now are changes
+        component = self.draft_store.get_item(location)
+        component.display_name = 'Changed Display Name'
+        self.draft_store.update_item(component, dummy_user)
+        self.assertTrue(self.draft_store.has_changes(location))
+
+        # Publish and verify again
+        self.draft_store.publish(location, dummy_user)
+        self.assertFalse(self.draft_store.has_changes(location))
+
+    def test_update_edit_info(self):
+        """
+        Tests that edited_on and edited_by are set correctly during an update
+        """
+        location = Location('edX', 'editInfoTest', '2012_Fall', 'html', 'test_html')
+        dummy_user = 123
+
+        # Create a dummy component to test against
+        self.draft_store.create_and_save_xmodule(location, user_id=dummy_user)
+
+        # 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, dummy_user)
+        old_edited_on = component.edited_on
+
+        # Change the component
+        component.display_name = component.display_name + ' Changed'
+        self.draft_store.update_item(component, 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, dummy_user)
+
+    def test_update_published_info(self):
+        """
+        Tests that published_date and published_by are set correctly
+        """
+        location = Location('edX', 'publishInfo', '2012_Fall', 'html', 'test_html')
+        create_user = 123
+        publish_user = 456
+
+        # Create a dummy component to test against
+        self.draft_store.create_and_save_xmodule(location, user_id=create_user)
+
+        # 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_date)
+        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
+        """
+
+        # Insert the test block directly into the module store
+        location = Location('edX', 'migration', '2012_Fall', 'html', 'test_html')
+        published_date = datetime(1970, 1, 1, tzinfo=UTC)
+        published_by = 123
+        self.store._update_single_item(
+            as_draft(location),
+            {
+                'definition.data': {},
+                'metadata': {
+                    # published_date was previously stored as a list of time components, not a datetime
+                    'published_date': list(published_date.timetuple()),
+                    'published_by': published_by,
+                },
+            },
+        )
+
+        # Retrieve the block and verify its fields
+        component = self.draft_store.get_item(location)
+        self.assertEqual(component.published_date, published_date)
+        self.assertEqual(component.published_by, published_by)
+
+
 
 class TestMongoKeyValueStore(object):
     """

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py

@@ -805,6 +805,32 @@ class SplitModuleItemTests(SplitModuleTest):
         with self.assertRaises(ItemNotFoundError):
             modulestore().get_item(course.location.for_branch("published"))
 
+    def test_has_changes(self):
+        """
+        Tests that has_changes() only returns true when changes are present
+        """
+        draft_course = CourseLocator(org='testx', offering='GreekHero', branch='draft')
+        published_course = CourseLocator(org='testx', offering='GreekHero', branch='published')
+        head = draft_course.make_usage_key('course', 'head12345')
+        dummy_user = 'testUser'
+
+        # Not yet published, so changes are present
+        self.assertTrue(modulestore().has_changes(head))
+
+        # Publish and verify that there are no unpublished changes
+        modulestore().xblock_publish(dummy_user, draft_course, published_course, [head], None)
+        self.assertFalse(modulestore().has_changes(head))
+
+        # Change the course, then check that there now are changes
+        course = modulestore().get_item(head)
+        course.show_calculator = not course.show_calculator
+        modulestore().update_item(course, dummy_user)
+        self.assertTrue(modulestore().has_changes(head))
+
+        # Publish and verify again
+        modulestore().xblock_publish(dummy_user, draft_course, published_course, [head], None)
+        self.assertFalse(modulestore().has_changes(head))
+
     def test_get_non_root(self):
         # not a course obj
         locator = BlockUsageLocator(