diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 597d74ce6f..380388b545 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -90,10 +90,12 @@ def add_histogram(get_html, module): # TODO (ichuang): Remove after fall 2012 LMS migration done if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'): - [filepath, filename] = module.definition.get('filename','') + [filepath, filename] = module.definition.get('filename', ['', None]) osfs = module.system.filestore if filename is not None and osfs.exists(filename): - filepath = filename # if original, unmangled filename exists then use it (github doesn't like symlinks) + # if original, unmangled filename exists then use it (github + # doesn't like symlinks) + filepath = filename data_dir = osfs.root_path.rsplit('/')[-1] edit_link = "https://github.com/MITx/%s/tree/master/%s" % (data_dir,filepath) else: diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 8d6604b06b..9d4744e2c9 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -103,7 +103,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): experiment = xml_object.get('experiment') if experiment is None: - raise InvalidDefinitionError("ABTests must specify an experiment. Not found in:\n{xml}".format(xml=etree.tostring(xml_object, pretty_print=True))) + raise InvalidDefinitionError( + "ABTests must specify an experiment. Not found in:\n{xml}" + .format(xml=etree.tostring(xml_object, pretty_print=True))) definition = { 'data': { @@ -127,7 +129,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): definition['data']['group_content'][name] = child_content_urls definition['children'].extend(child_content_urls) - default_portion = 1 - sum(portion for (name, portion) in definition['data']['group_portions'].items()) + default_portion = 1 - sum( + portion for (name, portion) in definition['data']['group_portions'].items()) + if default_portion < 0: raise InvalidDefinitionError("ABTest portions must add up to less than or equal to 1") diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index a384edf234..ba49f98257 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -119,9 +119,9 @@ class CapaModule(XModule): if self.show_answer == "": self.show_answer = "closed" - if instance_state != None: + if instance_state is not None: instance_state = json.loads(instance_state) - if instance_state != None and 'attempts' in instance_state: + if instance_state is not None and 'attempts' in instance_state: self.attempts = instance_state['attempts'] self.name = only_one(dom2.xpath('/problem/@name')) @@ -238,7 +238,7 @@ class CapaModule(XModule): content = {'name': self.metadata['display_name'], 'html': html, 'weight': self.weight, - } + } # We using strings as truthy values, because the terminology of the # check button is context-specific. @@ -563,6 +563,11 @@ class CapaDescriptor(RawDescriptor): module_class = CapaModule + # Capa modules have some additional metadata: + # TODO (vshnayder): do problems have any other metadata? Do they + # actually use type and points? + metadata_attributes = RawDescriptor.metadata_attributes + ('type', 'points') + # VS[compat] # TODO (cpennington): Delete this method once all fall 2012 course are being # edited in the cms @@ -572,8 +577,3 @@ class CapaDescriptor(RawDescriptor): 'problems/' + path[8:], path[8:], ] - - @classmethod - def split_to_file(cls, xml_object): - '''Problems always written in their own files''' - return True diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 7f4c204f45..ce1b93aa44 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -3,7 +3,7 @@ import time import dateutil.parser import logging -from util.decorators import lazyproperty +from xmodule.util.decorators import lazyproperty from xmodule.graders import load_grading_policy from xmodule.modulestore import Location from xmodule.seq_module import SequenceDescriptor, SequenceModule @@ -13,7 +13,6 @@ log = logging.getLogger(__name__) class CourseDescriptor(SequenceDescriptor): module_class = SequenceModule - metadata_attributes = SequenceDescriptor.metadata_attributes + ('org', 'course') def __init__(self, system, definition=None, **kwargs): super(CourseDescriptor, self).__init__(system, definition, **kwargs) @@ -37,84 +36,84 @@ class CourseDescriptor(SequenceDescriptor): def has_started(self): return time.gmtime() > self.start - + @property def grader(self): return self.__grading_policy['GRADER'] - + @property def grade_cutoffs(self): return self.__grading_policy['GRADE_CUTOFFS'] - + @lazyproperty def __grading_policy(self): policy_string = "" - + try: with self.system.resources_fs.open("grading_policy.json") as grading_policy_file: policy_string = grading_policy_file.read() except (IOError, ResourceNotFoundError): log.warning("Unable to load course settings file from grading_policy.json in course " + self.id) - + grading_policy = load_grading_policy(policy_string) - + return grading_policy - - + + @lazyproperty def grading_context(self): """ This returns a dictionary with keys necessary for quickly grading a student. They are used by grades.grade() - - The grading context has two keys: + + The grading context has two keys: graded_sections - This contains the sections that are graded, as well as all possible children modules that can affect the grading. This allows some sections to be skipped if the student hasn't seen any part of it. - + The format is a dictionary keyed by section-type. The values are arrays of dictionaries containing "section_descriptor" : The section descriptor - "xmoduledescriptors" : An array of xmoduledescriptors that + "xmoduledescriptors" : An array of xmoduledescriptors that could possibly be in the section, for any student - + all_descriptors - This contains a list of all xmodules that can effect grading a student. This is used to efficiently fetch all the xmodule state for a StudentModuleCache without walking the descriptor tree again. - - + + """ - + all_descriptors = [] graded_sections = {} - + def yield_descriptor_descendents(module_descriptor): for child in module_descriptor.get_children(): yield child for module_descriptor in yield_descriptor_descendents(child): yield module_descriptor - + for c in self.get_children(): sections = [] for s in c.get_children(): if s.metadata.get('graded', False): # TODO: Only include modules that have a score here xmoduledescriptors = [child for child in yield_descriptor_descendents(s)] - + section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : xmoduledescriptors} - + section_format = s.metadata.get('format', "") graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description] - - all_descriptors.extend(xmoduledescriptors) + + all_descriptors.extend(xmoduledescriptors) all_descriptors.append(s) - + return { 'graded_sections' : graded_sections, 'all_descriptors' : all_descriptors,} - - + + @staticmethod def id_to_location(course_id): '''Convert the given course_id (org/course/name) to a location object. diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index ecc90873b9..882d181b8b 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -1,5 +1,8 @@ -import sys +import hashlib import logging +import random +import string +import sys from pkg_resources import resource_string from lxml import etree @@ -35,7 +38,8 @@ class ErrorDescriptor(EditingDescriptor): error_msg='Error not available'): '''Create an instance of this descriptor from the supplied data. - Does not try to parse the data--just stores it. + Does not require that xml_data be parseable--just stores it and exports + as-is if not. Takes an extra, optional, parameter--the error that caused an issue. (should be a string, or convert usefully into one). @@ -45,6 +49,13 @@ class ErrorDescriptor(EditingDescriptor): definition = {'data': inner} inner['error_msg'] = str(error_msg) + # Pick a unique url_name -- the sha1 hash of the xml_data. + # NOTE: We could try to pull out the url_name of the errored descriptor, + # but url_names aren't guaranteed to be unique between descriptor types, + # and ErrorDescriptor can wrap any type. When the wrapped module is fixed, + # it will be written out with the original url_name. + url_name = hashlib.sha1(xml_data).hexdigest() + try: # If this is already an error tag, don't want to re-wrap it. xml_obj = etree.fromstring(xml_data) @@ -63,7 +74,7 @@ class ErrorDescriptor(EditingDescriptor): inner['contents'] = xml_data # TODO (vshnayder): Do we need a unique slug here? Just pick a random # 64-bit num? - location = ['i4x', org, course, 'error', 'slug'] + location = ['i4x', org, course, 'error', url_name] metadata = {} # stays in the xml_data return cls(system, definition, location=location, metadata=metadata) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 260b84278b..d8837d876f 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -13,6 +13,7 @@ from .html_checker import check_html log = logging.getLogger("mitx.courseware") + class HtmlModule(XModule): def get_html(self): return self.html @@ -36,18 +37,19 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # are being edited in the cms @classmethod def backcompat_paths(cls, path): - origpath = path if path.endswith('.html.xml'): - path = path[:-9] + '.html' #backcompat--look for html instead of 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 + nc = [] + for candidate in candidates: + if candidate.endswith('.xml'): + nc.append(candidate[:-4] + '.html') + return candidates + nc # NOTE: html descriptors are special. We do not want to parse and # export them ourselves, because that can break things (e.g. lxml @@ -69,7 +71,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): if filename is None: definition_xml = copy.deepcopy(xml_object) cls.clean_metadata_from_xml(definition_xml) - return {'data' : stringify_children(definition_xml)} + return {'data': stringify_children(definition_xml)} else: filepath = cls._format_filepath(xml_object.tag, filename) @@ -80,7 +82,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # 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)) + log.debug("candidates = {0}".format(candidates)) for candidate in candidates: if system.resources_fs.exists(candidate): filepath = candidate @@ -95,7 +97,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): log.warning(msg) system.error_tracker("Warning: " + msg) - definition = {'data' : html} + definition = {'data': html} # TODO (ichuang): remove this after migration # for Fall 2012 LMS migration: keep filename (and unmangled filename) @@ -109,17 +111,11 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # add more info and re-raise raise Exception(msg), None, sys.exc_info()[2] - @classmethod - def split_to_file(cls, xml_object): - '''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 + write just to filename.xml, and the html string to filename.html. ''' try: @@ -138,4 +134,3 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): elt = etree.Element('html') elt.set("filename", self.url_name) return elt - diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 46fcf19469..f9093f355a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -188,21 +188,26 @@ class XMLModuleStore(ModuleStoreBase): course_file = StringIO(clean_out_mako_templating(course_file.read())) course_data = etree.parse(course_file).getroot() + org = course_data.get('org') if org is None: - log.error("No 'org' attribute set for course in {dir}. " + msg = ("No 'org' attribute set for course in {dir}. " "Using default 'edx'".format(dir=course_dir)) + log.error(msg) + tracker(msg) org = 'edx' course = course_data.get('course') if course is None: - log.error("No 'course' attribute set for course in {dir}." + msg = ("No 'course' attribute set for course in {dir}." " Using default '{default}'".format( dir=course_dir, default=course_dir )) + log.error(msg) + tracker(msg) course = course_dir system = ImportSystem(self, org, course, course_dir, tracker) diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 5f7f41bf8d..e742a58471 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -122,16 +122,3 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): etree.fromstring(child.export_to_xml(resource_fs))) return xml_object - @classmethod - def split_to_file(cls, xml_object): - # Note: if we end up needing subclasses, can port this logic there. - yes = ('chapter',) - no = ('course',) - - if xml_object.tag in yes: - return True - elif xml_object.tag in no: - return False - - # otherwise maybe--delegate to superclass. - return XmlDescriptor.split_to_file(xml_object) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 4c0b6fc9b3..aeec2cb9b7 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -32,7 +32,7 @@ i4xs = ModuleSystem( user=Mock(), filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))), debug=True, - xqueue=None, + xqueue={'default_queuename': 'testqueue'}, is_staff=False ) diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index eacf8352be..bcbf81861c 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -1,36 +1,107 @@ -from xmodule.modulestore.xml import XMLModuleStore -from nose.tools import assert_equals -from nose import SkipTest -from tempfile import mkdtemp +import unittest + from fs.osfs import OSFS +from nose.tools import assert_equals, assert_true +from path import path +from tempfile import mkdtemp + +from xmodule.modulestore.xml import XMLModuleStore + +# from ~/mitx_all/mitx/common/lib/xmodule/xmodule/tests/ +# to ~/mitx_all/mitx/common/test +TEST_DIR = path(__file__).abspath().dirname() +for i in range(4): + TEST_DIR = TEST_DIR.dirname() +TEST_DIR = TEST_DIR / 'test' + +DATA_DIR = TEST_DIR / 'data' -def check_export_roundtrip(data_dir): - print "Starting import" - initial_import = XMLModuleStore('org', 'course', data_dir, eager=True) - initial_course = initial_import.course +def strip_metadata(descriptor, key): + """ + Recursively strips tag from all children. + """ + print "strip {key} from {desc}".format(key=key, desc=descriptor.location.url()) + descriptor.metadata.pop(key, None) + for d in descriptor.get_children(): + strip_metadata(d, key) - print "Starting export" - export_dir = mkdtemp() - fs = OSFS(export_dir) - xml = initial_course.export_to_xml(fs) - with fs.open('course.xml', 'w') as course_xml: - course_xml.write(xml) - - print "Starting second import" - second_import = XMLModuleStore('org', 'course', export_dir, eager=True) - - print "Checking key equality" - assert_equals(initial_import.modules.keys(), second_import.modules.keys()) - - print "Checking module equality" - for location in initial_import.modules.keys(): - print "Checking", location - assert_equals(initial_import.modules[location], second_import.modules[location]) +def strip_filenames(descriptor): + """ + Recursively strips 'filename' from all children's definitions. + """ + print "strip filename from {desc}".format(desc=descriptor.location.url()) + descriptor.definition.pop('filename', None) + for d in descriptor.get_children(): + strip_filenames(d) -def test_toy_roundtrip(): - dir = "" - # TODO: add paths and make this run. - raise SkipTest() - check_export_roundtrip(dir) + +class RoundTripTestCase(unittest.TestCase): + '''Check that our test courses roundtrip properly''' + def check_export_roundtrip(self, data_dir, course_dir): + print "Starting import" + initial_import = XMLModuleStore(data_dir, eager=True, course_dirs=[course_dir]) + + courses = initial_import.get_courses() + self.assertEquals(len(courses), 1) + initial_course = courses[0] + + print "Starting export" + export_dir = mkdtemp() + print "export_dir: {0}".format(export_dir) + fs = OSFS(export_dir) + export_course_dir = 'export' + export_fs = fs.makeopendir(export_course_dir) + + xml = initial_course.export_to_xml(export_fs) + with export_fs.open('course.xml', 'w') as course_xml: + course_xml.write(xml) + + print "Starting second import" + second_import = XMLModuleStore(export_dir, eager=True, + course_dirs=[export_course_dir]) + + courses2 = second_import.get_courses() + self.assertEquals(len(courses2), 1) + exported_course = courses2[0] + + print "Checking course equality" + # HACK: data_dir metadata tags break equality because they + # aren't real metadata, and depend on paths. Remove them. + strip_metadata(initial_course, 'data_dir') + strip_metadata(exported_course, 'data_dir') + + # HACK: filenames change when changing file formats + # during imports from old-style courses. Ignore them. + strip_filenames(initial_course) + strip_filenames(exported_course) + + self.assertEquals(initial_course, exported_course) + + print "Checking key equality" + self.assertEquals(sorted(initial_import.modules.keys()), + sorted(second_import.modules.keys())) + + print "Checking module equality" + for location in initial_import.modules.keys(): + print "Checking", location + if location.category == 'html': + print ("Skipping html modules--they can't import in" + " final form without writing files...") + continue + self.assertEquals(initial_import.modules[location], + second_import.modules[location]) + + + def setUp(self): + self.maxDiff = None + + def test_toy_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "toy") + + def test_simple_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "simple") + + def test_full_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "full") diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 0d3e2260fb..407057a4ab 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -5,6 +5,7 @@ from fs.memoryfs import MemoryFS from lxml import etree from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.xml_module import is_pointer_tag from xmodule.errortracker import make_error_tracker from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError @@ -46,22 +47,17 @@ class DummySystem(XMLParsingSystem): raise Exception("Shouldn't be called") - - class ImportTestCase(unittest.TestCase): '''Make sure module imports work properly, including for malformed inputs''' - - @staticmethod def get_system(): '''Get a dummy system''' return DummySystem() def test_fallback(self): - '''Make sure that malformed xml loads as an ErrorDescriptor.''' + '''Check that malformed xml loads as an ErrorDescriptor.''' bad_xml = '''''' - system = self.get_system() descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course', @@ -70,6 +66,22 @@ class ImportTestCase(unittest.TestCase): self.assertEqual(descriptor.__class__.__name__, 'ErrorDescriptor') + + def test_unique_url_names(self): + '''Check that each error gets its very own url_name''' + bad_xml = '''''' + bad_xml2 = '''''' + system = self.get_system() + + descriptor1 = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', + 'course', None) + + descriptor2 = XModuleDescriptor.load_from_xml(bad_xml2, system, 'org', + 'course', None) + + self.assertNotEqual(descriptor1.location, descriptor2.location) + + def test_reimport(self): '''Make sure an already-exported error xml tag loads properly''' @@ -111,30 +123,65 @@ class ImportTestCase(unittest.TestCase): xml_out = etree.fromstring(xml_str_out) self.assertEqual(xml_out.tag, 'sequential') - def test_metadata_inherit(self): - """Make sure metadata inherits properly""" + def test_metadata_import_export(self): + """Two checks: + - unknown metadata is preserved across import-export + - inherited metadata doesn't leak to children. + """ system = self.get_system() - v = "1 hour" - start_xml = ''' - - Two houses, ... - '''.format(grace=v) + v = '1 hour' + org = 'foo' + course = 'bbhh' + url_name = 'test1' + start_xml = ''' + + + Two houses, ... + + '''.format(grace=v, org=org, course=course, url_name=url_name) descriptor = XModuleDescriptor.load_from_xml(start_xml, system, - 'org', 'course') + org, course) - print "Errors: {0}".format(system.errorlog.errors) print descriptor, descriptor.metadata self.assertEqual(descriptor.metadata['graceperiod'], v) + self.assertEqual(descriptor.metadata['unicorn'], 'purple') - # Check that the child inherits correctly + # Check that the child inherits graceperiod correctly child = descriptor.get_children()[0] self.assertEqual(child.metadata['graceperiod'], v) - # Now export and see if the chapter tag has a graceperiod attribute + # check that the child does _not_ inherit any unicorns + self.assertTrue('unicorn' not in child.metadata) + + # Now export and check things resource_fs = MemoryFS() exported_xml = descriptor.export_to_xml(resource_fs) + + # Check that the exported xml is just a pointer print "Exported xml:", exported_xml - root = etree.fromstring(exported_xml) - chapter_tag = root[0] - self.assertEqual(chapter_tag.tag, 'chapter') - self.assertFalse('graceperiod' in chapter_tag.attrib) + pointer = etree.fromstring(exported_xml) + self.assertTrue(is_pointer_tag(pointer)) + # but it's a special case course pointer + self.assertEqual(pointer.attrib['course'], course) + self.assertEqual(pointer.attrib['org'], org) + + # Does the course still have unicorns? + with resource_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f: + course_xml = etree.fromstring(f.read()) + + self.assertEqual(course_xml.attrib['unicorn'], 'purple') + + # the course and org tags should be _only_ in the pointer + self.assertTrue('course' not in course_xml.attrib) + self.assertTrue('org' not in course_xml.attrib) + + # did we successfully strip the url_name from the definition contents? + self.assertTrue('url_name' not in course_xml.attrib) + + # Does the chapter tag now have a graceperiod attribute? + # hardcoded path to child + with resource_fs.open('chapter/ch.xml') as f: + chapter_xml = etree.fromstring(f.read()) + self.assertEqual(chapter_xml.tag, 'chapter') + self.assertFalse('graceperiod' in chapter_xml.attrib) diff --git a/common/lib/xmodule/xmodule/util/__init__.py b/common/lib/xmodule/xmodule/util/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/util/decorators.py b/common/lib/xmodule/xmodule/util/decorators.py similarity index 100% rename from common/djangoapps/util/decorators.py rename to common/lib/xmodule/xmodule/util/decorators.py diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9b005d1dae..818e0cd8fd 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -6,6 +6,7 @@ from fs.errors import ResourceNotFoundError from functools import partial from lxml import etree from lxml.etree import XMLSyntaxError +from pprint import pprint from xmodule.modulestore import Location from xmodule.errortracker import exc_info_to_str @@ -550,9 +551,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): if not eq: for attr in self.equality_attributes: - print(getattr(self, attr, None), - getattr(other, attr, None), - getattr(self, attr, None) == getattr(other, attr, None)) + pprint((getattr(self, attr, None), + getattr(other, attr, None), + getattr(self, attr, None) == getattr(other, attr, None))) return eq diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 7a12ed869d..c9fee6c9a5 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,22 +11,44 @@ import sys log = logging.getLogger(__name__) -_AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = set(['url_name']) + else: + expected_attr = set(['url_name', 'course', 'org']) + + actual_attr = set(xml_obj.attrib.keys()) + return len(xml_obj) == 0 and actual_attr == expected_attr + + + +_AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml') class AttrMap(_AttrMapBase): """ - A class that specifies a metadata_key, and two functions: + A class that specifies two functions: - to_metadata: convert value from the xml representation into + from_xml: convert value from the xml representation into an internal python representation - from_metadata: convert the internal python representation into + to_xml: convert the internal python representation into the value to store in the xml. """ - def __new__(_cls, metadata_key, - to_metadata=lambda x: x, - from_metadata=lambda x: x): - return _AttrMapBase.__new__(_cls, metadata_key, to_metadata, from_metadata) + def __new__(_cls, from_xml=lambda x: x, + to_xml=lambda x: x): + return _AttrMapBase.__new__(_cls, from_xml, to_xml) class XmlDescriptor(XModuleDescriptor): @@ -39,19 +61,28 @@ class XmlDescriptor(XModuleDescriptor): # The attributes will be removed from the definition xml passed # to definition_from_xml, and from the xml returned by definition_to_xml + + # Note -- url_name isn't in this list because it's handled specially on + # import and export. + + # TODO (vshnayder): Do we need a list of metadata we actually + # understand? And if we do, is this the place? + # Related: What's the right behavior for clean_metadata? metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', - 'ispublic', # if True, then course is listed for all users; see + 'ispublic', # if True, then course is listed for all users; see # VS[compat] Remove once unused. 'name', 'slug') + metadata_to_strip = ('data_dir', + # VS[compat] -- remove the below attrs once everything is in the CMS + 'course', 'org', 'url_name', 'filename') # A dictionary mapping xml attribute names AttrMaps that describe how # to import and export them xml_attribute_map = { # type conversion: want True/False in python, "true"/"false" in xml - 'graded': AttrMap('graded', - lambda val: val == 'true', + 'graded': AttrMap(lambda val: val == 'true', lambda val: str(val).lower()), } @@ -101,12 +132,32 @@ class XmlDescriptor(XModuleDescriptor): """ return etree.parse(file_object).getroot() + @classmethod + def load_file(cls, filepath, fs, location): + ''' + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + ''' + try: + with fs.open(filepath) as file: + return cls.file_to_xml(file) + except Exception as err: + # Add info about where we are, but keep the traceback + msg = 'Unable to load file contents at path %s for item %s: %s ' % ( + filepath, location.url(), str(err)) + raise Exception, msg, sys.exc_info()[2] + + @classmethod 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)''' + # VS[compat] -- the filename tag should go away once everything is + # converted. (note: make sure html files still work once this goes away) filename = xml_object.get('filename') if filename is None: definition_xml = copy.deepcopy(xml_object) @@ -120,22 +171,14 @@ class XmlDescriptor(XModuleDescriptor): # 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'): + 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 Exception: - msg = 'Unable to load file contents at path %s for item %s' % ( - filepath, location.url()) - # Add info about where we are, but keep the traceback - raise Exception, msg, sys.exc_info()[2] + definition_xml = cls.load_file(filepath, system.resources_fs, location) cls.clean_metadata_from_xml(definition_xml) definition = cls.definition_from_xml(definition_xml, system) @@ -146,6 +189,28 @@ class XmlDescriptor(XModuleDescriptor): return definition + @classmethod + def load_metadata(cls, xml_object): + """ + Read the metadata attributes from this xml_object. + + Returns a dictionary {key: value}. + """ + metadata = {} + for attr in xml_object.attrib: + val = xml_object.get(attr) + if val is not None: + # VS[compat]. Remove after all key translations done + attr = cls._translate(attr) + + if attr in cls.metadata_to_strip: + # don't load these + continue + + attr_map = cls.xml_attribute_map.get(attr, AttrMap()) + metadata[attr] = attr_map.from_xml(val) + return metadata + @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -160,26 +225,27 @@ class XmlDescriptor(XModuleDescriptor): url identifiers """ xml_object = etree.fromstring(xml_data) - # VS[compat] -- just have the url_name lookup once translation is done - slug = xml_object.get('url_name', xml_object.get('slug')) - location = Location('i4x', org, course, xml_object.tag, slug) + # VS[compat] -- just have the url_name lookup, once translation is done + url_name = xml_object.get('url_name', xml_object.get('slug')) + location = Location('i4x', org, course, xml_object.tag, url_name) - def load_metadata(): - metadata = {} - for attr in cls.metadata_attributes: - val = xml_object.get(attr) - if val is not None: - # VS[compat]. Remove after all key translations done - attr = cls._translate(attr) + # VS[compat] -- detect new-style each-in-a-file mode + if is_pointer_tag(xml_object): + # new style: + # read the actual definition file--named using url_name + filepath = cls._format_filepath(xml_object.tag, url_name) + definition_xml = cls.load_file(filepath, system.resources_fs, location) + else: + definition_xml = xml_object - attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr)) - metadata[attr_map.metadata_key] = attr_map.to_metadata(val) - return metadata + definition = cls.load_definition(definition_xml, system, location) + # VS[compat] -- make Ike's github preview links work in both old and + # new file layouts + if is_pointer_tag(xml_object): + # new style -- contents actually at filepath + definition['filename'] = [filepath, filepath] - 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')) + metadata = cls.load_metadata(definition_xml) return cls( system, definition, @@ -193,20 +259,6 @@ class XmlDescriptor(XModuleDescriptor): name=name, ext=cls.filename_extension) - @classmethod - def split_to_file(cls, xml_object): - ''' - Decide whether to write this object to a separate file or not. - - xml_object: an xml definition of an instance of cls. - - This default implementation will split if this has more than 7 - descendant tags. - - Can be overridden by subclasses. - ''' - return len(list(xml_object.iter())) > 7 - def export_to_xml(self, resource_fs): """ Returns an xml string representing this module, and all modules @@ -227,42 +279,39 @@ class XmlDescriptor(XModuleDescriptor): xml_object = self.definition_to_xml(resource_fs) self.__class__.clean_metadata_from_xml(xml_object) - # Set the tag first, so it's right if writing to a file + # Set the tag so we get the file path right xml_object.tag = self.category - # Write it to a file if necessary - if self.split_to_file(xml_object): - # Put this object in its own file - filepath = self.__class__._format_filepath(self.category, self.url_name) - resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) - with resource_fs.open(filepath, 'w') as file: - file.write(etree.tostring(xml_object, pretty_print=True)) - # ...and remove all of its children here - for child in xml_object: - xml_object.remove(child) - # also need to remove the text of this object. - xml_object.text = '' - # and the tail for good measure... - xml_object.tail = '' + def val_for_xml(attr): + """Get the value for this attribute that we want to store. + (Possible format conversion through an AttrMap). + """ + attr_map = self.xml_attribute_map.get(attr, AttrMap()) + return attr_map.to_xml(self.own_metadata[attr]) + # Add the non-inherited metadata + for attr in sorted(self.own_metadata): + # don't want e.g. data_dir + if attr not in self.metadata_to_strip: + xml_object.set(attr, val_for_xml(attr)) - xml_object.set('filename', self.url_name) + # Write the definition to a file + filepath = self.__class__._format_filepath(self.category, self.url_name) + resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) + with resource_fs.open(filepath, 'w') as file: + file.write(etree.tostring(xml_object, pretty_print=True)) - # Add the metadata - xml_object.set('url_name', self.url_name) - for attr in self.metadata_attributes: - attr_map = self.xml_attribute_map.get(attr, AttrMap(attr)) - metadata_key = attr_map.metadata_key + # And return just a pointer with the category and filename. + record_object = etree.Element(self.category) + record_object.set('url_name', self.url_name) - if (metadata_key not in self.metadata or - metadata_key in self._inherited_metadata): - continue + # Special case for course pointers: + if self.category == 'course': + # add org and course attributes on the pointer tag + record_object.set('org', self.location.org) + record_object.set('course', self.location.course) - val = attr_map.from_metadata(self.metadata[metadata_key]) - xml_object.set(attr, val) - - # Now we just have to make it beautiful - return etree.tostring(xml_object, pretty_print=True) + return etree.tostring(record_object, pretty_print=True) def definition_to_xml(self, resource_fs): """ diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index b92158cdb7..86dc8df45c 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -2,7 +2,7 @@