From 9a039e93ecb04d50a27a3fe6ca1d6f2f40721ff1 Mon Sep 17 00:00:00 2001 From: Mathew Peterson Date: Mon, 21 Jul 2014 17:29:43 +0000 Subject: [PATCH] Adding auto_publishing to split_draft Added update_item, create_item, create_child to split_draft.py Cleaned up TODOs in test_mixed_modulestore.py Unit test for auto-publish LMS-11017 Added tests to test_mixed_modulestore.py for has_changes and added black_list to _auto_publish Moved DIRECT_ONLY_CATEGORIES to draft_and_publish.py Deleted test_split_draft_modulestore.py Added _auto_publish to create course Publish cleanups for update_version, source_version Changed has_changes to use source_version Removing mixed test that no longer makes sense in auto_publish world Addressed TODOs in test_branch_setting --- cms/djangoapps/contentstore/tests/utils.py | 29 ++-- cms/djangoapps/contentstore/views/item.py | 2 +- .../lib/xmodule/xmodule/modulestore/draft.py | 2 +- .../modulestore/draft_and_published.py | 2 + .../xmodule/xmodule/modulestore/mongo/base.py | 6 +- .../xmodule/modulestore/mongo/draft.py | 4 +- .../split_mongo/caching_descriptor_system.py | 1 + .../xmodule/modulestore/split_mongo/split.py | 140 +++++++++++------ .../modulestore/split_mongo/split_draft.py | 73 +++++++-- .../tests/test_mixed_modulestore.py | 146 ++++++++++++++---- .../xmodule/modulestore/tests/test_mongo.py | 50 ------ .../tests/test_split_draft_modulestore.py | 62 -------- .../tests/test_split_modulestore.py | 7 +- .../xmodule/modulestore/xml_exporter.py | 2 +- 14 files changed, 302 insertions(+), 224 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 1707f92823..ec425f0e44 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -9,10 +9,11 @@ from django.test.client import Client from django.contrib.auth.models import User from xmodule.contentstore.django import contentstore -from xmodule.modulestore import PublishState, ModuleStoreEnum, mongo +from xmodule.modulestore import PublishState, ModuleStoreEnum from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.xml_importer import import_from_xml from student.models import Registration from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation @@ -262,23 +263,15 @@ class CourseTestCase(ModuleStoreTestCase): self.store.compute_publish_state(course2_item) ) except AssertionError: - # TODO LMS-11017 "Studio auto-publish course-wide features and settings" - # Temporary hack until autopublish implemented - right now, because we call - # update_item within create_course to set the wiki & other course-wide settings, - # the publish version does not necessarily equal the draft version in split. - # So if either item is in Split, just continue on - if not isinstance(course1_item.runtime.modulestore, SplitMongoModuleStore) and \ - not isinstance(course2_item.runtime.modulestore, SplitMongoModuleStore): - # old mongo calls things draft if draft exists even if it's != published; so, do more work - c1_state = self.compute_real_state(course1_item) - c2_state = self.compute_real_state(course2_item) - self.assertEqual( - c1_state, - c2_state, - "Course item {} in state {} != course item {} in state {}".format( - course1_item, c1_state, course2_item, c2_state - ) + c1_state = self.compute_real_state(course1_item) + c2_state = self.compute_real_state(course2_item) + self.assertEqual( + c1_state, + c2_state, + "Publish states not equal: course item {} in state {} != course item {} in state {}".format( + course1_item.location, c1_state, course2_item.location, c2_state ) + ) # compare data self.assertEqual(hasattr(course1_item, 'data'), hasattr(course2_item, 'data')) @@ -351,7 +344,7 @@ class CourseTestCase(ModuleStoreTestCase): return supposed_state # published == item in all respects, so return public return PublishState.public - elif supposed_state == PublishState.public and item.location.category in mongo.base.DIRECT_ONLY_CATEGORIES: + elif supposed_state == PublishState.public and item.location.category in DIRECT_ONLY_CATEGORIES: if not all([ self.store.has_item(child_loc, revision=ModuleStoreEnum.RevisionOption.draft_only) for child_loc in item.children diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 5db55ae8f3..5373915c5e 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -23,7 +23,7 @@ import xmodule from xmodule.tabs import StaticTab, CourseTabList from xmodule.modulestore import PublishState, ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES +from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, DuplicateItemError from xmodule.modulestore.inheritance import own_metadata from xmodule.x_module import PREVIEW_VIEWS, STUDIO_VIEW diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index d9ab3878a5..5fe4a3c90a 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -4,4 +4,4 @@ Backwards compatibility for old pointers to draft module store This modulestore has been moved to xmodule.modulestore.mongo.draft """ -from xmodule.modulestore.mongo.draft import DIRECT_ONLY_CATEGORIES, DraftModuleStore +from xmodule.modulestore.mongo.draft import DraftModuleStore diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index fea45d7d7f..27dbaf2e7b 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -7,6 +7,8 @@ from abc import ABCMeta, abstractmethod from contextlib import contextmanager from . import ModuleStoreEnum +# Things w/ these categories should never be marked as version=DRAFT +DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info'] class BranchSettingMixin(object): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 617b7b1aff..6ca71dcd31 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -35,7 +35,7 @@ from xblock.exceptions import InvalidScopeError from xblock.fields import Scope, ScopeIds, Reference, ReferenceList, ReferenceValueDict from xmodule.modulestore import ModuleStoreWriteBase, ModuleStoreEnum -from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished +from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES from opaque_keys.edx.locations import Location from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, ReferentialIntegrityError from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore @@ -46,10 +46,6 @@ from xmodule.exceptions import HeartbeatFailure log = logging.getLogger(__name__) - -# Things w/ these categories should never be marked as version=DRAFT -DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info'] - # sort order that returns DRAFT items first SORT_REVISION_FAVOR_DRAFT = ('_id.revision', pymongo.DESCENDING) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 7b955de8c3..66c2de808a 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -17,10 +17,10 @@ from xmodule.modulestore.exceptions import ( ) from xmodule.modulestore.mongo.base import ( MongoModuleStore, MongoRevisionKey, as_draft, as_published, - DIRECT_ONLY_CATEGORIES, SORT_REVISION_FAVOR_DRAFT + SORT_REVISION_FAVOR_DRAFT ) from xmodule.modulestore.store_utilities import rewrite_nonportable_content_links -from xmodule.modulestore.draft_and_published import UnsupportedRevisionError +from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, DIRECT_ONLY_CATEGORIES log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 759fa0264e..a2733d35b0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -153,6 +153,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): module.edited_on = edit_info.get('edited_on') module.previous_version = edit_info.get('previous_version') module.update_version = edit_info.get('update_version') + module.source_version = edit_info.get('source_version', None) module.definition_locator = definition_id # decache any pending field settings module.save() diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 70926631a3..6fb0da75db 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -37,6 +37,7 @@ Representation: ***** '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.) + ***** 'source_version': the guid for the structure was copied/published into this block * definition: shared content with revision history for xblock content fields ** '_id': definition_id (guid), ** 'category': xblock type id @@ -80,8 +81,6 @@ from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_fro log = logging.getLogger(__name__) #============================================================================== -# Documentation is at -# https://edx-wiki.atlassian.net/wiki/display/ENG/Mongostore+Data+Structure # # Known issue: # Inheritance for cached kvs doesn't work on edits. Use case. @@ -99,6 +98,9 @@ log = logging.getLogger(__name__) # #============================================================================== +# When blacklists are this, all children should be excluded +EXCLUDE_ALL = '*' + class SplitMongoModuleStore(ModuleStoreWriteBase): """ @@ -1040,34 +1042,33 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): else: versions_dict[master_branch] = new_id - else: + elif definition_fields or block_fields: # pointing to existing course w/ some overrides # just get the draft_version structure draft_version = CourseLocator(version_guid=versions_dict[master_branch]) draft_structure = self._lookup_course(draft_version)['structure'] - if definition_fields or block_fields: - draft_structure = self._version_structure(draft_structure, user_id) - new_id = draft_structure['_id'] - encoded_block_id = encode_key_for_mongo(draft_structure['root']) - root_block = draft_structure['blocks'][encoded_block_id] - if block_fields is not None: - root_block['fields'].update(self._serialize_fields(root_category, block_fields)) - if definition_fields is not None: - definition = self.db_connection.get_definition(root_block['definition']) - definition['fields'].update(definition_fields) - definition['edit_info']['previous_version'] = definition['_id'] - definition['edit_info']['edited_by'] = user_id - definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) - definition['_id'] = ObjectId() - definition['schema_version'] = self.SCHEMA_VERSION - self.db_connection.insert_definition(definition) - root_block['definition'] = definition['_id'] - root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) - root_block['edit_info']['edited_by'] = user_id - root_block['edit_info']['previous_version'] = root_block['edit_info'].get('update_version') - root_block['edit_info']['update_version'] = new_id + draft_structure = self._version_structure(draft_structure, user_id) + new_id = draft_structure['_id'] + encoded_block_id = encode_key_for_mongo(draft_structure['root']) + root_block = draft_structure['blocks'][encoded_block_id] + if block_fields is not None: + root_block['fields'].update(self._serialize_fields(root_category, block_fields)) + if definition_fields is not None: + definition = self.db_connection.get_definition(root_block['definition']) + definition['fields'].update(definition_fields) + definition['edit_info']['previous_version'] = definition['_id'] + definition['edit_info']['edited_by'] = user_id + definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) + definition['_id'] = ObjectId() + definition['schema_version'] = self.SCHEMA_VERSION + self.db_connection.insert_definition(definition) + root_block['definition'] = definition['_id'] + root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) + root_block['edit_info']['edited_by'] = user_id + root_block['edit_info']['previous_version'] = root_block['edit_info'].get('update_version') + root_block['edit_info']['update_version'] = new_id - self.db_connection.insert_structure(draft_structure) - versions_dict[master_branch] = new_id + self.db_connection.insert_structure(draft_structure) + versions_dict[master_branch] = new_id index_entry = { '_id': ObjectId(), @@ -1358,7 +1359,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): destination_structure = self._lookup_course(destination_course)['structure'] destination_structure = self._version_structure(destination_structure, user_id) - blacklist = [shunned.block_id for shunned in blacklist or []] + if blacklist != EXCLUDE_ALL: + blacklist = [shunned.block_id for shunned in blacklist or []] # iterate over subtree list filtering out blacklist. orphans = set() destination_blocks = destination_structure['blocks'] @@ -1379,7 +1381,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # update/create the subtree and its children in destination (skipping blacklist) orphans.update( self._publish_subdag( - user_id, subtree_root.block_id, source_structure['blocks'], destination_blocks, blacklist + user_id, destination_structure['_id'], + subtree_root.block_id, source_structure['blocks'], destination_blocks, blacklist ) ) # remove any remaining orphans @@ -1785,7 +1788,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): destination_parent['fields']['children'] = destination_reordered return orphans - def _publish_subdag(self, user_id, block_id, source_blocks, destination_blocks, blacklist): + def _publish_subdag(self, user_id, destination_version, 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. @@ -1797,29 +1800,50 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): destination_block = destination_blocks.get(encoded_block_id) new_block = source_blocks[encoded_block_id] if destination_block: - if destination_block['edit_info']['update_version'] != new_block['edit_info']['update_version']: - source_children = new_block['fields'].get('children', []) - for child in destination_block['fields'].get('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 + # reorder children to correspond to whatever order holds for source. + # remove any which source no longer claims (put into orphans) + # add any which are being published + source_children = new_block['fields'].get('children', []) + existing_children = destination_block['fields'].get('children', []) + destination_reordered = SparseList() + for child in existing_children: + try: + index = source_children.index(child) + destination_reordered[index] = child + except ValueError: + orphans.add(child) + if blacklist != EXCLUDE_ALL: + for index, child in enumerate(source_children): + if child not in blacklist: + destination_reordered[index] = child + # the history of the published leaps between publications and only points to + # previously published versions. + previous_version = destination_block['edit_info']['update_version'] + destination_block = copy.deepcopy(new_block) + destination_block['fields']['children'] = destination_reordered.compact_list() + destination_block['edit_info']['previous_version'] = previous_version + destination_block['edit_info']['update_version'] = destination_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'], + destination_version, raw=True ) - 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)) + + # introduce new edit info field for tracing where copied/published blocks came + destination_block['edit_info']['source_version'] = new_block['edit_info']['update_version'] + + if blacklist != EXCLUDE_ALL: + for child in destination_block['fields'].get('children', []): + if child not in blacklist: + orphans.update( + self._publish_subdag( + user_id, destination_version, child, source_blocks, destination_blocks, blacklist + ) + ) destination_blocks[encoded_block_id] = destination_block return orphans @@ -1828,7 +1852,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 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] + if blacklist == EXCLUDE_ALL: + fields['children'] = [] + else: + fields['children'] = [child for child in fields.get('children', []) if child not in blacklist] return fields def _delete_if_true_orphan(self, orphan, structure): @@ -1905,3 +1932,24 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Check that the db is reachable. """ return {ModuleStoreEnum.Type.split: self.db_connection.heartbeat()} + + +class SparseList(list): + """ + Enable inserting items into a list in arbitrary order and then retrieving them. + """ + # taken from http://stackoverflow.com/questions/1857780/sparse-assignment-list-in-python + def __setitem__(self, index, value): + """ + Add value to the list ensuring the list is long enough to accommodate it at the given index + """ + missing = index - len(self) + 1 + if missing > 0: + self.extend([None] * missing) + list.__setitem__(self, index, value) + + def compact_list(self): + """ + Return as a regular lists w/ all Nones removed + """ + return [ele for ele in self if ele is not None] 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 2e8bd77bd0..c41644ee14 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -3,11 +3,10 @@ Module for the dual-branch fall-back Draft->Published Versioning ModuleStore """ from ..exceptions import ItemNotFoundError -from split import SplitMongoModuleStore +from split import SplitMongoModuleStore, EXCLUDE_ALL from xmodule.modulestore import ModuleStoreEnum, PublishState -from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, UnsupportedRevisionError -from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import InsufficientSpecificationError +from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES, UnsupportedRevisionError class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore): @@ -46,9 +45,11 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS Returns: a CourseDescriptor """ master_branch = kwargs.pop('master_branch', ModuleStoreEnum.BranchName.draft) - return super(DraftVersioningModuleStore, self).create_course( + item = super(DraftVersioningModuleStore, self).create_course( org, course, run, user_id, master_branch=master_branch, **kwargs ) + self._auto_publish_no_children(item.location, item.location.category, user_id) + return item def get_courses(self): """ @@ -56,6 +57,51 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft) + def _auto_publish_no_children(self, location, category, user_id): + """ + Publishes item if the category is DIRECT_ONLY. This assumes another method has checked that + location points to the head of the branch and ignores the version. If you call this in any + other context, you may blow away another user's changes. + NOTE: only publishes the item at location: no children get published. + """ + if location.branch == ModuleStoreEnum.BranchName.draft and category in DIRECT_ONLY_CATEGORIES: + # version_agnostic b/c of above assumption in docstring + self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL) + + def update_item(self, descriptor, user_id, allow_not_found=False, force=False): + item = super(DraftVersioningModuleStore, self).update_item( + descriptor, + user_id, + allow_not_found=allow_not_found, + force=force + ) + self._auto_publish_no_children(item.location, item.location.category, user_id) + return item + + def create_item( + self, user_id, course_key, block_type, block_id=None, + definition_locator=None, fields=None, + force=False, continue_version=False, **kwargs + ): + item = super(DraftVersioningModuleStore, self).create_item( + user_id, course_key, block_type, block_id=block_id, + definition_locator=definition_locator, fields=fields, + force=force, continue_version=continue_version, **kwargs + ) + self._auto_publish_no_children(item.location, item.location.category, user_id) + return item + + def create_child( + self, user_id, parent_usage_key, block_type, block_id=None, + fields=None, continue_version=False, **kwargs + ): + item = super(DraftVersioningModuleStore, self).create_child( + user_id, parent_usage_key, block_type, block_id=block_id, + fields=fields, continue_version=continue_version, **kwargs + ) + self._auto_publish_no_children(parent_usage_key, item.location.category, user_id) + return item + def delete_item(self, location, user_id, revision=None, **kwargs): """ Delete the given item from persistence. kwargs allow modulestore specific parameters. @@ -86,7 +132,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS ) for branch in branches_to_delete: - SplitMongoModuleStore.delete_item(self, location.for_branch(branch), user_id, **kwargs) + branched_location = location.for_branch(branch) + parent_loc = self.get_parent_location(branched_location) + SplitMongoModuleStore.delete_item(self, branched_location, user_id, **kwargs) + self._auto_publish_no_children(parent_loc, parent_loc.category, user_id) def _map_revision_to_branch(self, key, revision=None): """ @@ -158,20 +207,24 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS except ItemNotFoundError: return True - return draft.update_version != published.update_version + return draft.update_version != published.source_version - def publish(self, location, user_id, **kwargs): + def publish(self, location, user_id, blacklist=None, **kwargs): """ - Save a current draft to the underlying modulestore. + Publishes the subtree under location from the draft branch to the published branch Returns the newly published item. """ SplitMongoModuleStore.copy( self, user_id, - location.course_key.for_branch(ModuleStoreEnum.BranchName.draft), + # Directly using the replace function rather than the for_branch function + # because for_branch obliterates the version_guid and will lead to missed version conflicts. + location.course_key.replace(branch=ModuleStoreEnum.BranchName.draft), location.course_key.for_branch(ModuleStoreEnum.BranchName.published), [location], + blacklist=blacklist ) + return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published)) def unpublish(self, location, user_id): """ @@ -211,7 +264,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS Return the version of the given database representation of a block. """ #TODO: make this method a more generic helper - return block['edit_info']['update_version'] + return block['edit_info'].get('source_version', block['edit_info']['update_version']) draft_head = get_head(ModuleStoreEnum.BranchName.draft) published_head = get_head(ModuleStoreEnum.BranchName.published) 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 f3ab2ec7ec..acbb502287 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -318,6 +318,62 @@ class TestMixedModuleStore(unittest.TestCase): course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key) self.assertTrue(course.show_calculator) + @ddt.data('draft', 'split') + def test_has_changes_direct_only(self, default_ms): + """ + Tests that has_changes() returns false when a new xblock in a direct only category is checked + """ + self.initdb(default_ms) + + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + + # Create dummy direct only xblocks + chapter = self.store.create_item( + self.user_id, + test_course.id.version_agnostic(), + 'chapter', + block_id='vertical_container' + ) + + # Check that neither xblock has changes + self.assertFalse(self.store.has_changes(test_course.location)) + self.assertFalse(self.store.has_changes(chapter.location)) + + @ddt.data('draft', 'split') + def test_has_changes(self, default_ms): + """ + Tests that has_changes() only returns true when changes are present + """ + self.initdb(default_ms) + + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + + # Create a dummy component to test against + xblock = self.store.create_item( + self.user_id, + test_course.id.version_agnostic(), + 'vertical', + block_id='test_vertical' + ) + + # Not yet published, so changes are present + self.assertTrue(self.store.has_changes(xblock.location)) + + # Publish and verify that there are no unpublished changes + self.store.publish(xblock.location, self.user_id) + self.assertFalse(self.store.has_changes(xblock.location)) + + # Change the component, then check that there now are changes + component = self.store.get_item(xblock.location) + component.display_name = 'Changed Display Name' + + component = self.store.update_item(component, self.user_id) + self.assertTrue(self.store.has_changes(component.location)) + + # Publish and verify again + self.store.publish(component.location, self.user_id) + self.assertFalse(self.store.has_changes(component.location)) + @ddt.data('draft', 'split') def test_delete_item(self, default_ms): """ @@ -370,10 +426,6 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_item(leaf_loc)) self.assertNotIn(vert_loc, course.children) - # 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_child( @@ -392,7 +444,7 @@ class TestMixedModuleStore(unittest.TestCase): revision=ModuleStoreEnum.RevisionOption.draft_preferred ) - self.store.publish(private_vert.location, self.user_id) + self.store.publish(private_vert.location.version_agnostic(), self.user_id) private_leaf.display_name = 'change me' private_leaf = self.store.update_item(private_leaf, self.user_id) # test succeeds if delete succeeds w/o error @@ -468,7 +520,7 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() # publish the course - self.store.publish(self.course.location, self.user_id) + self.course = self.store.publish(self.course.location.version_agnostic(), self.user_id) # make drafts of verticals self.store.convert_to_draft(self.vertical_x1a, self.user_id) @@ -494,7 +546,7 @@ class TestMixedModuleStore(unittest.TestCase): ]) # publish the course again - self.store.publish(self.course.location, self.user_id) + self.store.publish(self.course.location.version_agnostic(), self.user_id) self.verify_get_parent_locations_results([ (child_to_move_location, new_parent_location, None), (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred), @@ -724,7 +776,7 @@ class TestMixedModuleStore(unittest.TestCase): self._create_block_hierarchy() # publish - self.store.publish(self.course.location, self.user_id) + self.store.publish(self.course.location.version_agnostic(), self.user_id) published_xblock = self.store.get_item( self.vertical_x1a, revision=ModuleStoreEnum.RevisionOption.published_only @@ -755,11 +807,6 @@ class TestMixedModuleStore(unittest.TestCase): 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() @@ -795,6 +842,61 @@ class TestMixedModuleStore(unittest.TestCase): self.assertTrue(self.store.has_changes(item.location)) self.assertEquals(self.store.compute_publish_state(item), PublishState.draft) + @ddt.data('draft', 'split') + def test_auto_publish(self, default_ms): + """ + Test that the correct things have been published automatically + Assumptions: + * we auto-publish courses, chapters, sequentials + * we don't auto-publish problems + """ + + self.initdb(default_ms) + + # test create_course to make sure we are autopublishing + test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) + self.assertEqual(self.store.compute_publish_state(test_course), PublishState.public) + + test_course_key = test_course.id.version_agnostic() + + # test create_item of direct-only category to make sure we are autopublishing + chapter = self.store.create_item(self.user_id, test_course_key, 'chapter', 'Overview') + self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + + chapter_location = chapter.location.version_agnostic() + + # test create_child of direct-only category to make sure we are autopublishing + sequential = self.store.create_child(self.user_id, chapter_location, 'sequential', 'Sequence') + self.assertEqual(self.store.compute_publish_state(sequential), PublishState.public) + + # test update_item of direct-only category to make sure we are autopublishing + sequential.display_name = 'sequential1' + sequential = self.store.update_item(sequential, self.user_id) + self.assertEqual(self.store.compute_publish_state(sequential), PublishState.public) + + # test delete_item of direct-only category to make sure we are autopublishing + self.store.delete_item(sequential.location, self.user_id, revision=ModuleStoreEnum.RevisionOption.all) + chapter = self.store.get_item(chapter.location.for_branch(None)) + self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + + # test create_child of NOT direct-only category to make sure we aren't autopublishing + problem_child = self.store.create_child(self.user_id, chapter_location, 'problem', 'Problem_Child') + self.assertEqual(self.store.compute_publish_state(problem_child), PublishState.private) + + # test create_item of NOT direct-only category to make sure we aren't autopublishing + problem_item = self.store.create_item(self.user_id, test_course_key, 'problem', 'Problem_Item') + self.assertEqual(self.store.compute_publish_state(problem_item), PublishState.private) + + # test update_item of NOT direct-only category to make sure we aren't autopublishing + problem_item.display_name = 'Problem_Item1' + problem_item = self.store.update_item(problem_item, self.user_id) + self.assertEqual(self.store.compute_publish_state(problem_item), PublishState.private) + + # test delete_item of NOT direct-only category to make sure we aren't autopublishing + self.store.delete_item(problem_child.location, self.user_id) + chapter = self.store.get_item(chapter.location.for_branch(None)) + self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + @ddt.data('draft', 'split') def test_get_courses_for_wiki_shared(self, default_ms): """ @@ -858,19 +960,8 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) self._create_block_hierarchy() - # TODO - Remove these lines once LMS-11017 is implemented - course_location = self.course_locations[self.MONGO_COURSEID] - self.store.publish(course_location, self.user_id) - problem_original_name = 'Problem_Original' - problem = self.store.create_child( - self.user_id, self.writable_chapter_location, 'problem', 'prob_block', - fields={'display_name': problem_original_name}, - ) - problem_location = problem.location.version_agnostic().for_branch(None) - - # TODO - Uncomment out these lines once LMS-11017 is implemented - # problem_location = self.problem_x1a_1 - # problem_original_name = 'Problem_x1a_1' + problem_location = self.problem_x1a_1.for_branch(None) + problem_original_name = 'Problem_x1a_1' course_key = problem_location.course_key problem_new_name = 'New Problem Name' @@ -880,7 +971,7 @@ class TestMixedModuleStore(unittest.TestCase): Asserts the number of problems with the given display name is the given expected number. """ self.assertEquals( - len(self.store.get_items(course_key, settings={'display_name': display_name})), + len(self.store.get_items(course_key.for_branch(None), settings={'display_name': display_name})), expected_number ) @@ -907,6 +998,7 @@ class TestMixedModuleStore(unittest.TestCase): self.store.get_item(problem_location) # PUBLISH the problem + self.store.publish(self.vertical_x1a, self.user_id) self.store.publish(problem_location, self.user_id) # verify Published problem diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index cfee0d401b..4ea8530646 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -508,56 +508,6 @@ class TestMongoModuleStore(unittest.TestCase): finally: shutil.rmtree(root_dir) - def test_has_changes_direct_only(self): - """ - Tests that has_changes() returns false when a new xblock in a direct only category is checked - """ - 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, - 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)) - self.assertFalse(self.draft_store.has_changes(chapter_location)) - - def test_has_changes(self): - """ - Tests that has_changes() only returns true when changes are present - """ - location = Location('edX', 'toy', '2012_Fall', 'vertical', 'test_vertical') - - # Create a dummy component to test against - self.draft_store.create_item( - self.dummy_user, - location.course_key, - location.block_type, - block_id=location.block_id - ) - - # Not yet published, so changes are present - self.assertTrue(self.draft_store.has_changes(location)) - - # Publish and verify that there are no unpublished changes - self.draft_store.publish(location, self.dummy_user) - self.assertFalse(self.draft_store.has_changes(location)) - - # Change the component, then check that there now are changes - component = self.draft_store.get_item(location) - component.display_name = 'Changed Display Name' - self.draft_store.update_item(component, self.dummy_user) - self.assertTrue(self.draft_store.has_changes(location)) - - # Publish and verify again - self.draft_store.publish(location, self.dummy_user) - self.assertFalse(self.draft_store.has_changes(location)) - def test_has_changes_missing_child(self): """ Tests that has_changes() returns False when a published parent points to a child that doesn't exist. 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 deleted file mode 100644 index e3a230e41d..0000000000 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_draft_modulestore.py +++ /dev/null @@ -1,62 +0,0 @@ -""" - Test split_draft modulestore -""" -import unittest -import uuid -from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore -from xmodule.modulestore import ModuleStoreEnum -from opaque_keys.edx.locator import CourseLocator -from xmodule.modulestore.inheritance import InheritanceMixin -from xmodule.x_module import XModuleMixin -from xmodule.modulestore.tests.test_split_modulestore import SplitModuleTest - -# pylint: disable=W0613 -def render_to_template_mock(*args): - pass - - -class TestDraftVersioningModuleStore(unittest.TestCase): - def setUp(self): - super(TestDraftVersioningModuleStore, self).setUp() - self.module_store = DraftVersioningModuleStore( - contentstore=None, - doc_store_config={ - 'host': 'localhost', - 'db': 'test_xmodule', - 'collection': 'modulestore{0}'.format(uuid.uuid4().hex[:5]), - }, - fs_root='', - default_class='xmodule.raw_module.RawDescriptor', - render_template=render_to_template_mock, - xblock_mixins=(InheritanceMixin, XModuleMixin), - ) - self.addCleanup(self.module_store._drop_database) - - SplitModuleTest.bootstrapDB(self.module_store) - - def test_has_changes(self): - """ - Tests that has_changes() only returns true when changes are present - """ - draft_course = CourseLocator( - org='testx', course='GreekHero', run='run', branch=ModuleStoreEnum.BranchName.draft - ) - head = draft_course.make_usage_key('course', 'head12345') - dummy_user = ModuleStoreEnum.UserID.test - - # Not yet published, so changes are present - self.assertTrue(self.module_store.has_changes(head)) - - # Publish and verify that there are no unpublished changes - self.module_store.publish(head, dummy_user) - self.assertFalse(self.module_store.has_changes(head)) - - # Change the course, then check that there now are changes - course = self.module_store.get_item(head) - course.show_calculator = not course.show_calculator - self.module_store.update_item(course, dummy_user) - self.assertTrue(self.module_store.has_changes(head)) - - # Publish and verify again - self.module_store.publish(head, dummy_user) - self.assertFalse(self.module_store.has_changes(head)) 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 0554481435..a97ae4d264 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1687,7 +1687,12 @@ class TestPublish(SplitModuleTest): pub_copy = modulestore().get_item(dest_course_loc.make_usage_key("", 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( + source.update_version, pub_copy.source_version, + u"Versions don't match for {}: {} != {}".format( + expected, source.update_version, pub_copy.update_version + ) + ) self.assertEqual( self.user_id, pub_copy.edited_by, "{} edited_by {} not {}".format(pub_copy.location, pub_copy.edited_by, self.user_id) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 4a177040c0..60cb5f6700 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -15,7 +15,7 @@ import json import os from path import path import shutil -from xmodule.modulestore.mongo.base import DIRECT_ONLY_CATEGORIES +from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES DRAFT_DIR = "drafts" PUBLISHED_DIR = "published"