From 1dc6ea39f34577d5e1a47243597c79e12eeb2101 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 22 Jul 2014 00:18:00 -0400 Subject: [PATCH] Split branch settings LMS-11056 --- .../modulestore/draft_and_published.py | 8 +++ .../xmodule/modulestore/mongo/draft.py | 4 +- .../xmodule/modulestore/split_mongo/split.py | 14 +--- .../modulestore/split_mongo/split_draft.py | 70 +++++++++++++------ .../tests/test_mixed_modulestore.py | 21 +++++- .../tests/test_split_modulestore.py | 4 +- 6 files changed, 84 insertions(+), 37 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 2ae0da1588..ce565aafa0 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -100,3 +100,11 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): @abstractmethod def convert_to_draft(self, location, user_id): raise NotImplementedError + + def _unsupported_revision_error(self, allowed_revisions=None): + if not allowed_revisions: + allowed_revisions = [ + None, ModuleStoreEnum.RevisionOption.published_only, ModuleStoreEnum.RevisionOption.draft_only + ] + return ValueError('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..5f6cdd51e6 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -519,7 +519,9 @@ 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 self._unsupported_revision_error( + [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..19a60411dd 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -7,6 +7,7 @@ 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 +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,10 @@ 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 self._unsupported_revision_error( + [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 +92,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 self._unsupported_revision_error() def has_item(self, usage_key, revision=None): """ @@ -163,30 +198,25 @@ 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 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..1376c5c365 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -810,10 +810,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-2869 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-2869 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 +849,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 +863,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..e8370cc690 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -13,7 +13,7 @@ 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) + 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 +667,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))