From ce29b5aec20daa4c04150770a887beb87656e55f Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 16:06:42 -0400 Subject: [PATCH 1/7] Special case handling of course url_names in policy.json --- common/lib/xmodule/xmodule/modulestore/xml.py | 10 ++++++++-- common/lib/xmodule/xmodule/x_module.py | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 0c0d750882..9fc0ba29ca 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -234,10 +234,16 @@ class XMLModuleStore(ModuleStoreBase): system = ImportSystem(self, org, course, course_dir, tracker) - course_descriptor = system.process_xml(etree.tostring(course_data)) 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) + + 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 diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index af3f04e366..d68dd61e3d 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -303,6 +303,10 @@ 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) From f717b0421f250bd2fb7de8652a1e652e9fcf5b4c Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 21:39:10 -0400 Subject: [PATCH 2/7] fix bug in backcompat section replacement (s/sequence/sequential/) --- common/lib/xmodule/xmodule/backcompat_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index c49f23b99e..d9f10ffba0 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -74,7 +74,7 @@ class SemanticSectionDescriptor(XModuleDescriptor): return system.process_xml(etree.tostring(xml_object[0])) else: - xml_object.tag = 'sequence' + xml_object.tag = 'sequential' return system.process_xml(etree.tostring(xml_object)) From 5e2f676153899b3333aa5111865e4aefd2210a48 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:36:05 -0400 Subject: [PATCH 3/7] backcompat warnings --- common/lib/xmodule/xmodule/backcompat_module.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index d9f10ffba0..06713d3432 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -21,6 +21,7 @@ def process_includes(fn): xml_object = etree.fromstring(xml_data) next_include = xml_object.find('include') while next_include is not None: + system.error_tracker("WARNING: the tag is deprecated, and will go away.") file = next_include.get('file') parent = next_include.getparent() @@ -67,6 +68,8 @@ class SemanticSectionDescriptor(XModuleDescriptor): the child element """ xml_object = etree.fromstring(xml_data) + system.error_tracker("WARNING: the <{}> tag is deprecated. Please do not use in new content." + .format(xml_object.tag)) if len(xml_object) == 1: for (key, val) in xml_object.items(): @@ -83,10 +86,14 @@ class TranslateCustomTagDescriptor(XModuleDescriptor): def from_xml(cls, xml_data, system, org=None, course=None): """ Transforms the xml_data from <$custom_tag attr="" attr=""/> to - $custom_tag + """ xml_object = etree.fromstring(xml_data) + system.error_tracker('WARNING: the <{tag}> tag is deprecated. ' + 'Instead, use . ' + .format(xml_object.tag)) + tag = xml_object.tag xml_object.tag = 'customtag' xml_object.attrib['impl'] = tag From 691744e3599fc537c9b4f11c0902454660d3722b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:37:54 -0400 Subject: [PATCH 4/7] Use .display_name instead of metadata['display_name'] * the former does a fallback if metadata['display_name'] isn't set. --- common/lib/xmodule/xmodule/capa_module.py | 4 ++-- common/lib/xmodule/xmodule/course_module.py | 2 +- common/lib/xmodule/xmodule/seq_module.py | 4 ++-- lms/djangoapps/lms_migration/migrate.py | 12 ++++++------ 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 46e02542c8..791ec8be45 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -237,7 +237,7 @@ class CapaModule(XModule): else: raise - content = {'name': self.metadata['display_name'], + content = {'name': self.display_name, 'html': html, 'weight': self.weight, } @@ -464,7 +464,7 @@ class CapaModule(XModule): return {'success': msg} log.exception("Error in capa_module problem checking") raise Exception("error in capa_module") - + self.attempts = self.attempts + 1 self.lcp.done = True diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 11d4e090f9..04b38b55ad 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -140,7 +140,7 @@ class CourseDescriptor(SequenceDescriptor): @property def title(self): - return self.metadata['display_name'] + return self.display_name @property def number(self): diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 2986c948d3..fee4d53700 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -76,7 +76,7 @@ class SequenceModule(XModule): contents.append({ 'content': child.get_html(), 'title': "\n".join( - grand_child.metadata['display_name'].strip() + grand_child.display_name.strip() for grand_child in child.get_children() if 'display_name' in grand_child.metadata ), @@ -107,7 +107,7 @@ class SequenceModule(XModule): class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): mako_template = 'widgets/sequence-edit.html' module_class = SequenceModule - + stores_state = True # For remembering where in the sequence the student is @classmethod diff --git a/lms/djangoapps/lms_migration/migrate.py b/lms/djangoapps/lms_migration/migrate.py index 2bf893507b..dfdf86b4ac 100644 --- a/lms/djangoapps/lms_migration/migrate.py +++ b/lms/djangoapps/lms_migration/migrate.py @@ -35,7 +35,7 @@ def manage_modulestores(request,reload_dir=None): ip = request.META.get('HTTP_X_REAL_IP','') # nginx reverse proxy if not ip: ip = request.META.get('REMOTE_ADDR','None') - + if LOCAL_DEBUG: html += '

IP address: %s ' % ip html += '

User: %s ' % request.user @@ -48,7 +48,7 @@ def manage_modulestores(request,reload_dir=None): html += 'Permission denied' html += "" log.debug('request denied, ALLOWED_IPS=%s' % ALLOWED_IPS) - return HttpResponse(html) + return HttpResponse(html) #---------------------------------------- # reload course if specified @@ -74,10 +74,10 @@ def manage_modulestores(request,reload_dir=None): #---------------------------------------- dumpfields = ['definition','location','metadata'] - + for cdir, course in def_ms.courses.items(): html += '
' - html += '

Course: %s (%s)

' % (course.metadata['display_name'],cdir) + html += '

Course: %s (%s)

' % (course.display_name,cdir) for field in dumpfields: data = getattr(course,field) @@ -89,7 +89,7 @@ def manage_modulestores(request,reload_dir=None): html += '' else: html += '
  • %s
' % escape(data) - + #---------------------------------------- @@ -107,4 +107,4 @@ def manage_modulestores(request,reload_dir=None): log.debug('def_ms=%s' % unicode(def_ms)) html += "" - return HttpResponse(html) + return HttpResponse(html) From 00d9ecd6001d3441931687485b2b81d36c3ce6fd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:41:19 -0400 Subject: [PATCH 5/7] 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 From c02313a0990b6e3dd21f4c43d726cc0e2b5c8820 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:43:40 -0400 Subject: [PATCH 6/7] hackish cleanup script --- common/xml_cleanup.py | 112 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100755 common/xml_cleanup.py diff --git a/common/xml_cleanup.py b/common/xml_cleanup.py new file mode 100755 index 0000000000..8e794b97c2 --- /dev/null +++ b/common/xml_cleanup.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python + +""" +Victor's xml cleanup script. A big pile of useful hacks. Do not use +without carefully reading the code and deciding that this is what you want. + +In particular, the remove-meta option is only intended to be used after pulling out a policy +using the metadata_to_json management command. +""" + +import os, fnmatch, re, sys +from lxml import etree +from collections import defaultdict + +INVALID_CHARS = re.compile(r"[^\w.-]") + +def clean(value): + """ + Return value, made into a form legal for locations + """ + return re.sub('_+', '_', INVALID_CHARS.sub('_', value)) + + +# category -> set of url_names for that category that we've already seen +used_names = defaultdict(set) + +def clean_unique(category, name): + cleaned = clean(name) + if cleaned not in used_names[category]: + used_names[category].add(cleaned) + return cleaned + x = 1 + while cleaned + str(x) in used_names[category]: + x += 1 + + # Found one! + cleaned = cleaned + str(x) + used_names[category].add(cleaned) + return cleaned + +def cleanup(filepath, remove_meta): + # Keys that are exported to the policy file, and so + # can be removed from the xml afterward + to_remove = ('format', 'display_name', + 'graceperiod', 'showanswer', 'rerandomize', + 'start', 'due', 'graded', 'hide_from_toc', + 'ispublic', 'xqa_key') + + try: + print "Cleaning {}".format(filepath) + with open(filepath) as f: + parser = etree.XMLParser(remove_comments=False) + xml = etree.parse(filepath, parser=parser) + except: + print "Error parsing file {}".format(filepath) + return + + for node in xml.iter(tag=etree.Element): + attrs = node.attrib + if 'url_name' in attrs: + used_names[node.tag].add(attrs['url_name']) + if 'name' in attrs: + # Replace name with an identical display_name, and a unique url_name + name = attrs['name'] + attrs['display_name'] = name + attrs['url_name'] = clean_unique(node.tag, name) + del attrs['name'] + + if 'url_name' in attrs and 'slug' in attrs: + print "WARNING: {} has both slug and url_name" + + if ('url_name' in attrs and 'filename' in attrs and + len(attrs)==2 and attrs['url_name'] == attrs['filename']): + # This is a pointer tag in disguise. Get rid of the filename. + print 'turning {}.{} into a pointer tag'.format(node.tag, attrs['url_name']) + del attrs['filename'] + + if remove_meta: + for attr in to_remove: + if attr in attrs: + del attrs[attr] + + + with open(filepath, "w") as f: + f.write(etree.tostring(xml)) + + +def find_replace(directory, filePattern, remove_meta): + for path, dirs, files in os.walk(os.path.abspath(directory)): + for filename in fnmatch.filter(files, filePattern): + filepath = os.path.join(path, filename) + cleanup(filepath, remove_meta) + + +def main(args): + usage = "xml_cleanup [dir] [remove-meta]" + n = len(args) + if n < 1 or n > 2 or (n == 2 and args[1] != 'remove-meta'): + print usage + return + + remove_meta = False + if n == 2: + remove_meta = True + + find_replace(args[0], '*.xml', remove_meta) + + +if __name__ == '__main__': + main(sys.argv[1:]) + + From 61478f4ba0202a97ee3cb1c9969047c19054eb33 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 16 Aug 2012 11:46:56 -0400 Subject: [PATCH 7/7] fix string format bug --- common/lib/xmodule/xmodule/backcompat_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py index 06713d3432..ed2bdb837a 100644 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ b/common/lib/xmodule/xmodule/backcompat_module.py @@ -92,7 +92,7 @@ class TranslateCustomTagDescriptor(XModuleDescriptor): xml_object = etree.fromstring(xml_data) system.error_tracker('WARNING: the <{tag}> tag is deprecated. ' 'Instead, use . ' - .format(xml_object.tag)) + .format(tag=xml_object.tag)) tag = xml_object.tag xml_object.tag = 'customtag'