Merge remote-tracking branch 'edx/opaque-keys' into opaque-keys-merge-master

cms/djangoapps/contentstore/tests/test_crud.py

@@ -12,7 +12,6 @@ from xmodule.html_module import HtmlDescriptor
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
-@unittest.skip("Not fixing split until we land opaque-keys 0.9")
 class TemplateTests(unittest.TestCase):
     """
     Test finding and using the templates (boilerplates) for xblocks.
@@ -55,25 +54,25 @@ class TemplateTests(unittest.TestCase):
 
     def test_factories(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx',
+            offering='tempcourse', org='testx',
             display_name='fun test course', user_id='testbot'
         )
         self.assertIsInstance(test_course, CourseDescriptor)
         self.assertEqual(test_course.display_name, 'fun test course')
-        index_info = modulestore('split').get_course_index_info(test_course.location)
+        index_info = modulestore('split').get_course_index_info(test_course.id)
         self.assertEqual(index_info['org'], 'testx')
-        self.assertEqual(index_info['_id'], 'testx.tempcourse')
+        self.assertEqual(index_info['offering'], 'tempcourse')
 
         test_chapter = persistent_factories.ItemFactory.create(display_name='chapter 1',
             parent_location=test_course.location)
         self.assertIsInstance(test_chapter, SequenceDescriptor)
         # refetch parent which should now point to child
-        test_course = modulestore('split').get_course(test_course.id)
+        test_course = modulestore('split').get_course(test_course.id.version_agnostic())
         self.assertIn(test_chapter.location.block_id, test_course.children)
 
         with self.assertRaises(DuplicateCourseError):
             persistent_factories.PersistentCourseFactory.create(
-                course_id='testx.tempcourse', org='testx', 
+                offering='tempcourse', org='testx',
                 display_name='fun test course', user_id='testbot'
             )
 
@@ -82,7 +81,7 @@ class TemplateTests(unittest.TestCase):
         Test create_xblock to create non persisted xblocks
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx',
+            offering='tempcourse', org='testx',
             display_name='fun test course', user_id='testbot'
         )
 
@@ -109,7 +108,7 @@ class TemplateTests(unittest.TestCase):
         try saving temporary xblocks
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='testx.tempcourse', org='testx', 
+            offering='tempcourse', org='testx',
             display_name='fun test course', user_id='testbot'
         )
         test_chapter = modulestore('split').create_xblock(
@@ -148,7 +147,7 @@ class TemplateTests(unittest.TestCase):
 
     def test_delete_course(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='edu.harvard.history.doomed', org='testx',
+            offering='history.doomed', org='edu.harvard',
             display_name='doomed test course',
             user_id='testbot')
         persistent_factories.ItemFactory.create(display_name='chapter 1',
@@ -171,7 +170,7 @@ class TemplateTests(unittest.TestCase):
         Test get_block_generations
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            course_id='edu.harvard.history.hist101', org='testx',
+            offering='history.hist101', org='edu.harvard',
             display_name='history test course',
             user_id='testbot'
         )
@@ -193,7 +192,9 @@ class TemplateTests(unittest.TestCase):
 
         second_problem = persistent_factories.ItemFactory.create(
             display_name='problem 2',
-            parent_location=BlockUsageLocator.make_relative(updated_loc, block_id=sub.location.block_id),
+            parent_location=BlockUsageLocator.make_relative(
+                updated_loc, block_type='problem', block_id=sub.location.block_id
+            ),
             user_id='testbot', category='problem',
             data="<problem></problem>"
         )

cms/djangoapps/contentstore/views/tests/test_course_index.py

@@ -7,7 +7,7 @@ import lxml
 from contentstore.tests.utils import CourseTestCase
 from contentstore.utils import reverse_course_url
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
-from xmodule.modulestore import parsers
+from xmodule.modulestore.locator import Locator
 
 
 class TestCourseIndex(CourseTestCase):
@@ -38,7 +38,7 @@ class TestCourseIndex(CourseTestCase):
         for link in course_link_eles:
             self.assertRegexpMatches(
                 link.get("href"),
-                'course/slashes:{0}'.format(parsers.ALLOWED_ID_CHARS)
+                'course/slashes:{0}'.format(Locator.ALLOWED_ID_CHARS)
             )
             # now test that url
             outline_response = authed_client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')

common/djangoapps/cache_toolbox/core.py

@@ -118,4 +118,9 @@ def get_cached_content(location):
 
 
 def del_cached_content(location):
-    cache.delete(unicode(location).encode("utf-8"))
+    # delete content for the given location, as well as for content with run=None.
+    # it's possible that the content could have been cached without knowing the
+    # course_key - and so without having the run.
+    cache.delete_many(
+        [unicode(loc).encode("utf-8") for loc in [location, location.replace(run=None)]]
+    )

common/djangoapps/student/migrations/0035_access_roles.py

@@ -9,6 +9,7 @@ from opaque_keys import InvalidKeyError
 import bson.son
 import logging
 from django.db.models.query_utils import Q
+from django.db.utils import IntegrityError
 
 log = logging.getLogger(__name__)
 
@@ -28,69 +29,72 @@ class Migration(DataMigration):
         loc_map_collection = loc_mapper().location_map
         # b/c the Groups table had several entries for each course, we need to ensure we process each unique
         # course only once. The below datastructures help ensure that.
-        hold = {}
-        done = set()
-        orgs = {}
-        query = Q(name__startswith='course_creator_group')
+        hold = {}  # key of course_id_strings with array of group objects. Should only be org scoped entries
+        # or deleted courses
+        orgs = {}  # downcased org to last recorded normal case of the org
+        query = Q(name='course_creator_group')
         for role in ['staff', 'instructor', 'beta_testers', ]:
             query = query | Q(name__startswith=role)
         for group in orm['auth.Group'].objects.filter(query).all():
-            def _migrate_users(correct_course_key, lower_org):
+            def _migrate_users(correct_course_key, role, lower_org):
                 """
                 Get all the users from the old group and migrate to this course key in the new table
                 """
                 for user in orm['auth.user'].objects.filter(groups=group).all():
                     entry = orm['student.courseaccessrole'](
-                        role=parsed_entry.group('role_id'), user=user,
-                        org=correct_course_key.org, course_id=correct_course_key.to_deprecated_string()
+                        role=role, user=user,
+                        org=correct_course_key.org, course_id=correct_course_key
                     )
+                    try:
                         entry.save()
+                    except IntegrityError:
+                        # already stored
+                        pass
                 orgs[lower_org] = correct_course_key.org
-                done.add(correct_course_key)
 
-            # should this actually loop through all groups and log any which are not compliant? That is,
-            # remove the above filter
             parsed_entry = self.GROUP_ENTRY_RE.match(group.name)
-            if parsed_entry.group('role_id') == 'course_creator_group':
+            role = parsed_entry.group('role_id')
+            if role == 'course_creator_group':
                 for user in orm['auth.user'].objects.filter(groups=group).all():
-                    entry = orm['student.courseaccessrole'](role=parsed_entry.group('role_id'), user=user)
+                    entry = orm['student.courseaccessrole'](role=role, user=user)
                     entry.save()
             else:
                 course_id_string = parsed_entry.group('course_id_string')
                 try:
                     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id_string)
-                    if course_key not in done:
-                        # is the downcased version, get the normal cased one. loc_mapper() has no
+                    # course_key is the downcased version, get the normal cased one. loc_mapper() has no
                     # methods taking downcased SSCK; so, need to do it manually here
-                        correct_course_key = self._map_downcased_ssck(course_key, loc_map_collection, done)
-                        _migrate_users(correct_course_key, course_key.org)
-                        done.add(course_key)
+                    correct_course_key = self._map_downcased_ssck(course_key, loc_map_collection)
+                    if correct_course_key is not None:
+                        _migrate_users(correct_course_key, role, course_key.org)
                 except InvalidKeyError:
                     entry = loc_map_collection.find_one({
                         'course_id': re.compile(r'^{}$'.format(course_id_string), re.IGNORECASE)
                     })
                     if entry is None:
-                        # not a course_id as far as we can tell
-                        if course_id_string not in done:
-                            hold[course_id_string] = group
+                        hold.setdefault(course_id_string, []).append(group)
                     else:
-                        correct_course_key = self._cache_done_return_ssck(entry, done)
-                        _migrate_users(correct_course_key, entry['lower_id']['org'])
+                        correct_course_key = SlashSeparatedCourseKey(*entry['_id'].values())
+                        _migrate_users(correct_course_key, role, entry['lower_id']['org'])
 
-        # see if any in hold ere missed above
-        for not_ssck, group in hold.iteritems():
-            if not_ssck not in done:
-                if not_ssck in orgs:
+        # see if any in hold were missed above
+        for held_auth_scope, groups in hold.iteritems():
+            # orgs indexed by downcased org
+            held_auth_scope = held_auth_scope.lower()
+            if held_auth_scope in orgs:
+                for group in groups:
+                    role = self.GROUP_ENTRY_RE.match(group.name).group('role_id')
                     # they have org permission
                     for user in orm['auth.user'].objects.filter(groups=group).all():
                         entry = orm['student.courseaccessrole'](
-                            role=parsed_entry.group('role_id'), user=user,
-                            org=orgs[not_ssck],
+                            role=role,
+                            user=user,
+                            org=orgs[held_auth_scope],
                         )
                         entry.save()
             else:
-                    # should this just log or really make an effort to do the conversion?
-                    log.warn("Didn't convert role %s", group.name)
+                # don't silently skip unexpected roles
+                log.warn("Didn't convert roles %s", [group.name for group in groups])
 
     def backwards(self, orm):
         "Write your backwards methods here."
@@ -98,10 +102,9 @@ class Migration(DataMigration):
         # the semantic of backwards should be other than perhaps clearing the table.
         orm['student.courseaccessrole'].objects.all().delete()
 
-    def _map_downcased_ssck(self, downcased_ssck, loc_map_collection, done):
+    def _map_downcased_ssck(self, downcased_ssck, loc_map_collection):
         """
-        Get the normal cased version of this downcased slash sep course key and add
-        the lowercased locator form to done map
+        Get the normal cased version of this downcased slash sep course key
         """
         # given the regex, the son may be an overkill
         course_son = bson.son.SON([
@@ -111,25 +114,11 @@ class Migration(DataMigration):
         ])
         entry = loc_map_collection.find_one(course_son)
         if entry:
-            return self._cache_done_return_ssck(entry, done)
+            idpart = entry['_id']
+            return SlashSeparatedCourseKey(idpart['org'], idpart['course'], idpart['name'])
         else:
             return None
 
-    def _cache_done_return_ssck(self, entry, done):
-        """
-        Add all the various formats which auth may use to the done set and return the ssck for the entry
-        """
-        # cache that the dotted form is done too
-        if 'lower_course_id' in entry:
-            done.add(entry['lower_course_id'])
-        elif 'course_id' in entry:
-            done.add(entry['course_id'].lower())
-        elif 'lower_org' in entry:
-            done.add('{}.{}'.format(entry['lower_org'], entry['lower_offering']))
-        else:
-            done.add('{}.{}'.format(entry['org'].lower(), entry['offering'].lower()))
-        return SlashSeparatedCourseKey(*entry['_id'].values())
-
 
     models = {
         'auth.group': {

common/lib/xmodule/setup.py

@@ -79,5 +79,8 @@ setup(
             'asset-location = xmodule.modulestore.locations:AssetLocation',
             'edx = xmodule.modulestore.locator:BlockUsageLocator',
         ],
+        'definition_key': [
+            'defx = xmodule.modulestore.locator:DefinitionLocator',
+        ],
     },
 )

common/lib/xmodule/xmodule/modulestore/keys.py

@@ -58,7 +58,7 @@ class DefinitionKey(OpaqueKey):
     KEY_TYPE = 'definition_key'
     __slots__ = ()
 
-    @abstractmethod
+    @abstractproperty
     def block_type(self):
         """
         The XBlock type of this definition.
