diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 591ef3115f..bdc9e61fce 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -1,8 +1,7 @@ """ -Created on Mar 13, 2013 +Identifier for course resources. +""" -@author: dmitchell -""" from __future__ import absolute_import import logging import inspect @@ -15,6 +14,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 log = logging.getLogger(__name__) @@ -37,9 +37,6 @@ class Locator(object): """ raise InsufficientSpecificationError() - def quoted_url(self): - return quote(self.url(), '@;#') - def __eq__(self, other): return self.__dict__ == other.__dict__ @@ -90,11 +87,12 @@ class CourseLocator(Locator): Examples of valid CourseLocator specifications: CourseLocator(version_guid=ObjectId('519665f6223ebd6980884f2b')) CourseLocator(course_id='mit.eecs.6002x') - CourseLocator(course_id='mit.eecs.6002x;published') + CourseLocator(course_id='mit.eecs.6002x/branch/published') CourseLocator(course_id='mit.eecs.6002x', branch='published') - CourseLocator(url='edx://@519665f6223ebd6980884f2b') + CourseLocator(url='edx://version/519665f6223ebd6980884f2b') CourseLocator(url='edx://mit.eecs.6002x') - CourseLocator(url='edx://mit.eecs.6002x;published') + 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', @@ -115,10 +113,10 @@ class CourseLocator(Locator): if self.course_id: result = self.course_id if self.branch: - result += ';' + self.branch + result += BRANCH_PREFIX + self.branch return result elif self.version_guid: - return '@' + str(self.version_guid) + return URL_VERSION_PREFIX + str(self.version_guid) else: # raise InsufficientSpecificationError("missing course_id or version_guid") return '' @@ -223,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 ('#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): """ @@ -253,14 +248,14 @@ class CourseLocator(Locator): def init_from_course_id(self, course_id, explicit_branch=None): """ - Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x;published'. + Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. Revision (optional) is a string like 'published'. It may be provided explicitly (explicit_branch) or embedded into course_id. - If branch is part of course_id ("...;published"), parse it out separately. + If branch is part of course_id (".../branch/published"), parse it out separately. If branch is provided both ways, that's ok as long as they are the same value. - If a block ('#HW3') is a part of course_id, it is ignored. + If a block ('/block/HW3') is a part of course_id, it is ignored. """ @@ -295,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): """ @@ -390,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): @@ -400,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): """ @@ -411,14 +412,14 @@ class BlockUsageLocator(CourseLocator): rep = CourseLocator.__unicode__(self) if self.usage_id is None: # usage_id has not been initialized - return rep + '#NONE' + return rep + BLOCK_PREFIX + 'NONE' else: - return rep + '#' + self.usage_id + return rep + BLOCK_PREFIX + self.usage_id 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): @@ -427,14 +428,14 @@ class DescriptionLocator(Locator): def __unicode__(self): ''' Return a string representing this location. - unicode(self) returns something like this: "@519665f6223ebd6980884f2b" + unicode(self) returns something like this: "version/519665f6223ebd6980884f2b" ''' - return '@' + str(self.definition_guid) + return URL_VERSION_PREFIX + str(self.definition_id) def url(self): """ Return a string containing the URL for this location. - url(self) returns something like this: 'edx://@519665f6223ebd6980884f2b' + url(self) returns something like this: 'edx://version/519665f6223ebd6980884f2b' """ return 'edx://' + unicode(self) @@ -442,7 +443,7 @@ class DescriptionLocator(Locator): """ Returns the ObjectId referencing this specific location. """ - return self.definition_guid + return self.definition_id class VersionTree(object): diff --git a/common/lib/xmodule/xmodule/modulestore/parsers.py b/common/lib/xmodule/xmodule/modulestore/parsers.py index 8e5b685cec..9308894b86 100644 --- a/common/lib/xmodule/xmodule/modulestore/parsers.py +++ b/common/lib/xmodule/xmodule/modulestore/parsers.py @@ -1,5 +1,14 @@ import re +# Prefix for the branch portion of a locator URL +BRANCH_PREFIX = "/branch/" +# Prefix for the block portion of a locator URL +BLOCK_PREFIX = "/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/' + URL_RE = re.compile(r'^edx://(.+)$', re.IGNORECASE) @@ -9,26 +18,27 @@ def parse_url(string): followed by either a version_guid or a course_id. Examples: - 'edx://@0123FFFF' + 'edx://version/0123FFFF' 'edx://edu.mit.eecs.6002x' - 'edx://edu.mit.eecs.6002x;published' - 'edx://edu.mit.eecs.6002x;published#HW3' + '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) if not match: return None path = match.group(1) - if path[0] == '@': - return parse_guid(path[1:]) + if path.startswith(URL_VERSION_PREFIX): + return parse_guid(path[len(URL_VERSION_PREFIX):]) return parse_course_id(path) @@ -52,7 +62,7 @@ def parse_block_ref(string): return None -GUID_RE = re.compile(r'^(?P[A-F0-9]+)(#(?P\w+))?$', re.IGNORECASE) +GUID_RE = re.compile(r'^(?P[A-F0-9]+)(' + BLOCK_PREFIX + '(?P\w+))?$', re.IGNORECASE) def parse_guid(string): @@ -69,27 +79,34 @@ def parse_guid(string): return None -COURSE_ID_RE = re.compile(r'^(?P(\w+)(\.\w+\w*)*)(;(?P\w+))?(#(?P\w+))?$', re.IGNORECASE) +COURSE_ID_RE = re.compile( + r'^(?P(\w+)(\.\w+\w*)*)(' + + BRANCH_PREFIX + '(?P\w+))?(' + + VERSION_PREFIX + '(?P[A-F0-9]+))?(' + + BLOCK_PREFIX + '(?P\w+))?$', re.IGNORECASE +) def parse_course_id(string): r""" A course_id has a main id component. - There may also be an optional branch (;published or ;draft). - There may also be an optional block (#HW3 or #Quiz2). + 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: 'edu.mit.eecs.6002x' - 'edu.mit.eecs.6002x;published' - 'edu.mit.eecs.6002x#HW3' - 'edu.mit.eecs.6002x;published#HW3' + '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] [# block] + course_id = main_id [/branch/ branch] [/version/ version ] [/block/ block] main_id = name [. name]* diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index bb41131234..654de26db4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -1,16 +1,18 @@ -''' -Created on Mar 14, 2013 - -@author: dmitchell -''' +""" +Tests for xmodule.modulestore.locator. +""" from unittest import TestCase from bson.objectid import ObjectId -from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator +from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DescriptionLocator +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) @@ -32,12 +34,12 @@ class LocatorTest(TestCase): self.assertRaises( OverSpecificationError, CourseLocator, - url='edx://mit.eecs.6002x;published', + url='edx://mit.eecs.6002x' + BRANCH_PREFIX + 'published', branch='draft') self.assertRaises( OverSpecificationError, CourseLocator, - course_id='mit.eecs.6002x;published', + course_id='mit.eecs.6002x' + BRANCH_PREFIX + 'published', branch='draft') def test_course_constructor_underspecified(self): @@ -55,8 +57,8 @@ class LocatorTest(TestCase): testobj_1 = CourseLocator(version_guid=test_id_1) self.check_course_locn_fields(testobj_1, 'version_guid', version_guid=test_id_1) self.assertEqual(str(testobj_1.version_guid), test_id_1_loc) - self.assertEqual(str(testobj_1), '@' + test_id_1_loc) - self.assertEqual(testobj_1.url(), 'edx://@' + test_id_1_loc) + self.assertEqual(str(testobj_1), URL_VERSION_PREFIX + test_id_1_loc) + self.assertEqual(testobj_1.url(), 'edx://' + URL_VERSION_PREFIX + test_id_1_loc) # Test using a given string test_id_2_loc = '519665f6223ebd6980884f2b' @@ -64,8 +66,8 @@ class LocatorTest(TestCase): testobj_2 = CourseLocator(version_guid=test_id_2) 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), '@' + test_id_2_loc) - self.assertEqual(testobj_2.url(), 'edx://@' + 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) def test_course_constructor_bad_course_id(self): """ @@ -74,20 +76,20 @@ class LocatorTest(TestCase): for bad_id in ('mit.', ' mit.eecs', 'mit.eecs ', - '@mit.eecs', - '#mit.eecs', + URL_VERSION_PREFIX + 'mit.eecs', + BLOCK_PREFIX + 'block/mit.eecs', 'mit.ee cs', 'mit.ee,cs', 'mit.ee/cs', 'mit.ee$cs', 'mit.ee&cs', 'mit.ee()cs', - ';this', - 'mit.eecs;', - 'mit.eecs;this;that', - 'mit.eecs;this;', - 'mit.eecs;this ', - 'mit.eecs;th%is ', + 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 ', + 'mit.eecs' + BRANCH_PREFIX + 'th%is ', ): self.assertRaises(AssertionError, CourseLocator, course_id=bad_id) self.assertRaises(AssertionError, CourseLocator, url='edx://' + bad_id) @@ -106,7 +108,7 @@ class LocatorTest(TestCase): self.check_course_locn_fields(testobj, 'course_id', course_id=testurn) def test_course_constructor_redundant_002(self): - testurn = 'mit.eecs.6002x;published' + testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' expected_urn = 'mit.eecs.6002x' expected_rev = 'published' testobj = CourseLocator(course_id=testurn, url='edx://' + testurn) @@ -114,6 +116,32 @@ class LocatorTest(TestCase): course_id=expected_urn, branch=expected_rev) + def test_course_constructor_url(self): + # Test parsing a url when it starts with a version ID and there is also a block ID. + # This hits the parsers parse_guid method. + test_id_loc = '519665f6223ebd6980884f2b' + testobj = CourseLocator(url="edx://" + URL_VERSION_PREFIX + test_id_loc + BLOCK_PREFIX + "hw3") + self.check_course_locn_fields( + testobj, + 'test_block constructor', + 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) @@ -123,7 +151,7 @@ class LocatorTest(TestCase): self.assertEqual(testobj.url(), 'edx://' + testurn) def test_course_constructor_course_id_with_branch(self): - testurn = 'mit.eecs.6002x;published' + testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' expected_id = 'mit.eecs.6002x' expected_branch = 'published' testobj = CourseLocator(course_id=testurn) @@ -139,7 +167,7 @@ class LocatorTest(TestCase): def test_course_constructor_course_id_separate_branch(self): test_id = 'mit.eecs.6002x' test_branch = 'published' - expected_urn = 'mit.eecs.6002x;published' + expected_urn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' testobj = CourseLocator(course_id=test_id, branch=test_branch) self.check_course_locn_fields(testobj, 'course_id with separate branch', course_id=test_id, @@ -154,10 +182,10 @@ class LocatorTest(TestCase): """ The same branch appears in the course_id and the branch field. """ - test_id = 'mit.eecs.6002x;published' + test_id = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' test_branch = 'published' expected_id = 'mit.eecs.6002x' - expected_urn = 'mit.eecs.6002x;published' + expected_urn = test_id testobj = CourseLocator(course_id=test_id, branch=test_branch) self.check_course_locn_fields(testobj, 'course_id with repeated branch', course_id=expected_id, @@ -169,7 +197,7 @@ class LocatorTest(TestCase): self.assertEqual(testobj.url(), 'edx://' + expected_urn) def test_block_constructor(self): - testurn = 'mit.eecs.6002x;published#HW3' + testurn = 'mit.eecs.6002x' + BRANCH_PREFIX + 'published' + BLOCK_PREFIX + 'HW3' expected_id = 'mit.eecs.6002x' expected_branch = 'published' expected_block_ref = 'HW3' @@ -181,6 +209,43 @@ 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") + self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url()) + + def test_description_locator_version(self): + definition_locator = DescriptionLocator("chapter12345_2") + self.assertEqual("chapter12345_2", definition_locator.version()) # ------------------------------------------------------------------ # Utilities