diff --git a/cms/djangoapps/github_sync/tests/__init__.py b/cms/djangoapps/github_sync/tests/__init__.py index 581ac3cb25..e2b9a909a7 100644 --- a/cms/djangoapps/github_sync/tests/__init__.py +++ b/cms/djangoapps/github_sync/tests/__init__.py @@ -61,7 +61,7 @@ class GithubSyncTestCase(TestCase): self.assertIn( Location('i4x://edX/toy/chapter/Overview'), [child.location for child in self.import_course.get_children()]) - self.assertEquals(1, len(self.import_course.get_children())) + self.assertEquals(2, len(self.import_course.get_children())) @patch('github_sync.sync_with_github') def test_sync_all_with_github(self, sync_with_github): diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 6c424c26f2..3f834c335c 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -4,9 +4,10 @@ import logging import os import sys from lxml import etree +from path import path from .x_module import XModule -from .xml_module import XmlDescriptor +from .xml_module import XmlDescriptor, name_to_pathname from .editing_module import EditingDescriptor from .stringify import stringify_children from .html_checker import check_html @@ -75,9 +76,19 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): cls.clean_metadata_from_xml(definition_xml) return {'data': stringify_children(definition_xml)} else: - # html is special. cls.filename_extension is 'xml', but if 'filename' is in the definition, - # that means to load from .html - filepath = "{category}/{name}.html".format(category='html', name=filename) + # html is special. cls.filename_extension is 'xml', but + # if 'filename' is in the definition, that means to load + # from .html + # 'filename' in html pointers is a relative path + # (not same as 'html/blah.html' when the pointer is in a directory itself) + pointer_path = "{category}/{url_path}".format(category='html', + url_path=name_to_pathname(location.name)) + base = path(pointer_path).dirname() + #log.debug("base = {0}, base.dirname={1}, filename={2}".format(base, base.dirname(), filename)) + filepath = "{base}/{name}.html".format(base=base, name=filename) + #log.debug("looking for html file for {0} at {1}".format(location, filepath)) + + # VS[compat] # TODO (cpennington): If the file doesn't exist at the right path, @@ -128,13 +139,18 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): pass # Not proper format. Write html to file, return an empty tag - filepath = u'{category}/{name}.html'.format(category=self.category, - name=self.url_name) + pathname = name_to_pathname(self.url_name) + pathdir = path(pathname).dirname() + filepath = u'{category}/{pathname}.html'.format(category=self.category, + pathname=pathname) resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) with resource_fs.open(filepath, 'w') as file: file.write(self.definition['data']) + # write out the relative name + relname = path(pathname).basename() + elt = etree.Element('html') - elt.set("filename", self.url_name) + elt.set("filename", relname) return elt diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index d6dd85deea..c3bbc1e508 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -13,18 +13,21 @@ from xmodule.errortracker import ErrorLog, make_error_tracker log = logging.getLogger('mitx.' + 'modulestore') + URL_RE = re.compile(""" (?P[^:]+):// (?P[^/]+)/ (?P[^/]+)/ (?P[^/]+)/ - (?P[^/]+) - (/(?P[^/]+))? + (?P[^@]+) + (@(?P[^/]+))? """, re.VERBOSE) # TODO (cpennington): We should decide whether we want to expand the # list of valid characters in a location INVALID_CHARS = re.compile(r"[^\w.-]") +# Names are allowed to have colons. +INVALID_CHARS_NAME = re.compile(r"[^\w.:-]") _LocationBase = namedtuple('LocationBase', 'tag org course category name revision') @@ -34,7 +37,7 @@ class Location(_LocationBase): Encodes a location. Locations representations of URLs of the - form {tag}://{org}/{course}/{category}/{name}[/{revision}] + form {tag}://{org}/{course}/{category}/{name}[@{revision}] However, they can also be represented a dictionaries (specifying each component), tuples or list (specified in order), or as strings of the url @@ -81,7 +84,7 @@ class Location(_LocationBase): location - Can be any of the following types: string: should be of the form - {tag}://{org}/{course}/{category}/{name}[/{revision}] + {tag}://{org}/{course}/{category}/{name}[@{revision}] list: should be of the form [tag, org, course, category, name, revision] @@ -99,10 +102,11 @@ class Location(_LocationBase): ommitted. Components must be composed of alphanumeric characters, or the - characters '_', '-', and '.' + characters '_', '-', and '.'. The name component is additionally allowed to have ':', + which is interpreted specially for xml storage. - Components may be set to None, which may be interpreted by some contexts - to mean wildcard selection + Components may be set to None, which may be interpreted in some contexts + to mean wildcard selection. """ @@ -116,14 +120,23 @@ class Location(_LocationBase): return _LocationBase.__new__(_cls, *([None] * 6)) def check_dict(dict_): - check_list(dict_.itervalues()) + # Order matters, so flatten out into a list + keys = ['tag', 'org', 'course', 'category', 'name', 'revision'] + list_ = [dict_[k] for k in keys] + check_list(list_) def check_list(list_): - for val in list_: - if val is not None and INVALID_CHARS.search(val) is not None: + def check(val, regexp): + if val is not None and regexp.search(val) is not None: log.debug('invalid characters val="%s", list_="%s"' % (val, list_)) raise InvalidLocationError(location) + list_ = list(list_) + for val in list_[:4] + [list_[5]]: + check(val, INVALID_CHARS) + # names allow colons + check(list_[4], INVALID_CHARS_NAME) + if isinstance(location, basestring): match = URL_RE.match(location) if match is None: @@ -162,7 +175,7 @@ class Location(_LocationBase): """ url = "{tag}://{org}/{course}/{category}/{name}".format(**self.dict()) if self.revision: - url += "/" + self.revision + url += "@" + self.revision return url def html_id(self): @@ -170,6 +183,7 @@ class Location(_LocationBase): Return a string with a version of the location that is safe for use in html id attributes """ + # TODO: is ':' ok in html ids? return "-".join(str(v) for v in self.list() if v is not None).replace('.', '_') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py index 70c6351685..529b1f88eb 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py @@ -10,7 +10,7 @@ def check_string_roundtrip(url): def test_string_roundtrip(): check_string_roundtrip("tag://org/course/category/name") - check_string_roundtrip("tag://org/course/category/name/revision") + check_string_roundtrip("tag://org/course/category/name@revision") input_dict = { @@ -21,18 +21,28 @@ input_dict = { 'org': 'org' } + +also_valid_dict = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name:more_name', + 'org': 'org' +} + + input_list = ['tag', 'org', 'course', 'category', 'name'] input_str = "tag://org/course/category/name" -input_str_rev = "tag://org/course/category/name/revision" +input_str_rev = "tag://org/course/category/name@revision" -valid = (input_list, input_dict, input_str, input_str_rev) +valid = (input_list, input_dict, input_str, input_str_rev, also_valid_dict) invalid_dict = { 'tag': 'tag', 'course': 'course', 'category': 'category', - 'name': 'name/more_name', + 'name': 'name@more_name', 'org': 'org' } @@ -45,8 +55,9 @@ invalid_dict2 = { } invalid = ("foo", ["foo"], ["foo", "bar"], - ["foo", "bar", "baz", "blat", "foo/bar"], - "tag://org/course/category/name with spaces/revision", + ["foo", "bar", "baz", "blat:blat", "foo:bar"], # ':' ok in name, not in category + "tag://org/course/category/name with spaces@revision", + "tag://org/course/category/name/with/slashes@revision", invalid_dict, invalid_dict2) @@ -62,16 +73,15 @@ def test_dict(): assert_equals(dict(revision=None, **input_dict), Location(input_dict).dict()) input_dict['revision'] = 'revision' - assert_equals("tag://org/course/category/name/revision", Location(input_dict).url()) + assert_equals("tag://org/course/category/name@revision", Location(input_dict).url()) assert_equals(input_dict, Location(input_dict).dict()) - def test_list(): assert_equals("tag://org/course/category/name", Location(input_list).url()) assert_equals(input_list + [None], Location(input_list).list()) input_list.append('revision') - assert_equals("tag://org/course/category/name/revision", Location(input_list).url()) + assert_equals("tag://org/course/category/name@revision", Location(input_list).url()) assert_equals(input_list, Location(input_list).list()) @@ -87,8 +97,10 @@ def test_none(): def test_invalid_locations(): assert_raises(InvalidLocationError, Location, "foo") assert_raises(InvalidLocationError, Location, ["foo", "bar"]) + assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat/blat", "foo"]) assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat", "foo/bar"]) - assert_raises(InvalidLocationError, Location, "tag://org/course/category/name with spaces/revision") + assert_raises(InvalidLocationError, Location, "tag://org/course/category/name with spaces@revision") + assert_raises(InvalidLocationError, Location, "tag://org/course/category/name/revision") def test_equality(): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 3454366c1a..a369850209 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -255,3 +255,37 @@ class ImportTestCase(unittest.TestCase): two_toy_video = modulestore.get_instance(two_toy_id, location) self.assertEqual(toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh8") self.assertEqual(two_toy_video.metadata['youtube'], "1.0:p2Q6BrNhdh9") + + + def test_colon_in_url_name(self): + """Ensure that colons in url_names convert to file paths properly""" + + print "Starting import" + modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy']) + + courses = modulestore.get_courses() + self.assertEquals(len(courses), 1) + course = courses[0] + course_id = course.id + + print "course errors:" + for (msg, err) in modulestore.get_item_errors(course.location): + print msg + print err + + chapters = course.get_children() + self.assertEquals(len(chapters), 2) + + ch2 = chapters[1] + self.assertEquals(ch2.url_name, "secret:magic") + + print "Ch2 location: ", ch2.location + + also_ch2 = modulestore.get_instance(course_id, ch2.location) + self.assertEquals(ch2, also_ch2) + + print "making sure html loaded" + cloc = course.location + loc = Location(cloc.tag, cloc.org, cloc.course, 'html', 'secret:toylab') + html = modulestore.get_instance(course_id, loc) + self.assertEquals(html.display_name, "Toy lab") diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index b6f791ffc6..25dc4e0c6e 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -12,6 +12,12 @@ import sys log = logging.getLogger(__name__) +def name_to_pathname(name): + """ + Convert a location name for use in a path: replace ':' with '/'. + This allows users of the xml format to organize content into directories + """ + return name.replace(':', '/') def is_pointer_tag(xml_obj): """ @@ -245,8 +251,8 @@ class XmlDescriptor(XModuleDescriptor): # 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) + # read the actual definition file--named using url_name.replace(':','/') + filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) definition_xml = cls.load_file(filepath, system.resources_fs, location) else: definition_xml = xml_object # this is just a pointer, not the real definition content @@ -292,7 +298,8 @@ class XmlDescriptor(XModuleDescriptor): """If this returns True, write the definition of this descriptor to a separate file. - NOTE: Do not override this without a good reason. It is here specifically for customtag... + NOTE: Do not override this without a good reason. It is here + specifically for customtag... """ return True @@ -335,7 +342,8 @@ class XmlDescriptor(XModuleDescriptor): if self.export_to_file(): # Write the definition to a file - filepath = self.__class__._format_filepath(self.category, self.url_name) + url_path = name_to_pathname(self.url_name) + filepath = self.__class__._format_filepath(self.category, url_path) 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)) diff --git a/common/test/data/toy/chapter/secret/magic.xml b/common/test/data/toy/chapter/secret/magic.xml new file mode 100644 index 0000000000..dd16380a70 --- /dev/null +++ b/common/test/data/toy/chapter/secret/magic.xml @@ -0,0 +1,3 @@ + + diff --git a/common/test/data/toy/course/2012_Fall.xml b/common/test/data/toy/course/2012_Fall.xml index d34eb9d56a..c87fccd9ab 100644 --- a/common/test/data/toy/course/2012_Fall.xml +++ b/common/test/data/toy/course/2012_Fall.xml @@ -1,9 +1,10 @@ - + + diff --git a/common/test/data/toy/html/toylab.html b/common/test/data/toy/html/secret/toylab.html similarity index 100% rename from common/test/data/toy/html/toylab.html rename to common/test/data/toy/html/secret/toylab.html diff --git a/common/test/data/toy/html/toylab.xml b/common/test/data/toy/html/secret/toylab.xml similarity index 100% rename from common/test/data/toy/html/toylab.xml rename to common/test/data/toy/html/secret/toylab.xml diff --git a/common/test/data/toy/policies/2012_Fall.json b/common/test/data/toy/policies/2012_Fall.json index 6c501d66f8..5ea437a8e4 100644 --- a/common/test/data/toy/policies/2012_Fall.json +++ b/common/test/data/toy/policies/2012_Fall.json @@ -11,7 +11,7 @@ "display_name": "Toy Videos", "format": "Lecture Sequence" }, - "html/toylab": { + "html/secret:toylab": { "display_name": "Toy lab" }, "video/Video_Resources": {