fix for multiple url_name hashing bug

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -63,7 +63,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 # VS[compat]. Take this out once course conversion is done (perhaps leave the uniqueness check)
 
                 # tags that really need unique names--they store (or should store) state.
-                need_uniq_names = ('problem', 'sequence', 'video', 'course', 'chapter')
+                need_uniq_names = ('problem', 'sequential', 'video', 'course', 'chapter', 'videosequence')
 
                 attr = xml_data.attrib
                 tag = xml_data.tag
@@ -82,17 +82,22 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                             del attr[key]
                         break
 
+                def looks_like_fallback(url_name):
+                    """Does this look like something that came from fallback_name()?"""
+                    return (url_name is not None
+                            and url_name.startswith(tag)
+                            and re.search('[0-9a-fA-F]{12}$', url_name))
+
                 def fallback_name(orig_name=None):
                     """Return the fallback name for this module.  This is a function instead of a variable
                     because we want it to be lazy."""
+                    if looks_like_fallback(orig_name):
+                        # We're about to re-hash, in case something changed, so get rid of the tag_ and hash
+                        orig_name = orig_name[len(tag)+1:-12]
                     # append the hash of the content--the first 12 bytes should be plenty.
-                    orig_name = "_" + orig_name if orig_name is not None else ""
+                    orig_name = "_" + orig_name if orig_name not in (None, "") else ""
                     return tag + orig_name + "_" + hashlib.sha1(xml).hexdigest()[:12]
 
-                def looks_like_fallback(tag, url_name):
-                    """Does this look like something that came from fallback_name()?"""
-                    return url_name.startswith(tag) and re.search('[0-9a-fA-F]{12}$', url_name)
-
                 # Fallback if there was nothing we could use:
                 if url_name is None or url_name == "":
                     url_name = fallback_name()
@@ -114,7 +119,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                     # Always complain about modules that store state.  If it
                     # doesn't store state, don't complain about things that are
                     # hashed.
-                    if tag in need_uniq_names or not looks_like_fallback(tag, url_name):
+                    if tag in need_uniq_names:
                         msg = ("Non-unique url_name in xml.  This may break state tracking for content."
                                "  url_name={0}.  Content={1}".format(url_name, xml[:100]))
                         error_tracker("PROBLEM: " + msg)

common/lib/xmodule/xmodule/tests/test_import.py

@@ -293,3 +293,27 @@ class ImportTestCase(unittest.TestCase):
         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")
+
+    def test_url_name_mangling(self):
+        """
+        Make sure that url_names are only mangled once.
+        """
+
+        modulestore = XMLModuleStore(DATA_DIR, course_dirs=['toy'])
+
+        toy_id = "edX/toy/2012_Fall"
+
+        course = modulestore.get_courses()[0]
+        chapters = course.get_children()
+        ch1 = chapters[0]
+        sections = ch1.get_children()
+
+        self.assertEqual(len(sections), 4)
+
+        for i in (2,3):
+            video = sections[i]
+            # Name should be 'video_{hash}'
+            print "video {0} url_name: {1}".format(i, video.url_name)
+
+            self.assertEqual(len(video.url_name), len('video_') + 12)
+

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

@@ -5,6 +5,8 @@
       <video url_name="Video_Resources" youtube="1.0:1bK-WdDi6Qw"/>
     </videosequence>
     <video url_name="Welcome" youtube="1.0:p2Q6BrNhdh8"/>
+    <video url_name="video_123456789012" youtube="1.0:p2Q6BrNhdh8"/>
+    <video url_name="video_123456789012" youtube="1.0:p2Q6BrNhdh8"/>
   </chapter>
   <chapter url_name="secret:magic"/>
 </course>