Merge pull request #1416 from edx/dhm/regex_courseid

Parse locator url better

cms/djangoapps/contentstore/tests/test_course_index.py

@@ -61,7 +61,7 @@ class TestCourseIndex(CourseTestCase):
         Test the error conditions for the access
         """
         locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True)
-        outline_url = reverse('contentstore.views.course_handler', kwargs={'course_url': unicode(locator)})
+        outline_url = locator.url_reverse('course/', '')
         # register a non-staff member and try to delete the course branch
         non_staff_client, _ = self.createNonStaffAuthedUserClient()
         response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json')

cms/djangoapps/contentstore/views/course.py

@@ -60,7 +60,7 @@ __all__ = ['create_new_course', 'course_info', 'course_handler',
 
 
 @login_required
-def course_handler(request, course_url):
+def course_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for course specific requests.
     It provides the course tree with the necessary information for identifying and labeling the parts. The root
@@ -84,7 +84,10 @@ def course_handler(request, course_url):
     if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
         if request.method == 'GET':
             raise NotImplementedError('coming soon')
-        elif not has_access(request.user, BlockUsageLocator(course_url)):
+        elif not has_access(
+            request.user,
+            BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+        ):
             raise PermissionDenied()
         elif request.method == 'POST':
             raise NotImplementedError()
@@ -95,7 +98,7 @@ def course_handler(request, course_url):
         else:
             return HttpResponseBadRequest()
     elif request.method == 'GET':  # assume html
-        return course_index(request, course_url)
+        return course_index(request, course_id, branch, version_guid, block)
     else:
         return HttpResponseNotFound()
 
@@ -108,18 +111,18 @@ def old_course_index_shim(request, org, course, name):
     """
     old_location = Location(['i4x', org, course, 'course', name])
     locator = loc_mapper().translate_location(old_location.course_id, old_location, False, True)
-    return course_index(request, locator)
+    return course_index(request, locator.course_id, locator.branch, locator.version_guid, locator.usage_id)
 
 
 @login_required
 @ensure_csrf_cookie
-def course_index(request, course_url):
+def course_index(request, course_id, branch, version_guid, block):
     """
     Display an editable course overview.
 
     org, course, name: Attributes of the Location for the item to edit
     """
-    location = BlockUsageLocator(course_url)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
     # TODO: when converting to split backend, if location does not have a usage_id,
     # we'll need to get the course's root block_id
     if not has_access(request.user, location):

cms/djangoapps/contentstore/views/user.py

@@ -47,12 +47,13 @@ def index(request):
 
     def format_course_for_view(course):
         # published = false b/c studio manipulates draft versions not b/c the course isn't pub'd
-        course_url = loc_mapper().translate_location(
+        course_loc = loc_mapper().translate_location(
             course.location.course_id, course.location, published=False, add_entry_if_missing=True
         )
         return (
             course.display_name,
-            reverse("contentstore.views.course_handler", kwargs={'course_url': course_url}),
+            # note, couldn't get django reverse to work; so, wrote workaround
+            course_loc.url_reverse('course/', ''),
             get_lms_link_for_item(
                 course.location
             ),

cms/templates/widgets/header.html

@@ -15,10 +15,7 @@
       % if context_course:
       <% 
             ctx_loc = context_course.location
-            index_url = reverse(
-                'contentstore.views.course_handler', 
-                kwargs={'course_url': loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)}
-            )
+            index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '')
       %>
       <h2 class="info-course">
         <span class="sr">${_("Current Course:")}</span>

cms/urls.py

+import re
 from django.conf import settings
 from django.conf.urls import patterns, include, url
 
 # TODO: This should be removed once the CMS is running via wsgi on all production servers
 import cms.startup as startup
+from xmodule.modulestore import parsers
 startup.run()
 
 # There is a course creators admin table.
@@ -136,7 +138,8 @@ urlpatterns += patterns(
     ),
 
     url(r'^course$', 'index'),
-    url(r'^course/(?P<course_url>.*)$', 'course_handler'),
+    # (?ix) == ignore case and verbose (multiline regex)
+    url(r'(?ix)^course/{}$'.format(parsers.URL_RE_SOURCE), 'course_handler'),
 )
 
 js_info_dict = {

common/lib/xmodule/xmodule/modulestore/exceptions.py

@@ -42,7 +42,7 @@ class VersionConflictError(Exception):
     """
     The caller asked for either draft or published head and gave a version which conflicted with it.
     """
-    def __init__(self, requestedLocation, currentHead):
+    def __init__(self, requestedLocation, currentHeadVersionGuid):
         super(VersionConflictError, self).__init__()
         self.requestedLocation = requestedLocation
-        self.currentHead = currentHead
+        self.currentHeadVersionGuid = currentHeadVersionGuid

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -13,7 +13,7 @@ from bson.errors import InvalidId
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError
 
 from .parsers import parse_url, parse_course_id, parse_block_ref
-from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, URL_VERSION_PREFIX
+from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX
 import re
 from xmodule.modulestore import Location
 
@@ -190,8 +190,8 @@ class CourseLocator(Locator):
             self.init_from_version_guid(version_guid)
         if course_id or branch:
             self.init_from_course_id(course_id, branch)
-        assert self.version_guid or self.course_id, \
-            "Either version_guid or course_id should be set."
+        if self.version_guid is None and self.course_id is None:
+            raise ValueError("Either version_guid or course_id should be set: {}".format(url))
 
     def __unicode__(self):
         """
@@ -200,10 +200,10 @@ class CourseLocator(Locator):
         if self.course_id:
             result = self.course_id
             if self.branch:
-                result += BRANCH_PREFIX + self.branch
+                result += '/' + BRANCH_PREFIX + self.branch
             return result
         elif self.version_guid:
-            return URL_VERSION_PREFIX + str(self.version_guid)
+            return VERSION_PREFIX + str(self.version_guid)
         else:
             # raise InsufficientSpecificationError("missing course_id or version_guid")
             return '<InsufficientSpecificationError: missing course_id or version_guid>'
@@ -263,22 +263,42 @@ class CourseLocator(Locator):
                              version_guid=self.version_guid,
                              branch=self.branch)
 
+    def url_reverse(self, prefix, postfix):
+        """
+        Do what reverse is supposed to do but seems unable to do. Generate a url using prefix unicode(self) postfix
+        :param prefix: the beginning of the url (will be forced to begin and end with / if non-empty)
+        :param postfix: the part to append to the url (will be forced to begin w/ / if non-empty)
+        """
+        if prefix:
+            if not prefix.endswith('/'):
+                prefix += '/'
+            if not prefix.startswith('/'):
+                prefix = '/' + prefix
+        else:
+            prefix = '/'
+        if postfix and not postfix.startswith('/'):
+            postfix = '/' + postfix
+        elif postfix is None:
+            postfix = ''
+        return prefix + unicode(self) + postfix
+
     def init_from_url(self, url):
         """
         url must be a string beginning with 'edx://' and containing
         either a valid version_guid or course_id (with optional branch), or both.
         """
         if isinstance(url, Locator):
-            url = url.url()
-        if not isinstance(url, basestring):
+            parse = url.__dict__
+        elif not isinstance(url, basestring):
             raise TypeError('%s is not an instance of basestring' % url)
+        else:
             parse = parse_url(url, tag_optional=True)
             if not parse:
                 raise ValueError('Could not parse "%s" as a url' % url)
         self._set_value(
             parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid))
         )
-        self._set_value(parse, 'id', lambda (new_id): self.set_course_id(new_id))
+        self._set_value(parse, 'course_id', lambda (new_id): self.set_course_id(new_id))
         self._set_value(parse, 'branch', lambda (new_branch): self.set_branch(new_branch))
 
     def init_from_version_guid(self, version_guid):
@@ -313,9 +333,9 @@ class CourseLocator(Locator):
                     raise ValueError("%s does not have a valid course_id" % course_id)
 
             parse = parse_course_id(course_id)
-            if not parse:
+            if not parse or parse['course_id'] is None:
                 raise ValueError('Could not parse "%s" as a course_id' % course_id)
-            self.set_course_id(parse['id'])
+            self.set_course_id(parse['course_id'])
             rev = parse['branch']
             if rev:
                 self.set_branch(rev)
@@ -396,11 +416,12 @@ class BlockUsageLocator(CourseLocator):
             self.init_block_ref_from_course_id(course_id)
         if usage_id:
             self.init_block_ref(usage_id)
-        CourseLocator.__init__(self,
+        super(BlockUsageLocator, self).__init__(
             url=url,
             version_guid=version_guid,
             course_id=course_id,
-                               branch=branch)
+            branch=branch
+        )
 
     def is_initialized(self):
         """
@@ -459,10 +480,12 @@ class BlockUsageLocator(CourseLocator):
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
+            # FIXME the parsed course_id should never contain a block ref
             course_id = course_id.course_id
             assert course_id, "%s does not have a valid course_id"
         parse = parse_course_id(course_id)
-        assert parse, 'Could not parse "%s" as a course_id' % course_id
+        if parse is None:
+            raise ValueError('Could not parse "%s" as a course_id' % course_id)
         self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def __unicode__(self):
@@ -470,7 +493,7 @@ class BlockUsageLocator(CourseLocator):
         Return a string representing this location.
         """
         rep = super(BlockUsageLocator, self).__unicode__()
-        return rep + BLOCK_PREFIX + unicode(self.usage_id)
+        return rep + '/' + BLOCK_PREFIX + unicode(self.usage_id)
 
 
 class DefinitionLocator(Locator):
@@ -478,7 +501,7 @@ class DefinitionLocator(Locator):
     Container for how to locate a description (the course-independent content).
     """
 
-    URL_RE = re.compile(r'^defx://' + URL_VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
+    URL_RE = re.compile(r'^defx://' + VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
     def __init__(self, definition_id):
         if isinstance(definition_id, basestring):
             regex_match = self.URL_RE.match(definition_id)
@@ -491,7 +514,7 @@ class DefinitionLocator(Locator):
         Return a string representing this location.
         unicode(self) returns something like this: "version/519665f6223ebd6980884f2b"
         '''
-        return URL_VERSION_PREFIX + str(self.definition_id)
+        return VERSION_PREFIX + str(self.definition_id)
 
     def url(self):
         """

common/lib/xmodule/xmodule/modulestore/parsers.py

 import re
 
 # Prefix for the branch portion of a locator URL
-BRANCH_PREFIX = "/branch/"
+BRANCH_PREFIX = r"branch/"
 # Prefix for the block portion of a locator URL
-BLOCK_PREFIX = "/block/"
+BLOCK_PREFIX = r"block/"
 # Prefix for the version portion of a locator URL, when it is preceded by a course ID
-VERSION_PREFIX = "/version/"
-# Prefix for version when it begins the URL (no course ID).
-URL_VERSION_PREFIX = 'version/'
+VERSION_PREFIX = r"version/"
 
-URL_RE = re.compile(r'^(edx://)?(.+)$', re.IGNORECASE)
 ALLOWED_ID_CHARS = r'[a-zA-Z0-9_\-~.]'
 
+URL_RE_SOURCE = r"""
+    (?P<tag>edx://)?
+    ((?P<course_id>{ALLOWED_ID_CHARS}+)/?)?
+    ({BRANCH_PREFIX}(?P<branch>{ALLOWED_ID_CHARS}+)/?)?
+    ({VERSION_PREFIX}(?P<version_guid>[A-F0-9]+)/?)?
+    ({BLOCK_PREFIX}(?P<block>{ALLOWED_ID_CHARS}+))?
+    """.format(
+        ALLOWED_ID_CHARS=ALLOWED_ID_CHARS, BRANCH_PREFIX=BRANCH_PREFIX,
+        VERSION_PREFIX=VERSION_PREFIX, BLOCK_PREFIX=BLOCK_PREFIX
+    )
+
+URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE)
+
+
 def parse_url(string, tag_optional=False):
     """
     A url usually begins with 'edx://' (case-insensitive match),
@@ -21,9 +32,9 @@ def parse_url(string, tag_optional=False):
     Examples:
         'edx://version/0123FFFF'
         'edx://mit.eecs.6002x'
-        'edx://mit.eecs.6002x;published'
-        'edx://mit.eecs.6002x;published/block/HW3'
-        'edx://mit.eecs.6002x;published/version/000eee12345/block/HW3'
+        'edx://mit.eecs.6002x/branch/published'
+        'edx://mit.eecs.6002x/branch/published/block/HW3'
+        'edx://mit.eecs.6002x/branch/published/version/000eee12345/block/HW3'
 
     This returns None if string cannot be parsed.
 
@@ -37,12 +48,10 @@ def parse_url(string, tag_optional=False):
     match = URL_RE.match(string)
     if not match:
         return None
-    if match.group(1) is None and not tag_optional:
+    matched_dict = match.groupdict()
+    if matched_dict['tag'] is None and not tag_optional:
         return None
-    path = match.group(2)
-    if path.startswith(URL_VERSION_PREFIX):
-        return parse_guid(path[len(URL_VERSION_PREFIX):])
-    return parse_course_id(path)
+    return matched_dict
 
 
 BLOCK_RE = re.compile(r'^' + ALLOWED_ID_CHARS + r'+$', re.IGNORECASE)
@@ -60,34 +69,6 @@ def parse_block_ref(string):
     return None
 
 
-GUID_RE = re.compile(
-    r'^(?P<version_guid>[A-F0-9]+)(' + BLOCK_PREFIX + '(?P<block>' + ALLOWED_ID_CHARS + r'+))?$',
-    re.IGNORECASE
-)
-
-
-def parse_guid(string):
-    """
-    A version_guid is a string of hex digits (0-F).
-
-    If string is a version_guid, returns a dict with key 'version_guid' and the value,
-    otherwise returns None.
-    """
-    m = GUID_RE.match(string)
-    if m is not None:
-        return m.groupdict()
-    else:
-        return None
-
-
-COURSE_ID_RE = re.compile(
-    r'^(?P<id>' + ALLOWED_ID_CHARS + r'+)(' +
-    BRANCH_PREFIX + r'(?P<branch>' + ALLOWED_ID_CHARS + r'+))?(' +
-    VERSION_PREFIX + r'(?P<version_guid>[A-F0-9]+))?(' +
-    BLOCK_PREFIX + r'(?P<block>' + ALLOWED_ID_CHARS + r'+))?$', re.IGNORECASE
-)
-
-
 def parse_course_id(string):
     r"""
 
@@ -122,7 +103,7 @@ def parse_course_id(string):
     Block is optional: if missing returned_dict['block'] is None.
     Else returns None.
     """
-    match = COURSE_ID_RE.match(string)
+    match = URL_RE.match(string)
     if not match:
         return None
     return match.groupdict()

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -234,7 +234,7 @@ class SplitMongoModuleStore(ModuleStoreBase):
             version_guid = index['versions'][course_locator.branch]
             if course_locator.version_guid is not None and version_guid != course_locator.version_guid:
                 # This may be a bit too touchy but it's hard to infer intent
-                raise VersionConflictError(course_locator, CourseLocator(course_locator, version_guid=version_guid))
+                raise VersionConflictError(course_locator, version_guid)
         else:
             # TODO should this raise an exception if branch was provided?
             version_guid = course_locator.version_guid
@@ -1005,7 +1005,14 @@ class SplitMongoModuleStore(ModuleStoreBase):
                 self._update_head(index_entry, xblock.location.branch, new_id)
 
             # fetch and return the new item--fetching is unnecessary but a good qc step
-            return self.get_item(BlockUsageLocator(xblock.location, version_guid=new_id))
+            return self.get_item(
+                BlockUsageLocator(
+                    course_id=xblock.location.course_id,
+                    usage_id=xblock.location.usage_id,
+                    branch=xblock.location.branch,
+                    version_guid=new_id
+                )
+            )
         else:
             return xblock
 
@@ -1364,10 +1371,8 @@ class SplitMongoModuleStore(ModuleStoreBase):
             else:
                 raise VersionConflictError(
                     locator,
-                    CourseLocator(
-                        course_id=index_entry['_id'],
-                        version_guid=index_entry['versions'][locator.branch],
-                        branch=locator.branch))
+                    index_entry['versions'][locator.branch]
+                )
 
     def _version_structure(self, structure, user_id):
         """

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

@@ -247,7 +247,8 @@ class TestLocationMapper(unittest.TestCase):
         new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course)
         prob_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='problem2'
+            usage_id='problem2',
+            branch='published'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertIsNone(prob_location, 'found entry in empty map table')
@@ -265,7 +266,9 @@ class TestLocationMapper(unittest.TestCase):
         # default branch
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None))
         # explicit branch
-        prob_locator = BlockUsageLocator(prob_locator, branch='draft')
+        prob_locator = BlockUsageLocator(
+            course_id=prob_locator.course_id, branch='draft', usage_id=prob_locator.usage_id
+        )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', 'draft'))
         prob_locator = BlockUsageLocator(
@@ -276,12 +279,13 @@ class TestLocationMapper(unittest.TestCase):
         # same for chapter except chapter cannot be draft in old system
         chap_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='chapter48f'
+            usage_id='chapter48f',
+            branch='production'
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
         # explicit branch
-        chap_locator = BlockUsageLocator(chap_locator, branch='draft')
+        chap_locator.branch = 'draft'
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
         chap_locator = BlockUsageLocator(
@@ -382,7 +386,7 @@ class TestLocationMapper(unittest.TestCase):
         self.assertEqual(new_usage_id, new_usage_id2)
         # it should be in the distractor now
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id2)
+            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id2, branch='published')
         )
         self.assertEqual(new_location, location)
         # add one close to the existing chapter (cause name collision)
@@ -391,11 +395,15 @@ class TestLocationMapper(unittest.TestCase):
         self.assertRegexpMatches(new_usage_id, r'^chapter48f\d')
         # retrievable from both courses
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id)
+            BlockUsageLocator(course_id=new_style_course_id, usage_id=new_usage_id, branch='published')
         )
         self.assertEqual(new_location, location)
         new_location = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id='{}.{}.{}'.format(org, course, 'baz_run'), usage_id=new_usage_id)
+            BlockUsageLocator(
+                course_id='{}.{}.{}'.format(org, course, 'baz_run'),
+                usage_id=new_usage_id,
+                branch='published'
+            )
         )
         self.assertEqual(new_location, location)
 
@@ -443,11 +451,11 @@ class TestLocationMapper(unittest.TestCase):
         # change in all courses to same value
         loc_mapper().update_block_location_translator(location, 'problem1')
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1', branch='published')
         )
         self.assertEqual(trans_loc, location)
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem1', branch='published')
         )
         self.assertEqual(trans_loc, location)
         # try to change to overwrite used usage_id
@@ -457,12 +465,12 @@ class TestLocationMapper(unittest.TestCase):
         # just change the one course
         loc_mapper().update_block_location_translator(location, 'chapter2', '{}/{}/{}'.format(org, course, 'baz_run'))
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='chapter2')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='chapter2', branch='published')
         )
         self.assertEqual(trans_loc.name, '48f23a10395384929234')
         # but this still points to the old
         trans_loc = loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='chapter2')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='chapter2', branch='published')
         )
         self.assertEqual(trans_loc.name, '1')
 
@@ -496,10 +504,10 @@ class TestLocationMapper(unittest.TestCase):
         # delete from all courses
         loc_mapper().delete_block_location_translator(location)
         self.assertIsNone(loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1')
+            BlockUsageLocator(course_id=new_style_course_id, usage_id='problem1', branch='published')
         ))
         self.assertIsNone(loc_mapper().translate_locator_to_location(
-            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem2')
+            BlockUsageLocator(course_id=new_style_course_id2, usage_id='problem2', branch='published')
         ))
         # delete from one course
         location = location.replace(name='abc123')