Merge master

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:

common/lib/capa/capa/inputtypes.py

@@ -204,7 +204,7 @@ def extract_choices(element):
             raise Exception("[courseware.capa.inputtypes.extract_choices] \
                              Expected a <choice> tag; got %s instead"
                              % choice.tag)
-        choice_text = ''.join([etree.tostring(x) for x in choice])
+        choice_text = ''.join([x.text for x in choice])
 
         choices.append((choice.get("name"), choice_text))
 

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")
 

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

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.

common/lib/xmodule/xmodule/error_module.py

-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)

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 <html filename=""> tag to filename.xml, and the html
+        write just <html filename="" [meta-attrs="..."]> 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
-

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)

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)

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={'interface':None, 'callback_url':'/', 'default_queuename':'null'},
+    xqueue={'default_queuename': 'testqueue'},
     is_staff=False
 )
 

common/lib/xmodule/xmodule/tests/test_export.py

-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 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)
+
+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)
+
+
+
+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)
 
-def check_export_roundtrip(data_dir):
-    print "Starting import"
-    initial_import = XMLModuleStore('org', 'course', data_dir, eager=True)
-    initial_course = initial_import.course
+        print "Checking key equality"
+        self.assertEquals(sorted(initial_import.modules.keys()),
+                          sorted(second_import.modules.keys()))
 
-    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 "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])
 
-    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())
+    def setUp(self):
+        self.maxDiff = None
 
-    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 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_toy_roundtrip():
-    dir = ""
-    # TODO: add paths and make this run.
-    raise SkipTest()
-    check_export_roundtrip(dir)
+    def test_full_roundtrip(self):
+        self.check_export_roundtrip(DATA_DIR, "full")

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 = '''<sequential display_name="oops"><video url="hi"></sequential>'''
-
         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 = '''<sequential display_name="oops"><video url="hi"></sequential>'''
+        bad_xml2 = '''<sequential url_name="oops"><video url="hi"></sequential>'''
+        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 = '''<course graceperiod="{grace}" url_name="test1" display_name="myseq">
-                      <chapter url="hi" url_name="ch" display_name="CH">
-                       <html url_name="h" display_name="H">Two houses, ...</html></chapter>
-                   </course>'''.format(grace=v)
+        v = '1 hour'
+        org = 'foo'
+        course = 'bbhh'
+        url_name = 'test1'
+        start_xml = '''
+        <course org="{org}" course="{course}"
+                graceperiod="{grace}" url_name="{url_name}" unicorn="purple">
+            <chapter url="hi" url_name="ch" display_name="CH">
+                <html url_name="h" display_name="H">Two houses, ...</html>
+            </chapter>
+        </course>'''.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)

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
 

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: <blah url_name="something" />.
+    No children, one attribute named url_name.
+
+    Special case for course roots: the pointer is
+      <course url_name="something" org="myorg"  course="course">
+
+    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()),
     }
 
@@ -102,11 +133,31 @@ 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)
-
-        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)
-
-                    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(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'))
+        # 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)
+
+        # 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
+
+        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]
+
+        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 = ''
-
-
-            xml_object.set('filename', self.url_name)
-
-        # 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
-
-            if (metadata_key not in self.metadata or
-                metadata_key in self._inherited_metadata):
-                continue
-
-            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)
+        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))
+
+        # 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))
+
+        # And return just a pointer with the category and filename.
+        record_object = etree.Element(self.category)
+        record_object.set('url_name', self.url_name)
+
+        # 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)
+
+        return etree.tostring(record_object, pretty_print=True)
 
     def definition_to_xml(self, resource_fs):
         """

common/test/data/simple/course.xml

@@ -2,7 +2,7 @@
   <chapter name="Overview">
     <video name="Welcome" youtube="0.75:izygArpw-Qo,1.0:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8"/>
     <videosequence format="Lecture Sequence" name="A simple sequence">
-      <html id="toylab" filename="toylab"/>
+      <html name="toylab" filename="toylab"/>
       <video name="S0V1: Video Resources" youtube="0.75:EuzkdzfR0i8,1.0:1bK-WdDi6Qw,1.25:0v1VzoDVUTM,1.50:Bxk_-ZJb240"/>
     </videosequence>
     <section name="Lecture 2">
@@ -15,7 +15,7 @@
   <chapter name="Chapter 2">
     <section name="Problem Set 1">
       <sequential>
-        <problem type="lecture" showanswer="attempted" rerandomize="true" title="A simple coding problem" name="Simple coding problem" filename="ps01-simple"/>
+        <problem type="lecture" showanswer="attempted" rerandomize="true" display_name="A simple coding problem" name="Simple coding problem" filename="ps01-simple"/>
       </sequential>
     </section>
     <video name="Lost Video" youtube="1.0:TBvX7HzxexQ"/>

lms/djangoapps/courseware/courses.py

@@ -46,12 +46,15 @@ def check_course(course_id, course_must_be_open=True, course_required=True):
 
 
 def course_image_url(course):
-    return staticfiles_storage.url(course.metadata['data_dir'] + "/images/course_image.jpg")
+    return staticfiles_storage.url(course.metadata['data_dir'] +
+                                   "/images/course_image.jpg")
 
 
 def get_course_about_section(course, section_key):
     """
-    This returns the snippet of html to be rendered on the course about page, given the key for the section.
+    This returns the snippet of html to be rendered on the course about page,
+    given the key for the section.
+
     Valid keys:
     - overview
     - title
@@ -70,18 +73,23 @@ def get_course_about_section(course, section_key):
     - more_info
     """
 
-    # Many of these are stored as html files instead of some semantic markup. This can change without effecting
-    # this interface when we find a good format for defining so many snippets of text/html.
+    # Many of these are stored as html files instead of some semantic
+    # markup. This can change without effecting this interface when we find a
+    # good format for defining so many snippets of text/html.
 
 # TODO: Remove number, instructors from this list
-    if section_key in ['short_description', 'description', 'key_dates', 'video', 'course_staff_short', 'course_staff_extended',
-                        'requirements', 'syllabus', 'textbook', 'faq', 'more_info', 'number', 'instructors', 'overview',
-                        'effort', 'end_date', 'prerequisites']:
+    if section_key in ['short_description', 'description', 'key_dates', 'video',
+                       'course_staff_short', 'course_staff_extended',
+                       'requirements', 'syllabus', 'textbook', 'faq', 'more_info',
+                       'number', 'instructors', 'overview',
+                       'effort', 'end_date', 'prerequisites']:
         try:
             with course.system.resources_fs.open(path("about") / section_key + ".html") as htmlFile:
-                return replace_urls(htmlFile.read().decode('utf-8'), course.metadata['data_dir'])
+                return replace_urls(htmlFile.read().decode('utf-8'),
+                                    course.metadata['data_dir'])
         except ResourceNotFoundError:
-            log.warning("Missing about section {key} in course {url}".format(key=section_key, url=course.location.url()))
+            log.warning("Missing about section {key} in course {url}".format(
+                key=section_key, url=course.location.url()))
             return None
     elif section_key == "title":
         return course.metadata.get('display_name', course.url_name)
@@ -95,7 +103,9 @@ def get_course_about_section(course, section_key):
 
 def get_course_info_section(course, section_key):
     """
-    This returns the snippet of html to be rendered on the course info page, given the key for the section.
+    This returns the snippet of html to be rendered on the course info page,
+    given the key for the section.
+
     Valid keys:
     - handouts
     - guest_handouts
