Make jump_to work with the xml modulestore

* it now works in the context of a specific course_id
* add tracking of parent locations to xml modulestore
* adjust lots of tests, including some refactoring
* NOT working yet: jumping to the right position in a sequence.

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

@@ -6,15 +6,14 @@ from .exceptions import (ItemNotFoundError, NoPathToItem)
 from . import ModuleStore, Location
 
 
-def path_to_location(modulestore, location, course_name=None):
+def path_to_location(modulestore, course_id, location):
     '''
     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.
+    course_id: Search for paths in this course.
 
     raise ItemNotFoundError if the location doesn't exist.
 
@@ -41,7 +40,7 @@ def path_to_location(modulestore, location, course_name=None):
             xs = xs[1]
         return p
 
-    def find_path_to_course(location, course_name=None):
+    def find_path_to_course():
         '''Find a path up the location graph to a node with the
         specified category.
 
@@ -69,7 +68,8 @@ def path_to_location(modulestore, location, course_name=None):
 
             # print 'Processing loc={0}, path={1}'.format(loc, path)
             if loc.category == "course":
-                if course_name is None or course_name == loc.name:
+                # confirm that this is the right course
+                if course_id == CourseDescriptor.location_to_id(loc):
                     # Found it!
                     path = (loc, path)
                     return flatten(path)
@@ -81,7 +81,7 @@ def path_to_location(modulestore, location, course_name=None):
         # If we're here, there is no path
         return None
 
-    path = find_path_to_course(location, course_name)
+    path = find_path_to_course()
     if path is None:
         raise(NoPathToItem(location))
 

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

+from path import path
+
+# from ~/mitx_all/mitx/common/lib/xmodule/xmodule/modulestore/tests/
+# to   ~/mitx_all/mitx/common/test
+TEST_DIR = path(__file__).abspath().dirname()
+for i in range(5):
+    TEST_DIR = TEST_DIR.dirname()
+TEST_DIR = TEST_DIR / 'test'
+
+DATA_DIR = TEST_DIR / 'data'
+
+

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

+from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup
+
+from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem
+from xmodule.modulestore.search import path_to_location
+
+def check_path_to_location(modulestore):
+    '''Make sure that path_to_location works: should be passed a modulestore
+    with the toy and simple courses loaded.'''
+    should_work = (
+        ("i4x://edX/toy/video/Welcome",
+         ("edX/toy/2012_Fall", "Overview", "Welcome", None)),
+        ("i4x://edX/toy/chapter/Overview",
+         ("edX/toy/2012_Fall", "Overview", None, None)),
+        )
+    course_id = "edX/toy/2012_Fall"
+
+    for location, expected in should_work:
+        assert_equals(path_to_location(modulestore, course_id, location), expected)
+
+    not_found = (
+        "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome"
+        )
+    for location in not_found:
+        assert_raises(ItemNotFoundError, path_to_location, modulestore, course_id, 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
+    # another course.
+    no_path = (
+        "i4x://edX/simple/video/Lost_Video",
+        )
+    for location in no_path:
+        assert_raises(NoPathToItem, path_to_location, modulestore, course_id, location)
+

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

 import pymongo
 
 from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup
-from path import path
 from pprint import pprint
 
 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
-TEST_DIR = path(__file__).abspath().dirname()
-for i in range(5):
-    TEST_DIR = TEST_DIR.dirname()
-TEST_DIR = TEST_DIR / 'test'
-
-DATA_DIR = TEST_DIR / 'data'
+from .test_modulestore import check_path_to_location
+from . import DATA_DIR
 
 
 HOST = 'localhost'
@@ -110,27 +101,5 @@ class TestMongoModuleStore(object):
 
     def test_path_to_location(self):
         '''Make sure that path_to_location works'''
-        should_work = (
-            ("i4x://edX/toy/video/Welcome",
-             ("edX/toy/2012_Fall", "Overview", "Welcome", None)),
-            ("i4x://edX/toy/chapter/Overview",
-             ("edX/toy/2012_Fall", "Overview", None, None)),
-            )
-        for location, expected in should_work:
-            assert_equals(path_to_location(self.store, location), expected)
-
-        not_found = (
-            "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome"
-            )
-        for location in not_found:
-            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
-        # another course.
-        no_path = (
-            "i4x://edX/simple/video/Lost_Video",
-            )
-        for location in no_path:
-            assert_raises(NoPathToItem, path_to_location, self.store, location, "toy")
+        check_path_to_location(self.store)
 

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

+from xmodule.modulestore import Location
+from xmodule.modulestore.xml import XMLModuleStore
+from xmodule.modulestore.xml_importer import import_from_xml
+
+from .test_modulestore import check_path_to_location
+from . import DATA_DIR
+
+class TestXMLModuleStore(object):
+    def test_path_to_location(self):
+        """Make sure that path_to_location works properly"""
+
+        print "Starting import"
+        modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy', 'simple'])
+        print "finished import"
+        
+        check_path_to_location(modulestore)

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

@@ -37,7 +37,7 @@ def clean_out_mako_templating(xml_string):
 
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
     def __init__(self, xmlstore, course_id, course_dir,
-                 policy, error_tracker, **kwargs):
+                 policy, error_tracker, parent_tracker, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
         all elements have unique slugs.
@@ -134,8 +134,8 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
 
             xmlstore.modules[course_id][descriptor.location] = descriptor
 
-            if xmlstore.eager:
-                descriptor.get_children()
+            for child in descriptor.get_children():
+                parent_tracker.add_parent(child.location, descriptor.location)
             return descriptor
 
         render_template = lambda: ''
@@ -151,6 +151,46 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                                   error_tracker, process_xml, policy, **kwargs)
 
 
+class ParentTracker(object):
+    """A simple class to factor out the logic for tracking location parent pointers."""
+    def __init__(self):
+        """
+        Init
+        """
+        # location -> set(parents).  Not using defaultdict because we care about the empty case.
+        self._parents = dict()
+
+    def add_parent(self, child, parent):
+        """
+        Add a parent of child location to the set of parents.  Duplicate calls have no effect.
+
+        child and parent must be something that can be passed to Location.
+        """
+        child = Location(child)
+        parent = Location(parent)
+        s = self._parents.setdefault(child, set())
+        s.add(parent)
+
+    def is_known(self, child):
+        """
+        returns True iff child has some parents.
+        """
+        child = Location(child)
+        return child in self._parents
+
+    def make_known(self, location):
+        """Tell the parent tracker about an object, without registering any
+        parents for it.  Used for the top level course descriptor locations."""
+        self._parents.setdefault(location, set())
+
+    def parents(self, child):
+        """
+        Return a list of the parents of this child.  If not is_known(child), will throw a KeyError
+        """
+        child = Location(child)
+        return list(self._parents[child])
+
+
 class XMLModuleStore(ModuleStoreBase):
     """
     An XML backed ModuleStore
@@ -191,6 +231,8 @@ class XMLModuleStore(ModuleStoreBase):
         #log.debug('XMLModuleStore: eager=%s, data_dir = %s' % (eager, self.data_dir))
         #log.debug('default_class = %s' % self.default_class)
 
+        self.parent_tracker = ParentTracker()
+
         # If we are specifically asked for missing courses, that should
         # be an error.  If we are asked for "all" courses, find the ones
         # that have a course.xml
@@ -221,6 +263,7 @@ class XMLModuleStore(ModuleStoreBase):
         if course_descriptor is not None:
             self.courses[course_dir] = course_descriptor
             self._location_errors[course_descriptor.location] = errorlog
+            self.parent_tracker.make_known(course_descriptor.location)
         else:
             # Didn't load course.  Instead, save the errors elsewhere.
             self.errored_courses[course_dir] = errorlog
@@ -339,7 +382,7 @@ class XMLModuleStore(ModuleStoreBase):
 
 
             course_id = CourseDescriptor.make_id(org, course, url_name)
-            system = ImportSystem(self, course_id, course_dir, policy, tracker)
+            system = ImportSystem(self, course_id, course_dir, policy, tracker, self.parent_tracker)
 
             course_descriptor = system.process_xml(etree.tostring(course_data))
 
@@ -450,3 +493,19 @@ class XMLModuleStore(ModuleStoreBase):
         metadata: A nested dictionary of module metadata
         """
         raise NotImplementedError("XMLModuleStores are read-only")
