Merge pull request #2022 from edx/dhm/locator_course_id

Change course_id to package_id

cms/djangoapps/auth/authz.py

@@ -37,23 +37,21 @@ def get_all_course_role_groupnames(location, role, use_filter=True):
     '''
     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
     groupnames = []
-    try:
-        groupnames.append('{0}_{1}'.format(role, location.course_id))
-    except InvalidLocationError:  # will occur on old locations where location is not of category course
-        pass
     if isinstance(location, Location):
-        # least preferred role_course format
-        groupnames.append('{0}_{1}'.format(role, location.course))
+        try:
+            groupnames.append('{0}_{1}'.format(role, location.course_id))
+        except InvalidLocationError:  # will occur on old locations where location is not of category course
+            pass
         try:
             locator = loc_mapper().translate_location(location.course_id, location, False, False)
-            groupnames.append('{0}_{1}'.format(role, locator.course_id))
+            groupnames.append('{0}_{1}'.format(role, locator.package_id))
         except (InvalidLocationError, ItemNotFoundError):
             pass
+        # least preferred role_course format for legacy reasons
+        groupnames.append('{0}_{1}'.format(role, location.course))
     elif isinstance(location, CourseLocator):
+        groupnames.append('{0}_{1}'.format(role, location.package_id))
         old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
         if old_location:
             # the slashified version of the course_id (myu/mycourse/myrun)

cms/djangoapps/contentstore/tests/test_crud.py

@@ -136,13 +136,13 @@ class TemplateTests(unittest.TestCase):
         persistent_factories.ItemFactory.create(display_name='chapter 1',
             parent_location=test_course.location)
 
-        id_locator = CourseLocator(course_id=test_course.location.course_id, branch='draft')
+        id_locator = CourseLocator(package_id=test_course.location.package_id, branch='draft')
         guid_locator = CourseLocator(version_guid=test_course.location.version_guid)
         # verify it can be retireved by id
         self.assertIsInstance(modulestore('split').get_course(id_locator), CourseDescriptor)
         # and by guid
         self.assertIsInstance(modulestore('split').get_course(guid_locator), CourseDescriptor)
-        modulestore('split').delete_course(id_locator.course_id)
+        modulestore('split').delete_course(id_locator.package_id)
         # test can no longer retrieve by id
         self.assertRaises(ItemNotFoundError, modulestore('split').get_course, id_locator)
         # but can by guid

cms/djangoapps/contentstore/tests/test_permissions.py

@@ -90,7 +90,7 @@ class TestCourseAccess(ModuleStoreTestCase):
         # first check the groupname for the course creator.
         self.assertTrue(
             self.user.groups.filter(
-                name="{}_{}".format(INSTRUCTOR_ROLE_NAME, self.course_locator.course_id)
+                name="{}_{}".format(INSTRUCTOR_ROLE_NAME, self.course_locator.package_id)
             ).exists(),
             "Didn't add creator as instructor."
         )
@@ -124,4 +124,4 @@ class TestCourseAccess(ModuleStoreTestCase):
         for role in [INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME]:
             for user in user_by_role[role]:
                 self.assertTrue(has_access(user, copy_course_locator), "{} no copy access".format(user))
-                self.assertTrue(has_access(user, copy_course_location), "{} no copy access".format(user))
\ No newline at end of file
+                self.assertTrue(has_access(user, copy_course_location), "{} no copy access".format(user))

cms/djangoapps/contentstore/views/assets.py

@@ -34,7 +34,7 @@ __all__ = ['assets_handler']
 
 @login_required
 @ensure_csrf_cookie
-def assets_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, asset_id=None):
+def assets_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, asset_id=None):
     """
     The restful handler for assets.
     It allows retrieval of all the assets (as an HTML page), as well as uploading new assets,
@@ -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, block_id=block)
+    location = BlockUsageLocator(package_id=package_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

@@ -26,7 +26,7 @@ __all__ = ['checklists_handler']
 @require_http_methods(("GET", "POST", "PUT"))
 @login_required
 @ensure_csrf_cookie
-def checklists_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, checklist_index=None):
+def checklists_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, checklist_index=None):
     """
     The restful handler for checklists.
 
@@ -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, block_id=block)
+    location = BlockUsageLocator(package_id=package_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

@@ -56,7 +56,7 @@ ADVANCED_COMPONENT_POLICY_KEY = 'advanced_modules'
 
 @require_http_methods(["GET"])
 @login_required
-def subsection_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def subsection_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for subsection-specific requests.
 
@@ -65,7 +65,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, block_id=block)
+        locator = BlockUsageLocator(package_id=package_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:
@@ -145,7 +145,7 @@ def _load_mixed_class(category):
 
 @require_http_methods(["GET"])
 @login_required
-def unit_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def unit_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for unit-specific requests.
 
@@ -154,7 +154,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, block_id=block)
+        locator = BlockUsageLocator(package_id=package_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/import_export.py

@@ -50,7 +50,7 @@ CONTENT_RE = re.compile(r"(?P<start>\d{1,11})-(?P<stop>\d{1,11})/(?P<end>\d{1,11
 @login_required
 @ensure_csrf_cookie
 @require_http_methods(("GET", "POST", "PUT"))
-def import_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def import_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for importing a course.
 
@@ -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, block_id=block)
+    location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 
@@ -148,7 +148,7 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid=
 
                 # Use sessions to keep info about import progress
                 session_status = request.session.setdefault("import_status", {})
-                key = location.course_id + filename
+                key = location.package_id + filename
                 session_status[key] = 1
                 request.session.modified = True
 
@@ -261,7 +261,7 @@ def import_handler(request, tag=None, course_id=None, branch=None, version_guid=
 @require_GET
 @ensure_csrf_cookie
 @login_required
-def import_status_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, filename=None):
+def import_status_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, filename=None):
     """
     Returns an integer corresponding to the status of a file import. These are:
 
@@ -271,13 +271,13 @@ 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, block_id=block)
+    location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 
     try:
         session_status = request.session["import_status"]
-        status = session_status[location.course_id + filename]
+        status = session_status[location.package_id + filename]
     except KeyError:
         status = 0
 
@@ -287,7 +287,7 @@ def import_status_handler(request, tag=None, course_id=None, branch=None, versio
 @ensure_csrf_cookie
 @login_required
 @require_http_methods(("GET",))
-def export_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def export_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for exporting a course.
 
@@ -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, block_id=block)
+    location = BlockUsageLocator(package_id=package_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

@@ -47,7 +47,7 @@ CREATE_IF_NOT_FOUND = ['course_info']
 @require_http_methods(("DELETE", "GET", "PUT", "POST"))
 @login_required
 @expect_json
-def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for xblock requests.
 
@@ -78,8 +78,8 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
                 :boilerplate: template name for populating fields, optional
               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, block_id=block)
+    if package_id is not None:
+        locator = BlockUsageLocator(package_id=package_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)
@@ -117,7 +117,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
             delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False'))
 
             return _delete_item_at_location(old_location, delete_children, delete_all_versions)
-        else:  # Since we have a course_id, we are updating an existing xblock.
+        else:  # Since we have a package_id, we are updating an existing xblock.
             return _save_item(
                 request,
                 locator,
@@ -133,7 +133,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid=
         return _create_item(request)
     else:
         return HttpResponseBadRequest(
-            "Only instance creation is supported without a course_id.",
+            "Only instance creation is supported without a package_id.",
             content_type="text/plain"
         )
 
@@ -319,7 +319,7 @@ def _delete_item_at_location(item_location, delete_children=False, delete_all_ve
 # pylint: disable=W0613
 @login_required
 @require_http_methods(("GET", "DELETE"))
-def orphan_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     View for handling orphan related requests. GET gets all of the current orphans.
     DELETE removes all orphans (requires is_staff access)
@@ -328,9 +328,9 @@ def orphan_handler(request, tag=None, course_id=None, branch=None, version_guid=
     from the root via children
 
     :param request:
-    :param course_id: Locator syntax course_id
+    :param package_id: Locator syntax package_id
     """
-    location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, block_id=block)
+    location = BlockUsageLocator(package_id=package_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

@@ -48,7 +48,7 @@ def initialize_course_tabs(course):
 @login_required
 @ensure_csrf_cookie
 @require_http_methods(("GET", "POST", "PUT"))
-def tabs_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None):
+def tabs_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None):
     """
     The restful handler for static tabs.
 
@@ -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, block_id=block)
+    locator = BlockUsageLocator(package_id=package_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

@@ -40,7 +40,7 @@ def request_course_creator(request):
 @login_required
 @ensure_csrf_cookie
 @require_http_methods(("GET", "POST", "PUT", "DELETE"))
-def course_team_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, email=None):
+def course_team_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, email=None):
     """
     The restful handler for course team users.
 
@@ -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, block_id=block)
+    location = BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block)
     if not has_access(request.user, location):
         raise PermissionDenied()
 

common/lib/xmodule/xmodule/course_module.py

@@ -17,7 +17,6 @@ from xblock.fields import Scope, List, String, Dict, Boolean
 from .fields import Date
 from xmodule.modulestore.locator import CourseLocator
 from django.utils.timezone import UTC
-from xmodule.util import date_utils
 
 
 log = logging.getLogger(__name__)
@@ -400,7 +399,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
             if isinstance(self.location, Location):
                 self.wiki_slug = self.location.course
             elif isinstance(self.location, CourseLocator):
-                self.wiki_slug = self.location.course_id or self.display_name
+                self.wiki_slug = self.location.package_id or self.display_name
 
         if self.due_date_display_format is None and self.show_timezone is False:
             # For existing courses with show_timezone set to False (and no due_date_display_format specified),

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

@@ -51,21 +51,21 @@ class LocMapperStore(object):
         self.cache = cache
 
     # location_map functions
-    def create_map_entry(self, course_location, course_id=None, draft_branch='draft', prod_branch='published',
+    def create_map_entry(self, course_location, package_id=None, draft_branch='draft', prod_branch='published',
                          block_map=None):
         """
-        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
+        Add a new entry to map this course_location to the new style CourseLocator.package_id. If package_id is not
+        provided, it creates the default map of using org.course.name from the location if
         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).
+        same package_id target. In that case, the reverse translate will be arbitrary (no guarantee of which wins).
         The use
-        case for more than one mapping is to map both org/course/run and org/course to the same new course_id thus
+        case for more than one mapping is to map both org/course/run and org/course to the same new package_id thus
         making a default for org/course. When querying for just org/course, the translator will prefer any entry
         which does not have a name in the _id; otherwise, it will return an arbitrary match.
 
-        Note: the opposite is not true. That is, it never makes sense to use 2 different CourseLocator.course_id
+        Note: the opposite is not true. That is, it never makes sense to use 2 different CourseLocator.package_id
         keys to index the same old Locator org/course/.. pattern. There's no checking to ensure you don't do this.
 
         NOTE: if there's already an entry w the given course_location, this may either overwrite that entry or
@@ -74,8 +74,8 @@ class LocMapperStore(object):
         :param course_location: a Location preferably whose category is 'course'. Unlike the other
         map methods, this one doesn't take the old-style course_id.  It should be called with
         a course location not a block location; however, if called w/ a non-course Location, it creates
-        a "default" map for the org/course pair to a new course_id.
-        :param course_id: the CourseLocator style course_id
+        a "default" map for the org/course pair to a new package_id.
+        :param package_id: the CourseLocator style package_id
         :param draft_branch: the branch name to assign for drafts. This is hardcoded because old mongo had
         a fixed notion that there was 2 and only 2 versions for modules: draft and production. The old mongo
         did not, however, require that a draft version exist. The new one, however, does require a draft to
@@ -86,11 +86,11 @@ class LocMapperStore(object):
         :param block_map: an optional map to specify preferred names for blocks where the keys are the
         Location block names and the values are the BlockUsageLocator.block_id.
         """
-        if course_id is None:
+        if package_id is None:
             if course_location.category == 'course':
-                course_id = "{0.org}.{0.course}.{0.name}".format(course_location)
+                package_id = "{0.org}.{0.course}.{0.name}".format(course_location)
             else:
-                course_id = "{0.org}.{0.course}".format(course_location)
+                package_id = "{0.org}.{0.course}".format(course_location)
         # very like _interpret_location_id but w/o the _id
         location_id = self._construct_location_son(
             course_location.org, course_location.course, 
@@ -99,12 +99,12 @@ class LocMapperStore(object):
 
         self.location_map.insert({
             '_id': location_id,
-            'course_id': course_id,
+            'course_id': package_id,
             'draft_branch': draft_branch,
             'prod_branch': prod_branch,
             'block_map': block_map or {},
         })
-        return course_id
+        return package_id
 
     def translate_location(self, old_style_course_id, location, published=True, add_entry_if_missing=True):
         """
@@ -174,9 +174,9 @@ class LocMapperStore(object):
             raise InvalidLocationError()
 
         published_usage = BlockUsageLocator(
-            course_id=entry['course_id'], branch=entry['prod_branch'], block_id=block_id)
+            package_id=entry['course_id'], branch=entry['prod_branch'], block_id=block_id)
         draft_usage = BlockUsageLocator(
-            course_id=entry['course_id'], branch=entry['draft_branch'], block_id=block_id)
+            package_id=entry['course_id'], branch=entry['draft_branch'], block_id=block_id)
         if published:
             result = published_usage
         else:
@@ -199,13 +199,13 @@ 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
+        If there's more than one location to locator mapping to the same package_id, it looks for the first
         one with a mapping for the block block_id and picks that arbitrary course location.
 
         :param locator: a BlockUsageLocator
         """
         if get_course:
-            cached_value = self._get_course_location_from_cache(locator.course_id)
+            cached_value = self._get_course_location_from_cache(locator.package_id)
         else:
             cached_value = self._get_location_from_cache(locator)
         if cached_value:
@@ -213,7 +213,7 @@ 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})
+        maps = self.location_map.find({'course_id': locator.package_id})
         # look for one which maps to this block block_id
         if maps.count() == 0:
             return None
@@ -365,12 +365,12 @@ class LocMapperStore(object):
         """
         return self.cache.get(unicode(locator))
 
-    def _get_course_location_from_cache(self, locator_course_id):
+    def _get_course_location_from_cache(self, locator_package_id):
         """
-        See if the course_id is in the cache. If so, return the mapped location to the
+        See if the package_id is in the cache. If so, return the mapped location to the
         course root.
         """
-        return self.cache.get('courseId+{}'.format(locator_course_id))
+        return self.cache.get('courseId+{}'.format(locator_package_id))
 
     def _cache_location_map_entry(self, old_course_id, location, published_usage, draft_usage):
         """
@@ -380,7 +380,7 @@ class LocMapperStore(object):
         """
         setmany = {}
         if location.category == 'course':
-            setmany['courseId+{}'.format(published_usage.course_id)] = location
+            setmany['courseId+{}'.format(published_usage.package_id)] = location
         setmany[unicode(published_usage)] = location
         setmany[unicode(draft_usage)] = location
         setmany['{}+{}'.format(old_course_id, location.url())] = (published_usage, draft_usage)

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

@@ -11,7 +11,7 @@ ALLOWED_ID_CHARS = r'[a-zA-Z0-9_\-~.:]'
 
 URL_RE_SOURCE = r"""
     (?P<tag>edx://)?
-    ((?P<course_id>{ALLOWED_ID_CHARS}+)/?)?
+    ((?P<package_id>{ALLOWED_ID_CHARS}+)/?)?
     ({BRANCH_PREFIX}(?P<branch>{ALLOWED_ID_CHARS}+)/?)?
     ({VERSION_PREFIX}(?P<version_guid>[A-F0-9]+)/?)?
     ({BLOCK_PREFIX}(?P<block>{ALLOWED_ID_CHARS}+))?
@@ -26,7 +26,7 @@ URL_RE = re.compile('^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE)
 def parse_url(string, tag_optional=False):
     """
     A url usually begins with 'edx://' (case-insensitive match),
-    followed by either a version_guid or a course_id. If tag_optional, then
+    followed by either a version_guid or a package_id. If tag_optional, then
     the url does not have to start with the tag and edx will be assumed.
 
     Examples:
@@ -38,10 +38,10 @@ def parse_url(string, tag_optional=False):
 
     This returns None if string cannot be parsed.
 
-    If it can be parsed as a version_guid with no preceding course_id, returns a dict
+    If it can be parsed as a version_guid with no preceding package_id, returns a dict
     with key 'version_guid' and the value,
 
-    If it can be parsed as a course_id, returns a dict
+    If it can be parsed as a package_id, returns a dict
     with key 'id' and optional keys 'branch' and 'version_guid'.
 
     """
@@ -69,15 +69,15 @@ def parse_block_ref(string):
     return None
 
 
-def parse_course_id(string):
+def parse_package_id(string):
     r"""
 
-    A course_id has a main id component.
+    A package_id has a main id component.
     There may also be an optional branch (/branch/published or /branch/draft).
     There may also be an optional version (/version/519665f6223ebd6980884f2b).
     There may also be an optional block (/block/HW3 or /block/Quiz2).
 
-    Examples of valid course_ids:
+    Examples of valid package_ids:
 
       'mit.eecs.6002x'
       'mit.eecs.6002x/branch/published'
@@ -88,7 +88,7 @@ def parse_course_id(string):
 
     Syntax:
 
-      course_id = main_id [/branch/ branch] [/version/ version ] [/block/ block]
+      package_id = main_id [/branch/ branch] [/version/ version ] [/block/ block]
 
       main_id = name [. name]*
 
@@ -98,7 +98,7 @@ def parse_course_id(string):
 
       name = ALLOWED_ID_CHARS
 
-    If string is a course_id, returns a dict with keys 'id', 'branch', and 'block'.
+    If string is a package_id, returns a dict with keys 'id', 'branch', and 'block'.
     Revision is optional: if missing returned_dict['branch'] is None.
     Block is optional: if missing returned_dict['block'] is None.
     Else returns None.

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

@@ -23,41 +23,41 @@ class SplitMigrator(object):
         self.draft_modulestore = draft_modulestore
         self.loc_mapper = loc_mapper
 
-    def migrate_mongo_course(self, course_location, user_id, new_course_id=None):
+    def migrate_mongo_course(self, course_location, user_id, new_package_id=None):
         """
         Create a new course in split_mongo representing the published and draft versions of the course from the
-        original mongo store. And return the new_course_id (which the caller can also get by calling
+        original mongo store. And return the new_package_id (which the caller can also get by calling
         self.loc_mapper.translate_location(old_course_location)
 
         If the new course already exists, this raises DuplicateItemError
 
         :param course_location: a Location whose category is 'course' and points to the course
         :param user_id: the user whose action is causing this migration
-        :param new_course_id: (optional) the Locator.course_id for the new course. Defaults to
+        :param new_package_id: (optional) the Locator.package_id for the new course. Defaults to
         whatever translate_location_to_locator returns
         """
-        new_course_id = self.loc_mapper.create_map_entry(course_location, course_id=new_course_id)
+        new_package_id = self.loc_mapper.create_map_entry(course_location, package_id=new_package_id)
         old_course_id = course_location.course_id
         # the only difference in data between the old and split_mongo xblocks are the locations;
         # so, any field which holds a location must change to a Locator; otherwise, the persistence
         # layer and kvs's know how to store it.
         # locations are in location, children, conditionals, course.tab
 
-        # create the course: set fields to explicitly_set for each scope, id_root = new_course_id, master_branch = 'production'
+        # create the course: set fields to explicitly_set for each scope, id_root = new_package_id, master_branch = 'production'
         original_course = self.direct_modulestore.get_item(course_location)
         new_course_root_locator = self.loc_mapper.translate_location(old_course_id, course_location)
         new_course = self.split_modulestore.create_course(
             course_location.org, original_course.display_name,
-            user_id, id_root=new_course_id,
+            user_id, id_root=new_package_id,
             fields=self._get_json_fields_translate_children(original_course, old_course_id, True),
             root_block_id=new_course_root_locator.block_id,
             master_branch=new_course_root_locator.branch
         )
 
         self._copy_published_modules_to_course(new_course, course_location, old_course_id, user_id)
-        self._add_draft_modules_to_course(new_course_id, old_course_id, course_location, user_id)
+        self._add_draft_modules_to_course(new_package_id, old_course_id, course_location, user_id)
 
-        return new_course_id
+        return new_package_id
 
     def _copy_published_modules_to_course(self, new_course, old_course_loc, old_course_id, user_id):
         """
@@ -94,13 +94,13 @@ class SplitMigrator(object):
         # children which meant some pointers were to non-existent locations in 'direct'
         self.split_modulestore.internal_clean_children(course_version_locator)
 
-    def _add_draft_modules_to_course(self, new_course_id, old_course_id, old_course_loc, user_id):
+    def _add_draft_modules_to_course(self, new_package_id, old_course_id, old_course_loc, user_id):
         """
         update each draft. Create any which don't exist in published and attach to their parents.
         """
         # each true update below will trigger a new version of the structure. We may want to just have one new version
         # but that's for a later date.
-        new_draft_course_loc = CourseLocator(course_id=new_course_id, branch='draft')
+        new_draft_course_loc = CourseLocator(package_id=new_package_id, branch='draft')
         # to prevent race conditions of grandchilden being added before their parents and thus having no parent to
         # add to
         awaiting_adoption = {}
@@ -112,7 +112,7 @@ class SplitMigrator(object):
                 new_locator = self.loc_mapper.translate_location(
                     old_course_id, module.location, False, add_entry_if_missing=True
                 )
-                if self.split_modulestore.has_item(new_course_id, new_locator):
+                if self.split_modulestore.has_item(new_package_id, new_locator):
                     # was in 'direct' so draft is a new version
                     split_module = self.split_modulestore.get_item(new_locator)
                     # need to remove any no-longer-explicitly-set values and add/update any now set values.

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

@@ -27,9 +27,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         modulestore: the module store that can be used to retrieve additional
         modules
 
-        course_entry: the originally fetched enveloped course_structure w/ branch and course_id info.
+        course_entry: the originally fetched enveloped course_structure w/ branch and package_id info.
         Callers to _load_item provide an override but that function ignores the provided structure and
-        only looks at the branch and course_id
+        only looks at the branch and package_id
 
         module_data: a dict mapping Location -> json that was cached from the
             underlying modulestore
@@ -78,14 +78,14 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     # the thread is working with more than one named container pointing to the same specific structure is
     # 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.
+    # is the intended one when not given a course_entry_override; thus, the caching of the last branch/package_id.
     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:
             # most recent retrieval is most likely the right one for next caller (see comment above fn)
             self.course_entry['branch'] = course_entry_override['branch']
-            self.course_entry['course_id'] = course_entry_override['course_id']
+            self.course_entry['package_id'] = course_entry_override['package_id']
         # most likely a lazy loader or the id directly
         definition = json_data.get('definition', {})
         definition_id = self.modulestore.definition_locator(definition)
@@ -97,7 +97,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         block_locator = BlockUsageLocator(
             version_guid=course_entry_override['structure']['_id'],
             block_id=block_id,
-            course_id=course_entry_override.get('course_id'),
+            package_id=course_entry_override.get('package_id'),
             branch=course_entry_override.get('branch')
         )
 

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

@@ -30,7 +30,7 @@ class TestOrphan(unittest.TestCase):
         'xblock_mixins': (InheritanceMixin,)
     }
 
-    split_course_id = 'test_org.test_course.runid'
+    split_package_id = 'test_org.test_course.runid'
 
     def setUp(self):
         self.db_config['collection'] = 'modulestore{0}'.format(uuid.uuid4().hex[:5])
@@ -85,13 +85,13 @@ class TestOrphan(unittest.TestCase):
             self.old_mongo.update_children(parent_location, parent.children)
             # create pointer for split
             course_or_parent_locator = BlockUsageLocator(
-                course_id=self.split_course_id,
+                package_id=self.split_package_id,
                 branch='draft',
                 block_id=parent_name
             )
         else:
             course_or_parent_locator = CourseLocator(
-                course_id='test_org.test_course.runid',
+                package_id='test_org.test_course.runid',
                 branch='draft',
             )
         self.split_mongo.create_item(course_or_parent_locator, category, self.userid, block_id=name, fields=fields)
@@ -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_block_id='runid'
+            'test_org', 'my course', self.userid, self.split_package_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)
@@ -148,11 +148,11 @@ class TestOrphan(unittest.TestCase):
         """
         Test that old mongo finds the orphans
         """
-        orphans = self.split_mongo.get_orphans(self.split_course_id, ['static_tab', 'about', 'course_info'], 'draft')
+        orphans = self.split_mongo.get_orphans(self.split_package_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', block_id='OrphanChapter')
+        location = BlockUsageLocator(package_id=self.split_package_id, branch='draft', block_id='OrphanChapter')
         self.assertIn(location, orphans)
-        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', block_id='OrphanVert')
+        location = BlockUsageLocator(package_id=self.split_package_id, branch='draft', block_id='OrphanVert')
         self.assertIn(location, orphans)
-        location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', block_id='OrphanHtml')
+        location = BlockUsageLocator(package_id=self.split_package_id, branch='draft', block_id='OrphanHtml')
         self.assertIn(location, orphans)

lms/djangoapps/courseware/roles.py

@@ -10,7 +10,7 @@ from django.contrib.auth.models import User, Group
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError
 from xmodule.modulestore.django import loc_mapper
-from xmodule.modulestore.locator import CourseLocator
+from xmodule.modulestore.locator import CourseLocator, Locator
 
 
 class CourseContextRequired(Exception):
@@ -144,29 +144,29 @@ class CourseRole(GroupBasedRole):
         """
         # TODO: figure out how to make the group name generation lazy so it doesn't force the
         # loc mapping?
-        if not hasattr(location, 'course_id'):
-            location = Location(location)
+        location = Locator.to_locator_or_location(location)
         # direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap
         groupnames = []
-        try:
-            groupnames.append('{0}_{1}'.format(role, location.course_id))
-        except InvalidLocationError:  # will occur on old locations where location is not of category course
-            if course_context is None:
-                raise CourseContextRequired()
-            else:
-                groupnames.append('{0}_{1}'.format(role, course_context))
 
         # pylint: disable=no-member
         if isinstance(location, Location):
             try:
+                groupnames.append('{0}_{1}'.format(role, location.course_id))
+            except InvalidLocationError:  # will occur on old locations where location is not of category course
+                if course_context is None:
+                    raise CourseContextRequired()
+                else:
+                    groupnames.append('{0}_{1}'.format(role, course_context))
+            try:
                 locator = loc_mapper().translate_location(location.course_id, location, False, False)
-                groupnames.append('{0}_{1}'.format(role, locator.course_id))
+                groupnames.append('{0}_{1}'.format(role, locator.package_id))
             except (InvalidLocationError, ItemNotFoundError):
                 # if it's never been mapped, the auth won't be via the Locator syntax
                 pass
             # least preferred legacy role_course format
             groupnames.append('{0}_{1}'.format(role, location.course))
         elif isinstance(location, CourseLocator):
+            groupnames.append('{0}_{1}'.format(role, location.package_id))
             # handle old Location syntax
             old_location = loc_mapper().translate_locator_to_location(location, get_course=True)
             if old_location: