Merge pull request #4 from edx/diana/fix-error-descriptor

Ensure that the original error gets preserved by the ErrorModule

common/lib/xmodule/xmodule/error_module.py

@@ -121,7 +121,7 @@ class ErrorDescriptor(ErrorFields, JSONEditingDescriptor):
     def from_descriptor(cls, descriptor, error_msg='Error not available'):
         return cls._construct(
             descriptor.system,
-            descriptor._model_data,
+            str(descriptor),
             error_msg,
             location=descriptor.location,
         )

common/lib/xmodule/xmodule/tests/test_error_module.py

@@ -4,6 +4,9 @@ Tests for ErrorModule and NonStaffErrorModule
 import unittest
 from xmodule.tests import test_system
 import xmodule.error_module as error_module
+from xmodule.modulestore import Location
+from xmodule.x_module import XModuleDescriptor
+from mock import MagicMock
 
 
 class TestErrorModule(unittest.TestCase):
@@ -14,22 +17,33 @@ class TestErrorModule(unittest.TestCase):
         self.system = test_system()
         self.org = "org"
         self.course = "course"
-        self.fake_xml = "<problem />"
+        self.location = Location(['i4x', self.org, self.course, None, None])
+        self.valid_xml = "<problem />"
         self.broken_xml = "<problem>"
         self.error_msg = "Error"
 
-    def test_error_module_create(self):
+    def test_error_module_xml_rendering(self):
         descriptor = error_module.ErrorDescriptor.from_xml(
-            self.fake_xml, self.system, self.org, self.course)
+            self.valid_xml, self.system, self.org, self.course, self.error_msg)
         self.assertTrue(isinstance(descriptor, error_module.ErrorDescriptor))
-
-    def test_error_module_rendering(self):
-        descriptor = error_module.ErrorDescriptor.from_xml(
-            self.fake_xml, self.system, self.org, self.course, self.error_msg)
         module = descriptor.xmodule(self.system)
         rendered_html = module.get_html()
         self.assertIn(self.error_msg, rendered_html)
-        self.assertIn(self.fake_xml, rendered_html)
+        self.assertIn(self.valid_xml, rendered_html)
+
+    def test_error_module_from_descriptor(self):
+        descriptor = MagicMock([XModuleDescriptor],
+                               system=self.system,
+                               location=self.location,
+                               _model_data=self.valid_xml)
+
+        error_descriptor = error_module.ErrorDescriptor.from_descriptor(
+            descriptor, self.error_msg)
+        self.assertTrue(isinstance(error_descriptor, error_module.ErrorDescriptor))
+        module = error_descriptor.xmodule(self.system)
+        rendered_html = module.get_html()
+        self.assertIn(self.error_msg, rendered_html)
+        self.assertIn(str(descriptor), rendered_html)
 
 
 class TestNonStaffErrorModule(TestErrorModule):
@@ -39,13 +53,27 @@ class TestNonStaffErrorModule(TestErrorModule):
 
     def test_non_staff_error_module_create(self):
         descriptor = error_module.NonStaffErrorDescriptor.from_xml(
-            self.fake_xml, self.system, self.org, self.course)
+            self.valid_xml, self.system, self.org, self.course)
         self.assertTrue(isinstance(descriptor, error_module.NonStaffErrorDescriptor))
 
-    def test_non_staff_error_module_rendering(self):
+    def test_from_xml_render(self):
         descriptor = error_module.NonStaffErrorDescriptor.from_xml(
-            self.fake_xml, self.system, self.org, self.course)
+            self.valid_xml, self.system, self.org, self.course)
         module = descriptor.xmodule(self.system)
         rendered_html = module.get_html()
         self.assertNotIn(self.error_msg, rendered_html)
-        self.assertNotIn(self.fake_xml, rendered_html)
+        self.assertNotIn(self.valid_xml, rendered_html)
+
+    def test_error_module_from_descriptor(self):
+        descriptor = MagicMock([XModuleDescriptor],
+                               system=self.system,
+                               location=self.location,
+                               _model_data=self.valid_xml)
+
+        error_descriptor = error_module.NonStaffErrorDescriptor.from_descriptor(
+            descriptor, self.error_msg)
+        self.assertTrue(isinstance(error_descriptor, error_module.ErrorDescriptor))
+        module = error_descriptor.xmodule(self.system)
+        rendered_html = module.get_html()
+        self.assertNotIn(self.error_msg, rendered_html)
+        self.assertNotIn(str(descriptor), rendered_html)

lms/djangoapps/courseware/module_render.py

@@ -335,9 +335,8 @@ def get_module_for_descriptor(user, request, descriptor, model_data_cache, cours
         else:
             err_descriptor_class = NonStaffErrorDescriptor
 
-        err_descriptor = err_descriptor_class.from_xml(
-            str(descriptor), descriptor.system,
-            org=descriptor.location.org, course=descriptor.location.course,
+        err_descriptor = err_descriptor_class.from_descriptor(
+            descriptor,
             error_msg=exc_info_to_str(sys.exc_info())
         )