xml format cleanups

* rename slug->url_name
* rename name->display_name
* docstring cleanups
* comments :)

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

@@ -50,24 +50,25 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 log.exception("Unable to parse xml: {xml}".format(xml=xml))
                 raise
 
-            # TODO (vshnayder): is the slug munging permanent, or also intended
-            # to be taken out?
-            if xml_data.get('slug') is None:
+            # VS[compat]. Take this out once course conversion is done
+            if xml_data.get('slug') is None and xml_data.get('url_name') is None:
                 if xml_data.get('name'):
                     slug = Location.clean(xml_data.get('name'))
+                elif xml_data.get('display_name'):
+                    slug = Location.clean(xml_data.get('display_name'))
                 else:
                     self.unnamed_modules += 1
                     slug = '{tag}_{count}'.format(tag=xml_data.tag,
                                                   count=self.unnamed_modules)
 
-                if slug in self.used_slugs:
+                while slug in self.used_slugs:
                     self.unnamed_modules += 1
                     slug = '{slug}_{count}'.format(slug=slug,
                                                    count=self.unnamed_modules)
 
                 self.used_slugs.add(slug)
                 # log.debug('-> slug=%s' % slug)
-                xml_data.set('slug', slug)
+                xml_data.set('url_name', slug)
 
             module = XModuleDescriptor.load_from_xml(
                 etree.tostring(xml_data), self, org,

common/lib/xmodule/xmodule/xml_module.py

@@ -9,7 +9,6 @@ import os
 
 log = logging.getLogger(__name__)
 
-
 # TODO (cpennington): This was implemented in an attempt to improve performance,
 # but the actual improvement wasn't measured (and it was implemented late at night).
 # We should check if it hurts, and whether there's a better way of doing lazy loading
@@ -76,8 +75,13 @@ _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata')
 
 class AttrMap(_AttrMapBase):
     """
-    A class that specifies a metadata_key, a function to transform an xml
-    attribute to be placed in that key, and to transform that key value
+    A class that specifies a metadata_key, and two functions:
+
+        to_metadata: convert value from the xml representation into
+            an internal python representation
+
+        from_metadata: convert the internal python representation into
+            the value to store in the xml.
     """
     def __new__(_cls, metadata_key,
                 to_metadata=lambda x: x,
@@ -96,17 +100,35 @@ class XmlDescriptor(XModuleDescriptor):
     # The attributes will be removed from the definition xml passed
     # to definition_from_xml, and from the xml returned by definition_to_xml
     metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize',
-        'start', 'due', 'graded', 'name', 'slug', 'hide_from_toc')
+        'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc',
+        # VS[compat] Remove once unused.
+        'name', 'slug')
 
-    # A dictionary mapping xml attribute names to functions of the value
-    # that return the metadata key and value
+
+    # A dictionary mapping xml attribute names AttrMaps that describe how
+    # to import and export them
     xml_attribute_map = {
+        # type conversion: want True/False in python, "true"/"false" in xml
         'graded': AttrMap('graded',
                           lambda val: val == 'true',
                           lambda val: str(val).lower()),
-        'name': AttrMap('display_name'),
     }
 
+
+    # VS[compat].  Backwards compatibility code that can go away after
+    # importing 2012 courses.
+    # A set of metadata key conversions that we want to make
+    metadata_translations = {
+        'slug' : 'url_name',
+        'name' : 'display_name',
+        }
+
+    @classmethod
+    def _translate(cls, key):
+        'VS[compat]'
+        return cls.metadata_translations.get(key, key)
+
+
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         """
@@ -157,9 +179,11 @@ class XmlDescriptor(XModuleDescriptor):
             for attr in cls.metadata_attributes:
                 val = xml_object.get(attr)
                 if val is not None:
+                    # VS[compat].  Remove after all key translations done
+                    attr = cls._translate(attr)
+
                     attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr))
                     metadata[attr_map.metadata_key] = attr_map.to_metadata(val)
-
             return metadata
 
         def definition_loader():
@@ -182,7 +206,6 @@ class XmlDescriptor(XModuleDescriptor):
                             filepath = candidate
                             break
 
-                log.debug('filepath=%s, resources_fs=%s' % (filepath, system.resources_fs))
                 try:
                     with system.resources_fs.open(filepath) as file:
                         definition_xml = cls.file_to_xml(file)
@@ -190,12 +213,14 @@ class XmlDescriptor(XModuleDescriptor):
                     msg = 'Unable to load file contents at path %s' % filepath
                     log.exception(msg)
                     system.error_handler(msg)
-                    # if error_handler didn't reraise, work around it.
+                    # if error_handler didn't reraise, work around problem.
                     return {'data': 'Error loading file contents at path %s' % filepath}
 
             cls.clean_metadata_from_xml(definition_xml)
             return cls.definition_from_xml(definition_xml, system)
 
+        # VS[compat] -- just have the url_name lookup once translation is done
+        slug = xml_object.get('url_name', xml_object.get('slug'))
         return cls(
             system,
             LazyLoadingDict(definition_loader),
@@ -203,7 +228,7 @@ class XmlDescriptor(XModuleDescriptor):
                       org,
                       course,
                       xml_object.tag,
-                      xml_object.get('slug')],
+                      slug],
             metadata=LazyLoadingDict(metadata_loader),
         )
 
@@ -242,12 +267,15 @@ class XmlDescriptor(XModuleDescriptor):
 
         resource_fs is a pyfilesystem object (from the fs package)
         """
+
+        # Get the definition
         xml_object = self.definition_to_xml(resource_fs)
         self.__class__.clean_metadata_from_xml(xml_object)
 
+        # Set the tag first, so it's right if writing to a file
         xml_object.tag = self.category
-        xml_object.set('slug', self.name)
 
+        # Write it to a file if necessary
         if self.split_to_file(xml_object):
             # Put this object in it's own file
             filepath = self.__class__._format_filepath(self.category, self.name)
@@ -265,6 +293,8 @@ class XmlDescriptor(XModuleDescriptor):
 
             xml_object.set('filename', self.name)
 
+        # Add the metadata
+        xml_object.set('url_name', self.name)
         for attr in self.metadata_attributes:
             attr_map = self.xml_attribute_map.get(attr, AttrMap(attr))
             metadata_key = attr_map.metadata_key
@@ -276,6 +306,7 @@ class XmlDescriptor(XModuleDescriptor):
             val = attr_map.from_metadata(self.metadata[metadata_key])
             xml_object.set(attr, val)
 
+        # Now we just have to make it beautiful
         return etree.tostring(xml_object, pretty_print=True)
 
     def definition_to_xml(self, resource_fs):