diff --git a/cms/djangoapps/contentstore/course_info_model.py b/cms/djangoapps/contentstore/course_info_model.py index 0ff15e94bb..f2714af540 100644 --- a/cms/djangoapps/contentstore/course_info_model.py +++ b/cms/djangoapps/contentstore/course_info_model.py @@ -36,7 +36,7 @@ def get_course_updates(location, provided_id, user_id): try: course_updates = modulestore().get_item(location) except ItemNotFoundError: - course_updates = modulestore().create_item(user_id, location) + course_updates = modulestore().create_item(user_id, location.course_key, location.block_type, location.block_id) course_update_items = get_course_update_items(course_updates, provided_id) return _get_visible_update(course_update_items) @@ -51,7 +51,7 @@ def update_course_updates(location, update, passed_id=None, user=None): try: course_updates = modulestore().get_item(location) except ItemNotFoundError: - course_updates = modulestore().create_item(user.id, location) + course_updates = modulestore().create_item(user.id, location.course_key, location.block_type, location.block_id) course_update_items = list(reversed(get_course_update_items(course_updates))) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 74b7dddb88..41702b64df 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -575,7 +575,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase): location = course.id.make_usage_key('chapter', 'neuvo') # Ensure draft mongo store does not create drafts for things that shouldn't be draft - newobject = draft_store.create_item(self.user.id, location) + newobject = draft_store.create_item(self.user.id, location.course_key, location.block_type, location.block_id) self.assertFalse(getattr(newobject, 'is_draft', False)) with self.assertRaises(InvalidVersionError): draft_store.convert_to_draft(location, self.user.id) @@ -1392,12 +1392,9 @@ class ContentStoreTest(ContentStoreTestCase): def test_forum_id_generation(self): course = CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') - new_component_location = course.id.make_usage_key('discussion', 'new_component') # crate a new module and add it as a child to a vertical - self.store.create_item(self.user.id, new_component_location) - - new_discussion_item = self.store.get_item(new_component_location) + new_discussion_item = self.store.create_item(self.user.id, course.id, 'discussion', 'new_component') self.assertNotEquals(new_discussion_item.discussion_id, '$$GUID$$') diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index f49cdb7f79..4625f8a2ad 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -162,7 +162,7 @@ class TemplateTests(unittest.TestCase): self.assertIsInstance(self.split_store.get_course(id_locator), CourseDescriptor) # and by guid self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) - self.split_store.delete_course(id_locator, ModuleStoreEnum.UserID.test) + self.split_store.delete_course(id_locator, 'testbot') # test can no longer retrieve by id self.assertRaises(ItemNotFoundError, self.split_store.get_course, id_locator) # but can by guid @@ -187,11 +187,11 @@ class TemplateTests(unittest.TestCase): ) first_problem.max_attempts = 3 first_problem.save() # decache the above into the kvs - updated_problem = self.split_store.update_item(first_problem, ModuleStoreEnum.UserID.test) + updated_problem = self.split_store.update_item(first_problem, 'testbot') self.assertIsNotNone(updated_problem.previous_version) self.assertEqual(updated_problem.previous_version, first_problem.update_version) self.assertNotEqual(updated_problem.update_version, first_problem.update_version) - self.split_store.delete_item(updated_problem.location, ModuleStoreEnum.UserID.test, 'testbot') + self.split_store.delete_item(updated_problem.location, 'testbot') second_problem = persistent_factories.ItemFactory.create( display_name='problem 2', diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index be9916235a..1b0c2f3281 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -34,7 +34,13 @@ class TestOrphan(CourseTestCase): location = self.course.location.replace(category=category, name=name) store = modulestore() store.create_item( - self.user.id, location, definition_data=data, metadata=metadata, runtime=runtime + self.user.id, + location.course_key, + location.block_type, + location.block_id, + definition_data=data, + metadata=metadata, + runtime=runtime ) if parent_name: # add child to parent in mongo diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 9e04ddbe71..93938e17c4 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -151,11 +151,11 @@ class CourseTestCase(ModuleStoreTestCase): self.assertEqual(self.store.compute_publish_state(draft_vertical), PublishState.draft) # create a Private (draft only) vertical - private_vertical = self.store.create_item(self.user.id, course_id.make_usage_key('vertical', self.PRIVATE_VERTICAL)) + private_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PRIVATE_VERTICAL) self.assertEqual(self.store.compute_publish_state(private_vertical), PublishState.private) # create a Published (no draft) vertical - public_vertical = self.store.create_item(self.user.id, course_id.make_usage_key('vertical', self.PUBLISHED_VERTICAL)) + public_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PUBLISHED_VERTICAL) public_vertical = self.store.publish(public_vertical.location, self.user.id) self.assertEqual(self.store.compute_publish_state(public_vertical), PublishState.public) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index b9532499c5..6ee193b7dd 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -287,7 +287,7 @@ def _save_item(user, usage_key, data=None, children=None, metadata=None, nullout if usage_key.category in CREATE_IF_NOT_FOUND: # New module at this location, for pages that are not pre-created. # Used for course info handouts. - existing_item = store.create_item(user.id, usage_key) + existing_item = store.create_item(user.id, usage_key.course_key, usage_key.block_type, usage_key.block_id) else: raise except InvalidLocationError: @@ -416,9 +416,11 @@ def _create_item(request): if display_name is not None: metadata['display_name'] = display_name - created_block = store.create_item( + created_block = store.create_child( request.user.id, - dest_usage_key, + usage_key, + dest_usage_key.block_type, + block_id=dest_usage_key.block_id, definition_data=data, metadata=metadata, runtime=parent.runtime, @@ -437,11 +439,6 @@ def _create_item(request): ) store.update_item(course, request.user.id) - # TODO replace w/ nicer accessor - if not 'detached' in parent.runtime.load_block_type(category)._class_tags: - parent.children.append(created_block.location) - store.update_item(parent, request.user.id) - return JsonResponse({"locator": unicode(created_block.location), "courseKey": unicode(created_block.location.course_key)}) @@ -467,7 +464,9 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ dest_module = store.create_item( user.id, - dest_usage_key, + dest_usage_key.course_key, + dest_usage_key.block_type, + block_id=dest_usage_key.block_id, definition_data=source_item.get_explicitly_set_fields_by_scope(Scope.content), metadata=duplicate_metadata, runtime=source_item.runtime, @@ -555,7 +554,7 @@ def _get_module_info(usage_key, user, rewrite_static_links=True): except ItemNotFoundError: if usage_key.category in CREATE_IF_NOT_FOUND: # Create a new one for certain categories only. Used for course info handouts. - module = store.create_item(user.id, usage_key) + module = store.create_item(user.id, usage_key.course_key, usage_key.block_type, block_id=usage_key.block_id) else: raise diff --git a/cms/djangoapps/contentstore/views/tests/test_course_updates.py b/cms/djangoapps/contentstore/views/tests/test_course_updates.py index 4558ab5733..8a0fa8fe36 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_updates.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_updates.py @@ -130,7 +130,12 @@ class CourseUpdateTest(CourseTestCase): ''' # get the updates and populate 'data' field with some data. location = self.course.id.make_usage_key('course_info', 'updates') - course_updates = modulestore().create_item(self.user.id, location) + course_updates = modulestore().create_item( + self.user.id, + location.course_key, + location.block_type, + block_id=location.block_id + ) update_date = u"January 23, 2014" update_content = u"Hello world!" update_data = u"

  1. " + update_date + "

    " + update_content + "
