Move path_to_location out of mongo.py

* also bugfix for load_definition in html_module
* a bit of refactoring of Location checking code in mongo.py

common/lib/xmodule/xmodule/html_module.py

@@ -54,7 +54,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor):
     # snippets that will be included in the middle of pages.
 
     @classmethod
-    def definition_loader(cls, xml_object, system):
+    def load_definition(cls, xml_object, system, location):
         '''Load a descriptor from the specified xml_object:
 
         If there is a filename attribute, load it as a string, and

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

@@ -5,7 +5,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
+from .exceptions import InvalidLocationError, InsufficientSpecificationError
 import logging
 
 log = logging.getLogger('mitx.' + 'modulestore')
@@ -38,15 +38,15 @@ class Location(_LocationBase):
     '''
     __slots__ = ()
 
-    @classmethod
-    def clean(cls, value):
+    @staticmethod
+    def clean(value):
         """
         Return value, made into a form legal for locations
         """
         return re.sub('_+', '_', INVALID_CHARS.sub('_', value))
 
-    @classmethod
-    def is_valid(cls, value):
+    @staticmethod
+    def is_valid(value):
         '''
         Check if the value is a valid location, in any acceptable format.
         '''
@@ -56,6 +56,21 @@ class Location(_LocationBase):
             return False
         return True
 
+    @staticmethod
+    def ensure_fully_specified(location):
+        '''Make sure location is valid, and fully specified.  Raises
+        InvalidLocationError or InsufficientSpecificationError if not.
+
+        returns a Location object corresponding to location.
+        '''
+        loc = Location(location)
+        for key, val in loc.dict().iteritems():
+            if key != 'revision' and val is None:
+                raise InsufficientSpecificationError(location)
+        return loc
+
+
+
     def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None,
                 name=None, revision=None):
         """
@@ -254,25 +269,11 @@ class ModuleStore(object):
         '''
         raise NotImplementedError
 
-    def path_to_location(self, location, course=None, chapter=None, section=None):
-        '''
-        Try to find a course/chapter/section[/position] path to this location.
-
-        raise ItemNotFoundError if the location doesn't exist.
-
-        If course, chapter, section are not None, restrict search to paths with those
-            components as specified.
-            
-        raise NoPathToItem if the location exists, but isn't accessible via
-        a path that matches the course/chapter/section restrictions.
-
-        In general, a location may be accessible via many paths. This method may
-        return any valid path.
-
-        Return a tuple (course, chapter, section, position).
+    def get_parent_locations(self, location):
+        '''Find all locations that are the parents of this location.  Needed
+        for path_to_location().
 
-        If the section a sequence, position should be the position of this location
-        in that sequence.  Otherwise, position should be None.
+        returns an iterable of things that can be passed to Location.
         '''
         raise NotImplementedError
 

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -9,11 +9,10 @@ from importlib import import_module
 from xmodule.errorhandlers import strict_error_handler
 from xmodule.x_module import XModuleDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.course_module import CourseDescriptor
 from mitxmako.shortcuts import render_to_string
 
 from . import ModuleStore, Location
-from .exceptions import (ItemNotFoundError, InsufficientSpecificationError,
+from .exceptions import (ItemNotFoundError,
                          NoPathToItem, DuplicateItemError)
 
 # TODO (cpennington): This code currently operates under the assumption that
@@ -172,12 +171,17 @@ class MongoModuleStore(ModuleStore):
         return self.get_items(course_filter)
 
     def _find_one(self, location):
-        '''Look for a given location in the collection.
-        If revision isn't specified, returns the latest.'''
-        return self.collection.find_one(
+        '''Look for a given location in the collection.  If revision is not
+        specified, returns the latest.  If the item is not present, raise
+        ItemNotFoundError.
+        '''
+        item = self.collection.find_one(
             location_to_query(location),
             sort=[('revision', pymongo.ASCENDING)],
         )
+        if item is None:
+            raise ItemNotFoundError(location)
+        return item
 
     def get_item(self, location, depth=0):
         """