+
+    def get_parent_locations(self, location):
+        '''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.  This may
+        be empty if there are no parents.
+        '''
+        location = Location.ensure_fully_specified(location)
+        if not self.parent_tracker.is_known(location):
+            raise ItemNotFoundError(location)
+
+        return self.parent_tracker.parents(location)

common/lib/xmodule/xmodule/x_module.py

@@ -544,7 +544,13 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
             # Put import here to avoid circular import errors
             from xmodule.error_module import ErrorDescriptor
             msg = "Error loading from xml."
-            log.warning(msg + " " + str(err))
+            log.warning(msg + " " + str(err)[:200])
+
+            # Normally, we don't want lots of exception traces in our logs from common
+            # content problems.  But if you're debugging the xml loading code itself,
+            # uncomment the next line.
+            # log.exception(msg)
+
             system.error_tracker(msg)
             err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())
             descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course,

lms/djangoapps/courseware/tests/tests.py

@@ -204,7 +204,8 @@ class PageLoader(ActivateLoginTestCase):
         self.assertEqual(len(courses), 1)
         course = courses[0]
         self.enroll(course)
-
+        course_id = course.id
+        
         n = 0
         num_bad = 0
         all_ok = True
@@ -214,7 +215,8 @@ class PageLoader(ActivateLoginTestCase):
             print "Checking ", descriptor.location.url()
             #print descriptor.__class__, descriptor.location
             resp = self.client.get(reverse('jump_to',
-                                   kwargs={'location': descriptor.location.url()}))
+                                   kwargs={'course_id': course_id,
+                                           'location': descriptor.location.url()}))
             msg = str(resp.status_code)
 
             if resp.status_code != 200:

lms/djangoapps/courseware/views.py

@@ -194,7 +194,7 @@ def index(request, course_id, chapter=None, section=None,
 
 
 @ensure_csrf_cookie
-def jump_to(request, location):
+def jump_to(request, course_id, location):
     '''
     Show the page that contains a specific location.
 
@@ -211,7 +211,7 @@ def jump_to(request, location):
 
     # Complain if there's not data for this location
     try:
-        (course_id, chapter, section, position) = path_to_location(modulestore(), location)
+        (course_id, chapter, section, position) = path_to_location(modulestore(), course_id, location)
     except ItemNotFoundError:
         raise Http404("No data at this location: {0}".format(location))
     except NoPathToItem:

lms/urls.py

@@ -98,8 +98,9 @@ if settings.COURSEWARE_ENABLED:
     urlpatterns += (
         # Hook django-masquerade, allowing staff to view site as other users
         url(r'^masquerade/', include('masquerade.urls')),
-        url(r'^jump_to/(?P<location>.*)$', 'courseware.views.jump_to', name="jump_to"),
 
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/jump_to/(?P<location>.*)$',
+            'courseware.views.jump_to', name="jump_to"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/modx/(?P<location>.*?)/(?P<dispatch>[^/]*)$',
             'courseware.module_render.modx_dispatch',
             name='modx_dispatch'),