From 365495521e0fb81668c307d97239f79179a2d59a Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 22 Aug 2012 12:58:46 -0400 Subject: [PATCH] Catch errors in module load * if error is in xmodule_constructor(), catch and return an ErrorModule * if error is somewhere else in get_module(), return None --- common/lib/xmodule/xmodule/error_module.py | 29 ++++++++++++- lms/djangoapps/courseware/access.py | 1 - lms/djangoapps/courseware/module_render.py | 47 ++++++++++++++++++---- lms/templates/module-error.html | 15 +++++-- 4 files changed, 80 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index bdd7179a0a..f8e2467910 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/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 diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 281580cf33..dbe4ff376d 100644 --- a/lms/djangoapps/courseware/access.py +++ b/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__) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 65e30475f2..7967452647 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -1,5 +1,6 @@ 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'), diff --git a/lms/templates/module-error.html b/lms/templates/module-error.html index 2a51f5b11a..8855c5be48 100644 --- a/lms/templates/module-error.html +++ b/lms/templates/module-error.html @@ -2,10 +2,19 @@

There has been an error on the MITx servers

We're sorry, this module is temporarily unavailable. Our staff is working to fix it as soon as possible. Please email us at technical@mitx.mit.edu to report any problems or downtime.

-

Details below:

+% if staff_access: +

Details

-

Error: ${error | h}

+

Error: +

+${error | h}
+
+

-

Raw data: ${data | h}

+

Raw data: +

${data | h}
+

+ +% endif