Merge pull request #1980 from edx/dhm/locator_block_id

Rename BlockUsageLocator's block_id field from usage_id

cms/djangoapps/contentstore/tests/test_crud.py

@@ -64,7 +64,7 @@ class TemplateTests(unittest.TestCase):
         self.assertIsInstance(test_chapter, SequenceDescriptor)
         # refetch parent which should now point to child
         test_course = modulestore('split').get_course(test_chapter.location)
-        self.assertIn(test_chapter.location.usage_id, test_course.children)
+        self.assertIn(test_chapter.location.block_id, test_course.children)
 
     def test_temporary_xblocks(self):
         """
@@ -95,15 +95,20 @@ class TemplateTests(unittest.TestCase):
         """
         try saving temporary xblocks
         """
-        test_course = persistent_factories.PersistentCourseFactory.create(org='testx', prettyid='tempcourse',
+        test_course = persistent_factories.PersistentCourseFactory.create(
+            org='testx', prettyid='tempcourse',
             display_name='fun test course', user_id='testbot')
         test_chapter = self.load_from_json({'category': 'chapter',
             'fields': {'display_name': 'chapter n'}},
             test_course.system, parent_xblock=test_course)
         test_def_content = '<problem>boo</problem>'
         # create child
-        _ = self.load_from_json({'category': 'problem',
-            'fields': {'data': test_def_content}},
+        self.load_from_json({
+            'category': 'problem',
+            'fields': {
+                'data': test_def_content,
+                'display_name': 'problem'
+            }},
             test_course.system, parent_xblock=test_chapter)
         # better to pass in persisted parent over the subdag so
         # subdag gets the parent pointer (otherwise 2 ops, persist dag, update parent children,
@@ -117,6 +122,10 @@ class TemplateTests(unittest.TestCase):
         persisted_problem = persisted_chapter.get_children()[0]
         self.assertEqual(persisted_problem.category, 'problem')
         self.assertEqual(persisted_problem.data, test_def_content)
+        # update it
+        persisted_problem.display_name = 'altered problem'
+        persisted_problem = modulestore('split').persist_xblock_dag(persisted_problem, 'testbot')
+        self.assertEqual(persisted_problem.display_name, 'altered problem')
 
     def test_delete_course(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
@@ -166,7 +175,7 @@ class TemplateTests(unittest.TestCase):
 
         second_problem = persistent_factories.ItemFactory.create(
             display_name='problem 2',
-            parent_location=BlockUsageLocator(updated_loc, usage_id=sub.location.usage_id),
+            parent_location=BlockUsageLocator(updated_loc, block_id=sub.location.block_id),
             user_id='testbot', category='problem',
             data="<problem></problem>"
         )

cms/djangoapps/contentstore/views/assets.py

@@ -51,7 +51,7 @@ def assets_handler(request, tag=None, course_id=None, branch=None, version_guid=
     DELETE
         json: delete an asset
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 

cms/djangoapps/contentstore/views/checklist.py

@@ -36,7 +36,7 @@ def checklists_handler(request, tag=None, course_id=None, branch=None, version_g
     POST or PUT
         json: updates the checked state for items within a particular checklist. checklist_index is required.
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 

cms/djangoapps/contentstore/views/component.py

@@ -62,7 +62,7 @@ def subsection_handler(request, tag=None, course_id=None, branch=None, version_g
         json: not currently supported
     """
     if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):
-        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
         try:
             old_location, course, item, lms_link = _get_item_in_course(request, locator)
         except ItemNotFoundError:
