From 4e6a9b0df7e3cffc8e63590b8124c69dd2369b34 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 10:36:33 -0400 Subject: [PATCH 01/28] Add kwargs back to DescriptorSystem init()s * allow future expansion without breaking interface. --- common/lib/xmodule/xmodule/mako_module.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/xml.py | 6 +++--- common/lib/xmodule/xmodule/x_module.py | 7 ++++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index fcc47aaaaf..213e9077db 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -3,9 +3,9 @@ from x_module import XModuleDescriptor, DescriptorSystem class MakoDescriptorSystem(DescriptorSystem): def __init__(self, load_item, resources_fs, error_handler, - render_template): + render_template, **kwargs): super(MakoDescriptorSystem, self).__init__( - load_item, resources_fs, error_handler) + load_item, resources_fs, error_handler, **kwargs) self.render_template = render_template diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 7dd6868f78..8a084b19ee 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -29,7 +29,7 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, error_handler): + def __init__(self, xmlstore, org, course, course_dir, error_handler, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that all elements have unique slugs. @@ -88,9 +88,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): resources_fs = OSFS(xmlstore.data_dir / course_dir) MakoDescriptorSystem.__init__(self, load_item, resources_fs, - error_handler, render_template) + error_handler, render_template, **kwargs) XMLParsingSystem.__init__(self, load_item, resources_fs, - error_handler, process_xml) + error_handler, process_xml, **kwargs) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 7bb98dcdc5..97ae307809 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -521,7 +521,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): class DescriptorSystem(object): - def __init__(self, load_item, resources_fs, error_handler): + def __init__(self, load_item, resources_fs, error_handler, **kwargs): """ load_item: Takes a Location and returns an XModuleDescriptor @@ -561,14 +561,15 @@ class DescriptorSystem(object): class XMLParsingSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, error_handler, process_xml): + def __init__(self, load_item, resources_fs, error_handler, process_xml, **kwargs): """ load_item, resources_fs, error_handler: see DescriptorSystem process_xml: Takes an xml string, and returns a XModuleDescriptor created from that xml """ - DescriptorSystem.__init__(self, load_item, resources_fs, error_handler) + DescriptorSystem.__init__(self, load_item, resources_fs, error_handler, + **kwargs) self.process_xml = process_xml From ef6da22ac35b70f313ee8f9e60c89d68e89bd745 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 10:42:23 -0400 Subject: [PATCH 02/28] Add a fallback MalformedDescriptor * when things don't load normally, use this * separate raw editing functionality into EditingDescriptor * raw descriptor just enforces that xml is valid * add a MalformedDescriptor that just saves a string * Fallback to it on import. --- common/lib/xmodule/tests/test_import.py | 38 +++++++++++++++++ common/lib/xmodule/xmodule/editing_module.py | 24 +++++++++++ .../lib/xmodule/xmodule/malformed_module.py | 41 +++++++++++++++++++ common/lib/xmodule/xmodule/raw_module.py | 18 ++------ common/lib/xmodule/xmodule/x_module.py | 34 ++++++++++----- 5 files changed, 131 insertions(+), 24 deletions(-) create mode 100644 common/lib/xmodule/tests/test_import.py create mode 100644 common/lib/xmodule/xmodule/editing_module.py create mode 100644 common/lib/xmodule/xmodule/malformed_module.py diff --git a/common/lib/xmodule/tests/test_import.py b/common/lib/xmodule/tests/test_import.py new file mode 100644 index 0000000000..c0bd9af3d0 --- /dev/null +++ b/common/lib/xmodule/tests/test_import.py @@ -0,0 +1,38 @@ +from path import path + +import unittest + +from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.errorhandlers import ignore_errors_handler +from xmodule.modulestore import Location + +class ImportTestCase(unittest.TestCase): + '''Make sure module imports work properly, including for malformed inputs''' + + def test_fallback(self): + '''Make sure that malformed xml loads as a MalformedDescriptorb.''' + + bad_xml = '''''' + + # Shouldn't need any system params, because the initial parse should fail + def load_item(loc): + raise Exception("Shouldn't be called") + + resources_fs = None + + def process_xml(xml): + raise Exception("Shouldn't be called") + + + def render_template(template, context): + raise Exception("Shouldn't be called") + + system = XMLParsingSystem(load_item, resources_fs, + ignore_errors_handler, process_xml) + system.render_template = render_template + + descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course', + None) + + self.assertEqual(descriptor.__class__.__name__, + 'MalformedDescriptor') diff --git a/common/lib/xmodule/xmodule/editing_module.py b/common/lib/xmodule/xmodule/editing_module.py new file mode 100644 index 0000000000..4188165a24 --- /dev/null +++ b/common/lib/xmodule/xmodule/editing_module.py @@ -0,0 +1,24 @@ +from pkg_resources import resource_string +from lxml import etree +from xmodule.mako_module import MakoModuleDescriptor +import logging + +log = logging.getLogger(__name__) + +class EditingDescriptor(MakoModuleDescriptor): + """ + Module that provides a raw editing view of its data and children. It does not + perform any validation on its definition---just passes it along to the browser. + + This class is intended to be used as a mixin. + """ + mako_template = "widgets/raw-edit.html" + + js = {'coffee': [resource_string(__name__, 'js/src/raw/edit.coffee')]} + js_module_name = "RawDescriptor" + + def get_context(self): + return { + 'module': self, + 'data': self.definition['data'], + } diff --git a/common/lib/xmodule/xmodule/malformed_module.py b/common/lib/xmodule/xmodule/malformed_module.py new file mode 100644 index 0000000000..803813dee8 --- /dev/null +++ b/common/lib/xmodule/xmodule/malformed_module.py @@ -0,0 +1,41 @@ +from pkg_resources import resource_string +from lxml import etree +from xmodule.mako_module import MakoModuleDescriptor +from xmodule.xml_module import XmlDescriptor +from xmodule.editing_module import EditingDescriptor + +import logging + +log = logging.getLogger(__name__) + +class MalformedDescriptor(EditingDescriptor): + """ + Module that provides a raw editing view of broken xml. + """ + + @classmethod + def from_xml(cls, xml_data, system, org=None, course=None): + '''Create an instance of this descriptor from the supplied data. + + Does not try to parse the data--just stores it. + ''' + + # TODO (vshnayder): how does one get back from this to a valid descriptor? + # try to parse and if successfull, send back to x_module? + + definition = { 'data' : xml_data } + # TODO (vshnayder): Do we need a valid slug here? Just pick a random + # 64-bit num? + location = ['i4x', org, course, 'malformed', 'slug'] + metadata = {} # stays in the xml_data + + return cls(system, definition, location=location, metadata=metadata) + + def export_to_xml(self, resource_fs): + ''' + Export as a string wrapped in xml + ''' + root = etree.Element('malformed') + root.text = self.definition['data'] + return etree.tostring(root) + diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index 90f4139bd5..f9f358f945 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -1,27 +1,17 @@ from pkg_resources import resource_string from lxml import etree +from xmodule.editing_module import EditingDescriptor from xmodule.mako_module import MakoModuleDescriptor from xmodule.xml_module import XmlDescriptor import logging log = logging.getLogger(__name__) - -class RawDescriptor(MakoModuleDescriptor, XmlDescriptor): +class RawDescriptor(XmlDescriptor, EditingDescriptor): """ - Module that provides a raw editing view of its data and children + Module that provides a raw editing view of its data and children. It + requires that the definition xml is valid. """ - mako_template = "widgets/raw-edit.html" - - js = {'coffee': [resource_string(__name__, 'js/src/raw/edit.coffee')]} - js_module_name = "RawDescriptor" - - def get_context(self): - return { - 'module': self, - 'data': self.definition['data'], - } - @classmethod def definition_from_xml(cls, xml_object, system): return {'data': etree.tostring(xml_object)} diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 97ae307809..c4c5110abc 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1,10 +1,12 @@ from lxml import etree +from lxml.etree import XMLSyntaxError import pkg_resources import logging +from fs.errors import ResourceNotFoundError +from functools import partial from xmodule.modulestore import Location -from functools import partial log = logging.getLogger('mitx.' + __name__) @@ -443,16 +445,28 @@ class XModuleDescriptor(Plugin, HTMLSnippet): system is an XMLParsingSystem org and course are optional strings that will be used in the generated - modules url identifiers + module's url identifiers """ - class_ = XModuleDescriptor.load_class( - etree.fromstring(xml_data).tag, - default_class - ) - # leave next line, commented out - useful for low-level debugging - # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % ( - # etree.fromstring(xml_data).tag,class_)) - return class_.from_xml(xml_data, system, org, course) + try: + class_ = XModuleDescriptor.load_class( + etree.fromstring(xml_data).tag, + default_class + ) + # leave next line, commented out - useful for low-level debugging + # log.debug('[XModuleDescriptor.load_from_xml] tag=%s, class_=%s' % ( + # etree.fromstring(xml_data).tag,class_)) + + descriptor = class_.from_xml(xml_data, system, org, course) + except (ResourceNotFoundError, XMLSyntaxError) as err: + # Didn't load properly. Fall back on loading as a malformed + # descriptor. This should never error due to formatting. + + # Put import here to avoid circular import errors + from xmodule.malformed_module import MalformedDescriptor + + descriptor = MalformedDescriptor.from_xml(xml_data, system, org, course) + + return descriptor @classmethod def from_xml(cls, xml_data, system, org=None, course=None): From 53608922ba2cf74f27e5f05a23694943593704f4 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 11:36:43 -0400 Subject: [PATCH 03/28] Make the malformed descriptor import properly * Also get rid of lazy loading of metadata and definition --- common/lib/xmodule/setup.py | 1 + .../lib/xmodule/xmodule/malformed_module.py | 11 +- common/lib/xmodule/xmodule/x_module.py | 1 + common/lib/xmodule/xmodule/xml_module.py | 101 +++++++++--------- 4 files changed, 62 insertions(+), 52 deletions(-) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index b495cd0aee..09a6b7e712 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -25,6 +25,7 @@ setup( "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", "html = xmodule.html_module:HtmlDescriptor", "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", + "malformed = xmodule.malformed_module:MalformedDescriptor", "problem = xmodule.capa_module:CapaDescriptor", "problemset = xmodule.vertical_module:VerticalDescriptor", "section = xmodule.backcompat_module:SemanticSectionDescriptor", diff --git a/common/lib/xmodule/xmodule/malformed_module.py b/common/lib/xmodule/xmodule/malformed_module.py index 803813dee8..54032cf7d2 100644 --- a/common/lib/xmodule/xmodule/malformed_module.py +++ b/common/lib/xmodule/xmodule/malformed_module.py @@ -20,8 +20,15 @@ class MalformedDescriptor(EditingDescriptor): Does not try to parse the data--just stores it. ''' - # TODO (vshnayder): how does one get back from this to a valid descriptor? - # try to parse and if successfull, send back to x_module? + #log.debug("processing '{0}'".format(xml_data)) + try: + xml_obj = etree.fromstring(xml_data) + if xml_obj.tag == 'malformed': + xml_data = xml_obj.text + # TODO (vshnayder): how does one get back from this to a valid descriptor? + # For now, have to fix manually. + except etree.XMLSyntaxError: + pass definition = { 'data' : xml_data } # TODO (vshnayder): Do we need a valid slug here? Just pick a random diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index c4c5110abc..8803c37d78 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -464,6 +464,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): # Put import here to avoid circular import errors from xmodule.malformed_module import MalformedDescriptor + #system.error_handler("Error loading from xml.") descriptor = MalformedDescriptor.from_xml(xml_data, system, org, course) return descriptor diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 6750906eb4..5786dbd227 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,66 +11,65 @@ import os log = logging.getLogger(__name__) -# TODO (cpennington): This was implemented in an attempt to improve performance, -# but the actual improvement wasn't measured (and it was implemented late at night). -# We should check if it hurts, and whether there's a better way of doing lazy loading +# # TODO (cpennington): This was implemented in an attempt to improve performance, +# # but the actual improvement wasn't measured (and it was implemented late at night). +# # We should check if it hurts, and whether there's a better way of doing lazy loading +# class LazyLoadingDict(MutableMapping): +# """ +# A dictionary object that lazily loads its contents from a provided +# function on reads (of members that haven't already been set). +# """ -class LazyLoadingDict(MutableMapping): - """ - A dictionary object that lazily loads its contents from a provided - function on reads (of members that haven't already been set). - """ +# def __init__(self, loader): +# ''' +# On the first read from this dictionary, it will call loader() to +# populate its contents. loader() must return something dict-like. Any +# elements set before the first read will be preserved. +# ''' +# self._contents = {} +# self._loaded = False +# self._loader = loader +# self._deleted = set() - def __init__(self, loader): - ''' - On the first read from this dictionary, it will call loader() to - populate its contents. loader() must return something dict-like. Any - elements set before the first read will be preserved. - ''' - self._contents = {} - self._loaded = False - self._loader = loader - self._deleted = set() +# def __getitem__(self, name): +# if not (self._loaded or name in self._contents or name in self._deleted): +# self.load() - def __getitem__(self, name): - if not (self._loaded or name in self._contents or name in self._deleted): - self.load() +# return self._contents[name] - return self._contents[name] +# def __setitem__(self, name, value): +# self._contents[name] = value +# self._deleted.discard(name) - def __setitem__(self, name, value): - self._contents[name] = value - self._deleted.discard(name) +# def __delitem__(self, name): +# del self._contents[name] +# self._deleted.add(name) - def __delitem__(self, name): - del self._contents[name] - self._deleted.add(name) +# def __contains__(self, name): +# self.load() +# return name in self._contents - def __contains__(self, name): - self.load() - return name in self._contents +# def __len__(self): +# self.load() +# return len(self._contents) - def __len__(self): - self.load() - return len(self._contents) +# def __iter__(self): +# self.load() +# return iter(self._contents) - def __iter__(self): - self.load() - return iter(self._contents) +# def __repr__(self): +# self.load() +# return repr(self._contents) - def __repr__(self): - self.load() - return repr(self._contents) +# def load(self): +# if self._loaded: +# return - def load(self): - if self._loaded: - return - - loaded_contents = self._loader() - loaded_contents.update(self._contents) - self._contents = loaded_contents - self._loaded = True +# loaded_contents = self._loader() +# loaded_contents.update(self._contents) +# self._contents = loaded_contents +# self._loaded = True _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') @@ -230,11 +229,13 @@ class XmlDescriptor(XModuleDescriptor): cls.clean_metadata_from_xml(definition_xml) return cls.definition_from_xml(definition_xml, system) + # VS[compat] -- just have the url_name lookup once translation is done + slug = xml_object.get('url_name', xml_object.get('slug')) return cls( system, - LazyLoadingDict(definition_loader), + definition, location=location, - metadata=LazyLoadingDict(metadata_loader), + metadata=metadata, ) @classmethod From 0d83d2e645a4a701ebad7e5158493c198d552370 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 11:51:14 -0400 Subject: [PATCH 04/28] Add roundtrip test for malformed module * also fix error message in backcompat_module --- common/lib/xmodule/tests/test_import.py | 35 ++++++++++++++++--- .../lib/xmodule/xmodule/backcompat_module.py | 2 +- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/tests/test_import.py b/common/lib/xmodule/tests/test_import.py index c0bd9af3d0..6643b93a67 100644 --- a/common/lib/xmodule/tests/test_import.py +++ b/common/lib/xmodule/tests/test_import.py @@ -9,11 +9,9 @@ from xmodule.modulestore import Location class ImportTestCase(unittest.TestCase): '''Make sure module imports work properly, including for malformed inputs''' - def test_fallback(self): - '''Make sure that malformed xml loads as a MalformedDescriptorb.''' - - bad_xml = '''''' - + @staticmethod + def get_system(): + '''Get a dummy system''' # Shouldn't need any system params, because the initial parse should fail def load_item(loc): raise Exception("Shouldn't be called") @@ -31,8 +29,35 @@ class ImportTestCase(unittest.TestCase): ignore_errors_handler, process_xml) system.render_template = render_template + return system + + def test_fallback(self): + '''Make sure that malformed xml loads as a MalformedDescriptorb.''' + + bad_xml = '''''' + + system = self.get_system() + descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course', None) self.assertEqual(descriptor.__class__.__name__, 'MalformedDescriptor') + + def test_reimport(self): + '''Make sure an already-exported malformed xml tag loads properly''' + + bad_xml = '''''' + system = self.get_system() + descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course', + None) + resource_fs = None + tag_xml = descriptor.export_to_xml(resource_fs) + re_import_descriptor = XModuleDescriptor.load_from_xml(tag_xml, system, + 'org', 'course', + None) + self.assertEqual(re_import_descriptor.__class__.__name__, + 'MalformedDescriptor') + + self.assertEqual(descriptor.definition['data'], + re_import_descriptor.definition['data']) diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index 997ad476c4..da0d6788e4 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -32,7 +32,7 @@ def process_includes(fn): # read in and convert to XML incxml = etree.XML(ifp.read()) - # insert new XML into tree in place of inlcude + # insert new XML into tree in place of include parent.insert(parent.index(next_include), incxml) except Exception: msg = "Error in problem xml include: %s" % (etree.tostring(next_include, pretty_print=True)) From 10054e95ce0e0962e975ff236b5237f659cd6bc7 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 15:09:33 -0400 Subject: [PATCH 05/28] add message to import command --- cms/djangoapps/contentstore/management/commands/import.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cms/djangoapps/contentstore/management/commands/import.py b/cms/djangoapps/contentstore/management/commands/import.py index 69aaa35a7d..a6790ad59b 100644 --- a/cms/djangoapps/contentstore/management/commands/import.py +++ b/cms/djangoapps/contentstore/management/commands/import.py @@ -23,4 +23,7 @@ class Command(BaseCommand): course_dirs = args[1:] else: course_dirs = None + print "Importing. Data_dir={data}, course_dirs={courses}".format( + data=data_dir, + courses=course_dirs) import_from_xml(modulestore(), data_dir, course_dirs) From c53ed6a23830f842aa130c104e7c9125509c4b9a Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 15:10:21 -0400 Subject: [PATCH 06/28] remove some debugging messages --- common/lib/capa/capa/capa_problem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index ed99c71635..93b6a085c1 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -329,7 +329,7 @@ class LoncapaProblem(object): # path is an absolute path or a path relative to the data dir dir = os.path.join(self.system.filestore.root_path, dir) abs_dir = os.path.normpath(dir) - log.debug("appending to path: %s" % abs_dir) + #log.debug("appending to path: %s" % abs_dir) path.append(abs_dir) return path From d750d945fdd855ea05e69d346b248b222c6e4136 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 15:11:38 -0400 Subject: [PATCH 07/28] Remove malformed tags when contents aren't malformed anymore. --- common/lib/xmodule/tests/test_import.py | 26 +++++++++++++++- .../lib/xmodule/xmodule/malformed_module.py | 30 +++++++++++++++---- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/tests/test_import.py b/common/lib/xmodule/tests/test_import.py index 6643b93a67..dd55f8ff38 100644 --- a/common/lib/xmodule/tests/test_import.py +++ b/common/lib/xmodule/tests/test_import.py @@ -1,7 +1,8 @@ from path import path - import unittest +from lxml import etree + from xmodule.x_module import XMLParsingSystem, XModuleDescriptor from xmodule.errorhandlers import ignore_errors_handler from xmodule.modulestore import Location @@ -61,3 +62,26 @@ class ImportTestCase(unittest.TestCase): self.assertEqual(descriptor.definition['data'], re_import_descriptor.definition['data']) + + def test_fixed_xml_tag(self): + """Make sure a tag that's been fixed exports as the original tag type""" + + # create a malformed tag with valid xml contents + root = etree.Element('malformed') + good_xml = '''''' + root.text = good_xml + + xml_str_in = etree.tostring(root) + + # load it + system = self.get_system() + descriptor = XModuleDescriptor.load_from_xml(xml_str_in, system, 'org', 'course', + None) + # export it + resource_fs = None + xml_str_out = descriptor.export_to_xml(resource_fs) + + # Now make sure the exported xml is a sequential + xml_out = etree.fromstring(xml_str_out) + self.assertEqual(xml_out.tag, 'sequential') + diff --git a/common/lib/xmodule/xmodule/malformed_module.py b/common/lib/xmodule/xmodule/malformed_module.py index 54032cf7d2..ac419fff26 100644 --- a/common/lib/xmodule/xmodule/malformed_module.py +++ b/common/lib/xmodule/xmodule/malformed_module.py @@ -1,5 +1,6 @@ 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 @@ -8,10 +9,18 @@ import logging log = logging.getLogger(__name__) +class MalformedModule(XModule): + def get_html(self): + '''Show an error. + TODO (vshnayder): proper style, divs, etc. + ''' + return "Malformed content--not showing through get_html()" + class MalformedDescriptor(EditingDescriptor): """ Module that provides a raw editing view of broken xml. """ + module_class = MalformedModule @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -20,8 +29,8 @@ class MalformedDescriptor(EditingDescriptor): Does not try to parse the data--just stores it. ''' - #log.debug("processing '{0}'".format(xml_data)) try: + # If this is already a malformed tag, don't want to re-wrap it. xml_obj = etree.fromstring(xml_data) if xml_obj.tag == 'malformed': xml_data = xml_obj.text @@ -40,9 +49,18 @@ class MalformedDescriptor(EditingDescriptor): def export_to_xml(self, resource_fs): ''' - Export as a string wrapped in xml - ''' - root = etree.Element('malformed') - root.text = self.definition['data'] - return etree.tostring(root) + If the definition data is invalid xml, export it wrapped in a malformed + tag. If it is valid, export without the wrapper. + NOTE: There may still be problems with the valid xml--it could be + missing required attributes, could have the wrong tags, refer to missing + files, etc. + ''' + try: + xml = etree.fromstring(self.definition['data']) + return etree.tostring(xml) + except etree.XMLSyntaxError: + # still not valid. + root = etree.Element('malformed') + root.text = self.definition['data'] + return etree.tostring(root) From 46775386d376e0cba2e2d146bce0e34e3984cfda Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 18:29:18 -0400 Subject: [PATCH 08/28] make CACHE_TIMEOUT messages go away --- cms/envs/dev.py | 3 +++ lms/envs/dev.py | 13 ++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 59bc623729..4098263829 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -76,3 +76,6 @@ CACHES = { 'KEY_FUNCTION': 'util.memcache.safe_key', } } + +# Make the keyedcache startup warnings go away +CACHE_TIMEOUT = 0 diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 1a2659cb1f..3fd86f1aee 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -17,7 +17,7 @@ MITX_FEATURES['DISABLE_START_DATES'] = True WIKI_ENABLED = True -LOGGING = get_logger_config(ENV_ROOT / "log", +LOGGING = get_logger_config(ENV_ROOT / "log", logging_env="dev", tracking_filename="tracking.log", debug=True) @@ -30,7 +30,7 @@ DATABASES = { } CACHES = { - # This is the cache used for most things. Askbot will not work without a + # This is the cache used for most things. Askbot will not work without a # functioning cache -- it relies on caching to load its settings in places. # In staging/prod envs, the sessions also live here. 'default': { @@ -52,11 +52,14 @@ CACHES = { } } +# Make the keyedcache startup warnings go away +CACHE_TIMEOUT = 0 + # Dummy secret key for dev SECRET_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' ################################ DEBUG TOOLBAR ################################# -INSTALLED_APPS += ('debug_toolbar',) +INSTALLED_APPS += ('debug_toolbar',) MIDDLEWARE_CLASSES += ('debug_toolbar.middleware.DebugToolbarMiddleware',) INTERNAL_IPS = ('127.0.0.1',) @@ -71,8 +74,8 @@ DEBUG_TOOLBAR_PANELS = ( 'debug_toolbar.panels.logger.LoggingPanel', # Enabling the profiler has a weird bug as of django-debug-toolbar==0.9.4 and -# Django=1.3.1/1.4 where requests to views get duplicated (your method gets -# hit twice). So you can uncomment when you need to diagnose performance +# Django=1.3.1/1.4 where requests to views get duplicated (your method gets +# hit twice). So you can uncomment when you need to diagnose performance # problems, but you shouldn't leave it on. # 'debug_toolbar.panels.profiling.ProfilingDebugPanel', ) From ed35cefa295ae4b8e798d86587cbf8c838289df6 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 30 Jul 2012 18:36:02 -0400 Subject: [PATCH 09/28] Fix html file handling. * html files are now stored as follows: If the html file is valid xml, store as html/stuff.xml If it's not, store as html/stuff.xml, which contains , and html/stuff.html, which actually contains the contents. Warn if the contents are not parseable with lxml's html parser, but don't error. * for parseable html, strip out the html tag when storing, so that it isn't rendered into the middle of a page * lots of backcompat to deal with paths. Can go away soon. * fix output ordering in clean_xml --- common/lib/xmodule/html_checker.py | 14 +++ common/lib/xmodule/stringify.py | 20 ++++ common/lib/xmodule/tests/test_stringify.py | 9 ++ common/lib/xmodule/xmodule/html_module.py | 103 ++++++++++++++++-- common/lib/xmodule/xmodule/xml_module.py | 88 ++++++++------- .../management/commands/clean_xml.py | 1 + 6 files changed, 184 insertions(+), 51 deletions(-) create mode 100644 common/lib/xmodule/html_checker.py create mode 100644 common/lib/xmodule/stringify.py create mode 100644 common/lib/xmodule/tests/test_stringify.py diff --git a/common/lib/xmodule/html_checker.py b/common/lib/xmodule/html_checker.py new file mode 100644 index 0000000000..5e6b417d28 --- /dev/null +++ b/common/lib/xmodule/html_checker.py @@ -0,0 +1,14 @@ +from lxml import etree + +def check_html(html): + ''' + Check whether the passed in html string can be parsed by lxml. + Return bool success. + ''' + parser = etree.HTMLParser() + try: + etree.fromstring(html, parser) + return True + except Exception as err: + pass + return False diff --git a/common/lib/xmodule/stringify.py b/common/lib/xmodule/stringify.py new file mode 100644 index 0000000000..dad964140f --- /dev/null +++ b/common/lib/xmodule/stringify.py @@ -0,0 +1,20 @@ +from itertools import chain +from lxml import etree + +def stringify_children(node): + ''' + Return all contents of an xml tree, without the outside tags. + e.g. if node is parse of + "Hi
there Bruce!
" + should return + "Hi
there Bruce!
" + + fixed from + http://stackoverflow.com/questions/4624062/get-all-text-inside-a-tag-in-lxml + ''' + parts = ([node.text] + + list(chain(*([etree.tostring(c), c.tail] + for c in node.getchildren()) + ))) + # filter removes possible Nones in texts and tails + return ''.join(filter(None, parts)) diff --git a/common/lib/xmodule/tests/test_stringify.py b/common/lib/xmodule/tests/test_stringify.py new file mode 100644 index 0000000000..62d7683886 --- /dev/null +++ b/common/lib/xmodule/tests/test_stringify.py @@ -0,0 +1,9 @@ +from nose.tools import assert_equals +from lxml import etree +from stringify import stringify_children + +def test_stringify(): + html = '''Hi
there Bruce!
''' + xml = etree.fromstring(html) + out = stringify_children(xml) + assert_equals(out, '''Hi
there Bruce!
''') diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index b9bc34aed6..22d3bf163f 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -1,13 +1,17 @@ +import copy +from fs.errors import ResourceNotFoundError import logging import os from lxml import etree from xmodule.x_module import XModule -from xmodule.raw_module import RawDescriptor +from xmodule.xml_module import XmlDescriptor +from xmodule.editing_module import EditingDescriptor +from stringify import stringify_children +from html_checker import check_html log = logging.getLogger("mitx.courseware") - class HtmlModule(XModule): def get_html(self): return self.html @@ -19,33 +23,108 @@ class HtmlModule(XModule): self.html = self.definition['data'] -class HtmlDescriptor(RawDescriptor): +class HtmlDescriptor(XmlDescriptor, EditingDescriptor): """ Module for putting raw html in a course """ mako_template = "widgets/html-edit.html" module_class = HtmlModule - filename_extension = "html" + filename_extension = "xml" - # TODO (cpennington): Delete this method once all fall 2012 course are being - # edited in the cms + # VS[compat] TODO (cpennington): Delete this method once all fall 2012 course + # are being edited in the cms @classmethod def backcompat_paths(cls, path): - if path.endswith('.html.html'): - path = path[:-5] + origpath = path + if path.endswith('.html.xml'): + path = path[:-9] + '.html' #backcompat--look for html instead of xml candidates = [] while os.sep in path: candidates.append(path) _, _, path = path.partition(os.sep) + # also look for .html versions instead of .xml + if origpath.endswith('.xml'): + candidates.append(origpath[:-4] + '.html') return candidates + # NOTE: html descriptors are special. We do not want to parse and + # export them ourselves, because that can break things (e.g. lxml + # adds body tags when it exports, but they should just be html + # snippets that will be included in the middle of pages. + @classmethod - def file_to_xml(cls, file_object): - parser = etree.HTMLParser() - return etree.parse(file_object, parser).getroot() + def definition_loader(cls, xml_object, system): + '''Load a descriptor from the specified xml_object: + + If there is a filename attribute, load it as a string, and + log a warning if it is not parseable by etree.HTMLParser. + + If there is not a filename attribute, the definition is the body + of the xml_object, without the root tag (do not want in the + middle of a page) + ''' + filename = xml_object.get('filename') + if filename is None: + definition_xml = copy.deepcopy(xml_object) + cls.clean_metadata_from_xml(definition_xml) + return {'data' : stringify_children(definition_xml)} + else: + filepath = cls._format_filepath(xml_object.tag, filename) + + # VS[compat] + # TODO (cpennington): If the file doesn't exist at the right path, + # give the class a chance to fix it up. The file will be written out + # again in the correct format. This should go away once the CMS is + # online and has imported all current (fall 2012) courses from xml + if not system.resources_fs.exists(filepath): + candidates = cls.backcompat_paths(filepath) + #log.debug("candidates = {0}".format(candidates)) + for candidate in candidates: + if system.resources_fs.exists(candidate): + filepath = candidate + break + + try: + with system.resources_fs.open(filepath) as file: + html = file.read() + # Log a warning if we can't parse the file, but don't error + if not check_html(html): + log.warning("Couldn't parse html in {0}.".format(filepath)) + return {'data' : html} + except (ResourceNotFoundError) as err: + msg = 'Unable to load file contents at path {0}: {1} '.format( + filepath, err) + log.error(msg) + raise @classmethod def split_to_file(cls, xml_object): - # never include inline html + '''Never include inline html''' return True + + + # TODO (vshnayder): make export put things in the right places. + + def definition_to_xml(self, resource_fs): + '''If the contents are valid xml, write them to filename.xml. Otherwise, + write just the tag to filename.xml, and the html + string to filename.html. + ''' + try: + return etree.fromstring(self.definition['data']) + except etree.XMLSyntaxError: + pass + + # Not proper format. Write html to file, return an empty tag + filepath = u'{category}/{name}.html'.format(category=self.category, + name=self.name) + + resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) + with resource_fs.open(filepath, 'w') as file: + file.write(self.definition['data']) + + elt = etree.Element('html') + elt.set("filename", self.name) + return elt + diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 5786dbd227..188df21130 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -162,6 +162,52 @@ class XmlDescriptor(XModuleDescriptor): """ return etree.parse(file_object).getroot() + @classmethod + def definition_loader(cls, xml_object, system, location): + '''Load a descriptor definition from the specified xml_object. + Subclasses should not need to override this except in special + cases (e.g. html module)''' + + filename = xml_object.get('filename') + if filename is None: + definition_xml = copy.deepcopy(xml_object) + else: + filepath = cls._format_filepath(xml_object.tag, filename) + + # VS[compat] + # TODO (cpennington): If the file doesn't exist at the right path, + # give the class a chance to fix it up. The file will be written out again + # in the correct format. + # This should go away once the CMS is online and has imported all current (fall 2012) + # courses from xml + if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'): + candidates = cls.backcompat_paths(filepath) + for candidate in candidates: + if system.resources_fs.exists(candidate): + filepath = candidate + break + + try: + with system.resources_fs.open(filepath) as file: + definition_xml = cls.file_to_xml(file) + except (ResourceNotFoundError, etree.XMLSyntaxError): + msg = 'Unable to load file contents at path %s for item %s' % ( + filepath, location.url()) + + log.exception(msg) + system.error_handler(msg) + # if error_handler didn't reraise, work around problem. + error_elem = etree.Element('error') + message_elem = etree.SubElement(error_elem, 'error_message') + message_elem.text = msg + stack_elem = etree.SubElement(error_elem, 'stack_trace') + stack_elem.text = traceback.format_exc() + return {'data': etree.tostring(error_elem)} + + cls.clean_metadata_from_xml(definition_xml) + return cls.definition_from_xml(definition_xml, system) + + @classmethod def from_xml(cls, xml_data, system, org=None, course=None): """ @@ -191,44 +237,8 @@ class XmlDescriptor(XModuleDescriptor): metadata[attr_map.metadata_key] = attr_map.to_metadata(val) return metadata - def definition_loader(): - filename = xml_object.get('filename') - if filename is None: - definition_xml = copy.deepcopy(xml_object) - else: - filepath = cls._format_filepath(xml_object.tag, filename) - - # VS[compat] - # TODO (cpennington): If the file doesn't exist at the right path, - # give the class a chance to fix it up. The file will be written out again - # in the correct format. - # This should go away once the CMS is online and has imported all current (fall 2012) - # courses from xml - if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'): - candidates = cls.backcompat_paths(filepath) - for candidate in candidates: - if system.resources_fs.exists(candidate): - filepath = candidate - break - - try: - with system.resources_fs.open(filepath) as file: - definition_xml = cls.file_to_xml(file) - except (ResourceNotFoundError, etree.XMLSyntaxError): - msg = 'Unable to load file contents at path %s for item %s' % (filepath, location.url()) - log.exception(msg) - system.error_handler(msg) - # if error_handler didn't reraise, work around problem. - error_elem = etree.Element('error') - message_elem = etree.SubElement(error_elem, 'error_message') - message_elem.text = msg - stack_elem = etree.SubElement(error_elem, 'stack_trace') - stack_elem.text = traceback.format_exc() - return {'data': etree.tostring(error_elem)} - - cls.clean_metadata_from_xml(definition_xml) - return cls.definition_from_xml(definition_xml, system) - + definition = cls.definition_loader(xml_object, system, location) + metadata = metadata_loader() # VS[compat] -- just have the url_name lookup once translation is done slug = xml_object.get('url_name', xml_object.get('slug')) return cls( @@ -283,7 +293,7 @@ class XmlDescriptor(XModuleDescriptor): # Write it to a file if necessary if self.split_to_file(xml_object): - # Put this object in it's own file + # Put this object in its own file filepath = self.__class__._format_filepath(self.category, self.name) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) with resource_fs.open(filepath, 'w') as file: diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 7523fd8373..41f0c39cde 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -143,6 +143,7 @@ def check_roundtrip(course_dir): # dircmp doesn't do recursive diffs. # diff = dircmp(course_dir, export_dir, ignore=[], hide=[]) print "======== Roundtrip diff: =========" + sys.stdout.flush() # needed to make diff appear in the right place os.system("diff -r {0} {1}".format(course_dir, export_dir)) print "======== ideally there is no diff above this =======" From 0ae434cc092617b970fee3968084c378d609c4f2 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 11:24:39 -0400 Subject: [PATCH 10/28] Move path_to_location out of mongo.py * also bugfix for load_definition in html_module * a bit of refactoring of Location checking code in mongo.py --- common/lib/xmodule/xmodule/html_module.py | 2 +- .../xmodule/xmodule/modulestore/__init__.py | 47 +++---- .../lib/xmodule/xmodule/modulestore/mongo.py | 120 +++--------------- .../lib/xmodule/xmodule/modulestore/search.py | 96 ++++++++++++++ .../xmodule/modulestore/tests/test_mongo.py | 15 +-- common/lib/xmodule/xmodule/xml_module.py | 8 +- lms/djangoapps/courseware/views.py | 3 +- 7 files changed, 155 insertions(+), 136 deletions(-) create mode 100644 common/lib/xmodule/xmodule/modulestore/search.py diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 22d3bf163f..996d3c4ead 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -54,7 +54,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # snippets that will be included in the middle of pages. @classmethod - def definition_loader(cls, xml_object, system): + def load_definition(cls, xml_object, system, location): '''Load a descriptor from the specified xml_object: If there is a filename attribute, load it as a string, and diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 279782b61a..a0167d781b 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -5,7 +5,7 @@ that are stored in a database an accessible using their Location as an identifie import re from collections import namedtuple -from .exceptions import InvalidLocationError +from .exceptions import InvalidLocationError, InsufficientSpecificationError import logging log = logging.getLogger('mitx.' + 'modulestore') @@ -38,15 +38,15 @@ class Location(_LocationBase): ''' __slots__ = () - @classmethod - def clean(cls, value): + @staticmethod + def clean(value): """ Return value, made into a form legal for locations """ return re.sub('_+', '_', INVALID_CHARS.sub('_', value)) - @classmethod - def is_valid(cls, value): + @staticmethod + def is_valid(value): ''' Check if the value is a valid location, in any acceptable format. ''' @@ -56,6 +56,21 @@ class Location(_LocationBase): return False return True + @staticmethod + def ensure_fully_specified(location): + '''Make sure location is valid, and fully specified. Raises + InvalidLocationError or InsufficientSpecificationError if not. + + returns a Location object corresponding to location. + ''' + loc = Location(location) + for key, val in loc.dict().iteritems(): + if key != 'revision' and val is None: + raise InsufficientSpecificationError(location) + return loc + + + def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None, name=None, revision=None): """ @@ -254,25 +269,11 @@ class ModuleStore(object): ''' raise NotImplementedError - def path_to_location(self, location, course=None, chapter=None, section=None): - ''' - Try to find a course/chapter/section[/position] path to this location. + def get_parent_locations(self, location): + '''Find all locations that are the parents of this location. Needed + for path_to_location(). - raise ItemNotFoundError if the location doesn't exist. - - If course, chapter, section are not None, restrict search to paths with those - components as specified. - - raise NoPathToItem if the location exists, but isn't accessible via - a path that matches the course/chapter/section restrictions. - - In general, a location may be accessible via many paths. This method may - return any valid path. - - Return a tuple (course, chapter, section, position). - - If the section a sequence, position should be the position of this location - in that sequence. Otherwise, position should be None. + returns an iterable of things that can be passed to Location. ''' raise NotImplementedError diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index df4e20f3a7..061e2aafe9 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -9,11 +9,10 @@ from importlib import import_module from xmodule.errorhandlers import strict_error_handler from xmodule.x_module import XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem -from xmodule.course_module import CourseDescriptor from mitxmako.shortcuts import render_to_string from . import ModuleStore, Location -from .exceptions import (ItemNotFoundError, InsufficientSpecificationError, +from .exceptions import (ItemNotFoundError, NoPathToItem, DuplicateItemError) # TODO (cpennington): This code currently operates under the assumption that @@ -172,12 +171,17 @@ class MongoModuleStore(ModuleStore): return self.get_items(course_filter) def _find_one(self, location): - '''Look for a given location in the collection. - If revision isn't specified, returns the latest.''' - return self.collection.find_one( + '''Look for a given location in the collection. If revision is not + specified, returns the latest. If the item is not present, raise + ItemNotFoundError. + ''' + item = self.collection.find_one( location_to_query(location), sort=[('revision', pymongo.ASCENDING)], ) + if item is None: + raise ItemNotFoundError(location) + return item def get_item(self, location, depth=0): """ @@ -197,14 +201,8 @@ class MongoModuleStore(ModuleStore): calls to get_children() to cache. None indicates to cache all descendents. """ - - for key, val in Location(location).dict().iteritems(): - if key != 'revision' and val is None: - raise InsufficientSpecificationError(location) - + location = Location.ensure_fully_specified(location) item = self._find_one(location) - if item is None: - raise ItemNotFoundError(location) return self._load_items([item], depth)[0] def get_items(self, location, depth=0): @@ -282,96 +280,20 @@ class MongoModuleStore(ModuleStore): ) def get_parent_locations(self, location): - '''Find all locations that are the parents of this location. - Mostly intended for use in path_to_location, but exposed for testing - and possible other usefulness. + '''Find all locations that are the parents of this location. Needed + for path_to_location(). - returns an iterable of things that can be passed to Location. + If there is no data at location in this modulestore, raise + ItemNotFoundError. + + returns an iterable of things that can be passed to Location. This may + be empty if there are no parents. ''' - location = Location(location) + location = Location.ensure_fully_specified(location) + # Check that it's actually in this modulestore. + item = self._find_one(location) + # now get the parents items = self.collection.find({'definition.children': str(location)}, {'_id': True}) return [i['_id'] for i in items] - def path_to_location(self, location, course_name=None): - ''' - Try to find a course_id/chapter/section[/position] path to this location. - The courseware insists that the first level in the course is chapter, - but any kind of module can be a "section". - - location: something that can be passed to Location - course_name: [optional]. If not None, restrict search to paths - in that course. - - raise ItemNotFoundError if the location doesn't exist. - - raise NoPathToItem if the location exists, but isn't accessible via - a chapter/section path in the course(s) being searched. - - Return a tuple (course_id, chapter, section, position) suitable for the - courseware index view. - - A location may be accessible via many paths. This method may - return any valid path. - - If the section is a sequence, position will be the position - of this location in that sequence. Otherwise, position will - be None. TODO (vshnayder): Not true yet. - ''' - # Check that location is present at all - if self._find_one(location) is None: - raise ItemNotFoundError(location) - - def flatten(xs): - '''Convert lisp-style (a, (b, (c, ()))) lists into a python list. - Not a general flatten function. ''' - p = [] - while xs != (): - p.append(xs[0]) - xs = xs[1] - return p - - def find_path_to_course(location, course_name=None): - '''Find a path up the location graph to a node with the - specified category. If no path exists, return None. If a - path exists, return it as a list with target location - first, and the starting location last. - ''' - # Standard DFS - - # To keep track of where we came from, the work queue has - # tuples (location, path-so-far). To avoid lots of - # copying, the path-so-far is stored as a lisp-style - # list--nested hd::tl tuples, and flattened at the end. - queue = [(location, ())] - while len(queue) > 0: - (loc, path) = queue.pop() # Takes from the end - loc = Location(loc) - # print 'Processing loc={0}, path={1}'.format(loc, path) - if loc.category == "course": - if course_name is None or course_name == loc.name: - # Found it! - path = (loc, path) - return flatten(path) - - # otherwise, add parent locations at the end - newpath = (loc, path) - parents = self.get_parent_locations(loc) - queue.extend(zip(parents, repeat(newpath))) - - # If we're here, there is no path - return None - - path = find_path_to_course(location, course_name) - if path is None: - raise(NoPathToItem(location)) - - n = len(path) - course_id = CourseDescriptor.location_to_id(path[0]) - chapter = path[1].name if n > 1 else None - section = path[2].name if n > 2 else None - - # TODO (vshnayder): not handling position at all yet... - position = None - - return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py new file mode 100644 index 0000000000..a383b3f8ec --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -0,0 +1,96 @@ +from itertools import repeat + +from xmodule.course_module import CourseDescriptor + +from .exceptions import (ItemNotFoundError, NoPathToItem) +from . import ModuleStore, Location + + +def path_to_location(modulestore, location, course_name=None): + ''' + Try to find a course_id/chapter/section[/position] path to location in + modulestore. The courseware insists that the first level in the course is + chapter, but any kind of module can be a "section". + + location: something that can be passed to Location + course_name: [optional]. If not None, restrict search to paths + in that course. + + raise ItemNotFoundError if the location doesn't exist. + + raise NoPathToItem if the location exists, but isn't accessible via + a chapter/section path in the course(s) being searched. + + Return a tuple (course_id, chapter, section, position) suitable for the + courseware index view. + + A location may be accessible via many paths. This method may + return any valid path. + + If the section is a sequence, position will be the position + of this location in that sequence. Otherwise, position will + be None. TODO (vshnayder): Not true yet. + ''' + + def flatten(xs): + '''Convert lisp-style (a, (b, (c, ()))) list into a python list. + Not a general flatten function. ''' + p = [] + while xs != (): + p.append(xs[0]) + xs = xs[1] + return p + + def find_path_to_course(location, course_name=None): + '''Find a path up the location graph to a node with the + specified category. + + If no path exists, return None. + + If a path exists, return it as a list with target location first, and + the starting location last. + ''' + # Standard DFS + + # To keep track of where we came from, the work queue has + # tuples (location, path-so-far). To avoid lots of + # copying, the path-so-far is stored as a lisp-style + # list--nested hd::tl tuples, and flattened at the end. + queue = [(location, ())] + while len(queue) > 0: + (loc, path) = queue.pop() # Takes from the end + loc = Location(loc) + + # get_parent_locations should raise ItemNotFoundError if location + # isn't found so we don't have to do it explicitly. Call this + # first to make sure the location is there (even if it's a course, and + # we would otherwise immediately exit). + parents = modulestore.get_parent_locations(loc) + + # print 'Processing loc={0}, path={1}'.format(loc, path) + if loc.category == "course": + if course_name is None or course_name == loc.name: + # Found it! + path = (loc, path) + return flatten(path) + + # otherwise, add parent locations at the end + newpath = (loc, path) + queue.extend(zip(parents, repeat(newpath))) + + # If we're here, there is no path + return None + + path = find_path_to_course(location, course_name) + if path is None: + raise(NoPathToItem(location)) + + n = len(path) + course_id = CourseDescriptor.location_to_id(path[0]) + chapter = path[1].name if n > 1 else None + section = path[2].name if n > 2 else None + + # TODO (vshnayder): not handling position at all yet... + position = None + + return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index cb2bc6e20c..24f0441ee0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -8,6 +8,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem from xmodule.modulestore.mongo import MongoModuleStore from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.search import path_to_location # from ~/mitx_all/mitx/common/lib/xmodule/xmodule/modulestore/tests/ # to ~/mitx_all/mitx/common/test @@ -28,7 +29,7 @@ DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' class TestMongoModuleStore(object): - + '''Tests!''' @classmethod def setupClass(cls): cls.connection = pymongo.connection.Connection(HOST, PORT) @@ -67,7 +68,7 @@ class TestMongoModuleStore(object): def test_init(self): '''Make sure the db loads, and print all the locations in the db. - Call this directly from failing tests to see what's loaded''' + Call this directly from failing tests to see what is loaded''' ids = list(self.connection[DB][COLLECTION].find({}, {'_id': True})) pprint([Location(i['_id']).url() for i in ids]) @@ -93,8 +94,6 @@ class TestMongoModuleStore(object): self.store.get_item("i4x://edX/toy/video/Welcome"), None) - - def test_find_one(self): assert_not_equals( self.store._find_one(Location("i4x://edX/toy/course/2012_Fall")), @@ -117,13 +116,13 @@ class TestMongoModuleStore(object): ("edX/toy/2012_Fall", "Overview", "Toy_Videos", None)), ) for location, expected in should_work: - assert_equals(self.store.path_to_location(location), expected) + assert_equals(path_to_location(self.store, location), expected) not_found = ( - "i4x://edX/toy/video/WelcomeX", + "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome" ) for location in not_found: - assert_raises(ItemNotFoundError, self.store.path_to_location, location) + assert_raises(ItemNotFoundError, path_to_location, self.store, location) # Since our test files are valid, there shouldn't be any # elements with no path to them. But we can look for them in @@ -132,5 +131,5 @@ class TestMongoModuleStore(object): "i4x://edX/simple/video/Lost_Video", ) for location in no_path: - assert_raises(NoPathToItem, self.store.path_to_location, location, "toy") + assert_raises(NoPathToItem, path_to_location, self.store, location, "toy") diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 188df21130..de62bc5d38 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -163,7 +163,7 @@ class XmlDescriptor(XModuleDescriptor): return etree.parse(file_object).getroot() @classmethod - def definition_loader(cls, xml_object, system, location): + def load_definition(cls, xml_object, system, location): '''Load a descriptor definition from the specified xml_object. Subclasses should not need to override this except in special cases (e.g. html module)''' @@ -225,7 +225,7 @@ class XmlDescriptor(XModuleDescriptor): slug = xml_object.get('url_name', xml_object.get('slug')) location = Location('i4x', org, course, xml_object.tag, slug) - def metadata_loader(): + def load_metadata(): metadata = {} for attr in cls.metadata_attributes: val = xml_object.get(attr) @@ -237,8 +237,8 @@ class XmlDescriptor(XModuleDescriptor): metadata[attr_map.metadata_key] = attr_map.to_metadata(val) return metadata - definition = cls.definition_loader(xml_object, system, location) - metadata = metadata_loader() + definition = cls.load_definition(xml_object, system, location) + metadata = load_metadata() # VS[compat] -- just have the url_name lookup once translation is done slug = xml_object.get('url_name', xml_object.get('slug')) return cls( diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 7281ab01ad..a52f715efd 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -20,6 +20,7 @@ from module_render import toc_for_course, get_module, get_section from models import StudentModuleCache from student.models import UserProfile from xmodule.modulestore import Location +from xmodule.modulestore.search import path_to_location from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor @@ -233,7 +234,7 @@ def jump_to(request, location): # Complain if there's not data for this location try: - (course_id, chapter, section, position) = modulestore().path_to_location(location) + (course_id, chapter, section, position) = path_to_location(modulestore(), location) except ItemNotFoundError: raise Http404("No data at this location: {0}".format(location)) except NoPathToItem: From 0edc40de34f37c3322e440656e71d7dbe77c202a Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 11:25:10 -0400 Subject: [PATCH 11/28] Address minor comments in #313 --- common/lib/xmodule/xmodule/modulestore/mongo.py | 2 +- common/lib/xmodule/xmodule/x_module.py | 3 --- .../courseware/management/commands/clean_xml.py | 3 --- lms/djangoapps/courseware/tests/tests.py | 14 ++++++++------ 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 061e2aafe9..6a0bdad06a 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -293,7 +293,7 @@ class MongoModuleStore(ModuleStore): # Check that it's actually in this modulestore. item = self._find_one(location) # now get the parents - items = self.collection.find({'definition.children': str(location)}, + items = self.collection.find({'definition.children': location.url()}, {'_id': True}) return [i['_id'] for i in items] diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 8803c37d78..c4ba3e1e58 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -194,9 +194,6 @@ class XModule(HTMLSnippet): self.metadata = kwargs.get('metadata', {}) self._loaded_children = None - def get_name(self): - return self.name - def get_children(self): ''' Return module instances for all the children of this module. diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 41f0c39cde..05865bb289 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -17,9 +17,6 @@ def traverse_tree(course): queue = [course] while len(queue) > 0: node = queue.pop() -# print '{0}:'.format(node.location) -# if 'data' in node.definition: -# print '{0}'.format(node.definition['data']) queue.extend(node.get_children()) return True diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 8e9d13f8d5..0e2f7d7aa5 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -1,19 +1,20 @@ import copy import json +from path import path import os from pprint import pprint +from nose import SkipTest from django.test import TestCase from django.test.client import Client -from mock import patch, Mock -from override_settings import override_settings from django.conf import settings from django.core.urlresolvers import reverse -from path import path +from mock import patch, Mock +from override_settings import override_settings -from student.models import Registration from django.contrib.auth.models import User +from student.models import Registration from xmodule.modulestore.django import modulestore import xmodule.modulestore.django @@ -189,11 +190,12 @@ class RealCoursesLoadTestCase(PageLoader): xmodule.modulestore.django._MODULESTORES = {} xmodule.modulestore.django.modulestore().collection.drop() - # TODO: Disabled test for now.. Fix once things are cleaned up. - def Xtest_real_courses_loads(self): + def test_real_courses_loads(self): '''See if any real courses are available at the REAL_DATA_DIR. If they are, check them.''' + # TODO: Disabled test for now.. Fix once things are cleaned up. + raise SkipTest # TODO: adjust staticfiles_dirs if not os.path.isdir(REAL_DATA_DIR): # No data present. Just pass. From 707551b08d25bcf92572a323176aeeec0398aead Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 11:26:02 -0400 Subject: [PATCH 12/28] local variable naming tweak --- common/lib/xmodule/xmodule/modulestore/xml.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 8a084b19ee..b54963b513 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -70,18 +70,19 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): # log.debug('-> slug=%s' % slug) xml_data.set('url_name', slug) - module = XModuleDescriptor.load_from_xml( + descriptor = XModuleDescriptor.load_from_xml( etree.tostring(xml_data), self, org, course, xmlstore.default_class) - #log.debug('==> importing module location %s' % repr(module.location)) - module.metadata['data_dir'] = course_dir + #log.debug('==> importing descriptor location %s' % + # repr(descriptor.location)) + descriptor.metadata['data_dir'] = course_dir - xmlstore.modules[module.location] = module + xmlstore.modules[descriptor.location] = descriptor if xmlstore.eager: - module.get_children() - return module + descriptor.get_children() + return descriptor render_template = lambda: '' load_item = xmlstore.get_item From c0cdff7071431fb522a05b596bd0aa07232d1584 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 13:44:04 -0400 Subject: [PATCH 13/28] Rename MalformedDescriptor to ErrorDescriptor * change references and tests * add staff/non-staff display * added is_staff to ModuleSystem --- cms/djangoapps/contentstore/views.py | 5 ++- common/lib/xmodule/setup.py | 2 +- common/lib/xmodule/tests/__init__.py | 3 +- common/lib/xmodule/tests/test_import.py | 12 +++--- .../{malformed_module.py => error_module.py} | 41 +++++++++++-------- common/lib/xmodule/xmodule/x_module.py | 26 ++++++++---- lms/djangoapps/courseware/module_render.py | 1 + 7 files changed, 58 insertions(+), 32 deletions(-) rename common/lib/xmodule/xmodule/{malformed_module.py => error_module.py} (58%) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 6d5b2117c4..111c91e03a 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -214,7 +214,10 @@ def preview_module_system(request, preview_id, descriptor): get_module=partial(get_preview_module, request, preview_id), render_template=render_from_lms, debug=True, - replace_urls=replace_urls + replace_urls=replace_urls, + # TODO (vshnayder): CMS users won't see staff view unless they are CMS staff. + # is that what we want? + is_staff=request.user.is_staff ) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 09a6b7e712..8a0a6bb139 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -25,7 +25,7 @@ setup( "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor", "html = xmodule.html_module:HtmlDescriptor", "image = xmodule.backcompat_module:TranslateCustomTagDescriptor", - "malformed = xmodule.malformed_module:MalformedDescriptor", + "error = xmodule.error_module:ErrorDescriptor", "problem = xmodule.capa_module:CapaDescriptor", "problemset = xmodule.vertical_module:VerticalDescriptor", "section = xmodule.backcompat_module:SemanticSectionDescriptor", diff --git a/common/lib/xmodule/tests/__init__.py b/common/lib/xmodule/tests/__init__.py index 94bf1da65e..7f6fcfe00c 100644 --- a/common/lib/xmodule/tests/__init__.py +++ b/common/lib/xmodule/tests/__init__.py @@ -31,7 +31,8 @@ i4xs = ModuleSystem( user=Mock(), filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))), debug=True, - xqueue_callback_url='/' + xqueue_callback_url='/', + is_staff=False ) diff --git a/common/lib/xmodule/tests/test_import.py b/common/lib/xmodule/tests/test_import.py index dd55f8ff38..497354a18e 100644 --- a/common/lib/xmodule/tests/test_import.py +++ b/common/lib/xmodule/tests/test_import.py @@ -33,7 +33,7 @@ class ImportTestCase(unittest.TestCase): return system def test_fallback(self): - '''Make sure that malformed xml loads as a MalformedDescriptorb.''' + '''Make sure that malformed xml loads as an ErrorDescriptor.''' bad_xml = '''''' @@ -43,10 +43,10 @@ class ImportTestCase(unittest.TestCase): None) self.assertEqual(descriptor.__class__.__name__, - 'MalformedDescriptor') + 'ErrorDescriptor') def test_reimport(self): - '''Make sure an already-exported malformed xml tag loads properly''' + '''Make sure an already-exported error xml tag loads properly''' bad_xml = '''''' system = self.get_system() @@ -58,7 +58,7 @@ class ImportTestCase(unittest.TestCase): 'org', 'course', None) self.assertEqual(re_import_descriptor.__class__.__name__, - 'MalformedDescriptor') + 'ErrorDescriptor') self.assertEqual(descriptor.definition['data'], re_import_descriptor.definition['data']) @@ -66,8 +66,8 @@ class ImportTestCase(unittest.TestCase): def test_fixed_xml_tag(self): """Make sure a tag that's been fixed exports as the original tag type""" - # create a malformed tag with valid xml contents - root = etree.Element('malformed') + # create a error tag with valid xml contents + root = etree.Element('error') good_xml = '''''' root.text = good_xml diff --git a/common/lib/xmodule/xmodule/malformed_module.py b/common/lib/xmodule/xmodule/error_module.py similarity index 58% rename from common/lib/xmodule/xmodule/malformed_module.py rename to common/lib/xmodule/xmodule/error_module.py index ac419fff26..ee05e9c879 100644 --- a/common/lib/xmodule/xmodule/malformed_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -9,18 +9,26 @@ import logging log = logging.getLogger(__name__) -class MalformedModule(XModule): +class ErrorModule(XModule): def get_html(self): '''Show an error. TODO (vshnayder): proper style, divs, etc. ''' - return "Malformed content--not showing through get_html()" + if not self.system.is_staff: + return self.system.render_template('module-error.html') -class MalformedDescriptor(EditingDescriptor): + # 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') + }) + +class ErrorDescriptor(EditingDescriptor): """ Module that provides a raw editing view of broken xml. """ - module_class = MalformedModule + module_class = ErrorModule @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -29,38 +37,39 @@ class MalformedDescriptor(EditingDescriptor): Does not try to parse the data--just stores it. ''' + definition = {} try: - # If this is already a malformed tag, don't want to re-wrap it. + # If this is already an error tag, don't want to re-wrap it. xml_obj = etree.fromstring(xml_data) - if xml_obj.tag == 'malformed': + if xml_obj.tag == 'error': xml_data = xml_obj.text - # TODO (vshnayder): how does one get back from this to a valid descriptor? - # For now, have to fix manually. - except etree.XMLSyntaxError: - pass + except etree.XMLSyntaxError as err: + # Save the error to display later + definition['error'] = str(err) - definition = { 'data' : xml_data } - # TODO (vshnayder): Do we need a valid slug here? Just pick a random + + definition['data'] = xml_data + # TODO (vshnayder): Do we need a unique slug here? Just pick a random # 64-bit num? - location = ['i4x', org, course, 'malformed', 'slug'] + location = ['i4x', org, course, 'error', 'slug'] metadata = {} # stays in the xml_data return cls(system, definition, location=location, metadata=metadata) def export_to_xml(self, resource_fs): ''' - If the definition data is invalid xml, export it wrapped in a malformed + If the definition data is invalid xml, export it wrapped in an "error" tag. If it is valid, export without the wrapper. NOTE: There may still be problems with the valid xml--it could be missing required attributes, could have the wrong tags, refer to missing - files, etc. + files, etc. That would just get re-wrapped on import. ''' try: xml = etree.fromstring(self.definition['data']) return etree.tostring(xml) except etree.XMLSyntaxError: # still not valid. - root = etree.Element('malformed') + root = etree.Element('error') root.text = self.definition['data'] return etree.tostring(root) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index c4ba3e1e58..2d7447b5f6 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -455,14 +455,14 @@ class XModuleDescriptor(Plugin, HTMLSnippet): descriptor = class_.from_xml(xml_data, system, org, course) except (ResourceNotFoundError, XMLSyntaxError) as err: - # Didn't load properly. Fall back on loading as a malformed + # Didn't load properly. Fall back on loading as an error # descriptor. This should never error due to formatting. # Put import here to avoid circular import errors - from xmodule.malformed_module import MalformedDescriptor + from xmodule.error_module import ErrorDescriptor #system.error_handler("Error loading from xml.") - descriptor = MalformedDescriptor.from_xml(xml_data, system, org, course) + descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course) return descriptor @@ -597,10 +597,18 @@ class ModuleSystem(object): Note that these functions can be closures over e.g. a django request and user, or other environment-specific info. ''' - def __init__(self, ajax_url, track_function, - get_module, render_template, replace_urls, - user=None, filestore=None, debug=False, - xqueue_callback_url=None, xqueue_default_queuename="null"): + def __init__(self, + ajax_url, + track_function, + get_module, + render_template, + replace_urls, + user=None, + filestore=None, + debug=False, + xqueue_callback_url=None, + xqueue_default_queuename="null", + is_staff=False): ''' Create a closure around the system environment. @@ -626,6 +634,9 @@ class ModuleSystem(object): replace_urls - TEMPORARY - A function like static_replace.replace_urls that capa_module can use to fix up the static urls in ajax results. + + is_staff - Is the user making the request a staff user? + TODO (vshnayder): this will need to change once we have real user roles. ''' self.ajax_url = ajax_url self.xqueue_callback_url = xqueue_callback_url @@ -637,6 +648,7 @@ class ModuleSystem(object): self.DEBUG = self.debug = debug self.seed = user.id if user is not None else 0 self.replace_urls = replace_urls + self.is_staff = is_staff def get(self, attr): ''' provide uniform access to attributes (like etree).''' diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 80a4ef90fc..1ff39f602e 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -172,6 +172,7 @@ def get_module(user, request, location, student_module_cache, position=None): # a module is coming through get_html and is therefore covered # by the replace_static_urls code below replace_urls=replace_urls, + is_staff=user.is_staff, ) # pass position specified in URL to module through ModuleSystem system.set('position', position) From e2e524453faa235ac90fac008839d6175dbb7379 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 13:44:40 -0400 Subject: [PATCH 14/28] Make courseware index view more bulletproof --- lms/djangoapps/courseware/views.py | 68 +++++++++++++++++++----------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index a52f715efd..fb4d2942f9 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -184,34 +184,54 @@ def index(request, course_id, chapter=None, section=None, chapter = clean(chapter) section = clean(section) - context = { - 'csrf': csrf(request)['csrf_token'], - 'accordion': render_accordion(request, course, chapter, section), - 'COURSE_TITLE': course.title, - 'course': course, - 'init': '', - 'content': '' - } + try: + context = { + 'csrf': csrf(request)['csrf_token'], + 'accordion': render_accordion(request, course, chapter, section), + 'COURSE_TITLE': course.title, + 'course': course, + 'init': '', + 'content': '' + } - look_for_module = chapter is not None and section is not None - if look_for_module: - # TODO (cpennington): Pass the right course in here + look_for_module = chapter is not None and section is not None + if look_for_module: + # TODO (cpennington): Pass the right course in here - section_descriptor = get_section(course, chapter, section) - if section_descriptor is not None: - student_module_cache = StudentModuleCache(request.user, - section_descriptor) - module, _, _, _ = get_module(request.user, request, - section_descriptor.location, - student_module_cache) - context['content'] = module.get_html() + section_descriptor = get_section(course, chapter, section) + if section_descriptor is not None: + student_module_cache = StudentModuleCache(request.user, + section_descriptor) + module, _, _, _ = get_module(request.user, request, + section_descriptor.location, + student_module_cache) + context['content'] = module.get_html() + else: + log.warning("Couldn't find a section descriptor for course_id '{0}'," + "chapter '{1}', section '{2}'".format( + course_id, chapter, section)) + + + result = render_to_response('courseware.html', context) + except: + # In production, don't want to let a 500 out for any reason + if settings.DEBUG: + raise else: - log.warning("Couldn't find a section descriptor for course_id '{0}'," - "chapter '{1}', section '{2}'".format( - course_id, chapter, section)) + log.exception("Error in index view: user={user}, course={course}," + " chapter={chapter} section={section}" + "position={position}".format( + user=request.user, + course=course, + chapter=chapter, + section=section, + position=position + )) + try: + result = render_to_response('courseware-error.html', {}) + except: + result = HttpResponse("There was an unrecoverable error") - - result = render_to_response('courseware.html', context) return result @ensure_csrf_cookie From bc14441a75c010dd98c9389ad9d83015f91cc091 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 13:45:13 -0400 Subject: [PATCH 15/28] add template I forgot --- lms/templates/module-error-staff.html | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 lms/templates/module-error-staff.html diff --git a/lms/templates/module-error-staff.html b/lms/templates/module-error-staff.html new file mode 100644 index 0000000000..955f413db3 --- /dev/null +++ b/lms/templates/module-error-staff.html @@ -0,0 +1,11 @@ +
+

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.

+ +

Staff-only details below:

+ +

Error: ${error}

+ +

Raw data: ${data}

+ +
From 119ab639d0c816889a59c36540212eb1daed33fd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:12:00 -0400 Subject: [PATCH 16/28] make cms users look like staff by default * so they get the staff view of problems * does NOT actually set User.is_staff --- cms/djangoapps/contentstore/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 111c91e03a..0fccc2498b 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -215,9 +215,9 @@ def preview_module_system(request, preview_id, descriptor): render_template=render_from_lms, debug=True, replace_urls=replace_urls, - # TODO (vshnayder): CMS users won't see staff view unless they are CMS staff. + # TODO (vshnayder): All CMS users get staff view by default # is that what we want? - is_staff=request.user.is_staff + is_staff=True, ) From 58543bd84b451c1da7497633e9550a3d6538e0c1 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:12:47 -0400 Subject: [PATCH 17/28] definition and metadata no longer lazy-loaded * get rid of dump to json * formatting --- common/djangoapps/xmodule_modifiers.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index d0d1e0be15..b942d6726b 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -76,8 +76,9 @@ def add_histogram(get_html, module): histogram = grade_histogram(module_id) render_histogram = len(histogram) > 0 - # TODO: fixme - no filename in module.xml in general (this code block for edx4edx) - # the following if block is for summer 2012 edX course development; it will change when the CMS comes online + # TODO: fixme - no filename in module.xml in general (this code block + # for edx4edx) the following if block is for summer 2012 edX course + # development; it will change when the CMS comes online if settings.MITX_FEATURES.get('DISPLAY_EDIT_LINK') and settings.DEBUG and module_xml.get('filename') is not None: coursename = multicourse_settings.get_coursename_from_request(request) github_url = multicourse_settings.get_course_github_url(coursename) @@ -88,10 +89,8 @@ def add_histogram(get_html, module): else: edit_link = False - # Cast module.definition and module.metadata to dicts so that json can dump them - # even though they are lazily loaded - staff_context = {'definition': json.dumps(dict(module.definition), indent=4), - 'metadata': json.dumps(dict(module.metadata), indent=4), + staff_context = {'definition': dict(module.definition), + 'metadata': dict(module.metadata), 'element_id': module.location.html_id(), 'edit_link': edit_link, 'histogram': json.dumps(histogram), From 009bd230667bb722988e6aec69972ada6121d57d Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:13:49 -0400 Subject: [PATCH 18/28] save LazyLoadingDict in case we do want it later --- common/lib/xmodule/lazy_dict.py | 58 +++++++++++++++++++++++ common/lib/xmodule/xmodule/xml_module.py | 60 ------------------------ 2 files changed, 58 insertions(+), 60 deletions(-) create mode 100644 common/lib/xmodule/lazy_dict.py diff --git a/common/lib/xmodule/lazy_dict.py b/common/lib/xmodule/lazy_dict.py new file mode 100644 index 0000000000..fa614843ec --- /dev/null +++ b/common/lib/xmodule/lazy_dict.py @@ -0,0 +1,58 @@ +from collections import MutableMapping + +class LazyLoadingDict(MutableMapping): + """ + A dictionary object that lazily loads its contents from a provided + function on reads (of members that haven't already been set). + """ + + def __init__(self, loader): + ''' + On the first read from this dictionary, it will call loader() to + populate its contents. loader() must return something dict-like. Any + elements set before the first read will be preserved. + ''' + self._contents = {} + self._loaded = False + self._loader = loader + self._deleted = set() + + def __getitem__(self, name): + if not (self._loaded or name in self._contents or name in self._deleted): + self.load() + + return self._contents[name] + + def __setitem__(self, name, value): + self._contents[name] = value + self._deleted.discard(name) + + def __delitem__(self, name): + del self._contents[name] + self._deleted.add(name) + + def __contains__(self, name): + self.load() + return name in self._contents + + def __len__(self): + self.load() + return len(self._contents) + + def __iter__(self): + self.load() + return iter(self._contents) + + def __repr__(self): + self.load() + return repr(self._contents) + + def load(self): + if self._loaded: + return + + loaded_contents = self._loader() + loaded_contents.update(self._contents) + self._contents = loaded_contents + self._loaded = True + diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index de62bc5d38..89f1dc4ed2 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,66 +11,6 @@ import os log = logging.getLogger(__name__) -# # TODO (cpennington): This was implemented in an attempt to improve performance, -# # but the actual improvement wasn't measured (and it was implemented late at night). -# # We should check if it hurts, and whether there's a better way of doing lazy loading - -# class LazyLoadingDict(MutableMapping): -# """ -# A dictionary object that lazily loads its contents from a provided -# function on reads (of members that haven't already been set). -# """ - -# def __init__(self, loader): -# ''' -# On the first read from this dictionary, it will call loader() to -# populate its contents. loader() must return something dict-like. Any -# elements set before the first read will be preserved. -# ''' -# self._contents = {} -# self._loaded = False -# self._loader = loader -# self._deleted = set() - -# def __getitem__(self, name): -# if not (self._loaded or name in self._contents or name in self._deleted): -# self.load() - -# return self._contents[name] - -# def __setitem__(self, name, value): -# self._contents[name] = value -# self._deleted.discard(name) - -# def __delitem__(self, name): -# del self._contents[name] -# self._deleted.add(name) - -# def __contains__(self, name): -# self.load() -# return name in self._contents - -# def __len__(self): -# self.load() -# return len(self._contents) - -# def __iter__(self): -# self.load() -# return iter(self._contents) - -# def __repr__(self): -# self.load() -# return repr(self._contents) - -# def load(self): -# if self._loaded: -# return - -# loaded_contents = self._loader() -# loaded_contents.update(self._contents) -# self._contents = loaded_contents -# self._loaded = True - _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') From 0b67d1c40118c3a584c32bfe5b32f0f8c0a187df Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:20:04 -0400 Subject: [PATCH 19/28] Turn error_handlers into error_trackers * simplify logic--tracker just tracks errors. Trackers should not raise, and are not be responsible for logging. * adapted code to use trackers. * Started cleanup of error handling code: - if need to add info and re-raise, just do that. No logging. - if working around a problem, log and track as needed. --- common/lib/xmodule/tests/test_import.py | 4 +- .../lib/xmodule/xmodule/backcompat_module.py | 11 +++-- common/lib/xmodule/xmodule/error_module.py | 15 ++++--- common/lib/xmodule/xmodule/errorhandlers.py | 45 ------------------- common/lib/xmodule/xmodule/errortracker.py | 33 ++++++++++++++ common/lib/xmodule/xmodule/mako_module.py | 4 +- .../lib/xmodule/xmodule/modulestore/mongo.py | 27 ++++++----- common/lib/xmodule/xmodule/modulestore/xml.py | 20 ++++----- common/lib/xmodule/xmodule/raw_module.py | 10 ++--- common/lib/xmodule/xmodule/x_module.py | 38 ++++++++-------- common/lib/xmodule/xmodule/xml_module.py | 29 +++++------- .../management/commands/clean_xml.py | 23 ++-------- 12 files changed, 117 insertions(+), 142 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/errorhandlers.py create mode 100644 common/lib/xmodule/xmodule/errortracker.py diff --git a/common/lib/xmodule/tests/test_import.py b/common/lib/xmodule/tests/test_import.py index 497354a18e..34ace135a3 100644 --- a/common/lib/xmodule/tests/test_import.py +++ b/common/lib/xmodule/tests/test_import.py @@ -4,7 +4,7 @@ import unittest from lxml import etree from xmodule.x_module import XMLParsingSystem, XModuleDescriptor -from xmodule.errorhandlers import ignore_errors_handler +from xmodule.errortracker import null_error_tracker from xmodule.modulestore import Location class ImportTestCase(unittest.TestCase): @@ -27,7 +27,7 @@ class ImportTestCase(unittest.TestCase): raise Exception("Shouldn't be called") system = XMLParsingSystem(load_item, resources_fs, - ignore_errors_handler, process_xml) + null_error_tracker, process_xml) system.render_template = render_template return system diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index da0d6788e4..7ace342d1b 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -35,18 +35,21 @@ def process_includes(fn): # insert new XML into tree in place of include parent.insert(parent.index(next_include), incxml) except Exception: - msg = "Error in problem xml include: %s" % (etree.tostring(next_include, pretty_print=True)) - log.exception(msg) - parent = next_include.getparent() + # Log error and work around it + msg = "Error in problem xml include: %s" % ( + etree.tostring(next_include, pretty_print=True)) + system.error_tracker(msg) + + parent = next_include.getparent() errorxml = etree.Element('error') messagexml = etree.SubElement(errorxml, 'message') messagexml.text = msg stackxml = etree.SubElement(errorxml, 'stacktrace') stackxml.text = traceback.format_exc() - # insert error XML in place of include parent.insert(parent.index(next_include), errorxml) + parent.remove(next_include) next_include = xml_object.find('include') diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index ee05e9c879..62b3b82a39 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -15,7 +15,7 @@ class ErrorModule(XModule): TODO (vshnayder): proper style, divs, etc. ''' if not self.system.is_staff: - return self.system.render_template('module-error.html') + return self.system.render_template('module-error.html', {}) # staff get to see all the details return self.system.render_template('module-error-staff.html', { @@ -31,22 +31,27 @@ class ErrorDescriptor(EditingDescriptor): module_class = ErrorModule @classmethod - def from_xml(cls, xml_data, system, org=None, course=None): + def from_xml(cls, xml_data, system, org=None, course=None, err=None): '''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. ''' definition = {} + if err is not None: + definition['error'] = err + 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: - # Save the error to display later - definition['error'] = str(err) - + # Save the error to display later--overrides other problems + definition['error'] = err definition['data'] = xml_data # TODO (vshnayder): Do we need a unique slug here? Just pick a random diff --git a/common/lib/xmodule/xmodule/errorhandlers.py b/common/lib/xmodule/xmodule/errorhandlers.py deleted file mode 100644 index 0f97377b2a..0000000000 --- a/common/lib/xmodule/xmodule/errorhandlers.py +++ /dev/null @@ -1,45 +0,0 @@ -import logging -import sys - -log = logging.getLogger(__name__) - -def in_exception_handler(): - '''Is there an active exception?''' - return sys.exc_info() != (None, None, None) - -def strict_error_handler(msg, exc_info=None): - ''' - Do not let errors pass. If exc_info is not None, ignore msg, and just - re-raise. Otherwise, check if we are in an exception-handling context. - If so, re-raise. Otherwise, raise Exception(msg). - - Meant for use in validation, where any errors should trap. - ''' - if exc_info is not None: - raise exc_info[0], exc_info[1], exc_info[2] - - if in_exception_handler(): - raise - - raise Exception(msg) - - -def logging_error_handler(msg, exc_info=None): - '''Log all errors, but otherwise let them pass, relying on the caller to - workaround.''' - if exc_info is not None: - log.exception(msg, exc_info=exc_info) - return - - if in_exception_handler(): - log.exception(msg) - return - - log.error(msg) - - -def ignore_errors_handler(msg, exc_info=None): - '''Ignore all errors, relying on the caller to workaround. - Meant for use in the LMS, where an error in one part of the course - shouldn't bring down the whole system''' - pass diff --git a/common/lib/xmodule/xmodule/errortracker.py b/common/lib/xmodule/xmodule/errortracker.py new file mode 100644 index 0000000000..8dd893e814 --- /dev/null +++ b/common/lib/xmodule/xmodule/errortracker.py @@ -0,0 +1,33 @@ +import logging +import sys + +log = logging.getLogger(__name__) + +def in_exception_handler(): + '''Is there an active exception?''' + return sys.exc_info() != (None, None, None) + + +def make_error_tracker(): + '''Return a tuple (logger, error_list), where + the logger appends a tuple (message, exc_info=None) + to the error_list on every call. + + error_list is a simple list. If the caller messes with it, info + will be lost. + ''' + errors = [] + + def error_tracker(msg): + '''Log errors''' + exc_info = None + if in_exception_handler(): + exc_info = sys.exc_info() + + errors.append((msg, exc_info)) + + return (error_tracker, errors) + +def null_error_tracker(msg): + '''A dummy error tracker that just ignores the messages''' + pass diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index 213e9077db..eedac99aa8 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -2,10 +2,10 @@ from x_module import XModuleDescriptor, DescriptorSystem class MakoDescriptorSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, error_handler, + def __init__(self, load_item, resources_fs, error_tracker, render_template, **kwargs): super(MakoDescriptorSystem, self).__init__( - load_item, resources_fs, error_handler, **kwargs) + load_item, resources_fs, error_tracker, **kwargs) self.render_template = render_template diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 6a0bdad06a..76769b25b0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -6,7 +6,7 @@ from itertools import repeat from path import path from importlib import import_module -from xmodule.errorhandlers import strict_error_handler +from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem from mitxmako.shortcuts import render_to_string @@ -26,7 +26,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): from, with a backup of calling to the underlying modulestore for more data """ def __init__(self, modulestore, module_data, default_class, resources_fs, - error_handler, render_template): + error_tracker, render_template): """ modulestore: the module store that can be used to retrieve additional modules @@ -38,13 +38,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem): resources_fs: a filesystem, as per MakoDescriptorSystem - error_handler: + error_tracker: render_template: a function for rendering templates, as per MakoDescriptorSystem """ super(CachingDescriptorSystem, self).__init__( - self.load_item, resources_fs, error_handler, render_template) + self.load_item, resources_fs, error_tracker, render_template) self.modulestore = modulestore self.module_data = module_data self.default_class = default_class @@ -79,7 +79,8 @@ class MongoModuleStore(ModuleStore): """ # TODO (cpennington): Enable non-filesystem filestores - def __init__(self, host, db, collection, fs_root, port=27017, default_class=None): + def __init__(self, host, db, collection, fs_root, port=27017, default_class=None, + error_tracker=null_error_tracker): self.collection = pymongo.connection.Connection( host=host, port=port @@ -90,13 +91,17 @@ class MongoModuleStore(ModuleStore): # Force mongo to maintain an index over _id.* that is in the same order # that is used when querying by a location - self.collection.ensure_index(zip(('_id.' + field for field in Location._fields), repeat(1))) + self.collection.ensure_index( + zip(('_id.' + field for field in Location._fields), repeat(1))) - # TODO (vshnayder): default arg default_class=None will make this error - module_path, _, class_name = default_class.rpartition('.') - class_ = getattr(import_module(module_path), class_name) - self.default_class = class_ + if default_class is not None: + module_path, _, class_name = default_class.rpartition('.') + class_ = getattr(import_module(module_path), class_name) + self.default_class = class_ + else: + self.default_class = None self.fs_root = path(fs_root) + self.error_tracker = error_tracker def _clean_item_data(self, item): """ @@ -148,7 +153,7 @@ class MongoModuleStore(ModuleStore): data_cache, self.default_class, resource_fs, - strict_error_handler, + self.error_tracker, render_to_string, ) return system.load_item(item['location']) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index b54963b513..06e824e3d1 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -3,7 +3,7 @@ from fs.osfs import OSFS from importlib import import_module from lxml import etree from path import path -from xmodule.errorhandlers import logging_error_handler +from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor, XMLParsingSystem from xmodule.mako_module import MakoDescriptorSystem from cStringIO import StringIO @@ -29,7 +29,7 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, error_handler, **kwargs): + def __init__(self, xmlstore, org, course, course_dir, error_tracker, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that all elements have unique slugs. @@ -89,9 +89,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): resources_fs = OSFS(xmlstore.data_dir / course_dir) MakoDescriptorSystem.__init__(self, load_item, resources_fs, - error_handler, render_template, **kwargs) + error_tracker, render_template, **kwargs) XMLParsingSystem.__init__(self, load_item, resources_fs, - error_handler, process_xml, **kwargs) + error_tracker, process_xml, **kwargs) @@ -101,7 +101,7 @@ class XMLModuleStore(ModuleStore): """ def __init__(self, data_dir, default_class=None, eager=False, course_dirs=None, - error_handler=logging_error_handler): + error_tracker=null_error_tracker): """ Initialize an XMLModuleStore from data_dir @@ -116,8 +116,8 @@ class XMLModuleStore(ModuleStore): course_dirs: If specified, the list of course_dirs to load. Otherwise, load all course dirs - error_handler: The error handler used here and in the underlying - DescriptorSystem. By default, raise exceptions for all errors. + error_tracker: The error tracker used here and in the underlying + DescriptorSystem. By default, ignore all messages. See the comments in x_module.py:DescriptorSystem """ @@ -125,7 +125,7 @@ class XMLModuleStore(ModuleStore): self.data_dir = path(data_dir) self.modules = {} # location -> XModuleDescriptor self.courses = {} # course_dir -> XModuleDescriptor for the course - self.error_handler = error_handler + self.error_tracker = error_tracker if default_class is None: self.default_class = None @@ -154,7 +154,7 @@ class XMLModuleStore(ModuleStore): except: msg = "Failed to load course '%s'" % course_dir log.exception(msg) - error_handler(msg) + error_tracker(msg) def load_course(self, course_dir): @@ -192,7 +192,7 @@ class XMLModuleStore(ModuleStore): course = course_dir system = ImportSystem(self, org, course, course_dir, - self.error_handler) + self.error_tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) log.debug('========> Done with course import from {0}'.format(course_dir)) diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index f9f358f945..9fdb5d0b38 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -1,9 +1,8 @@ -from pkg_resources import resource_string from lxml import etree from xmodule.editing_module import EditingDescriptor -from xmodule.mako_module import MakoModuleDescriptor from xmodule.xml_module import XmlDescriptor import logging +import sys log = logging.getLogger(__name__) @@ -20,13 +19,12 @@ class RawDescriptor(XmlDescriptor, EditingDescriptor): try: return etree.fromstring(self.definition['data']) except etree.XMLSyntaxError as err: + # Can't recover here, so just add some info and + # re-raise lines = self.definition['data'].split('\n') line, offset = err.position msg = ("Unable to create xml for problem {loc}. " "Context: '{context}'".format( context=lines[line - 1][offset - 40:offset + 40], loc=self.location)) - log.exception(msg) - self.system.error_handler(msg) - # no workaround possible, so just re-raise - raise + raise Exception, msg, sys.exc_info()[2] diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 2d7447b5f6..a14f83eee6 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -454,15 +454,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet): # etree.fromstring(xml_data).tag,class_)) descriptor = class_.from_xml(xml_data, system, org, course) - except (ResourceNotFoundError, XMLSyntaxError) as err: + except Exception as err: # Didn't load properly. Fall back on loading as an error # descriptor. This should never error due to formatting. # Put import here to avoid circular import errors from xmodule.error_module import ErrorDescriptor - #system.error_handler("Error loading from xml.") - descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course) + system.error_tracker("Error loading from xml.") + descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course, err) return descriptor @@ -533,16 +533,19 @@ class XModuleDescriptor(Plugin, HTMLSnippet): class DescriptorSystem(object): - def __init__(self, load_item, resources_fs, error_handler, **kwargs): + def __init__(self, load_item, resources_fs, error_tracker, **kwargs): """ load_item: Takes a Location and returns an XModuleDescriptor resources_fs: A Filesystem object that contains all of the resources needed for the course - error_handler: A hook for handling errors in loading the descriptor. - Must be a function of (error_msg, exc_info=None). - See errorhandlers.py for some simple ones. + error_tracker: A hook for tracking errors in loading the descriptor. + Used for example to get a list of all non-fatal problems on course + load, and display them to the user. + + A function of (error_msg). errortracker.py provides a + handy make_error_tracker() function. Patterns for using the error handler: try: @@ -551,10 +554,8 @@ class DescriptorSystem(object): except SomeProblem: msg = 'Grommet {0} is broken'.format(x) log.exception(msg) # don't rely on handler to log - self.system.error_handler(msg) - # if we get here, work around if possible - raise # if no way to work around - OR + self.system.error_tracker(msg) + # work around return 'Oops, couldn't load grommet' OR, if not in an exception context: @@ -562,25 +563,26 @@ class DescriptorSystem(object): if not check_something(thingy): msg = "thingy {0} is broken".format(thingy) log.critical(msg) - error_handler(msg) - # if we get here, work around - pass # e.g. if no workaround needed + self.system.error_tracker(msg) + + NOTE: To avoid duplication, do not call the tracker on errors + that you're about to re-raise---let the caller track them. """ self.load_item = load_item self.resources_fs = resources_fs - self.error_handler = error_handler + self.error_tracker = error_tracker class XMLParsingSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, error_handler, process_xml, **kwargs): + def __init__(self, load_item, resources_fs, error_tracker, process_xml, **kwargs): """ - load_item, resources_fs, error_handler: see DescriptorSystem + load_item, resources_fs, error_tracker: see DescriptorSystem process_xml: Takes an xml string, and returns a XModuleDescriptor created from that xml """ - DescriptorSystem.__init__(self, load_item, resources_fs, error_handler, + DescriptorSystem.__init__(self, load_item, resources_fs, error_tracker, **kwargs) self.process_xml = process_xml diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 89f1dc4ed2..ea13bc2640 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -1,4 +1,3 @@ -from collections import MutableMapping from xmodule.x_module import XModuleDescriptor from xmodule.modulestore import Location from lxml import etree @@ -8,13 +7,12 @@ import traceback from collections import namedtuple from fs.errors import ResourceNotFoundError import os +import sys log = logging.getLogger(__name__) - _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') - class AttrMap(_AttrMapBase): """ A class that specifies a metadata_key, and two functions: @@ -116,11 +114,12 @@ class XmlDescriptor(XModuleDescriptor): # VS[compat] # TODO (cpennington): If the file doesn't exist at the right path, - # give the class a chance to fix it up. The file will be written out again - # in the correct format. - # This should go away once the CMS is online and has imported all current (fall 2012) - # courses from xml - if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'): + # give the class a chance to fix it up. The file will be written out + # again in the correct format. This should go away once the CMS is + # online and has imported all current (fall 2012) courses from xml + if not system.resources_fs.exists(filepath) and hasattr( + cls, + 'backcompat_paths'): candidates = cls.backcompat_paths(filepath) for candidate in candidates: if system.resources_fs.exists(candidate): @@ -130,19 +129,11 @@ class XmlDescriptor(XModuleDescriptor): try: with system.resources_fs.open(filepath) as file: definition_xml = cls.file_to_xml(file) - except (ResourceNotFoundError, etree.XMLSyntaxError): + except Exception: msg = 'Unable to load file contents at path %s for item %s' % ( filepath, location.url()) - - log.exception(msg) - system.error_handler(msg) - # if error_handler didn't reraise, work around problem. - error_elem = etree.Element('error') - message_elem = etree.SubElement(error_elem, 'error_message') - message_elem.text = msg - stack_elem = etree.SubElement(error_elem, 'stack_trace') - stack_elem.text = traceback.format_exc() - return {'data': etree.tostring(error_elem)} + # Add info about where we are, but keep the traceback + raise Exception, msg, sys.exc_info()[2] cls.clean_metadata_from_xml(definition_xml) return cls.definition_from_xml(definition_xml, system) diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 05865bb289..29ce246637 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -10,7 +10,7 @@ from lxml import etree from django.core.management.base import BaseCommand from xmodule.modulestore.xml import XMLModuleStore - +from xmodule.errortracker import make_error_tracker def traverse_tree(course): '''Load every descriptor in course. Return bool success value.''' @@ -21,23 +21,6 @@ def traverse_tree(course): return True -def make_logging_error_handler(): - '''Return a tuple (handler, error_list), where - the handler appends the message and any exc_info - to the error_list on every call. - ''' - errors = [] - - def error_handler(msg, exc_info=None): - '''Log errors''' - if exc_info is None: - if sys.exc_info() != (None, None, None): - exc_info = sys.exc_info() - - errors.append((msg, exc_info)) - - return (error_handler, errors) - def export(course, export_dir): """Export the specified course to course_dir. Creates dir if it doesn't exist. @@ -70,14 +53,14 @@ def import_with_checks(course_dir, verbose=True): data_dir = course_dir.dirname() course_dirs = [course_dir.basename()] - (error_handler, errors) = make_logging_error_handler() + (error_tracker, errors) = make_error_tracker() # No default class--want to complain if it doesn't find plugins for any # module. modulestore = XMLModuleStore(data_dir, default_class=None, eager=True, course_dirs=course_dirs, - error_handler=error_handler) + error_tracker=error_tracker) def str_of_err(tpl): (msg, exc_info) = tpl From 740c9b7df1b51dcafd5e4babc132ce894aae2411 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:20:18 -0400 Subject: [PATCH 20/28] fixed docstring for customtag --- common/lib/xmodule/xmodule/template_module.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index 3f926555f4..b4818dc29d 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -7,16 +7,14 @@ from mako.template import Template class CustomTagModule(XModule): """ This module supports tags of the form - - $tagname - + In this case, $tagname should refer to a file in data/custom_tags, which contains a mako template that uses ${option} and ${option2} for the content. For instance: - data/custom_tags/book:: + data/mycourse/custom_tags/book:: More information given in the text course.xml:: From 463b758434eae2a33d9fb82f992cb139b25c50ea Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 17:20:33 -0400 Subject: [PATCH 21/28] fixed args in call to add_histogram --- lms/djangoapps/courseware/module_render.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 1ff39f602e..91d4efa651 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -185,7 +185,7 @@ def get_module(user, request, location, student_module_cache, position=None): ) if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF') and user.is_staff: - module.get_html = add_histogram(module.get_html) + module.get_html = add_histogram(module.get_html, module) # If StudentModule for this instance wasn't already in the database, # and this isn't a guest user, create it. From 32253510d1892a14357a766aa97e23a9cbc3da73 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 19:37:53 -0400 Subject: [PATCH 22/28] Import error cleanup * call error tracker when needed * remove duplicate logging--just add info and re-raise * xml modulestore uses error tracker to capture load errors * add unstyled list of import errors to courseware homepage! --- .../lib/xmodule/xmodule/backcompat_module.py | 5 +- common/lib/xmodule/xmodule/capa_module.py | 25 ++++++--- common/lib/xmodule/xmodule/course_module.py | 14 +++-- common/lib/xmodule/xmodule/errortracker.py | 10 ++-- common/lib/xmodule/xmodule/html_module.py | 5 +- .../xmodule/xmodule/modulestore/__init__.py | 13 +++++ common/lib/xmodule/xmodule/modulestore/xml.py | 54 ++++++++++++------- common/lib/xmodule/xmodule/x_module.py | 5 +- lms/djangoapps/courseware/courses.py | 1 + lms/djangoapps/courseware/views.py | 15 ++++-- lms/templates/courseware.html | 7 +++ 11 files changed, 115 insertions(+), 39 deletions(-) diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index 7ace342d1b..c49f23b99e 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -35,12 +35,13 @@ def process_includes(fn): # insert new XML into tree in place of include parent.insert(parent.index(next_include), incxml) except Exception: - # Log error and work around it + # Log error msg = "Error in problem xml include: %s" % ( etree.tostring(next_include, pretty_print=True)) - + # tell the tracker system.error_tracker(msg) + # work around parent = next_include.getparent() errorxml = etree.Element('error') messagexml = etree.SubElement(errorxml, 'message') diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 2fae8b94e2..eed2cf3ac7 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -5,6 +5,7 @@ import json import logging import traceback import re +import sys from datetime import timedelta from lxml import etree @@ -91,7 +92,8 @@ class CapaModule(XModule): display_due_date_string = self.metadata.get('due', None) if display_due_date_string is not None: self.display_due_date = dateutil.parser.parse(display_due_date_string) - #log.debug("Parsed " + display_due_date_string + " to " + str(self.display_due_date)) + #log.debug("Parsed " + display_due_date_string + + # " to " + str(self.display_due_date)) else: self.display_due_date = None @@ -99,7 +101,8 @@ class CapaModule(XModule): if grace_period_string is not None and self.display_due_date: self.grace_period = parse_timedelta(grace_period_string) self.close_date = self.display_due_date + self.grace_period - #log.debug("Then parsed " + grace_period_string + " to closing date" + str(self.close_date)) + #log.debug("Then parsed " + grace_period_string + + # " to closing date" + str(self.close_date)) else: self.grace_period = None self.close_date = self.display_due_date @@ -138,10 +141,16 @@ class CapaModule(XModule): try: self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(), instance_state, seed=seed, system=self.system) - except Exception: - msg = 'cannot create LoncapaProblem %s' % self.location.url() - log.exception(msg) + except Exception as err: + msg = 'cannot create LoncapaProblem {loc}: {err}'.format( + loc=self.location.url(), err=err) + # TODO (vshnayder): do modules need error handlers too? + # We shouldn't be switching on DEBUG. if self.system.DEBUG: + log.error(msg) + # TODO (vshnayder): This logic should be general, not here--and may + # want to preserve the data instead of replacing it. + # e.g. in the CMS msg = '

