Make location into a named tuple, and use it more as a first class entity,…

Make location into a named tuple, and use it more as a first class entity, rather than URL for identifying content

cms/djangoapps/contentstore/management/commands/import.py

@@ -25,8 +25,8 @@ class Command(BaseCommand):
 
         module_store = XMLModuleStore(org, course, data_dir, 'xmodule.raw_module.RawDescriptor')
         for module in module_store.modules.itervalues():
-            keystore().create_item(module.url)
+            keystore().create_item(module.location)
             if 'data' in module.definition:
-                keystore().update_item(module.url, module.definition['data'])
+                keystore().update_item(module.location, module.definition['data'])
             if 'children' in module.definition:
-                keystore().update_children(module.url, module.definition['children'])
+                keystore().update_children(module.location, module.definition['children'])

cms/templates/widgets/navigation.html

@@ -38,7 +38,7 @@
     % for week in weeks:
     <li>
       <header>
-        <h1><a href="#" class="module-edit" id="${week.url}">${week.name}</a></h1>
+        <h1><a href="#" class="module-edit" id="${week.location.url()}">${week.name}</a></h1>
         <ul>
           % if week.goals:
             % for goal in week.goals:
@@ -52,8 +52,8 @@
 
       <ul>
         % for module in week.get_children():
-        <li class="${module.type}">
-            <a href="#" class="module-edit" id="${module.url}">${module.name}</a>
+        <li class="${module.category}">
+            <a href="#" class="module-edit" id="${module.location.url()}">${module.name}</a>
             <a href="#" class="draggable">handle</a>
         </li>
         % endfor

cms/templates/widgets/sequence-edit.html

@@ -37,7 +37,7 @@
           <ol>
             % for child in module.get_children():
             <li>
-              <a href="#" class="module-edit" id="${child.url}">${child.name}</a>
+              <a href="#" class="module-edit" id="${child.location.url()}">${child.name}</a>
               <a href="#" class="draggable">handle</a>
             </li>
             %endfor

common/lib/keystore/__init__.py

@@ -4,6 +4,7 @@ that are stored in a database an accessible using their Location as an identifie
 """
 
 import re
+from collections import namedtuple
 from .exceptions import InvalidLocationError
 
 URL_RE = re.compile("""
