Merge pull request #334 from MITx/feature/victor/metadata-inherit

Feature/victor/metadata inherit

cms/djangoapps/contentstore/views.py

@@ -176,7 +176,7 @@ def load_preview_state(request, preview_id, location):
 
 def save_preview_state(request, preview_id, location, instance_state, shared_state):
     """
-    Load the state of a preview module to the request
+    Save the state of a preview module to the request
 
     preview_id (str): An identifier specifying which preview this module is used for
     location: The Location of the module to dispatch to

cms/envs/dev.py

@@ -2,13 +2,18 @@
 This config file runs the simplest dev environment"""
 
 from .common import *
+from .logsettings import get_logger_config
 
 import logging
 import sys
-logging.basicConfig(stream=sys.stdout, )
 
 DEBUG = True
 TEMPLATE_DEBUG = DEBUG
+LOGGING = get_logger_config(ENV_ROOT / "log",
+                            logging_env="dev",
+                            tracking_filename="tracking.log",
+                            debug=True)
+
 
 MODULESTORE = {
     'default': {

cms/envs/logsettings.py

@@ -3,19 +3,19 @@ import os.path
 import platform
 import sys
 
-def get_logger_config(log_dir, 
-                      logging_env="no_env", 
+def get_logger_config(log_dir,
+                      logging_env="no_env",
                       tracking_filename=None,
                       syslog_addr=None,
                       debug=False):
     """Return the appropriate logging config dictionary. You should assign the
-    result of this to the LOGGING var in your settings. The reason it's done 
+    result of this to the LOGGING var in your settings. The reason it's done
     this way instead of registering directly is because I didn't want to worry
-    about resetting the logging state if this is called multiple times when 
+    about resetting the logging state if this is called multiple times when
     settings are extended."""
 
     # If we're given an explicit place to put tracking logs, we do that (say for
-    # debugging). However, logging is not safe for multiple processes hitting 
+    # debugging). However, logging is not safe for multiple processes hitting
     # the same file. So if it's left blank, we dynamically create the filename
     # based on the PID of this worker process.
     if tracking_filename:
@@ -33,6 +33,7 @@ def get_logger_config(log_dir,
 
     return {
         'version': 1,
+        'disable_existing_loggers': False,
         'formatters' : {
             'standard' : {
                 'format' : '%(asctime)s %(levelname)s %(process)d [%(name)s] %(filename)s:%(lineno)d - %(message)s',

common/lib/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
 from fs.osfs import OSFS
 
@@ -26,3 +27,10 @@ def check_export_roundtrip(data_dir):
     for location in initial_import.modules.keys():
         print "Checking", location
         assert_equals(initial_import.modules[location], second_import.modules[location])
+
+
+def test_toy_roundtrip():
+    dir = ""
+    # TODO: add paths and make this run.
+    raise SkipTest()
+    check_export_roundtrip(dir)

common/lib/xmodule/tests/test_import.py

 from path import path
 import unittest
+from fs.memoryfs import MemoryFS
 
 from lxml import etree
 
 from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
-from xmodule.errortracker import null_error_tracker
+from xmodule.errortracker import make_error_tracker
 from xmodule.modulestore import Location
+from xmodule.modulestore.exceptions import ItemNotFoundError
 
-class ImportTestCase(unittest.TestCase):
-    '''Make sure module imports work properly, including for malformed inputs'''
+ORG = 'test_org'
+COURSE = 'test_course'
+
+class DummySystem(XMLParsingSystem):
+    def __init__(self):
+
+        self.modules = {}
+        self.resources_fs = MemoryFS()
+        self.errorlog = make_error_tracker()
 
-    @staticmethod
-    def get_system():
-        '''Get a dummy system'''
-        # Shouldn't need any system params, because the initial parse should fail
         def load_item(loc):
-            raise Exception("Shouldn't be called")
+            loc = Location(loc)
+            if loc in self.modules:
+                return self.modules[loc]
 
-        resources_fs = None
+            print "modules: "
+            print self.modules
+            raise ItemNotFoundError("Can't find item at loc: {0}".format(loc))
 
         def process_xml(xml):
-            raise Exception("Shouldn't be called")
+            print "loading {0}".format(xml)
+            descriptor = XModuleDescriptor.load_from_xml(xml, self, ORG, COURSE, None)
+            # Need to save module so we can find it later
+            self.modules[descriptor.location] = descriptor
+
+            # always eager
+            descriptor.get_children()
+            return descriptor
+
 
+        XMLParsingSystem.__init__(self, load_item, self.resources_fs,
+                                  self.errorlog.tracker, process_xml)
 
-        def render_template(template, context):
+    def render_template(self, template, context):
             raise Exception("Shouldn't be called")
 
-        system = XMLParsingSystem(load_item, resources_fs,
-                                  null_error_tracker, process_xml)
-        system.render_template = render_template
 
-        return system
+
+
+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.'''
@@ -85,3 +110,30 @@ 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"""
+        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)
+        descriptor = XModuleDescriptor.load_from_xml(start_xml, system,
+                                                     'org', 'course')
+
+        print "Errors: {0}".format(system.errorlog.errors)
+        print descriptor, descriptor.metadata
+        self.assertEqual(descriptor.metadata['graceperiod'], v)
+
+        # Check that the child inherits correctly
+        child = descriptor.get_children()[0]
+        self.assertEqual(child.metadata['graceperiod'], v)
+
+        # Now export and see if the chapter tag has a graceperiod attribute
+        resource_fs = MemoryFS()
+        exported_xml = descriptor.export_to_xml(resource_fs)
+        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)

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -41,6 +41,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         self.used_slugs = set()
 
         def process_xml(xml):
+            """Takes an xml string, and returns a XModuleDescriptor created from
+            that xml.
+            """
             try:
                 # VS[compat]
                 # TODO (cpennington): Remove this once all fall 2012 courses

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -35,6 +35,8 @@ def import_from_xml(store, data_dir, course_dirs=None, eager=True,
             store.update_item(module.location, module.definition['data'])
         if 'children' in module.definition:
             store.update_children(module.location, module.definition['children'])
-        store.update_metadata(module.location, dict(module.metadata))
+        # NOTE: It's important to use own_metadata here to avoid writing
+        # inherited metadata everywhere.
+        store.update_metadata(module.location, dict(module.own_metadata))
 
     return module_store

common/lib/xmodule/xmodule/x_module.py

@@ -358,6 +358,14 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         self._child_instances = None
         self._inherited_metadata = set()
 
+    @property
+    def own_metadata(self):
+        """
+        Return the metadata that is not inherited, but was defined on this module.
+        """
+        return dict((k,v) for k,v in self.metadata.items()
+                    if k not in self._inherited_metadata)
+
     def inherit_metadata(self, metadata):
         """
         Updates this module with metadata inherited from a containing module.

lms/djangoapps/courseware/management/commands/clean_xml.py

@@ -53,14 +53,12 @@ def import_with_checks(course_dir, verbose=True):
     data_dir = course_dir.dirname()
     course_dirs = [course_dir.basename()]
 
-    (error_tracker, errors) = make_error_tracker()
     # No default class--want to complain if it doesn't find plugins for any
     # module.
     modulestore = XMLModuleStore(data_dir,
                    default_class=None,
                    eager=True,
-                   course_dirs=course_dirs,
-                   error_tracker=error_tracker)
+                   course_dirs=course_dirs)
 
     def str_of_err(tpl):
         (msg, exc_info) = tpl
@@ -71,6 +69,15 @@ def import_with_checks(course_dir, verbose=True):
         return '{msg}\n{exc}'.format(msg=msg, exc=exc_str)
 
     courses = modulestore.get_courses()
+
+    n = len(courses)
+    if n != 1:
+        print 'ERROR: Expect exactly 1 course.  Loaded {n}: {lst}'.format(
+            n=n, lst=courses)
+        return (False, None)
+
+    course = courses[0]
+    errors = modulestore.get_item_errors(course.location)
     if len(errors) != 0:
         all_ok = False
         print '\n'
@@ -80,13 +87,6 @@ def import_with_checks(course_dir, verbose=True):
         print "=" * 40
         print '\n'
 
-    n = len(courses)
-    if n != 1:
-        print 'ERROR: Expect exactly 1 course.  Loaded {n}: {lst}'.format(
-            n=n, lst=courses)
-        return (False, None)
-
-    course = courses[0]
 
     #print course
     validators = (