@@ -125,6 +125,10 @@ class UsageKey(CourseObjectMixin, OpaqueKey):
         """
         raise NotImplementedError()
 
+    @property
+    def block_type(self):
+        return self.category
+
 
 class OpaqueKeyReader(IdReader):
     """

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -150,6 +150,7 @@ class LocMapperStore(object):
             entry = self._migrate_if_necessary([entry])[0]
 
         block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name))
+        category = location.category
         if block_id is None:
             if add_entry_if_missing:
                 block_id = self._add_to_block_map(
@@ -159,14 +160,15 @@ class LocMapperStore(object):
                 raise ItemNotFoundError(location)
         else:
             # jump_to_id uses a None category.
-            if location.category is None:
+            if category is None:
                 if len(block_id) == 1:
                     # unique match (most common case)
+                    category = block_id.keys()[0]
                     block_id = block_id.values()[0]
                 else:
                     raise InvalidLocationError()
-            elif location.category in block_id:
-                block_id = block_id[location.category]
+            elif category in block_id:
+                block_id = block_id[category]
             elif add_entry_if_missing:
                 block_id = self._add_to_block_map(location, course_son, entry['block_map'])
             else:
@@ -179,10 +181,12 @@ class LocMapperStore(object):
         )
         published_usage = BlockUsageLocator(
             prod_course_locator,
+            block_type=category,
             block_id=block_id
         )
         draft_usage = BlockUsageLocator(
             prod_course_locator.for_branch(entry['draft_branch']),
+            block_type=category,
             block_id=block_id
         )
         if published:
@@ -285,6 +289,7 @@ class LocMapperStore(object):
                         org=entry[entry_org], offering=entry[entry_offering],
                         branch=entry['prod_branch']
                     ),
+                    block_type=category,
                     block_id=block_id
                 )
                 draft_locator = BlockUsageLocator(
@@ -292,6 +297,7 @@ class LocMapperStore(object):
                         org=entry[entry_org], offering=entry[entry_offering],
                         branch=entry['draft_branch']
                     ),
+                    block_type=category,
                     block_id=block_id
                 )
                 self._cache_location_map_entry(location, published_locator, draft_locator)

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

@@ -13,15 +13,7 @@ from bson.errors import InvalidId
 
 from opaque_keys import OpaqueKey, InvalidKeyError
 
-from xmodule.modulestore.keys import CourseKey, UsageKey
-
-from xmodule.modulestore.parsers import (
-    parse_url,
-    parse_block_ref,
-    BRANCH_PREFIX,
-    BLOCK_PREFIX,
-    VERSION_PREFIX,
-    ALLOWED_ID_RE)
+from xmodule.modulestore.keys import CourseKey, UsageKey, DefinitionKey
 
 log = logging.getLogger(__name__)
 
@@ -45,14 +37,10 @@ class Locator(OpaqueKey):
     Locator is an abstract base class: do not instantiate
     """
 
-    @abstractmethod
-    def url(self):
-        """
-        Return a string containing the URL for this location. Raises
-        InvalidKeyError if the instance doesn't have a
-        complete enough specification to generate a url
-        """
-        raise NotImplementedError()
+    BLOCK_TYPE_PREFIX = r"type"
+    # Prefix for the version portion of a locator URL, when it is preceded by a course ID
+    VERSION_PREFIX = r"version"
+    ALLOWED_ID_CHARS = r'[\w\-~.:]'
 
     def __str__(self):
         '''
@@ -86,35 +74,42 @@ class BlockLocatorBase(Locator):
     # Token separating org from offering
     ORG_SEPARATOR = '+'
 
-    def version(self):
-        """
-        Returns the ObjectId referencing this specific location.
-        """
-        return self.version_guid
+    # Prefix for the branch portion of a locator URL
+    BRANCH_PREFIX = r"branch"
+    # Prefix for the block portion of a locator URL
+    BLOCK_PREFIX = r"block"
+
+    ALLOWED_ID_RE = re.compile(r'^' + Locator.ALLOWED_ID_CHARS + '+$', re.UNICODE)
+
+    URL_RE_SOURCE = r"""
+        ((?P<org>{ALLOWED_ID_CHARS}+)\+(?P<offering>{ALLOWED_ID_CHARS}+)\+?)??
+        ({BRANCH_PREFIX}\+(?P<branch>{ALLOWED_ID_CHARS}+)\+?)?
+        ({VERSION_PREFIX}\+(?P<version_guid>[A-F0-9]+)\+?)?
+        ({BLOCK_TYPE_PREFIX}\+(?P<block_type>{ALLOWED_ID_CHARS}+)\+?)?
+        ({BLOCK_PREFIX}\+(?P<block_id>{ALLOWED_ID_CHARS}+))?
+        """.format(
+            ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS, BRANCH_PREFIX=BRANCH_PREFIX,
+            VERSION_PREFIX=Locator.VERSION_PREFIX, BLOCK_TYPE_PREFIX=Locator.BLOCK_TYPE_PREFIX, BLOCK_PREFIX=BLOCK_PREFIX
+        )
+
+    URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE)
 
-    def url(self):
-        """
-        Return a string containing the URL for this location.
-        """
-        return self.NAMESPACE_SEPARATOR.join([self.CANONICAL_NAMESPACE, self._to_string()])
 
     @classmethod
-    def _parse_url(cls, url):
-        """
-        url must be a string beginning with 'edx:' and containing
-        either a valid version_guid or org & offering (with optional branch), or both.
+    def parse_url(cls, string):
         """
-        if not isinstance(url, basestring):
-            raise TypeError('%s is not an instance of basestring' % url)
+        Raises InvalidKeyError if string cannot be parsed.
 
-        parse = parse_url(url)
-        if not parse:
-            raise InvalidKeyError(cls, url)
+        If it can be parsed as a version_guid with no preceding org + offering, returns a dict
+        with key 'version_guid' and the value,
 
-        if parse['version_guid']:
-            parse['version_guid'] = cls.as_object_id(parse['version_guid'])
-
-        return parse
+        If it can be parsed as a org + offering, returns a dict
+        with key 'id' and optional keys 'branch' and 'version_guid'.
+        """
+        match = cls.URL_RE.match(string)
+        if not match:
+            raise InvalidKeyError(cls, string)
+        return match.groupdict()
 
     @property
     def package_id(self):
@@ -130,13 +125,10 @@ class CourseLocator(BlockLocatorBase, CourseKey):
      CourseLocator(version_guid=ObjectId('519665f6223ebd6980884f2b'))
      CourseLocator(org='mit.eecs', offering='6.002x')
      CourseLocator(org='mit.eecs', offering='6002x', branch = 'published')
-     CourseLocator.from_string('edx:version/519665f6223ebd6980884f2b')
-     CourseLocator.from_string('version/519665f6223ebd6980884f2b')
-     CourseLocator.from_string('edx:mit.eecs+6002x')
-     CourseLocator.from_string('mit.eecs+6002x')
-     CourseLocator.from_string('edx:mit.eecs+6002x/branch/published')
-     CourseLocator.from_string('edx:mit.eecs+6002x/branch/published/version/519665f6223ebd6980884f2b')
-     CourseLocator.from_string('mit.eecs+6002x/branch/published/version/519665f6223ebd6980884f2b')
+     CourseLocator.from_string('course-locator:version+519665f6223ebd6980884f2b')
+     CourseLocator.from_string('course-locator:mit.eecs+6002x')
+     CourseLocator.from_string('course-locator:mit.eecs+6002x+branch+published')
+     CourseLocator.from_string('course-locator:mit.eecs+6002x+branch+published+version+519665f6223ebd6980884f2b')
 
     Should have at least a specific org & offering (id for the course as if it were a project w/
     versions) with optional 'branch',
@@ -163,7 +155,7 @@ class CourseLocator(BlockLocatorBase, CourseKey):
         if version_guid:
             version_guid = self.as_object_id(version_guid)
 
-        if not all(field is None or ALLOWED_ID_RE.match(field) for field in [org, offering, branch]):
+        if not all(field is None or self.ALLOWED_ID_RE.match(field) for field in [org, offering, branch]):
             raise InvalidKeyError(self.__class__, [org, offering, branch])
 
         super(CourseLocator, self).__init__(
@@ -173,31 +165,27 @@ class CourseLocator(BlockLocatorBase, CourseKey):
             version_guid=version_guid
         )
 
-        if self.version_guid is None and self.org is None and self.offering is None:
+        if self.version_guid is None and (self.org is None or self.offering is None):
             raise InvalidKeyError(self.__class__, "Either version_guid or org and offering should be set")
 
+    def version(self):
+        """
+        Returns the ObjectId referencing this specific location.
+        """
+        return self.version_guid
+
     @classmethod
     def _from_string(cls, serialized):
         """
         Return a CourseLocator parsing the given serialized string
         :param serialized: matches the string to a CourseLocator
         """
-        kwargs = cls._parse_url(serialized)
-        try:
-            return cls(**{key: kwargs.get(key) for key in cls.KEY_FIELDS})
-        except ValueError:
-            raise InvalidKeyError(cls, "Either version_guid or org and offering should be set: {}".format(serialized))
+        parse = cls.parse_url(serialized)
 
-    def is_fully_specified(self):
-        """
-        Returns True if either version_guid is specified, or org+offering+branch
-        are specified.
-        This should always return True, since this should be validated in the constructor.
-        """
-        return (
-            self.version_guid is not None or
-            (self.org is not None and self.offering is not None and self.branch is not None)
-        )
+        if parse['version_guid']:
+            parse['version_guid'] = cls.as_object_id(parse['version_guid'])
+
+        return cls(**{key: parse.get(key) for key in cls.KEY_FIELDS})
 
     def html_id(self):
         """
@@ -212,6 +200,7 @@ class CourseLocator(BlockLocatorBase, CourseKey):
     def make_usage_key(self, block_type, block_id):
         return BlockUsageLocator(
             course_key=self,
+            block_type=block_type,
             block_id=block_id
         )
 
@@ -280,13 +269,13 @@ class CourseLocator(BlockLocatorBase, CourseKey):
         if self.offering:
             parts.append(unicode(self.package_id))
             if self.branch:
-                parts.append(u"{prefix}{branch}".format(prefix=BRANCH_PREFIX, branch=self.branch))
+                parts.append(u"{prefix}+{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch))
         if self.version_guid:
-            parts.append(u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid))
-        return u"/".join(parts)
+            parts.append(u"{prefix}+{guid}".format(prefix=self.VERSION_PREFIX, guid=self.version_guid))
+        return u"+".join(parts)
 
 
-class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey methods
+class BlockUsageLocator(BlockLocatorBase, UsageKey):
     """
     Encodes a location.
 
