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

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