@@ -17,8 +18,8 @@ URL_RE = re.compile("""
 
 INVALID_CHARS = re.compile(r"[^\w.-]")
 
-
-class Location(object):
+_LocationBase = namedtuple('LocationBase', 'tag org course category name revision')
+class Location(_LocationBase):
     '''
     Encodes a location.
 
@@ -28,6 +29,7 @@ class Location(object):
     However, they can also be represented a dictionaries (specifying each component),
     tuples or list (specified in order), or as strings of the url
     '''
+    __slots__ = ()
 
     @classmethod
     def clean(cls, value):
@@ -36,7 +38,7 @@ class Location(object):
         """
         return re.sub('_+', '_', INVALID_CHARS.sub('_', value))
 
-    def __init__(self, location):
+    def __new__(_cls, loc_or_tag, org=None, course=None, category=None, name=None, revision=None):
         """
         Create a new location that is a clone of the specifed one.
 
@@ -60,47 +62,50 @@ class Location(object):
         Components may be set to None, which may be interpreted by some contexts to mean
         wildcard selection
         """
-        self.update(location)
 
-    def update(self, location):
-        """
-        Update this instance with data from another Location object.
+        if org is None and course is None and category is None and name is None and revision is None:
+            location = loc_or_tag
+        else:
+            location = (loc_or_tag, org, course, category, name, revision)
 
-        location: can take the same forms as specified by `__init__`
-        """
-        self.tag = self.org = self.course = self.category = self.name = self.revision = None
+        def check_dict(dict_):
+            check_list(dict_.values())
+
+        def check_list(list_):
+            for val in list_:
+                if val is not None and INVALID_CHARS.search(val) is not None:
+                    raise InvalidLocationError(location)
 
         if isinstance(location, basestring):
             match = URL_RE.match(location)
             if match is None:
                 raise InvalidLocationError(location)
             else:
-                self.update(match.groupdict())
-        elif isinstance(location, list):
+                groups = match.groupdict()
+                check_dict(groups)
+                return _LocationBase.__new__(_cls, **groups)
+        elif isinstance(location, (list, tuple)):
             if len(location) not in (5, 6):
                 raise InvalidLocationError(location)
 
-            (self.tag, self.org, self.course, self.category, self.name) = location[0:5]
-            self.revision = location[5] if len(location) == 6 else None
+            if len(location) == 5:
+                args = tuple(location) + (None, )
+            else:
+                args = tuple(location)
+
+            check_list(args)
+            return _LocationBase.__new__(_cls, *args)
         elif isinstance(location, dict):
-            try:
-                self.tag = location['tag']
-                self.org = location['org']
-                self.course = location['course']
-                self.category = location['category']
-                self.name = location['name']
-            except KeyError:
-                raise InvalidLocationError(location)
-            self.revision = location.get('revision')
+            kwargs = dict(location)
+            kwargs.setdefault('revision', None)
+
+            check_dict(kwargs)
+            return _LocationBase.__new__(_cls, **kwargs)
         elif isinstance(location, Location):
-            self.update(location.list())
+            return _LocationBase.__new__(_cls, location)
         else:
             raise InvalidLocationError(location)
 
-        for val in self.list():
-            if val is not None and INVALID_CHARS.search(val) is not None:
-                raise InvalidLocationError(location)
-
     def url(self):
         """
         Return a string containing the URL for this location
@@ -114,38 +119,19 @@ class Location(object):
         """
         Return a string with a version of the location that is safe for use in html id attributes
         """
-        return "-".join(str(v) for v in self.list() if v is not None)
-
-    def list(self):
-        """
-        Return a list representing this location
-        """
-        return [self.tag, self.org, self.course, self.category, self.name, self.revision]
+        return "-".join(str(v) for v in self if v is not None)
 
     def dict(self):
-        """
-        Return a dictionary representing this location
-        """
-        return {'tag': self.tag,
-                'org': self.org,
-                'course': self.course,
-                'category': self.category,
-                'name': self.name,
-                'revision': self.revision}
+        return self.__dict__
+
+    def list(self):
+        return list(self)
 
     def __str__(self):
         return self.url()
 
     def __repr__(self):
-        return 'Location(%r)' % str(self)
-
-    def __hash__(self):
-        return self.url()
-
-    def __eq__(self, other):
-        return (isinstance(other, Location) and
-                str(self) == str(other))
-
+        return "Location%r" % tuple(self)
 
 class ModuleStore(object):
     """

common/lib/keystore/tests/test_location.py

-from nose.tools import assert_equals, assert_raises
+from nose.tools import assert_equals, assert_raises, assert_not_equals
 from keystore import Location
 from keystore.exceptions import InvalidLocationError
 
@@ -11,7 +11,6 @@ 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 with spaces/revision")
 
 
 def test_dict():
@@ -50,3 +49,15 @@ def test_invalid_locations():
     assert_raises(InvalidLocationError, Location, ["foo", "bar"])
     assert_raises(InvalidLocationError, Location, ["foo", "bar", "baz", "blat", "foo/bar"])
     assert_raises(InvalidLocationError, Location, None)
+    assert_raises(InvalidLocationError, Location, "tag://org/course/category/name with spaces/revision")
+
+def test_equality():
+    assert_equals(
+        Location('tag', 'org', 'course', 'category', 'name'),
+        Location('tag', 'org', 'course', 'category', 'name')
+    )
+
+    assert_not_equals(
+        Location('tag', 'org', 'course', 'category', 'name1'),
+        Location('tag', 'org', 'course', 'category', 'name')
+    )

common/lib/keystore/xml.py

@@ -47,7 +47,7 @@ class XMLModuleStore(ModuleStore):
                             xml_data.set('slug', '{tag}_{count}'.format(tag=xml_data.tag, count=self.unnamed_modules))
 
                         module = XModuleDescriptor.load_from_xml(etree.tostring(xml_data), self, org, course, modulestore.default_class)
-                        modulestore.modules[module.url] = module
+                        modulestore.modules[module.location] = module
                         return module
 
                     XMLParsingSystem.__init__(self, modulestore.get_item, OSFS(data_dir), process_xml)
@@ -68,7 +68,7 @@ class XMLModuleStore(ModuleStore):
         """
         location = Location(location)
         try:
-            return self.modules[location.url()]
+            return self.modules[location]
         except KeyError:
             raise ItemNotFoundError(location)
 

common/lib/xmodule/abtest_module.py

@@ -102,7 +102,7 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor):
                 )
 
             child_content_urls = [
-                system.process_xml(etree.tostring(child)).url
+                system.process_xml(etree.tostring(child)).location.url()
                 for child in group
             ]
 

common/lib/xmodule/seq_module.py

@@ -105,6 +105,6 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor):
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         return {'children': [
-            system.process_xml(etree.tostring(child_module)).url
+            system.process_xml(etree.tostring(child_module)).location.url()
             for child_module in xml_object
         ]}

common/lib/xmodule/x_module.py

@@ -239,7 +239,7 @@ class XModuleDescriptor(Plugin):
         self.definition = definition if definition is not None else {}
         self.name = Location(kwargs.get('location')).name
         self.category = Location(kwargs.get('location')).category
-        self.url = Location(kwargs.get('location')).url()
+        self.location = Location(kwargs.get('location'))
         self.metadata = kwargs.get('metadata', {})
         self.shared_state_key = kwargs.get('shared_state_key')
 
@@ -364,7 +364,7 @@ class XModuleDescriptor(Plugin):
         return partial(
             self.module_class,
             system,
-            self.url,
+            self.location,
             self.definition,
             metadata=self.metadata
         )

lms/djangoapps/courseware/models.py

@@ -85,6 +85,7 @@ class StudentModuleCache(object):
                     student=user,
                     module_state_key__in=id_chunk)
                 )
+
         else:
             self.cache = []
 
@@ -93,7 +94,7 @@ class StudentModuleCache(object):
         Get a list of the state_keys needed for StudentModules
         required for this chunk of module xml
         '''
-        keys = [descriptor.url]
+        keys = [descriptor.location.url()]
 
         shared_state_key = getattr(descriptor, 'shared_state_key', None)
         if shared_state_key is not None:

lms/djangoapps/courseware/module_render.py

@@ -207,7 +207,7 @@ def get_module(user, request, location, student_module_cache, position=None):
     '''
     descriptor = keystore().get_item(location)
 
-    instance_module = student_module_cache.lookup(descriptor.category, descriptor.url)
+    instance_module = student_module_cache.lookup(descriptor.category, descriptor.location.url())
     shared_state_key = getattr(descriptor, 'shared_state_key', None)
     if shared_state_key is not None:
         shared_module = student_module_cache.lookup(descriptor.category, shared_state_key)
@@ -218,7 +218,7 @@ def get_module(user, request, location, student_module_cache, position=None):
     shared_state = shared_module.state if shared_module is not None else None
 
     # Setup system context for module instance
-    ajax_url = settings.MITX_ROOT_URL + '/modx/' + descriptor.url + '/'
+    ajax_url = settings.MITX_ROOT_URL + '/modx/' + descriptor.location.url() + '/'
 
     def _get_module(location):
         (module, _, _, _) = get_module(user, request, location, student_module_cache, position)

lms/djangoapps/courseware/views.py

@@ -209,7 +209,7 @@ def index(request, course=None, chapter=None, section=None,
         course_location = multicourse_settings.get_course_location(course)
         section = get_section(course_location, chapter, section)
         student_module_cache = StudentModuleCache(request.user, section)
-        module, _, _, _ = get_module(request.user, request, section.url, student_module_cache)
+        module, _, _, _ = get_module(request.user, request, section.location, student_module_cache)
         context['content'] = module.get_html()
 
     result = render_to_response('courseware.html', context)