Add more tests and replace ':' with '_' in html_id

* in-progress to fixing support for ':' in url_names

common/djangoapps/xmodule_modifiers.py

@@ -125,7 +125,7 @@ def add_histogram(get_html, module, user):
         mstart = getattr(module.descriptor,'start')
         if mstart is not None:
             is_released = "<font color='red'>Yes!</font>" if (now > mstart) else "<font color='green'>Not yet</font>"
-        
+
         staff_context = {'definition': module.definition.get('data'),
                          'metadata': json.dumps(module.metadata, indent=4),
                          'location': module.location,
@@ -133,6 +133,7 @@ def add_histogram(get_html, module, user):
                          'source_file' : source_file,
                          'source_url': '%s/%s/tree/master/%s' % (giturl,data_dir,source_file),
                          'category': str(module.__class__.__name__),
+                         # Template uses element_id in js function names, so can't allow dashes
                          'element_id': module.location.html_id().replace('-','_'),
                          'edit_link': edit_link,
                          'user': user,

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -29,6 +29,9 @@ INVALID_CHARS = re.compile(r"[^\w.-]")
 # Names are allowed to have colons.
 INVALID_CHARS_NAME = re.compile(r"[^\w.:-]")
 
+# html ids can contain word chars and dashes
+INVALID_HTML_CHARS = re.compile(r"[^\w-]")
+
 _LocationBase = namedtuple('LocationBase', 'tag org course category name revision')
 
 
@@ -44,12 +47,35 @@ class Location(_LocationBase):
     '''
     __slots__ = ()
 
+    @staticmethod
+    def _clean(value, invalid):
+        """
+        invalid should be a compiled regexp of chars to replace with '_'
+        """
+        return re.sub('_+', '_', invalid.sub('_', value))
+
+
     @staticmethod
     def clean(value):
         """
         Return value, made into a form legal for locations
         """
-        return re.sub('_+', '_', INVALID_CHARS.sub('_', value))
+        return Location._clean(value, INVALID_CHARS)
+
+    @staticmethod
+    def clean_for_url_name(value):
+        """
+        Convert value into a format valid for location names (allows colons).
+        """
+        return Location._clean(value, INVALID_CHARS_NAME)
+
+    @staticmethod
+    def clean_for_html(value):
+        """
+        Convert a string into a form that's safe for use in html ids, classes, urls, etc.
+        Replaces all INVALID_HTML_CHARS with '_', collapses multiple '_' chars
+        """
+        return Location._clean(value, INVALID_HTML_CHARS)
 
     @staticmethod
     def is_valid(value):
@@ -183,9 +209,9 @@ 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('.', '_')
+        s = "-".join(str(v) for v in self.list()
+                        if v is not None)
+        return Location.clean_for_html(s)
 
     def dict(self):
         """

common/lib/xmodule/xmodule/modulestore/tests/test_location.py

@@ -114,12 +114,44 @@ def test_equality():
         Location('tag', 'org', 'course', 'category', 'name')
     )
 
+# All the cleaning functions should do the same thing with these
+general_pairs = [ ('',''),
+                  (' ', '_'),
+                  ('abc,', 'abc_'),
+                  ('ab    fg!@//\\aj', 'ab_fg_aj'),
+                  (u"ab\xA9", "ab_"),  # no unicode allowed for now
+                  ]
+
 def test_clean():
-    pairs = [ ('',''),
-              (' ', '_'),
-              ('abc,', 'abc_'),
-              ('ab    fg!@//\\aj', 'ab_fg_aj'),
-              (u"ab\xA9", "ab_"),  # no unicode allowed for now
-              ]
+    pairs = general_pairs + [
+        ('a:b', 'a_b'),  # no colons in non-name components
+        ('a-b', 'a-b'),  # dashes ok 
+        ('a.b', 'a.b'),  # dot ok
+        ]
     for input, output in pairs:
         assert_equals(Location.clean(input), output)
+
+
+def test_clean_for_url_name():
+    pairs = general_pairs + [
+        ('a:b', 'a:b'),  # colons ok in names
+        ('a-b', 'a-b'),  # dashes ok in names
+        ('a.b', 'a.b'),  # dot ok in names
+        ]
+    for input, output in pairs:
+        assert_equals(Location.clean_for_url_name(input), output)
+
+
+def test_clean_for_html():
+    pairs = general_pairs + [
+              ("a:b", "a_b"),   # no colons for html use
+              ("a-b", "a-b"),   # dashes ok (though need to be replaced in various use locations. ugh.)
+              ('a.b', 'a_b'),   # no dots.
+              ]
+    for input, output in pairs:
+        assert_equals(Location.clean_for_html(input), output)
+
+
+def test_html_id():
+    loc = Location("tag://org/course/cat/name:more_name@rev")
+    assert_equals(loc.html_id(), "tag-org-course-cat-name_more_name-rev")

common/lib/xmodule/xmodule/xml_module.py

@@ -28,7 +28,7 @@ def is_pointer_tag(xml_obj):
     No children, one attribute named url_name.
 
     Special case for course roots: the pointer is
-      <course url_name="something" org="myorg"  course="course">
+      <course url_name="something" org="myorg" course="course">
 
     xml_obj: an etree Element