Clean up course_info views

* catch exceptions and return a 404 if course not found
* add Location.is_valid(), tests
* stub of jumpto/ view.

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
 

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)
 

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):
         """

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)

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 <problem>.
+    Show the page that contains a specific location.
 
-    We do the jump by (1) reading course.xml to find the first
-    instance of <problem> 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
-    <sequential>).
+    If the location is invalid, return a 404.
 
-    '''
-    # 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)
+    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?
+    '''
+    # 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):

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<probname>[^/]+)/$', 'courseware.views.jump_to'),
+        url(r'^jumpto/(?P<location>.*)$', 'courseware.views.jump_to'),
+        
         url(r'^modx/(?P<id>.*?)/(?P<dispatch>[^/]*)$', 'courseware.module_render.modx_dispatch'), #reset_problem'),
         url(r'^xqueue/(?P<userid>[^/]*)/(?P<id>.*?)/(?P<dispatch>[^/]*)$', 'courseware.module_render.xqueue_callback'),
         url(r'^change_setting$', 'student.views.change_setting'),