@@ -151,7 +151,7 @@ def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=No
         json: not currently supported
     """
     if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):
-        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
         try:
             old_location, course, item, lms_link = _get_item_in_course(request, locator)
         except ItemNotFoundError:

cms/djangoapps/contentstore/views/course.py

@@ -11,7 +11,7 @@ from django.utils.translation import ugettext as _
 from django.contrib.auth.decorators import login_required
 from django_future.csrf import ensure_csrf_cookie
 from django.conf import settings
-from django.views.decorators.http import require_http_methods, require_POST
+from django.views.decorators.http import require_http_methods
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseBadRequest, HttpResponseNotFound
@@ -60,12 +60,12 @@ __all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler'
            'textbooks_list_handler', 'textbooks_detail_handler']
 
 
-def _get_locator_and_course(course_id, branch, version_guid, usage_id, user, depth=0):
+def _get_locator_and_course(course_id, branch, version_guid, block_id, user, depth=0):
     """
     Internal method used to calculate and return the locator and course module
     for the view functions in this file.
     """
-    locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=usage_id)
+    locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block_id)
     if not has_access(user, locator):
         raise PermissionDenied()
     course_location = loc_mapper().translate_locator_to_location(locator)
@@ -104,7 +104,7 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid=
             return create_new_course(request)
         elif not has_access(
             request.user,
-            BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+            BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
         ):
             raise PermissionDenied()
         elif request.method == 'PUT':
@@ -366,7 +366,7 @@ def course_info_update_handler(request, tag=None, course_id=None, branch=None, v
     """
     if 'application/json' not in request.META.get('HTTP_ACCEPT', 'application/json'):
         return HttpResponseBadRequest("Only supports json requests")
-    updates_locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    updates_locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     updates_location = loc_mapper().translate_locator_to_location(updates_locator)
     if provided_id == '':
         provided_id = None

cms/djangoapps/contentstore/views/import_export.py

@@ -60,7 +60,7 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid=
     POST or PUT
         json: import a course via the .tar.gz file specified in request.FILES
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 
@@ -271,7 +271,7 @@ def import_status_handler(request, tag=None, course_id=None, branch=None, versio
         3 : Importing to mongo
 
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 
@@ -302,7 +302,7 @@ def export_handler(request, tag=None, course_id=None, branch=None, version_guid=
     If the tar.gz file has been requested but the export operation fails, an HTML page will be returned
     which describes the error.
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 

cms/djangoapps/contentstore/views/item.py

@@ -79,7 +79,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
               The locator (and old-style id) for the created xblock (minus children) is returned.
     """
     if course_id is not None:
-        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+        locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
         if not has_access(request.user, locator):
             raise PermissionDenied()
         old_location = loc_mapper().translate_locator_to_location(locator)
@@ -330,7 +330,7 @@ def orphan_handler(request, tag=None, course_id=None, branch=None, version_guid=
     :param request:
     :param course_id: Locator syntax course_id
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     # DHM: when split becomes back-end, move or conditionalize this conversion
     old_location = loc_mapper().translate_locator_to_location(location)
     if request.method == 'GET':

cms/djangoapps/contentstore/views/tabs.py

@@ -62,7 +62,7 @@ def tabs_handler(request, tag=None, course_id=None, branch=None, version_guid=No
     Creating a tab, deleting a tab, or changing its contents is not supported through this method.
     Instead use the general xblock URL (see item.xblock_handler).
     """
-    locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, locator):
         raise PermissionDenied()
 

cms/djangoapps/contentstore/views/user.py

@@ -52,7 +52,7 @@ def course_team_handler(request, tag=None, course_id=None, branch=None, version_
     DELETE:
         json: remove a particular course team member from the course team (email is required).
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block)
+    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 

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

@@ -6,7 +6,7 @@ import re
 import pymongo
 import bson.son
 
-from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError
+from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError
 from xmodule.modulestore.locator import BlockUsageLocator
 from xmodule.modulestore import Location
 import urllib
@@ -156,27 +156,27 @@ class LocMapperStore(object):
                     entry = item
                     break
 
-        usage_id = entry['block_map'].get(self._encode_for_mongo(location.name))
-        if usage_id is None:
+        block_id = entry['block_map'].get(self._encode_for_mongo(location.name))
+        if block_id is None:
             if add_entry_if_missing:
-                usage_id = self._add_to_block_map(location, location_id, entry['block_map'])
+                block_id = self._add_to_block_map(location, location_id, entry['block_map'])
             else:
                 raise ItemNotFoundError(location)
-        elif isinstance(usage_id, dict):
+        elif isinstance(block_id, dict):
             # name is not unique, look through for the right category
-            if location.category in usage_id:
-                usage_id = usage_id[location.category]
+            if location.category in block_id:
+                block_id = block_id[location.category]
             elif add_entry_if_missing:
-                usage_id = self._add_to_block_map(location, location_id, entry['block_map'])
+                block_id = self._add_to_block_map(location, location_id, entry['block_map'])
             else:
                 raise ItemNotFoundError()
         else:
             raise InvalidLocationError()
 
         published_usage = BlockUsageLocator(
-            course_id=entry['course_id'], branch=entry['prod_branch'], usage_id=usage_id)
+            course_id=entry['course_id'], branch=entry['prod_branch'], block_id=block_id)
         draft_usage = BlockUsageLocator(
-            course_id=entry['course_id'], branch=entry['draft_branch'], usage_id=usage_id)
+            course_id=entry['course_id'], branch=entry['draft_branch'], block_id=block_id)
         if published:
             result = published_usage
         else:
