Merge branch 'master' of github.com:MITx/mitx into merge

Conflicts:
	lms/urls.py

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/capa/capa/responsetypes.py

@@ -800,6 +800,12 @@ class CodeResponse(LoncapaResponse):
     '''
     Grade student code using an external queueing server, called 'xqueue'
 
+    Expects 'xqueue' dict in ModuleSystem with the following keys:
+        system.xqueue = { 'interface': XqueueInterface object,
+                          'callback_url': Per-StudentModule callback URL where results are posted (string),
+                          'default_queuename': Default queuename to submit request (string)
+                        }
+
     External requests are only submitted for student submission grading 
         (i.e. and not for getting reference answers)
     '''
@@ -873,15 +879,16 @@ class CodeResponse(LoncapaResponse):
                     'edX_cmd': 'get_score',
                     'edX_tests': self.tests,
                     'processor': self.code,
-                    'edX_student_response': unicode(submission), # unicode on File object returns its filename
                     }
         
         # Submit request
-        if hasattr(submission, 'read'): # Test for whether submission is a file
+        if is_file(submission):
+            contents.update({'edX_student_response': submission.name})
             (error, msg) = qinterface.send_to_queue(header=xheader,
                                                     body=json.dumps(contents),
                                                     file_to_upload=submission)
         else:
+            contents.update({'edX_student_response': submission})
             (error, msg) = qinterface.send_to_queue(header=xheader,
                                                     body=json.dumps(contents))
 

common/lib/capa/capa/util.py

@@ -39,5 +39,18 @@ def convert_files_to_filenames(answers):
     '''
     new_answers = dict()
     for answer_id in answers.keys():
-        new_answers[answer_id] = unicode(answers[answer_id])
+        if is_file(answers[answer_id]):
+            new_answers[answer_id] = answers[answer_id].name
+        else:
+            new_answers[answer_id] = answers[answer_id]
     return new_answers
+
+def is_file(file_to_test):
+    '''
+    Duck typing to check if 'file_to_test' is a File object
+    '''
+    is_file = True
+    for method in ['read', 'name']:
+        if not hasattr(file_to_test, method):
+            is_file = False
+    return is_file 

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'))
@@ -130,7 +130,7 @@ class CapaModule(XModule):
         if weight_string:
             self.weight = float(weight_string)
         else:
-            self.weight = 1
+            self.weight = None
 
         if self.rerandomize == 'never':
             seed = 1
@@ -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,6 +3,7 @@ import time
 import dateutil.parser
 import logging
 
+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
@@ -12,13 +13,9 @@ 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)
-        
-        self._grader = None
-        self._grade_cutoffs = None
 
         msg = None
         try:
@@ -39,34 +36,84 @@ class CourseDescriptor(SequenceDescriptor):
 
     def has_started(self):
         return time.gmtime() > self.start
-    
+
     @property
     def grader(self):
-        self.__load_grading_policy()
-        return self._grader
-    
+        return self.__grading_policy['GRADER']
+
     @property
     def grade_cutoffs(self):
-        self.__load_grading_policy()
-        return self._grade_cutoffs
-    
-    
-    def __load_grading_policy(self):
-        if not self._grader or not self._grade_cutoffs:
-            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)
-            
-            self._grader = grading_policy['GRADER']
-            self._grade_cutoffs = grading_policy['GRADE_CUTOFFS']
-    
-    
+        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:
+        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
+                    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.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/css/video/display.scss

@@ -207,7 +207,7 @@ div.video {
             h3 {
               color: #999;
               float: left;
-              font-size: 12px;
+              font-size: em(14);
               font-weight: normal;
               letter-spacing: 1px;
               padding: 0 lh(.25) 0 lh(.5);
@@ -221,6 +221,7 @@ div.video {
               margin-bottom: 0;
               padding: 0 lh(.5) 0 0;
               line-height: 46px;
+              color: #fff;
             }
 
             &:hover, &:active, &:focus {

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)

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

@@ -15,6 +15,7 @@ import xmodule
 import capa.calc as calc
 import capa.capa_problem as lcp
 from capa.correctmap import CorrectMap
+from capa.util import convert_files_to_filenames
 from xmodule import graders, x_module
 from xmodule.x_module import ModuleSystem
 from xmodule.graders import Score, aggregate_scores
@@ -31,7 +32,7 @@ i4xs = ModuleSystem(
     user=Mock(),
     filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))),
     debug=True,
-    xqueue_callback_url='/',
+    xqueue={'interface':None, 'callback_url':'/', 'default_queuename': 'testqueue'},
     is_staff=False
 )
 
@@ -278,7 +279,6 @@ class StringResponseWithHintTest(unittest.TestCase):
 class CodeResponseTest(unittest.TestCase):
     '''
     Test CodeResponse
-
     '''
     def test_update_score(self):
         problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml"
@@ -327,7 +327,18 @@ class CodeResponseTest(unittest.TestCase):
                         self.assertFalse(test_lcp.correct_map.is_queued(answer_ids[j]))  # Should be dequeued, message delivered
                     else:
                         self.assertTrue(test_lcp.correct_map.is_queued(answer_ids[j]))  # Should be queued, message undelivered
-
+    
+    def test_convert_files_to_filenames(self):
+        problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml"
+        fp = open(problem_file)
+        answers_with_file = {'1_2_1': 'String-based answer',
+                             '1_3_1': ['answer1', 'answer2', 'answer3'],
+                             '1_4_1': fp}
+        answers_converted = convert_files_to_filenames(answers_with_file)
+        self.assertEquals(answers_converted['1_2_1'], 'String-based answer')
+        self.assertEquals(answers_converted['1_3_1'], ['answer1', 'answer2', 'answer3'])
+        self.assertEquals(answers_converted['1_4_1'], fp.name)
+        
 
 class ChoiceResponseTest(unittest.TestCase):
 

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

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/util/decorators.py

@@ -0,0 +1,36 @@
+
+
+def lazyproperty(fn):
+    """
+    Use this decorator for lazy generation of properties that
+    are expensive to compute. From http://stackoverflow.com/a/3013910/86828
+    
+    
+    Example:
+    class Test(object):
+
+        @lazyproperty
+        def a(self):
+            print 'generating "a"'
+            return range(5)
+    
+    Interactive Session:
+    >>> t = Test()
+    >>> t.__dict__
+    {}
+    >>> t.a
+    generating "a"
+    [0, 1, 2, 3, 4]
+    >>> t.__dict__
+    {'_lazy_a': [0, 1, 2, 3, 4]}
+    >>> t.a
+    [0, 1, 2, 3, 4]
+    """
+    
+    attr_name = '_lazy_' + fn.__name__
+    @property
+    def _lazyprop(self):
+        if not hasattr(self, attr_name):
+            setattr(self, attr_name, fn(self))
+        return getattr(self, attr_name)
+    return _lazyprop

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
 
@@ -643,7 +644,7 @@ class ModuleSystem(object):
                  user=None,
                  filestore=None,
                  debug=False,
-                 xqueue = None,
+                 xqueue=None,
                  is_staff=False):
         '''
         Create a closure around the system environment.

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()),
     }
 
@@ -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):
         """

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