@@ -305,12 +294,13 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         branch : string
     """
     CANONICAL_NAMESPACE = 'edx'
-    KEY_FIELDS = ('course_key', 'block_id')
+    KEY_FIELDS = ('course_key', 'block_type', 'block_id')
 
     # fake out class instrospection as this is an attr in this class's instances
     course_key = None
+    block_type = None
 
-    def __init__(self, course_key, block_id):
+    def __init__(self, course_key, block_type, block_id):
         """
         Construct a BlockUsageLocator
         """
@@ -318,7 +308,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         if block_id is None:
             raise InvalidKeyError(self.__class__, "Missing block id")
 
-        super(BlockUsageLocator, self).__init__(course_key=course_key, block_id=block_id)
+        super(BlockUsageLocator, self).__init__(course_key=course_key, block_type=block_type, block_id=block_id)
 
     @classmethod
     def _from_string(cls, serialized):
@@ -326,11 +316,11 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         Requests CourseLocator to deserialize its part and then adds the local deserialization of block
         """
         course_key = CourseLocator._from_string(serialized)
-        parsed_parts = parse_url(serialized)
-        block_id = parsed_parts.get('block_id')
+        parsed_parts = cls.parse_url(serialized)
+        block_id = parsed_parts.get('block_id', None)
         if block_id is None:
             raise InvalidKeyError(cls, serialized)
-        return cls(course_key, block_id)
+        return cls(course_key, parsed_parts.get('block_type'), block_id)
 
     def version_agnostic(self):
         """
@@ -342,7 +332,8 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         """
         return BlockUsageLocator(
             course_key=self.course_key.version_agnostic(),
-            block_id=self.block_id
+            block_type=self.block_type,
+            block_id=self.block_id,
         )
 
     def course_agnostic(self):
@@ -354,6 +345,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         """
         return BlockUsageLocator(
             course_key=self.course_key.course_agnostic(),
+            block_type=self.block_type,
             block_id=self.block_id
         )
 
@@ -363,6 +355,17 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         """
         return BlockUsageLocator(
             self.course_key.for_branch(branch),
+            block_type=self.block_type,
+            block_id=self.block_id
+        )
+
+    def for_version(self, version_guid):
+        """
+        Return a UsageLocator for the same block in a different branch of the course.
+        """
+        return BlockUsageLocator(
+            self.course_key.for_version(version_guid),
+            block_type=self.block_type,
             block_id=self.block_id
         )
 
@@ -370,11 +373,10 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
     def _parse_block_ref(cls, block_ref):
         if isinstance(block_ref, LocalId):
             return block_ref
+        elif len(block_ref) > 0 and cls.ALLOWED_ID_RE.match(block_ref):
+            return block_ref
         else:
-            parse = parse_block_ref(block_ref)
-            if not parse:
             raise InvalidKeyError(cls, block_ref)
-            return parse.get('block_id')
 
     @property
     def definition_key(self):
@@ -400,6 +402,9 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
     def version_guid(self):
         return self.course_key.version_guid
 
+    def version(self):
+        return self.course_key.version_guid
+
     @property
     def name(self):
         """
@@ -411,7 +416,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         return self.course_key.is_fully_specified()
 
     @classmethod
-    def make_relative(cls, course_locator, block_id):
+    def make_relative(cls, course_locator, block_type, block_id):
         """
         Return a new instance which has the given block_id in the given course
         :param course_locator: may be a BlockUsageLocator in the same snapshot
@@ -420,6 +425,7 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
             course_locator = course_locator.course_key
         return BlockUsageLocator(
             course_key=course_locator,
+            block_type=block_type,
             block_id=block_id
         )
 
@@ -428,34 +434,17 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         Return a new instance which has the this block_id in the given course
         :param course_key: a CourseKey object representing the new course to map into
         """
-        return BlockUsageLocator.make_relative(course_key, self.block_id)
-
-    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
+        return BlockUsageLocator.make_relative(course_key, self.block_type, self.block_id)
 
     def _to_string(self):
         """
         Return a string representing this location.
         """
-        return u"{course_key}/{BLOCK_PREFIX}{block_id}".format(
+        return u"{course_key}+{BLOCK_TYPE_PREFIX}+{block_type}+{BLOCK_PREFIX}+{block_id}".format(
             course_key=self.course_key._to_string(),
-            BLOCK_PREFIX=BLOCK_PREFIX,
+            BLOCK_TYPE_PREFIX=self.BLOCK_TYPE_PREFIX,
+            block_type=self.block_type,
+            BLOCK_PREFIX=self.BLOCK_PREFIX,
             block_id=self.block_id
         )
 
@@ -467,43 +456,61 @@ class BlockUsageLocator(BlockLocatorBase, UsageKey):  # TODO implement UsageKey
         place, but I have no way to override. We should clearly define the purpose and restrictions of this
         (e.g., I'm assuming periods are fine).
         """
-        return re.sub('[^\w-]', '-', self._to_string())
+        return unicode(self)
 
 
-class DefinitionLocator(Locator):
+class DefinitionLocator(Locator, DefinitionKey):
     """
     Container for how to locate a description (the course-independent content).
     """
     CANONICAL_NAMESPACE = 'defx'
-    KEY_FIELDS = ('definition_id',)
+    KEY_FIELDS = ('definition_id', 'block_type')
 
-    URL_RE = re.compile(r'^defx:' + VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
+    # override the abstractproperty
+    block_type = None
+    definition_id = None
 
-    def __init__(self, definition_id):
+    def __init__(self, block_type, definition_id):
         if isinstance(definition_id, LocalId):
-            super(DefinitionLocator, self).__init__(definition_id)
+            super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type)
         elif isinstance(definition_id, basestring):
-            regex_match = self.URL_RE.match(definition_id)
-            if regex_match is not None:
-                super(DefinitionLocator, self).__init__(self.as_object_id(regex_match.group(1)))
-            else:
-                super(DefinitionLocator, self).__init__(self.as_object_id(definition_id))
-        else:
-            super(DefinitionLocator, self).__init__(self.as_object_id(definition_id))
+            try:
+                definition_id = self.as_object_id(definition_id)
+            except ValueError:
+                raise InvalidKeyError(self, definition_id)
+            super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type)
+        elif isinstance(definition_id, ObjectId):
+            super(DefinitionLocator, self).__init__(definition_id=definition_id, block_type=block_type)
 
     def _to_string(self):
         '''
         Return a string representing this location.
-        unicode(self) returns something like this: "version/519665f6223ebd6980884f2b"
+        unicode(self) returns something like this: "519665f6223ebd6980884f2b+type+problem"
         '''
-        return VERSION_PREFIX + str(self.definition_id)
+        return u"{}+{}+{}".format(unicode(self.definition_id), self.BLOCK_TYPE_PREFIX, self.block_type)
 
-    def url(self):
+    URL_RE = re.compile(
+        r"^(?P<definition_id>[A-F0-9]+)\+{}\+(?P<block_type>{ALLOWED_ID_CHARS}+)$".format(
+            Locator.BLOCK_TYPE_PREFIX, ALLOWED_ID_CHARS=Locator.ALLOWED_ID_CHARS
+        ),
+        re.IGNORECASE | re.VERBOSE | re.UNICODE
+    )
+
+    @classmethod
+    def _from_string(cls, serialized):
         """
-        Return a string containing the URL for this location.
-        url(self) returns something like this: 'defx:version/519665f6223ebd6980884f2b'
+        Return a DefinitionLocator parsing the given serialized string
+        :param serialized: matches the string to
         """
-        return u'defx:' + self._to_string()
+        parse = cls.URL_RE.match(serialized)
+        if not parse:
+            raise InvalidKeyError(cls, serialized)
+
+        parse = parse.groupdict()
+        if parse['definition_id']:
+            parse['definition_id'] = cls.as_object_id(parse['definition_id'])
+
+        return cls(**{key: parse.get(key) for key in cls.KEY_FIELDS})
 
     def version(self):
         """

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

-import re
-
-# Prefix for the branch portion of a locator URL
-BRANCH_PREFIX = r"branch/"
-# Prefix for the block portion of a locator URL
-BLOCK_PREFIX = r"block/"
-# Prefix for the version portion of a locator URL, when it is preceded by a course ID
-VERSION_PREFIX = r"version/"
-
-ALLOWED_ID_CHARS = r'[\w\-~.:+]'
-ALLOWED_ID_RE = re.compile(r'^{}+$'.format(ALLOWED_ID_CHARS), re.UNICODE)
-
-# NOTE: if we need to support period in place of +, make it aggressive (take the first period in the string)
-URL_RE_SOURCE = r"""
-    ((?P<org>{ALLOWED_ID_CHARS}+)\+(?P<offering>{ALLOWED_ID_CHARS}+)/?)?
-    ({BRANCH_PREFIX}(?P<branch>{ALLOWED_ID_CHARS}+)/?)?
-    ({VERSION_PREFIX}(?P<version_guid>[A-F0-9]+)/?)?
-    ({BLOCK_PREFIX}(?P<block_id>{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 | re.UNICODE)
-
-
-def parse_url(string):
-    """
-    followed by either a version_guid or a org + offering pair. If tag_optional, then
-    the url does not have to start with the tag and edx will be assumed.
-
-    Examples:
-        'edx:version/0123FFFF'
-        'edx:mit.eecs.6002x'
-        '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.
-
-    If it can be parsed as a version_guid with no preceding org + offering, returns a dict
-    with key 'version_guid' and the value,
-
-    If it can be parsed as a org + offering, returns a dict
-    with key 'id' and optional keys 'branch' and 'version_guid'.
-    """
-    match = URL_RE.match(string)
-    if not match:
-        return None
-    matched_dict = match.groupdict()
-    return matched_dict
-
-
-def parse_block_ref(string):
-    r"""
-    A block_ref is a string of url safe characters (see ALLOWED_ID_CHARS)
-
-    If string is a block_ref, returns a dict with key 'block_ref' and the value,
-    otherwise returns None.
-    """
-    if ALLOWED_ID_RE.match(string):
-        return {'block_id': string}
-    return None

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

@@ -108,6 +108,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 offering=course_entry_override.get('offering'),
                 branch=course_entry_override.get('branch'),
             ),
+            block_type=json_data.get('category'),
             block_id=block_id,
         )
 
@@ -131,6 +132,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 self,
                 BlockUsageLocator(
                     CourseLocator(version_guid=course_entry_override['structure']['_id']),
+                    block_type='error',
                     block_id=block_id
                 ),
                 error_msg=exc_info_to_str(sys.exc_info())

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

@@ -8,14 +8,14 @@ class DefinitionLazyLoader(object):
     object doesn't force access during init but waits until client wants the
     definition. Only works if the modulestore is a split mongo store.
     """
-    def __init__(self, modulestore, definition_id):
+    def __init__(self, modulestore, block_type, definition_id):
         """
         Simple placeholder for yet-to-be-fetched data
         :param modulestore: the pymongo db connection with the definitions
         :param definition_locator: the id of the record in the above to fetch
         """
         self.modulestore = modulestore
-        self.definition_locator = DefinitionLocator(definition_id)
+        self.definition_locator = DefinitionLocator(block_type, definition_id)
 
     def fetch(self):
         """

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

@@ -152,7 +152,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         if lazy:
             for block in new_module_data.itervalues():
-                block['definition'] = DefinitionLazyLoader(self, block['definition'])
+                block['definition'] = DefinitionLazyLoader(self, block['category'], block['definition'])
         else:
             # Load all descendants by id
             descendent_definitions = self.db_connection.find_matching_definitions({
@@ -242,11 +242,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         :param course_locator: any subclass of CourseLocator
         '''
-        # NOTE: if and when this uses cache, the update if changed logic will break if the cache
-        # holds the same objects as the descriptors!
-        if not course_locator.is_fully_specified():
-            raise InsufficientSpecificationError('Not fully specified: %s' % course_locator)
-
         if course_locator.org and course_locator.offering and course_locator.branch:
             # use the course id
             index = self.db_connection.get_course_index(course_locator)
@@ -258,6 +253,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             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, version_guid)
+        elif course_locator.version_guid is None:
+            raise InsufficientSpecificationError(course_locator)
         else:
             # TODO should this raise an exception if branch was provided?
             version_guid = course_locator.version_guid
@@ -322,9 +319,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
     def get_course(self, course_id, depth=None):
         '''
         Gets the course descriptor for the course identified by the locator
-        which may or may not be a blockLocator.
-
-        raises InsufficientSpecificationError
         '''
         assert(isinstance(course_id, CourseLocator))
         course_entry = self._lookup_course(course_id)
@@ -458,6 +452,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         return [
             BlockUsageLocator.make_relative(
                 locator,
+                block_type=course['structure']['blocks'][parent_id].get('category'),
                 block_id=LocMapperStore.decode_key_from_mongo(parent_id),
             )
             for parent_id in items
@@ -471,12 +466,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         course = self._lookup_course(course_key)
         items = {LocMapperStore.decode_key_from_mongo(block_id) for block_id in course['structure']['blocks'].keys()}
         items.remove(course['structure']['root'])
-        for block_id, block_data in course['structure']['blocks'].iteritems():
+        blocks = course['structure']['blocks']
+        for block_id, block_data in blocks.iteritems():
             items.difference_update(block_data.get('fields', {}).get('children', []))
             if block_data['category'] in detached_categories:
                 items.discard(LocMapperStore.decode_key_from_mongo(block_id))
         return [
-            BlockUsageLocator(course_key=course_key, block_id=block_id)
+            BlockUsageLocator(course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id)
             for block_id in items
         ]
 
@@ -613,11 +609,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # convert the results value sets to locators
         for k, versions in result.iteritems():
             result[k] = [
-                BlockUsageLocator(CourseLocator(version_guid=version), block_id=block_id)
+                block_locator.for_version(version)
                 for version in versions
             ]
         return VersionTree(
-            BlockUsageLocator(CourseLocator(version_guid=possible_roots[0]), block_id=block_id),
+            block_locator.for_version(possible_roots[0]),
             result
         )
 
@@ -650,7 +646,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             'schema_version': self.SCHEMA_VERSION,
         }
         self.db_connection.insert_definition(document)
-        definition_locator = DefinitionLocator(new_id)
+        definition_locator = DefinitionLocator(category, new_id)
         return definition_locator
 
     def update_definition_from_data(self, definition_locator, new_def_data, user_id):
@@ -685,7 +681,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             old_definition['edit_info']['previous_version'] = definition_locator.definition_id
             old_definition['schema_version'] = self.SCHEMA_VERSION
             self.db_connection.insert_definition(old_definition)
-            return DefinitionLocator(old_definition['_id']), True
+            return DefinitionLocator(old_definition['category'], old_definition['_id']), True
         else:
             return definition_locator, False
 
@@ -829,11 +825,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 self._update_head(index_entry, course_or_parent_locator.branch, new_id)
             item_loc = BlockUsageLocator(
                 course_or_parent_locator.version_agnostic(),
+                block_type=category,
                 block_id=new_block_id,
             )
         else:
             item_loc = BlockUsageLocator(
                 CourseLocator(version_guid=new_id),
+                block_type=category,
                 block_id=new_block_id,
             )
 
@@ -1029,7 +1027,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 course_key = CourseLocator(version_guid=new_id)
 
             # fetch and return the new item--fetching is unnecessary but a good qc step
-            new_locator = BlockUsageLocator(course_key, descriptor.location.block_id)
+            new_locator = descriptor.location.map_into_course(course_key)
             return self.get_item(new_locator)
         else:
             # nothing changed, just return the one sent in
@@ -1101,18 +1099,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 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.course_key.for_version(new_id),
-                    block_id=xblock.location.block_id,
-                )
-            )
+            return self.get_item(xblock.location.for_version(new_id))
         else:
             return xblock
 
     def _persist_subdag(self, xblock, user_id, structure_blocks, new_id):
         # persist the definition if persisted != passed
         new_def_data = self._filter_special_fields(xblock.get_explicitly_set_fields_by_scope(Scope.content))
+        is_updated = False
         if xblock.definition_locator is None or isinstance(xblock.definition_locator.definition_id, LocalId):
             xblock.definition_locator = self.create_definition_from_data(
                 new_def_data, xblock.category, user_id)
@@ -1134,9 +1128,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         else:
             is_new = False
             encoded_block_id = LocMapperStore.encode_key_for_mongo(xblock.location.block_id)
-            is_updated = is_updated or (
-                xblock.has_children and structure_blocks[encoded_block_id]['fields']['children'] != xblock.children
-            )
 
         children = []
         if xblock.has_children:
@@ -1147,6 +1138,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                     children.append(child_block.location.block_id)
                 else:
                     children.append(child)
+            is_updated = is_updated or structure_blocks[encoded_block_id]['fields']['children'] != children
 
         block_fields = xblock.get_explicitly_set_fields_by_scope(Scope.settings)
         if not is_new and not is_updated:
@@ -1419,9 +1411,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if isinstance(definition, DefinitionLazyLoader):
             return definition.definition_locator
         elif '_id' not in definition:
-            return DefinitionLocator(LocalId())
+            return DefinitionLocator(definition.get('category'), LocalId())
         else:
-            return DefinitionLocator(definition['_id'])
+            return DefinitionLocator(definition['category'], definition['_id'])
 
     def get_modulestore_type(self, course_id):
         """

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

@@ -32,14 +32,14 @@ class PersistentCourseFactory(SplitFactory):
 
     # pylint: disable=W0613
     @classmethod
-    def _create(cls, target_class, course_id='testX.999', org='testX', user_id='test_user',
+    def _create(cls, target_class, offering='999', org='testX', user_id='test_user',
                 master_branch='draft', **kwargs):
 
         modulestore = kwargs.pop('modulestore')
         root_block_id = kwargs.pop('root_block_id', 'course')
         # Write the data to the mongo datastore
         new_course = modulestore.create_course(
-            course_id, org, user_id, fields=kwargs,
+            org, offering, user_id, fields=kwargs,
             master_branch=master_branch, root_block_id=root_block_id
         )
 

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

@@ -267,6 +267,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
         )
         prob_locator = BlockUsageLocator(
             prob_course_key,
+            block_type='problem',
             block_id='problem2',
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
@@ -289,20 +290,21 @@ class TestLocationMapper(LocMapperSetupSansDjango):
         prob_location = loc_mapper().translate_locator_to_location(prob_locator, get_course=True)
         self.assertEqual(prob_location, SlashSeparatedCourseKey(org, course, run))
         # explicit branch
-        prob_locator = BlockUsageLocator(
-            prob_course_key.for_branch('draft'), block_id=prob_locator.block_id
-        )
+        prob_locator = prob_locator.for_branch('draft')
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         # Even though the problem was set as draft, we always return revision=None to work
         # with old mongo/draft modulestores.
         self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None))
-        prob_locator = BlockUsageLocator(prob_course_key.for_branch('production'), block_id='problem2')
+        prob_locator = BlockUsageLocator(
+            prob_course_key.for_branch('production'),
+            block_type='problem', block_id='problem2'
+        )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(prob_location, Location(org, course, run, 'problem', 'abc123', None))
         # same for chapter except chapter cannot be draft in old system
         chap_locator = BlockUsageLocator(
             prob_course_key.for_branch('production'),
-            block_id='chapter48f',
+            block_type='chapter', block_id='chapter48f',
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
@@ -311,7 +313,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
         chap_locator = BlockUsageLocator(
-            prob_course_key.for_branch('production'), block_id='chapter48f'
+            prob_course_key.for_branch('production'), block_type='chapter', block_id='chapter48f'
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location(org, course, run, 'chapter', '48f23a10395384929234'))
@@ -319,7 +321,7 @@ class TestLocationMapper(LocMapperSetupSansDjango):
         # look for non-existent problem
         prob_locator2 = BlockUsageLocator(
             prob_course_key.for_branch('draft'),
-            block_id='problem3'
+            block_type='problem', block_id='problem3'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator2)
         self.assertIsNone(prob_location, 'Found non-existent problem')

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

@@ -7,8 +7,8 @@ import random
 from bson.objectid import ObjectId
 from opaque_keys import InvalidKeyError
 from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DefinitionLocator
-from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX
 from ddt import ddt, data
+from xmodule.modulestore.keys import UsageKey, CourseKey, DefinitionKey
 
 
 @ddt
@@ -40,7 +40,7 @@ class LocatorTest(TestCase):
         testobj_1 = CourseLocator(version_guid=test_id_1)
         self.check_course_locn_fields(testobj_1, version_guid=test_id_1)
         self.assertEqual(str(testobj_1.version_guid), test_id_1_loc)
-        self.assertEqual(testobj_1._to_string(), VERSION_PREFIX + test_id_1_loc)
+        self.assertEqual(testobj_1._to_string(), u'+'.join((testobj_1.VERSION_PREFIX, test_id_1_loc)))
 
         # Test using a given string
         test_id_2_loc = '519665f6223ebd6980884f2b'
@@ -48,24 +48,24 @@ class LocatorTest(TestCase):
         testobj_2 = CourseLocator(version_guid=test_id_2)
         self.check_course_locn_fields(testobj_2, version_guid=test_id_2)
         self.assertEqual(str(testobj_2.version_guid), test_id_2_loc)
-        self.assertEqual(testobj_2._to_string(), VERSION_PREFIX + test_id_2_loc)
+        self.assertEqual(testobj_2._to_string(), u'+'.join((testobj_2.VERSION_PREFIX, test_id_2_loc)))
 
     @data(
         ' mit.eecs',
         'mit.eecs ',
-        VERSION_PREFIX + 'mit.eecs',
-        BLOCK_PREFIX + 'black/mit.eecs',
+        CourseLocator.VERSION_PREFIX + '+mit.eecs',
+        BlockUsageLocator.BLOCK_PREFIX + '+black+mit.eecs',
         'mit.ee cs',
         'mit.ee,cs',
-        'mit.ee/cs',
+        'mit.ee+cs',
         'mit.ee&cs',
         '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 ',
-        'mit.eecs/' + BRANCH_PREFIX + 'th%is ',
+        CourseLocator.BRANCH_PREFIX + '+this',
+        'mit.eecs+' + CourseLocator.BRANCH_PREFIX,
+        'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this+' + CourseLocator.BRANCH_PREFIX + '+that',
+        'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this+' + CourseLocator.BRANCH_PREFIX,
+        'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+this ',
+        'mit.eecs+' + CourseLocator.BRANCH_PREFIX + '+th%is ',
     )
     def test_course_constructor_bad_package_id(self, bad_id):
         """
@@ -78,18 +78,20 @@ class LocatorTest(TestCase):
             CourseLocator(org='test', offering=bad_id)
 
         with self.assertRaises(InvalidKeyError):
-            CourseLocator.from_string('course-locator:' + bad_id)
+            CourseKey.from_string('course-locator:test+{}'.format(bad_id))
 
     @data('course-locator:', 'course-locator:/mit.eecs', 'http:mit.eecs', 'course-locator//mit.eecs')
     def test_course_constructor_bad_url(self, bad_url):
         with self.assertRaises(InvalidKeyError):
-            CourseLocator.from_string(bad_url)
+            CourseKey.from_string(bad_url)
 
     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.from_string("course-locator:{}{}/{}hw3".format(VERSION_PREFIX, test_id_loc, BLOCK_PREFIX))
+        testobj = CourseKey.from_string("course-locator:{}+{}+{}+hw3".format(
+            CourseLocator.VERSION_PREFIX, test_id_loc, CourseLocator.BLOCK_PREFIX
+        ))
         self.check_course_locn_fields(
             testobj,
             version_guid=ObjectId(test_id_loc)
@@ -97,7 +99,9 @@ class LocatorTest(TestCase):
 
     def test_course_constructor_url_package_id_and_version_guid(self):
         test_id_loc = '519665f6223ebd6980884f2b'
-        testobj = CourseLocator.from_string('course-locator:mit.eecs+honors.6002x/' + VERSION_PREFIX + test_id_loc)
+        testobj = CourseKey.from_string(
+            'course-locator:mit.eecs+honors.6002x+{}+{}'.format(CourseLocator.VERSION_PREFIX, test_id_loc)
+        )
         self.check_course_locn_fields(
             testobj,
             org='mit.eecs',
@@ -109,8 +113,8 @@ class LocatorTest(TestCase):
         test_id_loc = '519665f6223ebd6980884f2b'
         org = 'mit.eecs'
         offering = '~6002x'
-        testobj = CourseLocator.from_string('course-locator:{}+{}/{}draft-1/{}{}'.format(
-            org, offering, BRANCH_PREFIX, VERSION_PREFIX, test_id_loc
+        testobj = CourseKey.from_string('course-locator:{}+{}+{}+draft-1+{}+{}'.format(
+            org, offering, CourseLocator.BRANCH_PREFIX, CourseLocator.VERSION_PREFIX, test_id_loc
         ))
         self.check_course_locn_fields(
             testobj,
@@ -126,15 +130,13 @@ class LocatorTest(TestCase):
         testurn = '{}+{}'.format(org, offering)
         testobj = CourseLocator(org=org, offering=offering)
         self.check_course_locn_fields(testobj, org=org, offering=offering)
-        self.assertEqual(testobj.package_id, testurn)
         self.assertEqual(testobj._to_string(), testurn)
 
     def test_course_constructor_package_id_separate_branch(self):
         org = 'mit.eecs'
         offering = '6002x'
-        testurn = '{}+{}'.format(org, offering)
         test_branch = 'published'
-        expected_urn = '{}+{}/{}{}'.format(org, offering, BRANCH_PREFIX, test_branch)
+        expected_urn = '{}+{}+{}+{}'.format(org, offering, CourseLocator.BRANCH_PREFIX, test_branch)
         testobj = CourseLocator(org=org, offering=offering, branch=test_branch)
         self.check_course_locn_fields(
             testobj,
@@ -142,7 +144,6 @@ class LocatorTest(TestCase):
             offering=offering,
             branch=test_branch,
         )
-        self.assertEqual(testobj.package_id, testurn)
         self.assertEqual(testobj.branch, test_branch)
         self.assertEqual(testobj._to_string(), expected_urn)
 
@@ -151,17 +152,21 @@ class LocatorTest(TestCase):
         expected_offering = '6002x'
         expected_branch = 'published'
         expected_block_ref = 'HW3'
-        testurn = 'edx:{}+{}/{}{}/{}{}'.format(
-            expected_org, expected_offering, BRANCH_PREFIX, expected_branch, BLOCK_PREFIX, 'HW3'
+        testurn = 'edx:{}+{}+{}+{}+{}+{}+{}+{}'.format(
+            expected_org, expected_offering, CourseLocator.BRANCH_PREFIX, expected_branch,
+            BlockUsageLocator.BLOCK_TYPE_PREFIX, 'problem', BlockUsageLocator.BLOCK_PREFIX, 'HW3'
         )
-        testobj = BlockUsageLocator.from_string(testurn)
-        self.check_block_locn_fields(testobj,
+        testobj = UsageKey.from_string(testurn)
+        self.check_block_locn_fields(
+             testobj,
              org=expected_org,
              offering=expected_offering,
              branch=expected_branch,
-                                     block=expected_block_ref)
+             block_type='problem',
+             block=expected_block_ref
+        )
         self.assertEqual(unicode(testobj), testurn)
-        testobj = BlockUsageLocator(testobj.course_key.for_version(ObjectId()), testobj.block_id)
+        testobj = testobj.for_version(ObjectId())
         agnostic = testobj.version_agnostic()
         self.assertIsNone(agnostic.version_guid)
         self.check_block_locn_fields(agnostic,
@@ -172,13 +177,16 @@ class LocatorTest(TestCase):
 
     def test_block_constructor_url_version_prefix(self):
         test_id_loc = '519665f6223ebd6980884f2b'
-        testobj = BlockUsageLocator.from_string(
-            'edx:mit.eecs+6002x/{}{}/{}lab2'.format(VERSION_PREFIX, test_id_loc, BLOCK_PREFIX)
+        testobj = UsageKey.from_string(
+            'edx:mit.eecs+6002x+{}+{}+{}+problem+{}+lab2'.format(
+                CourseLocator.VERSION_PREFIX, test_id_loc, BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX
+            )
         )
         self.check_block_locn_fields(
             testobj,
             org='mit.eecs',
             offering='6002x',
+            block_type='problem',
             block='lab2',
             version_guid=ObjectId(test_id_loc)
         )
@@ -195,9 +203,10 @@ class LocatorTest(TestCase):
 
     def test_block_constructor_url_kitchen_sink(self):
         test_id_loc = '519665f6223ebd6980884f2b'
-        testobj = BlockUsageLocator.from_string(
-            'edx:mit.eecs+6002x/{}draft/{}{}/{}lab2'.format(
-                BRANCH_PREFIX, VERSION_PREFIX, test_id_loc, BLOCK_PREFIX
+        testobj = UsageKey.from_string(
+            'edx:mit.eecs+6002x+{}+draft+{}+{}+{}+problem+{}+lab2'.format(
+                CourseLocator.BRANCH_PREFIX, CourseLocator.VERSION_PREFIX, test_id_loc,
+                BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX
             )
         )
         self.check_block_locn_fields(
@@ -219,6 +228,7 @@ class LocatorTest(TestCase):
         block_id = 'problem:with-colon~2'
         testobj = BlockUsageLocator(
             CourseLocator(org=org, offering=offering, branch=branch),
+            block_type='problem',
             block_id=block_id
         )
         self.check_block_locn_fields(
@@ -234,54 +244,32 @@ class LocatorTest(TestCase):
         branch = 'foo'
         baseobj = CourseLocator(org=org, offering=offering, branch=branch)
         block_id = 'problem:with-colon~2'
-        testobj = BlockUsageLocator.make_relative(baseobj, block_id)
+        testobj = BlockUsageLocator.make_relative(baseobj, 'problem', block_id)
         self.check_block_locn_fields(
             testobj, org=org, offering=offering, branch=branch, block=block_id
         )
         block_id = 'completely_different'
-        testobj = BlockUsageLocator.make_relative(testobj, block_id)
+        testobj = BlockUsageLocator.make_relative(testobj, 'problem', block_id)
         self.check_block_locn_fields(
             testobj, org=org, offering=offering, branch=branch, block=block_id
         )
 
     def test_repr(self):
-        testurn = 'edx:mit.eecs+6002x/' + BRANCH_PREFIX + 'published/' + BLOCK_PREFIX + 'HW3'
-        testobj = BlockUsageLocator.from_string(testurn)
-        self.assertEqual("BlockUsageLocator(CourseLocator(u'mit.eecs', u'6002x', u'published', None), u'HW3')", repr(testobj))
-
-    def test_url_reverse(self):
-        """
-        Test the url_reverse method
-        """
-        locator = BlockUsageLocator(
-            CourseLocator(org="a", offering="fancy_course-id", branch="branch_1.2-3"),
-            block_id='element'
-        )
-        self.assertEqual(
-            '/expression/{}/format'.format(unicode(locator)),
-            locator.url_reverse('expression', 'format')
-        )
-        self.assertEqual(
-            '/expression/{}/format'.format(unicode(locator)),
-            locator.url_reverse('/expression', '/format')
-        )
-        self.assertEqual(
-            '/expression/{}'.format(unicode(locator)),
-            locator.url_reverse('expression/', None)
-        )
-        self.assertEqual(
-            '/expression/{}'.format(unicode(locator)),
-            locator.url_reverse('/expression/', '')
+        testurn = u'edx:mit.eecs+6002x+{}+published+{}+problem+{}+HW3'.format(
+            CourseLocator.BRANCH_PREFIX, BlockUsageLocator.BLOCK_TYPE_PREFIX, BlockUsageLocator.BLOCK_PREFIX
         )
+        testobj = UsageKey.from_string(testurn)
+        self.assertEqual("BlockUsageLocator(CourseLocator(u'mit.eecs', u'6002x', u'published', None), u'problem', u'HW3')", repr(testobj))
 
     def test_description_locator_url(self):
         object_id = '{:024x}'.format(random.randrange(16 ** 24))
-        definition_locator = DefinitionLocator(object_id)
-        self.assertEqual('defx:' + VERSION_PREFIX + object_id, unicode(definition_locator))
+        definition_locator = DefinitionLocator('html', object_id)
+        self.assertEqual('defx:{}+{}+html'.format(object_id, DefinitionLocator.BLOCK_TYPE_PREFIX), unicode(definition_locator))
+        self.assertEqual(definition_locator, DefinitionKey.from_string(unicode(definition_locator)))
 
     def test_description_locator_version(self):
         object_id = '{:024x}'.format(random.randrange(16 ** 24))
-        definition_locator = DefinitionLocator(object_id)
+        definition_locator = DefinitionLocator('html', object_id)
         self.assertEqual(object_id, str(definition_locator.version()))
 
     # ------------------------------------------------------------------
@@ -298,10 +286,12 @@ class LocatorTest(TestCase):
         self.assertEqual(testobj.branch, branch)
 
     def check_block_locn_fields(self, testobj, version_guid=None,
-                                org=None, offering=None, branch=None, block=None):
+                                org=None, offering=None, branch=None, block_type=None, block=None):
         """
         Does adds a block id check over and above the check_course_locn_fields tests
         """
         self.check_course_locn_fields(testobj, version_guid, org, offering,
                                       branch)
+        if block_type is not None:
+            self.assertEqual(testobj.block_type, block_type)
         self.assertEqual(testobj.block_id, block)

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

@@ -442,7 +442,7 @@ class SplitModuleTest(unittest.TestCase):
         Sets up the initial data into the db
         '''
         split_store = modulestore()
-        for course_id, course_spec in SplitModuleTest.COURSE_CONTENT.iteritems():
+        for _course_id, course_spec in SplitModuleTest.COURSE_CONTENT.iteritems():
             course = split_store.create_course(
                 course_spec['org'], course_spec['offering'], course_spec['user_id'],
                 fields=course_spec['fields'],
@@ -454,7 +454,8 @@ class SplitModuleTest(unittest.TestCase):
                     if course.location.block_id == block_id:
                         block = course
                     else:
-                        block_usage = BlockUsageLocator.make_relative(course.location, block_id)
+                        # not easy to figure out the category but get_item won't care
+                        block_usage = BlockUsageLocator.make_relative(course.location, '', block_id)
                         block = split_store.get_item(block_usage)
                     for key, value in fields.iteritems():
                         setattr(block, key, value)
@@ -466,7 +467,7 @@ class SplitModuleTest(unittest.TestCase):
                     elif spec['parent'] == course.location.block_id:
                         parent = course
                     else:
-                        block_usage = BlockUsageLocator.make_relative(course.location, spec['parent'])
+                        block_usage = BlockUsageLocator.make_relative(course.location, '', spec['parent'])
                         parent = split_store.get_item(block_usage)
                     block_id = LocalId(spec['id'])
                     child = split_store.create_xblock(
@@ -477,6 +478,7 @@ class SplitModuleTest(unittest.TestCase):
         # publish "testx.wonderful"
         to_publish = BlockUsageLocator(
             CourseLocator(org="testx", offering="wonderful", branch="draft"),
+            block_type='course',
             block_id="head23456"
         )
         destination = CourseLocator(org="testx", offering="wonderful", branch="published")
@@ -676,12 +678,12 @@ class SplitModuleItemTests(SplitModuleTest):
         course = modulestore().get_course(course_locator)
         previous_version = course.previous_version
         # positive tests of various forms
-        locator = BlockUsageLocator(CourseLocator(version_guid=previous_version), block_id='head12345')
+        locator = course.location.map_into_course(CourseLocator(version_guid=previous_version))
         self.assertTrue(
             modulestore().has_item(locator), "couldn't find in %s" % previous_version
         )
 
-        locator = BlockUsageLocator(course_locator, block_id='head12345')
+        locator = course.location.version_agnostic()
         self.assertTrue(
             modulestore().has_item(locator),
         )
@@ -689,6 +691,7 @@ class SplitModuleItemTests(SplitModuleTest):
             modulestore().has_item(
                 BlockUsageLocator(
                     locator.course_key.for_branch('published'),
+                    block_type=locator.block_type,
                     block_id=locator.block_id
                 )
             ),
@@ -696,7 +699,7 @@ class SplitModuleItemTests(SplitModuleTest):
         )
 
         # not a course obj
-        locator = BlockUsageLocator(course_locator, block_id='chapter1')
+        locator = BlockUsageLocator(course_locator, block_type='chapter', block_id='chapter1')
         self.assertTrue(
             modulestore().has_item(locator),
             "couldn't find chapter1"
@@ -705,26 +708,25 @@ class SplitModuleItemTests(SplitModuleTest):
         # in published course
         locator = BlockUsageLocator(
             CourseLocator(org="testx", offering="wonderful", branch='draft'),
+            block_type="course",
             block_id="head23456"
         )
         self.assertTrue(
-            modulestore().has_item(
-                BlockUsageLocator(locator.course_key.for_branch("published"), block_id=locator.block_id)
-            )
+            modulestore().has_item(locator.for_branch("published"))
         )
-        locator = locator.for_branch('published')
-        self.assertTrue(modulestore().has_item(locator), "couldn't find in published")
 
     def test_negative_has_item(self):
         # negative tests--not found
         # no such course or block
         locator = BlockUsageLocator(
             CourseLocator(org="foo", offering="doesnotexist", branch='draft'),
+            block_type="course",
             block_id="head23456"
         )
         self.assertFalse(modulestore().has_item(locator))
         locator = BlockUsageLocator(
             CourseLocator(org="testx", offering="wonderful", branch='draft'),
+            block_type="vertical",
             block_id="doesnotexist"
         )
         self.assertFalse(modulestore().has_item(locator))
@@ -738,7 +740,7 @@ class SplitModuleItemTests(SplitModuleTest):
         previous_version = course.previous_version
 
         # positive tests of various forms
-        locator = BlockUsageLocator(CourseLocator(version_guid=previous_version), block_id='head12345')
+        locator = course.location.map_into_course(CourseLocator(version_guid=previous_version))
         block = modulestore().get_item(locator)
         self.assertIsInstance(block, CourseDescriptor)
         self.assertIsInstance(modulestore().get_item(locator), CourseDescriptor)
@@ -759,36 +761,27 @@ class SplitModuleItemTests(SplitModuleTest):
                 block.grade_cutoffs, {"Pass": 0.45},
             )
 
-        locator = BlockUsageLocator(hero_locator, block_id='head12345')
-        verify_greek_hero(modulestore().get_item(locator))
+        verify_greek_hero(modulestore().get_item(course.location))
 
         # try to look up other branches
         with self.assertRaises(ItemNotFoundError):
-            modulestore().get_item(
-                BlockUsageLocator(
-                    hero_locator.for_branch("published"),
-                    block_id=locator.block_id,
-                )
-            )
-        self.assertIsInstance(
-            modulestore().get_item(locator),
-            CourseDescriptor
-        )
+            modulestore().get_item(course.location.for_branch("published"))
 
     def test_get_non_root(self):
         # not a course obj
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter1'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter', 'chapter1'
         )
         block = modulestore().get_item(locator)
-        self.assertEqual(block.location.package_id, "testx+GreekHero")
+        self.assertEqual(block.location.org, "testx")
+        self.assertEqual(block.location.offering, "GreekHero")
         self.assertEqual(block.category, 'chapter')
         self.assertEqual(block.display_name, "Hercules")
         self.assertEqual(block.edited_by, "testassist@edx.org")
 
         # in published course
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='wonderful', branch='published'), 'head23456'
+            CourseLocator(org='testx', offering='wonderful', branch='published'), 'course', 'head23456'
         )
         self.assertIsInstance(
             modulestore().get_item(locator),
@@ -798,12 +791,12 @@ class SplitModuleItemTests(SplitModuleTest):
         # negative tests--not found
         # no such course or block
         locator = BlockUsageLocator(
-            CourseLocator(org='doesnotexist', offering='doesnotexist', branch='draft'), 'head23456'
+            CourseLocator(org='doesnotexist', offering='doesnotexist', branch='draft'), 'course', 'head23456'
         )
         with self.assertRaises(ItemNotFoundError):
             modulestore().get_item(locator)
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='wonderful', branch='draft'), 'doesnotexist'
+            CourseLocator(org='testx', offering='wonderful', branch='draft'), 'html', 'doesnotexist'
         )
         with self.assertRaises(ItemNotFoundError):
             modulestore().get_item(locator)
