Merge pull request #10389 from edx/mobile/handler-url

Changes to handler URL generation

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

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

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:

lms/djangoapps/lms_xblock/runtime.py

@@ -66,64 +66,68 @@ def unquote_slashes(text):
     return re.sub(r'(;;|;_)', _unquote_slashes, text)
 
 
-class LmsHandlerUrls(object):
-    """
-    A runtime mixin that provides a handler_url function that routes
-    to the LMS' xblock handler view.
-
-    This must be mixed in to a runtime that already accepts and stores
-    a course_id
-    """
-    # 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))
-
-        if thirdparty:
-            view_name = 'xblock_handler_noauth'
-
-        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 suffix is an empty string, remove the trailing '/'
-        if not suffix:
-            url = url.rstrip('/')
-
-        # If there is a query string, append it
-        if query:
-            url += '?' + query
-
-        # 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
-            )
-
-        return 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)
+def handler_url(block, handler_name, suffix='', query='', thirdparty=False):
+    """
+    This method matches the signature for `xblock.runtime:Runtime.handler_url()`
+
+    See :method:`xblock.runtime:Runtime.handler_url`
+    """
+    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))
+
+        # 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))
+
+    if thirdparty:
+        view_name = 'xblock_handler_noauth'
+
+    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 suffix is an empty string, remove the trailing '/'
+    if not suffix:
+        url = url.rstrip('/')
+
+    # If there is a query string, append it
+    if query:
+        url += '?' + query
+
+    # 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
+        )
+
+    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,

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',

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