Allow version ID to appear after course ID.

cleanup

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

@@ -92,6 +92,7 @@ class CourseLocator(Locator):
      CourseLocator(url='edx://version/519665f6223ebd6980884f2b')
      CourseLocator(url='edx://mit.eecs.6002x')
      CourseLocator(url='edx://mit.eecs.6002x/branch/published')
+     CourseLocator(url='edx://mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b')
 
     Should have at lease a specific course_id (id for the course as if it were a project w/
     versions) with optional 'branch',
@@ -220,21 +221,18 @@ class CourseLocator(Locator):
     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)
-        If a block ('/block/HW3') is present, it is ignored.
+        either a valid version_guid or course_id (with optional branch), or both.
         """
         if isinstance(url, Locator):
             url = url.url()
-        assert isinstance(url, basestring), \
-            '%s is not an instance of basestring' % url
+        assert isinstance(url, basestring), '%s is not an instance of basestring' % url
         parse = parse_url(url)
         assert parse, 'Could not parse "%s" as a url' % url
-        if 'version_guid' in parse:
-            new_guid = parse['version_guid']
-            self.set_version_guid(self.as_object_id(new_guid))
-        else:
-            self.set_course_id(parse['id'])
-            self.set_branch(parse['branch'])
+        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, 'branch', lambda (new_branch): self.set_branch(new_branch))
 
     def init_from_version_guid(self, version_guid):
         """
@@ -292,6 +290,16 @@ class CourseLocator(Locator):
         """
         return self.course_id
 
+    def _set_value(self, parse, key, setter):
+        """
+        Helper method that gets a value out of the dict returned by parse,
+        and then sets the corresponding bit of information in this locator
+        (via the supplied lambda 'setter'), unless the value is None.
+        """
+        value = parse.get(key, None)
+        if value:
+            setter(value)
+
 
 class BlockUsageLocator(CourseLocator):
     """
@@ -387,9 +395,7 @@ class BlockUsageLocator(CourseLocator):
             url = url.url()
         parse = parse_url(url)
         assert parse, 'Could not parse "%s" as a url' % url
-        block = parse.get('block', None)
-        if block:
-            self.set_usage_id(block)
+        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
@@ -397,9 +403,7 @@ class BlockUsageLocator(CourseLocator):
             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
-        block = parse.get('block', None)
-        if block:
-            self.set_usage_id(block)
+        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def __unicode__(self):
         """
@@ -415,7 +419,7 @@ class BlockUsageLocator(CourseLocator):
 
 class DescriptionLocator(Locator):
     """
-    Container for how to locate a description
+    Container for how to locate a description (the course-independent content).
     """
 
     def __init__(self, definition_id):

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

@@ -4,7 +4,9 @@ import re
 BRANCH_PREFIX = "/branch/"
 # Prefix for the block portion of a locator URL
 BLOCK_PREFIX = "/block/"
-# Prefix for when a course URL begins with a version ID
+# 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/'
 
 URL_RE = re.compile(r'^edx://(.+)$', re.IGNORECASE)
@@ -19,15 +21,16 @@ def parse_url(string):
         'edx://version/0123FFFF'
         'edx://edu.mit.eecs.6002x'
         'edx://edu.mit.eecs.6002x/branch/published'
+        'edx://edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3'
         'edx://edu.mit.eecs.6002x/branch/published/block/HW3'
 
     This returns None if string cannot be parsed.
 
-    If it can be parsed as a version_guid, returns a dict
+    If it can be parsed as a version_guid with no preceding course_id, returns a dict
     with key 'version_guid' and the value,
 
     If it can be parsed as a course_id, returns a dict
-    with keys 'id' and 'branch' (value of 'branch' may be None),
+    with key 'id' and optional keys 'branch' and 'version_guid'.
 
     """
     match = URL_RE.match(string)
@@ -61,6 +64,7 @@ def parse_block_ref(string):
 
 GUID_RE = re.compile(r'^(?P<version_guid>[A-F0-9]+)(' + BLOCK_PREFIX + '(?P<block>\w+))?$', re.IGNORECASE)
 
