From 00d9ecd6001d3441931687485b2b81d36c3ce6fd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:41:19 -0400 Subject: [PATCH] New policy organization: * course roots live in roots/{url_name}.xml - one is linked from course.xml * policies live in policies/url_name.json - loaded based on course url_name * Updated to pass policy through into xml parsing, so it takes effect before descriptor constructors are called. * Update toy test course to new structure, fix up tests --- .../xmodule/modulestore/tests/test_mongo.py | 4 +-- common/lib/xmodule/xmodule/modulestore/xml.py | 23 ++++++++-------- .../lib/xmodule/xmodule/tests/test_import.py | 4 +-- common/lib/xmodule/xmodule/x_module.py | 26 +++---------------- common/lib/xmodule/xmodule/xml_module.py | 7 ++++- common/test/data/toy/course.xml | 10 +------ common/test/data/toy/course/2012_Fall.xml | 9 +++++++ common/test/data/toy/policies/2012_Fall.json | 23 ++++++++++++++++ common/test/data/toy/roots/2012_Fall.xml | 1 + 9 files changed, 59 insertions(+), 48 deletions(-) mode change 100644 => 120000 common/test/data/toy/course.xml create mode 100644 common/test/data/toy/course/2012_Fall.xml create mode 100644 common/test/data/toy/policies/2012_Fall.json create mode 100644 common/test/data/toy/roots/2012_Fall.xml diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 24f0441ee0..cb94444b7a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -112,8 +112,8 @@ class TestMongoModuleStore(object): should_work = ( ("i4x://edX/toy/video/Welcome", ("edX/toy/2012_Fall", "Overview", "Welcome", None)), - ("i4x://edX/toy/html/toylab", - ("edX/toy/2012_Fall", "Overview", "Toy_Videos", None)), + ("i4x://edX/toy/chapter/Overview", + ("edX/toy/2012_Fall", "Overview", None, None)), ) for location, expected in should_work: assert_equals(path_to_location(self.store, location), expected) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 9fc0ba29ca..e52126a2c8 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -31,7 +31,8 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, error_tracker, **kwargs): + def __init__(self, xmlstore, org, course, course_dir, + policy, error_tracker, **kwargs): """ A class that handles loading from xml. Does some munging to ensure that all elements have unique slugs. @@ -97,7 +98,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): MakoDescriptorSystem.__init__(self, load_item, resources_fs, error_tracker, render_template, **kwargs) XMLParsingSystem.__init__(self, load_item, resources_fs, - error_tracker, process_xml, **kwargs) + error_tracker, process_xml, policy, **kwargs) class XMLModuleStore(ModuleStoreBase): @@ -184,6 +185,7 @@ class XMLModuleStore(ModuleStoreBase): if not os.path.exists(policy_path): return {} try: + log.debug("Loading policy from {}".format(policy_path)) with open(policy_path) as f: return json.load(f) except (IOError, ValueError) as err: @@ -232,19 +234,16 @@ class XMLModuleStore(ModuleStoreBase): tracker(msg) course = course_dir - system = ImportSystem(self, org, course, course_dir, tracker) + url_name = course_data.get('url_name') + if url_name: + policy_path = self.data_dir / course_dir / 'policies' / '{}.json'.format(url_name) + policy = self.load_policy(policy_path, tracker) + else: + policy = {} - policy_path = self.data_dir / course_dir / 'policy.json' - policy = self.load_policy(policy_path, tracker) - # Special case -- need to change the url_name of the course before - # it gets loaded, so its location and other fields are right - if 'course_url_name' in policy: - new_url_name = policy['course_url_name'] - log.info("changing course url_name to {}".format(new_url_name)) - course_data.set('url_name', new_url_name) + system = ImportSystem(self, org, course, course_dir, policy, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) - XModuleDescriptor.apply_policy(course_descriptor, policy) # NOTE: The descriptors end up loading somewhat bottom up, which # breaks metadata inheritance via get_children(). Instead diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 1da618f6a4..dfa75f9137 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -42,9 +42,9 @@ class DummySystem(XMLParsingSystem): descriptor.get_children() return descriptor - + policy = {} XMLParsingSystem.__init__(self, load_item, self.resources_fs, - self.errorlog.tracker, process_xml) + self.errorlog.tracker, process_xml, policy) def render_template(self, template, context): raise Exception("Shouldn't be called") diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index d68dd61e3d..f598204454 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -303,10 +303,6 @@ def policy_key(location): Get the key for a location in a policy file. (Since the policy file is specific to a course, it doesn't need the full location url). """ - # Special case--we need to be able to override the url_name on a course, - # so special case where we look for the course descriptor - if location.category == 'course': - return 'course' return '{cat}/{name}'.format(cat=location.category, name=location.name) @@ -429,23 +425,6 @@ class XModuleDescriptor(Plugin, HTMLSnippet): if k not in self._inherited_metadata) - @staticmethod - def apply_policy(node, policy): - """ - Given a descriptor, traverse all its descendants and update its metadata - with the policy. - - Notes: - - this does not propagate inherited metadata. The caller should - call compute_inherited_metadata after applying the policy. - - metadata specified in the policy overrides metadata in the xml - """ - k = policy_key(node.location) - if k in policy: - node.metadata.update(policy[k]) - for c in node.get_children(): - XModuleDescriptor.apply_policy(c, policy) - @staticmethod def compute_inherited_metadata(node): """Given a descriptor, traverse all of its descendants and do metadata @@ -701,16 +680,19 @@ class DescriptorSystem(object): class XMLParsingSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, error_tracker, process_xml, **kwargs): + def __init__(self, load_item, resources_fs, error_tracker, process_xml, policy, **kwargs): """ load_item, resources_fs, error_tracker: see DescriptorSystem + policy: a policy dictionary for overriding xml metadata + process_xml: Takes an xml string, and returns a XModuleDescriptor created from that xml """ DescriptorSystem.__init__(self, load_item, resources_fs, error_tracker, **kwargs) self.process_xml = process_xml + self.policy = policy class ModuleSystem(object): diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 3c2f3269ca..c7042efda2 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -1,4 +1,4 @@ -from xmodule.x_module import XModuleDescriptor +from xmodule.x_module import (XModuleDescriptor, policy_key) from xmodule.modulestore import Location from lxml import etree import json @@ -270,6 +270,11 @@ class XmlDescriptor(XModuleDescriptor): log.debug('Error %s in loading metadata %s' % (err,dmdata)) metadata['definition_metadata_err'] = str(err) + # Set/override any metadata specified by policy + k = policy_key(location) + if k in system.policy: + metadata.update(system.policy[k]) + return cls( system, definition, diff --git a/common/test/data/toy/course.xml b/common/test/data/toy/course.xml deleted file mode 100644 index 270a1eb27f..0000000000 --- a/common/test/data/toy/course.xml +++ /dev/null @@ -1,9 +0,0 @@ - - - - - - - diff --git a/common/test/data/toy/course.xml b/common/test/data/toy/course.xml new file mode 120000 index 0000000000..49041310f6 --- /dev/null +++ b/common/test/data/toy/course.xml @@ -0,0 +1 @@ +roots/2012_Fall.xml \ No newline at end of file diff --git a/common/test/data/toy/course/2012_Fall.xml b/common/test/data/toy/course/2012_Fall.xml new file mode 100644 index 0000000000..d34eb9d56a --- /dev/null +++ b/common/test/data/toy/course/2012_Fall.xml @@ -0,0 +1,9 @@ + + + + + + + diff --git a/common/test/data/toy/policies/2012_Fall.json b/common/test/data/toy/policies/2012_Fall.json new file mode 100644 index 0000000000..6c501d66f8 --- /dev/null +++ b/common/test/data/toy/policies/2012_Fall.json @@ -0,0 +1,23 @@ +{ + "course/2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2015-07-17T12:00", + "display_name": "Toy Course" + }, + "chapter/Overview": { + "display_name": "Overview" + }, + "videosequence/Toy_Videos": { + "display_name": "Toy Videos", + "format": "Lecture Sequence" + }, + "html/toylab": { + "display_name": "Toy lab" + }, + "video/Video_Resources": { + "display_name": "Video Resources" + }, + "video/Welcome": { + "display_name": "Welcome" + } +} diff --git a/common/test/data/toy/roots/2012_Fall.xml b/common/test/data/toy/roots/2012_Fall.xml new file mode 100644 index 0000000000..b71528809b --- /dev/null +++ b/common/test/data/toy/roots/2012_Fall.xml @@ -0,0 +1 @@ + \ No newline at end of file