Merge pull request #1287 from edx/dhm/restful_crud

Restful api prototype

cms/djangoapps/auth/authz.py

+#=======================================================================================================================
+#
+# This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story
+# but this implementation should be data compatible with the LMS implementation
+#
+#=======================================================================================================================
 from django.contrib.auth.models import User, Group
 from django.core.exceptions import PermissionDenied
 from django.conf import settings
 
 from xmodule.modulestore import Location
+from xmodule.modulestore.locator import CourseLocator, Locator
 
-'''
-This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story
-but this implementation should be data compatible with the LMS implementation
-'''
 
 # define a couple of simple roles, we just need ADMIN and EDITOR now for our purposes
 INSTRUCTOR_ROLE_NAME = 'instructor'
@@ -22,16 +25,22 @@ COURSE_CREATOR_GROUP_NAME = "course_creator_group"
 
 
 def get_course_groupname_for_role(location, role):
-    loc = Location(location)
+    location = Locator.to_locator_or_location(location)
+
     # hack: check for existence of a group name in the legacy LMS format <role>_<course>
     # if it exists, then use that one, otherwise use a <role>_<course_id> which contains
     # more information
-    groupname = '{0}_{1}'.format(role, loc.course)
-
-    if len(Group.objects.filter(name=groupname)) == 0:
-        groupname = '{0}_{1}'.format(role, loc.course_id)
-
+    groupnames = []
+    groupnames.append('{0}_{1}'.format(role, location.course_id))
+    if isinstance(location, Location):
+        groupnames.append('{0}_{1}'.format(role, location.course))
+    elif isinstance(location, CourseLocator):
+        groupnames.append('{0}_{1}'.format(role, location.as_old_location_course_id))
+
+    for groupname in groupnames:
+        if Group.objects.filter(name=groupname).exists():
             return groupname
+    return groupnames[0]
 
 
 def get_users_in_course_group_by_role(location, role):

cms/djangoapps/contentstore/tests/test_course_index.py

+"""
+Unit tests for getting the list of courses and the course outline.
+"""
+from django.core.urlresolvers import reverse
+import lxml
+
+from contentstore.tests.utils import CourseTestCase
+from xmodule.modulestore.django import loc_mapper
+from django.core.exceptions import PermissionDenied
+
+class TestCourseIndex(CourseTestCase):
+    """
+    Unit tests for getting the list of courses and the course outline.
+    """
+    def test_index(self):
+        """
+        Test getting the list of courses and then pulling up their outlines
+        """
+        index_url = reverse('contentstore.views.index')
+        index_response = self.client.get(index_url, {}, HTTP_ACCEPT='text/html')
+        parsed_html = lxml.html.fromstring(index_response.content)
+        course_link_eles = parsed_html.find_class('course-link')
+        for link in course_link_eles:
+            self.assertRegexpMatches(link.get("href"), r'course/\w+\.\w+\.\w+.*/branch/\w+/block/.*')
+            # now test that url
+            outline_response = self.client.get(link.get("href"), {}, HTTP_ACCEPT='text/html')
+            # ensure it has the expected 2 self referential links
+            outline_parsed = lxml.html.fromstring(outline_response.content)
+            outline_link = outline_parsed.find_class('course-link')[0]
+            self.assertEqual(outline_link.get("href"), link.get("href"))
+            course_menu_link = outline_parsed.find_class('nav-course-courseware-outline')[0]
+            self.assertEqual(course_menu_link.find("a").get("href"), link.get("href"))
+
+    def test_negative_conditions(self):
+        """
+        Test the error conditions for the access
+        """
+        locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True)
+        outline_url = reverse('contentstore.views.course_handler', kwargs={'course_url': unicode(locator)})
+        # register a non-staff member and try to delete the course branch
+        non_staff_client = self.createNonStaffAuthedUserClient()
+        response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json')
+        self.assertEqual(response.status_code, 403)

cms/djangoapps/contentstore/tests/test_crud.py

@@ -2,7 +2,7 @@ import unittest
 from xmodule import templates
 from xmodule.modulestore.tests import persistent_factories
 from xmodule.course_module import CourseDescriptor
-from xmodule.modulestore.django import modulestore, loc_mapper
+from xmodule.modulestore.django import modulestore, loc_mapper, clear_existing_modulestores
 from xmodule.seq_module import SequenceDescriptor
 from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator
