From 9b88bdb072221647af8daf94fecad39f9b2b7416 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 27 Oct 2015 21:35:51 -0400 Subject: [PATCH] Changes to handler URL generation * The LMS now also monkey-patches xmodule.x_module.descriptor_global_handler_url and xmodule.x_module.descriptor_global_local_resource_url so that we can get LMS XBlock URLs from the DescriptorSystem. That functionality is needed in the block transforms collect() phase for certain XModules like Video. For instance, say we want to generate the transcripts URLs. The collect phase is run asynchronously, without a user context. * The URL handler monkey-patching is now done in the startup.py files for LMS and Studio. Studio used to do this in the import of cms/djangoapps/contentstore/views/item.py. This was mostly just because it seemed like a sane and consistent place to put it. * LmsHandlerUrls was removed, its handler_url and local_resource_url methods were moved to be top level functions. The only reason that class existed seems to be to give a place to store course_id state, and that can now be derived from the block location. * To avoid the Module -> Descriptor ProxyAttribute magic that we do (which explodes with an UndefinedContext error because there is no user involved), when examining the block's handler method in handler_url, I made a few changes: ** Check the .__class__ to see if the handler was defined, instead of the block itself. ** The above required me to relax the check for _is_xblock_handler on the function, since that will no longer be defined. 90% of this goes away when we kill XModules and do the refactoring we've wanted to do for a while. --- cms/djangoapps/contentstore/views/item.py | 7 - cms/startup.py | 10 ++ common/lib/xmodule/xmodule/x_module.py | 9 +- lms/djangoapps/lms_xblock/runtime.py | 126 +++++++++++------- .../lms_xblock/test/test_runtime.py | 32 ++++- lms/startup.py | 10 ++ 6 files changed, 134 insertions(+), 60 deletions(-) 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(): """