Allow location url_names to contain ':', mapping to / on file load

* New format: {tag}://{org}/{course}/{category}/{name}[@{revision}]
* Updated tests, code
* Added test chapter with : in url_name in toy course
* added test html page with : in url_name
* added a note to docs

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):

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

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<tag>[^:]+)://
     (?P<org>[^/]+)/
     (?P<course>[^/]+)/
     (?P<category>[^/]+)/
-    (?P<name>[^/]+)
-    (/(?P<revision>[^/]+))?
+    (?P<name>[^@]+)
+    (@(?P<revision>[^/]+))?
     """, 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('.', '_')
 

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():

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")

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))

common/test/data/toy/chapter/secret/magic.xml

+<chapter>
+    <video url_name="toyvideo" youtube="blahblah"/>
+</chapter>

common/test/data/toy/course/2012_Fall.xml

 <course>
   <chapter url_name="Overview">
     <videosequence url_name="Toy_Videos">
-      <html url_name="toylab"/>
+      <html url_name="secret:toylab"/>
       <video url_name="Video_Resources" youtube="1.0:1bK-WdDi6Qw"/>
     </videosequence>
     <video url_name="Welcome" youtube="1.0:p2Q6BrNhdh8"/>
   </chapter>
+  <chapter url_name="secret:magic"/>
 </course>

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": {