@@ -17,6 +17,9 @@ class TemplateTests(unittest.TestCase):
     Test finding and using the templates (boilerplates) for xblocks.
     """
 
+    def setUp(self):
+        clear_existing_modulestores()
+
     def test_get_templates(self):
         found = templates.all_templates()
         self.assertIsNotNone(found.get('course'))

cms/djangoapps/contentstore/tests/utils.py

@@ -61,3 +61,21 @@ class CourseTestCase(ModuleStoreTestCase):
             number='999',
             display_name='Robot Super Course',
         )
+
+    def createNonStaffAuthedUserClient(self):
+        """
+        Create a non-staff user, log them in, and return the client to use for testing.
+        """
+        uname = 'teststudent'
+        password = 'foo'
+        nonstaff = User.objects.create_user(uname, 'test+student@edx.org', password)
+
+        # Note that we do not actually need to do anything
+        # for registration if we directly mark them active.
+        nonstaff.is_active = True
+        nonstaff.is_staff = False
+        nonstaff.save()
+
+        client = Client()
+        client.login(username=uname, password=password)
+        return client

cms/djangoapps/contentstore/views/access.py

@@ -3,6 +3,7 @@ from auth.authz import is_user_in_course_group_role
 from django.core.exceptions import PermissionDenied
 from ..utils import get_course_location_for_item
 from xmodule.modulestore import Location
+from xmodule.modulestore.locator import CourseLocator
 
 
 def get_location_and_verify_access(request, org, course, name):
@@ -29,13 +30,14 @@ def has_access(user, location, role=STAFF_ROLE_NAME):
     will not be in both INSTRUCTOR and STAFF groups, so we have to cascade our
     queries here as INSTRUCTOR has all the rights that STAFF do
     '''
-    course_location = get_course_location_for_item(location)
-    _has_access = is_user_in_course_group_role(user, course_location, role)
+    if not isinstance(location, CourseLocator):
+        location = get_course_location_for_item(location)
+    _has_access = is_user_in_course_group_role(user, location, role)
     # if we're not in STAFF, perhaps we're in INSTRUCTOR groups
     if not _has_access and role == STAFF_ROLE_NAME:
         _has_access = is_user_in_course_group_role(
                 user,
-                course_location,
+                location,
                 INSTRUCTOR_ROLE_NAME
         )
     return _has_access

cms/djangoapps/contentstore/views/course.py

@@ -12,11 +12,11 @@ from django.conf import settings
 from django.views.decorators.http import require_http_methods, require_POST
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
-from django.http import HttpResponseBadRequest
+from django.http import HttpResponseBadRequest, HttpResponseNotFound
 from util.json_request import JsonResponse
 from mitxmako.shortcuts import render_to_response
 
-from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore.inheritance import own_metadata
 from xmodule.contentstore.content import StaticContent
 
@@ -48,7 +48,8 @@ from django_comment_common.utils import seed_permissions_roles
 from student.models import CourseEnrollment
 
 from xmodule.html_module import AboutDescriptor
-__all__ = ['course_index', 'create_new_course', 'course_info',
+from xmodule.modulestore.locator import BlockUsageLocator
+__all__ = ['create_new_course', 'course_info', 'course_handler',
            'course_info_updates', 'get_course_settings',
            'course_config_graders_page',
            'course_config_advanced_page',
@@ -59,24 +60,83 @@ __all__ = ['course_index', 'create_new_course', 'course_info',
 
 
 @login_required
+def course_handler(request, course_url):
+    """
+    The restful handler for course specific requests.
+    It provides the course tree with the necessary information for identifying and labeling the parts. The root
+    will typically be a 'course' object but may not be especially as we support modules.
+
+    GET
+        html: return html page overview for the given course
+        json: return json representing the course branch's index entry as well as dag w/ all of the children
+        replaced w/ json docs where each doc has {'_id': , 'display_name': , 'children': }
+    POST
+        json: create (or update?) this course or branch in this course for this user, return resulting json
+        descriptor (same as in GET course/...). Leaving off /branch/draft would imply create the course w/ default
+        branches. Cannot change the structure contents ('_id', 'display_name', 'children') but can change the
+        index entry.
+    PUT
+        json: update this course (index entry not xblock) such as repointing head, changing display name, org,
+        course_id, prettyid. Return same json as above.
+    DELETE
+        json: delete this branch from this course (leaving off /branch/draft would imply delete the course)
+    """
+    if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
+        if request.method == 'GET':
+            raise NotImplementedError('coming soon')
+        elif not has_access(request.user, BlockUsageLocator(course_url)):
+            raise PermissionDenied()
+        elif request.method == 'POST':
+            raise NotImplementedError()
+        elif request.method == 'PUT':
+            raise NotImplementedError()
+        elif request.method == 'DELETE':
+            raise NotImplementedError()
+        else:
+            return HttpResponseBadRequest()
+    elif request.method == 'GET':  # assume html
+        return course_index(request, course_url)
+    else:
+        return HttpResponseNotFound()
+
+
+@login_required
 @ensure_csrf_cookie
-def course_index(request, org, course, name):
+def old_course_index_shim(request, org, course, name):
+    """
+    A shim for any unconverted uses of course_index
+    """
+    old_location = Location(['i4x', org, course, 'course', name])
+    locator = loc_mapper().translate_location(old_location.course_id, old_location, False, True)
+    return course_index(request, locator)
+
+
+@login_required
+@ensure_csrf_cookie
+def course_index(request, course_url):
     """
     Display an editable course overview.
 
     org, course, name: Attributes of the Location for the item to edit
     """
-    location = get_location_and_verify_access(request, org, course, name)
+    location = BlockUsageLocator(course_url)
+    # TODO: when converting to split backend, if location does not have a usage_id,
+    # we'll need to get the course's root block_id
+    if not has_access(request.user, location):
+        raise PermissionDenied()
+
+
+    old_location = loc_mapper().translate_locator_to_location(location)
 
-    lms_link = get_lms_link_for_item(location)
+    lms_link = get_lms_link_for_item(old_location)
 
     upload_asset_callback_url = reverse('upload_asset', kwargs={
-        'org': org,
-        'course': course,
-        'coursename': name
+        'org': location.as_old_location_org,
+        'course': location.as_old_location_course,
+        'coursename': location.as_old_location_run
     })
 
-    course = modulestore().get_item(location, depth=3)
+    course = modulestore().get_item(old_location, depth=3)
     sections = course.get_children()
 
     return render_to_response('overview.html', {

cms/djangoapps/contentstore/views/user.py

@@ -11,7 +11,7 @@ from django_future.csrf import ensure_csrf_cookie
 from mitxmako.shortcuts import render_to_response
 from django.core.context_processors import csrf
 
-from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore import Location
 from xmodule.error_module import ErrorDescriptor
 from contentstore.utils import get_lms_link_for_item
@@ -46,13 +46,13 @@ def index(request):
     courses = filter(course_filter, courses)
 
     def format_course_for_view(course):
+        # published = false b/c studio manipulates draft versions not b/c the course isn't pub'd
+        course_url = loc_mapper().translate_location(
+            course.location.course_id, course.location, published=False, add_entry_if_missing=True
+        )
         return (
             course.display_name,
-            reverse("course_index", kwargs={
-                'org': course.location.org,
-                'course': course.location.course,
-                'name': course.location.name,
-            }),
+            reverse("contentstore.views.course_handler", kwargs={'course_url': course_url}),
             get_lms_link_for_item(
                 course.location
             ),

cms/pydev_manage.py

-'''
-Used for pydev eclipse. Should be innocuous for everyone else.
-Created on May 8, 2013
-
-@author: dmitchell
-'''
-#!/home/<username>/mitx_all/python/bin/python
-from django.core import management
-
-if __name__ == '__main__':
-    management.execute_from_command_line()

cms/templates/widgets/header.html

@@ -2,6 +2,7 @@
 <%!
   from django.core.urlresolvers import reverse
   from django.utils.translation import ugettext as _
+  from xmodule.modulestore.django import loc_mapper
 %>
 
 <div class="wrapper-header wrapper" id="view-top">
@@ -12,10 +13,16 @@
       <h1 class="branding"><a href="/"><img src="${static.url("img/logo-edx-studio.png")}" alt="edX Studio" /></a></h1>
 
       % if context_course:
-      <% ctx_loc = context_course.location %>
+      <% 
+            ctx_loc = context_course.location
+            index_url = reverse(
+                'contentstore.views.course_handler', 
+                kwargs={'course_url': loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)}
+            )
+      %>
       <h2 class="info-course">
         <span class="sr">${_("Current Course:")}</span>
-        <a class="course-link" href="${reverse('course_index', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">
+        <a class="course-link" href="${index_url}">
           <span class="course-org">${context_course.display_org_with_default | h}</span><span class="course-number">${context_course.display_number_with_default | h}</span>
           <span class="course-title" title="${context_course.display_name_with_default}">${context_course.display_name_with_default}</span>
         </a>
@@ -31,7 +38,7 @@
               <div class="nav-sub">
                 <ul>
                   <li class="nav-item nav-course-courseware-outline">
-                    <a href="${reverse('course_index', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">${_("Outline")}</a>
+                    <a href="${index_url}">${_("Outline")}</a>
                   </li>
                   <li class="nav-item nav-course-courseware-updates">
                     <a href="${reverse('course_info', kwargs=dict(org=ctx_loc.org, course=ctx_loc.course, name=ctx_loc.name))}">${_("Updates")}</a>

cms/urls.py

@@ -9,7 +9,7 @@ startup.run()
 from ratelimitbackend import admin
 admin.autodiscover()
 
-urlpatterns = ('',  # nopep8
+urlpatterns = patterns('',  # nopep8
     url(r'^$', 'contentstore.views.howitworks', name='homepage'),
     url(r'^listing', 'contentstore.views.index', name='index'),
     url(r'^request_course_creator$', 'contentstore.views.request_course_creator', name='request_course_creator'),
@@ -25,8 +25,6 @@ urlpatterns = ('',  # nopep8
     url(r'^create_new_course', 'contentstore.views.create_new_course', name='create_new_course'),
     url(r'^reorder_static_tabs', 'contentstore.views.reorder_static_tabs', name='reorder_static_tabs'),
 
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$',
-        'contentstore.views.course_index', name='course_index'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/import/(?P<name>[^/]+)$',
         'contentstore.views.import_course', name='import_course'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/import_status/(?P<name>[^/]+)$',
@@ -106,7 +104,8 @@ urlpatterns = ('',  # nopep8
 )
 
 # User creation and updating views
-urlpatterns += (
+urlpatterns += patterns(
+    '',
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/checklists/(?P<name>[^/]+)$', 'contentstore.views.get_checklists', name='checklists'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/checklists/(?P<name>[^/]+)/update(/)?(?P<checklist_index>.+)?.*$',
         'contentstore.views.update_checklist', name='checklists_updates'),
@@ -125,22 +124,37 @@ urlpatterns += (
     url(r'^logout$', 'student.views.logout_user', name='logout'),
 )
 
+# restful api
+urlpatterns += patterns(
+    'contentstore.views',
+    # index page, course outline page, and course structure json access
+    # replaces url(r'^listing', 'contentstore.views.index', name='index'),
+    # ? url(r'^create_new_course', 'contentstore.views.create_new_course', name='create_new_course')
+    # TODO remove shim and this pattern once import_export and test_contentstore no longer use
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$',
+        'course.old_course_index_shim', name='course_index'
+    ),
+
+    url(r'^course$', 'index'),
+    url(r'^course/(?P<course_url>.*)$', 'course_handler'),
+)
+
 js_info_dict = {
     'domain': 'djangojs',
     'packages': ('cms',),
 }
 
-urlpatterns += (
+urlpatterns += patterns('',
     # Serve catalog of localized strings to be rendered by Javascript
     url(r'^i18n.js$', 'django.views.i18n.javascript_catalog', js_info_dict),
 )
 
 if settings.MITX_FEATURES.get('ENABLE_SERVICE_STATUS'):
-    urlpatterns += (
+    urlpatterns += patterns('',
         url(r'^status/', include('service_status.urls')),
     )
 
-urlpatterns += (url(r'^admin/', include(admin.site.urls)),)
+urlpatterns += patterns('', url(r'^admin/', include(admin.site.urls)),)
 
 # enable automatic login
 if settings.MITX_FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING'):
@@ -155,8 +169,6 @@ if settings.DEBUG:
     except ImportError:
         pass
 
-urlpatterns = patterns(*urlpatterns)
-
 # Custom error pages
 #pylint: disable=C0103
 handler404 = 'contentstore.views.render_404'

common/lib/xmodule/xmodule/modulestore/django.py

@@ -141,6 +141,9 @@ def clear_existing_modulestores():
     This is useful for flushing state between unit tests.
     """
     _MODULESTORES.clear()
+    # pylint: disable=W0603
+    global _loc_singleton
+    _loc_singleton = None
 
 
 def editable_modulestore(name='default'):

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

@@ -56,7 +56,7 @@ class LocMapperStore(object):
         """
         Add a new entry to map this course_location to the new style CourseLocator.course_id. If course_id is not
         provided, it creates the default map of using org.course.name from the location (just like course_id) if
-        the location.cateogry = 'course'; otherwise, it uses org.course.
+        the location.category = 'course'; otherwise, it uses org.course.
 
         You can create more than one mapping to the
         same course_id target. In that case, the reverse translate will be arbitrary (no guarantee of which wins).

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

@@ -14,6 +14,8 @@ from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverS
 
 from .parsers import parse_url, parse_course_id, parse_block_ref
 from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, URL_VERSION_PREFIX
+import re
+from xmodule.modulestore import Location
 
 log = logging.getLogger(__name__)
 
@@ -89,6 +91,59 @@ class Locator(object):
                                          (property_name, current, new))
         setattr(self, property_name, new)
 
+    @staticmethod
+    def to_locator_or_location(location):
+        """
+        Convert the given locator like thing to the appropriate type of object, or, if already
+        that type, just return it. Returns an old Location, BlockUsageLocator,
+        or DefinitionLocator.
+
+        :param location: can be a Location, Locator, string, tuple, list, or dict.
+        """
+        if isinstance(location, (Location, Locator)):
+            return location
+        if isinstance(location, basestring):
+            return Locator.parse_url(location)
+        if isinstance(location, (list, tuple)):
+            return Location(location)
+        if isinstance(location, dict) and 'name' in location:
+            return Location(location)
+        if isinstance(location, dict):
+            return BlockUsageLocator(**location)
+        raise ValueError(location)
+
+    URL_TAG_RE = re.compile(r'^(\w+)://')
+    @staticmethod
+    def parse_url(url):
+        """
+        Parse the url into one of the Locator types (must have a tag indicating type)
+        Return the new instance. Supports i4x, cvx, edx, defx
+
+        :param url: the url to parse
+        """
+        parsed = Locator.URL_TAG_RE.match(url)
+        if parsed is None:
+            raise ValueError(parsed)
+        parsed = parsed.group(1)
+        if parsed in ['i4x', 'c4x']:
+            return Location(url)
+        elif parsed == 'edx':
+            return BlockUsageLocator(url)
+        elif parsed == 'defx':
+            return DefinitionLocator(url)
+        return None
+
+    @classmethod
+    def as_object_id(cls, value):
+        """
+        Attempts to cast value as a bson.objectid.ObjectId.
+        If cast fails, raises ValueError
+        """
+        try:
+            return ObjectId(value)
+        except InvalidId:
+            raise ValueError('"%s" is not a valid version_guid' % value)
+
 
 class CourseLocator(Locator):
     """
@@ -208,18 +263,55 @@ class CourseLocator(Locator):
                              version_guid=self.version_guid,
                              branch=self.branch)
 
-    @classmethod
-    def as_object_id(cls, value):
+    OLD_COURSE_ID_RE = re.compile(r'^(?P<org>[^.]+)\.?(?P<old_course_id>.+)?\.(?P<run>[^.]+)$')
+    @property
+    def as_old_location_course_id(self):
         """
-        Attempts to cast value as a bson.objectid.ObjectId.
-        If cast fails, raises ValueError
+        The original Location type presented its course id as org/course/run. This function
+        assumes the course_id starts w/ org, has an arbitrarily long 'course' identifier, and then
+        ends w/ run all separated by periods.
+
+        If this object does not have a course_id, this function returns None.
         """
-        if isinstance(value, ObjectId):
-            return value
-        try:
-            return ObjectId(value)
-        except InvalidId:
-            raise ValueError('"%s" is not a valid version_guid' % value)
+        if self.course_id is None:
+            return None
+        parsed = self.OLD_COURSE_ID_RE.match(self.course_id)
+        # check whether there are 2 or > 2 'fields'
+        if parsed.group('old_course_id'):
+            return '/'.join(parsed.groups())
+        else:
+            return parsed.group('org') + '/' + parsed.group('run')
+
+    def _old_location_field_helper(self, field):
+        """
+        Parse course_id to get the old location field named field out
+        """
+        if self.course_id is None:
+            return None
+        parsed = self.OLD_COURSE_ID_RE.match(self.course_id)
+        return parsed.group(field)
+
+    @property
+    def as_old_location_org(self):
+        """
+        Presume the first part of the course_id is the org and return it.
+        """
+        return self._old_location_field_helper('org')
+
+    @property
+    def as_old_location_course(self):
+        """
+        Presume the middle part, if any, of the course_id is the old location scheme's
+        course id and return it.
+        """
+        return self._old_location_field_helper('old_course_id')
+
+    @property
+    def as_old_location_run(self):
+        """
+        Presume the last part of the course_id is the old location scheme's run and return it.
+        """
+        return self._old_location_field_helper('run')
 
     def init_from_url(self, url):
         """
@@ -230,7 +322,7 @@ class CourseLocator(Locator):
             url = url.url()
         if not isinstance(url, basestring):
             raise TypeError('%s is not an instance of basestring' % url)
-        parse = parse_url(url)
+        parse = parse_url(url, tag_optional=True)
         if not parse:
             raise ValueError('Could not parse "%s" as a url' % url)
         self._set_value(
@@ -349,7 +441,7 @@ class BlockUsageLocator(CourseLocator):
         """
         self._validate_args(url, version_guid, course_id)
         if url:
-            self.init_block_ref_from_url(url)
+            self.init_block_ref_from_str(url)
         if course_id:
             self.init_block_ref_from_course_id(course_id)
         if usage_id:
@@ -401,11 +493,18 @@ class BlockUsageLocator(CourseLocator):
                 raise ValueError('Could not parse "%s" as a block_ref' % block_ref)
             self.set_usage_id(parse['block'])
 
-    def init_block_ref_from_url(self, url):
-        if isinstance(url, Locator):
-            url = url.url()
-        parse = parse_url(url)
-        assert parse, 'Could not parse "%s" as a url' % url
+    def init_block_ref_from_str(self, value):
+        """
+        Create a block locator from the given string which may be a url or just the repr (no tag)
+        """
+        if hasattr(value, 'usage_id'):
+            self.init_block_ref(value.usage_id)
+            return
+        if not isinstance(value, basestring):
+            return None
+        parse = parse_url(value, tag_optional=True)
+        if parse is None:
+            raise ValueError('Could not parse "%s" as a url' % value)
         self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
 
     def init_block_ref_from_course_id(self, course_id):
@@ -429,8 +528,13 @@ class DefinitionLocator(Locator):
     Container for how to locate a description (the course-independent content).
     """
 
+    URL_RE = re.compile(r'^defx://' + URL_VERSION_PREFIX + '([^/]+)$', re.IGNORECASE)
     def __init__(self, definition_id):
-        self.definition_id = definition_id
+        if isinstance(definition_id, basestring):
+            regex_match = self.URL_RE.match(definition_id)
+            if regex_match is not None:
+                definition_id = self.as_object_id(regex_match.group(1))
+        self.definition_id = self.as_object_id(definition_id)
 
     def __unicode__(self):
         '''
@@ -442,9 +546,9 @@ class DefinitionLocator(Locator):
     def url(self):
         """
         Return a string containing the URL for this location.
-        url(self) returns something like this: 'edx://version/519665f6223ebd6980884f2b'
+        url(self) returns something like this: 'defx://version/519665f6223ebd6980884f2b'
         """
-        return 'edx://' + unicode(self)
+        return 'defx://' + unicode(self)
 
     def version(self):
         """

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

@@ -9,13 +9,14 @@ VERSION_PREFIX = "/version/"
 # Prefix for version when it begins the URL (no course ID).
 URL_VERSION_PREFIX = 'version/'
 
-URL_RE = re.compile(r'^edx://(.+)$', re.IGNORECASE)
+URL_RE = re.compile(r'^(edx://)?(.+)$', re.IGNORECASE)
 
 
-def parse_url(string):
+def parse_url(string, tag_optional=False):
     """
-    A url must begin with 'edx://' (case-insensitive match),
-    followed by either a version_guid or a course_id.
+    A url usually begins with 'edx://' (case-insensitive match),
+    followed by either a version_guid or a course_id. If tag_optional, then
+    the url does not have to start with the tag and edx will be assumed.
 
     Examples:
         'edx://version/0123FFFF'
@@ -36,7 +37,9 @@ def parse_url(string):
     match = URL_RE.match(string)
     if not match:
         return None
-    path = match.group(1)
+    if match.group(1) is None and not tag_optional:
+        return None
+    path = match.group(2)
     if path.startswith(URL_VERSION_PREFIX):
         return parse_guid(path[len(URL_VERSION_PREFIX):])
     return parse_course_id(path)

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

@@ -7,6 +7,8 @@ from bson.objectid import ObjectId
 from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DefinitionLocator
 from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX, URL_VERSION_PREFIX
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError
+from xmodule.modulestore import Location
+import random
 
 
 class LocatorTest(TestCase):
@@ -98,7 +100,6 @@ class LocatorTest(TestCase):
         for bad_url in ('edx://',
                         'edx:/mit.eecs',
                         'http://mit.eecs',
-                        'mit.eecs',
                         'edx//mit.eecs'):
             self.assertRaises(ValueError, CourseLocator, url=bad_url)
 
@@ -253,13 +254,62 @@ class LocatorTest(TestCase):
         testobj = BlockUsageLocator(course_id=testurn)
         self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj))
 
+    def test_old_location_helpers(self):
+        """
+        Test the functions intended to help with the conversion from old locations to locators
+        """
+        location_tuple = ('i4x', 'mit', 'eecs.6002x', 'course', 't3_2013')
+        location = Location(location_tuple)
+        self.assertEqual(location, Locator.to_locator_or_location(location))
+        self.assertEqual(location, Locator.to_locator_or_location(location_tuple))
+        self.assertEqual(location, Locator.to_locator_or_location(list(location_tuple)))
+        self.assertEqual(location, Locator.to_locator_or_location(location.dict()))
+
+        locator = BlockUsageLocator(course_id='foo.bar', branch='alpha', usage_id='deep')
+        self.assertEqual(locator, Locator.to_locator_or_location(locator))
+        self.assertEqual(locator.as_course_locator(), Locator.to_locator_or_location(locator.as_course_locator()))
+        self.assertEqual(location, Locator.to_locator_or_location(location.url()))
+        self.assertEqual(locator, Locator.to_locator_or_location(locator.url()))
+        self.assertEqual(locator, Locator.to_locator_or_location(locator.__dict__))
+
+        asset_location = Location(['c4x', 'mit', 'eecs.6002x', 'asset', 'selfie.jpeg'])
+        self.assertEqual(asset_location, Locator.to_locator_or_location(asset_location))
+        self.assertEqual(asset_location, Locator.to_locator_or_location(asset_location.url()))
+
+        def_location_url = "defx://version/" + '{:024x}'.format(random.randrange(16 ** 24))
+        self.assertEqual(DefinitionLocator(def_location_url), Locator.to_locator_or_location(def_location_url))
+
+        with self.assertRaises(ValueError):
+            Locator.to_locator_or_location(22)
+        with self.assertRaises(ValueError):
+            Locator.to_locator_or_location("hello.world.not.a.url")
+        self.assertIsNone(Locator.parse_url("unknown://foo.bar/baz"))
+
+    def test_as_old(self):
+        """
+        Test the as_old_location_xxx accessors
+        """
+        locator = CourseLocator(course_id='org.course.id.run', branch='mybranch')
+        self.assertEqual('org', locator.as_old_location_org)
+        self.assertEqual('course.id', locator.as_old_location_course)
+        self.assertEqual('run', locator.as_old_location_run)
+        self.assertEqual('org/course.id/run', locator.as_old_location_course_id)
+        locator = CourseLocator(course_id='org.course', branch='mybranch')
+        self.assertEqual('org', locator.as_old_location_org)
+        self.assertIsNone(locator.as_old_location_course)
+        self.assertEqual('course', locator.as_old_location_run)
+        self.assertEqual('org/course', locator.as_old_location_course_id)
+
     def test_description_locator_url(self):
-        definition_locator = DefinitionLocator("chapter12345_2")
-        self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url())
+        object_id = '{:024x}'.format(random.randrange(16 ** 24))
+        definition_locator = DefinitionLocator(object_id)
+        self.assertEqual('defx://' + URL_VERSION_PREFIX + object_id, definition_locator.url())
+        self.assertEqual(definition_locator, DefinitionLocator(definition_locator.url()))
 
     def test_description_locator_version(self):
-        definition_locator = DefinitionLocator("chapter12345_2")
-        self.assertEqual("chapter12345_2", definition_locator.version())
+        object_id = '{:024x}'.format(random.randrange(16 ** 24))
+        definition_locator = DefinitionLocator(object_id)
+        self.assertEqual(object_id, str(definition_locator.version()))
 
     # ------------------------------------------------------------------
     # Utilities

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

@@ -135,7 +135,7 @@ class SplitModuleCourseTests(SplitModuleTest):
         self.assertEqual(
             len(course.children), 3,
             "children")
-        self.assertEqual(course.definition_locator.definition_id, "head12345_12")
+        self.assertEqual(str(course.definition_locator.definition_id), "ad00000000000000dddd0000")
         # check dates and graders--forces loading of descriptor
         self.assertEqual(course.edited_by, "testassist@edx.org")
         self.assertEqual(str(course.previous_version), self.GUID_D1)
@@ -195,7 +195,7 @@ class SplitModuleCourseTests(SplitModuleTest):
         self.assertEqual(course.graceperiod, datetime.timedelta(hours=2))
         self.assertIsNone(course.advertised_start)
         self.assertEqual(len(course.children), 0)
-        self.assertEqual(course.definition_locator.definition_id, "head12345_11")
+        self.assertEqual(str(course.definition_locator.definition_id), "ad00000000000000dddd0001")
         # check dates and graders--forces loading of descriptor
         self.assertEqual(course.edited_by, "testassist@edx.org")
         self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.55})
@@ -345,7 +345,7 @@ class SplitModuleItemTests(SplitModuleTest):
             self.assertEqual(block.display_name, "The Ancient Greek Hero")
             self.assertEqual(block.advertised_start, "Fall 2013")
             self.assertEqual(len(block.children), 3)
-            self.assertEqual(block.definition_locator.definition_id, "head12345_12")
+            self.assertEqual(str(block.definition_locator.definition_id), "ad00000000000000dddd0000")
             # check dates and graders--forces loading of descriptor
             self.assertEqual(block.edited_by, "testassist@edx.org")
             self.assertDictEqual(
@@ -375,7 +375,7 @@ class SplitModuleItemTests(SplitModuleTest):
         block = modulestore().get_item(locator)
         self.assertEqual(block.location.course_id, "GreekHero")
         self.assertEqual(block.category, 'chapter')
-        self.assertEqual(block.definition_locator.definition_id, "chapter12345_1")
+        self.assertEqual(str(block.definition_locator.definition_id), "cd00000000000000dddd0020")
         self.assertEqual(block.display_name, "Hercules")
         self.assertEqual(block.edited_by, "testassist@edx.org")
 
@@ -562,13 +562,13 @@ class TestItemCrud(SplitModuleTest):
         new_module = modulestore().create_item(
             locator, category, 'user123',
             fields={'display_name': 'new chapter'},
-            definition_locator=DefinitionLocator("chapter12345_2")
+            definition_locator=DefinitionLocator("cd00000000000000dddd0022")
         )
         # check that course version changed and course's previous is the other one
         self.assertNotEqual(new_module.location.version_guid, premod_course.location.version_guid)
         parent = modulestore().get_item(locator)
         self.assertIn(new_module.location.usage_id, parent.children)
-        self.assertEqual(new_module.definition_locator.definition_id, "chapter12345_2")
+        self.assertEqual(str(new_module.definition_locator.definition_id), "cd00000000000000dddd0022")
 
     def test_unique_naming(self):
         """
