diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 6e2cf5b791..eeda9a6b65 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -30,6 +30,7 @@ from django_future.csrf import ensure_csrf_cookie from student.models import Registration, UserProfile, PendingNameChange, PendingEmailChange, CourseEnrollment from util.cache import cache_if_anonymous from xmodule.course_module import CourseDescriptor +from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 0808d5fbb0..1eda54ea32 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -30,6 +30,9 @@ class CourseDescriptor(SequenceDescriptor): @classmethod def id_to_location(cls, course_id): + '''Convert the given course_id (org/course/name) to a location object. + Throws ValueError if course_id is of the wrong format. + ''' org, course, name = course_id.split('/') return Location('i4x', org, course, 'course', name) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index ac03d92854..77dfacf372 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -45,6 +45,17 @@ class Location(_LocationBase): """ return re.sub('_+', '_', INVALID_CHARS.sub('_', value)) + @classmethod + def is_valid(cls, value): + ''' + Check if the value is a valid location, in any acceptable format. + ''' + try: + Location(value) + except InvalidLocationError: + return False + return True + def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None, name=None, revision=None): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py index 19bdb105c1..70c6351685 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py @@ -13,14 +13,51 @@ def test_string_roundtrip(): check_string_roundtrip("tag://org/course/category/name/revision") +input_dict = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name', + 'org': 'org' +} + +input_list = ['tag', 'org', 'course', 'category', 'name'] + +input_str = "tag://org/course/category/name" +input_str_rev = "tag://org/course/category/name/revision" + +valid = (input_list, input_dict, input_str, input_str_rev) + +invalid_dict = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name/more_name', + 'org': 'org' +} + +invalid_dict2 = { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name ', # extra space + 'org': 'org' +} + +invalid = ("foo", ["foo"], ["foo", "bar"], + ["foo", "bar", "baz", "blat", "foo/bar"], + "tag://org/course/category/name with spaces/revision", + invalid_dict, + invalid_dict2) + +def test_is_valid(): + for v in valid: + assert_equals(Location.is_valid(v), True) + + for v in invalid: + assert_equals(Location.is_valid(v), False) + def test_dict(): - input_dict = { - 'tag': 'tag', - 'course': 'course', - 'category': 'category', - 'name': 'name', - 'org': 'org' - } assert_equals("tag://org/course/category/name", Location(input_dict).url()) assert_equals(dict(revision=None, **input_dict), Location(input_dict).dict()) @@ -30,7 +67,6 @@ def test_dict(): def test_list(): - input_list = ['tag', 'org', 'course', 'category', 'name'] assert_equals("tag://org/course/category/name", Location(input_list).url()) assert_equals(input_list + [None], Location(input_list).list()) @@ -65,3 +101,13 @@ def test_equality(): Location('tag', 'org', 'course', 'category', 'name1'), Location('tag', 'org', 'course', 'category', 'name') ) + +def test_clean(): + pairs = [ ('',''), + (' ', '_'), + ('abc,', 'abc_'), + ('ab fg!@//\\aj', 'ab_fg_aj'), + (u"ab\xA9", "ab_"), # no unicode allowed for now + ] + for input, output in pairs: + assert_equals(Location.clean(input), output) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index f273778a3c..ca9aa417d1 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -20,12 +20,16 @@ from module_render import toc_for_course, get_module, get_section from models import StudentModuleCache from student.models import UserProfile from multicourse import multicourse_settings +from xmodule.modulestore import Location +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError +from xmodule.modulestore.django import modulestore +from xmodule.course_module import CourseDescriptor from util.cache import cache, cache_if_anonymous from student.models import UserTestGroup, CourseEnrollment from courseware import grades from courseware.courses import check_course -from xmodule.modulestore.django import modulestore + log = logging.getLogger("mitx.courseware") @@ -205,53 +209,30 @@ def index(request, course_id, chapter=None, section=None, return result -def jump_to(request, probname=None): +def jump_to(request, location): ''' - Jump to viewing a specific problem. The problem is specified by a - problem name - currently the filename (minus .xml) of the problem. - Maybe this should change to a more generic tag, eg "name" given as - an attribute in . + Show the page that contains a specific location. - We do the jump by (1) reading course.xml to find the first - instance of with the given filename, then (2) finding - the parent element of the problem, then (3) rendering that parent - element with a specific computed position value (if it is - ). + If the location is invalid, return a 404. + If the location is valid, but not present in a course, ? + + If the location is valid, but in a course the current user isn't registered for, ? + TODO -- let the index view deal with it? ''' - # get coursename if stored - coursename = multicourse_settings.get_coursename_from_request(request) - - # begin by getting course.xml tree - xml = content_parser.course_file(request.user, coursename) - - # look for problem of given name - pxml = xml.xpath('//problem[@filename="%s"]' % probname) - if pxml: - pxml = pxml[0] - - # get the parent element - parent = pxml.getparent() - - # figure out chapter and section names - chapter = None - section = None - branch = parent - for k in range(4): # max depth of recursion - if branch.tag == 'section': - section = branch.get('name') - if branch.tag == 'chapter': - chapter = branch.get('name') - branch = branch.getparent() - - position = None - if parent.tag == 'sequential': - position = parent.index(pxml) + 1 # position in sequence - - return index(request, - course=coursename, chapter=chapter, - section=section, position=position) + # Complain if the location isn't valid + try: + location = Location(location) + except InvalidLocationError: + raise Http404("Invalid location") + # Complain if there's not data for this location + try: + item = modulestore().get_item(location) + except ItemNotFoundError: + raise Http404("No data at this location: {0}".format(location)) + + return HttpResponse("O hai") @ensure_csrf_cookie def course_info(request, course_id): diff --git a/lms/urls.py b/lms/urls.py index 81e1343104..b583b252e2 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -97,7 +97,8 @@ if settings.PERFSTATS: if settings.COURSEWARE_ENABLED: urlpatterns += ( url(r'^masquerade/', include('masquerade.urls')), - url(r'^jumpto/(?P[^/]+)/$', 'courseware.views.jump_to'), + url(r'^jumpto/(?P.*)$', 'courseware.views.jump_to'), + url(r'^modx/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.modx_dispatch'), #reset_problem'), url(r'^xqueue/(?P[^/]*)/(?P.*?)/(?P[^/]*)$', 'courseware.module_render.xqueue_callback'), url(r'^change_setting$', 'student.views.change_setting'),