@@ -197,14 +201,8 @@ class MongoModuleStore(ModuleStore):
             calls to get_children() to cache. None indicates to cache all descendents.
 
         """
-
-        for key, val in Location(location).dict().iteritems():
-            if key != 'revision' and val is None:
-                raise InsufficientSpecificationError(location)
-
+        location = Location.ensure_fully_specified(location)
         item = self._find_one(location)
-        if item is None:
-            raise ItemNotFoundError(location)
         return self._load_items([item], depth)[0]
 
     def get_items(self, location, depth=0):
@@ -282,96 +280,20 @@ class MongoModuleStore(ModuleStore):
         )
 
     def get_parent_locations(self, location):
-        '''Find all locations that are the parents of this location.
-        Mostly intended for use in path_to_location, but exposed for testing
-        and possible other usefulness.
+        '''Find all locations that are the parents of this location.  Needed
+        for path_to_location().
+
+        If there is no data at location in this modulestore, raise
+            ItemNotFoundError.
 
-        returns an iterable of things that can be passed to Location.
+        returns an iterable of things that can be passed to Location.  This may
+        be empty if there are no parents.
         '''
-        location = Location(location)
+        location = Location.ensure_fully_specified(location)
+        # Check that it's actually in this modulestore.
+        item = self._find_one(location)
+        # now get the parents
         items = self.collection.find({'definition.children': str(location)},
                                     {'_id': True})
         return [i['_id'] for i in items]
 
-    def path_to_location(self, location, course_name=None):
-        '''
-        Try to find a course_id/chapter/section[/position] path to this location.
-        The courseware insists that the first level in the course is chapter,
-        but any kind of module can be a "section".
-
-        location: something that can be passed to Location
-        course_name: [optional].  If not None, restrict search to paths
-            in that course.
-
-        raise ItemNotFoundError if the location doesn't exist.
-
-        raise NoPathToItem if the location exists, but isn't accessible via
-        a chapter/section path in the course(s) being searched.
-
-        Return a tuple (course_id, chapter, section, position) suitable for the
-        courseware index view.
-
-        A location may be accessible via many paths. This method may
-        return any valid path.
-
-        If the section is a sequence, position will be the position
-        of this location in that sequence.  Otherwise, position will
-        be None. TODO (vshnayder): Not true yet.
-        '''
-        # Check that location is present at all
-        if self._find_one(location) is None:
-            raise ItemNotFoundError(location)
-
-        def flatten(xs):
-            '''Convert lisp-style (a, (b, (c, ()))) lists into a python list.
-            Not a general flatten function. '''
-            p = []
-            while xs != ():
-                p.append(xs[0])
-                xs = xs[1]
-            return p
-
-        def find_path_to_course(location, course_name=None):
-            '''Find a path up the location graph to a node with the
-            specified category.  If no path exists, return None.  If a
-            path exists, return it as a list with target location
-            first, and the starting location last.
-            '''
-            # Standard DFS
-
-            # To keep track of where we came from, the work queue has
-            # tuples (location, path-so-far).  To avoid lots of
-            # copying, the path-so-far is stored as a lisp-style
-            # list--nested hd::tl tuples, and flattened at the end.
-            queue = [(location, ())]
-            while len(queue) > 0:
-                (loc, path) = queue.pop()  # Takes from the end
-                loc = Location(loc)
-                # print 'Processing loc={0}, path={1}'.format(loc, path)
-                if loc.category == "course":
-                    if course_name is None or course_name == loc.name:
-                        # Found it!
-                        path = (loc, path)
-                        return flatten(path)
-
-                # otherwise, add parent locations at the end
-                newpath = (loc, path)
-                parents = self.get_parent_locations(loc)
-                queue.extend(zip(parents, repeat(newpath)))
-
-            # If we're here, there is no path
-            return None
-
-        path = find_path_to_course(location, course_name)
-        if path is None:
-            raise(NoPathToItem(location))
-
-        n = len(path)
-        course_id = CourseDescriptor.location_to_id(path[0])
-        chapter = path[1].name if n > 1 else None
-        section = path[2].name if n > 2 else None
-
-        # TODO (vshnayder): not handling position at all yet...
-        position = None
-
-        return (course_id, chapter, section, position)

common/lib/xmodule/xmodule/modulestore/search.py

+from itertools import repeat
+
+from xmodule.course_module import CourseDescriptor
+
+from .exceptions import (ItemNotFoundError, NoPathToItem)
+from . import ModuleStore, Location
+
+
+def path_to_location(modulestore, location, course_name=None):
+    '''
+    Try to find a course_id/chapter/section[/position] path to location in
+    modulestore.  The courseware insists that the first level in the course is
+    chapter, but any kind of module can be a "section".
+
+    location: something that can be passed to Location
+    course_name: [optional].  If not None, restrict search to paths
+        in that course.
+
+    raise ItemNotFoundError if the location doesn't exist.
+
+    raise NoPathToItem if the location exists, but isn't accessible via
+    a chapter/section path in the course(s) being searched.
+
+    Return a tuple (course_id, chapter, section, position) suitable for the
+    courseware index view.
+
+    A location may be accessible via many paths. This method may
+    return any valid path.
+
+    If the section is a sequence, position will be the position
+    of this location in that sequence.  Otherwise, position will
+    be None. TODO (vshnayder): Not true yet.
+    '''
+
+    def flatten(xs):
+        '''Convert lisp-style (a, (b, (c, ()))) list into a python list.
+        Not a general flatten function. '''
+        p = []
+        while xs != ():
+            p.append(xs[0])
+            xs = xs[1]
+        return p
+
+    def find_path_to_course(location, course_name=None):
+        '''Find a path up the location graph to a node with the
+        specified category.
+
+        If no path exists, return None.
+
+        If a path exists, return it as a list with target location first, and
+        the starting location last.
+        '''
+        # Standard DFS
+
+        # To keep track of where we came from, the work queue has
+        # tuples (location, path-so-far).  To avoid lots of
+        # copying, the path-so-far is stored as a lisp-style
+        # list--nested hd::tl tuples, and flattened at the end.
+        queue = [(location, ())]
+        while len(queue) > 0:
+            (loc, path) = queue.pop()  # Takes from the end
+            loc = Location(loc)
+
+            # get_parent_locations should raise ItemNotFoundError if location
+            # isn't found so we don't have to do it explicitly.  Call this
+            # first to make sure the location is there (even if it's a course, and
+            # we would otherwise immediately exit).
+            parents = modulestore.get_parent_locations(loc)
+
+            # print 'Processing loc={0}, path={1}'.format(loc, path)
+            if loc.category == "course":
+                if course_name is None or course_name == loc.name:
+                    # Found it!
+                    path = (loc, path)
+                    return flatten(path)
+
+            # otherwise, add parent locations at the end
+            newpath = (loc, path)
+            queue.extend(zip(parents, repeat(newpath)))
+
+        # If we're here, there is no path
+        return None
+
+    path = find_path_to_course(location, course_name)
+    if path is None:
+        raise(NoPathToItem(location))
+
+    n = len(path)
+    course_id = CourseDescriptor.location_to_id(path[0])
+    chapter = path[1].name if n > 1 else None
+    section = path[2].name if n > 2 else None
+
+    # TODO (vshnayder): not handling position at all yet...
+    position = None
+
+    return (course_id, chapter, section, position)

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

@@ -8,6 +8,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.mongo import MongoModuleStore
 from xmodule.modulestore.xml_importer import import_from_xml
+from xmodule.modulestore.search import path_to_location
 
 # from ~/mitx_all/mitx/common/lib/xmodule/xmodule/modulestore/tests/
 # to   ~/mitx_all/mitx/common/test
@@ -28,7 +29,7 @@ DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
 
 
 class TestMongoModuleStore(object):
-
+    '''Tests!'''
     @classmethod
     def setupClass(cls):
         cls.connection = pymongo.connection.Connection(HOST, PORT)
@@ -67,7 +68,7 @@ class TestMongoModuleStore(object):
 
     def test_init(self):
         '''Make sure the db loads, and print all the locations in the db.
-        Call this directly from failing tests to see what's loaded'''
+        Call this directly from failing tests to see what is loaded'''
         ids = list(self.connection[DB][COLLECTION].find({}, {'_id': True}))
 
         pprint([Location(i['_id']).url() for i in ids])
@@ -93,8 +94,6 @@ class TestMongoModuleStore(object):
             self.store.get_item("i4x://edX/toy/video/Welcome"),
             None)
 
-
-
     def test_find_one(self):
         assert_not_equals(
             self.store._find_one(Location("i4x://edX/toy/course/2012_Fall")),
@@ -117,13 +116,13 @@ class TestMongoModuleStore(object):
              ("edX/toy/2012_Fall", "Overview", "Toy_Videos", None)),
             )
         for location, expected in should_work:
-            assert_equals(self.store.path_to_location(location), expected)
+            assert_equals(path_to_location(self.store, location), expected)
 
         not_found = (
-            "i4x://edX/toy/video/WelcomeX",
+            "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome"
             )
         for location in not_found:
-            assert_raises(ItemNotFoundError, self.store.path_to_location, location)
+            assert_raises(ItemNotFoundError, path_to_location, self.store, location)
 
         # Since our test files are valid, there shouldn't be any
         # elements with no path to them.  But we can look for them in
@@ -132,5 +131,5 @@ class TestMongoModuleStore(object):
             "i4x://edX/simple/video/Lost_Video",
             )
         for location in no_path:
-            assert_raises(NoPathToItem, self.store.path_to_location, location, "toy")
+            assert_raises(NoPathToItem, path_to_location, self.store, location, "toy")
 

common/lib/xmodule/xmodule/xml_module.py

@@ -163,7 +163,7 @@ class XmlDescriptor(XModuleDescriptor):
         return etree.parse(file_object).getroot()
 
     @classmethod
-    def definition_loader(cls, xml_object, system, location):
+    def load_definition(cls, xml_object, system, location):
         '''Load a descriptor definition from the specified xml_object.
         Subclasses should not need to override this except in special
         cases (e.g. html module)'''
@@ -225,7 +225,7 @@ class XmlDescriptor(XModuleDescriptor):
         slug = xml_object.get('url_name', xml_object.get('slug'))
         location = Location('i4x', org, course, xml_object.tag, slug)
 
-        def metadata_loader():
+        def load_metadata():
             metadata = {}
             for attr in cls.metadata_attributes:
                 val = xml_object.get(attr)
@@ -237,8 +237,8 @@ class XmlDescriptor(XModuleDescriptor):
                     metadata[attr_map.metadata_key] = attr_map.to_metadata(val)
             return metadata
 
-        definition = cls.definition_loader(xml_object, system, location)
-        metadata = metadata_loader()
+        definition = cls.load_definition(xml_object, system, location)
+        metadata = load_metadata()
         # VS[compat] -- just have the url_name lookup once translation is done
         slug = xml_object.get('url_name', xml_object.get('slug'))
         return cls(

lms/djangoapps/courseware/views.py

@@ -20,6 +20,7 @@ from module_render import toc_for_course, get_module, get_section
 from models import StudentModuleCache
 from student.models import UserProfile
 from xmodule.modulestore import Location
+from xmodule.modulestore.search import path_to_location
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.django import modulestore
 from xmodule.course_module import CourseDescriptor
@@ -233,7 +234,7 @@ def jump_to(request, location):
 
     # Complain if there's not data for this location
     try:
-        (course_id, chapter, section, position) = modulestore().path_to_location(location)
+        (course_id, chapter, section, position) = path_to_location(modulestore(), location)
     except ItemNotFoundError:
         raise Http404("No data at this location: {0}".format(location))
     except NoPathToItem: