Merge pull request #4532 from edx/split/branch-settings

Split/branch settings LMS-11056

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

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

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

@@ -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)
+        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']
+
+        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

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

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

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