diff --git a/cms/djangoapps/modulestore_migrator/admin.py b/cms/djangoapps/modulestore_migrator/admin.py index 8eef778531..7da37d18d1 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 498d18d8b1..317d1cb73d 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 get_target_block_usage_keys(source_key: CourseKey | LibraryLocator) -> dict[ 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 0000000000..f043e208dc --- /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 810333fc9b..149f6dd05d 100644 --- a/cms/djangoapps/modulestore_migrator/models.py +++ b/cms/djangoapps/modulestore_migrator/models.py @@ -6,16 +6,19 @@ from __future__ import annotations 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 8c9f153876..5aae216a88 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.models import UserTaskStatus 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 @@ class LibraryMigrationCourseSerializer(serializers.ModelSerializer): 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 44c872c665..208825e0c0 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 @@ ROUTER.register( 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 3d887fff15..7ad377d7a3 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 @@ 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 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 @@ class LibraryCourseMigrationViewSet(GenericViewSet, ListModelMixin): 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 ef2386c69d..b4f24f149b 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -9,13 +9,15 @@ import typing as t 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 @@ from opaque_keys.edx.locator import ( 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 @@ from openedx_learning.api.authoring_models import ( 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 @@ class _MigrationContext: # 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 @@ class _MigrationContext: 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 244d435de5..9b4ac4130c 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py @@ -3,38 +3,39 @@ Tests for the modulestore_migrator tasks """ 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): # 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): ) self.assertIsNotNone(result) + self.assertIsNone(reason) component_content = result.componentversion.componentversioncontent_set.filter( key="static/test_image.png" @@ -504,7 +585,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -634,6 +721,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): ) 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -687,6 +775,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): ) self.assertIsNotNone(result) + self.assertIsNone(reason) referenced_content_exists = ( result.componentversion.componentversioncontent_set.filter( @@ -722,7 +811,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): created_by=self.user.id, ) - result = _migrate_component( + result, reason = _migrate_component( context=context, source_key=source_key, olx=olx, @@ -730,6 +819,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): ) self.assertIsNotNone(result) + self.assertIsNone(reason) self.assertEqual( "problem", result.componentversion.component.component_type.name @@ -765,21 +855,23 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): children=children, ) + self.assertIsNone(reason) self.assertIsInstance(result, PublishableEntityVersion) container_version = result.containerversion @@ -888,7 +981,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): children=[], ) + self.assertIsNone(reason) self.assertIsNotNone(result) container_version = result.containerversion @@ -921,7 +1015,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): children=[], ) + self.assertIsNone(reason) self.assertIsNotNone(result) container_version = result.containerversion @@ -1151,7 +1246,7 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): ) 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 @@ class TestMigrateFromModulestore(ModuleStoreTestCase): 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 be5b296147..4cf46627d1 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/djangoapps/notifications/README.md b/openedx/core/djangoapps/notifications/README.md new file mode 100644 index 0000000000..10e1ad81d8 --- /dev/null +++ b/openedx/core/djangoapps/notifications/README.md @@ -0,0 +1,5 @@ +# Notifications + +Functionality for notifications on Open edX. + +See the [./docs/](./docs/) directory for docs. diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index 5264053ace..1fec0d5930 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -1,6 +1,8 @@ """ Base setup for Notification Apps and Types. """ +from typing import Any, Literal, TypedDict, NotRequired + from django.utils.translation import gettext_lazy as _ from .email_notifications import EmailCadence @@ -11,6 +13,54 @@ from .notification_content import get_notification_type_context_function FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE = 'filter_audit_expired_users_with_no_role' + +class NotificationType(TypedDict): + """ + Define the fields for values in COURSE_NOTIFICATION_TYPES + """ + # The notification app associated with this notification. + # Must be a key in COURSE_NOTIFICATION_APPS. + notification_app: str + # Unique identifier for this notification type. + name: str + # Mark this as a core notification. + # When True, user preferences are taken from the notification app's `core_*` configuration, + # overriding the `web`, `email`, `push`, `email_cadence`, and `non_editable` attributes set here. + is_core: bool + # Template string for notification content (see ./docs/templates.md). + # Wrap in gettext_lazy (_) for translation support. + content_template: str + # A map of variable names that can be used in the template, along with their descriptions. + # The values for these variables are passed to the templates when generating the notification. + # NOTE: this field is for documentation purposes only; it is not used. + content_context: dict[str, Any] + # Template used when delivering notifications via email. + email_template: str + filters: list[str] + + # All fields below are required unless `is_core` is True. + # Core notifications take this config from the associated notification app instead (and ignore anything set here). + + # Set to True to enable delivery on web. + web: NotRequired[bool] + # Set to True to enable delivery via email. + email: NotRequired[bool] + # Set to True to enable delivery via push notifications. + # NOTE: push notifications are not implemented yet + push: NotRequired[bool] + # How often email notifications are sent. + email_cadence: NotRequired[Literal[ + EmailCadence.DAILY, EmailCadence.WEEKLY, EmailCadence.IMMEDIATELY, EmailCadence.NEVER + ]] + # Items in the list represent delivery channels + # where the user is blocked from changing from what is defined for the notification here + # (see `web`, `email`, and `push` above). + non_editable: NotRequired[list[Literal["web", "email", "push"]]] + # Descriptive information about the notification. + info: NotRequired[str] + + +# For help defining new notifications, see ./docs/creating_a_new_notification_guide.md COURSE_NOTIFICATION_TYPES = { 'new_comment_on_response': { 'notification_app': 'discussion', @@ -250,7 +300,42 @@ COURSE_NOTIFICATION_TYPES = { }, } -COURSE_NOTIFICATION_APPS = { + +class NotificationApp(TypedDict): + """ + Define the fields for values in COURSE_NOTIFICATION_APPS + + An instance of this type describes a notification app, + which is a way of grouping configuration of types of notifications for users. + + Each notification type defined in COURSE_NOTIFICATION_TYPES also references an app. + + Each notification type can also be optionally defined as a core notification. + In this case, the delivery preferences for that notification are taken + from the `core_*` fields of the associated notification app. + """ + # Set to True to enable this app and linked notification types. + enabled: bool + # Description to be displayed about core notifications for this app. + # This string should be wrapped in the gettext_lazy function (imported as `_`) to support translation. + core_info: str + # Set to True to enable delivery for associated core notifications on web. + core_web: bool + # Set to True to enable delivery for associated core notifications via emails. + core_email: bool + # Set to True to enable delivery for associated core notifications via push notifications. + # NOTE: push notifications are not implemented yet + core_push: bool + # How often email notifications are sent for associated core notifications. + core_email_cadence: Literal[EmailCadence.DAILY, EmailCadence.WEEKLY, EmailCadence.IMMEDIATELY, EmailCadence.NEVER] + # Items in the list represent core notification delivery channels + # where the user is blocked from changing from what is defined for the app here + # (see `core_web`, `core_email`, and `core_push` above). + non_editable: list[Literal["web", "email", "push"]] + + +# For help defining new notifications and notification apps, see ./docs/creating_a_new_notification_guide.md +COURSE_NOTIFICATION_APPS: dict[str, NotificationApp] = { 'discussion': { 'enabled': True, 'core_info': _('Notifications for responses and comments on your posts, and the ones you’re ' @@ -391,18 +476,22 @@ class NotificationTypeManager: def __init__(self): self.notification_types = COURSE_NOTIFICATION_TYPES - def get_notification_types_by_app(self, notification_app): + def get_notification_types_by_app(self, notification_app: str): """ - Returns notification types for the given notification app. + Returns notification types for the given notification app name. """ return [ notification_type.copy() for _, notification_type in self.notification_types.items() if notification_type.get('notification_app', None) == notification_app ] - def get_core_and_non_core_notification_types(self, notification_app): + def get_core_and_non_core_notification_types( + self, notification_app: str + ) -> tuple[NotificationType, NotificationType]: """ - Returns core notification types for the given app name. + Returns notification types for the given app name, split by core and non core. + + Return type is a tuple of (core_notification_types, non_core_notification_types). """ notification_types = self.get_notification_types_by_app(notification_app) core_notification_types = [] @@ -498,7 +587,7 @@ class NotificationAppManager: return course_notification_preference_config -def get_notification_content(notification_type, context): +def get_notification_content(notification_type: str, context: dict[str, Any]): """ Returns notification content for the given notification type with provided context. diff --git a/openedx/core/djangoapps/notifications/docs/architecture.md b/openedx/core/djangoapps/notifications/docs/architecture.md new file mode 100644 index 0000000000..6729680a5c --- /dev/null +++ b/openedx/core/djangoapps/notifications/docs/architecture.md @@ -0,0 +1,6 @@ + +## Data flow + +Below is a diagram of how data flows through the notification system. + +![data flow diagram](./data-flow.jpg) diff --git a/openedx/core/djangoapps/notifications/docs/creating_a_new_notification_guide.md b/openedx/core/djangoapps/notifications/docs/creating_a_new_notification_guide.md new file mode 100644 index 0000000000..2edc5ab7ad --- /dev/null +++ b/openedx/core/djangoapps/notifications/docs/creating_a_new_notification_guide.md @@ -0,0 +1,132 @@ +# Creating a new notification + +This documentation provides instructions to developers on how to add new notifications to the existing notification system. + +## Overview of terms + +* Type: every notification a user sees has a defined type. This type describes the notification's behaviour and template text. +* App: each notification type is associated with a notification app. A notification app is simply a way to group notifications, and to provide a mechanism for shared behaviour. +* Core notification: a notification type can be labelled as a core notification. In this case, the behaviour is managed at the app level, not at the level of the individual notification type. + +## Defining a notification + +The configuration consists of notification types and notification apps. Follow the steps below to define a new notification type. + +### Step 1: Define the Notification App + +The first step to defining a new notification is deciding on an app to associate it with. Either choose an existing one or create a new one in `COURSE_NOTIFICATION_APPS` in [base_notification.py](../base_notification.py). For example, here is an app named "discussion": + +```python +COURSE_NOTIFICATION_APPS = { + 'discussion': { + 'enabled': True, + 'core_info': '', + 'core_web': True, + 'core_email': True, + 'core_push': True, + 'non_editable': [], + 'core_email_cadence': 'weekly' + } +} +``` + +The app name (the key) can be any name you wish to add but ideally it should represent existing Django apps in the project. +For an explanation of the available fields, see `NotificationApp` in [base_notification.py](../base_notification.py). + +### **Step 2: Define the Notification Type** + +Now you can define the notification type itself. +To do this, add a new entry to `COURSE_NOTIFICATION_TYPES` in [base_notification.py](../base_notification.py). +For example, here is a notification defined for a new response to a discussion forum post, associated with the "discussion" app example from the previous step: + +```python +COURSE_NOTIFICATION_TYPES = { + 'new_response': { + 'notification_app': 'discussion', + 'name': 'new_response', + 'is_core': False, + 'web': True, + 'email': True, + 'push': True, + 'info': 'Response on post', + 'non_editable': [], + 'content_template': _('<{p}><{strong}>{replier_name} responded to your post <{strong}>{post_title}'), + 'content_context': { + 'post_title': 'Post title', + 'replier_name': 'replier name', + }, + 'email_template': '', + }, +} +``` + +For an explanation of the available fields, see `NotificationType` in [base_notification.py](../base_notification.py). + +### Step 3: Update the version in the model file + +Newly added types are only usable once you have updated the value of `COURSE_NOTIFICATION_CONFIG_VERSION` in [notifications/models.py](../models.py). +This constant is used to track changes in notification configuration, and whenever this version is updated preferences of users are also updated with newly available types. +To update it, increment the value by 1. + +Adding new notification types without this step will have no effect. +You don't need to update the constant for changes that are not stored in database (eg. templates). + +Now the notification type is defined and ready to use! +The next section details how to use this notification type to create and send a notification. + +## Sending a notification + +To send a notification, you need to send the `USER_NOTIFICATION_REQUESTED` signal with an instance of `UserNotificationData` containing information about the notification to send. + +Below is an example function to build and send the `new_response` notification type from earlier. + +from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED +from openedx_events.learning.data import UserNotificationData + +```python +def send_new_response_notification(user_ids, course, thread, replier_user): + notification_data = UserNotificationData( + user_ids=user_ids, + notification_type="new_response", + content_url=f"/{course.id}/posts/{thread.id}", + app_name='discussion', + course_key=course.id, + context={ + 'post_title': thread.title, + 'replier_name': replier_user.username, + }, + ) + USER_NOTIFICATION_REQUESTED.send_event(notification_data=notification_data) +``` + +Explanation of the parameters for `UserNotificationData`: + +| Name | Type | Description | +| :---- | :---- | :---- | +| `user_ids` | `list[int]` | List of user IDs to send the notification to. | +| `notification_type` | `str` | The type of notification to send. This must be a key of `COURSE_NOTIFICATION_TYPES`. | +| `content_url` | `str` | Url the user will navigate to if they click on the notification. | +| `app_name` | `str` | The app this notification is associated with. This must be a key of `COURSE_NOTIFICATION_APPS`. | +| `course_key` | `CourseKey` | The course that this notification will be associated with. | +| `context` | `dict[str, str]` | Context variables and values to pass to the notification content template. Keys are the variable names defined in the notification type. | + +That's it! You have implemented the code to send a new user notification using the `USER_NOTIFICATION_REQUESTED` signal. + +## Grouping notifications + +For some notification types, the volume for a learner can be huge and can cause annoyance. +For example, if a learner creates a post, and other learners and staff members start adding responses to his post, if for each comment, we add a response, it could result in dozens of notifications. +To avoid these scenarios, we have implemented a feature that allows grouping more than one similar notifications into a single notification. +Steps to group a notification: + +1. Enable grouping waffle flag `notifications.enable_notification_grouping`. +2. Add `group_by_id` in context before sending the `USER_NOTIFICATION_REQUESTED` event (see [discussions_notifications.py](../../../../../lms/djangoapps/discussion/rest_api/discussions_notifications.py), and search for `group_by_id` for an example). +3. Implement a grouper class to modify content_context (see [grouping_notifications.py](../grouping_notifications.py) for an example). + +## Legal + +When adding a new notification type, you will need a Privacy threshold assessment done by legal. + +## Troubleshooting + +If you have followed the above steps and notifications are still not working, check if the `notifications.enable_notifications` waffle flag is enabled. diff --git a/openedx/core/djangoapps/notifications/docs/data-flow.jpg b/openedx/core/djangoapps/notifications/docs/data-flow.jpg new file mode 100644 index 0000000000..c50392871f Binary files /dev/null and b/openedx/core/djangoapps/notifications/docs/data-flow.jpg differ diff --git a/openedx/core/djangoapps/notifications/docs/getting-started.md b/openedx/core/djangoapps/notifications/docs/getting-started.md new file mode 100644 index 0000000000..18a93c5b80 --- /dev/null +++ b/openedx/core/djangoapps/notifications/docs/getting-started.md @@ -0,0 +1,13 @@ +# Getting started with notifications + +1. You will need to configure `NOTIFICATIONS_DEFAULT_FROM_EMAIL` to send email notifications. +2. Daily and weekly digest emails require the respective management commands to be run on a daily and weekly basis: + - daily: `manage.py lms send_email_digest Daily` + - weekly: `manage.py lms send_email_digest Weekly` + + Example crontab entries: + + ``` + 0 22 * * * ./manage.py lms send_email_digest Daily + 0 22 * * SUN ./manage.py lms send_email_digest Weekly + ``` diff --git a/openedx/core/djangoapps/notifications/docs/notification_tray.md b/openedx/core/djangoapps/notifications/docs/notification_tray.md new file mode 100644 index 0000000000..f0defe08af --- /dev/null +++ b/openedx/core/djangoapps/notifications/docs/notification_tray.md @@ -0,0 +1,7 @@ +# How to enable the notification tray + +On the front end, the notification tray needs to be enabled to simplify the user experience. +Users can click the bell icon in the header to access the notification tray, which will display notifications from the apps listed above. +For detailed steps please view the following document: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5319491585/How+to+Enable+Notification+Tray + +And explicit implementation of the notification tray on the Learning Dashboard can be viewed in the following document: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5321916443/Notification+Tray+Implementation+in+Learner+Dashboard diff --git a/openedx/core/djangoapps/notifications/docs/templates.md b/openedx/core/djangoapps/notifications/docs/templates.md new file mode 100644 index 0000000000..ffb7524afc --- /dev/null +++ b/openedx/core/djangoapps/notifications/docs/templates.md @@ -0,0 +1,14 @@ + +# Notification template spec + +TODO: this needs expanding + +- Use `_(...)` to mark the template as translatable. +- Be sure to add content in `

...

` tags. Use `` to make words bold. +- Note that only `

` and `` are supported. + +TODO: show an example + +# Notification grouped content template spec + +TODO diff --git a/openedx/core/lib/xblock_serializer/block_serializer.py b/openedx/core/lib/xblock_serializer/block_serializer.py index 76611624cd..169352730e 100644 --- a/openedx/core/lib/xblock_serializer/block_serializer.py +++ b/openedx/core/lib/xblock_serializer/block_serializer.py @@ -79,10 +79,15 @@ class XBlockSerializer: 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 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6f626fa9ac..38cb447ea1 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -457,6 +457,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -814,7 +815,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/kernel.in openedx-calc==4.0.2 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 8e5a53f969..d814c55f28 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -739,6 +739,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1362,7 +1363,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index e8fef5599b..e9239b7320 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -545,6 +545,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -991,7 +992,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index edb337ce4b..e2c8512fb0 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -567,6 +567,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1037,7 +1038,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt