From 67949f8a323ea06a2af6eecab01f7bb0bf201916 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 10 Aug 2012 13:14:58 -0400 Subject: [PATCH] Fix metadata inheritance * with xml datastore, re-do all inheritance once the whole course is loaded --- .../lib/xmodule/xmodule/modulestore/mongo.py | 3 +++ common/lib/xmodule/xmodule/modulestore/xml.py | 6 +++++ .../lib/xmodule/xmodule/tests/test_import.py | 22 +++++++++++++++++++ common/lib/xmodule/xmodule/x_module.py | 19 ++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 1cec6c7f87..b6101a6929 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -55,6 +55,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem): if json_data is None: return self.modulestore.get_item(location) else: + # TODO (vshnayder): metadata inheritance is somewhat broken because mongo, doesn't + # always load an entire course. We're punting on this until after launch, and then + # will build a proper course policy framework. return XModuleDescriptor.load_from_json(json_data, self, self.default_class) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index f9093f355a..2dc3b33323 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -213,6 +213,12 @@ class XMLModuleStore(ModuleStoreBase): system = ImportSystem(self, org, course, course_dir, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) + # NOTE: The descriptors end up loading somewhat bottom up, which + # breaks metadata inheritance via get_children(). Instead + # (actually, in addition to, for now), we do a final inheritance pass + # after we have the course descriptor. + XModuleDescriptor.compute_inherited_metadata(course_descriptor) + log.debug('========> Done with course import from {0}'.format(course_dir)) return course_descriptor diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 407057a4ab..1da618f6a4 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -8,8 +8,11 @@ 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.xml import XMLModuleStore from xmodule.modulestore.exceptions import ItemNotFoundError +from .test_export import DATA_DIR + ORG = 'test_org' COURSE = 'test_course' @@ -185,3 +188,22 @@ class ImportTestCase(unittest.TestCase): chapter_xml = etree.fromstring(f.read()) self.assertEqual(chapter_xml.tag, 'chapter') self.assertFalse('graceperiod' in chapter_xml.attrib) + + def test_metadata_inherit(self): + """Make sure that metadata is inherited properly""" + + print "Starting import" + initial_import = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy']) + + courses = initial_import.get_courses() + self.assertEquals(len(courses), 1) + course = courses[0] + + def check_for_key(key, node): + "recursive check for presence of key" + print "Checking {}".format(node.location.url()) + self.assertTrue(key in node.metadata) + for c in node.get_children(): + check_for_key(key, c) + + check_for_key('graceperiod', course) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 72a8d32d1d..f196d32c31 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -403,6 +403,21 @@ class XModuleDescriptor(Plugin, HTMLSnippet): return dict((k,v) for k,v in self.metadata.items() if k not in self._inherited_metadata) + @staticmethod + def compute_inherited_metadata(course): + """Given a course descriptor, traverse the entire course tree and do + metadata inheritance. Should be called after importing a course. + + NOTE: This means that there is no such thing as lazy loading at the + moment--this accesses all the children.""" + def do_inherit(node): + for c in node.get_children(): + c.inherit_metadata(node.metadata) + do_inherit(c) + + do_inherit(course) + + def inherit_metadata(self, metadata): """ Updates this module with metadata inherited from a containing module. @@ -415,6 +430,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): if attr not in self.metadata and attr in metadata: self._inherited_metadata.add(attr) self.metadata[attr] = metadata[attr] + print "final self.metadata: {}".format(self.metadata) def get_children(self): """Returns a list of XModuleDescriptor instances for the children of @@ -423,6 +439,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): self._child_instances = [] for child_loc in self.definition.get('children', []): child = self.system.load_item(child_loc) + # TODO (vshnayder): this should go away once we have + # proper inheritance support in mongo. The xml + # datastore does all inheritance on course load. child.inherit_metadata(self.metadata) self._child_instances.append(child)