From 22aa325d83a3fcf7680f00fc4e4ae67873ca4099 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 30 Aug 2012 16:37:40 -0400 Subject: [PATCH 1/6] Enable linking into the middle of sequences * add a url that has an extra /{position} at the end * pass it through to get_module --- lms/djangoapps/courseware/module_render.py | 3 +-- lms/djangoapps/courseware/views.py | 4 +--- lms/urls.py | 4 +++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 82ec3cadeb..ee29491d27 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -195,7 +195,6 @@ def _get_module(user, request, location, student_module_cache, course_id, positi descriptor.category, shared_state_key) - instance_state = instance_module.state if instance_module is not None else None shared_state = shared_module.state if shared_module is not None else None @@ -254,7 +253,7 @@ def _get_module(user, request, location, student_module_cache, course_id, positi # by the replace_static_urls code below replace_urls=replace_urls, node_path=settings.NODE_PATH, - anonymous_student_id=anonymous_student_id + anonymous_student_id=anonymous_student_id, ) # pass position specified in URL to module through ModuleSystem system.set('position', position) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 92f6716320..5e75e99a4f 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -5,8 +5,6 @@ import itertools from functools import partial -from functools import partial - from django.conf import settings from django.core.context_processors import csrf from django.core.urlresolvers import reverse @@ -152,7 +150,7 @@ def index(request, course_id, chapter=None, section=None, course_id, request.user, section_descriptor) module = get_module(request.user, request, section_descriptor.location, - student_module_cache, course_id) + student_module_cache, course_id, position) if module is None: # User is probably being clever and trying to access something # they don't have access to. diff --git a/lms/urls.py b/lms/urls.py index 86d654eb40..f19f4682f9 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -142,6 +142,8 @@ if settings.COURSEWARE_ENABLED: 'courseware.views.index', name="courseware_chapter"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/courseware/(?P[^/]*)/(?P
[^/]*)/$', 'courseware.views.index', name="courseware_section"), + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/courseware/(?P[^/]*)/(?P
[^/]*)/(?P[^/]*)/?$', + 'courseware.views.index', name="courseware_position"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/progress$', 'courseware.views.progress', name="progress"), # Takes optional student_id for instructor use--shows profile as that student sees it. @@ -164,7 +166,7 @@ if settings.COURSEWARE_ENABLED: if settings.MITX_FEATURES.get('ENABLE_DISCUSSION_SERVICE'): urlpatterns += ( - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/news$', + url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/news$', 'courseware.views.news', name="news"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/discussion/', include('django_comment_client.urls')) From 021ccde1b41610363664ea110d228e46210608d1 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 30 Aug 2012 18:47:11 -0400 Subject: [PATCH 2/6] 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. --- .../lib/xmodule/xmodule/modulestore/search.py | 12 ++-- .../xmodule/modulestore/tests/__init__.py | 12 ++++ .../modulestore/tests/test_modulestore.py | 34 ++++++++++ .../xmodule/modulestore/tests/test_mongo.py | 37 +--------- .../xmodule/modulestore/tests/test_xml.py | 16 +++++ common/lib/xmodule/xmodule/modulestore/xml.py | 67 +++++++++++++++++-- common/lib/xmodule/xmodule/x_module.py | 8 ++- lms/djangoapps/courseware/tests/tests.py | 6 +- lms/djangoapps/courseware/views.py | 4 +- lms/urls.py | 3 +- 10 files changed, 149 insertions(+), 50 deletions(-) create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/__init__.py create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_xml.py 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'), From 484735bf034d5190a3e8064eb51e4b4ea252f4fd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 30 Aug 2012 18:55:38 -0400 Subject: [PATCH 3/6] make jump_to go to right position in a sequence. --- common/lib/xmodule/xmodule/modulestore/search.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index a1ed407ee2..87f1cbf825 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -26,7 +26,7 @@ def path_to_location(modulestore, course_id, location): 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 + If the section is a sequential or vertical, position will be the position of this location in that sequence. Otherwise, position will be None. TODO (vshnayder): Not true yet. ''' @@ -83,15 +83,21 @@ def path_to_location(modulestore, course_id, location): path = find_path_to_course() if path is None: - raise(NoPathToItem(location)) + raise NoPathToItem(location) n = len(path) course_id = CourseDescriptor.location_to_id(path[0]) # pull out the location names 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... + # Figure out the position position = None + if n > 3: + section_desc = modulestore.get_instance(course_id, path[2]) + child_locs = [c.location for c in section_desc.get_children()] + # positions are 1-indexed, and should be strings to be consistent with + # url parsing. + position = str(child_locs.index(path[3]) + 1) + return (course_id, chapter, section, position) From d815d50aee5d8660944a8ec37ae42f64612f7beb Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 30 Aug 2012 19:26:50 -0400 Subject: [PATCH 4/6] Remove obsolete eager flag for xml modulestore * has to be eager --- .../xmodule/xmodule/modulestore/tests/test_xml.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 12 +----------- .../lib/xmodule/xmodule/modulestore/xml_importer.py | 3 +-- common/lib/xmodule/xmodule/tests/test_export.py | 4 ++-- common/lib/xmodule/xmodule/tests/test_import.py | 8 ++++---- .../courseware/management/commands/clean_xml.py | 1 - .../management/commands/metadata_to_json.py | 1 - lms/djangoapps/courseware/tests/tests.py | 1 - lms/djangoapps/django_comment_client/utils.py | 9 +++++---- lms/envs/common.py | 1 - 10 files changed, 14 insertions(+), 28 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py index 0c5c212bbf..c4446bebb5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_xml.py @@ -10,7 +10,7 @@ class TestXMLModuleStore(object): """Make sure that path_to_location works properly""" print "Starting import" - modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy', 'simple']) + modulestore = XMLModuleStore(DATA_DIR, 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 ce3635c4b9..8cb3da7e7a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -195,8 +195,7 @@ class XMLModuleStore(ModuleStoreBase): """ An XML backed ModuleStore """ - def __init__(self, data_dir, default_class=None, eager=False, - course_dirs=None): + def __init__(self, data_dir, default_class=None, course_dirs=None): """ Initialize an XMLModuleStore from data_dir @@ -205,15 +204,11 @@ class XMLModuleStore(ModuleStoreBase): default_class: dot-separated string defining the default descriptor class to use if none is specified in entry_points - eager: If true, load the modules children immediately to force the - entire course tree to be parsed - course_dirs: If specified, the list of course_dirs to load. Otherwise, load all course dirs """ ModuleStoreBase.__init__(self) - self.eager = eager self.data_dir = path(data_dir) self.modules = defaultdict(dict) # course_id -> dict(location -> XModuleDescriptor) self.courses = {} # course_dir -> XModuleDescriptor for the course @@ -226,11 +221,6 @@ class XMLModuleStore(ModuleStoreBase): class_ = getattr(import_module(module_path), class_name) self.default_class = class_ - # TODO (cpennington): We need a better way of selecting specific sets of - # debug messages to enable. These were drowning out important messages - #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 diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 89f94d8cdb..be0bdc24c2 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -6,7 +6,7 @@ from .exceptions import DuplicateItemError log = logging.getLogger(__name__) -def import_from_xml(store, data_dir, course_dirs=None, eager=True, +def import_from_xml(store, data_dir, course_dirs=None, default_class='xmodule.raw_module.RawDescriptor'): """ Import the specified xml data_dir into the "store" modulestore, @@ -19,7 +19,6 @@ def import_from_xml(store, data_dir, course_dirs=None, eager=True, module_store = XMLModuleStore( data_dir, default_class=default_class, - eager=eager, course_dirs=course_dirs ) for course_id in module_store.modules.keys(): diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index 2520d95937..826e6c9d5a 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -49,7 +49,7 @@ class RoundTripTestCase(unittest.TestCase): copytree(data_dir / course_dir, root_dir / course_dir) print "Starting import" - initial_import = XMLModuleStore(root_dir, eager=True, course_dirs=[course_dir]) + initial_import = XMLModuleStore(root_dir, course_dirs=[course_dir]) courses = initial_import.get_courses() self.assertEquals(len(courses), 1) @@ -66,7 +66,7 @@ class RoundTripTestCase(unittest.TestCase): course_xml.write(xml) print "Starting second import" - second_import = XMLModuleStore(root_dir, eager=True, course_dirs=[course_dir]) + second_import = XMLModuleStore(root_dir, course_dirs=[course_dir]) courses2 = second_import.get_courses() self.assertEquals(len(courses2), 1) diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index a369850209..e81d82bf9e 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -193,7 +193,7 @@ class ImportTestCase(unittest.TestCase): """Make sure that metadata is inherited properly""" print "Starting import" - initial_import = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy']) + initial_import = XMLModuleStore(DATA_DIR, course_dirs=['toy']) courses = initial_import.get_courses() self.assertEquals(len(courses), 1) @@ -216,7 +216,7 @@ class ImportTestCase(unittest.TestCase): def get_course(name): print "Importing {0}".format(name) - modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=[name]) + modulestore = XMLModuleStore(DATA_DIR, course_dirs=[name]) courses = modulestore.get_courses() self.assertEquals(len(courses), 1) return courses[0] @@ -245,7 +245,7 @@ class ImportTestCase(unittest.TestCase): happen--locations should uniquely name definitions. But in our imperfect XML world, it can (and likely will) happen.""" - modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy', 'two_toys']) + modulestore = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'two_toys']) toy_id = "edX/toy/2012_Fall" two_toy_id = "edX/toy/TT_2012_Fall" @@ -261,7 +261,7 @@ class ImportTestCase(unittest.TestCase): """Ensure that colons in url_names convert to file paths properly""" print "Starting import" - modulestore = XMLModuleStore(DATA_DIR, eager=True, course_dirs=['toy']) + modulestore = XMLModuleStore(DATA_DIR, course_dirs=['toy']) courses = modulestore.get_courses() self.assertEquals(len(courses), 1) diff --git a/lms/djangoapps/courseware/management/commands/clean_xml.py b/lms/djangoapps/courseware/management/commands/clean_xml.py index 9fd52178a2..425dd156c1 100644 --- a/lms/djangoapps/courseware/management/commands/clean_xml.py +++ b/lms/djangoapps/courseware/management/commands/clean_xml.py @@ -57,7 +57,6 @@ def import_with_checks(course_dir, verbose=True): # module. modulestore = XMLModuleStore(data_dir, default_class=None, - eager=True, course_dirs=course_dirs) def str_of_err(tpl): diff --git a/lms/djangoapps/courseware/management/commands/metadata_to_json.py b/lms/djangoapps/courseware/management/commands/metadata_to_json.py index dcbcdc0df3..8ef0dee7b3 100644 --- a/lms/djangoapps/courseware/management/commands/metadata_to_json.py +++ b/lms/djangoapps/courseware/management/commands/metadata_to_json.py @@ -23,7 +23,6 @@ def import_course(course_dir, verbose=True): # module. modulestore = XMLModuleStore(data_dir, default_class=None, - eager=True, course_dirs=course_dirs) def str_of_err(tpl): diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 280dada391..56a2384bc5 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -68,7 +68,6 @@ def xml_store_config(data_dir): 'OPTIONS': { 'data_dir': data_dir, 'default_class': 'xmodule.hidden_module.HiddenDescriptor', - 'eager': True, } } } diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 516344d79b..1122db3ad8 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -43,7 +43,8 @@ def get_full_modules(): class_path = settings.MODULESTORE['default']['ENGINE'] module_path, _, class_name = class_path.rpartition('.') class_ = getattr(import_module(module_path), class_name) - modulestore = class_(**dict(settings.MODULESTORE['default']['OPTIONS'].items() + [('eager', True)])) + # TODO (vshnayder): wth is this doing??? + modulestore = class_(**dict(settings.MODULESTORE['default']['OPTIONS'].items())) _FULLMODULES = modulestore.modules return _FULLMODULES @@ -76,7 +77,7 @@ def initialize_discussion_info(request, course): _is_course_discussion = lambda x: x[0].dict()['category'] == 'discussion' \ and x[0].dict()['course'] == course_name - + _get_module_descriptor = operator.itemgetter(1) def _get_module(module_descriptor): @@ -135,8 +136,8 @@ class HtmlResponse(HttpResponse): def __init__(self, html=''): super(HtmlResponse, self).__init__(html, content_type='text/plain') -class ViewNameMiddleware(object): - def process_view(self, request, view_func, view_args, view_kwargs): +class ViewNameMiddleware(object): + def process_view(self, request, view_func, view_args, view_kwargs): request.view_name = view_func.__name__ class QueryCountDebugMiddleware(object): diff --git a/lms/envs/common.py b/lms/envs/common.py index ce08bf9666..0e2b2d38e6 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -219,7 +219,6 @@ MODULESTORE = { 'OPTIONS': { 'data_dir': DATA_DIR, 'default_class': 'xmodule.hidden_module.HiddenDescriptor', - 'eager': True, } } } From 32cb0897e9b95d3abdcd9419ad30859ee226c2f4 Mon Sep 17 00:00:00 2001 From: Arjun Singh Date: Fri, 31 Aug 2012 04:24:52 -0700 Subject: [PATCH 5/6] Fixing sequentials nested inside of verticals, provides a path to sequentials nested in sequentials. Changes jump_to to redirect to courseware index. --- common/lib/xmodule/xmodule/modulestore/search.py | 15 +++++++++------ lms/djangoapps/courseware/views.py | 7 +++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 87f1cbf825..1eb880c56a 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -93,11 +93,14 @@ def path_to_location(modulestore, course_id, location): # Figure out the position position = None if n > 3: - section_desc = modulestore.get_instance(course_id, path[2]) - child_locs = [c.location for c in section_desc.get_children()] - # positions are 1-indexed, and should be strings to be consistent with - # url parsing. - position = str(child_locs.index(path[3]) + 1) - + position_list = [] + for path_index in range(2, n-1): + if path[path_index].category == 'sequential': + section_desc = modulestore.get_instance(course_id, path[path_index]) + child_locs = [c.location for c in section_desc.get_children()] + # positions are 1-indexed, and should be strings to be consistent with + # url parsing. + position_list.append(str(child_locs.index(path[path_index+1]) + 1)) + position = "_".join(position_list) return (course_id, chapter, section, position) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 3f0f54147b..d1a52f06c1 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -218,8 +218,11 @@ def jump_to(request, course_id, location): raise Http404("This location is not in any class: {0}".format(location)) # Rely on index to do all error handling and access control. - return index(request, course_id, chapter, section, position) - + return redirect('courseware_position', + course_id=course_id, + chapter=chapter, + section=section, + position=position) @ensure_csrf_cookie def course_info(request, course_id): """ From b58436545a5cea8fea816677a8ced1bab3712d66 Mon Sep 17 00:00:00 2001 From: Arjun Singh Date: Fri, 31 Aug 2012 06:45:59 -0700 Subject: [PATCH 6/6] Add note about positional modules; check for videosequence. --- common/lib/xmodule/xmodule/modulestore/search.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 1eb880c56a..f9901e8bfe 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -92,10 +92,18 @@ def path_to_location(modulestore, course_id, location): section = path[2].name if n > 2 else None # Figure out the position position = None + + # This block of code will find the position of a module within a nested tree + # of modules. If a problem is on tab 2 of a sequence that's on tab 3 of a + # sequence, the resulting position is 3_2. However, no positional modules + # (e.g. sequential and videosequence) currently deal with this form of + # representing nested positions. This needs to happen before jumping to a + # module nested in more than one positional module will work. if n > 3: position_list = [] for path_index in range(2, n-1): - if path[path_index].category == 'sequential': + category = path[path_index].category + if category == 'sequential' or category == 'videosequence': section_desc = modulestore.get_instance(course_id, path[path_index]) child_locs = [c.location for c in section_desc.get_children()] # positions are 1-indexed, and should be strings to be consistent with