@@ -191,7 +191,7 @@ class LocMapperStore(object):
         Returns an old style Location for the given Locator if there's an appropriate entry in the
         mapping collection. Note, it requires that the course was previously mapped (a side effect of
         translate_location or explicitly via create_map_entry) and
-        the block's usage_id was previously stored in the
+        the block's block_id was previously stored in the
         map (a side effect of translate_location or via add|update_block_location).
 
         If get_course, then rather than finding the map for this locator, it finds the 'course' root
@@ -200,7 +200,7 @@ class LocMapperStore(object):
         If there are no matches, it returns None.
 
         If there's more than one location to locator mapping to the same course_id, it looks for the first
-        one with a mapping for the block usage_id and picks that arbitrary course location.
+        one with a mapping for the block block_id and picks that arbitrary course location.
 
         :param locator: a BlockUsageLocator
         """
@@ -214,14 +214,14 @@ class LocMapperStore(object):
         # This does not require that the course exist in any modulestore
         # only that it has a mapping entry.
         maps = self.location_map.find({'course_id': locator.course_id})
-        # look for one which maps to this block usage_id
+        # look for one which maps to this block block_id
         if maps.count() == 0:
             return None
         result = None
         for candidate in maps:
             old_course_id = self._generate_location_course_id(candidate['_id'])
             for old_name, cat_to_usage in candidate['block_map'].iteritems():
-                for category, usage_id in cat_to_usage.iteritems():
+                for category, block_id in cat_to_usage.iteritems():
                     # cache all entries and then figure out if we have the one we want
                     # Always return revision=None because the
                     # old draft module store wraps locations as draft before
@@ -234,16 +234,16 @@ class LocMapperStore(object):
                         self._decode_from_mongo(old_name),
                         None)
                     published_locator = BlockUsageLocator(
-                        candidate['course_id'], branch=candidate['prod_branch'], usage_id=usage_id
+                        candidate['course_id'], branch=candidate['prod_branch'], block_id=block_id
                     )
                     draft_locator = BlockUsageLocator(
-                        candidate['course_id'], branch=candidate['draft_branch'], usage_id=usage_id
+                        candidate['course_id'], branch=candidate['draft_branch'], block_id=block_id
                     )
                     self._cache_location_map_entry(old_course_id, location, published_locator, draft_locator)
                     
                     if get_course and category == 'course':
                         result = location
-                    elif not get_course and usage_id == locator.usage_id:
+                    elif not get_course and block_id == locator.block_id:
                         result = location
             if result is not None:
                 return result
@@ -256,13 +256,15 @@ class LocMapperStore(object):
             # The downside is that if there's more than one course mapped to from the same org/course root
             # the block ids will likely be out of sync and collide from an id perspective. HOWEVER,
             # if there are few == org/course roots or their content is unrelated, this will work well.
-            usage_id = self._verify_uniqueness(location.category + location.name[:3], block_map)
+            block_id = self._verify_uniqueness(location.category + location.name[:3], block_map)
         else:
-            usage_id = location.name
+            # if 2 different category locations had same name, then they'll collide. Make the later
+            # mapped ones unique
+            block_id = self._verify_uniqueness(location.name, block_map)
         encoded_location_name = self._encode_for_mongo(location.name)
-        block_map.setdefault(encoded_location_name, {})[location.category] = usage_id
+        block_map.setdefault(encoded_location_name, {})[location.category] = block_id
         self.location_map.update(location_id, {'$set': {'block_map': block_map}})
-        return usage_id
+        return block_id
 
     def _interpret_location_course_id(self, course_id, location):
         """

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

@@ -257,7 +257,7 @@ class CourseLocator(Locator):
         Returns a copy of itself (downcasting) as a CourseLocator.
         The copy has the same CourseLocator fields as the original.
         The copy does not include subclass information, such as
-        a usage_id (a property of BlockUsageLocator).
+        a block_id (a property of BlockUsageLocator).
         """
         return CourseLocator(course_id=self.course_id,
                              version_guid=self.version_guid,
@@ -298,8 +298,8 @@ class CourseLocator(Locator):
         self._set_value(
             parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid))
         )
-        self._set_value(parse, 'course_id', lambda (new_id): self.set_course_id(new_id))
-        self._set_value(parse, 'branch', lambda (new_branch): self.set_branch(new_branch))
+        self._set_value(parse, 'course_id', self.set_course_id)
+        self._set_value(parse, 'branch', self.set_branch)
 
     def init_from_version_guid(self, version_guid):
         """
@@ -390,23 +390,23 @@ class BlockUsageLocator(CourseLocator):
     """
 
     # Default value
-    usage_id = None
+    block_id = None
 
     def __init__(self, url=None, version_guid=None, course_id=None,
-                 branch=None, usage_id=None):
+                 branch=None, block_id=None):
         """
         Construct a BlockUsageLocator
         Caller may provide url, version_guid, or course_id, and optionally provide branch.
 
-        The usage_id may be specified, either explictly or as part of
+        The block_id may be specified, either explictly or as part of
         the url or course_id. If omitted, the locator is created but it
         has not yet been initialized.
 
-        Resulting BlockUsageLocator will have a usage_id property.
+        Resulting BlockUsageLocator will have a block_id property.
         It will have either a version_guid property or a course_id (with optional branch) property, or both.
 
         version_guid must be an instance of bson.objectid.ObjectId or None
-        url, course_id, branch, and usage_id must be strings or None
+        url, course_id, branch, and block_id must be strings or None
 
         """
         self._validate_args(url, version_guid, course_id)
@@ -414,8 +414,8 @@ class BlockUsageLocator(CourseLocator):
             self.init_block_ref_from_str(url)
         if course_id:
             self.init_block_ref_from_course_id(course_id)
-        if usage_id:
-            self.init_block_ref(usage_id)
+        if block_id:
+            self.init_block_ref(block_id)
         super(BlockUsageLocator, self).__init__(
             url=url,
             version_guid=version_guid,
@@ -425,9 +425,9 @@ class BlockUsageLocator(CourseLocator):
 
     def is_initialized(self):
         """
-        Returns True if usage_id has been initialized, else returns False
+        Returns True if block_id has been initialized, else returns False
         """
-        return self.usage_id is not None
+        return self.block_id is not None
 
     def version_agnostic(self):
         """
@@ -442,58 +442,57 @@ class BlockUsageLocator(CourseLocator):
         if self.version_guid:
             return BlockUsageLocator(version_guid=self.version_guid,
                                      branch=self.branch,
-                                     usage_id=self.usage_id)
+                                     block_id=self.block_id)
         else:
             return BlockUsageLocator(course_id=self.course_id,
                                      branch=self.branch,
-                                     usage_id=self.usage_id)
+                                     block_id=self.block_id)
 
-    def set_usage_id(self, new):
+    def set_block_id(self, new):
         """
-        Initialize usage_id to new value.
-        If usage_id has already been initialized to a different value, raise an exception.
+        Initialize block_id to new value.
+        If block_id has already been initialized to a different value, raise an exception.
         """
-        self.set_property('usage_id', new)
+        self.set_property('block_id', new)
 
     def init_block_ref(self, block_ref):
         if isinstance(block_ref, LocalId):
-            self.set_usage_id(block_ref)
+            self.set_block_id(block_ref)
         else:
             parse = parse_block_ref(block_ref)
             if not parse:
                 raise ValueError('Could not parse "%s" as a block_ref' % block_ref)
-            self.set_usage_id(parse['block'])
+            self.set_block_id(parse['block'])
 
     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)
+        if hasattr(value, 'block_id'):
+            self.init_block_ref(value.block_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))
+        self._set_value(parse, 'block', self.set_block_id)
 
     def init_block_ref_from_course_id(self, course_id):
         if isinstance(course_id, CourseLocator):
-            # FIXME the parsed course_id should never contain a block ref
             course_id = course_id.course_id
             assert course_id, "%s does not have a valid course_id"
         parse = parse_course_id(course_id)
         if parse is None:
             raise ValueError('Could not parse "%s" as a course_id' % course_id)
-        self._set_value(parse, 'block', lambda(new_block): self.set_usage_id(new_block))
+        self._set_value(parse, 'block', self.set_block_id)
 
     def __unicode__(self):
         """
         Return a string representing this location.
         """
         rep = super(BlockUsageLocator, self).__unicode__()
-        return rep + '/' + BLOCK_PREFIX + unicode(self.usage_id)
+        return rep + '/' + BLOCK_PREFIX + unicode(self.block_id)
 
 
 class DefinitionLocator(Locator):

common/lib/xmodule/xmodule/modulestore/split_migrator.py

@@ -50,7 +50,7 @@ class SplitMigrator(object):
             course_location.org, original_course.display_name,
             user_id, id_root=new_course_id,
             fields=self._get_json_fields_translate_children(original_course, old_course_id, True),
-            root_usage_id=new_course_root_locator.usage_id,
+            root_block_id=new_course_root_locator.block_id,
             master_branch=new_course_root_locator.branch
         )
 
@@ -74,13 +74,13 @@ class SplitMigrator(object):
             # don't copy the course again. No drafts should get here but check
             if module.location != old_course_loc and not getattr(module, 'is_draft', False):
                 # create split_xblock using split.create_item
-                # where usage_id is computed by translate_location_to_locator
+                # where block_id is computed by translate_location_to_locator
                 new_locator = self.loc_mapper.translate_location(
                     old_course_id, module.location, True, add_entry_if_missing=True
                 )
                 _new_module = self.split_modulestore.create_item(
                     course_version_locator, module.category, user_id,
-                    usage_id=new_locator.usage_id,
+                    block_id=new_locator.block_id,
                     fields=self._get_json_fields_translate_children(module, old_course_id, True),
                     continue_version=True
                 )
@@ -130,18 +130,18 @@ class SplitMigrator(object):
                     # create a new course version just in case the current head is also the prod head
                     _new_module = self.split_modulestore.create_item(
                         new_draft_course_loc, module.category, user_id,
-                        usage_id=new_locator.usage_id,
+                        block_id=new_locator.block_id,
                         fields=self._get_json_fields_translate_children(module, old_course_id, True)
                     )
-                    awaiting_adoption[module.location] = new_locator.usage_id
-        for draft_location, new_usage_id in awaiting_adoption.iteritems():
+                    awaiting_adoption[module.location] = new_locator.block_id
+        for draft_location, new_block_id in awaiting_adoption.iteritems():
             for parent_loc in self.draft_modulestore.get_parent_locations(draft_location, old_course_id):
                 old_parent = self.draft_modulestore.get_item(parent_loc)
                 new_parent = self.split_modulestore.get_item(
                     self.loc_mapper.translate_location(old_course_id, old_parent.location, False)
                 )
                 # this only occurs if the parent was also awaiting adoption
-                if new_usage_id in new_parent.children:
+                if new_block_id in new_parent.children:
                     break
                 # find index for module: new_parent may be missing quite a few of old_parent's children
                 new_parent_cursor = 0
@@ -152,15 +152,15 @@ class SplitMigrator(object):
                     sibling_loc = self.loc_mapper.translate_location(old_course_id, Location(old_child_loc), False)
                     # sibling may move cursor
                     for idx in range(new_parent_cursor, len(new_parent.children)):
-                        if new_parent.children[idx] == sibling_loc.usage_id:
+                        if new_parent.children[idx] == sibling_loc.block_id:
                             new_parent_cursor = idx + 1
                             break
-                new_parent.children.insert(new_parent_cursor, new_usage_id)
+                new_parent.children.insert(new_parent_cursor, new_block_id)
                 new_parent = self.split_modulestore.update_item(new_parent, user_id)
 
     def _get_json_fields_translate_children(self, xblock, old_course_id, published):
         """
-        Return the json repr for explicitly set fields but convert all children to their usage_id's
+        Return the json repr for explicitly set fields but convert all children to their block_id's
         """
         fields = self.get_json_fields_explicitly_set(xblock)
         # this will too generously copy the children even for ones that don't exist in the published b/c the old mongo
@@ -169,7 +169,7 @@ class SplitMigrator(object):
             fields['children'] = [
                 self.loc_mapper.translate_location(
                     old_course_id, Location(child), published, add_entry_if_missing=True
-                ).usage_id
+                ).block_id
                 for child in fields['children']]
         return fields
 

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

@@ -47,26 +47,26 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         self.default_class = default_class
         self.local_modules = {}
 
-    def _load_item(self, usage_id, course_entry_override=None):
-        if isinstance(usage_id, BlockUsageLocator) and isinstance(usage_id.usage_id, LocalId):
+    def _load_item(self, block_id, course_entry_override=None):
+        if isinstance(block_id, BlockUsageLocator) and isinstance(block_id.block_id, LocalId):
             try:
-                return self.local_modules[usage_id]
+                return self.local_modules[block_id]
             except KeyError:
                 raise ItemNotFoundError
 
-        json_data = self.module_data.get(usage_id)
+        json_data = self.module_data.get(block_id)
         if json_data is None:
             # deeper than initial descendant fetch or doesn't exist
-            self.modulestore.cache_items(self, [usage_id], lazy=self.lazy)
-            json_data = self.module_data.get(usage_id)
+            self.modulestore.cache_items(self, [block_id], lazy=self.lazy)
+            json_data = self.module_data.get(block_id)
             if json_data is None:
-                raise ItemNotFoundError(usage_id)
+                raise ItemNotFoundError(block_id)
 
         class_ = XModuleDescriptor.load_class(
             json_data.get('category'),
             self.default_class
         )
-        return self.xblock_from_json(class_, usage_id, json_data, course_entry_override)
+        return self.xblock_from_json(class_, block_id, json_data, course_entry_override)
 
     # xblock's runtime does not always pass enough contextual information to figure out
     # which named container (course x branch) or which parent is requesting an item. Because split allows
@@ -79,7 +79,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     # low; thus, the course_entry is most likely correct. If the thread is looking at > 1 named container
     # pointing to the same structure, the access is likely to be chunky enough that the last known container
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course_id.
-    def xblock_from_json(self, class_, usage_id, json_data, course_entry_override=None):
+    def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None):
         if course_entry_override is None:
             course_entry_override = self.course_entry
         else:
@@ -91,12 +91,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         definition_id = self.modulestore.definition_locator(definition)
 
         # If no usage id is provided, generate an in-memory id
-        if usage_id is None:
-            usage_id = LocalId()
+        if block_id is None:
+            block_id = LocalId()
 
         block_locator = BlockUsageLocator(
             version_guid=course_entry_override['structure']['_id'],
-            usage_id=usage_id,
+            block_id=block_id,
             course_id=course_entry_override.get('course_id'),
             branch=course_entry_override.get('branch')
         )
@@ -121,7 +121,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 self,
                 BlockUsageLocator(
                     version_guid=course_entry_override['structure']['_id'],
-                    usage_id=usage_id
+                    block_id=block_id
                 ),
                 error_msg=exc_info_to_str(sys.exc_info())
             )
@@ -136,7 +136,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         module.save()
 
         # If this is an in-memory block, store it in this system
-        if isinstance(block_locator.usage_id, LocalId):
+        if isinstance(block_locator.block_id, LocalId):
             self.local_modules[block_locator] = module
 
         return module

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

@@ -75,10 +75,9 @@ class TestLocationMapper(unittest.TestCase):
         self.assertEqual(entry['prod_branch'], 'live')
         self.assertEqual(entry['block_map'], block_map)
 
-
-    def translate_n_check(self, location, old_style_course_id, new_style_course_id, usage_id, branch, add_entry=False):
+    def translate_n_check(self, location, old_style_course_id, new_style_course_id, block_id, branch, add_entry=False):
         """
-        Request translation, check course_id, usage_id, and branch
+        Request translation, check course_id, block_id, and branch
         """
         prob_locator = loc_mapper().translate_location(
             old_style_course_id, 
@@ -87,7 +86,7 @@ class TestLocationMapper(unittest.TestCase):
             add_entry_if_missing=add_entry
         )
         self.assertEqual(prob_locator.course_id, new_style_course_id)
-        self.assertEqual(prob_locator.usage_id, usage_id)
+        self.assertEqual(prob_locator.block_id, block_id)
         self.assertEqual(prob_locator.branch, branch)
 
     def test_translate_location_read_only(self):
@@ -243,7 +242,7 @@ class TestLocationMapper(unittest.TestCase):
         new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course)
         prob_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='problem2',
+            block_id='problem2',
             branch='published'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
@@ -267,21 +266,21 @@ class TestLocationMapper(unittest.TestCase):
         self.assertEqual(prob_location, Location('i4x', org, course, 'course', 'baz_run', None))
         # explicit branch
         prob_locator = BlockUsageLocator(
-            course_id=prob_locator.course_id, branch='draft', usage_id=prob_locator.usage_id
+            course_id=prob_locator.course_id, branch='draft', block_id=prob_locator.block_id
         )
         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('i4x', org, course, 'problem', 'abc123', None))
         prob_locator = BlockUsageLocator(
-            course_id=new_style_course_id, usage_id='problem2', branch='production'
+            course_id=new_style_course_id, block_id='problem2', branch='production'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123', None))
         # same for chapter except chapter cannot be draft in old system
         chap_locator = BlockUsageLocator(
             course_id=new_style_course_id,
-            usage_id='chapter48f',
+            block_id='chapter48f',
             branch='production'
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
@@ -291,7 +290,7 @@ class TestLocationMapper(unittest.TestCase):
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
         chap_locator = BlockUsageLocator(
-            course_id=new_style_course_id, usage_id='chapter48f', branch='production'
+            course_id=new_style_course_id, block_id='chapter48f', branch='production'
         )
         chap_location = loc_mapper().translate_locator_to_location(chap_locator)
         self.assertEqual(chap_location, Location('i4x', org, course, 'chapter', '48f23a10395384929234'))
@@ -300,7 +299,7 @@ class TestLocationMapper(unittest.TestCase):
         prob_locator2 = BlockUsageLocator(
             course_id=new_style_course_id,
             branch='draft',
-            usage_id='problem3'
+            block_id='problem3'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator2)
         self.assertIsNone(prob_location, 'Found non-existent problem')
@@ -325,7 +324,7 @@ class TestLocationMapper(unittest.TestCase):
         prob_locator = BlockUsageLocator(
             course_id=new_style_course_id,
             branch='production',
-            usage_id='problem3'
+            block_id='problem3'
         )
         prob_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(prob_location, Location('i4x', org, course, 'problem', 'abc123'))
@@ -348,6 +347,25 @@ class TestLocationMapper(unittest.TestCase):
         reverted_location = loc_mapper().translate_locator_to_location(prob_locator)
         self.assertEqual(location, reverted_location)
 
+    def test_name_collision(self):
+        """
+        Test dwim translation when the old name was not unique
+        """
+        org = "myorg"
+        course = "another_course"
+        name = "running_again"
+        course_location = Location('i4x', org, course, 'course', name)
+        course_xlate = loc_mapper().translate_location(None, course_location, add_entry_if_missing=True)
+        self.assertEqual(course_location, loc_mapper().translate_locator_to_location(course_xlate))
+        eponymous_block = course_location.replace(category='chapter')
+        chapter_xlate = loc_mapper().translate_location(None, eponymous_block, add_entry_if_missing=True)
+        self.assertEqual(course_location, loc_mapper().translate_locator_to_location(course_xlate))
+        self.assertEqual(eponymous_block, loc_mapper().translate_locator_to_location(chapter_xlate))
+        # and a non-existent one w/o add
+        eponymous_block = course_location.replace(category='problem')
+        with self.assertRaises(ItemNotFoundError):
+            chapter_xlate = loc_mapper().translate_location(None, eponymous_block, add_entry_if_missing=False)
+
 
 #==================================
 # functions to mock existing services

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

@@ -255,9 +255,9 @@ class LocatorTest(TestCase):
         """
         course_id = 'mit.eecs-1'
         branch = 'foo'
-        usage_id = 'problem:with-colon~2'
-        testobj = BlockUsageLocator(course_id=course_id, branch=branch, usage_id=usage_id)
-        self.check_block_locn_fields(testobj, 'Cannot handle colon', course_id=course_id, branch=branch, block=usage_id)
+        block_id = 'problem:with-colon~2'
+        testobj = BlockUsageLocator(course_id=course_id, branch=branch, block_id=block_id)
+        self.check_block_locn_fields(testobj, 'Cannot handle colon', course_id=course_id, branch=branch, block=block_id)
 
     def test_repr(self):
         testurn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published/' + BLOCK_PREFIX + 'HW3'
@@ -275,7 +275,7 @@ class LocatorTest(TestCase):
         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')
+        locator = BlockUsageLocator(course_id='foo.bar', branch='alpha', block_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()))
@@ -347,4 +347,4 @@ class LocatorTest(TestCase):
         """
         self.check_course_locn_fields(testobj, msg, version_guid, course_id,
                                       branch)
-        self.assertEqual(testobj.usage_id, block)
+        self.assertEqual(testobj.block_id, block)

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

@@ -87,14 +87,14 @@ class TestOrphan(unittest.TestCase):
             course_or_parent_locator = BlockUsageLocator(
                 course_id=self.split_course_id,
                 branch='draft',
-                usage_id=parent_name
+                block_id=parent_name
             )
         else:
             course_or_parent_locator = CourseLocator(
                 course_id='test_org.test_course.runid',
                 branch='draft',
             )
-        self.split_mongo.create_item(course_or_parent_locator, category, self.userid, usage_id=name, fields=fields)
+        self.split_mongo.create_item(course_or_parent_locator, category, self.userid, block_id=name, fields=fields)
 
     def _create_course(self):
         """
@@ -114,7 +114,7 @@ class TestOrphan(unittest.TestCase):
         fields.update(data)
         # split requires the course to be created separately from creating items
         self.split_mongo.create_course(
-            'test_org', 'my course', self.userid, self.split_course_id, fields=fields, root_usage_id='runid'
+            'test_org', 'my course', self.userid, self.split_course_id, fields=fields, root_block_id='runid'
         )
         self.course_location = Location('i4x', 'test_org', 'test_course', 'course', 'runid')
         self.old_mongo.create_and_save_xmodule(self.course_location, data, metadata)
@@ -150,9 +150,9 @@ class TestOrphan(unittest.TestCase):
         """
         orphans = self.split_mongo.get_orphans(self.split_course_id, ['static_tab', 'about', 'course_info'], 'draft')
         self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans))
-        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanChapter')
+        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', block_id='OrphanChapter')
         self.assertIn(location, orphans)
-        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanVert')
+        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', block_id='OrphanVert')
         self.assertIn(location, orphans)
-        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanHtml')
+        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', block_id='OrphanHtml')
         self.assertIn(location, orphans)

common/lib/xmodule/xmodule/x_module.py

@@ -168,7 +168,7 @@ class XModuleMixin(XBlockMixin):
         if isinstance(self.location, Location):
             return self.location.name
         elif isinstance(self.location, BlockUsageLocator):
-            return self.location.usage_id
+            return self.location.block_id
         else:
             raise InsufficientSpecificationError()