@@ -103,43 +113,51 @@ def get_course_info_section(course, section_key):
     - guest_updates
     """
 
-    # Many of these are stored as html files instead of some semantic markup. This can change without effecting
-    # this interface when we find a good format for defining so many snippets of text/html.
+    # Many of these are stored as html files instead of some semantic
+    # markup. This can change without effecting this interface when we find a
+    # good format for defining so many snippets of text/html.
 
     if section_key in ['handouts', 'guest_handouts', 'updates', 'guest_updates']:
         try:
             with course.system.resources_fs.open(path("info") / section_key + ".html") as htmlFile:
-                return replace_urls(htmlFile.read().decode('utf-8'), course.metadata['data_dir'])
+                return replace_urls(htmlFile.read().decode('utf-8'),
+                                    course.metadata['data_dir'])
         except ResourceNotFoundError:
-            log.exception("Missing info section {key} in course {url}".format(key=section_key, url=course.location.url()))
+            log.exception("Missing info section {key} in course {url}".format(
+                key=section_key, url=course.location.url()))
             return "! Info section missing !"
 
     raise KeyError("Invalid about key " + str(section_key))
 
 def course_staff_group_name(course):
     '''
-    course should be either a CourseDescriptor instance, or a string (the .course entry of a Location)
+    course should be either a CourseDescriptor instance, or a string (the
+    .course entry of a Location)
     '''
-    if isinstance(course,str):
+    if isinstance(course, str) or isinstance(course, unicode):
         coursename = course
     else:
-        coursename = course.metadata.get('data_dir','UnknownCourseName')
-    if not coursename:					# Fall 2012: not all course.xml have metadata correct yet
-        coursename = course.metadata.get('course','')
+        # should be a CourseDescriptor, so grab its location.course:
+        coursename = course.location.course
     return 'staff_%s' % coursename
 
-def has_staff_access_to_course(user,course):
+def has_staff_access_to_course(user, course):
     '''
     Returns True if the given user has staff access to the course.
     This means that user is in the staff_* group, or is an overall admin.
+
+    course is the course field of the location being accessed.
     '''
     if user is None or (not user.is_authenticated()) or course is None:
         return False
     if user.is_staff:
         return True
-    user_groups = [x[1] for x in user.groups.values_list()]	# note this is the Auth group, not UserTestGroup
+
+    # note this is the Auth group, not UserTestGroup
+    user_groups = [x[1] for x in user.groups.values_list()]
     staff_group = course_staff_group_name(course)
-    log.debug('course %s user %s groups %s' % (staff_group, user, user_groups))
+    log.debug('course %s, staff_group %s, user %s, groups %s' % (
+        course, staff_group, user, user_groups))
     if staff_group in user_groups:
         return True
     return False
@@ -154,7 +172,8 @@ def get_courses_by_university(user):
     Returns dict of lists of courses available, keyed by course.org (ie university).
     Courses are sorted by course.number.
 
-    if ACCESS_REQUIRE_STAFF_FOR_COURSE then list only includes those accessible to user.
+    if ACCESS_REQUIRE_STAFF_FOR_COURSE then list only includes those accessible
+    to user.
     '''
     # TODO: Clean up how 'error' is done.
     # filter out any courses that errored.
@@ -168,4 +187,4 @@ def get_courses_by_university(user):
                 continue
         universities[course.org].append(course)
     return universities
-    
+

lms/djangoapps/courseware/tests/tests.py

@@ -135,10 +135,25 @@ class ActivateLoginTestCase(TestCase):
 class PageLoader(ActivateLoginTestCase):
     ''' Base class that adds a function to load all pages in a modulestore '''
 
+
+    def enroll(self, course):
+        resp = self.client.post('/change_enrollment', {
+            'enrollment_action': 'enroll',
+            'course_id': course.id,
+            })
+        data = parse_json(resp)
+        self.assertTrue(data['success'])
+
     def check_pages_load(self, course_name, data_dir, modstore):
         print "Checking course {0} in {1}".format(course_name, data_dir)
         import_from_xml(modstore, data_dir, [course_name])
 
+        # enroll in the course before trying to access pages
+        courses = modstore.get_courses()
+        self.assertEqual(len(courses), 1)
+        course = courses[0]
+        self.enroll(course)
+
         n = 0
         num_bad = 0
         all_ok = True

lms/envs/common.py

@@ -157,6 +157,9 @@ COURSE_SETTINGS =  {'6.002x_Fall_2012': {'number' : '6.002x',
                                           }
                     }
 
+# IP addresses that are allowed to reload the course, etc.
+# TODO (vshnayder): Will probably need to change as we get real access control in.
+LMS_MIGRATION_ALLOWED_IPS = []
 
 ############################### XModule Store ##################################
 MODULESTORE = {

lms/templates/portal/course_about.html

@@ -20,7 +20,7 @@
           if(json.success) {
             location.href="${reverse('dashboard')}";
           }else{
-	    $('#register_message).html("<p><font color='red'>" + json.error + "</font></p>")
+	    $('#register_message').html("<p><font color='red'>" + json.error + "</font></p>");
           }
         });
       })(this)