From 600eba56152c90c0abca58ae1cf6238c3be6f444 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sat, 22 Nov 2025 11:19:23 +0530 Subject: [PATCH 01/11] feat: store failed migration blocks in modulestore --- ..._alter_modulestoreblockmigration_target.py | 24 +++++++++++++++++++ cms/djangoapps/modulestore_migrator/models.py | 3 +++ cms/djangoapps/modulestore_migrator/tasks.py | 17 +++++++------ .../lib/xblock_serializer/block_serializer.py | 13 ++++++---- 4 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py diff --git a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py new file mode 100644 index 000000000000..1a25ad5fcec5 --- /dev/null +++ b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py @@ -0,0 +1,24 @@ +# Generated by Django 5.2.7 on 2025-11-19 06:23 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('modulestore_migrator', '0003_modulestoremigration_is_failed'), + ('oel_publishing', '0008_alter_draftchangelogrecord_options_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='modulestoreblockmigration', + name='target', + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to='oel_publishing.publishableentity', + ), + ), + ] diff --git a/cms/djangoapps/modulestore_migrator/models.py b/cms/djangoapps/modulestore_migrator/models.py index 810333fc9be9..6f8828b391b6 100644 --- a/cms/djangoapps/modulestore_migrator/models.py +++ b/cms/djangoapps/modulestore_migrator/models.py @@ -210,6 +210,8 @@ class ModulestoreBlockMigration(TimeStampedModel): target = models.ForeignKey( PublishableEntity, on_delete=models.CASCADE, + null=True, + blank=True, ) change_log_record = models.OneToOneField( DraftChangeLogRecord, @@ -222,6 +224,7 @@ class ModulestoreBlockMigration(TimeStampedModel): class Meta: unique_together = [ ('overall_migration', 'source'), + # By default defining a unique index on a nullable column will only enforce unicity of non-null values. ('overall_migration', 'target'), ] diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index ef2386c69d34..4c59dcdd6dda 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -154,7 +154,7 @@ def get_existing_target(self, source_key: UsageKey) -> PublishableEntity: # NOTE: This is a list of PublishableEntities, but we always return the first one. return self.existing_source_to_target_keys[source_key][0] - def add_migration(self, source_key: UsageKey, target: PublishableEntity) -> None: + def add_migration(self, source_key: UsageKey, target: PublishableEntity | None) -> None: """Update the context with a new migration (keeps it current)""" if source_key not in self.existing_source_to_target_keys: self.existing_source_to_target_keys[source_key] = [target] @@ -166,7 +166,7 @@ def get_existing_target_entity_keys(self, base_key: str) -> set[str]: publishable_entity.key for publishable_entity_list in self.existing_source_to_target_keys.values() for publishable_entity in publishable_entity_list - if publishable_entity.key.startswith(base_key) + if publishable_entity and publishable_entity.key.startswith(base_key) ) @property @@ -934,10 +934,10 @@ class _MigratedNode: This happens, particularly, if the node is above the requested composition level but has descendents which are at or below that level. """ - source_to_target: tuple[UsageKey, PublishableEntityVersion] | None + source_to_target: tuple[UsageKey, PublishableEntityVersion | None] | None children: list[_MigratedNode] - def all_source_to_target_pairs(self) -> t.Iterable[tuple[UsageKey, PublishableEntityVersion]]: + def all_source_to_target_pairs(self) -> t.Iterable[tuple[UsageKey, PublishableEntityVersion | None]]: """ Get all source_key->target_ver pairs via a pre-order traversal. """ @@ -1010,7 +1010,7 @@ def _migrate_node( children=[ migrated_child.source_to_target[1] for migrated_child in migrated_children if - migrated_child.source_to_target + migrated_child.source_to_target and migrated_child.source_to_target[1] ], ) if container_type else @@ -1021,9 +1021,8 @@ def _migrate_node( title=title, ) ) - if target_entity_version: - source_to_target = (source_key, target_entity_version) - context.add_migration(source_key, target_entity_version.entity) + source_to_target = (source_key, target_entity_version) + context.add_migration(source_key, target_entity_version.entity if target_entity_version else None) else: log.warning( f"Cannot migrate node from {context.source_context_key} to {context.target_library_key} " @@ -1334,7 +1333,7 @@ def _create_migration_artifacts_incrementally( ModulestoreBlockMigration.objects.create( overall_migration=migration, source=block_source, - target_id=target_version.entity_id, + target_id=target_version.entity_id if target_version else None, ) processed += 1 diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 76611624cdc6..169352730e1c 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -79,10 +79,15 @@ def _serialize_block(self, block) -> etree.Element: else: olx = self._serialize_normal_block(block) - # The url_name attribute can come either because it was already in the - # block's field data, or because this class adds it in the calls above. - # However it gets set though, we can remove it here: - if not self.write_url_name: + if self.write_url_name: + # Handles a weird case where url_name is not part of olx.attrib even if it is + # set in block. Known case is with openassessment blocks. + if "url_name" not in olx.attrib and hasattr(block, "url_name"): + olx.attrib['url_name'] = block.url_name + else: + # The url_name attribute can come either because it was already in the + # block's field data, or because this class adds it in the calls above. + # However it gets set though, we can remove it here: olx.attrib.pop("url_name", None) # Add copied_from_block and copied_from_version attribute the XBlock's OLX node, to help identify the source of From a017a0f117fb07a6da32b58399bc76bf926842b6 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Sat, 22 Nov 2025 20:35:15 +0530 Subject: [PATCH 02/11] feat: add api --- cms/djangoapps/modulestore_migrator/api.py | 22 ++++ cms/djangoapps/modulestore_migrator/models.py | 9 +- .../rest_api/v1/serializers.py | 7 +- .../modulestore_migrator/rest_api/v1/urls.py | 6 +- .../modulestore_migrator/rest_api/v1/views.py | 116 +++++++++++++++++- cms/djangoapps/modulestore_migrator/tasks.py | 16 +-- 6 files changed, 157 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/api.py b/cms/djangoapps/modulestore_migrator/api.py index 498d18d8b1fd..c589f7e33a61 100644 --- a/cms/djangoapps/modulestore_migrator/api.py +++ b/cms/djangoapps/modulestore_migrator/api.py @@ -1,6 +1,7 @@ """ API for migration from modulestore to learning core """ +from uuid import UUID from collections import defaultdict from celery.result import AsyncResult from opaque_keys import InvalidKeyError @@ -183,3 +184,24 @@ def construct_usage_key(lib_key_str: str, component: Component) -> LibraryUsageL for obj in query_set if obj.source.key is not None } + + +def get_migration_blocks_info( + target_key: str, + source_key: str | None, + target_collection_key: str | None, + task_uuid: str | None, + is_failed: bool | None, +): + filters = { + 'overall_migration__target__key': target_key + } + if source_key: + filters['overall_migration__source__key'] = source_key + if target_collection_key: + filters['overall_migration__target_collection__key'] = target_collection_key + if task_uuid: + filters['overall_migration__task_status__uuid'] = UUID(task_uuid) + if is_failed is not None: + filters['target__isnull'] = is_failed + return ModulestoreBlockMigration.objects.filter(**filters) diff --git a/cms/djangoapps/modulestore_migrator/models.py b/cms/djangoapps/modulestore_migrator/models.py index 6f8828b391b6..44d566290cc6 100644 --- a/cms/djangoapps/modulestore_migrator/models.py +++ b/cms/djangoapps/modulestore_migrator/models.py @@ -6,16 +6,19 @@ from django.contrib.auth import get_user_model from django.db import models from django.utils.translation import gettext_lazy as _ -from user_tasks.models import UserTaskStatus - from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import ( LearningContextKeyField, UsageKeyField, ) from openedx_learning.api.authoring_models import ( - LearningPackage, PublishableEntity, Collection, DraftChangeLog, DraftChangeLogRecord + Collection, + DraftChangeLog, + DraftChangeLogRecord, + LearningPackage, + PublishableEntity, ) +from user_tasks.models import UserTaskStatus from .data import CompositionLevel, RepeatHandlingStrategy diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py index 8c9f15387647..aa21a7c40097 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py @@ -11,7 +11,7 @@ from user_tasks.serializers import StatusSerializer from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy -from cms.djangoapps.modulestore_migrator.models import ModulestoreMigration, ModulestoreSource +from cms.djangoapps.modulestore_migrator.models import ModulestoreBlockMigration, ModulestoreMigration, ModulestoreSource class ModulestoreMigrationSerializer(serializers.Serializer): @@ -266,3 +266,8 @@ def get_progress(self, obj: ModulestoreMigration): Return the progress of the migration. """ return obj.task_status.completed_steps / obj.task_status.total_steps + + +class BlockMigrationInfoSerializer(serializers.Serializer): + source_key = serializers.CharField(source="source__key") + target_key = serializers.CharField(source="target__key") diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py index 44c872c6657a..bc0fc42f9b82 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py @@ -1,9 +1,10 @@ """ Course to Library Import API v1 URLs. """ -from django.urls import path, include +from django.urls import include, path from rest_framework.routers import SimpleRouter -from .views import MigrationViewSet, BulkMigrationViewSet, MigrationInfoViewSet, LibraryCourseMigrationViewSet + +from .views import BulkMigrationViewSet, LibraryCourseMigrationViewSet, MigrationInfoViewSet, MigrationViewSet, BlockMigrationInfo ROUTER = SimpleRouter() ROUTER.register(r'migrations', MigrationViewSet, basename='migrations') @@ -17,4 +18,5 @@ urlpatterns = [ path('', include(ROUTER.urls)), path('migration_info/', MigrationInfoViewSet.as_view(), name='migration-info'), + path('migration_blocks/', BlockMigrationInfo.as_view(), name='migration-blocks'), ] diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py index 3d887fff15f7..7cd56325fed2 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py @@ -6,36 +6,40 @@ import edx_api_doc_tools as apidocs from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from rest_framework.fields import BooleanField from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocatorV2 from rest_framework import status from rest_framework.exceptions import ParseError from rest_framework.mixins import ListModelMixin from rest_framework.permissions import IsAdminUser, IsAuthenticated +from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView from rest_framework.viewsets import GenericViewSet from user_tasks.models import UserTaskStatus from user_tasks.views import StatusViewSet -from opaque_keys.edx.keys import CourseKey from cms.djangoapps.modulestore_migrator.api import ( - start_migration_to_library, - start_bulk_migration_to_library, get_all_migrations_info, + get_migration_blocks_info, + start_bulk_migration_to_library, + start_migration_to_library, ) +from common.djangoapps.student.auth import has_studio_write_access from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser -from common.djangoapps.student.auth import has_studio_write_access from ...models import ModulestoreMigration from .serializers import ( BulkModulestoreMigrationSerializer, - MigrationInfoResponseSerializer, LibraryMigrationCourseSerializer, + MigrationInfoResponseSerializer, ModulestoreMigrationSerializer, StatusWithModulestoreMigrationsSerializer, + BlockMigrationInfoSerializer, ) log = logging.getLogger(__name__) @@ -493,3 +497,105 @@ def get_queryset(self): queryset = queryset.filter(target__key=library_key, source__key__startswith='course-v1') return queryset + + +class BlockMigrationInfo(APIView): + """ + Retrieve migration blocks information given task_uuid, source_key or target_key. + + It returns the migration block information for each block migrated by a specific task. + + API Endpoints + ------------- + GET /api/modulestore_migrator/v1/migration_blocks/ + Retrieve migration blocks info for given task_uuid, source_key or target_key. + + Query parameters: + task_uuid (str): task uuid + Example: ?task_uuid=dfe72eca-c54f-4b43-b53b-7996031f2102 + source_key (str): Source content key + Example: ?source_key=course-v1:UNIX+UX1+2025_T3 + target_key (str): target content key + Example: ?target_key=lib:UNIX:CIT1 + is_failed (boolean): has the block failed to migrate/import + Example: ?is_failed=true + + Example request: + GET /api/modulestore_migrator/v1/migration_blocks/?task_uuid=dfe72eca-c54f-4b43-b53b&is_failed=true + + Example response: + """ + + permission_classes = (IsAuthenticated,) + authentication_classes = ( + BearerAuthenticationAllowInactiveUser, + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter( + "target_key", + apidocs.ParameterLocation.QUERY, + description="Filter blocks by target key", + ), + apidocs.string_parameter( + "source_key", + apidocs.ParameterLocation.QUERY, + description="Filter blocks by source key", + ), + apidocs.string_parameter( + "target_collection_key", + apidocs.ParameterLocation.QUERY, + description="Filter blocks by target_collection_key", + ), + apidocs.string_parameter( + "task_uuid", + apidocs.ParameterLocation.QUERY, + description="Filter blocks by task_uuid", + ), + apidocs.string_parameter( + "is_failed", + apidocs.ParameterLocation.QUERY, + description="Filter blocks based on its migration status", + ), + ], + responses={ + 200: MigrationInfoResponseSerializer, + 400: "Missing required parameter: target_key", + 401: "The requester is not authenticated.", + }, + ) + def get(self, request: Request): + """ + Handle the migration info `GET` request + """ + source_key = request.query_params.get("source_key") + target_key = request.query_params.get("target_key") + target_collection_key = request.query_params.get("target_collection_key") + task_uuid = request.query_params.get("task_uuid") + is_failed = request.query_params.get("is_failed") + if not target_key: + return Response({"error": "Target key cannot be blank."}, status=400) + try: + target_key_parsed = LibraryLocatorV2.from_string(target_key) + except InvalidKeyError as e: + return Response({"error": str(e)}, status=400) + lib_api.require_permission_for_library_key( + target_key_parsed, + request.user, + lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) + if is_failed is not None: + is_failed = BooleanField().to_internal_value(is_failed) + + data = get_migration_blocks_info( + target_key, + source_key, + target_collection_key, + task_uuid, + is_failed, + ).values('source__key', 'target__key') + serializer = BlockMigrationInfoSerializer(data, many=True) + return Response(serializer.data) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 4c59dcdd6dda..2a22191a79e8 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -14,8 +14,9 @@ from celery import shared_task from celery.utils.log import get_task_logger from django.core.exceptions import ObjectDoesNotExist -from django.utils.text import slugify from django.db import transaction +from django.utils.text import slugify +from django.utils.translation import gettext_lazy as _ from edx_django_utils.monitoring import set_code_owner_attribute_from_module from lxml import etree from lxml.etree import _ElementTree as XmlTree @@ -26,7 +27,7 @@ LibraryContainerLocator, LibraryLocator, LibraryLocatorV2, - LibraryUsageLocatorV2 + LibraryUsageLocatorV2, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import ( @@ -35,14 +36,13 @@ ComponentType, LearningPackage, PublishableEntity, - PublishableEntityVersion + PublishableEntityVersion, ) from user_tasks.tasks import UserTask, UserTaskStatus from xblock.core import XBlock -from django.utils.translation import gettext_lazy as _ from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex -from common.djangoapps.util.date_utils import strftime_localized, DEFAULT_DATE_TIME_FORMAT +from common.djangoapps.util.date_utils import DEFAULT_DATE_TIME_FORMAT, strftime_localized from openedx.core.djangoapps.content_libraries import api as libraries_api from openedx.core.djangoapps.content_libraries.api import ContainerType, get_library from openedx.core.djangoapps.content_staging import api as staging_api @@ -126,7 +126,7 @@ class _MigrationContext: Context for the migration process. """ existing_source_to_target_keys: dict[ # Note: It's intended to be mutable to reflect changes during migration. - UsageKey, list[PublishableEntity] + UsageKey, list[PublishableEntity | None] ] target_package_id: int target_library_key: LibraryLocatorV2 @@ -141,7 +141,7 @@ class _MigrationContext: def is_already_migrated(self, source_key: UsageKey) -> bool: return source_key in self.existing_source_to_target_keys - def get_existing_target(self, source_key: UsageKey) -> PublishableEntity: + def get_existing_target(self, source_key: UsageKey) -> PublishableEntity | None: """ Get the target entity for a given source key. @@ -372,7 +372,7 @@ def _import_structure( # a given LearningPackage. # We use this mapping to ensure that we don't create duplicate # PublishableEntities during the migration process for a given LearningPackage. - existing_source_to_target_keys: dict[UsageKey, list[PublishableEntity]] = {} + existing_source_to_target_keys: dict[UsageKey, list[PublishableEntity | None]] = {} modulestore_blocks = ( ModulestoreBlockMigration.objects.filter(overall_migration__target=migration.target.id).order_by("source__key") ) From cbb89568af527346592bfdd65d6b42f4326aa580 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 25 Nov 2025 16:23:20 +0530 Subject: [PATCH 03/11] feat: unsupported reason --- cms/djangoapps/modulestore_migrator/admin.py | 1 + ...estoreblockmigration_unsupported_reason.py | 19 ++++++ cms/djangoapps/modulestore_migrator/models.py | 5 ++ cms/djangoapps/modulestore_migrator/tasks.py | 66 ++++++++++++++----- .../content_libraries/api/blocks.py | 8 ++- 5 files changed, 79 insertions(+), 20 deletions(-) create mode 100644 cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py diff --git a/cms/djangoapps/modulestore_migrator/admin.py b/cms/djangoapps/modulestore_migrator/admin.py index 8eef778531ac..7da37d18d169 100644 --- a/cms/djangoapps/modulestore_migrator/admin.py +++ b/cms/djangoapps/modulestore_migrator/admin.py @@ -178,6 +178,7 @@ class ModulestoreBlockMigrationInline(admin.TabularInline): "source", "target", "change_log_record", + "unsupported_reason", ) list_display = ("id", *readonly_fields) diff --git a/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py b/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py new file mode 100644 index 000000000000..ff062366d010 --- /dev/null +++ b/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.7 on 2025-11-25 10:44 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('modulestore_migrator', '0004_alter_modulestoreblockmigration_target'), + ] + + operations = [ + migrations.AddField( + model_name='modulestoreblockmigration', + name='unsupported_reason', + field=models.TextField( + blank=True, help_text='Reason if the block is unsupported and target is set to null', null=True + ), + ), + ] diff --git a/cms/djangoapps/modulestore_migrator/models.py b/cms/djangoapps/modulestore_migrator/models.py index 44d566290cc6..9802a9bf9cb2 100644 --- a/cms/djangoapps/modulestore_migrator/models.py +++ b/cms/djangoapps/modulestore_migrator/models.py @@ -223,6 +223,11 @@ class ModulestoreBlockMigration(TimeStampedModel): null=True, on_delete=models.SET_NULL, ) + unsupported_reason = models.TextField( + null=True, + blank=True, + help_text=_('Reason if the block is unsupported and target is set to null'), + ) class Meta: unique_together = [ diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 2a22191a79e8..34472afd0733 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -9,6 +9,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum +from gettext import ngettext from itertools import groupby from celery import shared_task @@ -84,7 +85,7 @@ class _MigrationTask(UserTask): """ @staticmethod - def calculate_total_steps(arguments_dict): + def calculate_total_steps(arguments_dict): # pylint: disable=unused-argument """ Get number of in-progress steps in importing process, as shown in the UI. """ @@ -154,6 +155,12 @@ def get_existing_target(self, source_key: UsageKey) -> PublishableEntity | None: # NOTE: This is a list of PublishableEntities, but we always return the first one. return self.existing_source_to_target_keys[source_key][0] + def is_already_successfully_migrated(self, source_key: UsageKey) -> bool: + """ + Check whether a source has successfully been migrated. This means it exists and has at least one target. + """ + return self.is_already_migrated(source_key) and self.get_existing_target(source_key) is not None + def add_migration(self, source_key: UsageKey, target: PublishableEntity | None) -> None: """Update the context with a new migration (keeps it current)""" if source_key not in self.existing_source_to_target_keys: @@ -465,7 +472,7 @@ def _create_collection(library_key: LibraryLocatorV2, title: str) -> Collection: title=f"{title}{f'_{attempt}' if attempt > 0 else ''}", description=description, ) - except libraries_api.LibraryCollectionAlreadyExists as e: + except libraries_api.LibraryCollectionAlreadyExists: attempt += 1 return collection @@ -925,6 +932,9 @@ def bulk_migrate_from_modulestore( status.fail(str(exc)) +SourceToTarget = tuple[UsageKey, PublishableEntityVersion | None, str | None] + + @dataclass(frozen=True) class _MigratedNode: """ @@ -934,10 +944,10 @@ class _MigratedNode: This happens, particularly, if the node is above the requested composition level but has descendents which are at or below that level. """ - source_to_target: tuple[UsageKey, PublishableEntityVersion | None] | None + source_to_target: SourceToTarget | None children: list[_MigratedNode] - def all_source_to_target_pairs(self) -> t.Iterable[tuple[UsageKey, PublishableEntityVersion | None]]: + def all_source_to_target_pairs(self) -> t.Iterable[SourceToTarget]: """ Get all source_key->target_ver pairs via a pre-order traversal. """ @@ -995,13 +1005,13 @@ def _migrate_node( ) for source_node_child in source_node.getchildren() ] - source_to_target: tuple[UsageKey, PublishableEntityVersion] | None = None + source_to_target: SourceToTarget | None = None if should_migrate_node: source_olx = etree.tostring(source_node).decode('utf-8') if source_block_id := source_node.get('url_name'): source_key: UsageKey = context.source_context_key.make_usage_key(source_node.tag, source_block_id) title = source_node.get('display_name', source_block_id) - target_entity_version = ( + target_entity_version, reason = ( _migrate_container( context=context, source_key=source_key, @@ -1021,7 +1031,20 @@ def _migrate_node( title=title, ) ) - source_to_target = (source_key, target_entity_version) + if container_type is None and target_entity_version is None and reason is not None: + # Currently, components with children are not supported + children_length = len(source_node.children) + if children_length: + reason += ( + ngettext( + 'It has {count} children block.', + 'It has {count} children blocks.', + children_length, + ) + ).format(count=children_length) + # __AUTO_GENERATED_PRINT_VAR_START__ + print(f"""======================================= _migrate_node reason: {reason}""") # __AUTO_GENERATED_PRINT_VAR_END__ + source_to_target = (source_key, target_entity_version, reason) context.add_migration(source_key, target_entity_version.entity if target_entity_version else None) else: log.warning( @@ -1038,12 +1061,14 @@ def _migrate_container( container_type: ContainerType, title: str, children: list[PublishableEntityVersion], -) -> PublishableEntityVersion: +) -> tuple[PublishableEntityVersion, str | None]: """ Create, update, or replace a container in a library based on a source key and children. (We assume that the destination is a library rather than some other future kind of learning - package, but let's keep than an internal assumption.) + package, but let's keep than an internal assumption.) + For now this returns None value for unsupported_reason as second value of tuple as we + don't have any concrete condition where a container cannot be imported/migrated. """ target_key = _get_distinct_target_container_key( context, @@ -1075,7 +1100,7 @@ def _migrate_container( return PublishableEntityVersion.objects.get( entity_id=container.container_pk, version_num=container.draft_version_num, - ) + ), None container_publishable_entity_version = authoring_api.create_next_container_version( container.container_pk, @@ -1098,7 +1123,7 @@ def _migrate_container( context.created_by, call_post_publish_events_sync=True, ) - return container_publishable_entity_version + return container_publishable_entity_version, None def _migrate_component( @@ -1107,7 +1132,7 @@ def _migrate_component( source_key: UsageKey, olx: str, title: str, -) -> PublishableEntityVersion | None: +) -> tuple[PublishableEntityVersion | None, str | None]: """ Create, update, or replace a component in a library based on a source key and OLX. @@ -1140,7 +1165,7 @@ def _migrate_component( ) except libraries_api.IncompatibleTypesError as e: log.error(f"Error validating block for library {context.target_library_key}: {e}") - return None + return None, str(e) component = authoring_api.create_component( context.target_package_id, component_type=component_type, @@ -1151,7 +1176,7 @@ def _migrate_component( # Component existed and we do not replace it and it is not deleted previously if component_existed and not component_deleted and context.should_skip_strategy: - return component.versioning.draft.publishable_entity_version + return component.versioning.draft.publishable_entity_version, None # If component existed and was deleted or we have to replace the current version # Create the new component version for it @@ -1170,7 +1195,7 @@ def _migrate_component( libraries_api.library_component_usage_key(context.target_library_key, component), context.created_by, ) - return component_version.publishable_entity_version + return component_version.publishable_entity_version, None def _get_distinct_target_container_key( @@ -1193,8 +1218,10 @@ def _get_distinct_target_container_key( """ # Check if we already processed this block and we are not forking. If we are forking, we will # want a new target key. - if context.is_already_migrated(source_key) and not context.should_fork_strategy: + if context.is_already_successfully_migrated(source_key) and not context.should_fork_strategy: existing_version = context.get_existing_target(source_key) + # This is not possible, just to satisfy type checker. + assert existing_version is not None return LibraryContainerLocator( context.target_library_key, @@ -1239,9 +1266,11 @@ def _get_distinct_target_usage_key( """ # Check if we already processed this block and we are not forking. If we are forking, we will # want a new target key. - if context.is_already_migrated(source_key) and not context.should_fork_strategy: + if context.is_already_successfully_migrated(source_key) and not context.should_fork_strategy: log.debug(f"Block {source_key} already exists, reusing first existing target") existing_target = context.get_existing_target(source_key) + # This is not possible, just to satisfy type checker. + assert existing_target is not None block_id = existing_target.component.local_key # mypy thinks LibraryUsageLocatorV2 is abstract. It's not. @@ -1324,7 +1353,7 @@ def _create_migration_artifacts_incrementally( total_nodes = len(nodes) processed = 0 - for source_usage_key, target_version in root_migrated_node.all_source_to_target_pairs(): + for source_usage_key, target_version, reason in root_migrated_node.all_source_to_target_pairs(): block_source, _ = ModulestoreBlockSource.objects.get_or_create( overall_source=source, key=source_usage_key @@ -1334,6 +1363,7 @@ def _create_migration_artifacts_incrementally( overall_migration=migration, source=block_source, target_id=target_version.entity_id if target_version else None, + unsupported_reason=reason, ) processed += 1 diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index be5b296147fd..b60f9363cc19 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -309,7 +309,9 @@ def validate_can_add_block_to_library( block_class = XBlock.load_class(block_type) # Will raise an exception if invalid if block_class.has_children: raise IncompatibleTypesError( - 'The "{block_type}" XBlock (ID: "{block_id}") has children, so it not supported in content libraries', + _( + 'The "{block_type}" XBlock (ID: "{block_id}") has children, so it not supported in content libraries' + ).format(block_type=block_type, block_id=block_id) ) # Make sure the new ID is not taken already: usage_key = LibraryUsageLocatorV2( # type: ignore[abstract] @@ -319,7 +321,9 @@ def validate_can_add_block_to_library( ) if _component_exists(usage_key): - raise LibraryBlockAlreadyExists(f"An XBlock with ID '{usage_key}' already exists") + raise LibraryBlockAlreadyExists( + _("An XBlock with ID '{usage_key}' already exists").format(usage_key=usage_key) + ) return content_library, usage_key From 8f2f2b4c219d4f6bc51b31d6c6beab971fb8e7e3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 25 Nov 2025 16:29:02 +0530 Subject: [PATCH 04/11] fixup! feat: unsupported reason --- cms/djangoapps/modulestore_migrator/tasks.py | 10 ++++------ .../core/djangoapps/content_libraries/api/blocks.py | 6 +++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 34472afd0733..b4f24f149b8e 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -457,7 +457,7 @@ def _create_collection(library_key: LibraryLocatorV2, title: str) -> Collection: The same is true for the title. """ key = slugify(title) - collection = None + collection: Collection | None = None attempt = 0 created_at = strftime_localized(datetime.now(timezone.utc), DEFAULT_DATE_TIME_FORMAT) description = f"{_('This collection contains content migrated from a legacy library on')}: {created_at}" @@ -1033,17 +1033,15 @@ def _migrate_node( ) if container_type is None and target_entity_version is None and reason is not None: # Currently, components with children are not supported - children_length = len(source_node.children) + children_length = len(source_node.getchildren()) if children_length: reason += ( ngettext( - 'It has {count} children block.', - 'It has {count} children blocks.', + ' It has {count} children block.', + ' It has {count} children blocks.', children_length, ) ).format(count=children_length) - # __AUTO_GENERATED_PRINT_VAR_START__ - print(f"""======================================= _migrate_node reason: {reason}""") # __AUTO_GENERATED_PRINT_VAR_END__ source_to_target = (source_key, target_entity_version, reason) context.add_migration(source_key, target_entity_version.entity if target_entity_version else None) else: diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index b60f9363cc19..4cf46627d1d2 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -298,7 +298,7 @@ def validate_can_add_block_to_library( component_count = authoring_api.get_all_drafts(content_library.learning_package_id).count() if component_count + 1 > settings.MAX_BLOCKS_PER_CONTENT_LIBRARY: raise BlockLimitReachedError( - _("Library cannot have more than {} Components").format( + _("Library cannot have more than {} Components.").format( settings.MAX_BLOCKS_PER_CONTENT_LIBRARY ) ) @@ -310,7 +310,7 @@ def validate_can_add_block_to_library( if block_class.has_children: raise IncompatibleTypesError( _( - 'The "{block_type}" XBlock (ID: "{block_id}") has children, so it not supported in content libraries' + 'The "{block_type}" XBlock (ID: "{block_id}") has children, so it not supported in content libraries.' ).format(block_type=block_type, block_id=block_id) ) # Make sure the new ID is not taken already: @@ -322,7 +322,7 @@ def validate_can_add_block_to_library( if _component_exists(usage_key): raise LibraryBlockAlreadyExists( - _("An XBlock with ID '{usage_key}' already exists").format(usage_key=usage_key) + _("An XBlock with ID '{usage_key}' already exists.").format(usage_key=usage_key) ) return content_library, usage_key From 080e429321f006c865d63209081ac17f24fee27d Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 26 Nov 2025 12:00:01 +0530 Subject: [PATCH 05/11] feat: add reason to api --- cms/djangoapps/modulestore_migrator/api.py | 2 +- .../modulestore_migrator/rest_api/v1/serializers.py | 4 ++++ cms/djangoapps/modulestore_migrator/rest_api/v1/views.py | 6 +++--- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/api.py b/cms/djangoapps/modulestore_migrator/api.py index c589f7e33a61..c13a41d4d80c 100644 --- a/cms/djangoapps/modulestore_migrator/api.py +++ b/cms/djangoapps/modulestore_migrator/api.py @@ -193,7 +193,7 @@ def get_migration_blocks_info( task_uuid: str | None, is_failed: bool | None, ): - filters = { + filters: dict[str, str | UUID | bool] = { 'overall_migration__target__key': target_key } if source_key: diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py index aa21a7c40097..0963ec7582a1 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py @@ -269,5 +269,9 @@ def get_progress(self, obj: ModulestoreMigration): class BlockMigrationInfoSerializer(serializers.Serializer): + """ + Serializer for the block migration info. + """ source_key = serializers.CharField(source="source__key") target_key = serializers.CharField(source="target__key") + unsupported_reason = serializers.CharField() diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py index 7cd56325fed2..51b0993df63d 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py @@ -6,12 +6,12 @@ import edx_api_doc_tools as apidocs from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser -from rest_framework.fields import BooleanField from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocatorV2 from rest_framework import status from rest_framework.exceptions import ParseError +from rest_framework.fields import BooleanField from rest_framework.mixins import ListModelMixin from rest_framework.permissions import IsAdminUser, IsAuthenticated from rest_framework.request import Request @@ -34,12 +34,12 @@ from ...models import ModulestoreMigration from .serializers import ( + BlockMigrationInfoSerializer, BulkModulestoreMigrationSerializer, LibraryMigrationCourseSerializer, MigrationInfoResponseSerializer, ModulestoreMigrationSerializer, StatusWithModulestoreMigrationsSerializer, - BlockMigrationInfoSerializer, ) log = logging.getLogger(__name__) @@ -596,6 +596,6 @@ def get(self, request: Request): target_collection_key, task_uuid, is_failed, - ).values('source__key', 'target__key') + ).values('source__key', 'target__key', 'unsupported_reason') serializer = BlockMigrationInfoSerializer(data, many=True) return Response(serializer.data) From 61e97be9aa882472795dcd2a06c4bfd8f62a2666 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 26 Nov 2025 12:11:58 +0530 Subject: [PATCH 06/11] chore: squash migration --- ...estoreblockmigration_unsupported_reason.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py diff --git a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py new file mode 100644 index 000000000000..9fef74fbdfc5 --- /dev/null +++ b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py @@ -0,0 +1,36 @@ +# Generated by Django 5.2.7 on 2025-11-26 06:35 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + replaces = [ + ('modulestore_migrator', '0004_alter_modulestoreblockmigration_target'), + ('modulestore_migrator', '0005_modulestoreblockmigration_unsupported_reason'), + ] + + dependencies = [ + ('modulestore_migrator', '0003_modulestoremigration_is_failed'), + ('oel_publishing', '0008_alter_draftchangelogrecord_options_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='modulestoreblockmigration', + name='target', + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to='oel_publishing.publishableentity', + ), + ), + migrations.AddField( + model_name='modulestoreblockmigration', + name='unsupported_reason', + field=models.TextField( + blank=True, help_text='Reason if the block is unsupported and target is set to null', null=True + ), + ), + ] From 757b528384e6a2963e3b3de48597ab0d2306026c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 26 Nov 2025 13:02:56 +0530 Subject: [PATCH 07/11] test: add tests --- .../modulestore_migrator/tests/test_tasks.py | 171 ++++++++++++++---- 1 file changed, 133 insertions(+), 38 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py index 244d435de50a..9b4ac4130c59 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py @@ -3,38 +3,39 @@ """ from unittest.mock import Mock, patch + import ddt from django.utils import timezone from lxml import etree from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 -from openedx_learning.api.authoring_models import Collection, PublishableEntityVersion from openedx_learning.api import authoring as authoring_api +from openedx_learning.api.authoring_models import Collection, PublishableEntityVersion from organizations.tests.factories import OrganizationFactory from user_tasks.models import UserTaskArtifact from user_tasks.tasks import UserTaskStatus -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory -from common.djangoapps.student.tests.factories import UserFactory from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy from cms.djangoapps.modulestore_migrator.models import ( ModulestoreMigration, ModulestoreSource, ) from cms.djangoapps.modulestore_migrator.tasks import ( + MigrationStep, + _BulkMigrationTask, _migrate_component, _migrate_container, _migrate_node, _MigratedNode, _MigrationContext, _MigrationTask, - _BulkMigrationTask, - migrate_from_modulestore, bulk_migrate_from_modulestore, - MigrationStep, + migrate_from_modulestore, ) +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as lib_api +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory @ddt.ddt @@ -235,9 +236,10 @@ def test_migrate_node_container_composition_level( if should_migrate: self.assertIsNotNone(result.source_to_target) - source_key, _ = result.source_to_target + source_key, _, reason = result.source_to_target self.assertEqual(source_key.block_type, tag_name) self.assertEqual(source_key.block_id, f"test_{tag_name}") + self.assertIsNone(reason) else: self.assertIsNone(result.source_to_target) @@ -269,6 +271,45 @@ def test_migrate_node_without_url_name(self): self.assertIsNone(result.source_to_target) self.assertEqual(len(result.children), 0) + def test_migrate_node_with_children_components(self): + """ + Test _migrate_node handles nodes with children components + """ + node_without_url_name = etree.fromstring(''' + + + + + ''') + context = _MigrationContext( + existing_source_to_target_keys={}, + target_package_id=self.learning_package.id, + target_library_key=self.library.library_key, + source_context_key=self.course.id, + content_by_filename={}, + composition_level=CompositionLevel.Unit, + repeat_handling_strategy=RepeatHandlingStrategy.Skip, + preserve_url_slugs=True, + created_at=timezone.now(), + created_by=self.user.id, + ) + + result = _migrate_node( + context=context, + source_node=node_without_url_name, + ) + + self.assertEqual( + result.source_to_target, + ( + self.course.id.make_usage_key('library_content', 'test_library_content'), + None, + 'The "library_content" XBlock (ID: "test_library_content") has children, ' + 'so it not supported in content libraries. It has 2 children blocks.', + ), + ) + self.assertEqual(len(result.children), 0) + def test_migrated_node_all_source_to_target_pairs(self): """ Test _MigratedNode.all_source_to_target_pairs traversal @@ -281,11 +322,11 @@ def test_migrated_node_all_source_to_target_pairs(self): key2 = self.course.id.make_usage_key("problem", "problem2") key3 = self.course.id.make_usage_key("problem", "problem3") - child_node = _MigratedNode(source_to_target=(key3, mock_version3), children=[]) + child_node = _MigratedNode(source_to_target=(key3, mock_version3, None), children=[]) parent_node = _MigratedNode( - source_to_target=(key1, mock_version1), + source_to_target=(key1, mock_version1, None), children=[ - _MigratedNode(source_to_target=(key2, mock_version2), children=[]), + _MigratedNode(source_to_target=(key2, mock_version2, None), children=[]), child_node, ], ) @@ -426,13 +467,14 @@ def test_migrate_component_success(self): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, title="test_problem" ) + self.assertIsNone(reason) self.assertIsNotNone(result) self.assertIsInstance(result, PublishableEntityVersion) @@ -443,6 +485,44 @@ def test_migrate_component_success(self): # The component is published self.assertFalse(result.componentversion.component.versioning.has_unpublished_changes) + def test_migrate_component_failure(self): + """ + Test _migrate_component fails to import component with children + """ + source_key = self.course.id.make_usage_key("library_content", "test_library_content") + olx = ''' + + + + + ''' + context = _MigrationContext( + existing_source_to_target_keys={}, + target_package_id=self.learning_package.id, + target_library_key=self.library.library_key, + source_context_key=self.course.id, + content_by_filename={}, + composition_level=CompositionLevel.Unit, + repeat_handling_strategy=RepeatHandlingStrategy.Skip, + preserve_url_slugs=True, + created_at=timezone.now(), + created_by=self.user.id, + ) + + result, reason = _migrate_component( + context=context, + source_key=source_key, + olx=olx, + title="test_library content" + ) + + self.assertIsNone(result) + self.assertEqual( + reason, + 'The "library_content" XBlock (ID: "test_library_content") has children,' + ' so it not supported in content libraries.', + ) + def test_migrate_component_with_static_content(self): """ Test _migrate_component with static file content @@ -470,7 +550,7 @@ def test_migrate_component_with_static_content(self): created_at=timezone.now(), created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -478,6 +558,7 @@ def test_migrate_component_with_static_content(self): ) self.assertIsNotNone(result) + self.assertIsNone(reason) component_content = result.componentversion.componentversioncontent_set.filter( key="static/test_image.png" @@ -504,7 +585,7 @@ def test_migrate_component_replace_existing_false(self): created_by=self.user.id, ) - first_result = _migrate_component( + first_result, first_reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -513,12 +594,14 @@ def test_migrate_component_replace_existing_false(self): context.existing_source_to_target_keys[source_key] = [first_result.entity] - second_result = _migrate_component( + second_result, second_reason = _migrate_component( context=context, source_key=source_key, olx='', title="updated_problem" ) + self.assertIsNone(first_reason) + self.assertIsNone(second_reason) self.assertEqual(first_result.entity_id, second_result.entity_id) self.assertEqual(first_result.version_num, second_result.version_num) @@ -546,7 +629,7 @@ def test_migrate_component_same_title(self): created_by=self.user.id, ) - first_result = _migrate_component( + first_result, first_reason = _migrate_component( context=context, source_key=source_key_1, olx=olx, @@ -555,12 +638,14 @@ def test_migrate_component_same_title(self): context.existing_source_to_target_keys[source_key_1] = [first_result.entity] - second_result = _migrate_component( + second_result, second_reason = _migrate_component( context=context, source_key=source_key_2, olx=olx, title="test_problem" ) + self.assertIsNone(first_reason) + self.assertIsNone(second_reason) self.assertNotEqual(first_result.entity_id, second_result.entity_id) self.assertNotEqual(first_result.entity.key, second_result.entity.key) @@ -584,7 +669,7 @@ def test_migrate_component_replace_existing_true(self): created_by=self.user.id, ) - first_result = _migrate_component( + first_result, first_reason = _migrate_component( context=context, source_key=source_key, olx=original_olx, @@ -594,12 +679,14 @@ def test_migrate_component_replace_existing_true(self): context.existing_source_to_target_keys[source_key] = [first_result.entity] updated_olx = '' - second_result = _migrate_component( + second_result, second_reason = _migrate_component( context=context, source_key=source_key, olx=updated_olx, title="updated" ) + self.assertIsNone(first_reason) + self.assertIsNone(second_reason) self.assertEqual(first_result.entity_id, second_result.entity_id) self.assertNotEqual(first_result.version_num, second_result.version_num) @@ -626,7 +713,7 @@ def test_migrate_component_different_block_types(self): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -634,6 +721,7 @@ def test_migrate_component_different_block_types(self): ) self.assertIsNotNone(result, f"Failed to migrate {block_type}") + self.assertIsNone(reason) self.assertEqual( block_type, result.componentversion.component.component_type.name @@ -679,7 +767,7 @@ def test_migrate_component_content_filename_not_in_olx(self): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -687,6 +775,7 @@ def test_migrate_component_content_filename_not_in_olx(self): ) self.assertIsNotNone(result) + self.assertIsNone(reason) referenced_content_exists = ( result.componentversion.componentversioncontent_set.filter( @@ -722,7 +811,7 @@ def test_migrate_component_library_source_key(self): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -730,6 +819,7 @@ def test_migrate_component_library_source_key(self): ) self.assertIsNotNone(result) + self.assertIsNone(reason) self.assertEqual( "problem", result.componentversion.component.component_type.name @@ -765,21 +855,23 @@ def test_migrate_component_duplicate_content_integrity_error(self): created_by=self.user.id, ) - first_result = _migrate_component( + first_result, first_reason = _migrate_component( context=context, source_key=source_key, olx=olx, title="test_problem" ) + self.assertIsNone(first_reason) context.existing_source_to_target_keys[source_key] = [first_result.entity] - second_result = _migrate_component( + second_result, second_reason = _migrate_component( context=context, source_key=source_key, olx=olx, title="test_problem" ) + self.assertIsNone(second_reason) self.assertIsNotNone(first_result) self.assertIsNotNone(second_result) @@ -840,7 +932,7 @@ def test_migrate_container_creates_new_container(self): created_by=self.user.id, ) - result = _migrate_container( + result, reason = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -848,6 +940,7 @@ def test_migrate_container_creates_new_container(self): children=children, ) + self.assertIsNone(reason) self.assertIsInstance(result, PublishableEntityVersion) container_version = result.containerversion @@ -888,7 +981,7 @@ def test_migrate_container_different_container_types(self): block_type, f"test_{block_type}" ) - result = _migrate_container( + result, reason = _migrate_container( context=context, source_key=source_key, container_type=container_type, @@ -896,6 +989,7 @@ def test_migrate_container_different_container_types(self): children=[], ) + self.assertIsNone(reason) self.assertIsNotNone(result) container_version = result.containerversion @@ -921,7 +1015,7 @@ def test_migrate_container_replace_existing_false(self): created_by=self.user.id, ) - first_result = _migrate_container( + first_result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -931,7 +1025,7 @@ def test_migrate_container_replace_existing_false(self): context.existing_source_to_target_keys[source_key] = [first_result.entity] - second_result = _migrate_container( + second_result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -967,7 +1061,7 @@ def test_migrate_container_same_title(self): created_by=self.user.id, ) - first_result = _migrate_container( + first_result, _ = _migrate_container( context=context, source_key=source_key_1, container_type=lib_api.ContainerType.Unit, @@ -977,7 +1071,7 @@ def test_migrate_container_same_title(self): context.existing_source_to_target_keys[source_key_1] = [first_result.entity] - second_result = _migrate_container( + second_result, _ = _migrate_container( context=context, source_key=source_key_2, container_type=lib_api.ContainerType.Unit, @@ -1027,7 +1121,7 @@ def test_migrate_container_replace_existing_true(self): created_by=self.user.id, ) - first_result = _migrate_container( + first_result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1037,7 +1131,7 @@ def test_migrate_container_replace_existing_true(self): context.existing_source_to_target_keys[source_key] = [first_result.entity] - second_result = _migrate_container( + second_result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1071,7 +1165,7 @@ def test_migrate_container_with_library_source_key(self): created_by=self.user.id, ) - result = _migrate_container( + result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1102,7 +1196,7 @@ def test_migrate_container_empty_children_list(self): created_by=self.user.id, ) - result = _migrate_container( + result, reason = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1110,6 +1204,7 @@ def test_migrate_container_empty_children_list(self): children=[], ) + self.assertIsNone(reason) self.assertIsNotNone(result) container_version = result.containerversion @@ -1151,7 +1246,7 @@ def test_migrate_container_preserves_child_order(self): ) children.append(child_version.publishable_entity_version) - result = _migrate_container( + result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1240,7 +1335,7 @@ def test_migrate_container_with_mixed_child_types(self): created_by=self.user.id, ) - result = _migrate_container( + result, _ = _migrate_container( context=context, source_key=source_key, container_type=lib_api.ContainerType.Unit, @@ -1279,7 +1374,7 @@ def test_migrate_container_generates_correct_target_key(self): created_by=self.user.id, ) - course_result = _migrate_container( + course_result, _ = _migrate_container( context=context, source_key=course_source_key, container_type=lib_api.ContainerType.Unit, @@ -1291,7 +1386,7 @@ def test_migrate_container_generates_correct_target_key(self): library_key = LibraryLocator(org="TestOrg", library="TestLibrary") library_source_key = library_key.make_usage_key("vertical", "test_vertical") - library_result = _migrate_container( + library_result, _ = _migrate_container( context=context, source_key=library_source_key, container_type=lib_api.ContainerType.Unit, From e66e9ed61f9f1194f9abc373bb47c86c29ac6652 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 26 Nov 2025 15:33:52 +0530 Subject: [PATCH 08/11] chore: fix lint issues --- cms/djangoapps/modulestore_migrator/api.py | 6 +++++- .../modulestore_migrator/rest_api/v1/serializers.py | 5 ++++- cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py | 8 +++++++- cms/djangoapps/modulestore_migrator/rest_api/v1/views.py | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/api.py b/cms/djangoapps/modulestore_migrator/api.py index c13a41d4d80c..317d1cb73db3 100644 --- a/cms/djangoapps/modulestore_migrator/api.py +++ b/cms/djangoapps/modulestore_migrator/api.py @@ -182,7 +182,7 @@ def construct_usage_key(lib_key_str: str, component: Component) -> LibraryUsageL return { obj.source.key: construct_usage_key(obj.target.learning_package.key, obj.target.component) for obj in query_set - if obj.source.key is not None + if obj.source.key is not None and obj.target is not None } @@ -193,6 +193,10 @@ def get_migration_blocks_info( task_uuid: str | None, is_failed: bool | None, ): + """ + Given the target key, and optional source key, target collection key, task_uuid and is_failed get a dictionary + containing information about migration blocks. + """ filters: dict[str, str | UUID | bool] = { 'overall_migration__target__key': target_key } diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py index 0963ec7582a1..5aae216a88a8 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py @@ -11,7 +11,10 @@ from user_tasks.serializers import StatusSerializer from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy -from cms.djangoapps.modulestore_migrator.models import ModulestoreBlockMigration, ModulestoreMigration, ModulestoreSource +from cms.djangoapps.modulestore_migrator.models import ( + ModulestoreMigration, + ModulestoreSource, +) class ModulestoreMigrationSerializer(serializers.Serializer): diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py index bc0fc42f9b82..208825e0c01a 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py @@ -4,7 +4,13 @@ from django.urls import include, path from rest_framework.routers import SimpleRouter -from .views import BulkMigrationViewSet, LibraryCourseMigrationViewSet, MigrationInfoViewSet, MigrationViewSet, BlockMigrationInfo +from .views import ( + BlockMigrationInfo, + BulkMigrationViewSet, + LibraryCourseMigrationViewSet, + MigrationInfoViewSet, + MigrationViewSet, +) ROUTER = SimpleRouter() ROUTER.register(r'migrations', MigrationViewSet, basename='migrations') diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py index 51b0993df63d..7ad377d7a3b7 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py @@ -575,7 +575,7 @@ def get(self, request: Request): target_key = request.query_params.get("target_key") target_collection_key = request.query_params.get("target_collection_key") task_uuid = request.query_params.get("task_uuid") - is_failed = request.query_params.get("is_failed") + is_failed: str | bool | None = request.query_params.get("is_failed") if not target_key: return Response({"error": "Target key cannot be blank."}, status=400) try: From d69ce899d1c5f3b5537ab6f33ce082cbbdda6e86 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 27 Nov 2025 10:58:07 +0530 Subject: [PATCH 09/11] chore: delete squashed migrations --- ..._alter_modulestoreblockmigration_target.py | 24 ------------------- ...estoreblockmigration_unsupported_reason.py | 5 ---- ...estoreblockmigration_unsupported_reason.py | 19 --------------- 3 files changed, 48 deletions(-) delete mode 100644 cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py delete mode 100644 cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py diff --git a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py deleted file mode 100644 index 1a25ad5fcec5..000000000000 --- a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target.py +++ /dev/null @@ -1,24 +0,0 @@ -# Generated by Django 5.2.7 on 2025-11-19 06:23 - -import django.db.models.deletion -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ('modulestore_migrator', '0003_modulestoremigration_is_failed'), - ('oel_publishing', '0008_alter_draftchangelogrecord_options_and_more'), - ] - - operations = [ - migrations.AlterField( - model_name='modulestoreblockmigration', - name='target', - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.CASCADE, - to='oel_publishing.publishableentity', - ), - ), - ] diff --git a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py index 9fef74fbdfc5..27544ea1c242 100644 --- a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py +++ b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py @@ -5,11 +5,6 @@ class Migration(migrations.Migration): - replaces = [ - ('modulestore_migrator', '0004_alter_modulestoreblockmigration_target'), - ('modulestore_migrator', '0005_modulestoreblockmigration_unsupported_reason'), - ] - dependencies = [ ('modulestore_migrator', '0003_modulestoremigration_is_failed'), ('oel_publishing', '0008_alter_draftchangelogrecord_options_and_more'), diff --git a/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py b/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py deleted file mode 100644 index ff062366d010..000000000000 --- a/cms/djangoapps/modulestore_migrator/migrations/0005_modulestoreblockmigration_unsupported_reason.py +++ /dev/null @@ -1,19 +0,0 @@ -# Generated by Django 5.2.7 on 2025-11-25 10:44 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ('modulestore_migrator', '0004_alter_modulestoreblockmigration_target'), - ] - - operations = [ - migrations.AddField( - model_name='modulestoreblockmigration', - name='unsupported_reason', - field=models.TextField( - blank=True, help_text='Reason if the block is unsupported and target is set to null', null=True - ), - ), - ] From cc1e6b0d1771d4c507ab026c418ef78053b80272 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 27 Nov 2025 11:00:27 +0530 Subject: [PATCH 10/11] chore: add help text to target field --- cms/djangoapps/modulestore_migrator/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/djangoapps/modulestore_migrator/models.py b/cms/djangoapps/modulestore_migrator/models.py index 9802a9bf9cb2..149f6dd05d95 100644 --- a/cms/djangoapps/modulestore_migrator/models.py +++ b/cms/djangoapps/modulestore_migrator/models.py @@ -213,6 +213,7 @@ class ModulestoreBlockMigration(TimeStampedModel): target = models.ForeignKey( PublishableEntity, on_delete=models.CASCADE, + help_text=_('The target entity of this block migration, set to null if it fails to migrate'), null=True, blank=True, ) From e80342d352b8b745c60590a70d46964ef10caf43 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 27 Nov 2025 19:59:22 +0530 Subject: [PATCH 11/11] fix: migration --- ...squashed_0005_modulestoreblockmigration_unsupported_reason.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py index 27544ea1c242..f043e208dc35 100644 --- a/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py +++ b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py @@ -16,6 +16,7 @@ class Migration(migrations.Migration): name='target', field=models.ForeignKey( blank=True, + help_text='The target entity of this block migration, set to null if it fails to migrate', null=True, on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.publishableentity',