" @@ -204,7 +209,12 @@ class CourseUpdateTest(CourseTestCase): '''Test trying to add to a saved course_update which is not an ol.''' # get the updates and set to something wrong location = self.course.id.make_usage_key('course_info', 'updates') - modulestore().create_item(self.user.id, location) + modulestore().create_item( + self.user.id, + location.course_key, + location.block_type, + block_id=location.block_id + ) course_updates = modulestore().get_item(location) course_updates.data = 'bad news' modulestore().update_item(course_updates, self.user.id) @@ -229,8 +239,7 @@ class CourseUpdateTest(CourseTestCase): """ Test that a user can successfully post on course updates and handouts of a course """ - course_key = SlashSeparatedCourseKey('Org1', 'Course_1', 'Run_1') - course_update_url = self.create_update_url(course_key=course_key) + course_update_url = self.create_update_url(course_key=self.course.id) # create a course via the view handler self.client.ajax_post(course_update_url) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 220fca229a..048a516917 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -367,6 +367,25 @@ class ModuleStoreWrite(ModuleStoreRead): """ pass + @abstractmethod + def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): + """ + Creates and saves a new item in a course. + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create + this item in + block_type: The type of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + fields (dict): A dictionary specifying initial values for some or all fields + in the newly created block + """ + pass + @abstractmethod def clone_course(self, source_course_id, dest_course_id, user_id): """ @@ -561,54 +580,6 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): result[field.scope][field_name] = value return result - def create_item(self, user_id, location, parent_location=None, category=None, **kwargs): - """ - Creates and saves a new item. - Either location or (category, parent_location) or both must be provided. - If parent_location is provided, a new item of the given category is added as a child. - If location is not provided, a new item with the given category and given block_id - is added to the parent_location. If the block_id is not provided, a unique name - is automatically generated. - - Returns the newly created item. - - :param user_id: ID of the user creating and saving the xmodule - :param location: a Location--must have a category - :param parent_location: optional parameter, specifying the Location of the parent item - :param category: optional parameter for the category of the new item - :param block_id: a unique identifier for the new item - """ - raise NotImplementedError - - def update_item(self, xblock, user_id, allow_not_found=False, force=False): - """ - Update the given xblock's persisted repr. Pass the user's unique id which the persistent store - should save with the update if it has that ability. - - :param allow_not_found: whether this method should raise an exception if the given xblock - has not been persisted before. - :param force: fork the structure and don't update the course draftVersion if there's a version - conflict (only applicable to version tracking and conflict detecting persistence stores) - - :raises VersionConflictError: if org, course, run, and version_guid given and the current - version head != version_guid and force is not True. (only applicable to version tracking stores) - """ - raise NotImplementedError - - def delete_item(self, location, user_id, force=False): - """ - Delete an item from persistence. Pass the user's unique id which the persistent store - should save with the update if it has that ability. - - :param user_id: ID of the user deleting the item - :param force: fork the structure and don't update the course draftVersion if there's a version - conflict (only applicable to version tracking and conflict detecting persistence stores) - - :raises VersionConflictError: if org, course, run, and version_guid given and the current - version head != version_guid and force is not True. (only applicable to version tracking stores) - """ - raise NotImplementedError - def clone_course(self, source_course_id, dest_course_id, user_id): """ This base method just copies the assets. The lower level impls must do the actual cloning of @@ -639,6 +610,27 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): self.contentstore._drop_database() # pylint: disable=protected-access super(ModuleStoreWriteBase, self)._drop_database() # pylint: disable=protected-access + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): + """ + Creates and saves a new xblock that as a child of the specified block + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifing the + block that this item should be parented under + block_type: The type of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + fields (dict): A dictionary specifying initial values for some or all fields + in the newly created block + """ + item = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, fields=fields, **kwargs) + parent = self.get_item(parent_usage_key) + parent.children.append(item.location) + self.update_item(parent, user_id) + @contextmanager def bulk_write_operations(self, course_id): """ @@ -699,18 +691,3 @@ class EdxJSONEncoder(json.JSONEncoder): return obj.isoformat() else: return super(EdxJSONEncoder, self).default(obj) - - -def compute_location_from_args(location=None, parent_location=None, **kwargs): - """ - Returns a Location object that is generated from the given arguments, as follows: - If location is provided, returns it. - If location is not provided, returns a location with the given category and given block_id. - If the block_id is not provided, a unique name is automatically generated. - """ - if location is None: - block_id = kwargs.get('block_id', kwargs.get('name', kwargs.get('display_name', uuid4().hex))) - category = kwargs.pop('category') - course_key = getattr(parent_location, 'course_key', parent_location) - location = course_key.make_usage_key(category, Location.clean(block_id)) - return location diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 77a245bc75..c6b98d1ba3 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -12,8 +12,8 @@ class ModuleStoreDraftAndPublished(object): """ __metaclass__ = ABCMeta - def __init__(self, **kwargs): - super(ModuleStoreDraftAndPublished, self).__init__(**kwargs) + def __init__(self, *args, **kwargs): + super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs) self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only) @abstractmethod diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 3cbf893463..cf976d4a18 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -7,21 +7,23 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule import logging from contextlib import contextmanager -from opaque_keys import InvalidKeyError +import itertools -from . import ModuleStoreWriteBase -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.exceptions import ItemNotFoundError +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey -import itertools -from xmodule.modulestore.split_migrator import SplitMigrator + +from . import ModuleStoreWriteBase +from . import ModuleStoreEnum +from .exceptions import ItemNotFoundError +from .draft_and_published import ModuleStoreDraftAndPublished +from .split_migrator import SplitMigrator log = logging.getLogger(__name__) -class MixedModuleStore(ModuleStoreWriteBase): +class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): """ ModuleStore knows how to route requests to the right persistence ms """ @@ -282,7 +284,7 @@ class MixedModuleStore(ModuleStoreWriteBase): Returns: a CourseDescriptor """ - store = self._get_modulestore_for_courseid(None) + store = self._verify_modulestore_support(None, 'create_course') return store.create_course(org, course, run, user_id, **kwargs) def clone_course(self, source_course_id, dest_course_id, user_id): @@ -312,40 +314,57 @@ class MixedModuleStore(ModuleStoreWriteBase): source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run ) - def create_item(self, user_id, location=None, parent_location=None, **kwargs): + def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs): """ - Creates and saves a new item. - Either location or (category, parent_location) or both must be provided. - If parent_location is provided, a new item of the given category is added as a child. - If location is not provided, a new item with the given category and given block_id - is added to the parent_location. If the block_id is not provided, a unique name - is automatically generated. + Creates and saves a new item in a course. Returns the newly created item. - :param user_id: ID of the user creating and saving the xmodule - :param location: a Location--must have a category - :param parent_location: optional parameter, specifying the Location of the parent item - :param category: optional parameter for the category of the new item - :param block_id: a unique identifier for the new item + Args: + user_id: ID of the user creating and saving the xmodule + course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create + this item in + block_type: The typo of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + fields (dict): A dictionary specifying initial values for some or all fields + in the newly created block """ - location = compute_location_from_args(location, parent_location, **kwargs) - modulestore = self._verify_modulestore_support(location, 'create_item') - return modulestore.create_item(user_id, location, parent_location, **kwargs) + modulestore = self._verify_modulestore_support(course_key, 'create_item') + return modulestore.create_item(user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs) + + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs): + """ + Creates and saves a new xblock that is a child of the specified block + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifying the + block that this item should be parented under + block_type: The typo of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + fields (dict): A dictionary specifying initial values for some or all fields + in the newly created block + """ + modulestore = self._verify_modulestore_support(parent_usage_key.course_key, 'create_child') + return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs) def update_item(self, xblock, user_id, allow_not_found=False): """ Update the xblock persisted to be the same as the given for all types of fields (content, children, and metadata) attribute the change to the given user. """ - store = self._verify_modulestore_support(xblock.location, 'update_item') + store = self._verify_modulestore_support(xblock.location.course_key, 'update_item') return store.update_item(xblock, user_id, allow_not_found) def delete_item(self, location, user_id, **kwargs): """ Delete the given item from persistence. kwargs allow modulestore specific parameters. """ - store = self._verify_modulestore_support(location, 'delete_item') + store = self._verify_modulestore_support(location.course_key, 'delete_item') store.delete_item(location, user_id=user_id, **kwargs) def revert_to_published(self, location, user_id): @@ -358,7 +377,7 @@ class MixedModuleStore(ModuleStoreWriteBase): :raises InvalidVersionError: if no published version exists for the location specified """ - store = self._verify_modulestore_support(location, 'revert_to_published') + store = self._verify_modulestore_support(location.course_key, 'revert_to_published') return store.revert_to_published(location, user_id) def close_all_connections(self): @@ -388,7 +407,7 @@ class MixedModuleStore(ModuleStoreWriteBase): :param runtime: if you already have an xblock from the course, the xblock.runtime value :param fields: a dictionary of field names and values for the new xmodule """ - store = self._verify_modulestore_support(location, 'create_xmodule') + store = self._verify_modulestore_support(location.course_key, 'create_xmodule') return store.create_xmodule(location, definition_data, metadata, runtime, fields, **kwargs) def get_courses_for_wiki(self, wiki_slug): @@ -433,7 +452,7 @@ class MixedModuleStore(ModuleStoreWriteBase): Save a current draft to the underlying modulestore Returns the newly published item. """ - store = self._verify_modulestore_support(location, 'publish') + store = self._verify_modulestore_support(location.course_key, 'publish') return store.publish(location, user_id) def unpublish(self, location, user_id): @@ -441,7 +460,7 @@ class MixedModuleStore(ModuleStoreWriteBase): Save a current draft to the underlying modulestore Returns the newly unpublished item. """ - store = self._verify_modulestore_support(location, 'unpublish') + store = self._verify_modulestore_support(location.course_key, 'unpublish') return store.unpublish(location, user_id) def convert_to_draft(self, location, user_id): @@ -451,18 +470,26 @@ class MixedModuleStore(ModuleStoreWriteBase): :param source: the location of the source (its revision must be None) """ - store = self._verify_modulestore_support(location, 'convert_to_draft') + store = self._verify_modulestore_support(location.course_key, 'convert_to_draft') return store.convert_to_draft(location, user_id) - def _verify_modulestore_support(self, location, method): + 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 + """ + store = self._verify_modulestore_support(usage_key.course_key, 'has_changes') + return store.has_changes(usage_key) + + def _verify_modulestore_support(self, course_key, method): """ Finds and returns the store that contains the course for the given location, and verifying that the store supports the given method. Raises NotImplementedError if the found store does not support the given method. """ - course_id = location.course_key - store = self._get_modulestore_for_courseid(course_id) + store = self._get_modulestore_for_courseid(course_key) if hasattr(store, method): return store else: diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f24b39cca2..87741d01c4 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -17,6 +17,7 @@ import sys import logging import copy import re +from uuid import uuid4 from bson.son import SON from fs.osfs import OSFS @@ -904,7 +905,12 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): ])) location = course_id.make_usage_key('course', course_id.run) - course = self.create_item(user_id, location, fields=fields, **kwargs) + course = self.create_xmodule( + location, + fields=fields, + **kwargs + ) + self.update_item(course, user_id, allow_not_found=True) # clone a default 'about' overview module as well about_location = location.replace( @@ -914,7 +920,9 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): overview_template = AboutDescriptor.get_template('overview.yaml') self.create_item( user_id, - about_location, + about_location.course_key, + about_location.block_type, + block_id=about_location.block_id, definition_data=overview_template.get('data'), runtime=course.system ) @@ -975,6 +983,52 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): xmodule.save() return xmodule + def create_item(self, user_id, course_key, block_type, block_id=None, **kwargs): + """ + Creates and saves a new item in a course. + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create + this item in + block_type: The typo of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + """ + if block_id is None: + block_id = uuid4().hex + + location = course_key.make_usage_key(block_type, block_id) + xblock = self.create_xmodule(location, **kwargs) + self.update_item(xblock, user_id, allow_not_found=True) + + return xblock + + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, **kwargs): + """ + Creates and saves a new xblock that as a child of the specified block + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifing the + block that this item should be parented under + block_type: The typo of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + """ + xblock = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, **kwargs) + # attach to parent if given + if 'detached' not in xblock._class_tags: + parent = self.get_item(parent_usage_key) + parent.children.append(xblock.location) + self.update_item(parent, user_id) + + return xblock + def _get_course_for_item(self, location, depth=0): ''' for a given Xmodule, return the course that it belongs to @@ -1065,6 +1119,9 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): except ItemNotFoundError: if not allow_not_found: raise + elif not self.has_course(xblock.location.course_key): + raise ItemNotFoundError(xblock.location.course_key) + return xblock diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index d70342cc48..3af17a3442 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -11,7 +11,7 @@ import logging from opaque_keys.edx.locations import Location from xmodule.exceptions import InvalidVersionError -from xmodule.modulestore import PublishState, ModuleStoreEnum, compute_location_from_args +from xmodule.modulestore import PublishState, ModuleStoreEnum from xmodule.modulestore.exceptions import ( ItemNotFoundError, DuplicateItemError, InvalidBranchSetting, DuplicateCourseError ) @@ -305,35 +305,6 @@ class DraftModuleStore(MongoModuleStore): super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields) ) - def create_item(self, user_id, location=None, parent_location=None, **kwargs): - """ - Creates and saves a new item. - Either location or (category, parent_location) or both must be provided. - If parent_location is provided, a new item of the given category is added as a child. - If location is not provided, a new item with the given category and given block_id - is added to the parent_location. If the block_id is not provided, a unique name - is automatically generated. - - Returns the newly created item. - - :param user_id: ID of the user creating and saving the xmodule - :param location: a Location--must have a category - :param parent_location: optional parameter, specifying the Location of the parent item - :param category: optional parameter for the category of the new item - :param block_id: a unique identifier for the new item - """ - location = compute_location_from_args(location, parent_location, **kwargs) - xblock = self.create_xmodule(location, **kwargs) - self.update_item(xblock, user_id, allow_not_found=True) - - # attach to parent if given - if parent_location is not None and not 'detached' in xblock._class_tags: - parent = self.get_item(parent_location) - parent.children.append(location) - self.update_item(parent, user_id) - - return xblock - def get_items(self, course_key, settings=None, content=None, revision=None, **kwargs): """ Performance Note: This is generally a costly operation, but useful for wildcard searches. @@ -511,6 +482,7 @@ class DraftModuleStore(MongoModuleStore): ModuleStoreEnum.RevisionOption.published_only - removes only Published versions ModuleStoreEnum.RevisionOption.all - removes both Draft and Published parents currently only provided by contentstore.views.item.orphan_handler + Otherwise, raises a ValueError. """ self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred) _verify_revision_is_published(location) @@ -555,8 +527,10 @@ class DraftModuleStore(MongoModuleStore): as_functions = [as_draft, as_published] elif revision == ModuleStoreEnum.RevisionOption.published_only: as_functions = [as_published] - else: + elif revision is None: as_functions = [as_draft] + else: + raise ValueError('revision not one of None, ModuleStoreEnum.RevisionOption.published_only, or ModuleStoreEnum.RevisionOption.all') self._delete_subtree(location, as_functions) def _delete_subtree(self, location, as_functions): diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index c6aecdcb64..17e92db137 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -85,8 +85,8 @@ class SplitMigrator(object): # the 'children' field as it goes. _new_module = self.split_modulestore.create_item( user_id, - course_version_locator.make_usage_key(module.location.category, module.location.block_id), - parent_location=course_version_locator, + course_version_locator, + module.location.block_type, block_id=module.location.block_id, fields=self._get_json_fields_translate_references( module, course_version_locator, new_course.location.block_id @@ -131,7 +131,8 @@ class SplitMigrator(object): else: # only a draft version (aka, 'private'). _new_module = self.split_modulestore.create_item( - user_id, new_locator, parent_location=new_draft_course_loc, + user_id, new_draft_course_loc, + new_locator.block_type, block_id=new_locator.block_id, fields=self._get_json_fields_translate_references( module, new_draft_course_loc, published_course_usage_key.block_id diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index a743906418..7eb016c7c8 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -65,7 +65,7 @@ from opaque_keys.edx.locator import ( from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \ DuplicateCourseError from xmodule.modulestore import ( - inheritance, ModuleStoreWriteBase, ModuleStoreEnum, compute_location_from_args + inheritance, ModuleStoreWriteBase, ModuleStoreEnum ) from ..exceptions import ItemNotFoundError @@ -267,7 +267,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_version_guid: if provided, clear only this entry """ if course_version_guid: - del self.thread_cache.course_cache[course_version_guid] + try: + del self.thread_cache.course_cache[course_version_guid] + except KeyError: + pass else: self.thread_cache.course_cache = {} @@ -630,7 +633,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Find the history of this block. Return as a VersionTree of each place the block changed (except deletion). - The block's history tracks its explicit changes but not the changes in its children. + The block's history tracks its explicit changes but not the changes in its children starting + from when the block was created. ''' # course_agnostic means we don't care if the head and version don't align, trust the version @@ -640,7 +644,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): all_versions_with_block = self.db_connection.find_matching_structures( { 'original_version': course_struct['original_version'], - update_version_field: {'$exists': True} + update_version_field: {'$exists': True}, } ) # find (all) root versions and build map {previous: {successors}..} @@ -650,6 +654,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block_payload = self._get_block_from_structure(version, block_id) if version['_id'] == block_payload['edit_info']['update_version']: if block_payload['edit_info'].get('previous_version') is None: + # this was when this block was created possible_roots.append(block_payload['edit_info']['update_version']) else: # map previous to {update..} result.setdefault(block_payload['edit_info']['previous_version'], set()).add( @@ -763,19 +768,16 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): serial += 1 return category + str(serial) - # DHM: Should I rewrite this to take a new xblock instance rather than to construct it? That is, require the - # caller to use XModuleDescriptor.load_from_json thus reducing similar code and making the object creation and - # validation behavior a responsibility of the model layer rather than the persistence layer. - def create_item(self, user_id, location=None, parent_location=None, - definition_locator=None, force=False, continue_version=False, **kwargs + def create_item( + self, user_id, course_key, block_type, block_id=None, + definition_locator=None, fields=None, + force=False, continue_version=False, **kwargs ): """ - Add a descriptor to persistence as the last child of the optional parent_location or just as an element - of the course (if no parent provided). Return the resulting post saved version with populated locators. + Add a descriptor to persistence as an element + of the course. Return the resulting post saved version with populated locators. - :param course_or_parent_locator: If BlockUsageLocator, then it's assumed to be the parent. - If it's a CourseLocator, then it's - merely the containing course. If it has a version_guid and a course org + course + run + branch, this + :param course_key: If it has a version_guid and a course org + course + run + branch, this method ensures that the version is the head of the given course branch before making the change. raises InsufficientSpecificationError if there is no course locator. @@ -815,27 +817,15 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): the course id'd by version_guid but instead in one w/ a new version_guid. Ensure in this case that you get the new version_guid from the locator in the returned object! """ - location = compute_location_from_args(location, parent_location, **kwargs) - - if not isinstance(location, (CourseLocator, BlockUsageLocator)): - raise ValueError(u"Cannot create item {} in split. Wrong repr.".format(location)) - - # convert fields into a single dict if separated by scope - fields = kwargs.get('fields', {}) - fields.update(kwargs.pop('metadata', {})) - fields.update(kwargs.pop('definition_data', {})) - kwargs['fields'] = fields - # find course_index entry if applicable and structures entry - index_entry = self._get_index_if_valid(location, force, continue_version) - structure = self._lookup_course(location)['structure'] + index_entry = self._get_index_if_valid(course_key, force, continue_version) + structure = self._lookup_course(course_key)['structure'] - category = location.block_type - partitioned_fields = self.partition_fields_by_scope(category, fields) + partitioned_fields = self.partition_fields_by_scope(block_type, fields) new_def_data = partitioned_fields.get(Scope.content, {}) # persist the definition if persisted != passed if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)): - definition_locator = self.create_definition_from_data(new_def_data, category, user_id) + definition_locator = self.create_definition_from_data(new_def_data, block_type, user_id) elif new_def_data is not None: definition_locator, _ = self.update_definition_from_data(definition_locator, new_def_data, user_id) @@ -848,22 +838,21 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): new_id = new_structure['_id'] # generate usage id - block_id = kwargs.pop('block_id', location.block_id) if block_id is not None: if encode_key_for_mongo(block_id) in new_structure['blocks']: raise DuplicateItemError(block_id, self, 'structures') else: new_block_id = block_id else: - new_block_id = self._generate_block_id(new_structure['blocks'], category) + new_block_id = self._generate_block_id(new_structure['blocks'], block_type) block_fields = partitioned_fields.get(Scope.settings, {}) if Scope.children in partitioned_fields: block_fields.update(partitioned_fields[Scope.children]) self._update_block_in_structure(new_structure, new_block_id, { - "category": category, + "category": block_type, "definition": definition_locator.definition_id, - "fields": self._serialize_fields(category, block_fields), + "fields": self._serialize_fields(block_type, block_fields), 'edit_info': { 'edited_on': datetime.datetime.now(UTC), 'edited_by': user_id, @@ -872,17 +861,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } }) - # if given parent, add new block as child and update parent's version - parent = None - if isinstance(parent_location, BlockUsageLocator) and parent_location.block_id is not None: - encoded_block_id = encode_key_for_mongo(parent_location.block_id) - parent = new_structure['blocks'][encoded_block_id] - parent['fields'].setdefault('children', []).append(new_block_id) - if not continue_version or parent['edit_info']['update_version'] != structure['_id']: - parent['edit_info']['edited_on'] = datetime.datetime.now(UTC) - parent['edit_info']['edited_by'] = user_id - parent['edit_info']['previous_version'] = parent['edit_info']['update_version'] - parent['edit_info']['update_version'] = new_id if continue_version: # db update self.db_connection.update_structure(new_structure) @@ -894,22 +872,68 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # update the index entry if appropriate if index_entry is not None: if not continue_version: - self._update_head(index_entry, location.branch, new_id) + self._update_head(index_entry, course_key.branch, new_id) item_loc = BlockUsageLocator( - location.version_agnostic(), - block_type=category, + course_key.version_agnostic(), + block_type=block_type, block_id=new_block_id, ) else: item_loc = BlockUsageLocator( CourseLocator(version_guid=new_id), - block_type=category, + block_type=block_type, block_id=new_block_id, ) # reconstruct the new_item from the cache return self.get_item(item_loc) + def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, continue_version=False, **kwargs): + """ + Creates and saves a new xblock that as a child of the specified block + + Returns the newly created item. + + Args: + user_id: ID of the user creating and saving the xmodule + parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifying the + block that this item should be parented under + block_type: The typo of block to create + block_id: a unique identifier for the new item. If not supplied, + a new identifier will be generated + fields (dict): A dictionary specifying initial values for some or all fields + in the newly created block + """ + xblock = self.create_item( + user_id, parent_usage_key.course_key, block_type, block_id=block_id, fields=fields, + continue_version=continue_version, + **kwargs) + + # don't version the structure as create_item handled that already. + new_structure = self._lookup_course(xblock.location.course_key)['structure'] + + # add new block as child and update parent's version + encoded_block_id = encode_key_for_mongo(parent_usage_key.block_id) + parent = new_structure['blocks'][encoded_block_id] + parent['fields'].setdefault('children', []).append(xblock.location.block_id) + if parent['edit_info']['update_version'] != new_structure['_id']: + # if the parent hadn't been previously changed in this bulk transaction, indicate that it's + # part of the bulk transaction + parent['edit_info'] = { + 'edited_on': datetime.datetime.now(UTC), + 'edited_by': user_id, + 'previous_version': parent['edit_info']['update_version'], + 'update_version': new_structure['_id'], + } + + # db update + self.db_connection.update_structure(new_structure) + # clear cache so things get refetched and inheritance recomputed + self._clear_cache(new_structure['_id']) + + # don't need to update the index b/c create_item did it for this version + return xblock + def clone_course(self, source_course_id, dest_course_id, user_id): """ See :meth: `.ModuleStoreWrite.clone_course` for documentation. 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 f19860e282..844fee3ffc 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -6,6 +6,7 @@ from ..exceptions import ItemNotFoundError from split import SplitMongoModuleStore from xmodule.modulestore import ModuleStoreEnum, PublishState from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished +from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore): @@ -13,13 +14,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS A subclass of Split that supports a dual-branch fall-back versioning framework with a Draft branch that falls back to a Published branch. """ - def __init__(self, **kwargs): - super(DraftVersioningModuleStore, self).__init__(**kwargs) - def create_course(self, org, course, run, user_id, **kwargs): master_branch = kwargs.pop('master_branch', ModuleStoreEnum.BranchName.draft) return super(DraftVersioningModuleStore, self).create_course( - org, course, run, user_id, master_branch, **kwargs + org, course, run, user_id, master_branch=master_branch, **kwargs ) def get_courses(self): @@ -31,32 +29,81 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS def delete_item(self, location, user_id, revision=None, **kwargs): """ Delete the given item from persistence. kwargs allow modulestore specific parameters. + + Args: + location: UsageKey of the item to be deleted + user_id: id of the user deleting the item + revision: + None - deletes the item and its subtree, and updates the parents per description above + ModuleStoreEnum.RevisionOption.published_only - removes only Published versions + ModuleStoreEnum.RevisionOption.all - removes both Draft and Published parents + currently only provided by contentstore.views.item.orphan_handler + Otherwise, raises a ValueError. """ if revision == ModuleStoreEnum.RevisionOption.published_only: branches_to_delete = [ModuleStoreEnum.BranchName.published] elif revision == ModuleStoreEnum.RevisionOption.all: branches_to_delete = [ModuleStoreEnum.BranchName.published, ModuleStoreEnum.BranchName.draft] - else: + elif revision is None: branches_to_delete = [ModuleStoreEnum.BranchName.draft] + else: + raise ValueError('revision not one of None, ModuleStoreEnum.RevisionOption.published_only, or ModuleStoreEnum.RevisionOption.all') for branch in branches_to_delete: SplitMongoModuleStore.delete_item(self, location.for_branch(branch), user_id, **kwargs) + def _map_revision_to_branch(self, key, revision=None): + """ + Maps RevisionOptions to BranchNames, inserting them into the key + """ + if revision == ModuleStoreEnum.RevisionOption.published_only: + return key.for_branch(ModuleStoreEnum.BranchName.published) + elif revision == ModuleStoreEnum.RevisionOption.draft_only: + return key.for_branch(ModuleStoreEnum.BranchName.draft) + else: + return key + + def has_item(self, usage_key, revision=None): + """ + Returns True if location exists in this ModuleStore. + """ + usage_key = self._map_revision_to_branch(usage_key, revision=revision) + return super(DraftVersioningModuleStore, self).has_item(usage_key) + def get_item(self, usage_key, depth=0, revision=None): """ Returns the item identified by usage_key and revision. """ - def for_branch(branch_state): - if usage_key.branch is not None and usage_key.branch is not branch_state: - raise ValueError('{} already has a branch.'.format(usage_key)) - return usage_key.for_branch(branch_state) - if revision == ModuleStoreEnum.RevisionOption.published_only: - usage_key = for_branch(ModuleStoreEnum.BranchName.published) - elif revision == ModuleStoreEnum.RevisionOption.draft_only: - usage_key = for_branch(ModuleStoreEnum.BranchName.draft) + usage_key = self._map_revision_to_branch(usage_key, revision=revision) return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth) + def get_items(self, course_locator, settings=None, content=None, revision=None, **kwargs): + """ + Returns a list of XModuleDescriptor instances for the matching items within the course with + the given course_locator. + """ + course_locator = self._map_revision_to_branch(course_locator, revision=revision) + return super(DraftVersioningModuleStore, self).get_items( + course_locator, + settings=settings, + content=content, + **kwargs + ) + def get_parent_location(self, location, revision=None, **kwargs): - # NAATODO - support draft_preferred + ''' + Returns the given location's parent location in this course. + Args: + revision: + None - uses the branch setting for the 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 + ''' + if revision == ModuleStoreEnum.RevisionOption.draft_preferred: + revision = ModuleStoreEnum.RevisionOption.draft_only + location = self._map_revision_to_branch(location, revision=revision) return SplitMongoModuleStore.get_parent_location(self, location, **kwargs) def has_changes(self, usage_key): @@ -65,6 +112,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :param usage_key: the block to check :return: True if the draft and published versions differ """ + # TODO for better performance: lookup the courses and get the block entry, don't create the instances draft = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.draft)) try: published = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.published)) @@ -120,20 +168,21 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure'] return self._get_block_from_structure(course_structure, xblock.location.block_id) - if xblock.location.branch is None: - raise ValueError(u'{} is not in a branch; so, this is nonsensical'.format(xblock.location)) if xblock.location.branch == ModuleStoreEnum.BranchName.draft: - other = get_head(ModuleStoreEnum.BranchName.published) + try: + other = get_head(ModuleStoreEnum.BranchName.published) + except ItemNotFoundError: + return PublishState.private elif xblock.location.branch == ModuleStoreEnum.BranchName.published: other = get_head(ModuleStoreEnum.BranchName.draft) else: - raise ValueError(u'{} is not in a branch other than draft or published; so, this is nonsensical'.format(xblock.location)) + raise ValueError(u'{} is in a branch other than draft or published.'.format(xblock.location)) if not other: if xblock.location.branch == ModuleStoreEnum.BranchName.draft: return PublishState.private else: - return PublishState.public # a bit nonsensical + return PublishState.public elif xblock.update_version != other['edit_info']['update_version']: return PublishState.draft else: diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 885182fd75..8f5453d91e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -174,7 +174,15 @@ class ItemFactory(XModuleFactory): if display_name is not None: metadata['display_name'] = display_name runtime = parent.runtime if parent else None - store.create_item(user_id, location, metadata=metadata, definition_data=data, runtime=runtime) + store.create_item( + user_id, + location.course_key, + location.block_type, + block_id=location.block_id, + metadata=metadata, + definition_data=data, + runtime=runtime + ) module = store.get_item(location) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py index d678724da0..bf00f73cf5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py @@ -4,6 +4,7 @@ from xmodule.course_module import CourseDescriptor from xmodule.x_module import XModuleDescriptor import factory from factory.helpers import lazy_attribute +from opaque_keys.edx.keys import UsageKey # Factories don't have __init__ methods, and are self documenting # pylint: disable=W0232, C0111 @@ -73,10 +74,16 @@ class ItemFactory(SplitFactory): :param definition_locator (optional): the DescriptorLocator for the definition this uses or branches """ modulestore = kwargs.pop('modulestore') - return modulestore.create_item( - user_id, category=category, parent_location=parent_location, defintion_locator=definition_locator, - force=force, continue_version=continue_version, **kwargs - ) + if isinstance(parent_location, UsageKey): + return modulestore.create_child( + user_id, parent_location, category, defintion_locator=definition_locator, + force=force, continue_version=continue_version, **kwargs + ) + else: + return modulestore.create_item( + user_id, parent_location, category, defintion_locator=definition_locator, + force=force, continue_version=continue_version, **kwargs + ) @classmethod def _build(cls, target_class, *args, **kwargs): 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 951d71c518..50300ff184 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -117,24 +117,21 @@ class TestMixedModuleStore(unittest.TestCase): """ Create a course w/ one item in the persistence store using the given course & item location. """ + # create course self.course = self.store.create_course(course_key.org, course_key.course, course_key.run, self.user_id) - self.writable_chapter_location = self.course.id.make_usage_key('chapter', 'Overview').version_agnostic() - block_id = self.writable_chapter_location.name - chapter = self.store.create_item( - # don't use course_location as it may not be the repr - self.user_id, self.writable_chapter_location, - parent_location=self.course.location, block_id=block_id - ) if isinstance(self.course.id, CourseLocator): self.course_locations[self.MONGO_COURSEID] = self.course.location.version_agnostic() else: self.assertEqual(self.course.id, course_key) - self.assertEqual(chapter.location, self.writable_chapter_location) + + # create chapter + chapter = self.store.create_child(self.user_id, self.course.location, 'chapter', block_id='Overview') + self.writable_chapter_location = chapter.location.version_agnostic() def _create_block_hierarchy(self): """ Creates a hierarchy of blocks for testing - Each block is assigned as a field of the class and can be easily accessed + Each block's (version_agnostic) location is assigned as a field of the class and can be easily accessed """ BlockInfo = namedtuple('BlockInfo', 'field_name, category, display_name, sub_tree') @@ -173,15 +170,13 @@ class TestMixedModuleStore(unittest.TestCase): ] def create_sub_tree(parent, block_info): - block = self.store.create_item( - self.user_id, parent_location=parent.location.version_agnostic(), - category=block_info.category, block_id=block_info.display_name + block = self.store.create_child( + self.user_id, parent.location.version_agnostic(), + block_info.category, block_id=block_info.display_name ) for tree in block_info.sub_tree: create_sub_tree(block, tree) - # reload the block to update its children field - block = self.store.get_item(block.location.version_agnostic()) - setattr(self, block_info.field_name, block) + setattr(self, block_info.field_name, block.location.version_agnostic()) for tree in trees: create_sub_tree(self.course, tree) @@ -317,17 +312,17 @@ class TestMixedModuleStore(unittest.TestCase): self.store.delete_item(self.writable_chapter_location, self.user_id) # verify it's gone with self.assertRaises(ItemNotFoundError): - self.store.get_item(self.writable_chapter_location.version_agnostic()) + self.store.get_item(self.writable_chapter_location) # create and delete a private vertical with private children - private_vert = self.store.create_item( + private_vert = self.store.create_child( # don't use course_location as it may not be the repr - self.user_id, parent_location=self.course_locations[self.MONGO_COURSEID], - category='vertical', block_id='private' + self.user_id, self.course_locations[self.MONGO_COURSEID], + 'vertical', block_id='private' ) - private_leaf = self.store.create_item( + private_leaf = self.store.create_child( # don't use course_location as it may not be the repr - self.user_id, parent_location=private_vert.location, category='html', block_id='private_leaf' + self.user_id, private_vert.location, 'html', block_id='private_leaf' ) # verify pre delete state (just to verify that the test is valid) @@ -357,18 +352,18 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_item(leaf_loc)) self.assertNotIn(vert_loc, course.children) - # NAATODO enable for split after your converge merge - if default_ms == 'split': - return + # TODO can remove this once LMS-2869 is implemented + # first create a Published branch + self.store.publish(self.course_locations[self.MONGO_COURSEID], self.user_id) # reproduce bug STUD-1965 # create and delete a private vertical with private children - private_vert = self.store.create_item( + private_vert = self.store.create_child( # don't use course_location as it may not be the repr - self.course_locations[self.MONGO_COURSEID], 'vertical', user_id=self.user_id, block_id='publish' + self.user_id, self.course_locations[self.MONGO_COURSEID], 'vertical', block_id='publish' ) - private_leaf = self.store.create_item( - private_vert.location, 'html', user_id=self.user_id, block_id='bug_leaf' + private_leaf = self.store.create_child( + self.user_id, private_vert.location, 'html', block_id='bug_leaf' ) self.store.publish(private_vert.location, self.user_id) @@ -435,14 +430,13 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(parent, self.course_locations[self.XML_COURSEID1]) def verify_get_parent_locations_results(self, expected_results): - # expected_results should be a list of (child, parent, revision) - for test in expected_results: + for child_location, parent_location, revision in expected_results: self.assertEqual( - test[1].location if test[1] else None, - self.store.get_parent_location(test[0].location, revision=test[2]) + parent_location, + self.store.get_parent_location(child_location, revision=revision) ) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_get_parent_locations_moved_child(self, default_ms): self.initdb(default_ms) self._create_block_hierarchy() @@ -451,30 +445,34 @@ class TestMixedModuleStore(unittest.TestCase): self.store.publish(self.course.location, self.user_id) # make drafts of verticals - self.store.convert_to_draft(self.vertical_x1a.location, self.user_id) - self.store.convert_to_draft(self.vertical_y1a.location, self.user_id) + self.store.convert_to_draft(self.vertical_x1a, self.user_id) + self.store.convert_to_draft(self.vertical_y1a, self.user_id) # move child problem_x1a_1 to vertical_y1a - child_to_move = self.problem_x1a_1 - old_parent = self.vertical_x1a - new_parent = self.vertical_y1a - old_parent.children.remove(child_to_move.location) - new_parent.children.append(child_to_move.location) + child_to_move_location = self.problem_x1a_1 + new_parent_location = self.vertical_y1a + old_parent_location = self.vertical_x1a + + old_parent = self.store.get_item(old_parent_location) + old_parent.children.remove(child_to_move_location.replace(version_guid=old_parent.location.version_guid)) self.store.update_item(old_parent, self.user_id) + + new_parent = self.store.get_item(new_parent_location) + new_parent.children.append(child_to_move_location.replace(version_guid=new_parent.location.version_guid)) self.store.update_item(new_parent, self.user_id) self.verify_get_parent_locations_results([ - (child_to_move, new_parent, None), - (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_move, old_parent, ModuleStoreEnum.RevisionOption.published_only), + (child_to_move_location, new_parent_location, None), + (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), + (child_to_move_location, old_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only), ]) # publish the course again self.store.publish(self.course.location, self.user_id) self.verify_get_parent_locations_results([ - (child_to_move, new_parent, None), - (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.published_only), + (child_to_move_location, new_parent_location, None), + (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), + (child_to_move_location, new_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only), ]) @ddt.data('draft') @@ -486,29 +484,28 @@ class TestMixedModuleStore(unittest.TestCase): self.store.publish(self.course.location, self.user_id) # make draft of vertical - self.store.convert_to_draft(self.vertical_y1a.location, self.user_id) + self.store.convert_to_draft(self.vertical_y1a, self.user_id) # delete child problem_y1a_1 - child_to_delete = self.problem_y1a_1 - old_parent = self.vertical_y1a - self.store.delete_item(child_to_delete.location, self.user_id) + child_to_delete_location = self.problem_y1a_1 + old_parent_location = self.vertical_y1a + self.store.delete_item(child_to_delete_location, self.user_id) self.verify_get_parent_locations_results([ - (child_to_delete, old_parent, None), + (child_to_delete_location, old_parent_location, None), # Note: The following could be an unexpected result, but we want to avoid an extra database call - (child_to_delete, old_parent, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_delete, old_parent, ModuleStoreEnum.RevisionOption.published_only), + (child_to_delete_location, old_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), + (child_to_delete_location, old_parent_location, ModuleStoreEnum.RevisionOption.published_only), ]) # publish the course again self.store.publish(self.course.location, self.user_id) self.verify_get_parent_locations_results([ - (child_to_delete, None, None), - (child_to_delete, None, ModuleStoreEnum.RevisionOption.draft_preferred), - (child_to_delete, None, ModuleStoreEnum.RevisionOption.published_only), + (child_to_delete_location, None, None), + (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.draft_preferred), + (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.published_only), ]) - @ddt.data('draft') def test_revert_to_published_root_draft(self, default_ms): """ @@ -516,23 +513,25 @@ class TestMixedModuleStore(unittest.TestCase): """ self.initdb(default_ms) self._create_block_hierarchy() - vertical_children_num = len(self.vertical_x1a.children) + + vertical = self.store.get_item(self.vertical_x1a) + vertical_children_num = len(vertical.children) self.store.publish(self.course.location, self.user_id) # delete leaf problem (will make parent vertical a draft) - self.store.delete_item(self.problem_x1a_1.location, self.user_id) + self.store.delete_item(self.problem_x1a_1, self.user_id) - draft_parent = self.store.get_item(self.vertical_x1a.location) + draft_parent = self.store.get_item(self.vertical_x1a) self.assertEqual(vertical_children_num - 1, len(draft_parent.children)) published_parent = self.store.get_item( - self.vertical_x1a.location, + self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.published_only ) self.assertEqual(vertical_children_num, len(published_parent.children)) - self.store.revert_to_published(self.vertical_x1a.location, self.user_id) - reverted_parent = self.store.get_item(self.vertical_x1a.location) + self.store.revert_to_published(self.vertical_x1a, self.user_id) + reverted_parent = self.store.get_item(self.vertical_x1a) self.assertEqual(vertical_children_num, len(published_parent.children)) self.assertEqual(reverted_parent, published_parent) @@ -545,14 +544,15 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() self.store.publish(self.course.location, self.user_id) - orig_display_name = self.problem_x1a_1.display_name + problem = self.store.get_item(self.problem_x1a_1) + orig_display_name = problem.display_name # Change display name of problem and update just it (so parent remains published) - self.problem_x1a_1.display_name = "updated before calling revert" - self.store.update_item(self.problem_x1a_1, self.user_id) - self.store.revert_to_published(self.vertical_x1a.location, self.user_id) + problem.display_name = "updated before calling revert" + self.store.update_item(problem, self.user_id) + self.store.revert_to_published(self.vertical_x1a, self.user_id) - reverted_problem = self.store.get_item(self.problem_x1a_1.location) + reverted_problem = self.store.get_item(self.problem_x1a_1) self.assertEqual(orig_display_name, reverted_problem.display_name) @ddt.data('draft') @@ -564,9 +564,9 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() self.store.publish(self.course.location, self.user_id) - orig_vertical = self.vertical_x1a - self.store.revert_to_published(self.vertical_x1a.location, self.user_id) - reverted_vertical = self.store.get_item(self.vertical_x1a.location) + orig_vertical = self.store.get_item(self.vertical_x1a) + self.store.revert_to_published(self.vertical_x1a, self.user_id) + reverted_vertical = self.store.get_item(self.vertical_x1a) self.assertEqual(orig_vertical, reverted_vertical) @ddt.data('draft') @@ -577,7 +577,7 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) self._create_block_hierarchy() with self.assertRaises(InvalidVersionError): - self.store.revert_to_published(self.vertical_x1a.location, self.user_id) + self.store.revert_to_published(self.vertical_x1a, self.user_id) @ddt.data('draft') def test_revert_to_published_direct_only(self, default_ms): @@ -586,8 +586,8 @@ class TestMixedModuleStore(unittest.TestCase): """ self.initdb(default_ms) self._create_block_hierarchy() - self.store.revert_to_published(self.sequential_x1.location, self.user_id) - reverted_parent = self.store.get_item(self.sequential_x1.location) + self.store.revert_to_published(self.sequential_x1, self.user_id) + reverted_parent = self.store.get_item(self.sequential_x1) # It does not discard the child vertical, even though that child is a draft (with no published version) self.assertEqual(1, len(reverted_parent.children)) @@ -596,6 +596,9 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) course_id = self.course_locations[self.MONGO_COURSEID].course_key + # create parented children + self._create_block_hierarchy() + # orphans orphan_locations = [ course_id.make_usage_key('chapter', 'OrphanChapter'), @@ -612,7 +615,12 @@ class TestMixedModuleStore(unittest.TestCase): ] for location in (orphan_locations + detached_locations): - self.store.create_item(self.user_id, location) + self.store.create_item( + self.user_id, + location.course_key, + location.block_type, + block_id=location.block_id + ) found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key) self.assertEqual(set(found_orphans), set(orphan_locations)) @@ -624,9 +632,11 @@ class TestMixedModuleStore(unittest.TestCase): new location for the child """ self.initdb(default_ms) - self.store.create_item( - self.user_id, parent_location=self.course_locations[self.MONGO_COURSEID], - category='problem', block_id='orphan' + self.store.create_child( + self.user_id, + self.course_locations[self.MONGO_COURSEID], + 'problem', + block_id='orphan' ) orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key) self.assertEqual(len(orphans), 0, "unexpected orphans: {}".format(orphans)) @@ -655,32 +665,80 @@ class TestMixedModuleStore(unittest.TestCase): """ self.initdb(default_ms) self._create_block_hierarchy() + # publish self.store.publish(self.course.location, self.user_id) published_xblock = self.store.get_item( - self.vertical_x1a.location.replace(branch=None), + self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.published_only ) self.assertIsNotNone(published_xblock) # unpublish - self.store.unpublish(self.vertical_x1a.location, self.user_id) + self.store.unpublish(self.vertical_x1a, self.user_id) with self.assertRaises(ItemNotFoundError): self.store.get_item( - self.vertical_x1a.location.replace(branch=None), + self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.published_only ) - draft_xblock_from_get_item = self.store.get_item( - self.vertical_x1a.location.replace(branch=None), + # make sure draft version still exists + draft_xblock = self.store.get_item( + self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.draft_only ) + self.assertIsNotNone(draft_xblock) + + @ddt.data('draft', 'split') + def test_compute_publish_state(self, default_ms): + """ + Test the compute_publish_state method + """ + self.initdb(default_ms) + self._create_block_hierarchy() + + # TODO - Remove this call to explicitly Publish the course once LMS-2869 is implemented + # For now, we need this since we can't publish a child item without its course already been published + course_location = self.course_locations[self.MONGO_COURSEID] + self.store.publish(course_location, self.user_id) + + # start off as Private + item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state') + item_location = item.location.version_agnostic() + self.assertEquals(self.store.compute_publish_state(item), PublishState.private) + + # Private -> Public + self.store.publish(item_location, self.user_id) + item = self.store.get_item(item_location) + self.assertEquals(self.store.compute_publish_state(item), PublishState.public) + + # Public -> Private + self.store.unpublish(item_location, self.user_id) + item = self.store.get_item(item_location) + self.assertEquals(self.store.compute_publish_state(item), PublishState.private) + + # Private -> Public + self.store.publish(item_location, self.user_id) + item = self.store.get_item(item_location) + self.assertEquals(self.store.compute_publish_state(item), PublishState.public) + + # Public -> Draft with NO changes + # Note: This is where Split and Mongo differ + self.store.convert_to_draft(item_location, self.user_id) + item = self.store.get_item(item_location) self.assertEquals( - published_xblock.location.version_agnostic().replace(branch=None), - draft_xblock_from_get_item.location.version_agnostic().replace(branch=None) + self.store.compute_publish_state(item), + PublishState.draft if default_ms == 'draft' else PublishState.public ) + # Draft WITH changes + item.display_name = 'new name' + item = self.store.update_item(item, self.user_id) + self.assertTrue(self.store.has_changes(item.location)) + self.assertEquals(self.store.compute_publish_state(item), PublishState.draft) + + #============================================================================================================= # General utils for not using django settings #============================================================================================================= diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 1dfb41d551..ed0b4d69fd 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -391,12 +391,27 @@ class TestMongoModuleStore(unittest.TestCase): course = self.draft_store.get_course(course_key) # can't use item factory as it depends on django settings p1ele = self.draft_store.create_item( - 99, course.id.make_usage_key('problem', 'p1'), runtime=course.runtime) + 99, + course_key, + 'problem', + block_id='p1', + runtime=course.runtime + ) p2ele = self.draft_store.create_item( - 99, course.id.make_usage_key('problem', 'p2'), runtime=course.runtime) + 99, + course_key, + 'problem', + block_id='p2', + runtime=course.runtime + ) self.refloc = course.id.make_usage_key('ref_test', 'ref_test') self.draft_store.create_item( - 99,self.refloc, runtime=course.runtime, fields={ + 99, + self.refloc.course_key, + self.refloc.block_type, + block_id=self.refloc.block_id, + runtime=course.runtime, + fields={ 'reference_link': p1ele.location, 'reference_list': [p1ele.location, p2ele.location], 'reference_dict': {'p1': p1ele.location, 'p2': p2ele.location}, @@ -497,12 +512,16 @@ class TestMongoModuleStore(unittest.TestCase): """ 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') + course_location = Location('edX', 'toy', '2012_Fall', 'course', '2012_Fall') + chapter_location = Location('edX', 'toy', '2012_Fall', 'chapter', 'vertical_container') # Create dummy direct only xblocks - self.draft_store.create_item(self.dummy_user, course_location) - self.draft_store.create_item(self.dummy_user, chapter_location) + self.draft_store.create_item( + self.dummy_user, + chapter_location.course_key, + chapter_location.block_type, + block_id=chapter_location.block_id + ) # Check that neither xblock has changes self.assertFalse(self.draft_store.has_changes(course_location)) @@ -512,10 +531,15 @@ class TestMongoModuleStore(unittest.TestCase): """ Tests that has_changes() only returns true when changes are present """ - location = Location('edX', 'changes', '2012_Fall', 'vertical', 'test_vertical') + location = Location('edX', 'toy', '2012_Fall', 'vertical', 'test_vertical') # Create a dummy component to test against - self.draft_store.create_item(self.dummy_user, location) + self.draft_store.create_item( + self.dummy_user, + location.course_key, + location.block_type, + block_id=location.block_id + ) # Not yet published, so changes are present self.assertTrue(self.draft_store.has_changes(location)) @@ -538,11 +562,16 @@ class TestMongoModuleStore(unittest.TestCase): """ Tests that has_changes() returns False when a published parent points to a child that doesn't exist. """ - location = Location('edX', 'missing', '2012_Fall', 'sequential', 'parent') + location = Location('edX', 'toy', '2012_Fall', 'sequential', 'parent') # Create the parent and point it to a fake child - parent = self.draft_store.create_item(self.dummy_user, location) - parent.children += [Location('edX', 'missing', '2012_Fall', 'vertical', 'does_not_exist')] + parent = self.draft_store.create_item( + self.dummy_user, + location.course_key, + location.block_type, + block_id=location.block_id + ) + parent.children += [Location('edX', 'toy', '2012_Fall', 'vertical', 'does_not_exist')] self.draft_store.update_item(parent, self.dummy_user) # Check the parent for changes should return False and not throw an exception @@ -561,27 +590,39 @@ class TestMongoModuleStore(unittest.TestCase): if user_id is None: user_id = self.dummy_user - 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'), - } + org = 'edX' + course = 'tree{}'.format(name) + run = name - for key in locations: - self.draft_store.create_item(user_id, locations[key]) + if not self.draft_store.has_course(SlashSeparatedCourseKey(org, course, run)): + self.draft_store.create_course(org, course, run, 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) + locations = { + 'grandparent': Location(org, course, run, 'chapter', 'grandparent'), + 'parent_sibling': Location(org, course, run, 'sequential', 'parent_sibling'), + 'parent': Location(org, course, run, 'sequential', 'parent'), + 'child_sibling': Location(org, course, run, 'vertical', 'child_sibling'), + 'child': Location(org, course, run, 'vertical', 'child'), + } - 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) + for key in locations: + self.draft_store.create_item( + user_id, + locations[key].course_key, + locations[key].block_type, + block_id=locations[key].block_id + ) - self.draft_store.publish(locations['parent'], user_id) - self.draft_store.publish(locations['parent_sibling'], 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 @@ -663,10 +704,12 @@ class TestMongoModuleStore(unittest.TestCase): # 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_item(self.dummy_user, new_child_location) - parent = self.draft_store.get_item(locations['parent']) - parent.children += [new_child_location] - self.draft_store.update_item(parent, user_id=self.dummy_user) + self.draft_store.create_child( + self.dummy_user, + locations['parent'], + new_child_location.block_type, + block_id=new_child_location.block_id + ) # Verify that the ancestors now have changes self.assertTrue(self.draft_store.has_changes(locations['grandparent'])) @@ -685,13 +728,21 @@ class TestMongoModuleStore(unittest.TestCase): """ Tests that has_changes() returns true after editing the child of a vertical (both not direct only categories). """ - parent_location = Location('edX', 'test', 'non_direct_only_children', 'vertical', 'parent') - child_location = Location('edX', 'test', 'non_direct_only_children', 'html', 'child') + parent_location = Location('edX', 'toy', '2012_Fall', 'vertical', 'parent') + child_location = Location('edX', 'toy', '2012_Fall', 'html', 'child') - parent = self.draft_store.create_item(self.dummy_user, parent_location) - child = self.draft_store.create_item(self.dummy_user, child_location) - parent.children += [child_location] - self.draft_store.update_item(parent, user_id=self.dummy_user) + parent = self.draft_store.create_item( + self.dummy_user, + parent_location.course_key, + parent_location.block_type, + block_id=parent_location.block_id + ) + child = self.draft_store.create_child( + self.dummy_user, + parent_location, + child_location.block_type, + block_id=child_location.block_id + ) self.draft_store.publish(parent_location, self.dummy_user) # Verify that there are no changes @@ -757,10 +808,15 @@ class TestMongoModuleStore(unittest.TestCase): """ Tests that edited_on and edited_by are set correctly during an update """ - location = Location('edX', 'editInfoTest', '2012_Fall', 'html', 'test_html') + 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) + 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) @@ -780,12 +836,17 @@ class TestMongoModuleStore(unittest.TestCase): """ Tests that published_date and published_by are set correctly """ - location = Location('edX', 'publishInfo', '2012_Fall', 'html', 'test_html') + 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) + 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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index 3790aea17b..a69d718404 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -19,7 +19,7 @@ class TestPublish(SplitWMongoCourseBoostrapper): # There are 12 created items and 7 parent updates # create course: finds: 1 to verify uniqueness, 1 to find parents # sends: 1 to create course, 1 to create overview - with check_mongo_calls(self.draft_mongo, 5, 2): + with check_mongo_calls(self.draft_mongo, 6, 2): super(TestPublish, self)._create_course(split=False) # 2 inserts (course and overview) # with bulk will delay all inheritance computations which won't be added into the mongo_calls diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py index d73b2d9b1c..e3a230e41d 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py @@ -30,8 +30,7 @@ class TestDraftVersioningModuleStore(unittest.TestCase): render_template=render_to_template_mock, xblock_mixins=(InheritanceMixin, XModuleMixin), ) - # NAATODO - uncomment once merged with drop_database PR - # self.addCleanup(module_store._drop_database) + self.addCleanup(self.module_store._drop_database) SplitModuleTest.bootstrapDB(self.module_store) 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 7ff8c5f492..06a72549a6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -444,7 +444,7 @@ class SplitModuleTest(unittest.TestCase): } }, } - + @staticmethod def bootstrapDB(split_store): ''' @@ -985,7 +985,7 @@ class TestItemCrud(SplitModuleTest): # add minimal one w/o a parent category = 'sequential' new_module = modulestore().create_item( - 'user123', parent_location=locator, category=category, + 'user123', locator, category, fields={'display_name': 'new sequential'} ) # check that course version changed and course's previous is the other one @@ -1025,8 +1025,8 @@ class TestItemCrud(SplitModuleTest): ) premod_course = modulestore().get_course(locator.course_key) category = 'chapter' - new_module = modulestore().create_item( - 'user123', parent_location=locator, category=category, + new_module = modulestore().create_child( + 'user123', locator, category, fields={'display_name': 'new chapter'}, definition_locator=original.definition_locator ) @@ -1054,13 +1054,13 @@ class TestItemCrud(SplitModuleTest): ) category = 'problem' new_payload = "empty" - new_module = modulestore().create_item( - 'anotheruser', parent_location=locator, category=category, + new_module = modulestore().create_child( + 'anotheruser', locator, category, fields={'display_name': 'problem 1', 'data': new_payload}, ) another_payload = "not empty" - another_module = modulestore().create_item( - 'anotheruser', parent_location=locator, category=category, + another_module = modulestore().create_child( + 'anotheruser', locator, category, fields={'display_name': 'problem 2', 'data': another_payload}, definition_locator=original.definition_locator, ) @@ -1086,8 +1086,8 @@ class TestItemCrud(SplitModuleTest): course_key = CourseLocator(org='guestx', course='contender', run="run", branch=BRANCH_NAME_DRAFT) parent_locator = BlockUsageLocator(course_key, 'course', block_id="head345679") chapter_locator = BlockUsageLocator(course_key, 'chapter', block_id="foo.bar_-~:0") - modulestore().create_item( - 'anotheruser', parent_location=parent_locator, category='chapter', + modulestore().create_child( + 'anotheruser', parent_locator, 'chapter', block_id=chapter_locator.block_id, fields={'display_name': 'chapter 99'}, ) @@ -1097,8 +1097,8 @@ class TestItemCrud(SplitModuleTest): # now try making that a parent of something new_payload = "empty" problem_locator = BlockUsageLocator(course_key, 'problem', block_id="prob.bar_-~:99a") - modulestore().create_item( - 'anotheruser', parent_location=chapter_locator, category='problem', + modulestore().create_child( + 'anotheruser', chapter_locator, 'problem', block_id=problem_locator.block_id, fields={'display_name': 'chapter 99', 'data': new_payload}, ) @@ -1123,8 +1123,8 @@ class TestItemCrud(SplitModuleTest): versionless_course_locator = new_course_locator.version_agnostic() # positive simple case: no force, add chapter - new_ele = modulestore().create_item( - user, parent_location=new_course.location, category='chapter', + new_ele = modulestore().create_child( + user, new_course.location, 'chapter', fields={'display_name': 'chapter 1'}, continue_version=True ) @@ -1141,40 +1141,40 @@ class TestItemCrud(SplitModuleTest): # try to create existing item with self.assertRaises(DuplicateItemError): - _fail = modulestore().create_item( - user, parent_location=new_course.location, category='chapter', + _fail = modulestore().create_child( + user, new_course.location, 'chapter', block_id=new_ele.location.block_id, fields={'display_name': 'chapter 2'}, continue_version=True ) # start a new transaction - new_ele = modulestore().create_item( - user, parent_location=new_course.location, category='chapter', + new_ele = modulestore().create_child( + user, new_course.location, 'chapter', fields={'display_name': 'chapter 2'}, continue_version=False ) transaction_guid = new_ele.location.version_guid # ensure force w/ continue gives exception with self.assertRaises(VersionConflictError): - _fail = modulestore().create_item( - user, parent_location=new_course.location, category='chapter', + _fail = modulestore().create_child( + user, new_course.location, 'chapter', fields={'display_name': 'chapter 2'}, force=True, continue_version=True ) # ensure trying to continue the old one gives exception with self.assertRaises(VersionConflictError): - _fail = modulestore().create_item( - user, parent_location=new_course.location, category='chapter', + _fail = modulestore().create_child( + user, new_course.location, 'chapter', fields={'display_name': 'chapter 3'}, continue_version=True ) # add new child to old parent in continued (leave off version_guid) course_module_locator = new_course.location.version_agnostic() - new_ele = modulestore().create_item( - user, parent_location=course_module_locator, category='chapter', + new_ele = modulestore().create_child( + user, course_module_locator, 'chapter', fields={'display_name': 'chapter 4'}, continue_version=True ) @@ -1283,13 +1283,13 @@ class TestItemCrud(SplitModuleTest): ) category = 'problem' new_payload = "empty" - modulestore().create_item( - 'test_update_manifold', parent_location=locator, category=category, + modulestore().create_child( + 'test_update_manifold', locator, category, fields={'display_name': 'problem 1', 'data': new_payload}, ) another_payload = "not empty" - modulestore().create_item( - 'test_update_manifold', parent_location=locator, category=category, + modulestore().create_child( + 'test_update_manifold', locator, category, fields={'display_name': 'problem 2', 'data': another_payload}, definition_locator=original.definition_locator, ) @@ -1368,8 +1368,8 @@ class TestItemCrud(SplitModuleTest): """ if not category_queue: return - node = modulestore().create_item( - 'deleting_user', parent_location=parent.version_agnostic(), category=category_queue[0] + node = modulestore().create_child( + 'deleting_user', parent.version_agnostic(), category_queue[0] ) node_loc = node.location.map_into_course(parent.course_key) for _ in range(4): @@ -1433,8 +1433,8 @@ class TestCourseCreation(SplitModuleTest): # changing this course will not change the original course # using new_draft.location will insert the chapter under the course root - new_item = modulestore().create_item( - 'leech_master', parent_location=new_draft.location, category='chapter', + new_item = modulestore().create_child( + 'leech_master', new_draft.location, 'chapter', fields={'display_name': 'new chapter'} ) new_draft_locator = new_draft_locator.course_key.version_agnostic() @@ -1595,8 +1595,8 @@ class TestPublish(SplitModuleTest): source_course, dest_course, expected, [chapter2.block_id, chapter3.block_id, "problem1", "problem3_2"] ) # add a child under chapter1 - new_module = modulestore().create_item( - self.user_id, parent_location=chapter1, category="sequential", + new_module = modulestore().create_child( + self.user_id, chapter1, "sequential", fields={'display_name': 'new sequential'}, ) # remove chapter1 from expected b/c its pub'd version != the source anymore since source changed @@ -1614,7 +1614,7 @@ class TestPublish(SplitModuleTest): ) # ensure intentionally orphaned blocks work (e.g., course_info) new_module = modulestore().create_item( - self.user_id, parent_location=source_course, category="course_info", block_id="handouts" + self.user_id, source_course, "course_info", block_id="handouts" ) # publish it modulestore().copy(self.user_id, source_course, dest_course, [new_module.location], None) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index 5e12bac3d8..a2e2c2dcb7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py @@ -87,7 +87,13 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): """ location = self.old_course_key.make_usage_key(category, name) self.draft_mongo.create_item( - self.user_id, location, definition_data=data, metadata=metadata, runtime=self.runtime + self.user_id, + location.course_key, + location.block_type, + block_id=location.block_id, + definition_data=data, + metadata=metadata, + runtime=self.runtime ) if not draft: self.draft_mongo.publish(location, self.user_id) @@ -104,16 +110,28 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): self.draft_mongo.update_item(parent, self.user_id) if not draft: self.draft_mongo.publish(parent_location, self.user_id) - # create pointer for split - course_or_parent_locator = BlockUsageLocator( - course_key=self.split_course_key, - block_type=parent_category, - block_id=parent_name - ) + # create child for split + if split: + self.split_mongo.create_child( + self.user_id, + BlockUsageLocator( + course_key=self.split_course_key, + block_type=parent_category, + block_id=parent_name + ), + category, + block_id=name, + fields=fields + ) else: - course_or_parent_locator = self.split_course_key - if split: - self.split_mongo.create_item(course_or_parent_locator, category, self.user_id, block_id=name, fields=fields) + if split: + self.split_mongo.create_item( + self.user_id, + self.split_course_key, + category, + block_id=name, + fields=fields + ) def _create_course(self, split=True): """ diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index de08124e80..f8de2bc990 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -578,7 +578,9 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes metadata = {'display_name': group.name} modulestore.create_item( user_id, - dest_usage_key, + self.location.course_key, + dest_usage_key.block_type, + block_id=dest_usage_key.block_id, definition_data=None, metadata=metadata, runtime=self.system,