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/api.py b/cms/djangoapps/modulestore_migrator/api.py index 498d18d8b1fd..317d1cb73db3 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 @@ -181,5 +182,30 @@ 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 } + + +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, +): + """ + 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 + } + 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/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..f043e208dc35 --- /dev/null +++ b/cms/djangoapps/modulestore_migrator/migrations/0004_alter_modulestoreblockmigration_target_squashed_0005_modulestoreblockmigration_unsupported_reason.py @@ -0,0 +1,32 @@ +# 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): + 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, + 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', + ), + ), + 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 810333fc9be9..149f6dd05d95 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 @@ -210,6 +213,9 @@ 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, ) change_log_record = models.OneToOneField( DraftChangeLogRecord, @@ -218,10 +224,16 @@ 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 = [ ('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/rest_api/v1/serializers.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py index 8c9f15387647..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 ModulestoreMigration, ModulestoreSource +from cms.djangoapps.modulestore_migrator.models import ( + ModulestoreMigration, + ModulestoreSource, +) class ModulestoreMigrationSerializer(serializers.Serializer): @@ -266,3 +269,12 @@ 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): + """ + 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/urls.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py index 44c872c6657a..208825e0c01a 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/urls.py @@ -1,9 +1,16 @@ """ 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 ( + BlockMigrationInfo, + BulkMigrationViewSet, + LibraryCourseMigrationViewSet, + MigrationInfoViewSet, + MigrationViewSet, +) ROUTER = SimpleRouter() ROUTER.register(r'migrations', MigrationViewSet, basename='migrations') @@ -17,4 +24,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..7ad377d7a3b7 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py @@ -7,33 +7,37 @@ from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser 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 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 ( + BlockMigrationInfoSerializer, BulkModulestoreMigrationSerializer, - MigrationInfoResponseSerializer, LibraryMigrationCourseSerializer, + MigrationInfoResponseSerializer, ModulestoreMigrationSerializer, StatusWithModulestoreMigrationsSerializer, ) @@ -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: str | bool | None = 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', 'unsupported_reason') + 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 ef2386c69d34..b4f24f149b8e 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -9,13 +9,15 @@ 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 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 +28,7 @@ LibraryContainerLocator, LibraryLocator, LibraryLocatorV2, - LibraryUsageLocatorV2 + LibraryUsageLocatorV2, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import ( @@ -35,14 +37,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 @@ -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. """ @@ -126,7 +127,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 +142,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. @@ -154,7 +155,13 @@ 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 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: self.existing_source_to_target_keys[source_key] = [target] @@ -166,7 +173,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 @@ -372,7 +379,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") ) @@ -450,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}" @@ -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 + source_to_target: SourceToTarget | 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[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, @@ -1010,7 +1020,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 +1031,19 @@ 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) + 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.getchildren()) + if children_length: + reason += ( + ngettext( + ' It has {count} children block.', + ' It has {count} children blocks.', + children_length, + ) + ).format(count=children_length) + 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( f"Cannot migrate node from {context.source_context_key} to {context.target_library_key} " @@ -1039,12 +1059,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, @@ -1076,7 +1098,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, @@ -1099,7 +1121,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( @@ -1108,7 +1130,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. @@ -1141,7 +1163,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, @@ -1152,7 +1174,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 @@ -1171,7 +1193,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( @@ -1194,8 +1216,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, @@ -1240,9 +1264,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. @@ -1325,7 +1351,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,7 +1360,8 @@ 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, + unsupported_reason=reason, ) processed += 1 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, diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index be5b296147fd..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 ) ) @@ -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 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