@@ -864,7 +857,7 @@ class SplitModuleItemTests(SplitModuleTest):
         '''
         locator = BlockUsageLocator(
             CourseLocator(org='testx', offering='GreekHero', branch='draft'),
-            block_id='chapter1'
+            'chapter', block_id='chapter1'
         )
         parents = modulestore().get_parent_locations(locator)
         self.assertEqual(len(parents), 1)
@@ -884,7 +877,7 @@ class SplitModuleItemTests(SplitModuleTest):
         Test the existing get_children method on xdescriptors
         """
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'head12345'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'course', 'head12345'
         )
         block = modulestore().get_item(locator)
         children = block.get_children()
@@ -952,11 +945,11 @@ class TestItemCrud(SplitModuleTest):
         self.assertIsNotNone(new_module.definition_locator)
         self.assertEqual(new_module.display_name, 'new sequential')
         # check that block does not exist in previous version
-        locator = BlockUsageLocator(
-            CourseLocator(version_guid=premod_course.location.version_guid),
-            block_id=new_module.location.block_id
+        locator = new_module.location.map_into_course(
+            CourseLocator(version_guid=premod_course.location.version_guid)
         )
-        self.assertRaises(ItemNotFoundError, modulestore().get_item, locator)
+        with self.assertRaises(ItemNotFoundError):
+            modulestore().get_item(locator)
 
     def test_create_parented_item(self):
         """
@@ -964,12 +957,12 @@ class TestItemCrud(SplitModuleTest):
         """
         locator = BlockUsageLocator(
             CourseLocator(org='testx', offering='GreekHero', branch='draft'),
-            block_id='chapter2'
+            'chapter', block_id='chapter2'
         )
         original = modulestore().get_item(locator)
 
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='wonderful', branch='draft'), 'head23456'
+            CourseLocator(org='testx', offering='wonderful', branch='draft'), 'course', 'head23456'
         )
         premod_course = modulestore().get_course(locator.course_key)
         category = 'chapter'
@@ -992,12 +985,12 @@ class TestItemCrud(SplitModuleTest):
         """
         locator = BlockUsageLocator(
             CourseLocator(org='testx', offering='GreekHero', branch='draft'),
-            block_id='problem1'
+            'problem', block_id='problem1'
         )
         original = modulestore().get_item(locator)
 
         locator = BlockUsageLocator(
-            CourseLocator(org='guestx', offering='contender', branch='draft'), 'head345679'
+            CourseLocator(org='guestx', offering='contender', branch='draft'), 'course', 'head345679'
         )
         category = 'problem'
         new_payload = "<problem>empty</problem>"
@@ -1031,8 +1024,8 @@ class TestItemCrud(SplitModuleTest):
         Check that using odd characters in block id don't break ability to add and retrieve block.
         """
         course_key = CourseLocator(org='guestx', offering='contender', branch='draft')
-        parent_locator = BlockUsageLocator(course_key, block_id="head345679")
-        chapter_locator = BlockUsageLocator(course_key, block_id="foo.bar_-~:0")
+        parent_locator = BlockUsageLocator(course_key, 'course', block_id="head345679")
+        chapter_locator = BlockUsageLocator(course_key, 'chapter', block_id="foo.bar_-~:0")
         modulestore().create_item(
             parent_locator, 'chapter', 'anotheruser',
             block_id=chapter_locator.block_id,
@@ -1043,7 +1036,7 @@ class TestItemCrud(SplitModuleTest):
         self.assertEqual(new_module.location.block_id, "foo.bar_-~:0")  # hardcode to ensure BUL init didn't change
         # now try making that a parent of something
         new_payload = "<problem>empty</problem>"
-        problem_locator = BlockUsageLocator(course_key, block_id="prob.bar_-~:99a")
+        problem_locator = BlockUsageLocator(course_key, 'problem', block_id="prob.bar_-~:99a")
         modulestore().create_item(
             chapter_locator, 'problem', 'anotheruser',
             block_id=problem_locator.block_id,
@@ -1119,10 +1112,7 @@ class TestItemCrud(SplitModuleTest):
             )
 
         # add new child to old parent in continued (leave off version_guid)
-        course_module_locator = BlockUsageLocator(
-            new_course.location.course_key.version_agnostic(),
-            block_id=new_course.location.block_id,
-        )
+        course_module_locator = new_course.location.version_agnostic()
         new_ele = modulestore().create_item(
             course_module_locator, 'chapter', user,
             fields={'display_name': 'chapter 4'},
@@ -1143,7 +1133,7 @@ class TestItemCrud(SplitModuleTest):
         """
         locator = BlockUsageLocator(
             CourseLocator(org="testx", offering="GreekHero", branch='draft'),
-            block_id="problem3_2"
+            'problem', block_id="problem3_2"
         )
         problem = modulestore().get_item(locator)
         pre_def_id = problem.definition_locator.definition_id
@@ -1160,10 +1150,7 @@ class TestItemCrud(SplitModuleTest):
         self.assertNotEqual(updated_problem.location.version_guid, pre_version_guid)
         self.assertEqual(updated_problem.max_attempts, 4)
         # refetch to ensure original didn't change
-        original_location = BlockUsageLocator(
-            CourseLocator(version_guid=pre_version_guid),
-            block_id=problem.location.block_id
-        )
+        original_location = problem.location.map_into_course(CourseLocator(version_guid=pre_version_guid))
         problem = modulestore().get_item(original_location)
         self.assertNotEqual(problem.max_attempts, 4, "original changed")
 
@@ -1179,7 +1166,7 @@ class TestItemCrud(SplitModuleTest):
         test updating an item's children ensuring the definition doesn't version but the course does if it should
         """
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter3'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'chapter', 'chapter3'
         )
         block = modulestore().get_item(locator)
         pre_def_id = block.definition_locator.definition_id
@@ -1206,7 +1193,7 @@ class TestItemCrud(SplitModuleTest):
         test updating an item's definition: ensure it gets versioned as well as the course getting versioned
         """
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'head12345'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'course', 'head12345'
         )
         block = modulestore().get_item(locator)
         pre_def_id = block.definition_locator.definition_id
@@ -1226,13 +1213,13 @@ class TestItemCrud(SplitModuleTest):
         """
         locator = BlockUsageLocator(
             CourseLocator('testx', 'GreekHero', branch='draft'),
-            block_id='problem1'
+            'problem', block_id='problem1'
         )
         original = modulestore().get_item(locator)
         # first add 2 children to the course for the update to manipulate
         locator = BlockUsageLocator(
             CourseLocator('guestx', 'contender', branch='draft'),
-            block_id="head345679"
+            'course', block_id="head345679"
         )
         category = 'problem'
         new_payload = "<problem>empty</problem>"
@@ -1282,11 +1269,7 @@ class TestItemCrud(SplitModuleTest):
         with self.assertRaises(VersionConflictError):
             modulestore().has_item(locn_to_del)
 
-        locator = BlockUsageLocator(
-            CourseLocator(version_guid=locn_to_del.version_guid),
-            block_id=locn_to_del.block_id
-        )
-        self.assertTrue(modulestore().has_item(locator))
+        self.assertTrue(modulestore().has_item(locn_to_del.course_agnostic()))
         self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid)
 
         # delete a subtree
@@ -1301,22 +1284,9 @@ class TestItemCrud(SplitModuleTest):
             if node:
                 node_loc = node.location
                 self.assertFalse(
-                    modulestore().has_item(
-                        BlockUsageLocator(
-                            CourseLocator(
-                                org=node_loc.org,
-                                offering=node_loc.offering,
-                                branch=node_loc.branch,
-                            ),
-                            block_id=node_loc.block_id
-                        )
-                    )
+                    modulestore().has_item(node_loc.version_agnostic())
                 )
-                locator = BlockUsageLocator(
-                    CourseLocator(version_guid=node.location.version_guid),
-                    block_id=node.location.block_id
-                )
-                self.assertTrue(modulestore().has_item(locator))
+                self.assertTrue(modulestore().has_item(node_loc.course_agnostic()))
                 if node.has_children:
                     for sub in node.get_children():
                         check_subtree(sub)
@@ -1327,10 +1297,7 @@ class TestItemCrud(SplitModuleTest):
         Create a course we can delete
         """
         course = modulestore().create_course('nihilx', 'deletion', 'deleting_user')
-        root = BlockUsageLocator(
-            course.id.version_agnostic().for_branch('draft'),
-            block_id=course.location.block_id,
-        )
+        root = course.location.version_agnostic().for_branch('draft')
         for _ in range(4):
             self.create_subtree_for_deletion(root, ['chapter', 'vertical', 'problem'])
         return modulestore().get_item(root)
@@ -1342,7 +1309,7 @@ class TestItemCrud(SplitModuleTest):
         if not category_queue:
             return
         node = modulestore().create_item(parent.version_agnostic(), category_queue[0], 'deleting_user')
-        node_loc = BlockUsageLocator(parent.course_key, block_id=node.location.block_id)
+        node_loc = node.location.map_into_course(parent.course_key)
         for _ in range(4):
             self.create_subtree_for_deletion(node_loc, category_queue[1:])
 
@@ -1523,13 +1490,13 @@ class TestInheritance(SplitModuleTest):
         # Note, not testing value where defined (course) b/c there's no
         # defined accessor for it on CourseDescriptor.
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem3_2'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem', 'problem3_2'
         )
         node = modulestore().get_item(locator)
         # inherited
         self.assertEqual(node.graceperiod, datetime.timedelta(hours=2))
         locator = BlockUsageLocator(
-            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem1'
+            CourseLocator(org='testx', offering='GreekHero', branch='draft'), 'problem', 'problem1'
         )
         node = modulestore().get_item(locator)
         # overridden
@@ -1560,19 +1527,19 @@ class TestPublish(SplitModuleTest):
         )
         # add a child under chapter1
         new_module = modulestore().create_item(
-            BlockUsageLocator.make_relative(source_course, "chapter1"), "sequential", self.user,
+            BlockUsageLocator.make_relative(source_course, "chapter", "chapter1"), "sequential", self.user,
             fields={'display_name': 'new sequential'},
         )
         # remove chapter1 from expected b/c its pub'd version != the source anymore since source changed
         expected.remove("chapter1")
         # check that it's not in published course
         with self.assertRaises(ItemNotFoundError):
-            modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id))
+            modulestore().get_item(new_module.location.map_into_course(dest_course))
         # publish it
         modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.block_id], None)
         expected.append(new_module.location.block_id)
         # check that it is in the published course and that its parent is the chapter
-        pub_module = modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id))
+        pub_module = modulestore().get_item(new_module.location.map_into_course(dest_course))
         self.assertEqual(
             modulestore().get_parent_locations(pub_module.location)[0].block_id, "chapter1"
         )
@@ -1584,7 +1551,7 @@ class TestPublish(SplitModuleTest):
         modulestore().xblock_publish(self.user, source_course, dest_course, [new_module.location.block_id], None)
         expected.append(new_module.location.block_id)
         # check that it is in the published course (no error means it worked)
-        pub_module = modulestore().get_item(BlockUsageLocator.make_relative(dest_course, new_module.location.block_id))
+        pub_module = modulestore().get_item(new_module.location.map_into_course(dest_course))
         self._check_course(
             source_course, dest_course, expected, ["chapter2", "chapter3", "problem1", "problem3_2"]
         )
@@ -1617,11 +1584,11 @@ class TestPublish(SplitModuleTest):
         expected = ["head12345", "chapter1", "chapter3", "problem1", "problem3_2"]
         self._check_course(source_course, dest_course, expected, ["chapter2"])
         # now move problem1 and delete problem3_2
-        chapter1 = modulestore().get_item(BlockUsageLocator.make_relative(source_course, "chapter1"))
-        chapter3 = modulestore().get_item(BlockUsageLocator.make_relative(source_course, "chapter3"))
+        chapter1 = modulestore().get_item(source_course.make_usage_key("chapter", "chapter1"))
+        chapter3 = modulestore().get_item(source_course.make_usage_key("chapter", "chapter3"))
         chapter1.children.append("problem1")
         chapter3.children.remove("problem1")
-        modulestore().delete_item(BlockUsageLocator.make_relative(source_course, "problem3_2"), self.user)
+        modulestore().delete_item(source_course.make_usage_key("problem", "problem3_2"), self.user)
         modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2"])
         expected = ["head12345", "chapter1", "chapter3", "problem1"]
         self._check_course(source_course, dest_course, expected, ["chapter2", "problem3_2"])
@@ -1633,8 +1600,9 @@ class TestPublish(SplitModuleTest):
         history_info = modulestore().get_course_history_info(dest_course_loc)
         self.assertEqual(history_info['edited_by'], self.user)
         for expected in expected_blocks:
-            source = modulestore().get_item(BlockUsageLocator.make_relative(source_course_loc, expected))
-            pub_copy = modulestore().get_item(BlockUsageLocator.make_relative(dest_course_loc, expected))
+            # since block_type has no impact on identity, we can just provide an empty string
+            source = modulestore().get_item(source_course_loc.make_usage_key("", expected))
+            pub_copy = modulestore().get_item(dest_course_loc.make_usage_key("", expected))
             # everything except previous_version & children should be the same
             self.assertEqual(source.category, pub_copy.category)
             self.assertEqual(source.update_version, pub_copy.update_version)
@@ -1649,7 +1617,7 @@ class TestPublish(SplitModuleTest):
                     self.assertEqual(field.read_from(source), field.read_from(pub_copy))
         for unexp in unexpected_blocks:
             with self.assertRaises(ItemNotFoundError):
-                modulestore().get_item(BlockUsageLocator.make_relative(dest_course_loc, unexp))
+                modulestore().get_item(dest_course_loc.make_usage_key("", unexp))
 
     def _compare_children(self, source_children, dest_children, unexpected):
         """

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

@@ -105,6 +105,7 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
             # create pointer for split
             course_or_parent_locator = BlockUsageLocator(
                 course_key=self.split_course_key,
+                block_type=parent_category,
                 block_id=parent_name
             )
         else:

common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py

@@ -37,7 +37,7 @@ class ControllerQueryService(GradingService):
     def check_combined_notifications(self, course_id, student_id, user_is_staff, last_time_viewed):
         params = {
             'student_id': student_id,
-            'course_id': course_id,
+            'course_id': course_id.to_deprecated_string(),
             'user_is_staff': user_is_staff,
             'last_time_viewed': last_time_viewed,
         }

common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py

@@ -85,7 +85,7 @@ class PeerGradingService(GradingService):
         return result
 
     def get_problem_list(self, course_id, grader_id):
-        params = {'course_id': course_id, 'student_id': grader_id}
+        params = {'course_id': course_id.to_deprecated_string(), 'student_id': grader_id}
         result = self.get(self.get_problem_list_url, params)
 
         if 'problem_list' in result:
@@ -100,7 +100,7 @@ class PeerGradingService(GradingService):
         return result
 
     def get_notifications(self, course_id, grader_id):
-        params = {'course_id': course_id, 'student_id': grader_id}
+        params = {'course_id': course_id.to_deprecated_string(), 'student_id': grader_id}
         result = self.get(self.get_notifications_url, params)
         self._record_result(
             'get_notifications',

lms/djangoapps/django_comment_client/base/views.py

@@ -182,7 +182,7 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None):
         'anonymous': anonymous,
         'anonymous_to_peers': anonymous_to_peers,
         'user_id': request.user.id,
-        'course_id': course_key,
+        'course_id': course_key.to_deprecated_string(),
         'thread_id': thread_id,
         'parent_id': parent_id,
     })

lms/djangoapps/django_comment_client/forum/tests.py

@@ -285,7 +285,7 @@ class UserProfileTestCase(ModuleStoreTestCase):
             StringEndsWithMatcher('/users/{}/active_threads'.format(self.profiled_user.id)),
             data=None,
             params=PartialDictMatcher({
-                "course_id": self.course.id,
+                "course_id": self.course.id.to_deprecated_string(),
                 "page": params.get("page", 1),
                 "per_page": views.THREADS_PER_PAGE
                 }),

lms/djangoapps/open_ended_grading/staff_grading_service.py

@@ -9,7 +9,6 @@ from django.conf import settings
 from django.http import HttpResponse, Http404
 from django.utils.translation import ugettext as _
 
-from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore.locations import SlashSeparatedCourseKey
 from xmodule.open_ended_grading_classes.grading_service_module import GradingService, GradingServiceError
 from xmodule.modulestore.django import ModuleI18nService
@@ -116,7 +115,7 @@ class StaffGradingService(GradingService):
         Raises:
             GradingServiceError: something went wrong with the connection.
         """
-        params = {'course_id': course_id, 'grader_id': grader_id}
+        params = {'course_id': course_id.to_deprecated_string(), 'grader_id': grader_id}
         result = self.get(self.get_problem_list_url, params)
         tags = [u'course_id:{}'.format(course_id)]
         self._record_result('get_problem_list', result, tags)
@@ -148,7 +147,7 @@ class StaffGradingService(GradingService):
             self.get(
                 self.get_next_url,
                 params={
-                    'location': location,
+                    'location': location.to_deprecated_string(),
                     'grader_id': grader_id
                 }
             )
@@ -170,7 +169,7 @@ class StaffGradingService(GradingService):
         Raises:
             GradingServiceError if there's a problem connecting.
         """
-        data = {'course_id': course_id,
+        data = {'course_id': course_id.to_deprecated_string(),
                 'submission_id': submission_id,
                 'score': score,
                 'feedback': feedback,
@@ -186,7 +185,7 @@ class StaffGradingService(GradingService):
         return result
 
     def get_notifications(self, course_id):
-        params = {'course_id': course_id}
+        params = {'course_id': course_id.to_deprecated_string()}
         result = self.get(self.get_notifications_url, params)
         tags = [
             u'course_id:{}'.format(course_id),
@@ -274,7 +273,7 @@ def get_next(request, course_id):
             ', '.join(missing)))
     grader_id = unique_id_for_user(request.user)
     p = request.POST
-    location = p['location']
+    location = course_key.make_usage_key_from_deprecated_string(p['location'])
 
     return HttpResponse(json.dumps(_get_next(course_key, grader_id, location)),
                         mimetype="application/json")
@@ -400,7 +399,7 @@ def save_grade(request, course_id):
 
     grader_id = unique_id_for_user(request.user)
 
-    location = p['location']
+    location = course_key.make_usage_key_from_deprecated_string(p['location'])
 
     try:
         result = staff_grading_service().save_grade(course_key,

lms/djangoapps/open_ended_grading/tests.py

@@ -109,13 +109,13 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.student = 'view@test.com'
         self.instructor = 'view2@test.com'
         self.password = 'foo'
-        self.location = 'TestLocation'
         self.create_account('u1', self.student, self.password)
         self.create_account('u2', self.instructor, self.password)
         self.activate_user(self.student)
         self.activate_user(self.instructor)
 
         self.course_id = SlashSeparatedCourseKey("edX", "toy", "2012_Fall")
+        self.location_string = self.course_id.make_usage_key('html', 'TestLocation').to_deprecated_string()
         self.toy = modulestore().get_course(self.course_id)
 
         make_instructor(self.toy, self.instructor)
@@ -140,7 +140,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.login(self.instructor, self.password)
 
         url = reverse('staff_grading_get_next', kwargs={'course_id': self.course_id.to_deprecated_string()})
-        data = {'location': self.location}
+        data = {'location': self.location_string}
 
         response = check_for_post_code(self, 200, url, data)
 
@@ -165,7 +165,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         data = {'score': '12',
                 'feedback': 'great!',
                 'submission_id': '123',
-                'location': self.location,
+                'location': self.location_string,
                 'submission_flagged': "true",
                 'rubric_scores[]': ['1', '2']}
         if skip:
@@ -227,7 +227,7 @@ class TestStaffGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
             'score': '12',
             'feedback': '',
             'submission_id': '123',
-            'location': self.location,
+            'location': self.location_string,
             'submission_flagged': "false",
             'rubric_scores[]': ['1', '2']
         }
@@ -262,13 +262,13 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.student = 'view@test.com'
         self.instructor = 'view2@test.com'
         self.password = 'foo'
-        self.location = 'TestLocation'
         self.create_account('u1', self.student, self.password)
         self.create_account('u2', self.instructor, self.password)
         self.activate_user(self.student)
         self.activate_user(self.instructor)
 
         self.course_id = SlashSeparatedCourseKey("edX", "toy", "2012_Fall")
+        self.location_string = self.course_id.make_usage_key('html', 'TestLocation').to_deprecated_string()
         self.toy = modulestore().get_course(self.course_id)
         location = "i4x://edX/toy/peergrading/init"
         field_data = DictFieldData({'data': "<peergrading/>", 'location': location, 'category':'peergrading'})
@@ -292,7 +292,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.logout()
 
     def test_get_next_submission_success(self):
-        data = {'location': self.location}
+        data = {'location': self.location_string}
 
         response = self.peer_module.get_next_submission(data)
         content = response
@@ -312,7 +312,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
     def test_save_grade_success(self):
         data = {
             'rubric_scores[]': [0, 0],
-            'location': self.location,
+            'location': self.location_string,
             'submission_id': 1,
             'submission_key': 'fake key',
             'score': 2,
@@ -342,7 +342,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertTrue(d['error'].find('Missing required keys:') > -1)
 
     def test_is_calibrated_success(self):
-        data = {'location': self.location}
+        data = {'location': self.location_string}
         response = self.peer_module.is_student_calibrated(data)
 
         self.assertTrue(response['success'])
@@ -355,7 +355,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertFalse('calibrated' in response)
 
     def test_show_calibration_essay_success(self):
-        data = {'location': self.location}
+        data = {'location': self.location_string}
 
         response = self.peer_module.show_calibration_essay(data)
 
@@ -376,7 +376,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
     def test_save_calibration_essay_success(self):
         data = {
             'rubric_scores[]': [0, 0],
-            'location': self.location,
+            'location': self.location_string,
             'submission_id': 1,
             'submission_key': 'fake key',
             'score': 2,
@@ -410,7 +410,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         data = {
             'rubric_scores[]': [0, 0],
-            'location': self.location,
+            'location': self.location_string,
             'submission_id': 1,
             'submission_key': 'fake key',
             'score': 2,

lms/djangoapps/open_ended_grading/views.py

 import logging
 
-from django.conf import settings
 from django.views.decorators.cache import cache_control
 from edxmako.shortcuts import render_to_response
 from django.core.urlresolvers import reverse
 
-from student.models import unique_id_for_user
 from courseware.courses import get_course_with_access
 
 from xmodule.open_ended_grading_classes.grading_service_module import GradingServiceError
@@ -20,11 +18,11 @@ from xmodule.modulestore import SlashSeparatedCourseKey
 from xmodule.modulestore.exceptions import NoPathToItem
 
 from django.http import HttpResponse, Http404, HttpResponseRedirect
-from edxmako.shortcuts import render_to_string
 from django.utils.translation import ugettext as _
 
-from open_ended_grading.utils import (STAFF_ERROR_MESSAGE, STUDENT_ERROR_MESSAGE,
-                                      StudentProblemList, generate_problem_url, create_controller_query_service)
+from open_ended_grading.utils import (
+    STAFF_ERROR_MESSAGE, StudentProblemList, generate_problem_url, create_controller_query_service
+)
 
 log = logging.getLogger(__name__)
 
@@ -68,9 +66,10 @@ def staff_grading(request, course_id):
     """
     Show the instructor grading interface.
     """
-    course = get_course_with_access(request.user, 'staff', course_id)
+    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+    course = get_course_with_access(request.user, 'staff', course_key)
 
-    ajax_url = _reverse_with_slash('staff_grading', course_id)
+    ajax_url = _reverse_with_slash('staff_grading', course_key)
 
     return render_to_response('instructor/staff_grading.html', {
         'course': course,
@@ -118,9 +117,9 @@ def peer_grading(request, course_id):
     When a student clicks on the "peer grading" button in the open ended interface, link them to a peer grading
     xmodule in the course.
     '''
-
+    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
     #Get the current course
-    course = get_course_with_access(request.user, 'load', course_id)
+    course = get_course_with_access(request.user, 'load', course_key)
 
     found_module, problem_url = find_peer_grading_module(course)
     if not found_module:
@@ -187,13 +186,11 @@ def flagged_problem_list(request, course_id):
     '''
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
     course = get_course_with_access(request.user, 'staff', course_key)
-    student_id = unique_id_for_user(request.user)
 
     # call problem list service
     success = False
     error_text = ""
     problem_list = []
-    base_course_url = reverse('courses')
 
     # Make a service that can query edX ORA.
     controller_qs = create_controller_query_service()

lms/lib/comment_client/user.py

@@ -86,7 +86,7 @@ class User(models.Model):
         if not self.course_id:
             raise CommentClientRequestError("Must provide course_id when retrieving active threads for the user")
         url = _url_for_user_active_threads(self.id)
-        params = {'course_id': self.course_id}
+        params = {'course_id': self.course_id.to_deprecated_string()}
         params = merge_dict(params, query_params)
         response = perform_request(
             'get',
@@ -102,7 +102,7 @@ class User(models.Model):
         if not self.course_id:
             raise CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user")
         url = _url_for_user_subscribed_threads(self.id)
-        params = {'course_id': self.course_id}
+        params = {'course_id': self.course_id.to_deprecated_string()}
         params = merge_dict(params, query_params)
         response = perform_request(
             'get',
@@ -118,7 +118,7 @@ class User(models.Model):
         url = self.url(action='get', params=self.attributes)
         retrieve_params = self.default_retrieve_params
         if self.attributes.get('course_id'):
-            retrieve_params['course_id'] = self.course_id
+            retrieve_params['course_id'] = self.course_id.to_deprecated_string()
         try:
             response = perform_request(
                 'get',