+
 def parse_guid(string):
     """
     A version_guid is a string of hex digits (0-F).
@@ -75,7 +79,12 @@ def parse_guid(string):
         return None
 
 
-COURSE_ID_RE = re.compile(r'^(?P<id>(\w+)(\.\w+\w*)*)('+ BRANCH_PREFIX + '(?P<branch>\w+))?(' + BLOCK_PREFIX + '(?P<block>\w+))?$', re.IGNORECASE)
+COURSE_ID_RE = re.compile(
+    r'^(?P<id>(\w+)(\.\w+\w*)*)(' +
+    BRANCH_PREFIX + '(?P<branch>\w+))?(' +
+    VERSION_PREFIX + '(?P<version_guid>[A-F0-9]+))?(' +
+    BLOCK_PREFIX + '(?P<block>\w+))?$', re.IGNORECASE
+)
 
 
 def parse_course_id(string):
@@ -83,6 +92,7 @@ def parse_course_id(string):
 
     A course_id has a main id component.
     There may also be an optional branch (/branch/published or /branch/draft).
+    There may also be an optional version (/version/519665f6223ebd6980884f2b).
     There may also be an optional block (/block/HW3 or /block/Quiz2).
 
     Examples of valid course_ids:
@@ -91,11 +101,12 @@ def parse_course_id(string):
       'edu.mit.eecs.6002x/branch/published'
       'edu.mit.eecs.6002x/block/HW3'
       'edu.mit.eecs.6002x/branch/published/block/HW3'
+      'edu.mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b/block/HW3'
 
 
     Syntax:
 
-      course_id = main_id [/branch/ branch] [/block/ block]
+      course_id = main_id [/branch/ branch] [/version/ version ] [/block/ block]
 
       main_id = name [. name]*
 

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

@@ -5,11 +5,14 @@ from unittest import TestCase
 
 from bson.objectid import ObjectId
 from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DescriptionLocator
-from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, URL_VERSION_PREFIX
+from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX, URL_VERSION_PREFIX
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError
 
 
 class LocatorTest(TestCase):
+    """
+    Tests for subclasses of Locator.
+    """
 
     def test_cant_instantiate_abstract_class(self):
         self.assertRaises(TypeError, Locator)
@@ -64,7 +67,7 @@ class LocatorTest(TestCase):
         self.check_course_locn_fields(testobj_2, 'version_guid', version_guid=test_id_2)
         self.assertEqual(str(testobj_2.version_guid), test_id_2_loc)
         self.assertEqual(str(testobj_2), URL_VERSION_PREFIX + test_id_2_loc)
-        self.assertEqual(testobj_2.url(), 'edx://'+ URL_VERSION_PREFIX + test_id_2_loc)
+        self.assertEqual(testobj_2.url(), 'edx://' + URL_VERSION_PREFIX + test_id_2_loc)
 
     def test_course_constructor_bad_course_id(self):
         """
@@ -83,8 +86,8 @@ class LocatorTest(TestCase):
                        'mit.ee()cs',
                        BRANCH_PREFIX + 'this',
                        'mit.eecs' + BRANCH_PREFIX,
-                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX +'that',
-                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX ,
+                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX + 'that',
+                       'mit.eecs' + BRANCH_PREFIX + 'this' + BRANCH_PREFIX,
                        'mit.eecs' + BRANCH_PREFIX + 'this ',
                        'mit.eecs' + BRANCH_PREFIX + 'th%is ',
                        ):
@@ -124,6 +127,21 @@ class LocatorTest(TestCase):
             version_guid=ObjectId(test_id_loc)
         )
 
+    def test_course_constructor_url_course_id_and_version_guid(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = CourseLocator(url='edx://mit.eecs.6002x' + VERSION_PREFIX + test_id_loc)
+        self.check_course_locn_fields(testobj, 'error parsing url with both course ID and version GUID',
+                                      course_id='mit.eecs.6002x',
+                                      version_guid=ObjectId(test_id_loc))
+
+    def test_course_constructor_url_course_id_branch_and_version_guid(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = CourseLocator(url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'draft' + VERSION_PREFIX + test_id_loc)
+        self.check_course_locn_fields(testobj, 'error parsing url with both course ID branch, and version GUID',
+                                      course_id='mit.eecs.6002x',
+                                      branch='draft',
+                                      version_guid=ObjectId(test_id_loc))
+
     def test_course_constructor_course_id_no_branch(self):
         testurn = 'mit.eecs.6002x'
         testobj = CourseLocator(course_id=testurn)
@@ -191,17 +209,42 @@ class LocatorTest(TestCase):
         self.assertEqual(str(testobj), testurn)
         self.assertEqual(testobj.url(), 'edx://' + testurn)
 
+    def test_block_constructor_url_version_prefix(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = BlockUsageLocator(
+            url='edx://mit.eecs.6002x' + VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + 'lab2'
+        )
+        self.check_block_locn_fields(
+            testobj, 'error parsing URL with version and block',
+            course_id='mit.eecs.6002x',
+            block='lab2',
+            version_guid=ObjectId(test_id_loc)
+        )
+
+    def test_block_constructor_url_kitchen_sink(self):
+        test_id_loc = '519665f6223ebd6980884f2b'
+        testobj = BlockUsageLocator(
+            url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'draft' + VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + 'lab2'
+        )
+        self.check_block_locn_fields(
+            testobj, 'error parsing URL with branch, version, and block',
+            course_id='mit.eecs.6002x',
+            branch='draft',
+            block='lab2',
+            version_guid=ObjectId(test_id_loc)
+        )
+
     def test_repr(self):
         testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' + BLOCK_PREFIX + 'HW3'
         testobj = BlockUsageLocator(course_id=testurn)
         self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj))
 
     def test_description_locator_url(self):
-        definition_locator=DescriptionLocator("chapter12345_2")
+        definition_locator = DescriptionLocator("chapter12345_2")
         self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url())
 
     def test_description_locator_version(self):
-        definition_locator=DescriptionLocator("chapter12345_2")
+        definition_locator = DescriptionLocator("chapter12345_2")
         self.assertEqual("chapter12345_2", definition_locator.version())
 
     # ------------------------------------------------------------------