diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 2ae0da1588..fea45d7d7f 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -100,3 +100,17 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): @abstractmethod def convert_to_draft(self, location, user_id): raise NotImplementedError + + +class UnsupportedRevisionError(ValueError): + """ + This error is raised if a method is called with an unsupported revision parameter. + """ + def __init__(self, allowed_revisions=None): + if not allowed_revisions: + allowed_revisions = [ + None, + ModuleStoreEnum.RevisionOption.published_only, + ModuleStoreEnum.RevisionOption.draft_only + ] + super(UnsupportedRevisionError, self).__init__('revision not one of {}'.format(allowed_revisions)) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 69bb96fa20..7b955de8c3 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -20,6 +20,7 @@ from xmodule.modulestore.mongo.base import ( DIRECT_ONLY_CATEGORIES, SORT_REVISION_FAVOR_DRAFT ) from xmodule.modulestore.store_utilities import rewrite_nonportable_content_links +from xmodule.modulestore.draft_and_published import UnsupportedRevisionError log = logging.getLogger(__name__) @@ -97,7 +98,7 @@ class DraftModuleStore(MongoModuleStore): elif self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: return get_published() - else: + elif revision is None: # could use a single query wildcarding revision and sorting by revision. would need to # use prefix form of to_deprecated_son try: @@ -107,6 +108,9 @@ class DraftModuleStore(MongoModuleStore): # otherwise, fall back to the published version return get_published() + else: + raise UnsupportedRevisionError() + def has_item(self, usage_key, revision=None): """ Returns True if location exists in this ModuleStore. @@ -127,13 +131,17 @@ class DraftModuleStore(MongoModuleStore): if revision == ModuleStoreEnum.RevisionOption.draft_only: return has_draft() - elif revision == ModuleStoreEnum.RevisionOption.published_only \ - or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: + elif ( + revision == ModuleStoreEnum.RevisionOption.published_only or + self.get_branch_setting() == ModuleStoreEnum.Branch.published_only + ): return has_published() - else: + elif revision is None: key = usage_key.to_deprecated_son(prefix='_id.') del key['_id.revision'] return self.collection.find(key).count() > 0 + else: + raise UnsupportedRevisionError() def delete_course(self, course_key, user_id): """ @@ -342,9 +350,11 @@ class DraftModuleStore(MongoModuleStore): elif revision == ModuleStoreEnum.RevisionOption.published_only \ or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: return published_items([]) - else: + elif revision is None: draft_items = draft_items() return draft_items + published_items(draft_items) + else: + raise UnsupportedRevisionError() def convert_to_draft(self, location, user_id): """ @@ -519,7 +529,13 @@ class DraftModuleStore(MongoModuleStore): elif revision is None: as_functions = [as_draft] else: - raise ValueError('revision not one of None, ModuleStoreEnum.RevisionOption.published_only, or ModuleStoreEnum.RevisionOption.all') + raise UnsupportedRevisionError( + [ + None, + ModuleStoreEnum.RevisionOption.published_only, + 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_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 1778fd05f9..70926631a3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -125,7 +125,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): super(SplitMongoModuleStore, self).__init__(contentstore, **kwargs) - self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only) self.db_connection = MongoConnection(**doc_store_config) self.db = self.db_connection.database @@ -296,14 +295,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): ''' if course_locator.org and course_locator.course and course_locator.run: if course_locator.branch is None: - # default it based on branch_setting - # NAATODO move this to your mixin - if self.branch_setting_func() == ModuleStoreEnum.Branch.draft_preferred: - course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.draft) - elif self.branch_setting_func() == ModuleStoreEnum.Branch.published_only: - course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.published) - else: - raise InsufficientSpecificationError(course_locator) + raise InsufficientSpecificationError(course_locator) # use the course id index = self.db_connection.get_course_index(course_locator) if index is None: @@ -1806,8 +1798,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 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']['children'] - for child in destination_block['fields']['children']: + source_children = new_block['fields'].get('children', []) + for child in destination_block['fields'].get('children', []): try: source_children.index(child) except ValueError: 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 844fee3ffc..2e8bd77bd0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -5,8 +5,9 @@ Module for the dual-branch fall-back Draft->Published Versioning ModuleStore 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_and_published import ModuleStoreDraftAndPublished, UnsupportedRevisionError from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES +from xmodule.modulestore.exceptions import InsufficientSpecificationError class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore): @@ -14,7 +15,36 @@ 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 _lookup_course(self, course_locator): + """ + overrides the implementation of _lookup_course in SplitMongoModuleStore in order to + use the configured branch_setting in the course_locator + """ + if course_locator.org and course_locator.course and course_locator.run: + if course_locator.branch is None: + # default it based on branch_setting + branch_setting = self.get_branch_setting() + if branch_setting == ModuleStoreEnum.Branch.draft_preferred: + course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.draft) + elif branch_setting == ModuleStoreEnum.Branch.published_only: + course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.published) + else: + raise InsufficientSpecificationError(course_locator) + return super(DraftVersioningModuleStore, self)._lookup_course(course_locator) + def create_course(self, org, course, run, user_id, **kwargs): + """ + Creates and returns the course. + + Args: + org (str): the organization that owns the course + course (str): the name of the course + run (str): the name of the run + user_id: id of the user creating the course + kwargs: Any optional arguments understood by a subset of modulestores to customize instantiation + + Returns: a CourseDescriptor + """ master_branch = kwargs.pop('master_branch', ModuleStoreEnum.BranchName.draft) return super(DraftVersioningModuleStore, self).create_course( org, course, run, user_id, master_branch=master_branch, **kwargs @@ -47,7 +77,14 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS 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') + raise UnsupportedRevisionError( + [ + None, + ModuleStoreEnum.RevisionOption.published_only, + ModuleStoreEnum.RevisionOption.all + ] + ) + for branch in branches_to_delete: SplitMongoModuleStore.delete_item(self, location.for_branch(branch), user_id, **kwargs) @@ -59,8 +96,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS return key.for_branch(ModuleStoreEnum.BranchName.published) elif revision == ModuleStoreEnum.RevisionOption.draft_only: return key.for_branch(ModuleStoreEnum.BranchName.draft) - else: + elif revision is None: return key + else: + raise UnsupportedRevisionError() def has_item(self, usage_key, revision=None): """ @@ -163,30 +202,29 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS PublishState.public - published exists and is the same as draft PublishState.private - no published version exists """ - # TODO figure out what to say if xblock is not from the HEAD of its branch def get_head(branch): 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 == ModuleStoreEnum.BranchName.draft: - 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 in a branch other than draft or published.'.format(xblock.location)) + def get_version(block): + """ + 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'] - if not other: - if xblock.location.branch == ModuleStoreEnum.BranchName.draft: - return PublishState.private - else: - return PublishState.public - elif xblock.update_version != other['edit_info']['update_version']: - return PublishState.draft - else: + draft_head = get_head(ModuleStoreEnum.BranchName.draft) + published_head = get_head(ModuleStoreEnum.BranchName.published) + + if not published_head: + # published version does not exist + return PublishState.private + elif get_version(draft_head) == get_version(published_head): + # published and draft versions are equal return PublishState.public + else: + # published and draft versions differ + return PublishState.draft def convert_to_draft(self, location, user_id): """ 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 1951138041..3de206161d 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -20,6 +20,7 @@ from django.conf import settings if not settings.configured: settings.configure() from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.draft_and_published import UnsupportedRevisionError @ddt.ddt @@ -255,6 +256,10 @@ class TestMixedModuleStore(unittest.TestCase): )) self.assertFalse(self.store.has_item(self.fake_location)) + # verify that an error is raised when the revision is not valid + with self.assertRaises(UnsupportedRevisionError): + self.store.has_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred) + @ddt.data('draft', 'split') def test_get_item(self, default_ms): self.initdb(default_ms) @@ -269,6 +274,10 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(ItemNotFoundError): self.store.get_item(self.fake_location) + # verify that an error is raised when the revision is not valid + with self.assertRaises(UnsupportedRevisionError): + self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred) + @ddt.data('draft', 'split') def test_get_items(self, default_ms): self.initdb(default_ms) @@ -279,6 +288,13 @@ class TestMixedModuleStore(unittest.TestCase): self.assertEqual(len(modules), 1) self.assertEqual(modules[0].location, course_locn) + # verify that an error is raised when the revision is not valid + with self.assertRaises(UnsupportedRevisionError): + self.store.get_items( + self.course_locations[self.MONGO_COURSEID].course_key, + revision=ModuleStoreEnum.RevisionOption.draft_preferred + ) + @ddt.data('draft', 'split') def test_update_item(self, default_ms): """ @@ -366,6 +382,14 @@ class TestMixedModuleStore(unittest.TestCase): self.user_id, private_vert.location, 'html', block_id='bug_leaf' ) + # verify that an error is raised when the revision is not valid + with self.assertRaises(UnsupportedRevisionError): + self.store.delete_item( + private_leaf.location, + self.user_id, + revision=ModuleStoreEnum.RevisionOption.draft_preferred + ) + self.store.publish(private_vert.location, self.user_id) private_leaf.display_name = 'change me' private_leaf = self.store.update_item(private_leaf, self.user_id) @@ -802,7 +826,7 @@ class TestMixedModuleStore(unittest.TestCase): wiki_courses ) - @ddt.data('draft') + @ddt.data('draft', 'split') def test_branch_setting(self, default_ms): """ Test the branch_setting context manager @@ -810,10 +834,22 @@ class TestMixedModuleStore(unittest.TestCase): self.initdb(default_ms) self._create_block_hierarchy() - problem_location = self.problem_x1a_1 - problem_original_name = 'Problem_x1a_1' - problem_new_name = 'New Problem Name' + # 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' + course_key = problem_location.course_key + problem_new_name = 'New Problem Name' def assertNumProblems(display_name, expected_number): """ @@ -837,10 +873,12 @@ class TestMixedModuleStore(unittest.TestCase): # verify Draft problem with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + self.assertTrue(self.store.has_item(problem_location)) assertProblemNameEquals(problem_original_name) # verify Published problem doesn't exist with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + self.assertFalse(self.store.has_item(problem_location)) with self.assertRaises(ItemNotFoundError): self.store.get_item(problem_location) @@ -849,6 +887,7 @@ class TestMixedModuleStore(unittest.TestCase): # verify Published problem with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + self.assertTrue(self.store.has_item(problem_location)) assertProblemNameEquals(problem_original_name) # verify Draft-preferred 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 b9995df043..0554481435 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -12,8 +12,11 @@ import random from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.exceptions import (ItemNotFoundError, VersionConflictError, - DuplicateItemError, DuplicateCourseError) +from xmodule.modulestore.exceptions import ( + ItemNotFoundError, VersionConflictError, + DuplicateItemError, DuplicateCourseError, + InsufficientSpecificationError +) from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator, VersionTree, LocalId from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin @@ -667,7 +670,7 @@ class SplitModuleCourseTests(SplitModuleTest): def test_get_course_negative(self): # Now negative testing - with self.assertRaises(ItemNotFoundError): + with self.assertRaises(InsufficientSpecificationError): modulestore().get_course(CourseLocator(org='edu', course='meh', run='blah')) with self.assertRaises(ItemNotFoundError): modulestore().get_course(CourseLocator(org='edu', course='nosuchthing', run="run", branch=BRANCH_NAME_DRAFT)) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index 8366413215..731e5dea7f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -122,4 +122,5 @@ class TestXMLModuleStore(unittest.TestCase): # XML store does NOT allow draft_preferred branch setting with self.assertRaises(ValueError): with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): - pass + # verify that the above context manager raises a ValueError + pass # pragma: no cover