Merge pull request #497 from MITx/feature/victor/catch-module-errors

Catch errors in module load

common/lib/xmodule/xmodule/error_module.py

@@ -15,18 +15,39 @@ from xmodule.errortracker import exc_info_to_str
 
 log = logging.getLogger(__name__)
 
+# NOTE: This is not the most beautiful design in the world, but there's no good
+# way to tell if the module is being used in a staff context or not.  Errors that get discovered
+# at course load time are turned into ErrorDescriptor objects, and automatically hidden from students.
+# Unfortunately, we can also have errors when loading modules mid-request, and then we need to decide
+# what to show, and the logic for that belongs in the LMS (e.g. in get_module), so the error handler
+# decides whether to create a staff or not-staff module.
+
 class ErrorModule(XModule):
     def get_html(self):
-        '''Show an error.
+        '''Show an error to staff.
         TODO (vshnayder): proper style, divs, etc.
         '''
         # staff get to see all the details
         return self.system.render_template('module-error.html', {
+            'staff_access' : True,
             'data' : self.definition['data']['contents'],
             'error' : self.definition['data']['error_msg'],
             })
 
 
+class NonStaffErrorModule(XModule):
+    def get_html(self):
+        '''Show an error to a student.
+        TODO (vshnayder): proper style, divs, etc.
+        '''
+        # staff get to see all the details
+        return self.system.render_template('module-error.html', {
+            'staff_access' : False,
+            'data' : "",
+            'error' : "",
+            })
+
+
 class ErrorDescriptor(EditingDescriptor):
     """
     Module that provides a raw editing view of broken xml.
@@ -99,3 +120,9 @@ class ErrorDescriptor(EditingDescriptor):
             err_node = etree.SubElement(root, 'error_msg')
             err_node.text = self.definition['data']['error_msg']
             return etree.tostring(root)
+
+class NonStaffErrorDescriptor(ErrorDescriptor):
+    """
+    Module that provides non-staff error messages.
+    """
+    module_class = NonStaffErrorModule

lms/djangoapps/courseware/access.py

@@ -13,7 +13,6 @@ from xmodule.modulestore import Location
 from xmodule.timeparse import parse_time
 from xmodule.x_module import XModule, XModuleDescriptor
 
-
 DEBUG_ACCESS = False
 
 log = logging.getLogger(__name__)

lms/djangoapps/courseware/module_render.py

 import json
 import logging
+import sys
 
 from django.conf import settings
 from django.contrib.auth.models import User
@@ -15,10 +16,12 @@ from courseware.access import has_access
 from mitxmako.shortcuts import render_to_string
 from models import StudentModule, StudentModuleCache
 from static_replace import replace_urls
+from xmodule.errortracker import exc_info_to_str
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.x_module import ModuleSystem
+from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor
 from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule
 
 log = logging.getLogger("mitx.courseware")
@@ -73,6 +76,8 @@ def toc_for_course(user, request, course, active_chapter, active_section, course
     student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
         course_id, user, course, depth=2)
     course = get_module(user, request, course.location, student_module_cache, course_id)
+    if course is None:
+        return None
 
     chapters = list()
     for chapter in course.get_display_items():
@@ -131,9 +136,9 @@ def get_section(course_module, chapter, section):
 
     return section_module
 
-
 def get_module(user, request, location, student_module_cache, course_id, position=None):
-    ''' Get an instance of the xmodule class identified by location,
+    """
+    Get an instance of the xmodule class identified by location,
     setting the state based on an existing StudentModule, or creating one if none
     exists.
 
@@ -146,9 +151,22 @@ def get_module(user, request, location, student_module_cache, course_id, positio
       - position              : extra information from URL for user-specified
                                 position within module
 
-    Returns: xmodule instance
+    Returns: xmodule instance, or None if the user does not have access to the
+    module.  If there's an error, will try to return an instance of ErrorModule
+    if possible.  If not possible, return None.
+    """
+    try:
+        return _get_module(user, request, location, student_module_cache, course_id, position)
+    except:
+        # Something has gone terribly wrong, but still not letting it turn into a 500.
+        log.exception("Error in get_module")
+        return None
 
-    '''
+def _get_module(user, request, location, student_module_cache, course_id, position=None):
+    """
+    Actually implement get_module.  See docstring there for details.
+    """
+    location = Location(location)
     descriptor = modulestore().get_instance(course_id, location)
 
     # Short circuit--if the user shouldn't have access, bail without doing any work
@@ -198,7 +216,7 @@ def get_module(user, request, location, student_module_cache, course_id, positio
               'callback_url': xqueue_callback_url,
               'default_queuename': xqueue_default_queuename.replace(' ', '_')}
 
-    def _get_module(location):
+    def inner_get_module(location):
         """
         Delegate to get_module.  It does an access check, so may return None
         """
@@ -214,7 +232,7 @@ def get_module(user, request, location, student_module_cache, course_id, positio
                           xqueue=xqueue,
                           # TODO (cpennington): Figure out how to share info between systems
                           filestore=descriptor.system.resources_fs,
-                          get_module=_get_module,
+                          get_module=inner_get_module,
                           user=user,
                           # TODO (cpennington): This should be removed when all html from
                           # a module is coming through get_html and is therefore covered
@@ -226,7 +244,22 @@ def get_module(user, request, location, student_module_cache, course_id, positio
     system.set('position', position)
     system.set('DEBUG', settings.DEBUG)
 
-    module = descriptor.xmodule_constructor(system)(instance_state, shared_state)
+    try:
+        module = descriptor.xmodule_constructor(system)(instance_state, shared_state)
+    except:
+        log.exception("Error creating module from descriptor {0}".format(descriptor))
+
+        # make an ErrorDescriptor -- assuming that the descriptor's system is ok
+        import_system = descriptor.system
+        if has_access(user, location, 'staff'):
+            err_descriptor = ErrorDescriptor.from_xml(str(descriptor), import_system,
+                                                      error_msg=exc_info_to_str(sys.exc_info()))
+        else:
+            err_descriptor = NonStaffErrorDescriptor.from_xml(str(descriptor), import_system,
+                                                              error_msg=exc_info_to_str(sys.exc_info()))
+
+        # Make an error module
+        return err_descriptor.xmodule_constructor(system)(None, None)
 
     module.get_html = replace_static_urls(
         wrap_xmodule(module.get_html, module, 'xmodule_display.html'),

lms/templates/module-error.html

@@ -2,10 +2,19 @@
   <h1>There has been an error on the <em>MITx</em> servers</h1>
   <p>We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible. Please email us at <a href="mailto:technical@mitx.mit.edu">technical@mitx.mit.edu</a> to report any problems or downtime.</p>
 
-<h1>Details below:</h1>
+% if staff_access:
+<h1>Details</h1>
 
-<p>Error: ${error | h}</p>
+<p>Error:
+<pre>
+${error | h}
+</pre>
+</p>
 
-<p>Raw data: ${data | h}</p>
+<p>Raw data:
 
+<pre>${data | h}</pre>
+</p>
+
+% endif
 </section>