Use XBlock handlers for handle_ajax in XModules

Adds xblock handler_url support to the LMS, and makes handle_ajax use
that code.

[LMS-230] [LMS-229]

cms/djangoapps/contentstore/views/preview.py

@@ -1,22 +1,23 @@
 import logging
-import sys
 from functools import partial
 
 from django.conf import settings
-from django.http import HttpResponse, Http404, HttpResponseBadRequest, HttpResponseForbidden
 from django.core.urlresolvers import reverse
+from django.http import Http404, HttpResponseBadRequest, HttpResponseForbidden
 from django.contrib.auth.decorators import login_required
 from mitxmako.shortcuts import render_to_response, render_to_string
 
 from xmodule_modifiers import replace_static_urls, wrap_xblock
 from xmodule.error_module import ErrorDescriptor
-from xmodule.errortracker import exc_info_to_str
 from xmodule.exceptions import NotFoundError, ProcessingError
 from xmodule.modulestore.django import modulestore
 from xmodule.x_module import ModuleSystem
 from xblock.runtime import DbModel
+from xblock.django.request import webob_to_django_response, django_to_webob_request
+from xblock.exceptions import NoSuchHandlerError
 
 from lms.lib.xblock.field_data import LmsFieldData
+from lms.lib.xblock.runtime import quote_slashes, unquote_slashes
 
 from util.sandboxing import can_execute_unsafe_code
 
@@ -26,30 +27,47 @@
 from .access import has_access
 from ..utils import get_course_for_item
 
-__all__ = ['preview_dispatch', 'preview_component']
+__all__ = ['preview_handler', 'preview_component']
 
 log = logging.getLogger(__name__)
 
 
-@login_required
-def preview_dispatch(request, preview_id, location, dispatch=None):
+def handler_prefix(block, handler='', suffix=''):
+    """
+    Return a url prefix for XBlock handler_url. The full handler_url
+    should be '{prefix}/{handler}/{suffix}?{query}'.
+
+    Trailing `/`s are removed from the returned url.
     """
-    Dispatch an AJAX action to a preview XModule
+    return reverse('preview_handler', kwargs={
+        'usage_id': quote_slashes(str(block.scope_ids.usage_id)),
+        'handler': handler,
+        'suffix': suffix,
+    }).rstrip('/?')
 
-    Expects a POST request, and passes the arguments to the module
 
-    preview_id (str): An identifier specifying which preview this module is used for
-    location: The Location of the module to dispatch to
-    dispatch: The action to execute
+@login_required
+def preview_handler(request, usage_id, handler, suffix=''):
     """
+    Dispatch an AJAX action to an xblock
+
+    usage_id: The usage-id of the block to dispatch to, passed through `quote_slashes`
+    handler: The handler to execute
+    suffix: The remaineder of the url to be passed to the handler
+    """
+
+    location = unquote_slashes(usage_id)
 
     descriptor = modulestore().get_item(location)
-    instance = load_preview_module(request, preview_id, descriptor)
+    instance = load_preview_module(request, descriptor)
     # Let the module handle the AJAX
+    req = django_to_webob_request(request)
     try:
-        ajax_return = instance.handle_ajax(dispatch, request.POST)
-        # Save any module data that has changed to the underlying KeyValueStore
-        instance.save()
+        resp = instance.handle(handler, req, suffix)
+
+    except NoSuchHandlerError:
+        log.exception("XBlock %s attempted to access missing handler %r", instance, handler)
+        raise Http404
 
     except NotFoundError:
         log.exception("Module indicating to user that request doesn't exist")
@@ -60,11 +78,11 @@ def preview_dispatch(request, preview_id, location, dispatch=None):
                     exc_info=True)
         return HttpResponseBadRequest()
 
-    except:
+    except Exception:
         log.exception("error processing ajax call")
         raise
 
-    return HttpResponse(ajax_return)
+    return webob_to_django_response(resp)
 
 
 @login_required