@@ -588,7 +588,7 @@ class TestItemCrud(SplitModuleTest):
         another_module = modulestore().create_item(
             locator, category, 'anotheruser',
             fields={'display_name': 'problem 2', 'data': another_payload},
-            definition_locator=DefinitionLocator("problem12345_3_1"),
+            definition_locator=DefinitionLocator("0d00000040000000dddd0031"),
         )
         # check that course version changed and course's previous is the other one
         parent = modulestore().get_item(locator)
@@ -605,7 +605,7 @@ class TestItemCrud(SplitModuleTest):
         self.assertLessEqual(new_history['edited_on'], datetime.datetime.now(UTC))
         self.assertGreaterEqual(new_history['edited_on'], premod_time)
         another_history = modulestore().get_definition_history_info(another_module.definition_locator)
-        self.assertEqual(another_history['previous_version'], 'problem12345_3_1')
+        self.assertEqual(str(another_history['previous_version']), '0d00000040000000dddd0031')
 
     def test_create_continue_version(self):
         """
@@ -789,7 +789,7 @@ class TestItemCrud(SplitModuleTest):
         modulestore().create_item(
             locator, category, 'test_update_manifold',
             fields={'display_name': 'problem 2', 'data': another_payload},
-            definition_locator=DefinitionLocator("problem12345_3_1"),
+            definition_locator=DefinitionLocator("0d00000040000000dddd0031"),
         )
         # pylint: disable=W0212
         modulestore()._clear_cache()

common/test/data/splitmongo_json/definitions.json

 [
     {
-        "_id":"head12345_12",
+        "_id": { "$oid" : "ad00000000000000dddd0000"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -46,12 +46,12 @@
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364481713238},
-            "previous_version":"head12345_11",
-            "original_version":"head12345_10"
+            "previous_version":{ "$oid" : "ad00000000000000dddd0001"},
+            "original_version":{ "$oid" : "ad00000000000000dddd0010"}
         }
     },
     {
-        "_id":"head12345_11",
+        "_id":{ "$oid" : "ad00000000000000dddd0001"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -97,12 +97,12 @@
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364481713238},
-            "previous_version":"head12345_10",
-            "original_version":"head12345_10"
+            "previous_version":{ "$oid" : "ad00000000000000dddd0010"},
+            "original_version":{ "$oid" : "ad00000000000000dddd0010"}
         }
     },
     {
-        "_id":"head12345_10",
+        "_id":{ "$oid" : "ad00000000000000dddd0010"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -149,11 +149,11 @@
             "edited_by":"test@edx.org",
             "edited_on":{"$date": 1364473713238},
             "previous_version":null,
-            "original_version":"head12345_10"
+            "original_version":{ "$oid" : "ad00000000000000dddd0010"}
         }
     },
     {
-        "_id":"head23456_1",
+        "_id":{ "$oid" : "ad00000000000000dddd0020"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -199,12 +199,12 @@
         "edit_info": {
             "edited_by":"test@edx.org",
             "edited_on":{"$date": 1364481313238},
-            "previous_version":"head23456_0",
-            "original_version":"head23456_0"
+            "previous_version":{ "$oid" : "2d00000000000000dddd0020"},
+            "original_version":{ "$oid" : "2d00000000000000dddd0020"}
         }
     },
     {
-        "_id":"head23456_0",
+        "_id":{ "$oid" : "2d00000000000000dddd0020"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -251,11 +251,11 @@
             "edited_by":"test@edx.org",
             "edited_on":{"$date" : 1364481313238},
             "previous_version":null,
-            "original_version":"head23456_0"
+            "original_version":{ "$oid" : "2d00000000000000dddd0020"}
         }
     },
     {
-        "_id":"head345679_1",
+        "_id":{ "$oid" : "3d00000000000000dddd0020"},
         "category":"course",
         "fields":{
             "textbooks":[
@@ -295,62 +295,62 @@
             "edited_by":"test@edx.org",
             "edited_on":{"$date" : 1364481313238},
             "previous_version":null,
-            "original_version":"head23456_0"
+            "original_version":{ "$oid" : "2d00000000000000dddd0020"}
         }
     },
     {
-        "_id":"chapter12345_1",
+        "_id":{ "$oid" : "cd00000000000000dddd0020"},
         "category":"chapter",
         "fields":{},
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364483713238},
             "previous_version":null,
-            "original_version":"chapter12345_1"
+            "original_version":{ "$oid" : "cd00000000000000dddd0020"}
         }
     },
     {
-        "_id":"chapter12345_2",
+        "_id":{ "$oid" : "cd00000000000000dddd0022"},
         "category":"chapter",
         "fields":{},
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364483713238},
             "previous_version":null,
-            "original_version":"chapter12345_2"
+            "original_version":{ "$oid" : "cd00000000000000dddd0022"}
         }
     },
     {
-        "_id":"chapter12345_3",
+        "_id":{ "$oid" : "cd00000000000000dddd0032"},
         "category":"chapter",
         "fields":{},
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364483713238},
             "previous_version":null,
-            "original_version":"chapter12345_3"
+            "original_version":{ "$oid" : "cd00000000000000dddd0032"}
         }
     },
     {
-        "_id":"problem12345_3_1",
+        "_id":{ "$oid" : "0d00000040000000dddd0031"},
         "category":"problem",
         "fields": {"data": ""},
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364483713238},
             "previous_version":null,
-            "original_version":"problem12345_3_1"
+            "original_version":{ "$oid" : "0d00000040000000dddd0031"}
         }
     },
     {
-        "_id":"problem12345_3_2",
+        "_id":{ "$oid" : "0d00000040000000dddd0032"},
         "category":"problem",
         "fields": {"data": ""},
         "edit_info": {
             "edited_by":"testassist@edx.org",
             "edited_on":{"$date" : 1364483713238},
             "previous_version":null,
-            "original_version":"problem12345_3_2"
+            "original_version":{ "$oid" : "0d00000040000000dddd0032"}
         }
     }
 ]
\ No newline at end of file

common/test/data/splitmongo_json/structures.json

@@ -11,7 +11,7 @@
         "blocks":{
             "head12345":{
                 "category":"course",
-                "definition":"head12345_12",
+                "definition":{ "$oid" : "ad00000000000000dddd0000"},
                 "fields":{
                     "children":[
                         "chapter1",
@@ -65,7 +65,7 @@
             },
             "chapter1":{
                 "category":"chapter",
-                "definition":"chapter12345_1",
+                "definition":{ "$oid" : "cd00000000000000dddd0020"},
                 "fields":{
                     "children":[
                         
@@ -83,7 +83,7 @@
             },
             "chapter2":{
                 "category":"chapter",
-                "definition":"chapter12345_2",
+                "definition":{ "$oid" : "cd00000000000000dddd0022"},
                 "fields":{
                     "children":[
                         
@@ -101,7 +101,7 @@
             },
             "chapter3":{
                 "category":"chapter",
-                "definition":"chapter12345_3",
+                "definition":{ "$oid" : "cd00000000000000dddd0032"},
                 "fields":{
                     "children":[
                         "problem1",
@@ -120,7 +120,7 @@
             },
             "problem1":{
                 "category":"problem",
-                "definition":"problem12345_3_1",
+                "definition":{ "$oid" : "0d00000040000000dddd0031"},
                 "fields":{
                     "children":[
                         
@@ -139,7 +139,7 @@
             },
             "problem3_2":{
                 "category":"problem",
-                "definition":"problem12345_3_2",
+                "definition":{ "$oid" : "0d00000040000000dddd0032"},
                 "fields":{
                     "children":[
                         
@@ -169,7 +169,7 @@
         "blocks":{
             "head12345":{
                 "category":"course",
-                "definition":"head12345_11",
+                "definition":{ "$oid" : "ad00000000000000dddd0001"},
                 "fields":{
                     "children":[
                         
@@ -233,7 +233,7 @@
         "blocks":{
             "head12345":{
                 "category":"course",
-                "definition":"head12345_10",
+                "definition":{ "$oid" : "ad00000000000000dddd0010"},
                 "fields":{
                     "children":[
                         
@@ -287,7 +287,7 @@
         "blocks":{
             "head23456":{
                 "category":"course",
-                "definition":"head23456_1",
+                "definition":{ "$oid" : "ad00000000000000dddd0020"},
                 "fields":{
                     "children":[
                         
@@ -342,7 +342,7 @@
         "blocks":{
             "head23456":{
                 "category":"course",
-                "definition":"head23456_0",
+                "definition":{ "$oid" : "2d00000000000000dddd0020"},
                 "fields":{
                     "children":[
                         
@@ -396,7 +396,7 @@
         "blocks":{
             "head23456":{
                 "category":"course",
-                "definition":"head23456_1",
+                "definition":{ "$oid" : "ad00000000000000dddd0020"},
                 "fields":{
                     "children":[
                         
@@ -450,7 +450,7 @@
         "blocks":{
             "head345679":{
                 "category":"course",
-                "definition":"head345679_1",
+                "definition":{ "$oid" : "3d00000000000000dddd0020"},
                 "fields":{
                     "children":[