From ea97dd450882a0e626286da377f448f7f47e1836 Mon Sep 17 00:00:00 2001 From: Ben McMorran Date: Mon, 9 Jun 2014 15:53:40 -0400 Subject: [PATCH] Tracks last edited date and user, and adds has_changes for blocks. --- .../contentstore/course_info_model.py | 2 +- .../views/tests/test_course_updates.py | 2 +- .../models/settings/course_metadata.py | 4 - cms/envs/common.py | 3 +- cms/lib/xblock/mixin.py | 8 -- common/lib/xmodule/xmodule/error_module.py | 3 +- .../xmodule/xmodule/modulestore/mongo/base.py | 37 +++++- .../xmodule/modulestore/mongo/draft.py | 32 ++++- .../xmodule/modulestore/split_mongo/split.py | 14 +++ .../xmodule/modulestore/tests/test_mongo.py | 117 ++++++++++++++++++ .../tests/test_split_modulestore.py | 26 ++++ 11 files changed, 222 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/course_info_model.py b/cms/djangoapps/contentstore/course_info_model.py index d94138b67c..bf834458fb 100644 --- a/cms/djangoapps/contentstore/course_info_model.py +++ b/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 diff --git a/cms/djangoapps/contentstore/views/tests/test_course_updates.py b/cms/djangoapps/contentstore/views/tests/test_course_updates.py index 1ffcedfff4..33fdcadce5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_updates.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_updates.py @@ -135,7 +135,7 @@ class CourseUpdateTest(CourseTestCase): update_content = u"Hello world!" update_data = u"

  1. " + update_date + "

    " + update_content + "
" 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() diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 5b40aa5a11..ac3078e359 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -1,7 +1,6 @@ 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 diff --git a/cms/envs/common.py b/cms/envs/common.py index 112de27e76..8e0948e8c1 100644 --- a/cms/envs/common.py +++ b/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 diff --git a/cms/lib/xblock/mixin.py b/cms/lib/xblock/mixin.py index e4c9610a2b..950cc277f9 100644 --- a/cms/lib/xblock/mixin.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index 9ce938fea6..b39a34a6c1 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/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 ) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index e3f0e9c548..bd7807189d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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 @@ -206,6 +208,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 @@ -826,7 +845,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 @@ -837,12 +856,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) @@ -856,7 +876,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 @@ -888,7 +908,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. @@ -902,7 +922,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']}) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 2d5d26592c..839069b21d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 933f452b37..0e0d0714b7 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 063f05f973..7e4ab2e72f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 580b5f510a..10f86c6251 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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(