%s

' % msg.replace('<', '<') msg += '

%s

' % traceback.format_exc().replace('<', '<') # create a dummy problem with error message instead of failing @@ -152,7 +161,8 @@ class CapaModule(XModule): problem_text, self.location.html_id(), instance_state, seed=seed, system=self.system) else: - raise + # add extra info and raise + raise Exception(msg), None, sys.exc_info()[2] @property def rerandomize(self): @@ -191,6 +201,7 @@ class CapaModule(XModule): try: return Progress(score, total) except Exception as err: + # TODO (vshnayder): why is this still here? still needed? if self.system.DEBUG: return None raise @@ -210,6 +221,7 @@ class CapaModule(XModule): try: html = self.lcp.get_html() except Exception, err: + # TODO (vshnayder): another switch on DEBUG. if self.system.DEBUG: log.exception(err) msg = ( @@ -561,6 +573,7 @@ class CapaDescriptor(RawDescriptor): 'problems/' + path[8:], path[8:], ] + @classmethod def split_to_file(cls, xml_object): '''Problems always written in their own files''' diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 7e0019205e..acdc574220 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -20,15 +20,22 @@ class CourseDescriptor(SequenceDescriptor): self._grader = None self._grade_cutoffs = None + msg = None try: self.start = time.strptime(self.metadata["start"], "%Y-%m-%dT%H:%M") except KeyError: self.start = time.gmtime(0) #The epoch - log.critical("Course loaded without a start date. %s", self.id) + msg = "Course loaded without a start date. id = %s" % self.id + log.critical(msg) except ValueError as e: self.start = time.gmtime(0) #The epoch - log.critical("Course loaded with a bad start date. %s '%s'", - self.id, e) + msg = "Course loaded with a bad start date. %s '%s'" % (self.id, e) + log.critical(msg) + + # Don't call the tracker from the exception handler. + if msg is not None: + system.error_tracker(msg) + def has_started(self): return time.gmtime() > self.start @@ -104,3 +111,4 @@ class CourseDescriptor(SequenceDescriptor): @property def org(self): return self.location.org + diff --git a/common/lib/xmodule/xmodule/errortracker.py b/common/lib/xmodule/xmodule/errortracker.py index 8dd893e814..09c04c061c 100644 --- a/common/lib/xmodule/xmodule/errortracker.py +++ b/common/lib/xmodule/xmodule/errortracker.py @@ -1,17 +1,21 @@ import logging import sys +from collections import namedtuple + log = logging.getLogger(__name__) +ErrorLog = namedtuple('ErrorLog', 'tracker errors') + def in_exception_handler(): '''Is there an active exception?''' return sys.exc_info() != (None, None, None) def make_error_tracker(): - '''Return a tuple (logger, error_list), where + '''Return an ErrorLog (named tuple), with fields (tracker, errors), where the logger appends a tuple (message, exc_info=None) - to the error_list on every call. + to the errors on every call. error_list is a simple list. If the caller messes with it, info will be lost. @@ -26,7 +30,7 @@ def make_error_tracker(): errors.append((msg, exc_info)) - return (error_tracker, errors) + return ErrorLog(error_tracker, errors) def null_error_tracker(msg): '''A dummy error tracker that just ignores the messages''' diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 996d3c4ead..165c402487 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -2,6 +2,7 @@ import copy from fs.errors import ResourceNotFoundError import logging import os +import sys from lxml import etree from xmodule.x_module import XModule @@ -95,8 +96,8 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): except (ResourceNotFoundError) as err: msg = 'Unable to load file contents at path {0}: {1} '.format( filepath, err) - log.error(msg) - raise + # add more info and re-raise + raise Exception(msg), None, sys.exc_info()[2] @classmethod def split_to_file(cls, xml_object): diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index a0167d781b..7241179d8e 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -213,6 +213,18 @@ class ModuleStore(object): """ raise NotImplementedError + def get_item_errors(self, location): + """ + Return a list of (msg, exception-or-None) errors that the modulestore + encountered when loading the item at location. + + location : something that can be passed to Location + + Raises the same exceptions as get_item if the location isn't found or + isn't fully specified. + """ + raise NotImplementedError + def get_items(self, location, depth=0): """ Returns a list of XModuleDescriptor instances for the items @@ -269,6 +281,7 @@ class ModuleStore(object): ''' raise NotImplementedError + def get_parent_locations(self, location): '''Find all locations that are the parents of this location. Needed for path_to_location(). diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 06e824e3d1..e7f9c9ce0a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -1,14 +1,16 @@ import logging +import os +import re + from fs.osfs import OSFS from importlib import import_module from lxml import etree from path import path -from xmodule.errortracker import null_error_tracker +from xmodule.errortracker import ErrorLog, make_error_tracker from xmodule.x_module import XModuleDescriptor, XMLParsingSystem +from xmodule.course_module import CourseDescriptor from xmodule.mako_module import MakoDescriptorSystem from cStringIO import StringIO -import os -import re from . import ModuleStore, Location from .exceptions import ItemNotFoundError @@ -19,7 +21,6 @@ etree.set_default_parser( log = logging.getLogger('mitx.' + __name__) - # VS[compat] # TODO (cpennington): Remove this once all fall 2012 courses have been imported # into the cms from xml @@ -94,14 +95,12 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): error_tracker, process_xml, **kwargs) - class XMLModuleStore(ModuleStore): """ An XML backed ModuleStore """ def __init__(self, data_dir, default_class=None, eager=False, - course_dirs=None, - error_tracker=null_error_tracker): + course_dirs=None): """ Initialize an XMLModuleStore from data_dir @@ -115,17 +114,14 @@ class XMLModuleStore(ModuleStore): course_dirs: If specified, the list of course_dirs to load. Otherwise, load all course dirs - - error_tracker: The error tracker used here and in the underlying - DescriptorSystem. By default, ignore all messages. - See the comments in x_module.py:DescriptorSystem """ self.eager = eager self.data_dir = path(data_dir) self.modules = {} # location -> XModuleDescriptor self.courses = {} # course_dir -> XModuleDescriptor for the course - self.error_tracker = error_tracker + self.location_errors = {} # location -> ErrorLog + if default_class is None: self.default_class = None @@ -149,15 +145,18 @@ class XMLModuleStore(ModuleStore): for course_dir in course_dirs: try: - course_descriptor = self.load_course(course_dir) + # make a tracker, then stick in the right place once the course loads + # and we know its location + errorlog = make_error_tracker() + course_descriptor = self.load_course(course_dir, errorlog.tracker) self.courses[course_dir] = course_descriptor + self.location_errors[course_descriptor.location] = errorlog except: msg = "Failed to load course '%s'" % course_dir log.exception(msg) - error_tracker(msg) - def load_course(self, course_dir): + def load_course(self, course_dir, tracker): """ Load a course into this module store course_path: Course directory name @@ -191,13 +190,13 @@ class XMLModuleStore(ModuleStore): )) course = course_dir - system = ImportSystem(self, org, course, course_dir, - self.error_tracker) + system = ImportSystem(self, org, course, course_dir, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) log.debug('========> Done with course import from {0}'.format(course_dir)) return course_descriptor + def get_item(self, location, depth=0): """ Returns an XModuleDescriptor instance for the item at location. @@ -218,9 +217,28 @@ class XMLModuleStore(ModuleStore): except KeyError: raise ItemNotFoundError(location) + + def get_item_errors(self, location): + """ + Return list of errors for this location, if any. Raise the same + errors as get_item if location isn't present. + + NOTE: This only actually works for courses in the xml datastore-- + will return an empty list for all other modules. + """ + location = Location(location) + # check that item is present + self.get_item(location) + + # now look up errors + if location in self.location_errors: + return self.location_errors[location].errors + return [] + def get_courses(self, depth=0): """ - Returns a list of course descriptors + Returns a list of course descriptors. If there were errors on loading, + some of these may be ErrorDescriptors instead. """ return self.courses.values() diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index a14f83eee6..2a1769bbd7 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -460,8 +460,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): # Put import here to avoid circular import errors from xmodule.error_module import ErrorDescriptor - - system.error_tracker("Error loading from xml.") + msg = "Error loading from xml." + log.exception(msg) + system.error_tracker(msg) descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course, err) return descriptor diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index c2c391b08e..cfda6497d5 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -33,6 +33,7 @@ def check_course(course_id, course_must_be_open=True, course_required=True): try: course_loc = CourseDescriptor.id_to_location(course_id) course = modulestore().get_item(course_loc) + except (KeyError, ItemNotFoundError): raise Http404("Course not found.") diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index fb4d2942f9..a049638d1b 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -55,14 +55,20 @@ def user_groups(user): def format_url_params(params): - return [urllib.quote(string.replace(' ', '_')) for string in params] + return [urllib.quote(string.replace(' ', '_')) + if string is not None else None + for string in params] @ensure_csrf_cookie @cache_if_anonymous def courses(request): # TODO: Clean up how 'error' is done. - courses = sorted(modulestore().get_courses(), key=lambda course: course.number) + + # filter out any courses that errored. + courses = [c for c in modulestore().get_courses() + if isinstance(c, CourseDescriptor)] + courses = sorted(courses, key=lambda course: course.number) universities = defaultdict(list) for course in courses: universities[course.org].append(course) @@ -210,7 +216,10 @@ def index(request, course_id, chapter=None, section=None, log.warning("Couldn't find a section descriptor for course_id '{0}'," "chapter '{1}', section '{2}'".format( course_id, chapter, section)) - + else: + if request.user.is_staff: + # Add a list of all the errors... + context['course_errors'] = modulestore().get_item_errors(course.location) result = render_to_response('courseware.html', context) except: diff --git a/lms/templates/courseware.html b/lms/templates/courseware.html index 9c145ba8c0..0955134694 100644 --- a/lms/templates/courseware.html +++ b/lms/templates/courseware.html @@ -47,6 +47,13 @@
${content} + + % if course_errors is not UNDEFINED: +

Course errors

+
+ ${course_errors} +
+ % endif
From 05c22c4901158bdca2c23242473aacd0b2cf6325 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 1 Aug 2012 09:29:31 -0400 Subject: [PATCH 23/28] Prettier error display * Log formatted traceback string instead of exc_info tuple itself * display as a list --- common/lib/xmodule/xmodule/errortracker.py | 13 +++++++------ lms/templates/courseware.html | 8 +++++++- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/errortracker.py b/common/lib/xmodule/xmodule/errortracker.py index 09c04c061c..b8d42a6983 100644 --- a/common/lib/xmodule/xmodule/errortracker.py +++ b/common/lib/xmodule/xmodule/errortracker.py @@ -1,5 +1,6 @@ import logging import sys +import traceback from collections import namedtuple @@ -14,21 +15,21 @@ def in_exception_handler(): def make_error_tracker(): '''Return an ErrorLog (named tuple), with fields (tracker, errors), where - the logger appends a tuple (message, exc_info=None) - to the errors on every call. + the logger appends a tuple (message, exception_str) to the errors on every + call. exception_str is in the format returned by traceback.format_exception. - error_list is a simple list. If the caller messes with it, info + error_list is a simple list. If the caller modifies it, info will be lost. ''' errors = [] def error_tracker(msg): '''Log errors''' - exc_info = None + exc_str = '' if in_exception_handler(): - exc_info = sys.exc_info() + exc_str = ''.join(traceback.format_exception(*sys.exc_info())) - errors.append((msg, exc_info)) + errors.append((msg, exc_str)) return ErrorLog(error_tracker, errors) diff --git a/lms/templates/courseware.html b/lms/templates/courseware.html index 0955134694..a14f35d154 100644 --- a/lms/templates/courseware.html +++ b/lms/templates/courseware.html @@ -51,7 +51,13 @@ % if course_errors is not UNDEFINED:

Course errors

- ${course_errors} +
    + % for (msg, err) in course_errors: +
  • ${msg} + 
    • ${err}
    +
    • + % endfor +
% endif From 7fb831a2e8f398e4cf1b9863b26627d6e5777a0e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 1 Aug 2012 09:36:13 -0400 Subject: [PATCH 24/28] Record warning on bad html files --- common/lib/xmodule/xmodule/html_module.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 165c402487..b2a5df9803 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -91,7 +91,9 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): html = file.read() # Log a warning if we can't parse the file, but don't error if not check_html(html): - log.warning("Couldn't parse html in {0}.".format(filepath)) + msg = "Couldn't parse html in {0}.".format(filepath) + log.warning(msg) + system.error_tracker("Warning: " + msg) return {'data' : html} except (ResourceNotFoundError) as err: msg = 'Unable to load file contents at path {0}: {1} '.format( From ea26c25cb4dc7d0651b4cf2f06209b49c0ae2642 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 1 Aug 2012 16:33:28 -0400 Subject: [PATCH 25/28] add back-compat code to customtag * apparently 6002x is using the impl-as-child structure already. --- common/lib/xmodule/xmodule/template_module.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index b4818dc29d..260ad009f9 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -32,7 +32,18 @@ class CustomTagModule(XModule): instance_state, shared_state, **kwargs) xmltree = etree.fromstring(self.definition['data']) - template_name = xmltree.attrib['impl'] + if 'impl' in xmltree.attrib: + template_name = xmltree.attrib['impl'] + else: + # VS[compat] backwards compatibility with old nested customtag structure + child_impl = xmltree.find('impl') + if child_impl is not None: + template_name = child_impl.text + else: + # TODO (vshnayder): better exception type + raise Exception("Could not find impl attribute in customtag {0}" + .format(location)) + params = dict(xmltree.items()) with self.system.filestore.open( 'custom_tags/{name}'.format(name=template_name)) as template: From 17d11a6391e8f0d9c7dd07f91d36935f9ea0f7cb Mon Sep 17 00:00:00 2001 From: Rocky Duan Date: Wed, 1 Aug 2012 16:45:08 -0400 Subject: [PATCH 26/28] pagination --- .../django_comment_client/forum/views.py | 77 +++++++++++++++---- lms/lib/comment_client.py | 7 +- lms/static/sass/_discussion.scss | 7 +- lms/templates/discussion/_forum.html | 3 +- lms/templates/discussion/_inline.html | 3 +- lms/templates/discussion/_paginator.html | 52 +++++++++++++ 6 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 lms/templates/discussion/_paginator.html diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 16bee537d3..d259a59b88 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -3,18 +3,28 @@ from django.views.decorators.http import require_POST from django.http import HttpResponse from django.utils import simplejson from django.core.context_processors import csrf +from django.core.urlresolvers import reverse from mitxmako.shortcuts import render_to_response, render_to_string from courseware.courses import check_course -import comment_client -import dateutil from dateutil.tz import tzlocal from datehelper import time_ago_in_words from django_comment_client.utils import get_categorized_discussion_info +from urllib import urlencode import json +import comment_client +import dateutil + + +THREADS_PER_PAGE = 20 +PAGES_NEARBY_DELTA = 2 + +class HtmlResponse(HttpResponse): + def __init__(self, html=''): + super(HtmlResponse, self).__init__(html, content_type='text/plain') def render_accordion(request, course, discussion_id): @@ -29,29 +39,58 @@ def render_accordion(request, course, discussion_id): return render_to_string('discussion/_accordion.html', context) -def render_discussion(request, course_id, threads, discussion_id=None, with_search_bar=True, search_text='', template='discussion/_inline.html'): +def render_discussion(request, course_id, threads, discussion_id=None, with_search_bar=True, + search_text='', discussion_type='inline', page=1, num_pages=None, + per_page=THREADS_PER_PAGE, url_for_page=None): + template = { + 'inline': 'discussion/_inline.html', + 'forum': 'discussion/_forum.html', + }[discussion_type] + + def _url_for_inline_page(page, per_page): + raw_url = reverse('django_comment_client.forum.views.inline_discussion', args=[course_id, discussion_id]) + return raw_url + '?' + urlencode({'page': page, 'per_page': per_page}) + + def _url_for_forum_page(page, per_page): + raw_url = reverse('django_comment_client.forum.views.forum_form_discussion', args=[course_id, discussion_id]) + return raw_url + '?' + urlencode({'page': page, 'per_page': per_page}) + + url_for_page = { + 'inline': _url_for_inline_page, + 'forum': _url_for_forum_page, + }[discussion_type] + + context = { 'threads': threads, 'discussion_id': discussion_id, 'search_bar': '' if not with_search_bar \ else render_search_bar(request, course_id, discussion_id, text=search_text), 'user_info': comment_client.get_user_info(request.user.id, raw=True), - 'tags': comment_client.get_threads_tags(raw=True), 'course_id': course_id, 'request': request, + 'page': page, + 'per_page': per_page, + 'num_pages': num_pages, + 'pages_nearby_delta': PAGES_NEARBY_DELTA, + 'discussion_type': discussion_type, + 'url_for_page': url_for_page, } return render_to_string(template, context) def render_inline_discussion(*args, **kwargs): - return render_discussion(template='discussion/_inline.html', *args, **kwargs) + + return render_discussion(discussion_type='inline', *args, **kwargs) def render_forum_discussion(*args, **kwargs): - return render_discussion(template='discussion/_forum.html', *args, **kwargs) + return render_discussion(discussion_type='forum', *args, **kwargs) +# discussion per page is fixed for now def inline_discussion(request, course_id, discussion_id): - threads = comment_client.get_threads(discussion_id, recursive=False) - html = render_inline_discussion(request, course_id, threads, discussion_id=discussion_id) - return HttpResponse(html, content_type="text/plain") + formpage = request.GET.get('page', 1) + threads, page, num_pages = comment_client.get_threads(discussion_id, recursive=False, page=page, per_page=THREADS_PER_PAGE) + html = render_inline_discussion(request, course_id, threads, discussion_id=discussion_id, num_pages=num_pages, page=page) + return HtmlResponse(html) def render_search_bar(request, course_id, discussion_id=None, text=''): if not discussion_id: @@ -66,18 +105,29 @@ def render_search_bar(request, course_id, discussion_id=None, text=''): def forum_form_discussion(request, course_id, discussion_id): course = check_course(course_id) - search_text = request.GET.get('text', '') + page = request.GET.get('page', 1) if len(search_text) > 0: - threads = comment_client.search_threads({'text': search_text, 'commentable_id': discussion_id}) + threads, page, per_page, num_pages = comment_client.search_threads({ + 'text': search_text, + 'commentable_id': discussion_id, + 'course_id': course_id, + 'page': page, + 'per_page': THREADS_PER_PAGE, + }) else: - threads = comment_client.get_threads(discussion_id, recursive=False) + threads, page, per_page, num_pages = comment_client.get_threads(discussion_id, recursive=False, page=page, per_page=THREADS_PER_PAGE) context = { 'csrf': csrf(request)['csrf_token'], 'course': course, - 'content': render_forum_discussion(request, course_id, threads, discussion_id=discussion_id, search_text=search_text), + 'content': render_forum_discussion(request, course_id, threads, + discussion_id=discussion_id, + search_text=search_text, + num_pages=num_pages, + per_page=per_page, + page=page), 'accordion': render_accordion(request, course, discussion_id), } @@ -102,7 +152,6 @@ def render_single_thread(request, course_id, thread_id): 'thread': thread, 'user_info': comment_client.get_user_info(request.user.id, raw=True), 'annotated_content_info': json.dumps(get_annotated_content_info(thread=thread, user_id=request.user.id)), - 'tags': comment_client.get_threads_tags(raw=True), 'course_id': course_id, 'request': request, } diff --git a/lms/lib/comment_client.py b/lms/lib/comment_client.py index e530527ef3..c9c1e74122 100644 --- a/lms/lib/comment_client.py +++ b/lms/lib/comment_client.py @@ -15,8 +15,9 @@ class CommentClientUnknownError(CommentClientError): def delete_threads(commentable_id, *args, **kwargs): return _perform_request('delete', _url_for_commentable_threads(commentable_id), *args, **kwargs) -def get_threads(commentable_id, recursive=False, *args, **kwargs): - return _perform_request('get', _url_for_threads(commentable_id), {'recursive': recursive}, *args, **kwargs) +def get_threads(commentable_id, recursive=False, page=1, per_page=20, *args, **kwargs): + response = _perform_request('get', _url_for_threads(commentable_id), {'recursive': recursive, 'page': page, 'per_page': per_page}, *args, **kwargs) + return response['collection'], response['page'], response['per_page'], response['num_pages'] def get_threads_tags(*args, **kwargs): return _perform_request('get', _url_for_threads_tags(), {}, *args, **kwargs) @@ -98,6 +99,8 @@ def unsubscribe_commentable(user_id, commentable_id, *args, **kwargs): return unsubscribe(user_id, {'source_type': 'other', 'source_id': commentable_id}) def search_threads(attributes, *args, **kwargs): + default_attributes = {'page': 1, 'per_page': 20} + attributes = dict(default_attributes.items() + attributes.items()) return _perform_request('get', _url_for_search_threads(), attributes, *args, **kwargs) def _perform_request(method, url, data_or_params=None, *args, **kwargs): diff --git a/lms/static/sass/_discussion.scss b/lms/static/sass/_discussion.scss index adefe34683..a27ad60f51 100644 --- a/lms/static/sass/_discussion.scss +++ b/lms/static/sass/_discussion.scss @@ -280,7 +280,12 @@ $discussion_input_width: 90%; } } } - + .discussion-paginator { + margin-top: 40px; + div { + display: inline-block; + } + } } diff --git a/lms/templates/discussion/_forum.html b/lms/templates/discussion/_forum.html index 967ebb9994..f8a2d54f00 100644 --- a/lms/templates/discussion/_forum.html +++ b/lms/templates/discussion/_forum.html @@ -8,9 +8,11 @@ ${search_bar}
New Post
+ <%include file="_paginator.html" /> % for thread in threads: ${renderer.render_thread(course_id, thread, edit_thread=False, show_comments=False)} % endfor + <%include file="_paginator.html" /> <%! @@ -21,5 +23,4 @@ diff --git a/lms/templates/discussion/_inline.html b/lms/templates/discussion/_inline.html index 967ebb9994..f8a2d54f00 100644 --- a/lms/templates/discussion/_inline.html +++ b/lms/templates/discussion/_inline.html @@ -8,9 +8,11 @@ ${search_bar}
New Post
+ <%include file="_paginator.html" /> % for thread in threads: ${renderer.render_thread(course_id, thread, edit_thread=False, show_comments=False)} % endfor + <%include file="_paginator.html" /> <%! @@ -21,5 +23,4 @@ diff --git a/lms/templates/discussion/_paginator.html b/lms/templates/discussion/_paginator.html new file mode 100644 index 0000000000..9df1df0d87 --- /dev/null +++ b/lms/templates/discussion/_paginator.html @@ -0,0 +1,52 @@ +<%def name="link_to_page(_page)"> + % if _page != page: +
+ ${_page} +
+ % else: +
${_page}
+ % endif + + +<%def name="list_pages(*args)"> + % for arg in args: + % if arg == 'dots': +
...
+ % elif isinstance(arg, list): + % for _page in arg: + ${link_to_page(_page)} + % endfor + % else: + ${link_to_page(arg)} + % endif + % endfor + + +
+
+ % if page > 1: + < Previous page + % else: + < Previous page + % endif +
+ + % if num_pages <= 2 * pages_nearby_delta + 2: + ${list_pages(range(1, num_pages + 1))} + % else: + % if page <= 2 * pages_nearby_delta: + ${list_pages(range(1, 2 * pages_nearby_delta + 2), 'dots', num_pages)} + % elif num_pages - page + 1 <= 2 * pages_nearby_delta: + ${list_pages(1, 'dots', range(num_pages - 2 * pages_nearby_delta, num_pages + 1))} + % else: + ${list_pages(1, 'dots', range(page - pages_nearby_delta, page + pages_nearby_delta + 1), 'dots', num_pages)} + % endif + % endif +
+ % if page < num_pages: + Next page > + % else: + Next page > + % endif +
+
From 0dccfc0a8d7acac734ebc50123a06505da6fabc3 Mon Sep 17 00:00:00 2001 From: Rocky Duan Date: Wed, 1 Aug 2012 16:47:20 -0400 Subject: [PATCH 27/28] forgot to change template name --- common/lib/xmodule/xmodule/discussion_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index e3531468ba..c3388a968c 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/common/lib/xmodule/xmodule/discussion_module.py @@ -15,7 +15,7 @@ class DiscussionModule(XModule): context = { 'discussion_id': self.discussion_id, } - return self.system.render_template('discussion/_show_discussion.html', context) + return self.system.render_template('discussion/_discussion_module.html', context) def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs): XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs) From 1ee1d900abb3fd0135dc578ed64ae5ef46d8fdde Mon Sep 17 00:00:00 2001 From: Rocky Duan Date: Wed, 1 Aug 2012 16:51:32 -0400 Subject: [PATCH 28/28] fixded inline with pagination --- lms/djangoapps/django_comment_client/forum/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index d259a59b88..81b1764f93 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -41,7 +41,7 @@ def render_accordion(request, course, discussion_id): def render_discussion(request, course_id, threads, discussion_id=None, with_search_bar=True, search_text='', discussion_type='inline', page=1, num_pages=None, - per_page=THREADS_PER_PAGE, url_for_page=None): + per_page=THREADS_PER_PAGE): template = { 'inline': 'discussion/_inline.html', 'forum': 'discussion/_forum.html', @@ -87,9 +87,9 @@ def render_forum_discussion(*args, **kwargs): # discussion per page is fixed for now def inline_discussion(request, course_id, discussion_id): - formpage = request.GET.get('page', 1) - threads, page, num_pages = comment_client.get_threads(discussion_id, recursive=False, page=page, per_page=THREADS_PER_PAGE) - html = render_inline_discussion(request, course_id, threads, discussion_id=discussion_id, num_pages=num_pages, page=page) + page = request.GET.get('page', 1) + threads, page, per_page, num_pages = comment_client.get_threads(discussion_id, recursive=False, page=page, per_page=THREADS_PER_PAGE) + html = render_inline_discussion(request, course_id, threads, discussion_id=discussion_id, num_pages=num_pages, page=page, per_page=per_page) return HtmlResponse(html) def render_search_bar(request, course_id, discussion_id=None, text=''):