diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 19c00f832a..9ad595c668 100644 --- a/cms/djangoapps/auth/authz.py +++ b/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 _ - # if it exists, then use that one, otherwise use a _ 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) diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 9bce00d650..04a8311979 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/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 diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py index 6dae9d8831..8362fc1369 100644 --- a/cms/djangoapps/contentstore/tests/test_permissions.py +++ b/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)) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index abac202b98..decdd85d30 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/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() diff --git a/cms/djangoapps/contentstore/views/checklist.py b/cms/djangoapps/contentstore/views/checklist.py index f9156703f4..9fddf774c6 100644 --- a/cms/djangoapps/contentstore/views/checklist.py +++ b/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() diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index b20dae9e1d..0273ccc716 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/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: diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index ecbb0890b2..d084859536 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -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, block_id, user, depth=0): +def _get_locator_and_course(package_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, block_id=block_id) + locator = BlockUsageLocator(package_id=package_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) @@ -75,7 +75,7 @@ def _get_locator_and_course(course_id, branch, version_guid, block_id, user, dep # pylint: disable=unused-argument @login_required -def course_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): +def course_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None): """ The restful handler for course specific requests. It provides the course tree with the necessary information for identifying and labeling the parts. The root @@ -93,7 +93,7 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid= 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. + package_id, prettyid. Return same json as above. DELETE json: delete this branch from this course (leaving off /branch/draft would imply delete the course) """ @@ -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, block_id=block) + BlockUsageLocator(package_id=package_id, branch=branch, version_guid=version_guid, block_id=block) ): raise PermissionDenied() elif request.method == 'PUT': @@ -114,10 +114,10 @@ def course_handler(request, tag=None, course_id=None, branch=None, version_guid= else: return HttpResponseBadRequest() elif request.method == 'GET': # assume html - if course_id is None: + if package_id is None: return course_listing(request) else: - return course_index(request, course_id, branch, version_guid, block) + return course_index(request, package_id, branch, version_guid, block) else: return HttpResponseNotFound() @@ -157,9 +157,7 @@ def course_listing(request): course.display_name, # note, couldn't get django reverse to work; so, wrote workaround course_loc.url_reverse('course/', ''), - get_lms_link_for_item( - course.location - ), + get_lms_link_for_item(course.location), course.display_org_with_default, course.display_number_with_default, course.location.name @@ -175,14 +173,14 @@ def course_listing(request): @login_required @ensure_csrf_cookie -def course_index(request, course_id, branch, version_guid, block): +def course_index(request, package_id, branch, version_guid, block): """ Display an editable course overview. org, course, name: Attributes of the Location for the item to edit """ locator, course = _get_locator_and_course( - course_id, branch, version_guid, block, request.user, depth=3 + package_id, branch, version_guid, block, request.user, depth=3 ) lms_link = get_lms_link_for_item(course.location) sections = course.get_children() @@ -315,13 +313,13 @@ def create_new_course(request): @login_required @ensure_csrf_cookie @require_http_methods(["GET"]) -def course_info_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): +def course_info_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None): """ GET html: return html for editing the course info handouts and updates. """ __, course_module = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): handouts_old_location = course_module.location.replace(category='course_info', name='handouts') @@ -352,7 +350,7 @@ def course_info_handler(request, tag=None, course_id=None, branch=None, version_ @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) @expect_json -def course_info_update_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, +def course_info_update_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, provided_id=None): """ restful CRUD operations on course_info updates. @@ -366,7 +364,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, block_id=block) + updates_locator = BlockUsageLocator(package_id=package_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 @@ -400,7 +398,7 @@ def course_info_update_handler(request, tag=None, course_id=None, branch=None, v @ensure_csrf_cookie @require_http_methods(("GET", "PUT", "POST")) @expect_json -def settings_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): +def settings_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None): """ Course settings for dates and about pages GET @@ -410,7 +408,7 @@ def settings_handler(request, tag=None, course_id=None, branch=None, version_gui json: update the Course and About xblocks through the CourseDetails model """ locator, course_module = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': upload_asset_url = locator.url_reverse('assets/') @@ -444,7 +442,7 @@ def settings_handler(request, tag=None, course_id=None, branch=None, version_gui @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) @expect_json -def grading_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None, grader_index=None): +def grading_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None, grader_index=None): """ Course Grading policy configuration GET @@ -456,7 +454,7 @@ def grading_handler(request, tag=None, course_id=None, branch=None, version_guid json w/ grader_index: create or update the specific grader (create if index out of range) """ locator, course_module = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': @@ -550,7 +548,7 @@ def _config_course_advanced_components(request, course_module): @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT")) @expect_json -def advanced_settings_handler(request, course_id=None, branch=None, version_guid=None, block=None, tag=None): +def advanced_settings_handler(request, package_id=None, branch=None, version_guid=None, block=None, tag=None): """ Course settings configuration GET @@ -562,7 +560,7 @@ def advanced_settings_handler(request, course_id=None, branch=None, version_guid of keys whose values to unset: i.e., revert to default """ locator, course_module = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) if 'text/html' in request.META.get('HTTP_ACCEPT', '') and request.method == 'GET': @@ -652,7 +650,7 @@ def assign_textbook_id(textbook, used_ids=()): @require_http_methods(("GET", "POST", "PUT")) @login_required @ensure_csrf_cookie -def textbooks_list_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): +def textbooks_list_handler(request, tag=None, package_id=None, branch=None, version_guid=None, block=None): """ A RESTful handler for textbook collections. @@ -665,7 +663,7 @@ def textbooks_list_handler(request, tag=None, course_id=None, branch=None, versi json: overwrite all textbooks in the course with the given list """ locator, course = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) store = get_modulestore(course.location) @@ -729,7 +727,7 @@ def textbooks_list_handler(request, tag=None, course_id=None, branch=None, versi @login_required @ensure_csrf_cookie @require_http_methods(("GET", "POST", "PUT", "DELETE")) -def textbooks_detail_handler(request, tid, tag=None, course_id=None, branch=None, version_guid=None, block=None): +def textbooks_detail_handler(request, tid, tag=None, package_id=None, branch=None, version_guid=None, block=None): """ JSON API endpoint for manipulating a textbook via its internal ID. Used by the Backbone application. @@ -742,7 +740,7 @@ def textbooks_detail_handler(request, tid, tag=None, course_id=None, branch=None json: remove textbook """ __, course = _get_locator_and_course( - course_id, branch, version_guid, block, request.user + package_id, branch, version_guid, block, request.user ) store = get_modulestore(course.location) matching_id = [tb for tb in course.pdf_textbooks diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index d555748fb4..f532734daf 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/cms/djangoapps/contentstore/views/import_export.py @@ -50,7 +50,7 @@ CONTENT_RE = re.compile(r"(?P\d{1,11})-(?P\d{1,11})/(?P\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() diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index bf059390df..34bcc7128f 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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': diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index b9bda31291..e4f9985b3a 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/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() diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index ac6ca3bc4c..02327bb822 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/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() diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 0ddd27d202..842a235bed 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/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), diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index fdfb757267..3de215dbc7 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 7e2854b042..777646b3d6 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -12,7 +12,7 @@ from bson.errors import InvalidId from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError -from .parsers import parse_url, parse_course_id, parse_block_ref +from .parsers import parse_url, parse_package_id, parse_block_ref from .parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX import re from xmodule.modulestore import Location @@ -149,15 +149,15 @@ class CourseLocator(Locator): """ Examples of valid CourseLocator specifications: CourseLocator(version_guid=ObjectId('519665f6223ebd6980884f2b')) - CourseLocator(course_id='mit.eecs.6002x') - CourseLocator(course_id='mit.eecs.6002x/branch/published') - CourseLocator(course_id='mit.eecs.6002x', branch='published') + CourseLocator(package_id='mit.eecs.6002x') + CourseLocator(package_id='mit.eecs.6002x/branch/published') + CourseLocator(package_id='mit.eecs.6002x', branch='published') CourseLocator(url='edx://version/519665f6223ebd6980884f2b') CourseLocator(url='edx://mit.eecs.6002x') CourseLocator(url='edx://mit.eecs.6002x/branch/published') CourseLocator(url='edx://mit.eecs.6002x/branch/published/version/519665f6223ebd6980884f2b') - Should have at lease a specific course_id (id for the course as if it were a project w/ + Should have at lease a specific package_id (id for the course as if it were a project w/ versions) with optional 'branch', or version_guid (which points to a specific version). Can contain both in which case the persistence layer may raise exceptions if the given version != the current such version @@ -166,47 +166,47 @@ class CourseLocator(Locator): # Default values version_guid = None - course_id = None + package_id = None branch = None - def __init__(self, url=None, version_guid=None, course_id=None, branch=None): + def __init__(self, url=None, version_guid=None, package_id=None, branch=None): """ Construct a CourseLocator Caller may provide url (but no other parameters). Caller may provide version_guid (but no other parameters). - Caller may provide course_id (optionally provide branch). + Caller may provide package_id (optionally provide branch). Resulting CourseLocator will have either a version_guid property - or a course_id (with optional branch) property, or both. + or a package_id (with optional branch) property, or both. version_guid must be an instance of bson.objectid.ObjectId or None - url, course_id, and branch must be strings or None + url, package_id, and branch must be strings or None """ - self._validate_args(url, version_guid, course_id) + self._validate_args(url, version_guid, package_id) if url: self.init_from_url(url) if version_guid: self.init_from_version_guid(version_guid) - if course_id or branch: - self.init_from_course_id(course_id, branch) - if self.version_guid is None and self.course_id is None: - raise ValueError("Either version_guid or course_id should be set: {}".format(url)) + if package_id or branch: + self.init_from_package_id(package_id, branch) + if self.version_guid is None and self.package_id is None: + raise ValueError("Either version_guid or package_id should be set: {}".format(url)) def __unicode__(self): """ Return a string representing this location. """ - if self.course_id: - result = self.course_id + if self.package_id: + result = self.package_id if self.branch: result += '/' + BRANCH_PREFIX + self.branch return result elif self.version_guid: return VERSION_PREFIX + str(self.version_guid) else: - # raise InsufficientSpecificationError("missing course_id or version_guid") - return '' + # raise InsufficientSpecificationError("missing package_id or version_guid") + return '' def url(self): """ @@ -214,29 +214,29 @@ class CourseLocator(Locator): """ return 'edx://' + unicode(self) - def _validate_args(self, url, version_guid, course_id): + def _validate_args(self, url, version_guid, package_id): """ Validate provided arguments. Internal use only which is why it checks for each arg and doesn't use keyword """ - if not any((url, version_guid, course_id)): - raise InsufficientSpecificationError("Must provide one of url, version_guid, course_id") + if not any((url, version_guid, package_id)): + raise InsufficientSpecificationError("Must provide one of url, version_guid, package_id") def is_fully_specified(self): """ - Returns True if either version_guid is specified, or course_id+branch + Returns True if either version_guid is specified, or package_id+branch are specified. This should always return True, since this should be validated in the constructor. """ return (self.version_guid is not None or - (self.course_id is not None and self.branch is not None)) + (self.package_id is not None and self.branch is not None)) - def set_course_id(self, new): + def set_package_id(self, new): """ - Initialize course_id to new value. - If course_id has already been initialized to a different value, raise an exception. + Initialize package_id to new value. + If package_id has already been initialized to a different value, raise an exception. """ - self.set_property('course_id', new) + self.set_property('package_id', new) def set_branch(self, new): """ @@ -259,7 +259,7 @@ class CourseLocator(Locator): The copy does not include subclass information, such as a block_id (a property of BlockUsageLocator). """ - return CourseLocator(course_id=self.course_id, + return CourseLocator(package_id=self.package_id, version_guid=self.version_guid, branch=self.branch) @@ -285,7 +285,7 @@ class CourseLocator(Locator): def init_from_url(self, url): """ url must be a string beginning with 'edx://' and containing - either a valid version_guid or course_id (with optional branch), or both. + either a valid version_guid or package_id (with optional branch), or both. """ if isinstance(url, Locator): parse = url.__dict__ @@ -298,7 +298,7 @@ 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', self.set_course_id) + self._set_value(parse, 'package_id', self.set_package_id) self._set_value(parse, 'branch', self.set_branch) def init_from_version_guid(self, version_guid): @@ -313,29 +313,29 @@ class CourseLocator(Locator): raise TypeError('%s is not an instance of ObjectId' % version_guid) self.set_version_guid(version_guid) - def init_from_course_id(self, course_id, explicit_branch=None): + def init_from_package_id(self, package_id, explicit_branch=None): """ - Course_id is a CourseLocator or a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. + package_id is a CourseLocator or a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. Revision (optional) is a string like 'published'. - It may be provided explicitly (explicit_branch) or embedded into course_id. - If branch is part of course_id (".../branch/published"), parse it out separately. + It may be provided explicitly (explicit_branch) or embedded into package_id. + If branch is part of package_id (".../branch/published"), parse it out separately. If branch is provided both ways, that's ok as long as they are the same value. - If a block ('/block/HW3') is a part of course_id, it is ignored. + If a block ('/block/HW3') is a part of package_id, it is ignored. """ - if course_id: - if isinstance(course_id, CourseLocator): - course_id = course_id.course_id - if not course_id: - raise ValueError("%s does not have a valid course_id" % course_id) + if package_id: + if isinstance(package_id, CourseLocator): + package_id = package_id.package_id + if not package_id: + raise ValueError("%s does not have a valid package_id" % package_id) - parse = parse_course_id(course_id) - if not parse or parse['course_id'] is None: - raise ValueError('Could not parse "%s" as a course_id' % course_id) - self.set_course_id(parse['course_id']) + parse = parse_package_id(package_id) + if not parse or parse['package_id'] is None: + raise ValueError('Could not parse "%s" as a package_id' % package_id) + self.set_package_id(parse['package_id']) rev = parse['branch'] if rev: self.set_branch(rev) @@ -357,7 +357,7 @@ class CourseLocator(Locator): the name although that's mutable. We should also clearly define the purpose and restrictions of this (e.g., I'm assuming periods are fine). """ - return self.course_id + return self.package_id def _set_value(self, parse, key, setter): """ @@ -379,12 +379,12 @@ class BlockUsageLocator(CourseLocator): the defined element in the course. Courses can be a version of an offering, the current draft head, or the current production version. - Locators can contain both a version and a course_id w/ branch. The split mongo functions + Locators can contain both a version and a package_id w/ branch. The split mongo functions may raise errors if these conflict w/ the current db state (i.e., the course's branch != the version_guid) Locations can express as urls as well as dictionaries. They consist of - course_identifier: course_guid | version_guid + package_identifier: course_guid | version_guid block : guid branch : string """ @@ -392,34 +392,34 @@ class BlockUsageLocator(CourseLocator): # Default value block_id = None - def __init__(self, url=None, version_guid=None, course_id=None, + def __init__(self, url=None, version_guid=None, package_id=None, branch=None, block_id=None): """ Construct a BlockUsageLocator - Caller may provide url, version_guid, or course_id, and optionally provide branch. + Caller may provide url, version_guid, or package_id, and optionally provide branch. 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 + the url or package_id. If omitted, the locator is created but it has not yet been initialized. 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. + It will have either a version_guid property or a package_id (with optional branch) property, or both. version_guid must be an instance of bson.objectid.ObjectId or None - url, course_id, branch, and block_id must be strings or None + url, package_id, branch, and block_id must be strings or None """ - self._validate_args(url, version_guid, course_id) + self._validate_args(url, version_guid, package_id) if url: self.init_block_ref_from_str(url) - if course_id: - self.init_block_ref_from_course_id(course_id) + if package_id: + self.init_block_ref_from_package_id(package_id) if block_id: self.init_block_ref(block_id) super(BlockUsageLocator, self).__init__( url=url, version_guid=version_guid, - course_id=course_id, + package_id=package_id, branch=branch ) @@ -432,7 +432,7 @@ class BlockUsageLocator(CourseLocator): def version_agnostic(self): """ Returns a copy of itself. - If both version_guid and course_id are known, use a blank course_id in the copy. + If both version_guid and package_id are known, use a blank package_id in the copy. We don't care if the locator's version is not the current head; so, avoid version conflict by reducing info. @@ -444,7 +444,7 @@ class BlockUsageLocator(CourseLocator): branch=self.branch, block_id=self.block_id) else: - return BlockUsageLocator(course_id=self.course_id, + return BlockUsageLocator(package_id=self.package_id, branch=self.branch, block_id=self.block_id) @@ -478,13 +478,13 @@ class BlockUsageLocator(CourseLocator): raise ValueError('Could not parse "%s" as a url' % value) self._set_value(parse, 'block', self.set_block_id) - def init_block_ref_from_course_id(self, course_id): - if isinstance(course_id, CourseLocator): - course_id = course_id.course_id - assert course_id, "%s does not have a valid course_id" - parse = parse_course_id(course_id) + def init_block_ref_from_package_id(self, package_id): + if isinstance(package_id, CourseLocator): + package_id = package_id.package_id + assert package_id, "%s does not have a valid package_id" + parse = parse_package_id(package_id) if parse is None: - raise ValueError('Could not parse "%s" as a course_id' % course_id) + raise ValueError('Could not parse "%s" as a package_id' % package_id) self._set_value(parse, 'block', self.set_block_id) def __unicode__(self): diff --git a/common/lib/xmodule/xmodule/modulestore/parsers.py b/common/lib/xmodule/xmodule/modulestore/parsers.py index dbef47d610..f1f49febb4 100644 --- a/common/lib/xmodule/xmodule/modulestore/parsers.py +++ b/common/lib/xmodule/xmodule/modulestore/parsers.py @@ -11,7 +11,7 @@ ALLOWED_ID_CHARS = r'[a-zA-Z0-9_\-~.:]' URL_RE_SOURCE = r""" (?Pedx://)? - ((?P{ALLOWED_ID_CHARS}+)/?)? + ((?P{ALLOWED_ID_CHARS}+)/?)? ({BRANCH_PREFIX}(?P{ALLOWED_ID_CHARS}+)/?)? ({VERSION_PREFIX}(?P[A-F0-9]+)/?)? ({BLOCK_PREFIX}(?P{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. diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 93a42b5f12..c3c7ed6a2f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index b7381b57df..55ec01c5cb 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/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') ) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index ff61571126..9acae4431d 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -3,7 +3,7 @@ Provides full versioning CRUD and representation for collections of xblocks (e.g Representation: * course_index: a dictionary: - ** '_id': course_id (e.g., myu.mydept.mycourse.myrun), + ** '_id': package_id (e.g., myu.mydept.mycourse.myrun), ** 'org': the org's id. Only used for searching not identity, ** 'prettyid': a vague to-be-determined field probably more useful to storing searchable tags, ** 'edited_by': user_id of user who created the original entry, @@ -227,7 +227,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return the CourseDescriptor! It returns the actual db json from structures. - Semantics: if course_id and branch given, then it will get that branch. If + Semantics: if package_id and branch given, then it will get that branch. If also give a version_guid, it will see if the current head of that branch == that guid. If not it raises VersionConflictError (the version now differs from what it was when you got your reference) @@ -239,9 +239,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if not course_locator.is_fully_specified(): raise InsufficientSpecificationError('Not fully specified: %s' % course_locator) - if course_locator.course_id is not None and course_locator.branch is not None: - # use the course_id - index = self.db_connection.get_course_index(course_locator.course_id) + if course_locator.package_id is not None and course_locator.branch is not None: + # use the package_id + index = self.db_connection.get_course_index(course_locator.package_id) if index is None: raise ItemNotFoundError(course_locator) if course_locator.branch not in index['versions']: @@ -258,11 +258,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): version_guid = course_locator.as_object_id(version_guid) entry = self.db_connection.get_structure(version_guid) - # b/c more than one course can use same structure, the 'course_id' and 'branch' are not intrinsic to structure + # b/c more than one course can use same structure, the 'package_id' and 'branch' are not intrinsic to structure # and the one assoc'd w/ it by another fetch may not be the one relevant to this fetch; so, # add it in the envelope for the structure. envelope = { - 'course_id': course_locator.course_id, + 'package_id': course_locator.package_id, 'branch': course_locator.branch, 'structure': entry, } @@ -300,7 +300,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): result = [] for entry in course_entries: envelope = { - 'course_id': id_version_map[entry['_id']], + 'package_id': id_version_map[entry['_id']], 'branch': branch, 'structure': entry, } @@ -327,7 +327,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): ''' return self.get_course(location) - def has_item(self, course_id, block_location): + def has_item(self, package_id, block_location): """ Returns True if location exists in its course. Returns false if the course or the block w/in the course do not exist for the given version. @@ -433,13 +433,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return [BlockUsageLocator(url=locator.as_course_locator(), block_id=parent_id) for parent_id in items] - def get_orphans(self, course_id, detached_categories, branch): + def get_orphans(self, package_id, detached_categories, branch): """ Return a dict of all of the orphans in the course. - :param course_id: + :param package_id: """ - course = self._lookup_course(CourseLocator(course_id=course_id, branch=branch)) + course = self._lookup_course(CourseLocator(package_id=package_id, branch=branch)) items = set(course['structure']['blocks'].keys()) items.remove(course['structure']['root']) for block_id, block_data in course['structure']['blocks'].iteritems(): @@ -447,7 +447,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if block_data['category'] in detached_categories: items.discard(block_id) return [ - BlockUsageLocator(course_id=course_id, branch=branch, block_id=block_id) + BlockUsageLocator(package_id=package_id, branch=branch, block_id=block_id) for block_id in items ] @@ -456,7 +456,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): The index records the initial creation of the indexed course and tracks the current version heads. This function is primarily for test verification but may serve some more general purpose. - :param course_locator: must have a course_id set + :param course_locator: must have a package_id set :return {'org': , 'prettyid': , versions: {'draft': the head draft version id, 'published': the head published version id if any, @@ -465,9 +465,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'edited_on': when the course was originally created } """ - if course_locator.course_id is None: + if course_locator.package_id is None: return None - index = self.db_connection.get_course_index(course_locator.course_id) + index = self.db_connection.get_course_index(course_locator.package_id) return index # TODO figure out a way to make this info accessible from the course descriptor @@ -662,7 +662,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): serial += 1 return category + str(serial) - def _generate_course_id(self, id_root): + def _generate_package_id(self, id_root): """ Generate a somewhat readable course id unique w/in this db using the id_root :param course_blocks: the current list of blocks. @@ -699,7 +699,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): merely the containing course. raises InsufficientSpecificationError if there is no course locator. - raises VersionConflictError if course_id and version_guid given and the current version head != version_guid + raises VersionConflictError if package_id and version_guid given and the current version head != version_guid and force is not True. :param force: fork the structure and don't update the course draftVersion if the above :param continue_revision: for multistep transactions, continue revising the given version rather than creating @@ -724,11 +724,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Rules for course locator: - * If the course locator specifies a course_id and either it doesn't + * If the course locator specifies a package_id and either it doesn't specify version_guid or the one it specifies == the current head of the branch, it progresses the course to point to the new head and sets the active version to point to the new head - * If the locator has a course_id but its version_guid != current head, it raises VersionConflictError. + * If the locator has a package_id but its version_guid != current head, it raises VersionConflictError. NOTE: using a version_guid will end up creating a new version of the course. Your new item won't be in the course id'd by version_guid but instead in one w/ a new version_guid. Ensure in this case that you get @@ -805,7 +805,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course_parent = None # reconstruct the new_item from the cache - return self.get_item(BlockUsageLocator(course_id=course_parent, + return self.get_item(BlockUsageLocator(package_id=course_parent, block_id=new_block_id, version_guid=new_id)) @@ -818,7 +818,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Create a new entry in the active courses index which points to an existing or new structure. Returns the course root of the resulting entry (the location has the course id) - id_root: allows the caller to specify the course_id. It's a root in that, if it's already taken, + id_root: allows the caller to specify the package_id. It's a root in that, if it's already taken, this method will append things to the root to make it unique. (defaults to org) fields: if scope.settings fields provided, will set the fields of the root course object in the @@ -911,7 +911,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # create the index entry if id_root is None: id_root = org - new_id = self._generate_course_id(id_root) + new_id = self._generate_package_id(id_root) index_entry = { '_id': new_id, @@ -921,7 +921,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'edited_on': datetime.datetime.now(UTC), 'versions': versions_dict} self.db_connection.insert_course_index(index_entry) - return self.get_course(CourseLocator(course_id=new_id, branch=master_branch)) + return self.get_course(CourseLocator(package_id=new_id, branch=master_branch)) def update_item(self, descriptor, user_id, force=False): """ @@ -930,7 +930,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): raises ItemNotFoundError if the location does not exist. - Creates a new course version. If the descriptor's location has a course_id, it moves the course head + Creates a new course version. If the descriptor's location has a package_id, it moves the course head pointer. If the version_guid of the descriptor points to a non-head version and there's been an intervening change to this item, it raises a VersionConflictError unless force is True. In the force case, it forks the course but leaves the head pointer where it is (this change will not be in the course head). @@ -1019,7 +1019,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # fetch and return the new item--fetching is unnecessary but a good qc step return self.get_item( BlockUsageLocator( - course_id=xblock.location.course_id, + package_id=xblock.location.package_id, block_id=xblock.location.block_id, branch=xblock.location.branch, version_guid=new_id @@ -1143,7 +1143,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ # get the destination's index, and source and destination structures. source_structure = self._lookup_course(source_course)['structure'] - index_entry = self.db_connection.get_course_index(destination_course.course_id) + index_entry = self.db_connection.get_course_index(destination_course.package_id) if index_entry is None: # brand new course raise ItemNotFoundError(destination_course) @@ -1207,7 +1207,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): raises ItemNotFoundError if the location does not exist. raises ValueError if usage_locator points to the structure root - Creates a new course version. If the descriptor's location has a course_id, it moves the course head + Creates a new course version. If the descriptor's location has a package_id, it moves the course head pointer. If the version_guid of the descriptor points to a non-head version and there's been an intervening change to this item, it raises a VersionConflictError unless force is True. In the force case, it forks the course but leaves the head pointer where it is (this change will not be in the course head). @@ -1249,12 +1249,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # update the index entry if appropriate if index_entry is not None: self._update_head(index_entry, usage_locator.branch, new_id) - result.course_id = usage_locator.course_id + result.package_id = usage_locator.package_id result.branch = usage_locator.branch return result - def delete_course(self, course_id): + def delete_course(self, package_id): """ Remove the given course from the course index. @@ -1262,11 +1262,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): with a versions hash to restore the course; however, the edited_on and edited_by won't reflect the originals, of course. - :param course_id: uses course_id rather than locator to emphasize its global effect + :param package_id: uses package_id rather than locator to emphasize its global effect """ - index = self.db_connection.get_course_index(course_id) + index = self.db_connection.get_course_index(package_id) if index is None: - raise ItemNotFoundError(course_id) + raise ItemNotFoundError(package_id) # this is the only real delete in the system. should it do something else? self.db_connection.delete_course_index(index['_id']) @@ -1405,7 +1405,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param continue_version: if True, assumes this operation requires a head version and will not create a new version but instead continue an existing transaction on this version. This flag cannot be True if force is True. """ - if locator.course_id is None or locator.branch is None: + if locator.package_id is None or locator.branch is None: if continue_version: raise InsufficientSpecificationError( "To continue a version, the locator must point to one ({}).".format(locator) @@ -1413,7 +1413,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): else: return None else: - index_entry = self.db_connection.get_course_index(locator.course_id) + index_entry = self.db_connection.get_course_index(locator.package_id) is_head = ( locator.version_guid is None or index_entry['versions'][locator.branch] == locator.version_guid diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 7783681262..dee30ba491 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -75,9 +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, block_id, branch, add_entry=False): + def translate_n_check(self, location, old_style_course_id, new_style_package_id, block_id, branch, add_entry=False): """ - Request translation, check course_id, block_id, and branch + Request translation, check package_id, block_id, and branch """ prob_locator = loc_mapper().translate_location( old_style_course_id, @@ -85,7 +85,7 @@ class TestLocationMapper(unittest.TestCase): published= (branch=='published'), add_entry_if_missing=add_entry ) - self.assertEqual(prob_locator.course_id, new_style_course_id) + self.assertEqual(prob_locator.package_id, new_style_package_id) self.assertEqual(prob_locator.block_id, block_id) self.assertEqual(prob_locator.branch, branch) @@ -104,7 +104,7 @@ class TestLocationMapper(unittest.TestCase): add_entry_if_missing=False ) - new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + new_style_package_id = '{}.geek_dept.{}.baz_run'.format(org, course) block_map = { 'abc123': {'problem': 'problem2'}, 'def456': {'problem': 'problem4'}, @@ -112,15 +112,15 @@ class TestLocationMapper(unittest.TestCase): } loc_mapper().create_map_entry( Location('i4x', org, course, 'course', 'baz_run'), - new_style_course_id, + new_style_package_id, block_map=block_map ) test_problem_locn = Location('i4x', org, course, 'problem', 'abc123') # only one course matches - self.translate_n_check(test_problem_locn, old_style_course_id, new_style_course_id, 'problem2', 'published') + self.translate_n_check(test_problem_locn, old_style_course_id, new_style_package_id, 'problem2', 'published') # look for w/ only the Location (works b/c there's only one possible course match). Will force # cache as default translation for this problemid - self.translate_n_check(test_problem_locn, None, new_style_course_id, 'problem2', 'published') + self.translate_n_check(test_problem_locn, None, new_style_package_id, 'problem2', 'published') # look for non-existent problem with self.assertRaises(ItemNotFoundError): loc_mapper().translate_location( @@ -143,7 +143,7 @@ class TestLocationMapper(unittest.TestCase): block_map=distractor_block_map ) # test that old translation still works - self.translate_n_check(test_problem_locn, old_style_course_id, new_style_course_id, 'problem2', 'published') + self.translate_n_check(test_problem_locn, old_style_course_id, new_style_package_id, 'problem2', 'published') # and new returns new id self.translate_n_check(test_problem_locn, test_delta_old_id, test_delta_new_id, 'problem3', 'published') # look for default translation of uncached Location (not unique; so, just verify it returns something) @@ -177,24 +177,24 @@ class TestLocationMapper(unittest.TestCase): old_style_course_id = '{}/{}/{}'.format(org, course, 'baz_run') problem_name = 'abc123abc123abc123abc123abc123f9' location = Location('i4x', org, course, 'problem', problem_name) - new_style_course_id = '{}.{}.{}'.format(org, course, 'baz_run') - self.translate_n_check(location, old_style_course_id, new_style_course_id, 'problemabc', 'published', True) + new_style_package_id = '{}.{}.{}'.format(org, course, 'baz_run') + self.translate_n_check(location, old_style_course_id, new_style_package_id, 'problemabc', 'published', True) # look for w/ only the Location (works b/c there's only one possible course match): causes cache - self.translate_n_check(location, None, new_style_course_id, 'problemabc', 'published', True) + self.translate_n_check(location, None, new_style_package_id, 'problemabc', 'published', True) # create an entry w/o a guid name other_location = Location('i4x', org, course, 'chapter', 'intro') - self.translate_n_check(other_location, old_style_course_id, new_style_course_id, 'intro', 'published', True) + self.translate_n_check(other_location, old_style_course_id, new_style_package_id, 'intro', 'published', True) # add a distractor course - delta_new_course_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') + delta_new_package_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') delta_course_locn = Location('i4x', org, course, 'course', 'delta_run') loc_mapper().create_map_entry( delta_course_locn, - delta_new_course_id, + delta_new_package_id, block_map={problem_name: {'problem': 'problem3'}} ) - self.translate_n_check(location, old_style_course_id, new_style_course_id, 'problemabc', 'published', True) + self.translate_n_check(location, old_style_course_id, new_style_package_id, 'problemabc', 'published', True) # add a new one to both courses (ensure name doesn't have same beginning) new_prob_name = uuid.uuid4().hex @@ -202,9 +202,9 @@ class TestLocationMapper(unittest.TestCase): new_prob_name = uuid.uuid4().hex new_prob_locn = location.replace(name=new_prob_name) new_usage_id = 'problem{}'.format(new_prob_name[:3]) - self.translate_n_check(new_prob_locn, old_style_course_id, new_style_course_id, new_usage_id, 'published', True) + self.translate_n_check(new_prob_locn, old_style_course_id, new_style_package_id, new_usage_id, 'published', True) self.translate_n_check( - new_prob_locn, delta_course_locn.course_id, delta_new_course_id, new_usage_id, 'published', True + new_prob_locn, delta_course_locn.course_id, delta_new_package_id, new_usage_id, 'published', True ) # look for w/ only the Location: causes caching and not unique; so, can't check which course prob_locator = loc_mapper().translate_location( @@ -217,7 +217,7 @@ class TestLocationMapper(unittest.TestCase): # add a default course pointing to the delta_run loc_mapper().create_map_entry( Location('i4x', org, course, 'problem', '789abc123efg456'), - delta_new_course_id, + delta_new_package_id, block_map={problem_name: {'problem': 'problem3'}} ) # now the ambiguous query should return delta @@ -226,11 +226,11 @@ class TestLocationMapper(unittest.TestCase): again_prob_name = uuid.uuid4().hex again_prob_locn = location.replace(name=again_prob_name) again_usage_id = 'problem{}'.format(again_prob_name[:3]) - self.translate_n_check(again_prob_locn, old_style_course_id, new_style_course_id, again_usage_id, 'published', True) + self.translate_n_check(again_prob_locn, old_style_course_id, new_style_package_id, again_usage_id, 'published', True) self.translate_n_check( - again_prob_locn, delta_course_locn.course_id, delta_new_course_id, again_usage_id, 'published', True + again_prob_locn, delta_course_locn.course_id, delta_new_package_id, again_usage_id, 'published', True ) - self.translate_n_check(again_prob_locn, None, delta_new_course_id, again_usage_id, 'published', True) + self.translate_n_check(again_prob_locn, None, delta_new_package_id, again_usage_id, 'published', True) def test_translate_locator(self): """ @@ -239,9 +239,9 @@ class TestLocationMapper(unittest.TestCase): # lookup for non-existent course org = 'foo_org' course = 'bar_course' - new_style_course_id = '{}.geek_dept.{}.baz_run'.format(org, course) + new_style_package_id = '{}.geek_dept.{}.baz_run'.format(org, course) prob_locator = BlockUsageLocator( - course_id=new_style_course_id, + package_id=new_style_package_id, block_id='problem2', branch='published' ) @@ -250,7 +250,7 @@ class TestLocationMapper(unittest.TestCase): loc_mapper().create_map_entry( Location('i4x', org, course, 'course', 'baz_run'), - new_style_course_id, + new_style_package_id, block_map={ 'abc123': {'problem': 'problem2'}, '48f23a10395384929234': {'chapter': 'chapter48f'}, @@ -266,20 +266,20 @@ 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', block_id=prob_locator.block_id + package_id=prob_locator.package_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, block_id='problem2', branch='production' + package_id=new_style_package_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, + package_id=new_style_package_id, block_id='chapter48f', branch='production' ) @@ -290,14 +290,14 @@ 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, block_id='chapter48f', branch='production' + package_id=new_style_package_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')) # look for non-existent problem prob_locator2 = BlockUsageLocator( - course_id=new_style_course_id, + package_id=new_style_package_id, branch='draft', block_id='problem3' ) @@ -305,10 +305,10 @@ class TestLocationMapper(unittest.TestCase): self.assertIsNone(prob_location, 'Found non-existent problem') # add a distractor course - new_style_course_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') + new_style_package_id = '{}.geek_dept.{}.{}'.format(org, course, 'delta_run') loc_mapper().create_map_entry( Location('i4x', org, course, 'course', 'delta_run'), - new_style_course_id, + new_style_package_id, block_map={'abc123': {'problem': 'problem3'}} ) prob_location = loc_mapper().translate_locator_to_location(prob_locator) @@ -317,12 +317,12 @@ class TestLocationMapper(unittest.TestCase): # add a default course pointing to the delta_run loc_mapper().create_map_entry( Location('i4x', org, course, 'problem', '789abc123efg456'), - new_style_course_id, + new_style_package_id, block_map={'abc123': {'problem': 'problem3'}} ) # now query delta (2 entries point to it) prob_locator = BlockUsageLocator( - course_id=new_style_course_id, + package_id=new_style_package_id, branch='production', block_id='problem3' ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 64a6ab2b5b..5922f13afc 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -24,14 +24,14 @@ class LocatorTest(TestCase): OverSpecificationError, CourseLocator, url='edx://mit.eecs.6002x', - course_id='harvard.history', + package_id='harvard.history', branch='published', version_guid=ObjectId()) self.assertRaises( OverSpecificationError, CourseLocator, url='edx://mit.eecs.6002x', - course_id='harvard.history', + package_id='harvard.history', version_guid=ObjectId()) self.assertRaises( OverSpecificationError, @@ -41,7 +41,7 @@ class LocatorTest(TestCase): self.assertRaises( OverSpecificationError, CourseLocator, - course_id='mit.eecs.6002x/' + BRANCH_PREFIX + 'published', + package_id='mit.eecs.6002x/' + BRANCH_PREFIX + 'published', branch='draft') def test_course_constructor_underspecified(self): @@ -71,9 +71,9 @@ class LocatorTest(TestCase): self.assertEqual(str(testobj_2), VERSION_PREFIX + test_id_2_loc) self.assertEqual(testobj_2.url(), 'edx://' + VERSION_PREFIX + test_id_2_loc) - def test_course_constructor_bad_course_id(self): + def test_course_constructor_bad_package_id(self): """ - Test all sorts of badly-formed course_ids (and urls with those course_ids) + Test all sorts of badly-formed package_ids (and urls with those package_ids) """ for bad_id in (' mit.eecs', 'mit.eecs ', @@ -91,7 +91,7 @@ class LocatorTest(TestCase): 'mit.eecs/' + BRANCH_PREFIX + 'this ', 'mit.eecs/' + BRANCH_PREFIX + 'th%is ', ): - self.assertRaises(ValueError, CourseLocator, course_id=bad_id) + self.assertRaises(ValueError, CourseLocator, package_id=bad_id) self.assertRaises(ValueError, CourseLocator, url='edx://' + bad_id) def test_course_constructor_bad_url(self): @@ -103,16 +103,16 @@ class LocatorTest(TestCase): def test_course_constructor_redundant_001(self): testurn = 'mit.eecs.6002x' - testobj = CourseLocator(course_id=testurn, url='edx://' + testurn) - self.check_course_locn_fields(testobj, 'course_id', course_id=testurn) + testobj = CourseLocator(package_id=testurn, url='edx://' + testurn) + self.check_course_locn_fields(testobj, 'package_id', package_id=testurn) def test_course_constructor_redundant_002(self): testurn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published' expected_urn = 'mit.eecs.6002x' expected_rev = 'published' - testobj = CourseLocator(course_id=testurn, url='edx://' + testurn) - self.check_course_locn_fields(testobj, 'course_id', - course_id=expected_urn, + testobj = CourseLocator(package_id=testurn, url='edx://' + testurn) + self.check_course_locn_fields(testobj, 'package_id', + package_id=expected_urn, branch=expected_rev) def test_course_constructor_url(self): @@ -126,71 +126,71 @@ class LocatorTest(TestCase): version_guid=ObjectId(test_id_loc) ) - def test_course_constructor_url_course_id_and_version_guid(self): + def test_course_constructor_url_package_id_and_version_guid(self): test_id_loc = '519665f6223ebd6980884f2b' testobj = CourseLocator(url='edx://mit.eecs-honors.6002x/' + VERSION_PREFIX + test_id_loc) self.check_course_locn_fields(testobj, 'error parsing url with both course ID and version GUID', - course_id='mit.eecs-honors.6002x', + package_id='mit.eecs-honors.6002x', version_guid=ObjectId(test_id_loc)) - def test_course_constructor_url_course_id_branch_and_version_guid(self): + def test_course_constructor_url_package_id_branch_and_version_guid(self): test_id_loc = '519665f6223ebd6980884f2b' testobj = CourseLocator(url='edx://mit.eecs.~6002x/' + BRANCH_PREFIX + 'draft-1/' + VERSION_PREFIX + test_id_loc) self.check_course_locn_fields(testobj, 'error parsing url with both course ID branch, and version GUID', - course_id='mit.eecs.~6002x', + package_id='mit.eecs.~6002x', branch='draft-1', version_guid=ObjectId(test_id_loc)) - def test_course_constructor_course_id_no_branch(self): + def test_course_constructor_package_id_no_branch(self): testurn = 'mit.eecs.6002x' - testobj = CourseLocator(course_id=testurn) - self.check_course_locn_fields(testobj, 'course_id', course_id=testurn) - self.assertEqual(testobj.course_id, testurn) + testobj = CourseLocator(package_id=testurn) + self.check_course_locn_fields(testobj, 'package_id', package_id=testurn) + self.assertEqual(testobj.package_id, testurn) self.assertEqual(str(testobj), testurn) self.assertEqual(testobj.url(), 'edx://' + testurn) - def test_course_constructor_course_id_with_branch(self): + def test_course_constructor_package_id_with_branch(self): testurn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published' expected_id = 'mit.eecs.6002x' expected_branch = 'published' - testobj = CourseLocator(course_id=testurn) - self.check_course_locn_fields(testobj, 'course_id with branch', - course_id=expected_id, + testobj = CourseLocator(package_id=testurn) + self.check_course_locn_fields(testobj, 'package_id with branch', + package_id=expected_id, branch=expected_branch, ) - self.assertEqual(testobj.course_id, expected_id) + self.assertEqual(testobj.package_id, expected_id) self.assertEqual(testobj.branch, expected_branch) self.assertEqual(str(testobj), testurn) self.assertEqual(testobj.url(), 'edx://' + testurn) - def test_course_constructor_course_id_separate_branch(self): + def test_course_constructor_package_id_separate_branch(self): test_id = 'mit.eecs.6002x' test_branch = 'published' expected_urn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published' - testobj = CourseLocator(course_id=test_id, branch=test_branch) - self.check_course_locn_fields(testobj, 'course_id with separate branch', - course_id=test_id, + testobj = CourseLocator(package_id=test_id, branch=test_branch) + self.check_course_locn_fields(testobj, 'package_id with separate branch', + package_id=test_id, branch=test_branch, ) - self.assertEqual(testobj.course_id, test_id) + self.assertEqual(testobj.package_id, test_id) self.assertEqual(testobj.branch, test_branch) self.assertEqual(str(testobj), expected_urn) self.assertEqual(testobj.url(), 'edx://' + expected_urn) - def test_course_constructor_course_id_repeated_branch(self): + def test_course_constructor_package_id_repeated_branch(self): """ - The same branch appears in the course_id and the branch field. + The same branch appears in the package_id and the branch field. """ test_id = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published' test_branch = 'published' expected_id = 'mit.eecs.6002x' expected_urn = test_id - testobj = CourseLocator(course_id=test_id, branch=test_branch) - self.check_course_locn_fields(testobj, 'course_id with repeated branch', - course_id=expected_id, + testobj = CourseLocator(package_id=test_id, branch=test_branch) + self.check_course_locn_fields(testobj, 'package_id with repeated branch', + package_id=expected_id, branch=test_branch, ) - self.assertEqual(testobj.course_id, expected_id) + self.assertEqual(testobj.package_id, expected_id) self.assertEqual(testobj.branch, test_branch) self.assertEqual(str(testobj), expected_urn) self.assertEqual(testobj.url(), 'edx://' + expected_urn) @@ -200,9 +200,9 @@ class LocatorTest(TestCase): expected_id = 'mit.eecs.6002x' expected_branch = 'published' expected_block_ref = 'HW3' - testobj = BlockUsageLocator(course_id=testurn) + testobj = BlockUsageLocator(package_id=testurn) self.check_block_locn_fields(testobj, 'test_block constructor', - course_id=expected_id, + package_id=expected_id, branch=expected_branch, block=expected_block_ref) self.assertEqual(str(testobj), testurn) @@ -210,7 +210,7 @@ class LocatorTest(TestCase): agnostic = testobj.version_agnostic() self.assertIsNone(agnostic.version_guid) self.check_block_locn_fields(agnostic, 'test_block constructor', - course_id=expected_id, + package_id=expected_id, branch=expected_branch, block=expected_block_ref) @@ -221,7 +221,7 @@ class LocatorTest(TestCase): ) self.check_block_locn_fields( testobj, 'error parsing URL with version and block', - course_id='mit.eecs.6002x', + package_id='mit.eecs.6002x', block='lab2', version_guid=ObjectId(test_id_loc) ) @@ -229,10 +229,10 @@ class LocatorTest(TestCase): self.check_block_locn_fields( agnostic, 'error parsing URL with version and block', block='lab2', - course_id=None, + package_id=None, version_guid=ObjectId(test_id_loc) ) - self.assertIsNone(agnostic.course_id) + self.assertIsNone(agnostic.package_id) def test_block_constructor_url_kitchen_sink(self): test_id_loc = '519665f6223ebd6980884f2b' @@ -243,7 +243,7 @@ class LocatorTest(TestCase): ) self.check_block_locn_fields( testobj, 'error parsing URL with branch, version, and block', - course_id='mit.eecs.6002x', + package_id='mit.eecs.6002x', branch='draft', block='lab2', version_guid=ObjectId(test_id_loc) @@ -253,15 +253,15 @@ class LocatorTest(TestCase): """ It seems we used to use colons in names; so, ensure they're acceptable. """ - course_id = 'mit.eecs-1' + package_id = 'mit.eecs-1' branch = 'foo' 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) + testobj = BlockUsageLocator(package_id=package_id, branch=branch, block_id=block_id) + self.check_block_locn_fields(testobj, 'Cannot handle colon', package_id=package_id, branch=branch, block=block_id) def test_repr(self): testurn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published/' + BLOCK_PREFIX + 'HW3' - testobj = BlockUsageLocator(course_id=testurn) + testobj = BlockUsageLocator(package_id=testurn) self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj)) def test_old_location_helpers(self): @@ -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', block_id='deep') + locator = BlockUsageLocator(package_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())) @@ -299,7 +299,7 @@ class LocatorTest(TestCase): """ Test the url_reverse method """ - locator = CourseLocator(course_id="a.fancy_course-id", branch="branch_1.2-3") + locator = CourseLocator(package_id="a.fancy_course-id", branch="branch_1.2-3") self.assertEqual( '/expression/{}/format'.format(unicode(locator)), locator.url_reverse('expression', 'format') @@ -332,19 +332,19 @@ class LocatorTest(TestCase): # Utilities def check_course_locn_fields(self, testobj, msg, version_guid=None, - course_id=None, branch=None): + package_id=None, branch=None): """ - Checks the version, course_id, and branch in testobj + Checks the version, package_id, and branch in testobj """ self.assertEqual(testobj.version_guid, version_guid, msg) - self.assertEqual(testobj.course_id, course_id, msg) + self.assertEqual(testobj.package_id, package_id, msg) self.assertEqual(testobj.branch, branch, msg) def check_block_locn_fields(self, testobj, msg, version_guid=None, - course_id=None, branch=None, block=None): + package_id=None, branch=None, block=None): """ Does adds a block id check over and above the check_course_locn_fields tests """ - self.check_course_locn_fields(testobj, msg, version_guid, course_id, + self.check_course_locn_fields(testobj, msg, version_guid, package_id, branch) self.assertEqual(testobj.block_id, block) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index 877c7bcb36..7aaf1723c6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index e3fb461fbc..32e9d5bbdb 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -120,7 +120,7 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(len(courses), 3, "Wrong number of courses") # check metadata -- NOTE no promised order course = self.findByIdInResult(courses, "head12345") - self.assertEqual(course.location.course_id, "GreekHero") + self.assertEqual(course.location.package_id, "GreekHero") self.assertEqual( str(course.location.version_guid), self.GUID_D0, "course version mismatch" @@ -151,7 +151,7 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(len(courses_published), 1, len(courses_published)) course = self.findByIdInResult(courses_published, "head23456") self.assertIsNotNone(course, "published courses") - self.assertEqual(course.location.course_id, "wonderful") + self.assertEqual(course.location.package_id, "wonderful") self.assertEqual(str(course.location.version_guid), self.GUID_P, course.location.version_guid) self.assertEqual(course.category, 'course', 'wrong category') @@ -190,7 +190,7 @@ class SplitModuleCourseTests(SplitModuleTest): ''' locator = CourseLocator(version_guid=self.GUID_D1) course = modulestore().get_course(locator) - self.assertIsNone(course.location.course_id) + self.assertIsNone(course.location.package_id) self.assertEqual(str(course.location.version_guid), self.GUID_D1) self.assertEqual(course.category, 'course') self.assertEqual(len(course.tabs), 6) @@ -203,9 +203,9 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(course.edited_by, "testassist@edx.org") self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.55}) - locator = CourseLocator(course_id='GreekHero', branch='draft') + locator = CourseLocator(package_id='GreekHero', branch='draft') course = modulestore().get_course(locator) - self.assertEqual(course.location.course_id, "GreekHero") + self.assertEqual(course.location.package_id, "GreekHero") self.assertEqual(str(course.location.version_guid), self.GUID_D0) self.assertEqual(course.category, 'course') self.assertEqual(len(course.tabs), 6) @@ -216,24 +216,24 @@ class SplitModuleCourseTests(SplitModuleTest): self.assertEqual(course.edited_by, "testassist@edx.org") self.assertDictEqual(course.grade_cutoffs, {"Pass": 0.45}) - locator = CourseLocator(course_id='wonderful', branch='published') + locator = CourseLocator(package_id='wonderful', branch='published') course = modulestore().get_course(locator) - self.assertEqual(course.location.course_id, "wonderful") + self.assertEqual(course.location.package_id, "wonderful") self.assertEqual(str(course.location.version_guid), self.GUID_P) - locator = CourseLocator(course_id='wonderful', branch='draft') + locator = CourseLocator(package_id='wonderful', branch='draft') course = modulestore().get_course(locator) self.assertEqual(str(course.location.version_guid), self.GUID_D2) def test_get_course_negative(self): # Now negative testing self.assertRaises(InsufficientSpecificationError, - modulestore().get_course, CourseLocator(course_id='edu.meh.blah')) + modulestore().get_course, CourseLocator(package_id='edu.meh.blah')) self.assertRaises(ItemNotFoundError, - modulestore().get_course, CourseLocator(course_id='nosuchthing', branch='draft')) + modulestore().get_course, CourseLocator(package_id='nosuchthing', branch='draft')) self.assertRaises(ItemNotFoundError, modulestore().get_course, - CourseLocator(course_id='GreekHero', branch='published')) + CourseLocator(package_id='GreekHero', branch='published')) def test_cache(self): """ @@ -252,7 +252,7 @@ class SplitModuleCourseTests(SplitModuleTest): locator = CourseLocator(version_guid=self.GUID_D3) result = modulestore().get_course_successors(locator) self.assertIsInstance(result, VersionTree) - self.assertIsNone(result.locator.course_id) + self.assertIsNone(result.locator.package_id) self.assertEqual(str(result.locator.version_guid), self.GUID_D3) self.assertEqual(len(result.children), 1) self.assertEqual(str(result.children[0].locator.version_guid), self.GUID_D1) @@ -275,71 +275,71 @@ class SplitModuleItemTests(SplitModuleTest): ''' has_item(BlockUsageLocator) ''' - course_id = 'GreekHero' + package_id = 'GreekHero' # positive tests of various forms locator = BlockUsageLocator(version_guid=self.GUID_D1, block_id='head12345') - self.assertTrue(modulestore().has_item(course_id, locator), + self.assertTrue(modulestore().has_item(package_id, locator), "couldn't find in %s" % self.GUID_D1) - locator = BlockUsageLocator(course_id='GreekHero', block_id='head12345', branch='draft') + locator = BlockUsageLocator(package_id='GreekHero', block_id='head12345', branch='draft') self.assertTrue( - modulestore().has_item(locator.course_id, locator), + modulestore().has_item(locator.package_id, locator), "couldn't find in 12345" ) self.assertTrue( - modulestore().has_item(locator.course_id, BlockUsageLocator( - course_id=locator.course_id, + modulestore().has_item(locator.package_id, BlockUsageLocator( + package_id=locator.package_id, branch='draft', block_id=locator.block_id )), "couldn't find in draft 12345" ) self.assertFalse( - modulestore().has_item(locator.course_id, BlockUsageLocator( - course_id=locator.course_id, + modulestore().has_item(locator.package_id, BlockUsageLocator( + package_id=locator.package_id, branch='published', block_id=locator.block_id)), "found in published 12345" ) locator.branch = 'draft' self.assertTrue( - modulestore().has_item(locator.course_id, locator), + modulestore().has_item(locator.package_id, locator), "not found in draft 12345" ) # not a course obj - locator = BlockUsageLocator(course_id='GreekHero', block_id='chapter1', branch='draft') + locator = BlockUsageLocator(package_id='GreekHero', block_id='chapter1', branch='draft') self.assertTrue( - modulestore().has_item(locator.course_id, locator), + modulestore().has_item(locator.package_id, locator), "couldn't find chapter1" ) # in published course - locator = BlockUsageLocator(course_id="wonderful", block_id="head23456", branch='draft') + locator = BlockUsageLocator(package_id="wonderful", block_id="head23456", branch='draft') self.assertTrue( modulestore().has_item( - locator.course_id, - BlockUsageLocator(course_id=locator.course_id, block_id=locator.block_id, branch='published') + locator.package_id, + BlockUsageLocator(package_id=locator.package_id, block_id=locator.block_id, branch='published') ), "couldn't find in 23456" ) locator.branch = 'published' - self.assertTrue(modulestore().has_item(course_id, locator), "couldn't find in 23456") + self.assertTrue(modulestore().has_item(package_id, locator), "couldn't find in 23456") def test_negative_has_item(self): # negative tests--not found # no such course or block - course_id = 'GreekHero' - locator = BlockUsageLocator(course_id="doesnotexist", block_id="head23456", branch='draft') - self.assertFalse(modulestore().has_item(course_id, locator)) - locator = BlockUsageLocator(course_id="wonderful", block_id="doesnotexist", branch='draft') - self.assertFalse(modulestore().has_item(course_id, locator)) + package_id = 'GreekHero' + locator = BlockUsageLocator(package_id="doesnotexist", block_id="head23456", branch='draft') + self.assertFalse(modulestore().has_item(package_id, locator)) + locator = BlockUsageLocator(package_id="wonderful", block_id="doesnotexist", branch='draft') + self.assertFalse(modulestore().has_item(package_id, locator)) # negative tests--insufficient specification self.assertRaises(InsufficientSpecificationError, BlockUsageLocator) self.assertRaises(InsufficientSpecificationError, modulestore().has_item, None, BlockUsageLocator(version_guid=self.GUID_D1)) self.assertRaises(InsufficientSpecificationError, - modulestore().has_item, None, BlockUsageLocator(course_id='GreekHero')) + modulestore().has_item, None, BlockUsageLocator(package_id='GreekHero')) def test_get_item(self): ''' @@ -349,11 +349,11 @@ class SplitModuleItemTests(SplitModuleTest): locator = BlockUsageLocator(version_guid=self.GUID_D1, block_id='head12345') block = modulestore().get_item(locator) self.assertIsInstance(block, CourseDescriptor) - # get_instance just redirects to get_item, ignores course_id - self.assertIsInstance(modulestore().get_instance("course_id", locator), CourseDescriptor) + # get_instance just redirects to get_item, ignores package_id + self.assertIsInstance(modulestore().get_instance("package_id", locator), CourseDescriptor) def verify_greek_hero(block): - self.assertEqual(block.location.course_id, "GreekHero") + self.assertEqual(block.location.package_id, "GreekHero") self.assertEqual(len(block.tabs), 6, "wrong number of tabs") self.assertEqual(block.display_name, "The Ancient Greek Hero") self.assertEqual(block.advertised_start, "Fall 2013") @@ -365,15 +365,15 @@ class SplitModuleItemTests(SplitModuleTest): block.grade_cutoffs, {"Pass": 0.45}, ) - locator = BlockUsageLocator(course_id='GreekHero', block_id='head12345', branch='draft') + locator = BlockUsageLocator(package_id='GreekHero', block_id='head12345', branch='draft') verify_greek_hero(modulestore().get_item(locator)) - # get_instance just redirects to get_item, ignores course_id - verify_greek_hero(modulestore().get_instance("course_id", locator)) + # get_instance just redirects to get_item, ignores package_id + verify_greek_hero(modulestore().get_instance("package_id", locator)) # try to look up other branches self.assertRaises(ItemNotFoundError, modulestore().get_item, - BlockUsageLocator(course_id=locator.as_course_locator(), + BlockUsageLocator(package_id=locator.as_course_locator(), block_id=locator.block_id, branch='published')) locator.branch = 'draft' @@ -384,16 +384,16 @@ class SplitModuleItemTests(SplitModuleTest): def test_get_non_root(self): # not a course obj - locator = BlockUsageLocator(course_id='GreekHero', block_id='chapter1', branch='draft') + locator = BlockUsageLocator(package_id='GreekHero', block_id='chapter1', branch='draft') block = modulestore().get_item(locator) - self.assertEqual(block.location.course_id, "GreekHero") + self.assertEqual(block.location.package_id, "GreekHero") self.assertEqual(block.category, 'chapter') self.assertEqual(str(block.definition_locator.definition_id), "cd00000000000000dddd0020") self.assertEqual(block.display_name, "Hercules") self.assertEqual(block.edited_by, "testassist@edx.org") # in published course - locator = BlockUsageLocator(course_id="wonderful", block_id="head23456", branch='published') + locator = BlockUsageLocator(package_id="wonderful", block_id="head23456", branch='published') self.assertIsInstance( modulestore().get_item(locator), CourseDescriptor @@ -401,10 +401,10 @@ class SplitModuleItemTests(SplitModuleTest): # negative tests--not found # no such course or block - locator = BlockUsageLocator(course_id="doesnotexist", block_id="head23456", branch='draft') + locator = BlockUsageLocator(package_id="doesnotexist", block_id="head23456", branch='draft') with self.assertRaises(ItemNotFoundError): modulestore().get_item(locator) - locator = BlockUsageLocator(course_id="wonderful", block_id="doesnotexist", branch='draft') + locator = BlockUsageLocator(package_id="wonderful", block_id="doesnotexist", branch='draft') with self.assertRaises(ItemNotFoundError): modulestore().get_item(locator) @@ -412,7 +412,7 @@ class SplitModuleItemTests(SplitModuleTest): with self.assertRaises(InsufficientSpecificationError): modulestore().get_item(BlockUsageLocator(version_guid=self.GUID_D1)) with self.assertRaises(InsufficientSpecificationError): - modulestore().get_item(BlockUsageLocator(course_id='GreekHero', branch='draft')) + modulestore().get_item(BlockUsageLocator(package_id='GreekHero', branch='draft')) # pylint: disable=W0212 def test_matching(self): @@ -473,11 +473,11 @@ class SplitModuleItemTests(SplitModuleTest): ''' get_parent_locations(locator, [block_id], [branch]): [BlockUsageLocator] ''' - locator = BlockUsageLocator(course_id="GreekHero", branch='draft', block_id='chapter1') + locator = BlockUsageLocator(package_id="GreekHero", branch='draft', block_id='chapter1') parents = modulestore().get_parent_locations(locator) self.assertEqual(len(parents), 1) self.assertEqual(parents[0].block_id, 'head12345') - self.assertEqual(parents[0].course_id, "GreekHero") + self.assertEqual(parents[0].package_id, "GreekHero") locator.block_id = 'chapter2' parents = modulestore().get_parent_locations(locator) self.assertEqual(len(parents), 1) @@ -490,7 +490,7 @@ class SplitModuleItemTests(SplitModuleTest): """ Test the existing get_children method on xdescriptors """ - locator = BlockUsageLocator(course_id="GreekHero", block_id="head12345", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="head12345", branch='draft') block = modulestore().get_item(locator) children = block.get_children() expected_ids = [ @@ -532,7 +532,7 @@ class TestItemCrud(SplitModuleTest): create_item(course_or_parent_locator, category, user, definition_locator=None, fields): new_desciptor """ # grab link to course to ensure new versioning works - locator = CourseLocator(course_id="GreekHero", branch='draft') + locator = CourseLocator(package_id="GreekHero", branch='draft') premod_course = modulestore().get_course(locator) premod_time = datetime.datetime.now(UTC) - datetime.timedelta(seconds=1) # add minimal one w/o a parent @@ -542,7 +542,7 @@ class TestItemCrud(SplitModuleTest): fields={'display_name': 'new sequential'} ) # check that course version changed and course's previous is the other one - self.assertEqual(new_module.location.course_id, "GreekHero") + self.assertEqual(new_module.location.package_id, "GreekHero") self.assertNotEqual(new_module.location.version_guid, premod_course.location.version_guid) self.assertIsNone(locator.version_guid, "Version inadvertently filled in") current_course = modulestore().get_course(locator) @@ -569,7 +569,7 @@ class TestItemCrud(SplitModuleTest): """ Test create_item w/ specifying the parent of the new item """ - locator = BlockUsageLocator(course_id="wonderful", block_id="head23456", branch='draft') + locator = BlockUsageLocator(package_id="wonderful", block_id="head23456", branch='draft') premod_course = modulestore().get_course(locator) category = 'chapter' new_module = modulestore().create_item( @@ -589,7 +589,7 @@ class TestItemCrud(SplitModuleTest): a definition id and new def data that it branches the definition in the db. Actually, this tries to test all create_item features not tested above. """ - locator = BlockUsageLocator(course_id="contender", block_id="head345679", branch='draft') + locator = BlockUsageLocator(package_id="contender", block_id="head345679", branch='draft') category = 'problem' premod_time = datetime.datetime.now(UTC) - datetime.timedelta(seconds=1) new_payload = "empty" @@ -633,7 +633,7 @@ class TestItemCrud(SplitModuleTest): course_block_update_version = new_course.update_version self.assertIsNotNone(new_course_locator.version_guid, "Want to test a definite version") versionless_course_locator = CourseLocator( - course_id=new_course_locator.course_id, branch=new_course_locator.branch + package_id=new_course_locator.package_id, branch=new_course_locator.branch ) # positive simple case: no force, add chapter @@ -687,7 +687,7 @@ class TestItemCrud(SplitModuleTest): # add new child to old parent in continued (leave off version_guid) course_module_locator = BlockUsageLocator( - course_id=new_course.location.course_id, + package_id=new_course.location.package_id, block_id=new_course.location.block_id, branch=new_course.location.branch ) @@ -709,7 +709,7 @@ class TestItemCrud(SplitModuleTest): """ test updating an items metadata ensuring the definition doesn't version but the course does if it should """ - locator = BlockUsageLocator(course_id="GreekHero", block_id="problem3_2", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="problem3_2", branch='draft') problem = modulestore().get_item(locator) pre_def_id = problem.definition_locator.definition_id pre_version_guid = problem.location.version_guid @@ -747,7 +747,7 @@ class TestItemCrud(SplitModuleTest): """ test updating an item's children ensuring the definition doesn't version but the course does if it should """ - locator = BlockUsageLocator(course_id="GreekHero", block_id="chapter3", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="chapter3", branch='draft') block = modulestore().get_item(locator) pre_def_id = block.definition_locator.definition_id pre_version_guid = block.location.version_guid @@ -773,7 +773,7 @@ class TestItemCrud(SplitModuleTest): """ test updating an item's definition: ensure it gets versioned as well as the course getting versioned """ - locator = BlockUsageLocator(course_id="GreekHero", block_id="head12345", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="head12345", branch='draft') block = modulestore().get_item(locator) pre_def_id = block.definition_locator.definition_id pre_version_guid = block.location.version_guid @@ -791,7 +791,7 @@ class TestItemCrud(SplitModuleTest): Test updating metadata, children, and definition in a single call ensuring all the versioning occurs """ # first add 2 children to the course for the update to manipulate - locator = BlockUsageLocator(course_id="contender", block_id="head345679", branch='draft') + locator = BlockUsageLocator(package_id="contender", block_id="head345679", branch='draft') category = 'problem' new_payload = "empty" modulestore().create_item( @@ -832,7 +832,7 @@ class TestItemCrud(SplitModuleTest): course.location, 'deleting_user') reusable_location = BlockUsageLocator( - course_id=course.location.course_id, + package_id=course.location.package_id, block_id=course.location.block_id, branch='draft') @@ -840,16 +840,16 @@ class TestItemCrud(SplitModuleTest): problems = modulestore().get_items(reusable_location, {'category': 'problem'}) locn_to_del = problems[0].location new_course_loc = modulestore().delete_item(locn_to_del, 'deleting_user', delete_children=False) - deleted = BlockUsageLocator(course_id=reusable_location.course_id, + deleted = BlockUsageLocator(package_id=reusable_location.package_id, branch=reusable_location.branch, block_id=locn_to_del.block_id) - self.assertFalse(modulestore().has_item(reusable_location.course_id, deleted)) - self.assertRaises(VersionConflictError, modulestore().has_item, reusable_location.course_id, locn_to_del) + self.assertFalse(modulestore().has_item(reusable_location.package_id, deleted)) + self.assertRaises(VersionConflictError, modulestore().has_item, reusable_location.package_id, locn_to_del) locator = BlockUsageLocator( version_guid=locn_to_del.version_guid, block_id=locn_to_del.block_id ) - self.assertTrue(modulestore().has_item(reusable_location.course_id, locator)) + self.assertTrue(modulestore().has_item(reusable_location.package_id, locator)) self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid) # delete a subtree @@ -860,15 +860,15 @@ class TestItemCrud(SplitModuleTest): def check_subtree(node): if node: node_loc = node.location - self.assertFalse(modulestore().has_item(reusable_location.course_id, + self.assertFalse(modulestore().has_item(reusable_location.package_id, BlockUsageLocator( - course_id=node_loc.course_id, + package_id=node_loc.package_id, branch=node_loc.branch, block_id=node.location.block_id))) locator = BlockUsageLocator( version_guid=node.location.version_guid, block_id=node.location.block_id) - self.assertTrue(modulestore().has_item(reusable_location.course_id, locator)) + self.assertTrue(modulestore().has_item(reusable_location.package_id, locator)) if node.has_children: for sub in node.get_children(): check_subtree(sub) @@ -877,7 +877,7 @@ class TestItemCrud(SplitModuleTest): def create_course_for_deletion(self): course = modulestore().create_course('nihilx', 'deletion', 'deleting_user') root = BlockUsageLocator( - course_id=course.location.course_id, + package_id=course.location.package_id, block_id=course.location.block_id, branch='draft') for _ in range(4): @@ -934,13 +934,13 @@ class TestCourseCreation(SplitModuleTest): Test making a course which points to an existing draft and published but not making any changes to either. """ pre_time = datetime.datetime.now(UTC) - original_locator = CourseLocator(course_id="wonderful", branch='draft') + original_locator = CourseLocator(package_id="wonderful", branch='draft') original_index = modulestore().get_course_index_info(original_locator) new_draft = modulestore().create_course( 'leech', 'best_course', 'leech_master', id_root='best', versions_dict=original_index['versions']) new_draft_locator = new_draft.location - self.assertRegexpMatches(new_draft_locator.course_id, r'best.*') + self.assertRegexpMatches(new_draft_locator.package_id, r'best.*') # the edited_by and other meta fields on the new course will be the original author not this one self.assertEqual(new_draft.edited_by, 'test@edx.org') self.assertLess(new_draft.edited_on, pre_time) @@ -951,7 +951,7 @@ class TestCourseCreation(SplitModuleTest): self.assertLessEqual(new_index["edited_on"], datetime.datetime.now(UTC)) self.assertEqual(new_index['edited_by'], 'leech_master') - new_published_locator = CourseLocator(course_id=new_draft_locator.course_id, branch='published') + new_published_locator = CourseLocator(package_id=new_draft_locator.package_id, branch='published') new_published = modulestore().get_course(new_published_locator) self.assertEqual(new_published.edited_by, 'test@edx.org') self.assertLess(new_published.edited_on, pre_time) @@ -979,7 +979,7 @@ class TestCourseCreation(SplitModuleTest): original_course = modulestore().get_course(original_locator) self.assertEqual(original_course.location.version_guid, original_index['versions']['draft']) self.assertFalse( - modulestore().has_item(new_draft_locator.course_id, BlockUsageLocator( + modulestore().has_item(new_draft_locator.package_id, BlockUsageLocator( original_locator, block_id=new_item.location.block_id )) @@ -990,7 +990,7 @@ class TestCourseCreation(SplitModuleTest): Create a new course which overrides metadata and course_data """ pre_time = datetime.datetime.now(UTC) - original_locator = CourseLocator(course_id="contender", branch='draft') + original_locator = CourseLocator(package_id="contender", branch='draft') original = modulestore().get_course(original_locator) original_index = modulestore().get_course_index_info(original_locator) fields = {} @@ -1007,7 +1007,7 @@ class TestCourseCreation(SplitModuleTest): fields=fields ) new_draft_locator = new_draft.location - self.assertRegexpMatches(new_draft_locator.course_id, r'counter.*') + self.assertRegexpMatches(new_draft_locator.package_id, r'counter.*') # the edited_by and other meta fields on the new course will be the original author not this one self.assertEqual(new_draft.edited_by, 'leech_master') self.assertGreaterEqual(new_draft.edited_on, pre_time) @@ -1027,7 +1027,7 @@ class TestCourseCreation(SplitModuleTest): """ Test changing the org, pretty id, etc of a course. Test that it doesn't allow changing the id, etc. """ - locator = CourseLocator(course_id="GreekHero", branch='draft') + locator = CourseLocator(package_id="GreekHero", branch='draft') course_info = modulestore().get_course_index_info(locator) course_info['org'] = 'funkyU' modulestore().update_course_index(course_info) @@ -1051,7 +1051,7 @@ class TestCourseCreation(SplitModuleTest): # an allowed but not recommended way to publish a course versions['published'] = self.GUID_D1 modulestore().update_course_index(course_info) - course = modulestore().get_course(CourseLocator(course_id=locator.course_id, branch="published")) + course = modulestore().get_course(CourseLocator(package_id=locator.package_id, branch="published")) self.assertEqual(str(course.location.version_guid), self.GUID_D1) def test_create_with_root(self): @@ -1086,11 +1086,11 @@ class TestInheritance(SplitModuleTest): """ # Note, not testing value where defined (course) b/c there's no # defined accessor for it on CourseDescriptor. - locator = BlockUsageLocator(course_id="GreekHero", block_id="problem3_2", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="problem3_2", branch='draft') node = modulestore().get_item(locator) # inherited self.assertEqual(node.graceperiod, datetime.timedelta(hours=2)) - locator = BlockUsageLocator(course_id="GreekHero", block_id="problem1", branch='draft') + locator = BlockUsageLocator(package_id="GreekHero", block_id="problem1", branch='draft') node = modulestore().get_item(locator) # overridden self.assertEqual(node.graceperiod, datetime.timedelta(hours=4)) @@ -1111,8 +1111,8 @@ class TestPublish(SplitModuleTest): """ Test the standard patterns: publish to new branch, revise and publish """ - source_course = CourseLocator(course_id="GreekHero", branch='draft') - dest_course = CourseLocator(course_id="GreekHero", branch="published") + source_course = CourseLocator(package_id="GreekHero", branch='draft') + dest_course = CourseLocator(package_id="GreekHero", branch="published") modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2", "chapter3"]) expected = ["head12345", "chapter1"] self._check_course( @@ -1153,13 +1153,13 @@ class TestPublish(SplitModuleTest): """ Test the exceptions which preclude successful publication """ - source_course = CourseLocator(course_id="GreekHero", branch='draft') + source_course = CourseLocator(package_id="GreekHero", branch='draft') # destination does not exist - destination_course = CourseLocator(course_id="Unknown", branch="published") + destination_course = CourseLocator(package_id="Unknown", branch="published") with self.assertRaises(ItemNotFoundError): modulestore().xblock_publish(self.user, source_course, destination_course, ["chapter3"], None) # publishing into a new branch w/o publishing the root - destination_course = CourseLocator(course_id="GreekHero", branch="published") + destination_course = CourseLocator(package_id="GreekHero", branch="published") with self.assertRaises(ItemNotFoundError): modulestore().xblock_publish(self.user, source_course, destination_course, ["chapter3"], None) # publishing a subdag w/o the parent already in course @@ -1171,8 +1171,8 @@ class TestPublish(SplitModuleTest): """ Test publishing moves and deletes. """ - source_course = CourseLocator(course_id="GreekHero", branch='draft') - dest_course = CourseLocator(course_id="GreekHero", branch="published") + source_course = CourseLocator(package_id="GreekHero", branch='draft') + dest_course = CourseLocator(package_id="GreekHero", branch="published") modulestore().xblock_publish(self.user, source_course, dest_course, ["head12345"], ["chapter2"]) expected = ["head12345", "chapter1", "chapter3", "problem1", "problem3_2"] self._check_course(source_course, dest_course, expected, ["chapter2"]) @@ -1210,7 +1210,7 @@ class TestPublish(SplitModuleTest): """ Generate a BlockUsageLocator for the combo of the course location and block id """ - return BlockUsageLocator(course_id=course_loc.course_id, branch=course_loc.branch, block_id=block_id) + return BlockUsageLocator(package_id=course_loc.package_id, branch=course_loc.branch, block_id=block_id) def _compare_children(self, source_children, dest_children, unexpected): """ diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 7d8f56ad00..d99a5ecb91 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/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: