diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 2f6fabfce0..0f9e0a4d8f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -34,7 +34,6 @@ from xblock.fields import Scope, ScopeIds from xmodule.modulestore import ModuleStoreWriteBase, Location, MONGO_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore -import re log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index a6349e6113..ac515b1013 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1,3 +1,50 @@ +""" +Provides full versioning CRUD and representation for collections of xblocks (e.g., courses, modules, etc). + +Representation: +* course_index: a dictionary: + ** '_id': course_id (e.g., myu.mydept.mycourse.myrun), + ** 'org': the org's id. Only used for searching not identity, + ** 'prettyid': a vague to-be-determined field probably more useful to storing searchable tags, + ** 'edited_by': user_id of user who created the original entry, + ** 'edited_on': the datetime of the original creation, + ** 'versions': versions_dict: {branch_id: structure_id, ...} +* structure: + ** '_id': an ObjectId (guid), + ** 'root': root_usage_id (string of key in 'blocks' for the root of this structure, + ** 'previous_version': the structure from which this one was derived. For published courses, this + points to the previously published version of the structure not the draft published to this. + ** 'original_version': the original structure id in the previous_version relation. Is a pseudo object + identifier enabling quick determination if 2 structures have any shared history, + ** 'edited_by': user_id of the user whose change caused the creation of this structure version, + ** 'edited_on': the datetime for the change causing this creation of this structure version, + ** 'blocks': dictionary of xblocks in this structure: + *** block_id: dictionary of block settings and children: + **** 'category': the xblock type id + **** 'definition': the db id of the record containing the content payload for this xblock + **** 'fields': the Scope.settings and children field values + **** 'edit_info': dictionary: + ***** 'edited_on': when was this xblock's fields last changed (will be edited_on value of + update_version structure) + ***** 'edited_by': user_id for who changed this xblock last (will be edited_by value of + update_version structure) + ***** 'update_version': the guid for the structure where this xblock got its current field + values. This may point to a structure not in this structure's history (e.g., to a draft + branch from which this version was published.) + ***** 'previous_version': the guid for the structure which previously changed this xblock + (will be the previous value of update_version; so, may point to a structure not in this + structure's history.) +* definition: shared content with revision history for xblock content fields + ** '_id': definition_id (guid), + ** 'category': xblock type id + ** 'fields': scope.content (and possibly other) field values. + ** 'edit_info': dictionary: + *** 'edited_by': user_id whose edit caused this version of the definition, + *** 'edited_on': datetime of the change causing this version + *** 'previous_version': the definition_id of the previous version of this definition + *** 'original_version': definition_id of the root of the previous version relation on this + definition. Acts as a pseudo-object identifier. +""" import threading import datetime import logging @@ -382,12 +429,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param usage_id: ignored. Only included for API compatibility. Specify the usage_id within the locator. ''' course = self._lookup_course(locator) - items = [] - for parent_id, value in course['structure']['blocks'].iteritems(): - for child_id in value['fields'].get('children', []): - if locator.usage_id == child_id: - items.append(BlockUsageLocator(url=locator.as_course_locator(), usage_id=parent_id)) - return items + items = self._get_parents_from_structure(locator.usage_id, course['structure']) + return [BlockUsageLocator(url=locator.as_course_locator(), usage_id=parent_id) + for parent_id in items] def get_orphans(self, course_id, detached_categories, branch): """ @@ -557,13 +601,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): "category" : category, "fields": new_def_data, "edit_info": { - "_id": new_id, "edited_by": user_id, "edited_on": datetime.datetime.now(UTC), "previous_version": None, "original_version": new_id, - } } + } self.db_connection.insert_definition(document) definition_locator = DefinitionLocator(new_id) return definition_locator @@ -826,28 +869,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } self.db_connection.insert_definition(definition_entry) - new_id = ObjectId() - draft_structure = { - '_id': new_id, - 'root': root_usage_id, - 'previous_version': None, - 'original_version': new_id, - 'edited_by': user_id, - 'edited_on': datetime.datetime.now(UTC), - 'blocks': { - root_usage_id: { - 'category': root_category, - 'definition': definition_id, - 'fields': block_fields, - 'edit_info': { - 'edited_on': datetime.datetime.now(UTC), - 'edited_by': user_id, - 'previous_version': None, - 'update_version': new_id, - } - } - } - } + draft_structure = self._new_structure( + user_id, root_usage_id, root_category, block_fields, definition_id + ) + new_id = draft_structure['_id'] + self.db_connection.insert_structure(draft_structure) if versions_dict is None: @@ -1083,6 +1109,84 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): '''Deprecated, use update_item.''' raise NotImplementedError('use update_item') + def xblock_publish(self, user_id, source_course, destination_course, subtree_list, blacklist): + """ + Publishes each xblock in subtree_list and those blocks descendants excluding blacklist + from source_course to destination_course. + + To delete a block, publish its parent. You can blacklist the other sibs to keep them from + being refreshed. You can also just call delete_item on the destination. + + To unpublish a block, call delete_item on the destination. + + Ensures that each subtree occurs in the same place in destination as it does in source. If any + of the source's subtree parents are missing from destination, it raises ItemNotFound([parent_ids]). + To determine the same relative order vis-a-vis published siblings, + publishing may involve changing the order of previously published siblings. For example, + if publishing `[c, d]` and source parent has children `[a, b, c, d, e]` and destination parent + currently has children `[e, b]`, there's no obviously correct resulting order; thus, publish will + reorder destination to `[b, c, d, e]` to make it conform with the source. + + :param source_course: a CourseLocator (can be a version or course w/ branch) + + :param destination_course: a CourseLocator which must be an existing course but branch doesn't have + to exist yet. (The course must exist b/c Locator doesn't have everything necessary to create it). + Note, if the branch doesn't exist, then the source_course structure's root must be in subtree_list; + otherwise, the publish will violate the parents must exist rule. + + :param subtree_list: a list of xblock ids whose subtrees to publish. The ids can be the block + id or BlockUsageLocator. If a Locator and if the Locator's course id != source_course, this will + raise InvalidLocationError(locator) + + :param blacklist: a list of xblock ids to not change in the destination: i.e., don't add + if not there, don't update if there. + """ + # get the destination's index, and source and destination structures. + source_structure = self._lookup_course(source_course)['structure'] + index_entry = self.db_connection.get_course_index(destination_course.course_id) + if index_entry is None: + # brand new course + raise ItemNotFoundError(destination_course) + if destination_course.branch not in index_entry['versions']: + # must be publishing the dag root if there's no current dag + if source_structure['root'] not in subtree_list: + raise ItemNotFoundError(source_structure['root']) + # create branch + destination_structure = self._new_structure(user_id, source_structure['root']) + else: + destination_structure = self._lookup_course(destination_course)['structure'] + destination_structure = self._version_structure(destination_structure, user_id) + + # iterate over subtree list filtering out blacklist. + orphans = set() + destination_blocks = destination_structure['blocks'] + for subtree_root in subtree_list: + # find the parents and put root in the right sequence + parents = self._get_parents_from_structure(subtree_root, source_structure) + if not all(parent in destination_blocks for parent in parents): + raise ItemNotFoundError(parents) + for parent_loc in parents: + orphans.update( + self._sync_children( + source_structure['blocks'][parent_loc], + destination_blocks[parent_loc], + subtree_root + )) + # update/create the subtree and its children in destination (skipping blacklist) + orphans.update( + self._publish_subdag( + user_id, subtree_root, source_structure['blocks'], destination_blocks, blacklist or [] + ) + ) + # remove any remaining orphans + for orphan in orphans: + # orphans will include moved as well as deleted xblocks. Only delete the deleted ones. + self._delete_if_true_orphan(orphan, destination_structure) + + # update the db + self.db_connection.insert_structure(destination_structure) + self._update_head(index_entry, destination_course.branch, destination_structure['_id']) + def update_course_index(self, updated_index_entry): """ Change the given course's index entry. @@ -1091,9 +1195,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Does not return anything useful. """ - # TODO how should this log the change? edited_on and edited_by for this entry - # has the semantic of who created the course and when; so, changing those will lose - # that information. self.db_connection.update_course_index(updated_index_entry) def delete_item(self, usage_locator, user_id, delete_children=False, force=False): @@ -1135,6 +1236,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): del new_blocks[usage_id] if delete_children: remove_subtree(usage_locator.usage_id) + else: + del new_blocks[usage_locator.usage_id] # update index if appropriate and structures self.db_connection.insert_structure(new_structure) @@ -1419,3 +1522,125 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): "split" for new-style split MongoDB backed courses. """ return SPLIT_MONGO_MODULESTORE_TYPE + + def _new_structure(self, user_id, root_usage_id, + root_category=None, block_fields=None, definition_id=None): + """ + Internal function: create a structure element with no previous version. Must provide the root id + but not necessarily the info needed to create it (for the use case of publishing). If providing + root_category, must also provide block_fields and definition_id + """ + new_id = ObjectId() + if root_category is not None: + blocks = { + root_usage_id: self._new_block(user_id, root_category, block_fields, definition_id, new_id) + } + else: + blocks = {} + return { + '_id': new_id, + 'root': root_usage_id, + 'previous_version': None, + 'original_version': new_id, + 'edited_by': user_id, + 'edited_on': datetime.datetime.now(UTC), + 'blocks': blocks + } + + def _get_parents_from_structure(self, usage_id, structure): + """ + Given a structure, find all of usage_id's parents in that structure + """ + items = [] + for parent_id, value in structure['blocks'].iteritems(): + for child_id in value['fields'].get('children', []): + if usage_id == child_id: + items.append(parent_id) + + return items + + def _sync_children(self, source_parent, destination_parent, new_child): + """ + Reorder destination's children to the same as source's and remove any no longer in source. + Return the removed ones as orphans (a set). + """ + destination_reordered = [] + destination_children = destination_parent['fields']['children'] + source_children = source_parent['fields']['children'] + orphans = set() + for child in destination_children: + try: + source_children.index(child) + except ValueError: + orphans.add(child) + for child in source_children: + if child == new_child or child in destination_children: + destination_reordered.append(child) + destination_parent['fields']['children'] = destination_reordered + return orphans + + def _publish_subdag(self, user_id, block_id, source_blocks, destination_blocks, blacklist): + """ + Update destination_blocks for the sub-dag rooted at block_id to be like the one in + source_blocks excluding blacklist. + + Return any newly discovered orphans (as a set) + """ + orphans = set() + destination_block = destination_blocks.get(block_id) + new_block = source_blocks[block_id] + if destination_block: + if destination_block['edit_info']['update_version'] != new_block['edit_info']['update_version']: + source_children = new_block['fields']['children'] + for child in destination_block['fields']['children']: + try: + source_children.index(child) + except ValueError: + orphans.add(child) + previous_version = new_block['edit_info']['update_version'] + destination_block = copy.deepcopy(new_block) + destination_block['fields'] = self._filter_blacklist(destination_block['fields'], blacklist) + destination_block['edit_info']['previous_version'] = previous_version + destination_block['edit_info']['edited_by'] = user_id + else: + destination_block = self._new_block( + user_id, new_block['category'], + self._filter_blacklist(copy.copy(new_block['fields']), blacklist), + new_block['definition'], + new_block['edit_info']['update_version'] + ) + for child in destination_block['fields'].get('children', []): + if child not in blacklist: + orphans.update(self._publish_subdag(user_id, child, source_blocks, destination_blocks, blacklist)) + destination_blocks[block_id] = destination_block + return orphans + + def _filter_blacklist(self, fields, blacklist): + """ + Filter out blacklist from the children field in fields. Will construct a new list for children; + so, no need to worry about copying the children field, but it will modify fiels. + """ + fields['children'] = [child for child in fields.get('children', []) if child not in blacklist] + return fields + + def _delete_if_true_orphan(self, orphan, structure): + """ + Delete the orphan and any of its descendants which no longer have parents. + """ + if not self._get_parents_from_structure(orphan, structure): + for child in structure['blocks'][orphan]['fields'].get('children', []): + self._delete_if_true_orphan(child, structure) + del structure['blocks'][orphan] + + def _new_block(self, user_id, category, block_fields, definition_id, new_id): + return { + 'category': category, + 'definition': definition_id, + 'fields': block_fields, + 'edit_info': { + 'edited_on': datetime.datetime.now(UTC), + 'edited_by': user_id, + 'previous_version': None, + 'update_version': new_id + } + } 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 92de35e39f..2e89b761d2 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -829,7 +829,7 @@ class TestItemCrud(SplitModuleTest): # delete a leaf problems = modulestore().get_items(reusable_location, {'category': 'problem'}) locn_to_del = problems[0].location - new_course_loc = modulestore().delete_item(locn_to_del, 'deleting_user', delete_children=True) + new_course_loc = modulestore().delete_item(locn_to_del, 'deleting_user', delete_children=False) deleted = BlockUsageLocator(course_id=reusable_location.course_id, branch=reusable_location.branch, usage_id=locn_to_del.usage_id) @@ -1085,7 +1085,135 @@ class TestInheritance(SplitModuleTest): # overridden self.assertEqual(node.graceperiod, datetime.timedelta(hours=4)) - # TODO test inheritance after set and delete of attrs + +class TestPublish(SplitModuleTest): + """ + Test the publishing api + """ + def setUp(self): + SplitModuleTest.setUp(self) + self.user = random.getrandbits(32) + + def tearDown(self): + SplitModuleTest.tearDownClass() + + def test_publish_safe(self): + """ + Test the standard patterns: publish to new branch, revise and publish + """ + source_course = CourseLocator(course_id="GreekHero", branch='draft') + dest_course = CourseLocator(course_id="GreekHero", branch="published") + modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2", "chapter3"]) + expected = ["head12345", "chapter1"] + self._check_course( + source_course, dest_course, expected, ["chapter2", "chapter3", "problem1", "problem3_2"] + ) + # add a child under chapter1 + new_module = modulestore().create_item( + self._usage(source_course, "chapter1"), "sequential", self.user, + fields={'display_name': 'new sequential'}, + ) + # remove chapter1 from expected b/c its pub'd version != the source anymore since source changed + expected.remove("chapter1") + # check that it's not in published course + with self.assertRaises(ItemNotFoundError): + modulestore().get_item(self._usage(dest_course, new_module.location.usage_id)) + # publish it + modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.usage_id], None) + expected.append(new_module.location.usage_id) + # check that it is in the published course and that its parent is the chapter + pub_module = modulestore().get_item(self._usage(dest_course, new_module.location.usage_id)) + self.assertEqual( + modulestore().get_parent_locations(pub_module.location)[0].usage_id, "chapter1" + ) + # ensure intentionally orphaned blocks work (e.g., course_info) + new_module = modulestore().create_item( + source_course, "course_info", self.user, usage_id="handouts" + ) + # publish it + modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.usage_id], None) + expected.append(new_module.location.usage_id) + # check that it is in the published course (no error means it worked) + pub_module = modulestore().get_item(self._usage(dest_course, new_module.location.usage_id)) + self._check_course( + source_course, dest_course, expected, ["chapter2", "chapter3", "problem1", "problem3_2"] + ) + + def test_exceptions(self): + """ + Test the exceptions which preclude successful publication + """ + source_course = CourseLocator(course_id="GreekHero", branch='draft') + # destination does not exist + destination_course = CourseLocator(course_id="Unknown", branch="published") + with self.assertRaises(ItemNotFoundError): + modulestore().xblock_publish(self.user, source_course, destination_course, ["chapter3"], None) + # publishing into a new branch w/o publishing the root + destination_course = CourseLocator(course_id="GreekHero", branch="published") + with self.assertRaises(ItemNotFoundError): + modulestore().xblock_publish(self.user, source_course, destination_course, ["chapter3"], None) + # publishing a subdag w/o the parent already in course + modulestore().xblock_publish(self.user, source_course, destination_course, ["head12345"], ["chapter3"]) + with self.assertRaises(ItemNotFoundError): + modulestore().xblock_publish(self.user, source_course, destination_course, ["problem1"], []) + + def test_move_delete(self): + """ + Test publishing moves and deletes. + """ + source_course = CourseLocator(course_id="GreekHero", branch='draft') + dest_course = CourseLocator(course_id="GreekHero", branch="published") + modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2"]) + expected = ["head12345", "chapter1", "chapter3", "problem1", "problem3_2"] + self._check_course(source_course, dest_course, expected, ["chapter2"]) + # now move problem1 and delete problem3_2 + chapter1 = modulestore().get_item(self._usage(source_course, "chapter1")) + chapter3 = modulestore().get_item(self._usage(source_course, "chapter3")) + chapter1.children.append("problem1") + chapter3.children.remove("problem1") + modulestore().delete_item(self._usage(source_course, "problem3_2"), self.user) + modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2"]) + expected = ["head12345", "chapter1", "chapter3", "problem1"] + self._check_course(source_course, dest_course, expected, ["chapter2", "problem3_2"]) + + def _check_course(self, source_course_loc, dest_course_loc, expected_blocks, unexpected_blocks): + """ + Check that the course has the expected blocks and does not have the unexpected blocks + """ + for expected in expected_blocks: + source = modulestore().get_item(self._usage(source_course_loc, expected)) + pub_copy = modulestore().get_item(self._usage(dest_course_loc, expected)) + # everything except previous_version & children should be the same + self.assertEqual(source.category, pub_copy.category) + self.assertEqual(source.update_version, pub_copy.update_version) + self.assertEqual(self.user, pub_copy.edited_by) + for field in source.fields.values(): + if field.name == 'children': + self._compare_children(field.read_from(source), field.read_from(pub_copy), unexpected_blocks) + else: + self.assertEqual(field.read_from(source), field.read_from(pub_copy)) + for unexp in unexpected_blocks: + with self.assertRaises(ItemNotFoundError): + modulestore().get_item(self._usage(dest_course_loc, unexp)) + + def _usage(self, course_loc, block_id): + """ + Generate a BlockUsageLocator for the combo of the course location and block id + """ + return BlockUsageLocator(course_id=course_loc.course_id, branch=course_loc.branch, usage_id=block_id) + + def _compare_children(self, source_children, dest_children, unexpected): + """ + Ensure dest_children == source_children minus unexpected + """ + dest_cursor = 0 + for child in source_children: + if child in unexpected: + self.assertNotIn(child, dest_children) + else: + self.assertEqual(child, dest_children[dest_cursor]) + dest_cursor += 1 + self.assertEqual(dest_cursor, len(dest_children)) #===========================================