Merge pull request #1692 from cpennington/pure-xblock-handler-urls

Update handler_url support to match the latest in the xblock repo, for both js and python runtimes

common/static/coffee/spec/xblock/runtime.v1_spec.coffee

@@ -12,7 +12,7 @@ describe "XBlock.runtime.v1", ->
 
     @runtime = XBlock.runtime.v1(@element, @children)
   it "provides a handler url", ->
-    expect(@runtime.handlerUrl('foo')).toBe('/xblock/fake-usage-id/handler/foo')
+    expect(@runtime.handlerUrl(@element, 'foo')).toBe('/xblock/fake-usage-id/handler/foo')
 
   it "provides a list of children", ->
     expect(@runtime.children).toBe(@children)

common/static/coffee/src/xblock/runtime.v1.coffee

@@ -4,9 +4,21 @@
     childMap[child.name] = child
 
   return {
-    handlerUrl: (handlerName) ->
+    # Generate the handler url for the specified handler.
+    #
+    # element is the html element containing the xblock requesting the url
+    # handlerName is the name of the handler
+    # suffix is the optional url suffix to include in the handler url
+    # query is an optional query-string (note, this should not include a preceding ? or &)
+    handlerUrl: (element, handlerName, suffix, query) ->
       handlerPrefix = $(element).data("handler-prefix")
-      "#{handlerPrefix}/#{handlerName}"
+      suffix = if suffix? then "/#{suffix}" else ''
+      query = if query? then "?#{query}" else ''
+      "#{handlerPrefix}/#{handlerName}#{suffix}#{query}"
+
+    # A list of xblock children of this element
     children: children
+
+    # A map of name -> child for the xblock children of this element
     childMap: childMap
   }

lms/lib/xblock/runtime.py

@@ -82,12 +82,22 @@ def handler_url(course_id, block, handler, suffix='', query='', thirdparty=False
     if thirdparty:
         view_name = 'xblock_handler_noauth'
 
-    return reverse(view_name, kwargs={
+    url = reverse(view_name, kwargs={
         'course_id': course_id,
         'usage_id': quote_slashes(str(block.scope_ids.usage_id)),
         'handler': handler,
         'suffix': suffix,
-    }) + '?' + query
+    })
+
+    # 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
+
+    return url
 
 
 def handler_prefix(course_id, block):
@@ -96,9 +106,13 @@ def handler_prefix(course_id, block):
 
     The prefix is a valid handler url after the handler name is slash-appended
     to it.
-
     """
-    return handler_url(course_id, block, '').rstrip('/')
+    # This depends on handler url having the handler_name as the final piece of the url
+    # so that leaving an empty handler_name really does leave the opportunity to append
+    # the handler_name on the frontend
+
+    # This is relied on by the xblock/runtime.v1.coffee frontend handlerUrl function
+    return handler_url(course_id, block, '').rstrip('/?')
 
 
 class LmsHandlerUrls(object):

lms/lib/xblock/test/test_runtime.py

@@ -3,8 +3,10 @@ Tests of the LMS XBlock Runtime and associated utilities
 """
 
 from ddt import ddt, data
+from mock import Mock
 from unittest import TestCase
-from lms.lib.xblock.runtime import quote_slashes, unquote_slashes
+from urlparse import urlparse
+from lms.lib.xblock.runtime import quote_slashes, unquote_slashes, handler_url
 
 TEST_STRINGS = [
     '',
@@ -31,3 +33,46 @@ class TestQuoteSlashes(TestCase):
     @data(*TEST_STRINGS)
     def test_escaped(self, test_string):
         self.assertNotIn('/', quote_slashes(test_string))
+
+
+class TestHandlerUrl(TestCase):
+    """Test the LMS handler_url"""
+
+    def setUp(self):
+        self.block = Mock()
+        self.course_id = "org/course/run"
+
+    def test_trailing_characters(self):
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler').endswith('?'))
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler').endswith('/'))
+
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix').endswith('?'))
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix').endswith('/'))
+
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix', 'query').endswith('?'))
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', 'suffix', 'query').endswith('/'))
+
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', query='query').endswith('?'))
+        self.assertFalse(handler_url(self.course_id, self.block, 'handler', query='query').endswith('/'))
+
+    def _parsed_query(self, query_string):
+        """Return the parsed query string from a handler_url generated with the supplied query_string"""
+        return urlparse(handler_url(self.course_id, self.block, 'handler', query=query_string)).query
+
+    def test_query_string(self):
+        self.assertIn('foo=bar', self._parsed_query('foo=bar'))
+        self.assertIn('foo=bar&baz=true', self._parsed_query('foo=bar&baz=true'))
+        self.assertIn('foo&bar&baz', self._parsed_query('foo&bar&baz'))
+
+    def _parsed_path(self, handler_name='handler', suffix=''):
+        """Return the parsed path from a handler_url with the supplied handler_name and suffix"""
+        return urlparse(handler_url(self.course_id, self.block, handler_name, suffix=suffix)).path
+
+    def test_suffix(self):
+        self.assertTrue(self._parsed_path(suffix="foo").endswith('foo'))
+        self.assertTrue(self._parsed_path(suffix="foo/bar").endswith('foo/bar'))
+        self.assertTrue(self._parsed_path(suffix="/foo/bar").endswith('/foo/bar'))
+
+    def test_handler_name(self):
+        self.assertIn('handler1', self._parsed_path('handler1'))
+        self.assertIn('handler_a', self._parsed_path('handler_a'))

rakelib/tests.rake

@@ -21,13 +21,20 @@ def run_tests(system, report_dir, test_id=nil, stop_on_failure=true)
     # If no test id is provided, we need to limit the test runner
     # to the Djangoapps we want to test.  Otherwise, it will
     # run tests on all installed packages.
+
+    default_test_id = "#{system}/djangoapps common/djangoapps"
+
+    if system == :lms
+        default_test_id += " #{system}/lib"
+    end
+
     if test_id.nil?
-        test_id = "#{system}/djangoapps common/djangoapps"
+        test_id = default_test_id
 
     # Handle "--failed" as a special case: we want to re-run only
     # the tests that failed within our Django apps
     elsif test_id == '--failed'
-        test_id = "#{system}/djangoapps common/djangoapps --failed"
+        test_id = "#{default_test_id} --failed"
     end
 
     cmd = django_admin(system, :test, 'test', test_id)