@@ -77,7 +95,7 @@ def preview_component(request, location):
     component = modulestore().get_item(location)
     # Wrap the generated fragment in the xmodule_editor div so that the javascript
     # can bind to it correctly
-    component.runtime.wrappers.append(wrap_xblock)
+    component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix))
 
     try:
         content = component.render('studio_view').content
@@ -88,30 +106,36 @@ def preview_component(request, location):
         content = render_to_string('html_error.html', {'message': str(exc)})
 
     return render_to_response('component.html', {
-        'preview': get_preview_html(request, component, 0),
+        'preview': get_preview_html(request, component),
         'editor': content
     })
 
 
-def preview_module_system(request, preview_id, descriptor):
+class PreviewModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
+    """
+    An XModule ModuleSystem for use in Studio previews
+    """
+    def handler_url(self, block, handler_name, suffix='', query=''):
+        return handler_prefix(block, handler_name, suffix) + '?' + query
+
+
+def preview_module_system(request, descriptor):
     """
     Returns a ModuleSystem for the specified descriptor that is specialized for
     rendering module previews.
 
     request: The active django request
-    preview_id (str): An identifier specifying which preview this module is used for
     descriptor: An XModuleDescriptor
     """
 
     course_id = get_course_for_item(descriptor.location).location.course_id
 
-    return ModuleSystem(
+    return PreviewModuleSystem(
         static_url=settings.STATIC_URL,
-        ajax_url=reverse('preview_dispatch', args=[preview_id, descriptor.location.url(), '']).rstrip('/'),
         # TODO (cpennington): Do we want to track how instructors are using the preview problems?
         track_function=lambda event_type, event: None,
         filestore=descriptor.runtime.resources_fs,
-        get_module=partial(load_preview_module, request, preview_id),
+        get_module=partial(load_preview_module, request),
         render_template=render_from_lms,
         debug=True,
         replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id),
@@ -124,7 +148,7 @@ def preview_module_system(request, preview_id, descriptor):
         # Set up functions to modify the fragment produced by student_view
         wrappers=(
             # This wrapper wraps the module in the template specified above
-            partial(wrap_xblock, display_name_only=descriptor.location.category == 'static_tab'),
+            partial(wrap_xblock, handler_prefix, display_name_only=descriptor.location.category == 'static_tab'),
 
             # This wrapper replaces urls in the output that start with /static
             # with the correct course-specific url for the static content
@@ -138,28 +162,27 @@ def preview_module_system(request, preview_id, descriptor):
     )
 
 
-def load_preview_module(request, preview_id, descriptor):
+def load_preview_module(request, descriptor):
     """
     Return a preview XModule instantiated from the supplied descriptor.
 
     request: The active django request
-    preview_id (str): An identifier specifying which preview this module is used for
     descriptor: An XModuleDescriptor
     """
     student_data = DbModel(SessionKeyValueStore(request))
     descriptor.bind_for_student(
-        preview_module_system(request, preview_id, descriptor),
+        preview_module_system(request, descriptor),
         LmsFieldData(descriptor._field_data, student_data),  # pylint: disable=protected-access
     )
     return descriptor
 
 
-def get_preview_html(request, descriptor, idx):
+def get_preview_html(request, descriptor):
     """
     Returns the HTML returned by the XModule's student_view,
     specified by the descriptor and idx.
     """
-    module = load_preview_module(request, str(idx), descriptor)
+    module = load_preview_module(request, descriptor)
     try:
         content = module.render("student_view").content
     except Exception as exc:                          # pylint: disable=W0703

cms/urls.py

@@ -31,8 +31,8 @@
     url(r'^unpublish_unit$', 'contentstore.views.unpublish_unit', name='unpublish_unit'),
     url(r'^reorder_static_tabs', 'contentstore.views.reorder_static_tabs', name='reorder_static_tabs'),
 
-    url(r'^preview/modx/(?P<preview_id>[^/]*)/(?P<location>.*?)/(?P<dispatch>[^/]*)$',
-        'contentstore.views.preview_dispatch', name='preview_dispatch'),
+    url(r'^preview/xblock/(?P<usage_id>.*?)/handler/(?P<handler>[^/]*)(?:/(?P<suffix>[^/]*))?$',
+        'contentstore.views.preview_handler', name='preview_handler'),
 
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/info/(?P<name>[^/]+)$',
         'contentstore.views.course_info', name='course_info'),

