From 8909cb30e754032fb7925178cc53ab78c3bbf8f1 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Tue, 12 Dec 2023 22:46:38 +0000 Subject: [PATCH 1/5] fix: preserve learner state after v2 migration --- cms/djangoapps/contentstore/tasks.py | 2 +- .../core/djangoapps/content_libraries/api.py | 23 +++++++++--- .../djangoapps/content_libraries/tasks.py | 35 ++++++++++++++----- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 8cf1f3172832..646b9adf61de 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -896,7 +896,7 @@ def _create_copy_content_task(v2_library_key, v1_library_key): spin up a celery task to import the V1 Library's content into the V2 library. This utalizes the fact that course and v1 library content is stored almost identically. """ - return v2contentlib_api.import_blocks_create_task(v2_library_key, v1_library_key) + return v2contentlib_api.import_blocks_create_task(v2_library_key, v1_library_key, use_course_key_as_block_id_suffix=False) def _create_metadata(v1_library_key, collection_uuid): diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 10383de336e5..76a95e18fbf0 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1214,12 +1214,13 @@ class BaseEdxImportClient(abc.ABC): "video", } - def __init__(self, library_key=None, library=None): + def __init__(self, library_key=None, library=None, use_course_key_as_block_id_suffix=True): """ Initialize an import client for a library. The method accepts either a library object or a key to a library object. """ + self.use_course_key_as_block_id_suffix = use_course_key_as_block_id_suffix if bool(library_key) == bool(library): raise ValueError('Provide at least one of `library_key` or ' '`library`, but not both.') @@ -1249,7 +1250,9 @@ def import_block(self, modulestore_key): """ Import a single modulestore block. """ + print(f'modulestore_key {modulestore_key}') block_data = self.get_block_data(modulestore_key) + print(f'block_data {block_data}') # Get or create the block in the library. # @@ -1261,8 +1264,19 @@ def import_block(self, modulestore_key): str(modulestore_key.course_key).encode() ).digest() )[:16].decode().lower() + print(f'course_key_id {course_key_id}') # Prepend 'c' to allow changing hash without conflicts. - block_id = f"{modulestore_key.block_id}_c{course_key_id}" + + # add the course_key_id if use_course_key_as_suffix is enabled to increase the namespace. + # in some cases, for the preservation of child-parent relationships + # of the same content across modulestore and blockstore, only the block_id is used. + block_id = block_id = ( + f"{modulestore_key.block_id}_c{course_key_id}" + if self.use_course_key_as_block_id_suffix + else f"{modulestore_key.block_id}" + ) + + print(f'block_id {block_id}') log.info('Importing to library block: id=%s', block_id) try: library_block = create_library_block( @@ -1341,6 +1355,7 @@ def __init__(self, modulestore_instance=None, **kwargs): """ Initialize the client with a modulestore instance. """ + print(kwargs.items()) super().__init__(**kwargs) self.modulestore = modulestore_instance or modulestore() @@ -1473,7 +1488,7 @@ def _call(self, method, *args, **kwargs): return response -def import_blocks_create_task(library_key, course_key): +def import_blocks_create_task(library_key, course_key, use_course_key_as_block_id_suffix=True): """ Create a new import block task. @@ -1486,7 +1501,7 @@ def import_blocks_create_task(library_key, course_key): course_id=course_key, ) result = tasks.import_blocks_from_course.apply_async( - args=(import_task.pk, str(course_key)) + args=(import_task.pk, str(course_key), use_course_key_as_block_id_suffix) ) log.info(f"Import block task created: import_task={import_task} " f"celery_task={result.id}") diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 3c259aaba956..ca44e0bae700 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -36,6 +36,11 @@ from common.djangoapps.student.auth import has_studio_write_access from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import ( + LibraryLocatorV2, + LibraryUsageLocatorV2, + LibraryLocator as LibraryLocatorV1 +) from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.xblock.api import load_block from openedx.core.lib import ensure_cms, blockstore_api @@ -45,6 +50,8 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.store_utilities import derived_key from . import api from .models import ContentLibraryBlockImportTask @@ -55,7 +62,7 @@ @shared_task(base=LoggedTask) @set_code_owner_attribute -def import_blocks_from_course(import_task_id, course_key_str): +def import_blocks_from_course(import_task_id, course_key_str, use_course_key_as_block_id_suffix=True): """ A Celery task to import blocks from a course through modulestore. """ @@ -72,7 +79,10 @@ def on_progress(block_key, block_num, block_count, exception=None): logger.info('Import block succesful: %s', block_key) import_task.save_progress(block_num / block_count) - edx_client = api.EdxModulestoreImportClient(library=import_task.library) + edx_client = api.EdxModulestoreImportClient( + library=import_task.library, + use_course_key_as_block_id_suffix=use_course_key_as_block_id_suffix + ) edx_client.import_blocks_from_course( course_key, on_progress ) @@ -84,14 +94,23 @@ def _import_block(store, user_id, source_block, dest_parent_key): """ def generate_block_key(source_key, dest_parent_key): """ - Deterministically generate an ID for the new block and return the key + Deterministically generate an ID for the new block and return the key. + Keys are generated such that they appear identical to a v1 library with + the same input block_id, library name, library organization, and parent block using derived_key """ - block_id = ( - dest_parent_key.block_id[:10] + - '-' + - hashlib.sha1(str(source_key).encode('utf-8')).hexdigest()[:10] + if not isinstance(source_key.lib_key, LibraryLocatorV2): + raise ValueError("Input must be an instance of LibraryLocatorV2") + source_key_as_v1_course_key = LibraryLocatorV1(org=source_key.lib_key.org, library=source_key.lib_key.slug) + source_key_usage_id_as_block_key = BlockKey( + type= source_key.block_type, + id=source_key.usage_id, + ) + block_id = derived_key( + source_key_as_v1_course_key, + source_key_usage_id_as_block_key, + BlockKey(dest_parent_key.block_type, dest_parent_key.block_id) ) - return dest_parent_key.context_key.make_usage_key(source_key.block_type, block_id) + return dest_parent_key.context_key.make_usage_key(source_key.block_type, block_id.id) source_key = source_block.scope_ids.usage_id new_block_key = generate_block_key(source_key, dest_parent_key) From 04c13aed19dc99e29fdd02526a6e9a09e829e61d Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 13 Dec 2023 14:54:53 +0000 Subject: [PATCH 2/5] fix: add dummy branch to v2->v1 key --- openedx/core/djangoapps/content_libraries/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index ca44e0bae700..9e338afc6176 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -100,7 +100,7 @@ def generate_block_key(source_key, dest_parent_key): """ if not isinstance(source_key.lib_key, LibraryLocatorV2): raise ValueError("Input must be an instance of LibraryLocatorV2") - source_key_as_v1_course_key = LibraryLocatorV1(org=source_key.lib_key.org, library=source_key.lib_key.slug) + source_key_as_v1_course_key = LibraryLocatorV1(org=source_key.lib_key.org, library=source_key.lib_key.slug, branch='library') source_key_usage_id_as_block_key = BlockKey( type= source_key.block_type, id=source_key.usage_id, From 472806d2e09a87812798f6ba3043854e090a9fb8 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Thu, 14 Dec 2023 16:06:05 +0000 Subject: [PATCH 3/5] fix: fixes based on feedback --- openedx/core/djangoapps/content_libraries/api.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 76a95e18fbf0..d25c3e829dbb 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1250,9 +1250,7 @@ def import_block(self, modulestore_key): """ Import a single modulestore block. """ - print(f'modulestore_key {modulestore_key}') block_data = self.get_block_data(modulestore_key) - print(f'block_data {block_data}') # Get or create the block in the library. # @@ -1264,19 +1262,18 @@ def import_block(self, modulestore_key): str(modulestore_key.course_key).encode() ).digest() )[:16].decode().lower() - print(f'course_key_id {course_key_id}') - # Prepend 'c' to allow changing hash without conflicts. # add the course_key_id if use_course_key_as_suffix is enabled to increase the namespace. - # in some cases, for the preservation of child-parent relationships - # of the same content across modulestore and blockstore, only the block_id is used. - block_id = block_id = ( + # The option exists to not use the course key as a suffix because + # in order to preserve learner state in the v1 to v2 libraries migration, + # the v2 and v1 libraries' child block ids must be the same. + block_id = ( + # Prepend 'c' to allow changing hash without conflicts. f"{modulestore_key.block_id}_c{course_key_id}" if self.use_course_key_as_block_id_suffix else f"{modulestore_key.block_id}" ) - print(f'block_id {block_id}') log.info('Importing to library block: id=%s', block_id) try: library_block = create_library_block( @@ -1355,7 +1352,6 @@ def __init__(self, modulestore_instance=None, **kwargs): """ Initialize the client with a modulestore instance. """ - print(kwargs.items()) super().__init__(**kwargs) self.modulestore = modulestore_instance or modulestore() From 536983372819cbd92ab6022c086be9228969037a Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Thu, 14 Dec 2023 18:58:32 +0000 Subject: [PATCH 4/5] fix: lint fixes --- cms/djangoapps/contentstore/tasks.py | 7 ++++--- .../core/djangoapps/content_libraries/tasks.py | 18 +++++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 646b9adf61de..b3a20f6c92b9 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -896,14 +896,15 @@ def _create_copy_content_task(v2_library_key, v1_library_key): spin up a celery task to import the V1 Library's content into the V2 library. This utalizes the fact that course and v1 library content is stored almost identically. """ - return v2contentlib_api.import_blocks_create_task(v2_library_key, v1_library_key, use_course_key_as_block_id_suffix=False) + return v2contentlib_api.import_blocks_create_task( + v2_library_key, v1_library_key, + use_course_key_as_block_id_suffix=False + ) def _create_metadata(v1_library_key, collection_uuid): """instansiate an index for the V2 lib in the collection""" - print(collection_uuid) - store = modulestore() v1_library = store.get_library(v1_library_key) collection = get_collection(collection_uuid).uuid diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 9e338afc6176..ad1f363218f0 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -17,7 +17,6 @@ from __future__ import annotations import logging -import hashlib from celery import shared_task from celery_utils.logged_task import LoggedTask @@ -28,7 +27,9 @@ from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import ( BlockUsageLocator, - LibraryUsageLocatorV2 + LibraryLocatorV2, + LibraryUsageLocatorV2, + LibraryLocator as LibraryLocatorV1 ) from user_tasks.tasks import UserTask, UserTaskStatus @@ -36,11 +37,6 @@ from common.djangoapps.student.auth import has_studio_write_access from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import ( - LibraryLocatorV2, - LibraryUsageLocatorV2, - LibraryLocator as LibraryLocatorV1 -) from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.xblock.api import load_block from openedx.core.lib import ensure_cms, blockstore_api @@ -100,9 +96,13 @@ def generate_block_key(source_key, dest_parent_key): """ if not isinstance(source_key.lib_key, LibraryLocatorV2): raise ValueError("Input must be an instance of LibraryLocatorV2") - source_key_as_v1_course_key = LibraryLocatorV1(org=source_key.lib_key.org, library=source_key.lib_key.slug, branch='library') + source_key_as_v1_course_key = LibraryLocatorV1( + org=source_key.lib_key.org, + library=source_key.lib_key.slug, + branch='library' + ) source_key_usage_id_as_block_key = BlockKey( - type= source_key.block_type, + type=source_key.block_type, id=source_key.usage_id, ) block_id = derived_key( From 02e86596d726122bf2dc8577e697c003add30285 Mon Sep 17 00:00:00 2001 From: connorhaugh <49422820+connorhaugh@users.noreply.github.com> Date: Mon, 18 Dec 2023 11:34:04 -0500 Subject: [PATCH 5/5] fix: expound error msg Co-authored-by: Kyle McCormick --- openedx/core/djangoapps/content_libraries/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index ad1f363218f0..13a6a32a1987 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -95,7 +95,7 @@ def generate_block_key(source_key, dest_parent_key): the same input block_id, library name, library organization, and parent block using derived_key """ if not isinstance(source_key.lib_key, LibraryLocatorV2): - raise ValueError("Input must be an instance of LibraryLocatorV2") + raise TypeError(f"Expected source library key of type LibraryLocatorV2, got {source_key.lib_key} instead.") source_key_as_v1_course_key = LibraryLocatorV1( org=source_key.lib_key.org, library=source_key.lib_key.slug,