Split branch settings LMS-11056

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))
+

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):

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:

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)
+        def get_version(block):
+            return block['edit_info']['update_version']
+
+        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:
-            raise ValueError(u'{} is in a branch other than draft or published.'.format(xblock.location))
-
-        if not other:
-            if xblock.location.branch == ModuleStoreEnum.BranchName.draft:
-                return PublishState.private
-            else:
-                return PublishState.public
-        elif xblock.update_version != other['edit_info']['update_version']:
+            # published and draft versions differ
             return PublishState.draft
-        else:
-            return PublishState.public
 
     def convert_to_draft(self, location, user_id):
         """

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

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))