common/djangoapps/xmodule_modifiers.py

@@ -29,11 +29,14 @@ def wrap_fragment(fragment, new_content):
     return wrapper_frag
 
 
-def wrap_xblock(block, view, frag, context, display_name_only=False):  # pylint: disable=unused-argument
+def wrap_xblock(handler_prefix, block, view, frag, context, display_name_only=False):  # pylint: disable=unused-argument
     """
     Wraps the results of rendering an XBlock view in a standard <section> with identifying
     data so that the appropriate javascript module can be loaded onto it.
 
+    :param handler_prefix: A function that takes a block and returns the url prefix for
+        the javascript handler_url. This prefix should be able to have {handler_name}/{suffix}?{query}
+        appended to it to return a valid handler_url
     :param block: An XBlock (that may be an XModule or XModuleDescriptor)
     :param view: The name of the view that rendered the fragment being wrapped
     :param frag: The :class:`Fragment` to be wrapped
@@ -63,7 +66,7 @@ def wrap_xblock(block, view, frag, context, display_name_only=False):  # pylint:
     if frag.js_init_fn:
         data['init'] = frag.js_init_fn
         data['runtime-version'] = frag.js_init_version
-        data['usage-id'] = block.scope_ids.usage_id
+        data['handler-prefix'] = handler_prefix(block)
         data['block-type'] = block.scope_ids.block_type
 
     template_context = {

common/lib/capa/capa/tests/__init__.py

@@ -33,9 +33,9 @@ def test_system():
     """
     the_system = Mock(
         spec=ModuleSystem,
+        ajax_url='/dummy-ajax-url',
         STATIC_URL='/dummy-static/',
         DEBUG=True,
-        ajax_url='courses/course_id/modx/a_location',
         track_function=Mock(),
         get_module=Mock(),
         render_template=tst_render_template,

common/lib/xmodule/setup.py

@@ -51,6 +51,7 @@
         'docopt',
         'capa',
         'path.py',
+        'webob',
     ],
     package_data={
         'xmodule': ['js/module/*'],

common/lib/xmodule/xmodule/capa_module.py

@@ -803,7 +803,7 @@ def make_dict_of_responses(data):
         """
         Make dictionary of student responses (aka "answers")
 
-        `data` is POST dictionary (Django QueryDict).
+        `data` is POST dictionary (webob.multidict.MultiDict).
 
         The `data` dict has keys of the form 'x_y', which are mapped
         to key 'y' in the returned dict.  For example,
@@ -835,7 +835,10 @@ def make_dict_of_responses(data):
         """
         answers = dict()
 
-        for key in data:
+        # webob.multidict.MultiDict is a view of a list of tuples,
+        # so it will return a multi-value key once for each value.
+        # We only want to consider each key a single time, so we use set(data.keys())
+        for key in set(data.keys()):
             # e.g. input_resistor_1 ==> resistor_1
             _, _, name = key.partition('_')
 
@@ -857,7 +860,7 @@ def make_dict_of_responses(data):
                 name = name[:-2] if is_list_key or is_dict_key else name
 
                 if is_list_key:
-                    val = data.getlist(key)
+                    val = data.getall(key)
                 elif is_dict_key:
                     try:
                         val = json.loads(data[key])

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -217,9 +217,8 @@ def html_id(self):
         Return a string with a version of the location that is safe for use in
         html id attributes
         """
-        s = "-".join(str(v) for v in self.list()
-                     if v is not None)
-        return Location.clean_for_html(s)
+        id_string = "-".join(str(v) for v in self.list() if v is not None)
+        return Location.clean_for_html(id_string)
 
     def dict(self):
         """

common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py

@@ -213,16 +213,18 @@ def save_assessment(self, data, _system):
 
         with 'error' only present if 'success' is False, and 'hint_html' or
         'message_html' only if success is true
+
+        :param data: A `webob.multidict.MultiDict` containing the keys
+            asasssment: The sum of assessment scores
+            score_list[]: A multivalue key containing all the individual scores
         """
 
         if self.child_state != self.ASSESSING:
             return self.out_of_sync_error(data)
 
         try:
             score = int(data.get('assessment'))
-            score_list = data.getlist('score_list[]')
-            for i in xrange(0, len(score_list)):
-                score_list[i] = int(score_list[i])
+            score_list = [int(x) for x in data.getall('score_list[]')]
         except (ValueError, TypeError):
             # This is a dev_facing_error
             log.error("Non-integer score value passed to save_assessment, or no score list present.")

common/lib/xmodule/xmodule/peer_grading_module.py

@@ -140,9 +140,15 @@ def __init__(self, *args, **kwargs):
         except Exception:
             pass
 
-        self.ajax_url = self.system.ajax_url
-        if not self.ajax_url.endswith("/"):
-            self.ajax_url = self.ajax_url + "/"
+    @property
+    def ajax_url(self):
+        """
+        Returns the `ajax_url` from the system, with any trailing '/' stripped off.
+        """
+        ajax_url = self.system.ajax_url
+        if not ajax_url.endswith("/"):
+            ajax_url += "/"
+        return ajax_url
 
     def closed(self):
         return self._closed(self.timeinfo)
@@ -333,7 +339,7 @@ def save_grade(self, data):
 
         data_dict = {k:data.get(k) for k in required}
         if 'rubric_scores[]' in required:
-            data_dict['rubric_scores'] = data.getlist('rubric_scores[]')
+            data_dict['rubric_scores'] = data.getall('rubric_scores[]')
         data_dict['grader_id'] = self.system.anonymous_student_id
 
         try:
@@ -469,7 +475,7 @@ def save_calibration_essay(self, data):
             return self._err_response(message)
 
         data_dict = {k:data.get(k) for k in required}
-        data_dict['rubric_scores'] = data.getlist('rubric_scores[]')
+        data_dict['rubric_scores'] = data.getall('rubric_scores[]')
         data_dict['student_id'] = self.system.anonymous_student_id
         data_dict['calibration_essay_id'] = data_dict['submission_id']
 

common/lib/xmodule/xmodule/tests/__init__.py

@@ -38,6 +38,14 @@
     }
 
 
+class TestModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
+    """
+    ModuleSystem for testing
+    """
+    def handler_url(self, block, handler, suffix='', query=''):
+        return str(block.scope_ids.usage_id) + '/' + handler + '/' + suffix + '?' + query
+
+
 def get_test_system(course_id=''):
     """
     Construct a test ModuleSystem instance.
@@ -51,9 +59,8 @@ def get_test_system(course_id=''):
     where `my_render_func` is a function of the form my_render_func(template, context).
 
     """
-    return ModuleSystem(
+    return TestModuleSystem(
         static_url='/static',
-        ajax_url='courses/course_id/modx/a_location',
         track_function=Mock(),
         get_module=Mock(),
         render_template=mock_render_template,
@@ -103,15 +110,6 @@ def test_load_class(self):
         vc_str = "<class 'xmodule.video_module.VideoDescriptor'>"
         self.assertEqual(str(vc), vc_str)
 
-class PostData(object):
-    """Class which emulate postdata."""
-    def __init__(self, dict_data):
-        self.dict_data = dict_data
-
-    def getlist(self, key):
-        """Get data by key from `self.dict_data`."""
-        return self.dict_data.get(key)
-
 
 class LogicTest(unittest.TestCase):
     """Base class for testing xmodule logic."""