diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index baf3d46b57..a1ed407ee2 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/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)) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/__init__.py b/common/lib/xmodule/xmodule/modulestore/tests/__init__.py new file mode 100644 index 0000000000..126f0136e2 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/__init__.py @@ -0,0 +1,12 @@ +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' + + diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py new file mode 100644 index 0000000000..c1d1d50a53 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py @@ -0,0 +1,34 @@ +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) + diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 746240e763..4c593e391e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -1,23 +1,14 @@ 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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py new file mode 100644 index 0000000000..0c5c212bbf --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -0,0 +1,16 @@ +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) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 92eca8f5e6..ce3635c4b9 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 0dc16bd976..82f623e977 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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, diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 88e0f53f70..280dada391 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/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: diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 5e75e99a4f..3f0f54147b 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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: diff --git a/lms/urls.py b/lms/urls.py index f19f4682f9..2fecaa94cc 100644 --- a/lms/urls.py +++ b/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.*)$', 'courseware.views.jump_to', name="jump_to"), + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/jump_to/(?P.*)$', + 'courseware.views.jump_to', name="jump_to"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/modx/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.modx_dispatch', name='modx_dispatch'),