diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 055fa4afd7..91cd2b0ce2 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -51,7 +51,6 @@ from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primar from contentstore.views.preview import get_preview_fragment from edxmako.shortcuts import render_to_string from models.settings.course_grading import CourseGradingModel -from cms.lib.xblock.runtime import handler_url, local_resource_url from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryUsageLocator from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW @@ -68,12 +67,6 @@ CREATE_IF_NOT_FOUND = ['course_info'] NEVER = lambda x: False ALWAYS = lambda x: True -# In order to allow descriptors to use a handler url, we need to -# monkey-patch the x_module library. -# TODO: Remove this code when Runtimes are no longer created by modulestores -xmodule.x_module.descriptor_global_handler_url = handler_url -xmodule.x_module.descriptor_global_local_resource_url = local_resource_url - def hash_resource(resource): """ diff --git a/cms/startup.py b/cms/startup.py index bff8af0077..ca7d72dba3 100644 --- a/cms/startup.py +++ b/cms/startup.py @@ -10,6 +10,9 @@ settings.INSTALLED_APPS # pylint: disable=pointless-statement from openedx.core.lib.django_startup import autostartup from monkey_patch import django_utils_translation +import xmodule.x_module +import cms.lib.xblock.runtime + def run(): """ @@ -24,6 +27,13 @@ def run(): if settings.FEATURES.get('USE_CUSTOM_THEME', False): enable_theme() + # In order to allow descriptors to use a handler url, we need to + # monkey-patch the x_module library. + # TODO: Remove this code when Runtimes are no longer created by modulestores + # https://openedx.atlassian.net/wiki/display/PLAT/Convert+from+Storage-centric+runtimes+to+Application-centric+runtimes + xmodule.x_module.descriptor_global_handler_url = cms.lib.xblock.runtime.handler_url + xmodule.x_module.descriptor_global_local_resource_url = cms.lib.xblock.runtime.local_resource_url + def add_mimetypes(): """ diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 2f55f57a41..b9c6ef297f 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -36,7 +36,6 @@ from opaque_keys.edx.asides import AsideUsageKeyV1, AsideDefinitionKeyV1 from xmodule.exceptions import UndefinedContext import dogstats_wrapper as dog_stats_api - log = logging.getLogger(__name__) XMODULE_METRIC_NAME = 'edxapp.xmodule' @@ -1207,7 +1206,10 @@ class ConfigurableFragmentWrapper(object): # pylint: disable=abstract-method # This function exists to give applications (LMS/CMS) a place to monkey-patch until # we can refactor modulestore to split out the FieldData half of its interface from -# the Runtime part of its interface. This function matches the Runtime.handler_url interface +# the Runtime part of its interface. This function mostly matches the +# Runtime.handler_url interface. +# +# The monkey-patching happens in (lms|cms)/startup.py def descriptor_global_handler_url(block, handler_name, suffix='', query='', thirdparty=False): # pylint: disable=invalid-name, unused-argument """ See :meth:`xblock.runtime.Runtime.handler_url`. @@ -1218,6 +1220,8 @@ def descriptor_global_handler_url(block, handler_name, suffix='', query='', thir # This function exists to give applications (LMS/CMS) a place to monkey-patch until # we can refactor modulestore to split out the FieldData half of its interface from # the Runtime part of its interface. This function matches the Runtime.local_resource_url interface +# +# The monkey-patching happens in (lms|cms)/startup.py def descriptor_global_local_resource_url(block, uri): # pylint: disable=invalid-name, unused-argument """ See :meth:`xblock.runtime.Runtime.local_resource_url`. @@ -1821,6 +1825,7 @@ class CombinedSystem(object): DescriptorSystem, instead. This allows XModuleDescriptors that are bound as XModules to still function as XModuleDescriptors. """ + # First we try a lookup in the module system... try: return getattr(self._module_system, name) except AttributeError: diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index d480c07182..7fc1ddcedd 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -66,64 +66,68 @@ def unquote_slashes(text): return re.sub(r'(;;|;_)', _unquote_slashes, text) -class LmsHandlerUrls(object): +def handler_url(block, handler_name, suffix='', query='', thirdparty=False): """ - A runtime mixin that provides a handler_url function that routes - to the LMS' xblock handler view. + This method matches the signature for `xblock.runtime:Runtime.handler_url()` - This must be mixed in to a runtime that already accepts and stores - a course_id + See :method:`xblock.runtime:Runtime.handler_url` """ - # pylint: disable=unused-argument - def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False): - """See :method:`xblock.runtime:Runtime.handler_url`""" - view_name = 'xblock_handler' - if handler_name: - # Be sure this is really a handler. - func = getattr(block, handler_name, None) - if not func: - raise ValueError("{!r} is not a function name".format(handler_name)) - if not getattr(func, "_is_xblock_handler", False): - raise ValueError("{!r} is not a handler name".format(handler_name)) + view_name = 'xblock_handler' + if handler_name: + # Be sure this is really a handler. + # + # We're checking the .__class__ instead of the block itself to avoid + # auto-proxying from Descriptor -> Module, in case descriptors want + # to ask for handler URLs without a student context. + func = getattr(block.__class__, handler_name, None) + if not func: + raise ValueError("{!r} is not a function name".format(handler_name)) - if thirdparty: - view_name = 'xblock_handler_noauth' + # Is the following necessary? ProxyAttribute causes an UndefinedContext error + # if trying this without the module system. + # + #if not getattr(func, "_is_xblock_handler", False): + # raise ValueError("{!r} is not a handler name".format(handler_name)) - url = reverse(view_name, kwargs={ - 'course_id': unicode(self.course_id), - 'usage_id': quote_slashes(unicode(block.scope_ids.usage_id).encode('utf-8')), - 'handler': handler_name, - 'suffix': suffix, - }) + if thirdparty: + view_name = 'xblock_handler_noauth' - # If suffix is an empty string, remove the trailing '/' - if not suffix: - url = url.rstrip('/') + url = reverse(view_name, kwargs={ + 'course_id': unicode(block.location.course_key), + 'usage_id': quote_slashes(unicode(block.scope_ids.usage_id).encode('utf-8')), + 'handler': handler_name, + 'suffix': suffix, + }) - # If there is a query string, append it - if query: - url += '?' + query + # If suffix is an empty string, remove the trailing '/' + if not suffix: + url = url.rstrip('/') - # If third-party, return fully-qualified url - if thirdparty: - scheme = "https" if settings.HTTPS == "on" else "http" - url = '{scheme}://{host}{path}'.format( - scheme=scheme, - host=settings.SITE_NAME, - path=url - ) + # If there is a query string, append it + if query: + url += '?' + query - return url + # If third-party, return fully-qualified url + if thirdparty: + scheme = "https" if settings.HTTPS == "on" else "http" + url = '{scheme}://{host}{path}'.format( + scheme=scheme, + host=settings.SITE_NAME, + path=url + ) - def local_resource_url(self, block, uri): - """ - local_resource_url for Studio - """ - path = reverse('xblock_resource_url', kwargs={ - 'block_type': block.scope_ids.block_type, - 'uri': uri, - }) - return '//{}{}'.format(settings.SITE_NAME, path) + return url + + +def local_resource_url(block, uri): + """ + local_resource_url for Studio + """ + path = reverse('xblock_resource_url', kwargs={ + 'block_type': block.scope_ids.block_type, + 'uri': uri, + }) + return '//{}{}'.format(settings.SITE_NAME, path) class LmsPartitionService(PartitionService): @@ -190,7 +194,7 @@ class UserTagsService(object): ) -class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract-method +class LmsModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ ModuleSystem specialized to the LMS """ @@ -210,6 +214,30 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract self.request_token = kwargs.pop('request_token', None) super(LmsModuleSystem, self).__init__(**kwargs) + def handler_url(self, *args, **kwargs): + """ + Implement the XBlock runtime handler_url interface. + + This is mostly just proxying to the module level `handler_url` function + defined higher up in this file. + + We're doing this indirection because the module level `handler_url` + logic is also needed by the `DescriptorSystem`. The particular + `handler_url` that a `DescriptorSystem` needs will be different when + running an LMS process or a CMS/Studio process. That's accomplished by + monkey-patching a global. It's a long story, but please know that you + can't just refactor and fold that logic into here without breaking + things. + + https://openedx.atlassian.net/wiki/display/PLAT/Convert+from+Storage-centric+runtimes+to+Application-centric+runtimes + + See :method:`xblock.runtime:Runtime.handler_url` + """ + return handler_url(*args, **kwargs) + + def local_resource_url(self, *args, **kwargs): + return local_resource_url(*args, **kwargs) + def wrap_aside(self, block, aside, view, frag, context): """ Creates a div which identifies the aside, points to the original block, diff --git a/lms/djangoapps/lms_xblock/test/test_runtime.py b/lms/djangoapps/lms_xblock/test/test_runtime.py index a8fe5a9df2..e844c92069 100644 --- a/lms/djangoapps/lms_xblock/test/test_runtime.py +++ b/lms/djangoapps/lms_xblock/test/test_runtime.py @@ -8,7 +8,7 @@ from ddt import ddt, data from mock import Mock from unittest import TestCase from urlparse import urlparse -from opaque_keys.edx.locations import SlashSeparatedCourseKey +from opaque_keys.edx.locations import BlockUsageLocator, CourseLocator, SlashSeparatedCourseKey from lms.djangoapps.lms_xblock.runtime import quote_slashes, unquote_slashes, LmsModuleSystem from xblock.fields import ScopeIds @@ -39,12 +39,40 @@ class TestQuoteSlashes(TestCase): self.assertNotIn('/', quote_slashes(test_string)) +class BlockMock(Mock): + """Mock class that we fill with our "handler" methods.""" + + def handler(self, _context): + """ + A test handler method. + """ + pass + + def handler1(self, _context): + """ + A test handler method. + """ + pass + + def handler_a(self, _context): + """ + A test handler method. + """ + pass + + @property + def location(self): + """Create a functional BlockUsageLocator for testing URL generation.""" + course_key = CourseLocator(org="mockx", course="100", run="2015") + return BlockUsageLocator(course_key, block_type='mock_type', block_id="mock_id") + + class TestHandlerUrl(TestCase): """Test the LMS handler_url""" def setUp(self): super(TestHandlerUrl, self).setUp() - self.block = Mock(name='block', scope_ids=ScopeIds(None, None, None, 'dummy')) + self.block = BlockMock(name='block', scope_ids=ScopeIds(None, None, None, 'dummy')) self.course_key = SlashSeparatedCourseKey("org", "course", "run") self.runtime = LmsModuleSystem( static_url='/static', diff --git a/lms/startup.py b/lms/startup.py index 9dc2b0d7bb..04ae2f1b89 100644 --- a/lms/startup.py +++ b/lms/startup.py @@ -16,6 +16,9 @@ from monkey_patch import django_utils_translation import analytics +import xmodule.x_module +import lms_xblock.runtime + log = logging.getLogger(__name__) @@ -54,6 +57,13 @@ def run(): set_runtime_service('credit', CreditService()) set_runtime_service('instructor', InstructorService()) + # In order to allow modules to use a handler url, we need to + # monkey-patch the x_module library. + # TODO: Remove this code when Runtimes are no longer created by modulestores + # https://openedx.atlassian.net/wiki/display/PLAT/Convert+from+Storage-centric+runtimes+to+Application-centric+runtimes + xmodule.x_module.descriptor_global_handler_url = lms_xblock.runtime.handler_url + xmodule.x_module.descriptor_global_local_resource_url = lms_xblock.runtime.local_resource_url + def add_mimetypes(): """