diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index b18803649234..6e86a6485b3d 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/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.xblock.field_data import LmsFieldData +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 diff --git a/cms/envs/common.py b/cms/envs/common.py index 3434185209c3..5d72556e5252 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -28,7 +28,7 @@ from lms.envs.common import USE_TZ, TECH_SUPPORT_EMAIL, PLATFORM_NAME, BUGS_EMAIL from path import path -from lms.xblock.mixin import LmsBlockMixin +from lms.lib.xblock.mixin import LmsBlockMixin from cms.xmodule_namespace import CmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin diff --git a/cms/urls.py b/cms/urls.py index 43600bb72cd9..44cef1c646ed 100644 --- a/cms/urls.py +++ b/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[^/]*)/(?P.*?)/(?P[^/]*)$', - 'contentstore.views.preview_dispatch', name='preview_dispatch'), + url(r'^preview/xblock/(?P.*?)/handler/(?P[^/]*)(?:/(?P[^/]*))?$', + 'contentstore.views.preview_handler', name='preview_handler'), url(r'^(?P[^/]+)/(?P[^/]+)/info/(?P[^/]+)$', 'contentstore.views.course_info', name='course_info'), diff --git a/common/djangoapps/student/management/commands/sync_user_info.py b/common/djangoapps/student/management/commands/sync_user_info.py index 04257e2a5da1..d7fdd3323f5a 100644 --- a/common/djangoapps/student/management/commands/sync_user_info.py +++ b/common/djangoapps/student/management/commands/sync_user_info.py @@ -4,7 +4,7 @@ from django.core.management.base import BaseCommand from django.contrib.auth.models import User -import comment_client as cc +import lms.lib.comment_client as cc class Command(BaseCommand): diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 6387f282e153..b4a78d64d27a 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -27,7 +27,7 @@ from django.forms import ModelForm, forms from course_modes.models import CourseMode -import comment_client as cc +import lms.lib.comment_client as cc from pytz import UTC import crum @@ -41,6 +41,7 @@ log = logging.getLogger(__name__) AUDIT_LOG = logging.getLogger("audit") + class UserStanding(models.Model): """ This table contains a student's account's status. diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 50e98ee994a8..6e6d01c28f4d 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/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
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 = { diff --git a/common/lib/capa/capa/tests/__init__.py b/common/lib/capa/capa/tests/__init__.py index 1b93bc61239e..7cc8a85a4a62 100644 --- a/common/lib/capa/capa/tests/__init__.py +++ b/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, diff --git a/common/lib/logsettings.py b/common/lib/logsettings.py index 26ef3c8478bc..aff9f8123759 100644 --- a/common/lib/logsettings.py +++ b/common/lib/logsettings.py @@ -82,7 +82,7 @@ def get_logger_config(log_dir, }, 'newrelic': { 'level': 'ERROR', - 'class': 'newrelic_logging.NewRelicHandler', + 'class': 'lms.lib.newrelic_logging.NewRelicHandler', 'formatter': 'raw', } }, diff --git a/common/lib/symmath/symmath/formula.py b/common/lib/symmath/symmath/formula.py index d5b97a2550aa..b7c4dca2b46d 100644 --- a/common/lib/symmath/symmath/formula.py +++ b/common/lib/symmath/symmath/formula.py @@ -14,6 +14,7 @@ import re import logging import operator +import requests import sympy from sympy.printing.latex import LatexPrinter from sympy.printing.str import StrPrinter @@ -25,11 +26,9 @@ # import sympy.physics.quantum.qubit from xml.sax.saxutils import unescape -import sympy import unicodedata from lxml import etree #import subprocess -import requests from copy import deepcopy log = logging.getLogger(__name__) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index c99234e385d5..c55ff4a34811 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -51,6 +51,7 @@ 'docopt', 'capa', 'path.py', + 'webob', ], package_data={ 'xmodule': ['js/module/*'], diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 27184bb46b73..3b2907f0418b 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/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]) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index d7f8a93067d5..7ed33e567316 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py index d36aa7d62c77..45295fd09f77 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py @@ -213,6 +213,10 @@ 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: @@ -220,9 +224,7 @@ def save_assessment(self, data, _system): 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.") diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 7c056d611ce4..054af321e260 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/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'] diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 2ea9040a1ea1..90c7f4500985 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/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 = "" 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.""" diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 7cbdc130ae4c..889376ba4271 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -8,11 +8,13 @@ #pylint: disable=C0302 import datetime -from mock import Mock, patch import unittest import random import json +from mock import Mock, patch +from webob.multidict import MultiDict + import xmodule from capa.responsetypes import (StudentInputError, LoncapaProblemError, ResponseError) @@ -21,8 +23,6 @@ from xblock.field_data import DictFieldData from xblock.fields import ScopeIds -from django.http import QueryDict - from . import get_test_system from pytz import UTC from capa.correctmap import CorrectMap @@ -133,6 +133,7 @@ def create(graceperiod=None, DictFieldData(field_data), ScopeIds(None, None, location, location), ) + system.xmodule_instance = module if correct: # TODO: probably better to actually set the internal state properly, but... @@ -330,19 +331,16 @@ def test_closed(self): def test_parse_get_params(self): - # We have to set up Django settings in order to use QueryDict - from django.conf import settings - if not settings.configured: - settings.configure() - # Valid GET param dict - valid_get_dict = self._querydict_from_dict({'input_1': 'test', - 'input_1_2': 'test', - 'input_1_2_3': 'test', - 'input_[]_3': 'test', - 'input_4': None, - 'input_5': [], - 'input_6': 5}) + # 'input_5' intentionally left unset, + valid_get_dict = MultiDict({ + 'input_1': 'test', + 'input_1_2': 'test', + 'input_1_2_3': 'test', + 'input_[]_3': 'test', + 'input_4': None, + 'input_6': 5 + }) result = CapaModule.make_dict_of_responses(valid_get_dict) @@ -355,52 +353,31 @@ def test_parse_get_params(self): self.assertEqual(valid_get_dict[original_key], result[key]) # Valid GET param dict with list keys - valid_get_dict = self._querydict_from_dict({'input_2[]': ['test1', 'test2']}) + # Each tuple represents a single parameter in the query string + valid_get_dict = MultiDict((('input_2[]', 'test1'), ('input_2[]', 'test2'))) result = CapaModule.make_dict_of_responses(valid_get_dict) self.assertTrue('2' in result) self.assertEqual(['test1', 'test2'], result['2']) # If we use [] at the end of a key name, we should always # get a list, even if there's just one value - valid_get_dict = self._querydict_from_dict({'input_1[]': 'test'}) + valid_get_dict = MultiDict({'input_1[]': 'test'}) result = CapaModule.make_dict_of_responses(valid_get_dict) self.assertEqual(result['1'], ['test']) # If we have no underscores in the name, then the key is invalid - invalid_get_dict = self._querydict_from_dict({'input': 'test'}) + invalid_get_dict = MultiDict({'input': 'test'}) with self.assertRaises(ValueError): result = CapaModule.make_dict_of_responses(invalid_get_dict) # Two equivalent names (one list, one non-list) # One of the values would overwrite the other, so detect this # and raise an exception - invalid_get_dict = self._querydict_from_dict({'input_1[]': 'test 1', - 'input_1': 'test 2'}) + invalid_get_dict = MultiDict({'input_1[]': 'test 1', + 'input_1': 'test 2'}) with self.assertRaises(ValueError): result = CapaModule.make_dict_of_responses(invalid_get_dict) - def _querydict_from_dict(self, param_dict): - """ - Create a Django QueryDict from a Python dictionary - """ - - # QueryDict objects are immutable by default, so we make - # a copy that we can update. - querydict = QueryDict('') - copyDict = querydict.copy() - - for (key, val) in param_dict.items(): - - # QueryDicts handle lists differently from ordinary values, - # so we have to specifically tell the QueryDict that - # this is a list - if type(val) is list: - copyDict.setlist(key, val) - else: - copyDict[key] = val - - return copyDict - def test_check_problem_correct(self): module = CapaFactory.create(attempts=1) diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index 948ba74cf820..b72eeab66b70 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -6,14 +6,15 @@ """ -from datetime import datetime import json import logging import unittest +from datetime import datetime from lxml import etree from mock import Mock, MagicMock, ANY, patch from pytz import UTC +from webob.multidict import MultiDict from xmodule.open_ended_grading_classes.openendedchild import OpenEndedChild from xmodule.open_ended_grading_classes.open_ended_module import OpenEndedModule @@ -24,7 +25,7 @@ from xmodule.tests import get_test_system, test_util_open_ended from xmodule.progress import Progress from xmodule.tests.test_util_open_ended import ( - MockQueryDict, DummyModulestore, TEST_STATE_SA_IN, + DummyModulestore, TEST_STATE_SA_IN, MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID, TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE, MockUploadedFile ) @@ -646,9 +647,13 @@ def generate_oe_module(self, task_state, task_number, task_xml): """ Return a combined open ended module with the specified parameters """ - definition = {'prompt': etree.XML(self.prompt), 'rubric': etree.XML(self.rubric), - 'task_xml': task_xml} + definition = { + 'prompt': etree.XML(self.prompt), + 'rubric': etree.XML(self.rubric), + 'task_xml': task_xml + } descriptor = Mock(data=definition) + module = Mock(scope_ids=Mock(usage_id='dummy-usage-id')) instance_state = {'task_states': task_state, 'graded': True} if task_number is not None: instance_state.update({'current_task_number': task_number}) @@ -659,6 +664,7 @@ def generate_oe_module(self, task_state, task_number, task_xml): static_data=self.static_data, metadata=self.metadata, instance_state=instance_state) + self.test_system.xmodule_instance = module return combinedoe def ai_state_reset(self, task_state, task_number=None): @@ -764,8 +770,9 @@ def test_open_ended_flow_reset(self): module.save() # Mock a student submitting an assessment - assessment_dict = MockQueryDict() - assessment_dict.update({'assessment': sum(assessment), 'score_list[]': assessment}) + assessment_dict = MultiDict({'assessment': sum(assessment)}) + assessment_dict.extend(('score_list[]', val) for val in assessment) + module.handle_ajax("save_assessment", assessment_dict) module.save() task_one_json = json.loads(module.task_states[0]) @@ -807,8 +814,9 @@ def test_open_ended_flow_correct(self): self.assertIsInstance(status, basestring) # Mock a student submitting an assessment - assessment_dict = MockQueryDict() - assessment_dict.update({'assessment': sum(assessment), 'score_list[]': assessment}) + assessment_dict = MultiDict({'assessment': sum(assessment)}) + assessment_dict.extend(('score_list[]', val) for val in assessment) + module.handle_ajax("save_assessment", assessment_dict) module.save() task_one_json = json.loads(module.task_states[0]) @@ -905,8 +913,9 @@ def test_reset_fail(self): module.save() # Mock a student submitting an assessment - assessment_dict = MockQueryDict() - assessment_dict.update({'assessment': sum(assessment), 'score_list[]': assessment}) + assessment_dict = MultiDict({'assessment': sum(assessment)}) + assessment_dict.extend(('score_list[]', val) for val in assessment) + module.handle_ajax("save_assessment", assessment_dict) module.save() task_one_json = json.loads(module.task_states[0]) diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 8771af3db757..c1b6ccbae75d 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -225,7 +225,8 @@ def replace_urls(text, staticfiles_prefix=None, replace_prefix='/static/', cours html_expect = module.xmodule_runtime.render_template( 'conditional_ajax.html', { - 'ajax_url': 'courses/course_id/modx/a_location', + # Test ajax url is just usage-id / handler_name + 'ajax_url': 'i4x://HarvardX/ER22x/conditional/condone/xmodule_handler', 'element_id': 'i4x-HarvardX-ER22x-conditional-condone', 'id': 'i4x://HarvardX/ER22x/conditional/condone', 'depends': 'i4x-HarvardX-ER22x-problem-choiceprob' diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 33fc264ff996..d73c79035de0 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -143,6 +143,7 @@ def fake_get_module(descriptor): return capa_module system.get_module = fake_get_module module = CrowdsourceHinterModule(descriptor, system, DictFieldData(field_data), Mock()) + system.xmodule_instance = module return module diff --git a/common/lib/xmodule/xmodule/tests/test_peer_grading.py b/common/lib/xmodule/xmodule/tests/test_peer_grading.py index 20000f6f602a..c6d08b2026d2 100644 --- a/common/lib/xmodule/xmodule/tests/test_peer_grading.py +++ b/common/lib/xmodule/xmodule/tests/test_peer_grading.py @@ -2,13 +2,14 @@ import json import logging from mock import Mock +from webob.multidict import MultiDict from xblock.field_data import DictFieldData from xblock.fields import ScopeIds from xmodule.modulestore import Location from xmodule.tests import get_test_system, get_test_descriptor_system -from xmodule.tests.test_util_open_ended import MockQueryDict, DummyModulestore +from xmodule.tests.test_util_open_ended import DummyModulestore from xmodule.open_ended_grading_classes.peer_grading_service import MockPeerGradingService from xmodule.peer_grading_module import PeerGradingModule, PeerGradingDescriptor from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem @@ -29,17 +30,16 @@ class PeerGradingModuleTest(unittest.TestCase, DummyModulestore): coe_location = Location(["i4x", "edX", "open_ended", "combinedopenended", "SampleQuestion"]) calibrated_dict = {'location': "blah"} coe_dict = {'location': coe_location.url()} - save_dict = MockQueryDict() - save_dict.update({ + save_dict = MultiDict({ 'location': "blah", 'submission_id': 1, 'submission_key': "", 'score': 1, 'feedback': "", - 'rubric_scores[]': [0, 1], 'submission_flagged': False, 'answer_unknown': False, }) + save_dict.extend(('rubric_scores[]', val) for val in (0, 1)) def setUp(self): """ @@ -277,6 +277,7 @@ def _create_peer_grading_with_linked_problem(self, location, valid_linked_descri self.field_data, self.scope_ids, ) + self.test_system.xmodule_instance = peer_grading return peer_grading diff --git a/common/lib/xmodule/xmodule/tests/test_self_assessment.py b/common/lib/xmodule/xmodule/tests/test_self_assessment.py index f19edd7d731a..e29658c22cd8 100644 --- a/common/lib/xmodule/xmodule/tests/test_self_assessment.py +++ b/common/lib/xmodule/xmodule/tests/test_self_assessment.py @@ -1,10 +1,10 @@ import json -from mock import Mock, MagicMock import unittest +from mock import Mock, MagicMock +from webob.multidict import MultiDict from xmodule.open_ended_grading_classes.self_assessment_module import SelfAssessmentModule from xmodule.modulestore import Location -from xmodule.tests.test_util_open_ended import MockQueryDict from lxml import etree from . import get_test_system @@ -21,10 +21,11 @@ class SelfAssessmentTest(unittest.TestCase): ''' prompt = etree.XML("This is sample prompt text.") - definition = {'rubric': rubric, - 'prompt': prompt, - 'submitmessage': 'Shall we submit now?', - 'hintprompt': 'Consider this...', + definition = { + 'rubric': rubric, + 'prompt': prompt, + 'submitmessage': 'Shall we submit now?', + 'hintprompt': 'Consider this...', } location = Location(["i4x", "edX", "sa_test", "selfassessment", @@ -33,12 +34,6 @@ class SelfAssessmentTest(unittest.TestCase): descriptor = Mock() def setUp(self): - state = json.dumps({'student_answers': ["Answer 1", "answer 2", "answer 3"], - 'scores': [0, 1], - 'hints': ['o hai'], - 'state': SelfAssessmentModule.INITIAL, - 'attempts': 2}) - self.static_data = { 'max_attempts': 10, 'rubric': etree.XML(self.rubric), @@ -56,13 +51,18 @@ def setUp(self): 'min_to_calibrate': 3, 'max_to_calibrate': 6, 'peer_grade_finished_submissions_when_none_pending': False, - } + } } - self.module = SelfAssessmentModule(get_test_system(), self.location, - self.definition, - self.descriptor, - self.static_data) + system = get_test_system() + system.xmodule_instance = Mock(scope_ids=Mock(usage_id='dummy-usage-id')) + self.module = SelfAssessmentModule( + system, + self.location, + self.definition, + self.descriptor, + self.static_data + ) def test_get_html(self): html = self.module.get_html(self.module.system) @@ -83,7 +83,7 @@ def get_data_for_location(self, location, student): mock_query_dict = MagicMock() mock_query_dict.__getitem__.side_effect = get_fake_item - mock_query_dict.getlist = get_fake_item + mock_query_dict.getall = get_fake_item self.module.peer_gs.get_data_for_location = get_data_for_location @@ -140,8 +140,7 @@ def test_self_assessment_display(self): self.assertEqual(test_module.latest_answer(), submitted_response) # Mock saving an assessment. - assessment = [0] - assessment_dict = MockQueryDict({'assessment': sum(assessment), 'score_list[]': assessment}) + assessment_dict = MultiDict({'assessment': 0, 'score_list[]': 0}) data = test_module.handle_ajax("save_assessment", assessment_dict, get_test_system()) self.assertTrue(json.loads(data)['success']) diff --git a/common/lib/xmodule/xmodule/tests/test_util_open_ended.py b/common/lib/xmodule/xmodule/tests/test_util_open_ended.py index c185d5b25397..939152251abb 100644 --- a/common/lib/xmodule/xmodule/tests/test_util_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_util_open_ended.py @@ -74,20 +74,6 @@ def read(self): return self.mock_file.read() -class MockQueryDict(dict): - """ - Mock a query dict so that it can be used in test classes. This will only work with the combinedopenended tests, - and does not mock the full query dict, only the behavior that is needed there (namely get_list). - """ - def getlist(self, key, default=None): - try: - return super(MockQueryDict, self).__getitem__(key) - except KeyError: - if default is None: - return [] - return default - - class DummyModulestore(object): """ A mixin that allows test classes to have convenience functions to get a module given a location diff --git a/common/lib/xmodule/xmodule/tests/test_word_cloud.py b/common/lib/xmodule/xmodule/tests/test_word_cloud.py index 0db5caceeaad..bc7b542003a5 100644 --- a/common/lib/xmodule/xmodule/tests/test_word_cloud.py +++ b/common/lib/xmodule/xmodule/tests/test_word_cloud.py @@ -1,8 +1,9 @@ # -*- coding: utf-8 -*- """Test for Word cloud Xmodule functional logic.""" +from webob.multidict import MultiDict from xmodule.word_cloud_module import WordCloudDescriptor -from . import PostData, LogicTest +from . import LogicTest class WordCloudModuleTest(LogicTest): @@ -24,7 +25,7 @@ def test_bad_ajax_request(self): def test_good_ajax_request(self): "Make shure that ajax request works correctly" - post_data = PostData({'student_words[]': ['cat', 'cat', 'dog', 'sun']}) + post_data = MultiDict(('student_words[]', word) for word in ['cat', 'cat', 'dog', 'sun']) response = self.ajax_request('submit', post_data) self.assertEqual(response['status'], 'success') self.assertEqual(response['submitted'], True) diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index 58b2fe0ca897..258ee5b03824 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py @@ -5,7 +5,8 @@ # For tests, ignore access to protected members # pylint: disable=protected-access -from nose.tools import assert_equal # pylint: disable=E0611 +import webob +from nose.tools import assert_equal, assert_is_instance # pylint: disable=E0611 from unittest.case import SkipTest from mock import Mock @@ -32,7 +33,7 @@ from xmodule.randomize_module import RandomizeDescriptor from xmodule.vertical_module import VerticalDescriptor from xmodule.wrapper_module import WrapperDescriptor -from xmodule.tests import get_test_descriptor_system, mock_render_template +from xmodule.tests import get_test_descriptor_system, get_test_system LEAF_XMODULES = ( AnnotatableDescriptor, @@ -66,24 +67,12 @@ PollDescriptor ) - class TestXBlockWrapper(object): """Helper methods used in test case classes below.""" @property def leaf_module_runtime(self): - runtime = ModuleSystem( - render_template=mock_render_template, - anonymous_student_id='dummy_anonymous_student_id', - open_ended_grading_interface={}, - static_url='/static', - ajax_url='dummy_ajax_url', - get_module=Mock(), - replace_urls=Mock(), - track_function=Mock(), - error_descriptor_class=ErrorDescriptor, - ) - return runtime + return get_test_system() def leaf_descriptor(self, descriptor_cls): location = 'i4x://org/course/category/name' @@ -258,3 +247,27 @@ def check_studio_view_container_node_xblocks_only(self, descriptor_cls): raise SkipTest(descriptor_cls.__name__ + "is not editable in studio") raise SkipTest("XBlock support in XModules not yet fully implemented") + + +class TestXModuleHandler(TestXBlockWrapper): + """ + Tests that the xmodule_handler function correctly wraps handle_ajax + """ + + def setUp(self): + self.module = XModule(descriptor=Mock(), field_data=Mock(), runtime=Mock(), scope_ids=Mock()) + self.module.handle_ajax = Mock(return_value='{}') + self.request = Mock() + + def test_xmodule_handler_passed_data(self): + self.module.xmodule_handler(self.request) + self.module.handle_ajax.assert_called_with(None, self.request.POST) + + def test_xmodule_handler_dispatch(self): + self.module.xmodule_handler(self.request, 'dispatch') + self.module.handle_ajax.assert_called_with('dispatch', self.request.POST) + + def test_xmodule_handler_return_value(self): + response = self.module.xmodule_handler(self.request) + assert_is_instance(response, webob.Response) + assert_equal(response.body, '{}') diff --git a/common/lib/xmodule/xmodule/word_cloud_module.py b/common/lib/xmodule/xmodule/word_cloud_module.py index fd644c0f1f4a..9595eb6ef9b9 100644 --- a/common/lib/xmodule/xmodule/word_cloud_module.py +++ b/common/lib/xmodule/xmodule/word_cloud_module.py @@ -193,7 +193,7 @@ def handle_ajax(self, dispatch, data): # Student words from client. # FIXME: we must use raw JSON, not a post data (multipart/form-data) - raw_student_words = data.getlist('student_words[]') + raw_student_words = data.getall('student_words[]') student_words = filter(None, map(self.good_word, raw_student_words)) self.student_words = student_words diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index e13d1ec6eba9..055033a3bb0e 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -7,6 +7,7 @@ from lxml import etree from collections import namedtuple from pkg_resources import resource_listdir, resource_string, resource_isdir +from webob import Response from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError @@ -115,7 +116,6 @@ class XModuleMixin(XBlockMixin): # student interacts with the module on the page. A specific example is # FoldIt, which posts grade-changing updates through a separate API. always_recalculate_grades = False - # The default implementation of get_icon_class returns the icon_class # attribute of the class # @@ -273,8 +273,7 @@ def get_score(self): NOTE (vshnayder): not sure if this was the intended return value, but that's what it's doing now. I suspect that we really want it to just - return a number. Would need to change (at least) capa and - modx_dispatch to match if we did that. + return a number. Would need to change (at least) capa to match if we did that. """ return None @@ -402,6 +401,13 @@ def handle_ajax(self, _dispatch, _data): data is a dictionary-like object with the content of the request""" return u"" + def xmodule_handler(self, request, suffix=None): + """ + XBlock handler that wraps `handle_ajax` + """ + response_data = self.handle_ajax(suffix, request.POST) + return Response(response_data, content_type='application/json') + def get_children(self): """ Return module instances for all the children of this module. @@ -762,6 +768,7 @@ def _xmodule(self): max_score = module_attr('max_score') student_view = module_attr('student_view') get_child_descriptors = module_attr('get_child_descriptors') + xmodule_handler = module_attr('xmodule_handler') # ~~~~~~~~~~~~~~~ XBlock API Wrappers ~~~~~~~~~~~~~~~~ def studio_view(self, _context): @@ -924,7 +931,7 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs and user, or other environment-specific info. """ def __init__( - self, static_url, ajax_url, track_function, get_module, render_template, + self, static_url, track_function, get_module, render_template, replace_urls, user=None, filestore=None, debug=False, hostname="", xqueue=None, publish=None, node_path="", anonymous_student_id='', course_id=None, @@ -936,8 +943,6 @@ def __init__( static_url - the base URL to static assets - ajax_url - the url where ajax calls to the encapsulating module go. - track_function - function of (event_type, event), intended for logging or otherwise tracking the event. TODO: Not used, and has inconsistent args in different @@ -988,7 +993,6 @@ def __init__( super(ModuleSystem, self).__init__(usage_store=None, field_data=None, **kwargs) self.STATIC_URL = static_url - self.ajax_url = ajax_url self.xqueue = xqueue self.track_function = track_function self.filestore = filestore @@ -1032,6 +1036,13 @@ def __repr__(self): def __str__(self): return str(self.__dict__) + @property + def ajax_url(self): + """ + The url prefix to be used by XModules to call into handle_ajax + """ + return self.handler_url(self.xmodule_instance, 'xmodule_handler', '', '').rstrip('/?') + class DoNothingCache(object): """A duck-compatible object to use in ModuleSystem when there's no cache.""" diff --git a/common/static/coffee/spec/xblock/runtime.v1_spec.coffee b/common/static/coffee/spec/xblock/runtime.v1_spec.coffee index de84d676161b..266bc7df0eed 100644 --- a/common/static/coffee/spec/xblock/runtime.v1_spec.coffee +++ b/common/static/coffee/spec/xblock/runtime.v1_spec.coffee @@ -1,7 +1,7 @@ describe "XBlock.runtime.v1", -> beforeEach -> setFixtures """ -
+
""" @children = [ {name: 'childA'}, @@ -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/handler/fake-usage-id/foo') + expect(@runtime.handlerUrl('foo')).toBe('/xblock/fake-usage-id/handler/foo') it "provides a list of children", -> expect(@runtime.children).toBe(@children) diff --git a/common/static/coffee/src/xblock/runtime.v1.coffee b/common/static/coffee/src/xblock/runtime.v1.coffee index 0ad6521dd355..ef05c064722b 100644 --- a/common/static/coffee/src/xblock/runtime.v1.coffee +++ b/common/static/coffee/src/xblock/runtime.v1.coffee @@ -5,8 +5,8 @@ return { handlerUrl: (handlerName) -> - usageId = $(element).data("usage-id") - "/xblock/handler/#{usageId}/#{handlerName}" + handlerPrefix = $(element).data("handler-prefix") + "#{handlerPrefix}/#{handlerName}" children: children childMap: childMap } diff --git a/docs/internal/overview.md b/docs/internal/overview.md index c38c61b43ee9..a74c512fbfb5 100644 --- a/docs/internal/overview.md +++ b/docs/internal/overview.md @@ -99,7 +99,7 @@ The LMS is a django site, with root in `lms/`. It runs in many different enviro - calls the module's `.get_html()` method. If the module has nested submodules, render_x_module() will be called again for each. - - ajax calls go to `module_render.py:modx_dispatch()`, which passes it to the module's `handle_ajax()` function, and then updates the grade and state if they changed. + - ajax calls go to `module_render.py:handle_xblock_callback()`, which passes it to one of the `XBlock`s handler functions - [This diagram](https://github.com/MITx/mitx/wiki/MITx-Architecture) visually shows how the clients communicate with problems + modules. diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 863c71f3725c..1539b883d34f 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -20,21 +20,22 @@ from courseware.access import has_access from courseware.masquerade import setup_masquerade from courseware.model_data import FieldDataCache, DjangoKeyValueStore -from lms.xblock.field_data import LmsFieldData +from lms.lib.xblock.field_data import LmsFieldData +from lms.lib.xblock.runtime import LmsModuleSystem, handler_prefix, unquote_slashes from mitxmako.shortcuts import render_to_string from psychometrics.psychoanalyze import make_psychometrics_data_update_handler from student.models import unique_id_for_user from util.json_request import JsonResponse from util.sandboxing import can_execute_unsafe_code from xblock.fields import Scope -from xblock.runtime import DbModel -from xblock.runtime import KeyValueStore +from xblock.runtime import DbModel, KeyValueStore +from xblock.exceptions import NoSuchHandlerError +from xblock.django.request import django_to_webob_request, webob_to_django_response from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.x_module import ModuleSystem from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_histogram, wrap_xblock @@ -220,17 +221,6 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours student_data = DbModel(DjangoKeyValueStore(field_data_cache)) descriptor._field_data = LmsFieldData(descriptor._field_data, student_data) - # Setup system context for module instance - ajax_url = reverse( - 'modx_dispatch', - kwargs=dict( - course_id=course_id, - location=descriptor.location.url(), - dispatch='' - ), - ) - # Intended use is as {ajax_url}/{dispatch_command}, so get rid of the trailing slash. - ajax_url = ajax_url.rstrip('/') def make_xqueue_callback(dispatch='score_update'): # Fully qualified callback URL for external queueing system @@ -338,7 +328,7 @@ def publish(event): # Wrap the output display in a single div to allow for the XModule # javascript to be bound correctly if wrap_xmodule_display is True: - block_wrappers.append(wrap_xblock) + block_wrappers.append(partial(wrap_xblock, partial(handler_prefix, course_id))) # TODO (cpennington): When modules are shared between courses, the static # prefix is going to have to be specific to the module, not the directory @@ -371,11 +361,10 @@ def publish(event): if has_access(user, descriptor, 'staff', course_id): block_wrappers.append(partial(add_histogram, user)) - system = ModuleSystem( + system = LmsModuleSystem( track_function=track_function, render_template=render_to_string, static_url=settings.STATIC_URL, - ajax_url=ajax_url, xqueue=xqueue, # TODO (cpennington): Figure out how to share info between systems filestore=descriptor.runtime.resources_fs, @@ -493,15 +482,13 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch): return HttpResponse("") -def modx_dispatch(request, dispatch, location, course_id): - ''' Generic view for extensions. This is where AJAX calls go. +def handle_xblock_callback(request, course_id, usage_id, handler, suffix=None): + """ + Generic view for extensions. This is where AJAX calls go. Arguments: - request -- the django request. - - dispatch -- the command string to pass through to the module's handle_ajax call - (e.g. 'problem_reset'). If this string contains '?', only pass - through the part before the first '?'. - location -- the module location. Used to look up the XModule instance - course_id -- defines the course context for this request. @@ -509,8 +496,8 @@ def modx_dispatch(request, dispatch, location, course_id): the location and course_id do not identify a valid module, the module is not accessible by the user, or the module raises NotFoundError. If the module raises any other error, it will escape this function. - ''' - # ''' (fix emacs broken parsing) + """ + location = unquote_slashes(usage_id) # Check parameters and fail fast if there's a problem if not Location.is_valid(location): @@ -519,16 +506,11 @@ def modx_dispatch(request, dispatch, location, course_id): if not request.user.is_authenticated(): raise PermissionDenied - # Get the submitted data - data = request.POST.copy() - - # Get and check submitted files + # Check submitted files files = request.FILES or {} error_msg = _check_files_limits(files) if error_msg: return HttpResponse(json.dumps({'success': error_msg})) - for key in files: # Merge files into to data dictionary - data[key] = files.getlist(key) try: descriptor = modulestore().get_instance(course_id, location) @@ -551,14 +533,16 @@ def modx_dispatch(request, dispatch, location, course_id): if instance is None: # Either permissions just changed, or someone is trying to be clever # and load something they shouldn't have access to. - log.debug("No module {0} for user {1}--access denied?".format(location, request.user)) + log.debug("No module %s for user %s -- access denied?", location, request.user) raise Http404 - # Let the module handle the AJAX + req = django_to_webob_request(request) try: - ajax_return = instance.handle_ajax(dispatch, data) - # Save any fields that have 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 # If we can't find the module, respond with a 404 except NotFoundError: @@ -572,12 +556,11 @@ def modx_dispatch(request, dispatch, location, course_id): return JsonResponse(object={'success': err.args[0]}, status=200) # If any other error occurred, re-raise it to trigger a 500 response - except: - log.exception("error processing ajax call") + except Exception: + log.exception("error executing xblock handler") raise - # Return whatever the module wanted to return to the client/caller - return HttpResponse(ajax_return) + return webob_to_django_response(resp) def get_score_bucket(grade, max_grade): diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index 3fcfd4232432..1f8b5f1f15d9 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -21,7 +21,8 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from lms.xblock.field_data import LmsFieldData +from lms.lib.xblock.field_data import LmsFieldData +from lms.lib.xblock.runtime import quote_slashes @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -127,8 +128,8 @@ def setUp(self): def get_url(self, dispatch): """Return item url with dispatch.""" return reverse( - 'modx_dispatch', - args=(self.course.id, self.item_url, dispatch) + 'xblock_handler', + args=(self.course.id, quote_slashes(self.item_url), 'xmodule_handler', dispatch) ) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index d17e7eba4521..6ac27366233f 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -19,6 +19,7 @@ from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.django import modulestore, clear_existing_modulestores +from lms.lib.xblock.runtime import quote_slashes @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -91,10 +92,11 @@ def get_problem(self): pun = 'H1P1' problem_location = "i4x://edX/graded/problem/%s" % pun - modx_url = reverse('modx_dispatch', + modx_url = reverse('xblock_handler', kwargs={'course_id': self.graded_course.id, - 'location': problem_location, - 'dispatch': 'problem_get', }) + 'usage_id': quote_slashes(problem_location), + 'handler': 'xmodule_handler', + 'suffix': 'problem_get'}) resp = self.client.get(modx_url) @@ -115,6 +117,5 @@ def test_no_showanswer_for_student(self): resp = self.get_problem() html = json.loads(resp.content)['html'] - print html sabut = '' self.assertFalse(sabut in html) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index a537e5c29a80..3f4f3b673b2e 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -12,6 +12,7 @@ from django.test.utils import override_settings from xmodule.modulestore.django import modulestore +from xmodule.modulestore import Location from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase import courseware.module_render as render @@ -22,12 +23,16 @@ from courseware.courses import get_course_with_access, course_image_url, get_course_info_section from .factories import UserFactory +from lms.lib.xblock.runtime import quote_slashes @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Tests of courseware.module_render + """ def setUp(self): - self.location = ['i4x', 'edX', 'toy', 'chapter', 'Overview'] + self.location = Location('i4x', 'edX', 'toy', 'chapter', 'Overview') self.course_id = 'edX/toy/2012_Fall' self.toy_course = modulestore().get_course(self.course_id) self.mock_user = UserFactory() @@ -80,59 +85,6 @@ def test_module_render_with_jump_to_id(self): # note if the URL mapping changes then this assertion will break self.assertIn('/courses/' + self.course_id + '/jump_to_id/vertical_test', html) - def test_modx_dispatch(self): - self.assertRaises(Http404, render.modx_dispatch, 'dummy', 'dummy', - 'invalid Location', 'dummy') - mock_request = MagicMock() - mock_request.FILES.keys.return_value = ['file_id'] - mock_request.FILES.getlist.return_value = ['file'] * (settings.MAX_FILEUPLOADS_PER_INPUT + 1) - self.assertEquals(render.modx_dispatch(mock_request, 'dummy', self.location, 'dummy').content, - json.dumps({'success': 'Submission aborted! Maximum %d files may be submitted at once' % - settings.MAX_FILEUPLOADS_PER_INPUT})) - mock_request_2 = MagicMock() - mock_request_2.FILES.keys.return_value = ['file_id'] - inputfile = MagicMock() - inputfile.size = 1 + settings.STUDENT_FILEUPLOAD_MAX_SIZE - inputfile.name = 'name' - filelist = [inputfile] - mock_request_2.FILES.getlist.return_value = filelist - self.assertEquals(render.modx_dispatch(mock_request_2, 'dummy', self.location, - 'dummy').content, - json.dumps({'success': 'Submission aborted! Your file "%s" is too large (max size: %d MB)' % - (inputfile.name, settings.STUDENT_FILEUPLOAD_MAX_SIZE / (1000 ** 2))})) - mock_request_3 = MagicMock() - mock_request_3.POST.copy.return_value = {'position': 1} - mock_request_3.FILES = False - mock_request_3.user = self.mock_user - inputfile_2 = MagicMock() - inputfile_2.size = 1 - inputfile_2.name = 'name' - self.assertIsInstance(render.modx_dispatch(mock_request_3, 'goto_position', - self.location, self.course_id), HttpResponse) - self.assertRaises( - Http404, - render.modx_dispatch, - mock_request_3, - 'goto_position', - self.location, - 'bad_course_id' - ) - self.assertRaises( - Http404, - render.modx_dispatch, - mock_request_3, - 'goto_position', - ['i4x', 'edX', 'toy', 'chapter', 'bad_location'], - self.course_id - ) - self.assertRaises( - Http404, - render.modx_dispatch, - mock_request_3, - 'bad_dispatch', - self.location, - self.course_id - ) def test_xqueue_callback_success(self): """ @@ -182,12 +134,13 @@ def test_get_score_bucket(self): self.assertEquals(render.get_score_bucket(11, 10), 'incorrect') self.assertEquals(render.get_score_bucket(-1, 10), 'incorrect') - def test_anonymous_modx_dispatch(self): + def test_anonymous_handle_xblock_callback(self): dispatch_url = reverse( - 'modx_dispatch', + 'xblock_handler', args=[ 'edX/toy/2012_Fall', - 'i4x://edX/toy/videosequence/Toy_Videos', + quote_slashes('i4x://edX/toy/videosequence/Toy_Videos'), + 'xmodule_handler', 'goto_position' ] ) @@ -195,6 +148,152 @@ def test_anonymous_modx_dispatch(self): self.assertEquals(403, response.status_code) +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class TestHandleXBlockCallback(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test the handle_xblock_callback function + """ + + def setUp(self): + self.location = Location('i4x', 'edX', 'toy', 'chapter', 'Overview') + self.course_id = 'edX/toy/2012_Fall' + self.toy_course = modulestore().get_course(self.course_id) + self.mock_user = UserFactory() + self.mock_user.id = 1 + self.request_factory = RequestFactory() + + # Construct a mock module for the modulestore to return + self.mock_module = MagicMock() + self.mock_module.id = 1 + self.dispatch = 'score_update' + + # Construct a 'standard' xqueue_callback url + self.callback_url = reverse('xqueue_callback', kwargs=dict(course_id=self.course_id, + userid=str(self.mock_user.id), + mod_id=self.mock_module.id, + dispatch=self.dispatch)) + + def _mock_file(self, name='file', size=10): + """Create a mock file object for testing uploads""" + mock_file = MagicMock( + size=size, + read=lambda: 'x' * size + ) + # We can't use `name` as a kwarg to Mock to set the name attribute + # because mock uses `name` to name the mock itself + mock_file.name = name + return mock_file + + def test_invalid_location(self): + with self.assertRaises(Http404): + render.handle_xblock_callback( + None, + 'dummy/course/id', + 'invalid Location', + 'dummy_handler' + 'dummy_dispatch' + ) + + def test_too_many_files(self): + request = self.request_factory.post( + 'dummy_url', + data={'file_id': (self._mock_file(), ) * (settings.MAX_FILEUPLOADS_PER_INPUT + 1)} + ) + request.user = self.mock_user + self.assertEquals( + render.handle_xblock_callback( + request, + 'dummy/course/id', + quote_slashes(str(self.location)), + 'dummy_handler' + ).content, + json.dumps({ + 'success': 'Submission aborted! Maximum %d files may be submitted at once' % + settings.MAX_FILEUPLOADS_PER_INPUT + }) + ) + + def test_too_large_file(self): + inputfile = self._mock_file(size=1 + settings.STUDENT_FILEUPLOAD_MAX_SIZE) + request = self.request_factory.post( + 'dummy_url', + data={'file_id': inputfile} + ) + request.user = self.mock_user + self.assertEquals( + render.handle_xblock_callback( + request, + 'dummy/course/id', + quote_slashes(str(self.location)), + 'dummy_handler' + ).content, + json.dumps({ + 'success': 'Submission aborted! Your file "%s" is too large (max size: %d MB)' % + (inputfile.name, settings.STUDENT_FILEUPLOAD_MAX_SIZE / (1000 ** 2)) + }) + ) + + def test_xmodule_dispatch(self): + request = self.request_factory.post('dummy_url', data={'position': 1}) + request.user = self.mock_user + response = render.handle_xblock_callback( + request, + self.course_id, + quote_slashes(str(self.location)), + 'xmodule_handler', + 'goto_position', + ) + self.assertIsInstance(response, HttpResponse) + + def test_bad_course_id(self): + request = self.request_factory.post('dummy_url') + request.user = self.mock_user + with self.assertRaises(Http404): + render.handle_xblock_callback( + request, + 'bad_course_id', + quote_slashes(str(self.location)), + 'xmodule_handler', + 'goto_position', + ) + + def test_bad_location(self): + request = self.request_factory.post('dummy_url') + request.user = self.mock_user + with self.assertRaises(Http404): + render.handle_xblock_callback( + request, + self.course_id, + quote_slashes(str(Location('i4x', 'edX', 'toy', 'chapter', 'bad_location'))), + 'xmodule_handler', + 'goto_position', + ) + + def test_bad_xmodule_dispatch(self): + request = self.request_factory.post('dummy_url') + request.user = self.mock_user + with self.assertRaises(Http404): + render.handle_xblock_callback( + request, + self.course_id, + quote_slashes(str(self.location)), + 'xmodule_handler', + 'bad_dispatch', + ) + + def test_missing_handler(self): + request = self.request_factory.post('dummy_url') + request.user = self.mock_user + with self.assertRaises(Http404): + render.handle_xblock_callback( + request, + self.course_id, + quote_slashes(str(self.location)), + 'bad_handler', + 'bad_dispatch', + ) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class TestTOC(TestCase): """Check the Table of Contents for a course""" diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index 8857425d9059..0736e08baf3f 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -21,6 +21,7 @@ from capa.tests.response_xml_factory import OptionResponseXMLFactory, CustomResponseXMLFactory, SchematicResponseXMLFactory from courseware.tests.helpers import LoginEnrollmentTestCase from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE +from lms.lib.xblock.runtime import quote_slashes @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -71,11 +72,12 @@ def modx_url(self, problem_location, dispatch): example: 'check_problem' for having responses processed """ return reverse( - 'modx_dispatch', + 'xblock_handler', kwargs={ 'course_id': self.course.id, - 'location': problem_location, - 'dispatch': dispatch, + 'usage_id': quote_slashes(problem_location), + 'handler': 'xmodule_handler', + 'suffix': dispatch, } ) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 1ab22060cea5..a6a265763de8 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -20,7 +20,7 @@ TEST_DATA_MONGO_MODULESTORE, \ TEST_DATA_DRAFT_MONGO_MODULESTORE, \ TEST_DATA_MIXED_MODULESTORE -from lms.xblock.field_data import LmsFieldData +from lms.lib.xblock.field_data import LmsFieldData class ActivateLoginTest(LoginEnrollmentTestCase): diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 473cce917a15..8709d670ebed 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -18,7 +18,7 @@ @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -@patch('comment_client.utils.requests.request') +@patch('lms.lib.comment_client.utils.requests.request') class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): @patch.dict("django.conf.settings.MITX_FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index c6ea414f8226..4600f121f707 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -6,7 +6,7 @@ import urlparse import functools -import comment_client as cc +import lms.lib.comment_client as cc import django_comment_client.utils as utils import django_comment_client.settings as cc_settings @@ -72,7 +72,7 @@ def create_thread(request, course_id, commentable_id): """ Given a course and commentble ID, create the thread """ - + log.debug("Creating new thread in %r, id %r", course_id, commentable_id) course = get_course_with_access(request.user, course_id, 'load') post = request.POST diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index fb53f3c0b785..dac91512d6a0 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -17,7 +17,7 @@ from django_comment_client.permissions import cached_has_permission from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context) import django_comment_client.utils as utils -import comment_client as cc +import lms.lib.comment_client as cc THREADS_PER_PAGE = 20 INLINE_THREADS_PER_PAGE = 20 diff --git a/lms/djangoapps/django_comment_client/management/commands/reload_forum_users.py b/lms/djangoapps/django_comment_client/management/commands/reload_forum_users.py index 53d76cda8fc9..81154da69adc 100644 --- a/lms/djangoapps/django_comment_client/management/commands/reload_forum_users.py +++ b/lms/djangoapps/django_comment_client/management/commands/reload_forum_users.py @@ -4,7 +4,7 @@ from django.core.management.base import BaseCommand from django.contrib.auth.models import User -import comment_client as cc +import lms.lib.comment_client as cc class Command(BaseCommand): diff --git a/lms/djangoapps/django_comment_client/middleware.py b/lms/djangoapps/django_comment_client/middleware.py index 3a4031f871e2..7e2daab8ac24 100644 --- a/lms/djangoapps/django_comment_client/middleware.py +++ b/lms/djangoapps/django_comment_client/middleware.py @@ -1,4 +1,4 @@ -from comment_client import CommentClientRequestError +from lms.lib.comment_client import CommentClientRequestError from django_comment_client.utils import JsonError import json import logging diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index b868d46e3647..5cc4ff18df9e 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -1,7 +1,13 @@ +""" +Module for checking permissions with the comment_client backend +""" + import logging -from util.cache import cache from django.core import cache -cache = cache.get_cache('default') + + +CACHE = cache.get_cache('default') +CACHE_LIFESPAN = 60 def cached_has_permission(user, permission, course_id=None): @@ -9,12 +15,11 @@ def cached_has_permission(user, permission, course_id=None): Call has_permission if it's not cached. A change in a user's role or a role's permissions will only become effective after CACHE_LIFESPAN seconds. """ - CACHE_LIFESPAN = 60 key = "permission_%d_%s_%s" % (user.id, str(course_id), permission) - val = cache.get(key, None) + val = CACHE.get(key, None) if val not in [True, False]: val = has_permission(user, permission, course_id=course_id) - cache.set(key, val, CACHE_LIFESPAN) + CACHE.set(key, val, CACHE_LIFESPAN) return val @@ -72,31 +77,31 @@ def test(user, per, operator="or"): VIEW_PERMISSIONS = { - 'update_thread' : ['edit_content', ['update_thread', 'is_open', 'is_author']], - 'create_comment' : [["create_comment", "is_open"]], - 'delete_thread' : ['delete_thread', ['update_thread', 'is_author']], - 'update_comment' : ['edit_content', ['update_comment', 'is_open', 'is_author']], - 'endorse_comment' : ['endorse_comment'], - 'openclose_thread' : ['openclose_thread'], - 'create_sub_comment': [['create_sub_comment', 'is_open']], - 'delete_comment' : ['delete_comment', ['update_comment', 'is_open', 'is_author']], - 'vote_for_comment' : [['vote', 'is_open']], - 'undo_vote_for_comment': [['unvote', 'is_open']], - 'vote_for_thread' : [['vote', 'is_open']], - 'flag_abuse_for_thread': [['vote', 'is_open']], - 'un_flag_abuse_for_thread': [['vote', 'is_open']], - 'flag_abuse_for_comment': [['vote', 'is_open']], - 'un_flag_abuse_for_comment': [['vote', 'is_open']], - 'undo_vote_for_thread': [['unvote', 'is_open']], - 'pin_thread': ['create_comment'], - 'un_pin_thread': ['create_comment'], - 'follow_thread' : ['follow_thread'], - 'follow_commentable': ['follow_commentable'], - 'follow_user' : ['follow_user'], - 'unfollow_thread' : ['unfollow_thread'], - 'unfollow_commentable': ['unfollow_commentable'], - 'unfollow_user' : ['unfollow_user'], - 'create_thread' : ['create_thread'], + 'update_thread': ['edit_content', ['update_thread', 'is_open', 'is_author']], + 'create_comment': [["create_comment", "is_open"]], + 'delete_thread': ['delete_thread', ['update_thread', 'is_author']], + 'update_comment': ['edit_content', ['update_comment', 'is_open', 'is_author']], + 'endorse_comment': ['endorse_comment'], + 'openclose_thread': ['openclose_thread'], + 'create_sub_comment': [['create_sub_comment', 'is_open']], + 'delete_comment': ['delete_comment', ['update_comment', 'is_open', 'is_author']], + 'vote_for_comment': [['vote', 'is_open']], + 'undo_vote_for_comment': [['unvote', 'is_open']], + 'vote_for_thread': [['vote', 'is_open']], + 'flag_abuse_for_thread': [['vote', 'is_open']], + 'un_flag_abuse_for_thread': [['vote', 'is_open']], + 'flag_abuse_for_comment': [['vote', 'is_open']], + 'un_flag_abuse_for_comment': [['vote', 'is_open']], + 'undo_vote_for_thread': [['unvote', 'is_open']], + 'pin_thread': ['create_comment'], + 'un_pin_thread': ['create_comment'], + 'follow_thread': ['follow_thread'], + 'follow_commentable': ['follow_commentable'], + 'follow_user': ['follow_user'], + 'unfollow_thread': ['unfollow_thread'], + 'unfollow_commentable': ['unfollow_commentable'], + 'unfollow_user': ['unfollow_user'], + 'create_thread': ['create_thread'], 'update_moderator_status': ['manage_moderator'], } diff --git a/lms/xblock/__init__.py b/lms/djangoapps/django_comment_client/tests/__init__.py similarity index 100% rename from lms/xblock/__init__.py rename to lms/djangoapps/django_comment_client/tests/__init__.py diff --git a/lms/djangoapps/django_comment_client/tests/test_middleware.py b/lms/djangoapps/django_comment_client/tests/test_middleware.py index 6b21fe056de0..a7e869fac06c 100644 --- a/lms/djangoapps/django_comment_client/tests/test_middleware.py +++ b/lms/djangoapps/django_comment_client/tests/test_middleware.py @@ -2,7 +2,7 @@ from django.test import TestCase import json -import comment_client +import lms.lib.comment_client import django_comment_client.middleware as middleware @@ -11,9 +11,9 @@ def setUp(self): self.a = middleware.AjaxExceptionMiddleware() self.request1 = django.http.HttpRequest() self.request0 = django.http.HttpRequest() - self.exception1 = comment_client.CommentClientRequestError('{}', 401) - self.exception2 = comment_client.CommentClientRequestError('Foo!', 404) - self.exception0 = comment_client.CommentClient500Error("Holy crap the server broke!") + self.exception1 = lms.lib.comment_client.CommentClientRequestError('{}', 401) + self.exception2 = lms.lib.comment_client.CommentClientRequestError('Foo!', 404) + self.exception0 = lms.lib.comment_client.CommentClient500Error("Holy crap the server broke!") self.request1.META['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest" self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX" diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 4ba950ace613..acc0d4655fe8 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -4,7 +4,7 @@ from django.test.utils import override_settings from student.tests.factories import UserFactory, CourseEnrollmentFactory from django_comment_common.models import Role, Permission -from factories import RoleFactory +from django_comment_client.tests.factories import RoleFactory import django_comment_client.utils as utils from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 401d08b6df36..c7aa4a7c1e4d 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -1,6 +1,7 @@ """ Instructor Dashboard Views """ +from functools import partial from django.utils.translation import ugettext as _ from django_future.csrf import ensure_csrf_cookie @@ -23,6 +24,7 @@ from django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from student.models import CourseEnrollment from bulk_email.models import CourseAuthorization +from lms.lib.xblock.runtime import handler_prefix @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -174,9 +176,13 @@ def _section_data_download(course_id, access): def _section_send_email(course_id, access, course): """ Provide data for the corresponding bulk email section """ - html_module = HtmlDescriptor(course.system, DictFieldData({'data': ''}), ScopeIds(None, None, None, None)) + html_module = HtmlDescriptor( + course.system, + DictFieldData({'data': ''}), + ScopeIds(None, None, None, 'i4x://dummy_org/dummy_course/html/dummy_name') + ) fragment = course.system.render(html_module, 'studio_view') - fragment = wrap_xblock(html_module, 'studio_view', fragment, None) + fragment = wrap_xblock(partial(handler_prefix, course_id), html_module, 'studio_view', fragment, None) email_editor = fragment.content section_data = { 'section_key': 'send_email', diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index e34ce10ec777..78bd9ce9d01b 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -9,6 +9,7 @@ import requests from collections import defaultdict, OrderedDict +from functools import partial from markupsafe import escape from requests.status_codes import codes from StringIO import StringIO @@ -58,6 +59,7 @@ from xblock.field_data import DictFieldData from xblock.fields import ScopeIds from django.utils.translation import ugettext as _u +from lms.lib.xblock.runtime import handler_prefix log = logging.getLogger(__name__) @@ -475,7 +477,7 @@ def domatch(x): except IndexError: log.debug('No grade for assignment %s (%s) for student %s' % (aidx, aname, x.email)) datatable['data'] = ddata - + datatable['title'] = 'Grades for assignment "%s"' % aname if 'Export CSV' in action: @@ -830,9 +832,13 @@ def get_analytics_result(analytics_name): email_editor = None # HTML editor for email if idash_mode == 'Email' and is_studio_course: - html_module = HtmlDescriptor(course.system, DictFieldData({'data': html_message}), ScopeIds(None, None, None, None)) + html_module = HtmlDescriptor( + course.system, + DictFieldData({'data': html_message}), + ScopeIds(None, None, None, 'i4x://dummy_org/dummy_course/html/dummy_name') + ) fragment = html_module.render('studio_view') - fragment = wrap_xblock(html_module, 'studio_view', fragment, None) + fragment = wrap_xblock(partial(handler_prefix, course_id), html_module, 'studio_view', fragment, None) email_editor = fragment.content # Enable instructor email only if the following conditions are met: diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index b36467c16fb0..56cb0211a09b 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -28,6 +28,7 @@ from instructor_task.tests.test_base import (InstructorTaskModuleTestCase, TEST_COURSE_ORG, TEST_COURSE_NUMBER, OPTION_1, OPTION_2) from capa.responsetypes import StudentInputError +from lms.lib.xblock.runtime import quote_slashes log = logging.getLogger(__name__) @@ -57,10 +58,11 @@ def get_input_id(response_id): # on the right problem: self.login_username(username) # make ajax call: - modx_url = reverse('modx_dispatch', + modx_url = reverse('xblock_handler', kwargs={'course_id': self.course.id, - 'location': InstructorTaskModuleTestCase.problem_location(problem_url_name), - 'dispatch': 'problem_check', }) + 'usage_id': quote_slashes(InstructorTaskModuleTestCase.problem_location(problem_url_name)), + 'handler': 'xmodule_handler', + 'suffix': 'problem_check', }) # we assume we have two responses, so assign them the correct identifiers. resp = self.client.post(modx_url, { @@ -110,10 +112,11 @@ def render_problem(self, username, problem_url_name): # on the right problem: self.login_username(username) # make ajax call: - modx_url = reverse('modx_dispatch', + modx_url = reverse('xblock_handler', kwargs={'course_id': self.course.id, - 'location': InstructorTaskModuleTestCase.problem_location(problem_url_name), - 'dispatch': 'problem_get', }) + 'usage_id': quote_slashes(InstructorTaskModuleTestCase.problem_location(problem_url_name)), + 'handler': 'xmodule_handler', + 'suffix': 'problem_get', }) resp = self.client.post(modx_url, {}) return resp diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index c54f316aba3c..c4580dd30436 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -8,7 +8,7 @@ from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService from courseware.access import has_access -from xmodule.x_module import ModuleSystem +from lms.lib.xblock.runtime import LmsModuleSystem from mitxmako.shortcuts import render_to_string from student.models import unique_id_for_user from util.cache import cache @@ -64,8 +64,7 @@ def staff_grading_notifications(course, user): def peer_grading_notifications(course, user): - system = ModuleSystem( - ajax_url=None, + system = LmsModuleSystem( track_function=None, get_module = None, render_template=render_to_string, @@ -124,9 +123,8 @@ def combined_notifications(course, user): return notification_dict #Define a mock modulesystem - system = ModuleSystem( + system = LmsModuleSystem( static_url="/static", - ajax_url=None, track_function=None, get_module = None, render_template=render_to_string, diff --git a/lms/djangoapps/open_ended_grading/staff_grading_service.py b/lms/djangoapps/open_ended_grading/staff_grading_service.py index b0a043b812ae..1322f3f06933 100644 --- a/lms/djangoapps/open_ended_grading/staff_grading_service.py +++ b/lms/djangoapps/open_ended_grading/staff_grading_service.py @@ -9,10 +9,10 @@ from django.http import HttpResponse, Http404 from xmodule.course_module import CourseDescriptor -from xmodule.x_module import ModuleSystem from xmodule.open_ended_grading_classes.grading_service_module import GradingService, GradingServiceError from courseware.access import has_access +from lms.lib.xblock.runtime import LmsModuleSystem from mitxmako.shortcuts import render_to_string from student.models import unique_id_for_user from util.json_request import expect_json @@ -69,9 +69,8 @@ class StaffGradingService(GradingService): """ def __init__(self, config): - config['system'] = ModuleSystem( + config['system'] = LmsModuleSystem( static_url='/static', - ajax_url=None, track_function=None, get_module = None, render_template=render_to_string, diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 62a7c896f389..8e309ee2dee1 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -21,12 +21,12 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.open_ended_grading_classes import peer_grading_service, controller_query_service from xmodule.tests import test_util_open_ended -from xmodule.x_module import ModuleSystem from courseware.access import _course_staff_group_name from courseware.tests import factories from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code, check_for_post_code from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE +from lms.lib.xblock.runtime import LmsModuleSystem from mitxmako.shortcuts import render_to_string from student.models import unique_id_for_user @@ -243,9 +243,8 @@ def setUp(self): location = "i4x://edX/toy/peergrading/init" field_data = DictFieldData({'data': "", 'location': location, 'category':'peergrading'}) self.mock_service = peer_grading_service.MockPeerGradingService() - self.system = ModuleSystem( + self.system = LmsModuleSystem( static_url=settings.STATIC_URL, - ajax_url=location, track_function=None, get_module=None, render_template=render_to_string, @@ -400,7 +399,7 @@ def test_open_ended_panel(self): Mock( return_value=controller_query_service.MockControllerQueryService( settings.OPEN_ENDED_GRADING_INTERFACE, - utils.system + utils.SYSTEM ) ) ) diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index 4e8e971f582b..bfde229e9c85 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -6,11 +6,11 @@ from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.open_ended_grading_classes.controller_query_service import ControllerQueryService from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError -from xmodule.x_module import ModuleSystem from django.utils.translation import ugettext as _ from django.conf import settings +from lms.lib.xblock.runtime import LmsModuleSystem from mitxmako.shortcuts import render_to_string @@ -27,9 +27,8 @@ STUDENT_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify course staff.") STAFF_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify your edX point of contact.") -system = ModuleSystem( +SYSTEM = LmsModuleSystem( static_url='/static', - ajax_url=None, track_function=None, get_module=None, render_template=render_to_string, @@ -79,8 +78,7 @@ def create_controller_query_service(): """ Return an instance of a service that can query edX ORA. """ - - return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) + return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, SYSTEM) class StudentProblemList(object): diff --git a/lms/envs/common.py b/lms/envs/common.py index de148ef3c851..8420e53ab16e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -30,7 +30,7 @@ from .discussionsettings import * -from lms.xblock.mixin import LmsBlockMixin +from lms.lib.xblock.mixin import LmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin @@ -213,9 +213,9 @@ DATA_DIR = COURSES_ROOT +# TODO: Remove the rest of the sys.path modification here and in cms/envs/common.py sys.path.append(REPO_ROOT) sys.path.append(PROJECT_ROOT / 'djangoapps') -sys.path.append(PROJECT_ROOT / 'lib') sys.path.append(COMMON_ROOT / 'djangoapps') sys.path.append(COMMON_ROOT / 'lib') @@ -917,7 +917,7 @@ # Our courseware 'circuit', 'courseware', - 'perfstats', + 'lms.lib.perfstats', 'student', 'static_template_view', 'staticbook', diff --git a/lms/envs/test.py b/lms/envs/test.py index 42a4d8443658..117ccb7f1f50 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -223,7 +223,7 @@ new_staticfiles_dirs.append(static_dir) STATICFILES_DIRS = new_staticfiles_dirs -FILE_UPLOAD_TEMP_DIR = PROJECT_ROOT / "uploads" +FILE_UPLOAD_TEMP_DIR = TEST_ROOT / "uploads" FILE_UPLOAD_HANDLERS = ( 'django.core.files.uploadhandler.MemoryFileUploadHandler', 'django.core.files.uploadhandler.TemporaryFileUploadHandler', diff --git a/lms/lib/__init__.py b/lms/lib/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/lms/lib/comment_client/models.py b/lms/lib/comment_client/models.py index 7296151c80c4..367b9abd10bf 100644 --- a/lms/lib/comment_client/models.py +++ b/lms/lib/comment_client/models.py @@ -1,4 +1,4 @@ -from .utils import * +from .utils import extract, perform_request, CommentClientRequestError class Model(object): diff --git a/lms/lib/xblock/__init__.py b/lms/lib/xblock/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/lms/xblock/field_data.py b/lms/lib/xblock/field_data.py similarity index 92% rename from lms/xblock/field_data.py rename to lms/lib/xblock/field_data.py index 03b3deafac1c..e26897b884e2 100644 --- a/lms/xblock/field_data.py +++ b/lms/lib/xblock/field_data.py @@ -16,7 +16,7 @@ class LmsFieldData(SplitFieldData): def __init__(self, authored_data, student_data): # Make sure that we don't repeatedly nest LmsFieldData instances if isinstance(authored_data, LmsFieldData): - authored_data = authored_data._authored_data + authored_data = authored_data._authored_data # pylint: disable=protected-member else: authored_data = ReadOnlyFieldData(authored_data) diff --git a/lms/xblock/mixin.py b/lms/lib/xblock/mixin.py similarity index 100% rename from lms/xblock/mixin.py rename to lms/lib/xblock/mixin.py diff --git a/lms/lib/xblock/runtime.py b/lms/lib/xblock/runtime.py new file mode 100644 index 000000000000..cecdd4d121da --- /dev/null +++ b/lms/lib/xblock/runtime.py @@ -0,0 +1,99 @@ +""" +Module implementing `xblock.runtime.Runtime` functionality for the LMS +""" + +import re + +from django.core.urlresolvers import reverse + +from xmodule.x_module import ModuleSystem + + +def _quote_slashes(match): + """ + Helper function for `quote_slashes` + """ + matched = match.group(0) + # We have to escape ';', because that is our + # escape sequence identifier (otherwise, the escaping) + # couldn't distinguish between us adding ';_' to the string + # and ';_' appearing naturally in the string + if matched == ';': + return ';;' + elif matched == '/': + return ';_' + else: + return matched + + +def quote_slashes(text): + """ + Quote '/' characters so that they aren't visible to + django's url quoting, unquoting, or url regex matching. + + Escapes '/'' to the sequence ';_', and ';' to the sequence + ';;'. By making the escape sequence fixed length, and escaping + identifier character ';', we are able to reverse the escaping. + """ + return re.sub(r'[;/]', _quote_slashes, text) + + +def _unquote_slashes(match): + """ + Helper function for `unquote_slashes` + """ + matched = match.group(0) + if matched == ';;': + return ';' + elif matched == ';_': + return '/' + else: + return matched + + +def unquote_slashes(text): + """ + Unquote slashes quoted by `quote_slashes` + """ + return re.sub(r'(;;|;_)', _unquote_slashes, text) + + +def handler_url(course_id, block, handler, suffix='', query=''): + """ + Return an xblock handler url for the specified course, block and handler + """ + return reverse('xblock_handler', kwargs={ + 'course_id': course_id, + 'usage_id': quote_slashes(str(block.scope_ids.usage_id)), + 'handler': handler, + 'suffix': suffix, + }) + '?' + query + + +def handler_prefix(course_id, block): + """ + Returns a prefix for use by the javascript handler_url function. + The prefix is a valid handler url the handler name is appended to it. + """ + return handler_url(course_id, block, '').rstrip('/') + + +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 + """ + + def handler_url(self, block, handler_name, suffix='', query=''): # pylint: disable=unused-argument + """See :method:`xblock.runtime:Runtime.handler_url`""" + return handler_url(self.course_id, block, handler_name, suffix='', query='') # pylint: disable=no-member + + +class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract-method + """ + ModuleSystem specialized to the LMS + """ + pass diff --git a/lms/lib/xblock/test/__init__.py b/lms/lib/xblock/test/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/lms/lib/xblock/test/test_runtime.py b/lms/lib/xblock/test/test_runtime.py new file mode 100644 index 000000000000..c8c4d006f791 --- /dev/null +++ b/lms/lib/xblock/test/test_runtime.py @@ -0,0 +1,33 @@ +""" +Tests of the LMS XBlock Runtime and associated utilities +""" + +from ddt import ddt, data +from unittest import TestCase +from lms.lib.xblock.runtime import quote_slashes, unquote_slashes + +TEST_STRINGS = [ + '', + 'foobar', + 'foo/bar', + 'foo/bar;', + 'foo;;bar', + 'foo;_bar', + 'foo/', + '/bar', + 'foo//bar', + 'foo;;;bar', +] + + +@ddt +class TestQuoteSlashes(TestCase): + """Test the quote_slashes and unquote_slashes functions""" + + @data(*TEST_STRINGS) + def test_inverse(self, test_string): + self.assertEquals(test_string, unquote_slashes(quote_slashes(test_string))) + + @data(*TEST_STRINGS) + def test_escaped(self, test_string): + self.assertNotIn('/', quote_slashes(test_string)) diff --git a/lms/urls.py b/lms/urls.py index 7186cfff3ba6..4aec95d01fc1 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -144,7 +144,7 @@ if settings.PERFSTATS: - urlpatterns += (url(r'^reprofile$', 'perfstats.views.end_profile'),) + urlpatterns += (url(r'^reprofile$', 'lms.lib.perfstats.views.end_profile'),) # Multicourse wiki (Note: wiki urls must be above the courseware ones because of # the custom tab catch-all) @@ -175,9 +175,9 @@ 'courseware.views.jump_to', name="jump_to"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/jump_to_id/(?P.*)$', 'courseware.views.jump_to_id', name="jump_to_id"), - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/modx/(?P.*?)/(?P[^/]*)$', - 'courseware.module_render.modx_dispatch', - name='modx_dispatch'), + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/xblock/(?P[^/]*)/handler/(?P[^/]*)(?:/(?P.*))?$', + 'courseware.module_render.handle_xblock_callback', + name='xblock_handler'), # Software Licenses diff --git a/rakelib/js_test.rake b/rakelib/js_test.rake index db79f8027739..d8d5203336c7 100644 --- a/rakelib/js_test.rake +++ b/rakelib/js_test.rake @@ -40,7 +40,7 @@ def js_test_tool(env, command, do_coverage) cmd += " --coverage-xml #{report_dir}" end - sh(cmd) + test_sh(cmd) end # Print a list of js_test commands for diff --git a/rakelib/quality.rake b/rakelib/quality.rake index dd34b0ccf41a..0add71f7274b 100644 --- a/rakelib/quality.rake +++ b/rakelib/quality.rake @@ -1,5 +1,10 @@ def run_pylint(system, report_dir, flags='') - apps = Dir["#{system}", "#{system}/djangoapps/*", "#{system}/lib/*"].map do |app| + apps = Dir["#{system}", "#{system}/djangoapps/*"] + if system != 'lms' + apps += Dir["#{system}/lib/*"] + end + + apps.map do |app| File.basename(app) end.select do |app| app !=~ /.pyc$/ diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index bbf5c8f089b2..a0199378e7f0 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -90,6 +90,7 @@ transifex-client==0.9.1 # Used for testing coverage==3.6 +ddt==0.4.0 factory_boy==2.0.2 mock==1.0.1 nosexcover==1.0.7 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index ee7750133379..c5b23fbaf51a 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -15,7 +15,7 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@74c1a2e9#egg=XBlock +-e git+https://github.com/edx/XBlock.git@2daa4e54#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.2.6#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.1.4#egg=js_test_tool diff --git a/setup.cfg b/setup.cfg index abc86c43fd44..45d3aa27cf10 100644 --- a/setup.cfg +++ b/setup.cfg @@ -7,6 +7,10 @@ with-id=1 exclude-dir=lms/envs cms/envs +# Without this flag, nose adds /lib directories to the path, +# which shadows the xblock library (among other things) +no-path-adjustment=1 + # Uncomment the following lines to open pdb when a test fails #nocapture=1 #pdb=1