Merge pull request #1743 from MITx/fix/will/bug_288

Fix/will/bug 288

cms/djangoapps/contentstore/views.py

@@ -42,7 +42,7 @@ from xmodule.modulestore.mongo import MongoUsage
 from mitxmako.shortcuts import render_to_response, render_to_string
 from xmodule.modulestore.django import modulestore
 from xmodule_modifiers import replace_static_urls, wrap_xmodule
-from xmodule.exceptions import NotFoundError
+from xmodule.exceptions import NotFoundError, ProcessingError
 from functools import partial
 
 from xmodule.contentstore.django import contentstore
@@ -439,9 +439,16 @@ def preview_dispatch(request, preview_id, location, dispatch=None):
     # Let the module handle the AJAX
     try:
         ajax_return = instance.handle_ajax(dispatch, request.POST)
+
     except NotFoundError:
         log.exception("Module indicating to user that request doesn't exist")
         raise Http404
+
+    except ProcessingError:
+        log.warning("Module raised an error while processing AJAX request",
+                    exc_info=True)
+        return HttpResponseBadRequest()
+
     except:
         log.exception("error processing ajax call")
         raise

common/lib/capa/capa/responsetypes.py

@@ -17,6 +17,7 @@ import logging
 import numbers
 import numpy
 import os
+import sys
 import random
 import re
 import requests
@@ -52,12 +53,17 @@ class LoncapaProblemError(Exception):
 
 class ResponseError(Exception):
     '''
-    Error for failure in processing a response
+    Error for failure in processing a response, including
+    exceptions that occur when executing a custom script.
     '''
     pass
 
 
 class StudentInputError(Exception):
+    '''
+    Error for an invalid student input.
+    For example, submitting a string when the problem expects a number
+    '''
     pass
 
 #-----------------------------------------------------------------------------
@@ -833,7 +839,7 @@ class NumericalResponse(LoncapaResponse):
             import sys
             type, value, traceback = sys.exc_info()
 
-            raise StudentInputError, ("Invalid input: could not interpret '%s' as a number" %
+            raise StudentInputError, ("Could not interpret '%s' as a number" %
                                       cgi.escape(student_answer)), traceback
 
         if correct:
@@ -1072,13 +1078,10 @@ def sympy_check2():
                 correct = self.context['correct']
                 messages = self.context['messages']
                 overall_message = self.context['overall_message']
+
             except Exception as err:
-                print "oops in customresponse (code) error %s" % err
-                print "context = ", self.context
-                print traceback.format_exc()
-                # Notify student
-                raise StudentInputError(
-                    "Error: Problem could not be evaluated with your input")
+                self._handle_exec_exception(err)
+
         else:
             # self.code is not a string; assume its a function
 
@@ -1105,13 +1108,9 @@ def sympy_check2():
                     nargs, args, kwargs))
 
                 ret = fn(*args[:nargs], **kwargs)
+
             except Exception as err:
-                log.error("oops in customresponse (cfn) error %s" % err)
-                # print "context = ",self.context
-                log.error(traceback.format_exc())
-                raise Exception("oops in customresponse (cfn) error %s" % err)
-            log.debug(
-                "[courseware.capa.responsetypes.customresponse.get_score] ret = %s" % ret)
+                self._handle_exec_exception(err)
 
             if type(ret) == dict:
 
@@ -1147,9 +1146,9 @@ def sympy_check2():
                     correct = []
                     messages = []
                     for input_dict in input_list:
