Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: keep learner state associated after libraries migration #33920

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions cms/djangoapps/contentstore/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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
Expand Down
21 changes: 16 additions & 5 deletions openedx/core/djangoapps/content_libraries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
Expand Down Expand Up @@ -1261,8 +1262,18 @@ def import_block(self, modulestore_key):
str(modulestore_key.course_key).encode()
).digest()
)[:16].decode().lower()
# 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.
# 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}"
)

log.info('Importing to library block: id=%s', block_id)
try:
library_block = create_library_block(
Expand Down Expand Up @@ -1473,7 +1484,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.

Expand All @@ -1486,7 +1497,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}")
Expand Down
39 changes: 29 additions & 10 deletions openedx/core/djangoapps/content_libraries/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from __future__ import annotations

import logging
import hashlib

from celery import shared_task
from celery_utils.logged_task import LoggedTask
Expand All @@ -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
Expand All @@ -45,6 +46,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
Expand All @@ -55,7 +58,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.
"""
Expand All @@ -72,7 +75,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
)
Expand All @@ -84,14 +90,27 @@ 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
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved
"""
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")
connorhaugh marked this conversation as resolved.
Show resolved Hide resolved
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,
)
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)
Expand Down
Loading