Responding to comments on pull #326

* cleaned up error module:
  - only one template
  - save error message in xml and reload
* better display of problem definition and metadata to staff
* save error messages as string, not exception objects.

common/djangoapps/xmodule_modifiers.py

@@ -89,8 +89,8 @@ def add_histogram(get_html, module):
         else:
             edit_link = False
 
-        staff_context = {'definition': dict(module.definition),
-                         'metadata': dict(module.metadata),
+        staff_context = {'definition': json.dumps(module.definition, indent=4),
+                         'metadata': json.dumps(module.metadata, indent=4),
                          'element_id': module.location.html_id(),
                          'edit_link': edit_link,
                          'histogram': json.dumps(histogram),

common/lib/xmodule/tests/test_import.py

@@ -73,6 +73,7 @@ class ImportTestCase(unittest.TestCase):
     def test_reimport(self):
         '''Make sure an already-exported error xml tag loads properly'''
 
+        self.maxDiff = None
         bad_xml = '''<sequential display_name="oops"><video url="hi"></sequential>'''
         system = self.get_system()
         descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course',

common/lib/xmodule/xmodule/error_module.py

+import sys
+import logging
+
 from pkg_resources import resource_string
 from lxml import etree
 from xmodule.x_module import XModule
 from xmodule.mako_module import MakoModuleDescriptor
 from xmodule.xml_module import XmlDescriptor
 from xmodule.editing_module import EditingDescriptor
+from xmodule.errortracker import exc_info_to_str
 
-import logging
 
 log = logging.getLogger(__name__)
 
@@ -14,14 +17,11 @@ class ErrorModule(XModule):
         '''Show an error.
         TODO (vshnayder): proper style, divs, etc.
         '''
-        if not self.system.is_staff:
-            return self.system.render_template('module-error.html', {})
-
         # staff get to see all the details
-        return self.system.render_template('module-error-staff.html', {
-            'data' : self.definition['data'],
-            # TODO (vshnayder): need to get non-syntax errors in here somehow
-            'error' : self.definition.get('error', 'Error not available')
+        return self.system.render_template('module-error.html', {
+            'data' : self.definition['data']['contents'],
+            'error' : self.definition['data']['error_msg'],
+            'is_staff' : self.system.is_staff,
             })
 
 class ErrorDescriptor(EditingDescriptor):
@@ -31,29 +31,36 @@ class ErrorDescriptor(EditingDescriptor):
     module_class = ErrorModule
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None, err=None):
+    def from_xml(cls, xml_data, system, org=None, course=None,
+                 error_msg='Error not available'):
         '''Create an instance of this descriptor from the supplied data.
 
         Does not try to parse the data--just stores it.
 
         Takes an extra, optional, parameter--the error that caused an
-        issue.
+        issue.  (should be a string, or convert usefully into one).
         '''
-
-        definition = {}
-        if err is not None:
-            definition['error'] = err
+        # Use a nested inner dictionary because 'data' is hardcoded
+        inner = {}
+        definition = {'data': inner}
+        inner['error_msg'] = str(error_msg)
 
         try:
             # If this is already an error tag, don't want to re-wrap it.
             xml_obj = etree.fromstring(xml_data)
             if xml_obj.tag == 'error':
                 xml_data = xml_obj.text
-        except etree.XMLSyntaxError as err:
+                error_node = xml_obj.find('error_msg')
+                if error_node is not None:
+                    inner['error_msg'] = error_node.text
+                else:
+                    inner['error_msg'] = 'Error not available'
+
+        except etree.XMLSyntaxError:
             # Save the error to display later--overrides other problems
-            definition['error'] = err
+            inner['error_msg'] = exc_info_to_str(sys.exc_info())
 
-        definition['data'] = xml_data
+        inner['contents'] = xml_data
         # TODO (vshnayder): Do we need a unique slug here?  Just pick a random
         # 64-bit num?
         location = ['i4x', org, course, 'error', 'slug']
@@ -71,10 +78,12 @@ class ErrorDescriptor(EditingDescriptor):
         files, etc.  That would just get re-wrapped on import.
         '''
         try:
-           xml = etree.fromstring(self.definition['data'])
+           xml = etree.fromstring(self.definition['data']['contents'])
            return etree.tostring(xml)
         except etree.XMLSyntaxError:
             # still not valid.
             root = etree.Element('error')
-            root.text = self.definition['data']
+            root.text = self.definition['data']['contents']
+            err_node = etree.SubElement(root, 'error_msg')
+            err_node.text = self.definition['data']['error_msg']
             return etree.tostring(root)

common/lib/xmodule/xmodule/errortracker.py

@@ -8,6 +8,12 @@ log = logging.getLogger(__name__)
 
 ErrorLog = namedtuple('ErrorLog', 'tracker errors')
 
+def exc_info_to_str(exc_info):
+    """Given some exception info, convert it into a string using
+    the traceback.format_exception() function.
+    """
+    return ''.join(traceback.format_exception(*exc_info))
+
 def in_exception_handler():
     '''Is there an active exception?'''
     return sys.exc_info() != (None, None, None)
@@ -27,7 +33,7 @@ def make_error_tracker():
         '''Log errors'''
         exc_str = ''
         if in_exception_handler():
-            exc_str = ''.join(traceback.format_exception(*sys.exc_info()))
+            exc_str = exc_info_to_str(sys.exc_info())
 
         errors.append((msg, exc_str))
 

common/lib/xmodule/xmodule/x_module.py

-from lxml import etree
-from lxml.etree import XMLSyntaxError
-import pkg_resources
 import logging
+import pkg_resources
+import sys
+
 from fs.errors import ResourceNotFoundError
 from functools import partial
+from lxml import etree
+from lxml.etree import XMLSyntaxError
 
 from xmodule.modulestore import Location
-
+from xmodule.errortracker import exc_info_to_str
 
 log = logging.getLogger('mitx.' + __name__)
 
@@ -471,7 +473,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
             msg = "Error loading from xml."
             log.exception(msg)
             system.error_tracker(msg)
-            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course, err)
+            err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
+            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course,
+                                                  err_msg)
 
         return descriptor
 

lms/templates/module-error-staff.html

-<section class="outside-app">
-  <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>Staff-only details below:</h1>
-
-<p>Error: ${error}</p>
-
-<p>Raw data: ${data}</p>
-
-</section>

lms/templates/module-error.html

 <section class="outside-app">
   <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>
+
+% if is_staff:
+<h1>Staff-only details below:</h1>
+
+<p>Error: ${error | h}</p>
+
+<p>Raw data: ${data | h}</p>
+% endif
+
 </section>