-                        correct.append('correct' 
+                        correct.append('correct'
                                 if input_dict['ok'] else 'incorrect')
-                        msg = (self.clean_message_html(input_dict['msg']) 
+                        msg = (self.clean_message_html(input_dict['msg'])
                                 if 'msg' in input_dict else None)
                         messages.append(msg)
 
@@ -1157,7 +1156,7 @@ def sympy_check2():
                 # Raise an exception
                 else:
                     log.error(traceback.format_exc())
-                    raise Exception(
+                    raise ResponseError(
                         "CustomResponse: check function returned an invalid dict")
 
             # The check function can return a boolean value,
@@ -1174,7 +1173,7 @@ def sympy_check2():
         correct_map.set_overall_message(overall_message)
 
         for k in range(len(idset)):
-            npoints = (self.maxpoints[idset[k]] 
+            npoints = (self.maxpoints[idset[k]]
                     if correct[k] == 'correct' else 0)
             correct_map.set(idset[k], correct[k], msg=messages[k],
                             npoints=npoints)
@@ -1227,6 +1226,22 @@ def sympy_check2():
             return {self.answer_ids[0]: self.expect}
         return self.default_answer_map
 
+    def _handle_exec_exception(self, err):
+        '''
+        Handle an exception raised during the execution of
+        custom Python code.
+
+        Raises a ResponseError
+        '''
+
+        # Log the error if we are debugging
+        msg = 'Error occurred while evaluating CustomResponse'
+        log.warning(msg, exc_info=True)
+
+        # Notify student with a student input error
+        _, _, traceback_obj = sys.exc_info()
+        raise ResponseError, err.message, traceback_obj
+
 #-----------------------------------------------------------------------------
 
 
@@ -1901,7 +1916,14 @@ class SchematicResponse(LoncapaResponse):
         submission = [json.loads(student_answers[
                                  k]) for k in sorted(self.answer_ids)]
         self.context.update({'submission': submission})
-        exec self.code in global_context, self.context
+
+        try:
+            exec self.code in global_context, self.context
+
+        except Exception as err:
+            _, _, traceback_obj = sys.exc_info()
+            raise ResponseError, ResponseError(err.message), traceback_obj
+
         cmap = CorrectMap()
         cmap.set_dict(dict(zip(sorted(
             self.answer_ids), self.context['correct'])))
@@ -2106,7 +2128,7 @@ class AnnotationResponse(LoncapaResponse):
             option_scoring = dict([(option['id'], {
                     'correctness': choices.get(option['choice']),
                     'points': scoring.get(option['choice'])
-                }) for option in self._find_options(inputfield) ])
+                }) for option in self._find_options(inputfield)])
 
             scoring_map[inputfield.get('id')] = option_scoring
 

common/lib/capa/capa/tests/test_responsetypes.py

@@ -13,6 +13,8 @@ import textwrap
 from . import test_system
 
 import capa.capa_problem as lcp
+from capa.responsetypes import LoncapaProblemError, \
+                            StudentInputError, ResponseError
 from capa.correctmap import CorrectMap
 from capa.util import convert_files_to_filenames
 from capa.xqueue_interface import dateformat
@@ -864,7 +866,7 @@ class CustomResponseTest(ResponseTest):
         # Message is interpreted as an "overall message"
         self.assertEqual(correct_map.get_overall_message(), 'Message text')
 
