Split branch settings code review fixes LMS-11056

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -101,10 +101,16 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
     def convert_to_draft(self, location, user_id):
         raise NotImplementedError
 
-    def _unsupported_revision_error(self, allowed_revisions=None):
+
+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
+                None,
+                ModuleStoreEnum.RevisionOption.published_only,
+                ModuleStoreEnum.RevisionOption.draft_only
             ]
-        return ValueError('revision not one of {}'.format(allowed_revisions))
-
+        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,8 +529,12 @@ class DraftModuleStore(MongoModuleStore):
         elif revision is None:
             as_functions = [as_draft]
         else:
-            raise self._unsupported_revision_error(
-                [None, ModuleStoreEnum.RevisionOption.published_only, ModuleStoreEnum.RevisionOption.all]
+            raise UnsupportedRevisionError(
+                [
+                    None,
+                    ModuleStoreEnum.RevisionOption.published_only,
+                    ModuleStoreEnum.RevisionOption.all
+                ]
             )
         self._delete_subtree(location, as_functions)
 

common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py

@@ -5,7 +5,7 @@ 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
 
@@ -77,8 +77,12 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         elif revision is None:
             branches_to_delete = [ModuleStoreEnum.BranchName.draft]
         else:
-            raise self._unsupported_revision_error(
-                [None, ModuleStoreEnum.RevisionOption.published_only, ModuleStoreEnum.RevisionOption.all]
+            raise UnsupportedRevisionError(
+                [
+                    None,
+                    ModuleStoreEnum.RevisionOption.published_only,
+                    ModuleStoreEnum.RevisionOption.all
+                ]
             )
 
         for branch in branches_to_delete:
@@ -95,7 +99,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         elif revision is None:
             return key
         else:
-            raise self._unsupported_revision_error()
+            raise UnsupportedRevisionError()
 
     def has_item(self, usage_key, revision=None):
         """
@@ -203,6 +207,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             return self._get_block_from_structure(course_structure, xblock.location.block_id)
 
         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)

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,7 +834,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         self._create_block_hierarchy()
 
-        # TODO - Remove these lines once LMS-2869 is implemented
+        # 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'
@@ -820,7 +844,7 @@ class TestMixedModuleStore(unittest.TestCase):
         )
         problem_location = problem.location.version_agnostic().for_branch(None)
 
-        # TODO - Uncomment out these lines once LMS-2869 is implemented
+        # TODO - Uncomment out these lines once LMS-11017 is implemented
         # problem_location = self.problem_x1a_1
         # problem_original_name = 'Problem_x1a_1'
 

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

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