diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index b2bc7b0eaf..f7af93dace 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -548,18 +548,6 @@ class ModuleStoreReadBase(ModuleStoreRead): raise ValueError(u"Cannot set default store to type {}".format(store_type)) yield - @contextmanager - def branch_setting(self, branch_setting, course_id=None): - """ - A context manager for temporarily setting a store's branch value - """ - previous_branch_setting_func = getattr(self, 'branch_setting_func', None) - try: - self.branch_setting_func = lambda: branch_setting - yield - finally: - self.branch_setting_func = previous_branch_setting_func - class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): ''' diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index c6b98d1ba3..2ae0da1588 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -2,10 +2,64 @@ This module provides an abstraction for Module Stores that support Draft and Published branches. """ +import threading from abc import ABCMeta, abstractmethod +from contextlib import contextmanager from . import ModuleStoreEnum -class ModuleStoreDraftAndPublished(object): + +class BranchSettingMixin(object): + """ + A mixin to manage a module store's branch setting. + The order of override is (from higher precedence to lower): + 1. thread-specific setting temporarily set using the branch_setting contextmanager + 2. the return value of the branch_setting_func passed into this mixin's init method + 3. the default branch setting being ModuleStoreEnum.Branch.published_only + """ + + def __init__(self, *args, **kwargs): + """ + :param branch_setting_func: a function that returns the default branch setting for this object. + If not specified, ModuleStoreEnum.Branch.published_only is used as the default setting. + """ + super(BranchSettingMixin, self).__init__(*args, **kwargs) + self.default_branch_setting_func = kwargs.pop( + 'branch_setting_func', + lambda: ModuleStoreEnum.Branch.published_only + ) + + # cache the branch setting on a local thread to support a multi-threaded environment + self.thread_cache = threading.local() + + @contextmanager + def branch_setting(self, branch_setting, course_id=None): # pylint: disable=unused-argument + """ + A context manager for temporarily setting a store's branch value on the current thread. + """ + previous_thread_branch_setting = getattr(self.thread_cache, 'branch_setting', None) + try: + self.thread_cache.branch_setting = branch_setting + yield + finally: + self.thread_cache.branch_setting = previous_thread_branch_setting + + def get_branch_setting(self, course_id=None): # pylint: disable=unused-argument + """ + Returns the current branch_setting on the store. + + Returns the thread-local setting, if set. + Otherwise, returns the default value of the setting function set during the store's initialization. + """ + # first check the thread-local cache + thread_local_branch_setting = getattr(self.thread_cache, 'branch_setting', None) + if thread_local_branch_setting: + return thread_local_branch_setting + else: + # return the default value + return self.default_branch_setting_func() + + +class ModuleStoreDraftAndPublished(BranchSettingMixin): """ A mixin for a read-write database backend that supports two branches, Draft and Published, with options to prefer Draft and fallback to Published. @@ -14,7 +68,6 @@ class ModuleStoreDraftAndPublished(object): def __init__(self, *args, **kwargs): super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs) - self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only) @abstractmethod def delete_item(self, location, user_id, revision=None, **kwargs): diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 8e2f29cd6a..2e067c3c9d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -142,7 +142,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): store = self._get_modulestore_for_courseid(usage_key.course_key) return store.get_item(usage_key, depth, **kwargs) - def get_items(self, course_key, settings=None, content=None, **kwargs): + def get_items(self, course_key, **kwargs): """ Returns: list of XModuleDescriptor instances for the matching items within the course with @@ -153,11 +153,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): Args: course_key (CourseKey): the course identifier - settings (dict): fields to look for which have settings scope. Follows same syntax - and rules as kwargs below - content (dict): fields to look for which have content scope. Follows same syntax and - rules as kwargs below. - kwargs (key=value): what to look for within the course. + kwargs: + settings (dict): fields to look for which have settings scope. Follows same syntax + and rules as kwargs below + content (dict): fields to look for which have content scope. Follows same syntax and + rules as kwargs below. + additional kwargs (key=value): what to look for within the course. Common qualifiers are ``category`` or any field name. if the target field is a list, then it searches for the given value in the list not list equivalence. Substring matching pass a regex object. @@ -170,7 +171,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): raise Exception("Must pass in a course_key when calling get_items()") store = self._get_modulestore_for_courseid(course_key) - return store.get_items(course_key, settings=settings, content=content, **kwargs) + return store.get_items(course_key, **kwargs) def get_courses(self): ''' @@ -519,7 +520,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): A context manager for temporarily setting the branch value for the given course' store to the given branch_setting. If course_id is None, the default store is used. """ - store = self._get_modulestore_for_courseid(course_id) + store = self._verify_modulestore_support(course_id, 'branch_setting') with store.branch_setting(branch_setting, course_id): yield diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 63f960da5b..69bb96fa20 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -46,15 +46,6 @@ class DraftModuleStore(MongoModuleStore): This module also includes functionality to promote DRAFT modules (and their children) to published modules. """ - - def __init__(self, *args, **kwargs): - """ - Args: - branch_setting_func: a function that returns the branch setting to use for this store's operations. - This should be an attribute from ModuleStoreEnum.Branch - """ - super(DraftModuleStore, self).__init__(*args, **kwargs) - def get_item(self, usage_key, depth=0, revision=None): """ Returns an XModuleDescriptor instance for the item at usage_key. @@ -103,7 +94,7 @@ class DraftModuleStore(MongoModuleStore): elif revision == ModuleStoreEnum.RevisionOption.draft_only: return get_draft() - elif self.branch_setting_func() == ModuleStoreEnum.Branch.published_only: + elif self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: return get_published() else: @@ -137,7 +128,7 @@ class DraftModuleStore(MongoModuleStore): if revision == ModuleStoreEnum.RevisionOption.draft_only: return has_draft() elif revision == ModuleStoreEnum.RevisionOption.published_only \ - or self.branch_setting_func() == ModuleStoreEnum.Branch.published_only: + or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: return has_published() else: key = usage_key.to_deprecated_son(prefix='_id.') @@ -282,7 +273,7 @@ class DraftModuleStore(MongoModuleStore): ''' if revision is None: revision = ModuleStoreEnum.RevisionOption.published_only \ - if self.branch_setting_func() == ModuleStoreEnum.Branch.published_only \ + if self.get_branch_setting() == ModuleStoreEnum.Branch.published_only \ else ModuleStoreEnum.RevisionOption.draft_preferred return super(DraftModuleStore, self).get_parent_location(location, revision, **kwargs) @@ -305,7 +296,7 @@ class DraftModuleStore(MongoModuleStore): super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields) ) - def get_items(self, course_key, settings=None, content=None, revision=None, **kwargs): + def get_items(self, course_key, revision=None, **kwargs): """ Performance Note: This is generally a costly operation, but useful for wildcard searches. @@ -317,8 +308,6 @@ class DraftModuleStore(MongoModuleStore): Args: course_key (CourseKey): the course identifier - settings: not used - content: not used revision: ModuleStoreEnum.RevisionOption.published_only - returns only Published items ModuleStoreEnum.RevisionOption.draft_only - returns only Draft items @@ -351,7 +340,7 @@ class DraftModuleStore(MongoModuleStore): if revision == ModuleStoreEnum.RevisionOption.draft_only: return draft_items() elif revision == ModuleStoreEnum.RevisionOption.published_only \ - or self.branch_setting_func() == ModuleStoreEnum.Branch.published_only: + or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: return published_items([]) else: draft_items = draft_items() @@ -747,7 +736,7 @@ class DraftModuleStore(MongoModuleStore): for non_draft in to_process_non_drafts: to_process_dict[Location._from_deprecated_son(non_draft["_id"], course_key.run)] = non_draft - if self.branch_setting_func() == ModuleStoreEnum.Branch.draft_preferred: + if self.get_branch_setting() == ModuleStoreEnum.Branch.draft_preferred: # now query all draft content in another round-trip query = [] for item in items: @@ -801,7 +790,7 @@ class DraftModuleStore(MongoModuleStore): """ Raises an exception if the current branch setting does not match the expected branch setting. """ - actual_branch_setting = self.branch_setting_func() + actual_branch_setting = self.get_branch_setting() if actual_branch_setting != expected_branch_setting: raise InvalidBranchSetting( expected_setting=expected_branch_setting, 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 400b5dbb06..1951138041 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,7 +1,6 @@ import pymongo from uuid import uuid4 import ddt -from mock import patch from importlib import import_module from collections import namedtuple import unittest @@ -172,7 +171,8 @@ class TestMixedModuleStore(unittest.TestCase): def create_sub_tree(parent, block_info): block = self.store.create_child( self.user_id, parent.location.version_agnostic(), - block_info.category, block_id=block_info.display_name + block_info.category, block_id=block_info.display_name, + fields={'display_name': block_info.display_name}, ) for tree in block_info.sub_tree: create_sub_tree(block, tree) @@ -802,6 +802,83 @@ class TestMixedModuleStore(unittest.TestCase): wiki_courses ) + @ddt.data('draft') + def test_branch_setting(self, default_ms): + """ + Test the branch_setting context manager + """ + 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' + course_key = problem_location.course_key + + def assertNumProblems(display_name, expected_number): + """ + Asserts the number of problems with the given display name is the given expected number. + """ + self.assertEquals( + len(self.store.get_items(course_key, settings={'display_name': display_name})), + expected_number + ) + + def assertProblemNameEquals(expected_display_name): + """ + Asserts the display_name of the xblock at problem_location matches the given expected value. + """ + # check the display_name of the problem + problem = self.store.get_item(problem_location) + self.assertEquals(problem.display_name, expected_display_name) + + # there should be only 1 problem with the expected_display_name + assertNumProblems(expected_display_name, 1) + + # verify Draft problem + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + assertProblemNameEquals(problem_original_name) + + # verify Published problem doesn't exist + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + with self.assertRaises(ItemNotFoundError): + self.store.get_item(problem_location) + + # PUBLISH the problem + self.store.publish(problem_location, self.user_id) + + # verify Published problem + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + assertProblemNameEquals(problem_original_name) + + # verify Draft-preferred + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + assertProblemNameEquals(problem_original_name) + + # EDIT name + problem = self.store.get_item(problem_location) + problem.display_name = problem_new_name + self.store.update_item(problem, self.user_id) + + # verify Draft problem has new name + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + assertProblemNameEquals(problem_new_name) + + # verify Published problem still has old name + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + assertProblemNameEquals(problem_original_name) + # there should be no published problems with the new name + assertNumProblems(problem_new_name, 0) + + # PUBLISH the problem + self.store.publish(problem_location, self.user_id) + + # verify Published problem has new name + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + assertProblemNameEquals(problem_new_name) + # there should be no published problems with the old name + assertNumProblems(problem_original_name, 0) + #============================================================================================================= # General utils for not using django settings diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index 3c247b7644..8366413215 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -107,3 +107,19 @@ class TestXMLModuleStore(unittest.TestCase): SlashSeparatedCourseKey('edX', 'toy', '2012_Fall'), locator_key_fields=SlashSeparatedCourseKey.KEY_FIELDS ) + + def test_branch_setting(self): + """ + Test the branch setting context manager + """ + store = XMLModuleStore(DATA_DIR, course_dirs=['toy']) + course_key = store.get_courses()[0] + + # XML store allows published_only branch setting + with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + store.get_item(course_key.location) + + # XML store does NOT allow draft_preferred branch setting + with self.assertRaises(ValueError): + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + pass diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 9ee73cefa3..5c71518364 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -13,6 +13,7 @@ from fs.osfs import OSFS from importlib import import_module from lxml import etree from path import path +from contextlib import contextmanager from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import make_error_tracker, exc_info_to_str @@ -409,9 +410,6 @@ class XMLModuleStore(ModuleStoreReadBase): self.i18n_service = i18n_service - # The XML Module Store is a read-only store and only handles published content - self.branch_setting_func = lambda: ModuleStoreEnum.RevisionOption.published_only - # If we are specifically asked for missing courses, that should # be an error. If we are asked for "all" courses, find the ones # that have a course.xml. We sort the dirs in alpha order so we always @@ -826,3 +824,11 @@ class XMLModuleStore(ModuleStoreReadBase): """ return {ModuleStoreEnum.Type.xml: True} + @contextmanager + def branch_setting(self, branch_setting, course_id=None): # pylint: disable=unused-argument + """ + A context manager for temporarily setting the branch value for the store to the given branch_setting. + """ + if branch_setting != ModuleStoreEnum.Branch.published_only: + raise ValueError(u"Cannot set branch setting to {} on a ReadOnly store".format(branch_setting)) + yield diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index d3f729ee2e..84527ce8ba 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -162,8 +162,8 @@ def import_from_xml( # method on XmlModuleStore. course_items = [] - with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - for course_key in xml_module_store.modules.keys(): + for course_key in xml_module_store.modules.keys(): + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): if target_course_id is not None: dest_course_id = target_course_id