-    def test_script_exception(self):
+    def test_script_exception_function(self):
 
         # Construct a script that will raise an exception
         script = textwrap.dedent("""
@@ -875,7 +877,17 @@ class CustomResponseTest(ResponseTest):
         problem = self.build_problem(script=script, cfn="check_func")
 
         # Expect that an exception gets raised when we check the answer
-        with self.assertRaises(Exception):
+        with self.assertRaises(ResponseError):
+            problem.grade_answers({'1_2_1': '42'})
+
+    def test_script_exception_inline(self):
+
+        # Construct a script that will raise an exception
+        script = 'raise Exception("Test")'
+        problem = self.build_problem(answer=script)
+
+        # Expect that an exception gets raised when we check the answer
+        with self.assertRaises(ResponseError):
             problem.grade_answers({'1_2_1': '42'})
 
     def test_invalid_dict_exception(self):
@@ -889,7 +901,7 @@ class CustomResponseTest(ResponseTest):
         problem = self.build_problem(script=script, cfn="check_func")
 
         # Expect that an exception gets raised when we check the answer
-        with self.assertRaises(Exception):
+        with self.assertRaises(ResponseError):
             problem.grade_answers({'1_2_1': '42'})
 
 
@@ -922,6 +934,18 @@ class SchematicResponseTest(ResponseTest):
         # is what we expect)
         self.assertEqual(correct_map.get_correctness('1_2_1'), 'correct')
 
+    def test_script_exception(self):
+
+        # Construct a script that will raise an exception
+        script = "raise Exception('test')"
+        problem = self.build_problem(answer=script)
+
+        # Expect that an exception gets raised when we check the answer
+        with self.assertRaises(ResponseError):
+            submission_dict = {'test': 'test'}
+            input_dict = {'1_2_1': json.dumps(submission_dict)}
+            problem.grade_answers(input_dict)
+
 
 class AnnotationResponseTest(ResponseTest):
     from response_xml_factory import AnnotationResponseXMLFactory

common/lib/xmodule/xmodule/capa_module.py

@@ -12,12 +12,13 @@ from lxml import etree
 from pkg_resources import resource_string
 
 from capa.capa_problem import LoncapaProblem
-from capa.responsetypes import StudentInputError
+from capa.responsetypes import StudentInputError, \
+                                ResponseError, LoncapaProblemError
 from capa.util import convert_files_to_filenames
 from .progress import Progress
 from xmodule.x_module import XModule
 from xmodule.raw_module import RawDescriptor
-from xmodule.exceptions import NotFoundError
+from xmodule.exceptions import NotFoundError, ProcessingError
 from xblock.core import Integer, Scope, BlockScope, ModelType, String, Boolean, Object, Float
 from .fields import Timedelta
 
@@ -454,7 +455,14 @@ class CapaModule(CapaFields, XModule):
             return 'Error'
 
         before = self.get_progress()
-        d = handlers[dispatch](get)
+
+        try:
+            d = handlers[dispatch](get)
+
+        except Exception as err:
+            _, _, traceback_obj = sys.exc_info()
+            raise ProcessingError, err.message, traceback_obj
+
         after = self.get_progress()
         d.update({
             'progress_changed': after != before,
@@ -576,7 +584,7 @@ class CapaModule(CapaFields, XModule):
         # save any state changes that may occur
         self.set_state_from_lcp()
         return response
-        
+
 
     def get_answer(self, get):
         '''
@@ -725,9 +733,24 @@ class CapaModule(CapaFields, XModule):
         try:
             correct_map = self.lcp.grade_answers(answers)
             self.set_state_from_lcp()
-        except StudentInputError as inst:
-            log.exception("StudentInputError in capa_module:problem_check")
-            return {'success': inst.message}
+
+        except (StudentInputError, ResponseError, LoncapaProblemError) as inst:
+            log.warning("StudentInputError in capa_module:problem_check",
+                        exc_info=True)
+
+            # If the user is a staff member, include
+            # the full exception, including traceback,
+            # in the response
+            if self.system.user_is_staff:
+                msg = "Staff debug info: %s" % traceback.format_exc()
+
+            # Otherwise, display just an error message,
+            # without a stack trace
+            else:
+                msg = "Error: %s" % str(inst.message)
+
+            return {'success': msg}
+
         except Exception, err:
             if self.system.DEBUG:
                 msg = "Error checking problem: " + str(err)
@@ -778,7 +801,7 @@ class CapaModule(CapaFields, XModule):
         event_info['answers'] = answers
 
         # Too late. Cannot submit
-        if self.closed() and not self.max_attempts ==0:
+        if self.closed() and not self.max_attempts == 0:
             event_info['failure'] = 'closed'
             self.system.track_function('save_problem_fail', event_info)
             return {'success': False,
@@ -798,7 +821,7 @@ class CapaModule(CapaFields, XModule):
 
         self.system.track_function('save_problem_success', event_info)
         msg = "Your answers have been saved"
-        if not self.max_attempts ==0:
+        if not self.max_attempts == 0:
             msg += " but not graded. Hit 'Check' to grade them."
         return {'success': True,
                 'msg': msg}

common/lib/xmodule/xmodule/exceptions.py

 class InvalidDefinitionError(Exception):
     pass
 
-
 class NotFoundError(Exception):
     pass
+
+class ProcessingError(Exception):
+    '''
+    An error occurred while processing a request to the XModule.
+    For example: if an exception occurs while checking a capa problem.
+    '''
+    pass

common/lib/xmodule/xmodule/tests/test_capa_module.py

@@ -7,6 +7,8 @@ import random
 
 import xmodule
 import capa
+from capa.responsetypes import StudentInputError, \
+                                LoncapaProblemError, ResponseError
 from xmodule.capa_module import CapaModule
 from xmodule.modulestore import Location
 from lxml import etree
@@ -407,7 +409,7 @@ class CapaModuleTest(unittest.TestCase):
             mock_html.return_value = "Test HTML"
 
             # Check the problem
-            get_request_dict = { CapaFactory.input_key(): '3.14'}
+            get_request_dict = {CapaFactory.input_key(): '3.14'}
             result = module.check_problem(get_request_dict)
 
         # Expect that the problem is marked correct
@@ -428,7 +430,7 @@ class CapaModuleTest(unittest.TestCase):
             mock_is_correct.return_value = False
 
             # Check the problem
-            get_request_dict = { CapaFactory.input_key(): '0'}
+            get_request_dict = {CapaFactory.input_key(): '0'}
             result = module.check_problem(get_request_dict)
 
         # Expect that the problem is marked correct
@@ -446,7 +448,7 @@ class CapaModuleTest(unittest.TestCase):
         with patch('xmodule.capa_module.CapaModule.closed') as mock_closed:
             mock_closed.return_value = True
             with self.assertRaises(xmodule.exceptions.NotFoundError):
-                get_request_dict = { CapaFactory.input_key(): '3.14'}
+                get_request_dict = {CapaFactory.input_key(): '3.14'}
                 module.check_problem(get_request_dict)
 
         # Expect that number of attempts NOT incremented
@@ -492,7 +494,7 @@ class CapaModuleTest(unittest.TestCase):
             mock_is_queued.return_value = True
             mock_get_queuetime.return_value = datetime.datetime.now()
 
-            get_request_dict = { CapaFactory.input_key(): '3.14'}
+            get_request_dict = {CapaFactory.input_key(): '3.14'}
             result = module.check_problem(get_request_dict)
 
             # Expect an AJAX alert message in 'success'
@@ -502,21 +504,61 @@ class CapaModuleTest(unittest.TestCase):
         self.assertEqual(module.attempts, 1)
 
 
-    def test_check_problem_student_input_error(self):
-        module = CapaFactory.create(attempts=1)
+    def test_check_problem_error(self):
 
-        # Simulate a student input exception
-        with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade:
-            mock_grade.side_effect = capa.responsetypes.StudentInputError('test error')
+        # Try each exception that capa_module should handle
+        for exception_class in [StudentInputError, 
+                                LoncapaProblemError, 
+                                ResponseError]:
 
-            get_request_dict = { CapaFactory.input_key(): '3.14'}
-            result = module.check_problem(get_request_dict)
+            # Create the module
+            module = CapaFactory.create(attempts=1)
+
+            # Ensure that the user is NOT staff
+            module.system.user_is_staff = False
+
+            # Simulate answering a problem that raises the exception
+            with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade:
+                mock_grade.side_effect = exception_class('test error')
+
+                get_request_dict = {CapaFactory.input_key(): '3.14'}
+                result = module.check_problem(get_request_dict)
+
+            # Expect an AJAX alert message in 'success'
+            expected_msg = 'Error: test error'
+            self.assertEqual(expected_msg, result['success'])
+
+            # Expect that the number of attempts is NOT incremented
+            self.assertEqual(module.attempts, 1)
+
+    def test_check_problem_error_with_staff_user(self):
+        
+        # Try each exception that capa module should handle
+        for exception_class in [StudentInputError, 
+                                LoncapaProblemError,
+                                ResponseError]:
+
+            # Create the module
+            module = CapaFactory.create(attempts=1)
+
+            # Ensure that the user IS staff
+            module.system.user_is_staff = True
+
+            # Simulate answering a problem that raises an exception
+            with patch('capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade:
+                mock_grade.side_effect = exception_class('test error')
+
+                get_request_dict = {CapaFactory.input_key(): '3.14'}
+                result = module.check_problem(get_request_dict)
 
             # Expect an AJAX alert message in 'success'
             self.assertTrue('test error' in result['success'])
 
-        # Expect that the number of attempts is NOT incremented
-        self.assertEqual(module.attempts, 1)
+            # We DO include traceback information for staff users
+            self.assertTrue('Traceback' in result['success'])
+
+            # Expect that the number of attempts is NOT incremented
+            self.assertEqual(module.attempts, 1)
 
 
     def test_reset_problem(self):
@@ -573,11 +615,11 @@ class CapaModuleTest(unittest.TestCase):
         module = CapaFactory.create(done=False)
 
         # Save the problem
-        get_request_dict = { CapaFactory.input_key(): '3.14'}
+        get_request_dict = {CapaFactory.input_key(): '3.14'}
         result = module.save_problem(get_request_dict)
 
         # Expect that answers are saved to the problem
-        expected_answers = { CapaFactory.answer_key(): '3.14'}
+        expected_answers = {CapaFactory.answer_key(): '3.14'}
         self.assertEqual(module.lcp.student_answers, expected_answers)
 
         # Expect that the result is success
@@ -592,7 +634,7 @@ class CapaModuleTest(unittest.TestCase):
             mock_closed.return_value = True
 
             # Try to save the problem
-            get_request_dict = { CapaFactory.input_key(): '3.14'}
+            get_request_dict = {CapaFactory.input_key(): '3.14'}
             result = module.save_problem(get_request_dict)
 
         # Expect that the result is failure
@@ -603,7 +645,7 @@ class CapaModuleTest(unittest.TestCase):
         module = CapaFactory.create(rerandomize='always', done=True)
 
         # Try to save
-        get_request_dict = { CapaFactory.input_key(): '3.14'}
+        get_request_dict = {CapaFactory.input_key(): '3.14'}
         result = module.save_problem(get_request_dict)
 
         # Expect that we cannot save
@@ -614,7 +656,7 @@ class CapaModuleTest(unittest.TestCase):
         module = CapaFactory.create(rerandomize='never', done=True)
 
         # Try to save
-        get_request_dict = { CapaFactory.input_key(): '3.14'}
+        get_request_dict = {CapaFactory.input_key(): '3.14'}
         result = module.save_problem(get_request_dict)
 
         # Expect that we succeed
@@ -626,7 +668,7 @@ class CapaModuleTest(unittest.TestCase):
         # Just in case, we also check what happens if we have
         # more attempts than allowed.
         attempts = random.randint(1, 10)
-        module = CapaFactory.create(attempts=attempts -1, max_attempts=attempts)
+        module = CapaFactory.create(attempts=attempts - 1, max_attempts=attempts)
         self.assertEqual(module.check_button_name(), "Final Check")
 
         module = CapaFactory.create(attempts=attempts, max_attempts=attempts)
@@ -636,14 +678,14 @@ class CapaModuleTest(unittest.TestCase):
         self.assertEqual(module.check_button_name(), "Final Check")
 
         # Otherwise, button name is "Check"
-        module = CapaFactory.create(attempts=attempts -2, max_attempts=attempts)
+        module = CapaFactory.create(attempts=attempts - 2, max_attempts=attempts)
         self.assertEqual(module.check_button_name(), "Check")
 
-        module = CapaFactory.create(attempts=attempts -3, max_attempts=attempts)
+        module = CapaFactory.create(attempts=attempts - 3, max_attempts=attempts)
         self.assertEqual(module.check_button_name(), "Check")
 
         # If no limit on attempts, then always show "Check"
-        module = CapaFactory.create(attempts=attempts -3)
+        module = CapaFactory.create(attempts=attempts - 3)
         self.assertEqual(module.check_button_name(), "Check")
 
         module = CapaFactory.create(attempts=0)

lms/djangoapps/courseware/module_render.py

@@ -11,7 +11,7 @@ from django.contrib.auth.models import User
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
 from django.http import Http404
-from django.http import HttpResponse
+from django.http import HttpResponse, HttpResponseBadRequest
 from django.views.decorators.csrf import csrf_exempt
 
 from requests.auth import HTTPBasicAuth
@@ -23,7 +23,7 @@ from .models import StudentModule
 from psychometrics.psychoanalyze import make_psychometrics_data_update_handler
 from student.models import unique_id_for_user
 from xmodule.errortracker import exc_info_to_str
-from xmodule.exceptions import NotFoundError
+from xmodule.exceptions import NotFoundError, ProcessingError
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.x_module import ModuleSystem
@@ -443,9 +443,19 @@ def modx_dispatch(request, dispatch, location, course_id):
     # Let the module handle the AJAX
     try:
         ajax_return = instance.handle_ajax(dispatch, p)
+
+    # If we can't find the module, respond with a 404
     except NotFoundError:
         log.exception("Module indicating to user that request doesn't exist")
         raise Http404
+
+    # For XModule-specific errors, we respond with 400
+    except ProcessingError:
+        log.warning("Module encountered an error while prcessing AJAX call",
+                    exc_info=True)
+        return HttpResponseBadRequest()
+
+    # If any other error occurred, re-raise it to trigger a 500 response
     except:
         log.exception("error processing ajax call")
         raise