From c1f04b7c4c606be8013d71ce6aabf0009a49cd36 Mon Sep 17 00:00:00 2001 From: Andrii Date: Wed, 14 May 2025 22:52:09 +0300 Subject: [PATCH 1/3] feat: redesign import task + api/helpers tests adaptation --- .../import_from_modulestore/admin.py | 23 +++- cms/djangoapps/import_from_modulestore/api.py | 36 +++--- .../import_from_modulestore/data.py | 33 +++-- .../import_from_modulestore/helpers.py | 4 +- ...created_remove_import_modified_and_more.py | 48 +++++++ .../import_from_modulestore/models.py | 37 ++++-- .../import_from_modulestore/tasks.py | 117 ++++++++++++------ .../tests/factories.py | 4 +- .../import_from_modulestore/tests/test_api.py | 75 +++++------ .../tests/test_helpers.py | 92 ++++++++++---- .../tests/test_tasks.py | 61 +++++---- 11 files changed, 355 insertions(+), 175 deletions(-) create mode 100644 cms/djangoapps/import_from_modulestore/migrations/0002_remove_import_created_remove_import_modified_and_more.py diff --git a/cms/djangoapps/import_from_modulestore/admin.py b/cms/djangoapps/import_from_modulestore/admin.py index ed1d7a202303..61f90f5f7d48 100644 --- a/cms/djangoapps/import_from_modulestore/admin.py +++ b/cms/djangoapps/import_from_modulestore/admin.py @@ -19,7 +19,7 @@ class ImportAdmin(admin.ModelAdmin): 'target_change', ) list_filter = ( - 'status', + 'user_task_status__state', ) search_fields = ( 'source_key', @@ -27,7 +27,26 @@ class ImportAdmin(admin.ModelAdmin): ) raw_id_fields = ('user',) - readonly_fields = ('status',) + readonly_fields = ('user_task_status',) + + def uuid(self, obj): + """ + Returns the UUID of the import. + """ + return getattr(obj.user_task_status, 'uuid', None) + uuid.short_description = 'UUID' + + def created(self, obj): + """ + Returns the creation date of the import. + """ + return getattr(obj.user_task_status, 'created', None) + + def status(self, obj): + """ + Returns the status of the import. + """ + return getattr(obj.user_task_status, 'state', None) admin.site.register(Import, ImportAdmin) diff --git a/cms/djangoapps/import_from_modulestore/api.py b/cms/djangoapps/import_from_modulestore/api.py index 7f8dc16b76cb..5c9bfc2aaec9 100644 --- a/cms/djangoapps/import_from_modulestore/api.py +++ b/cms/djangoapps/import_from_modulestore/api.py @@ -7,39 +7,37 @@ from .helpers import cancel_incomplete_old_imports from .models import Import as _Import -from .tasks import import_staged_content_to_library_task, save_legacy_content_to_staged_content_task +from .tasks import import_to_library_task from .validators import validate_usage_keys_to_import -def stage_content_for_import(source_key: LearningContextKey, user_id: int) -> _Import: - """ - Create a new import event to import a course to a library and save course to staged content. - """ - import_from_modulestore = _Import.objects.create(source_key=source_key, user_id=user_id) - cancel_incomplete_old_imports(import_from_modulestore) - save_legacy_content_to_staged_content_task.delay_on_commit(import_from_modulestore.uuid) - return import_from_modulestore - - -def import_staged_content_to_library( +def import_to_library( + source_key: LearningContextKey, usage_ids: Sequence[str | UsageKey], - import_uuid: str, target_learning_package_id: int, user_id: int, composition_level: str, - override: bool, -) -> None: + override: bool = False, +) -> _Import: """ Import staged content to a library from staged content. """ validate_usage_keys_to_import(usage_ids) - import_staged_content_to_library_task.apply_async( + + import_from_modulestore = _Import.objects.create( + source_key=source_key, + user_id=user_id, + composition_level=composition_level, + override=override, + ) + cancel_incomplete_old_imports(import_from_modulestore) + + import_to_library_task.apply_async( kwargs={ + 'import_pk': import_from_modulestore.pk, 'usage_key_strings': usage_ids, - 'import_uuid': import_uuid, 'learning_package_id': target_learning_package_id, 'user_id': user_id, - 'composition_level': composition_level, - 'override': override, }, ) + return import_from_modulestore diff --git a/cms/djangoapps/import_from_modulestore/data.py b/cms/djangoapps/import_from_modulestore/data.py index 998ea8dfc745..b7303281a985 100644 --- a/cms/djangoapps/import_from_modulestore/data.py +++ b/cms/djangoapps/import_from_modulestore/data.py @@ -5,23 +5,27 @@ from enum import Enum from openedx.core.djangoapps.content_libraries import api as content_libraries_api -from django.db.models import TextChoices from django.utils.translation import gettext_lazy as _ -class ImportStatus(TextChoices): +class ImportStatus(Enum): """ The status of this modulestore-to-learning-core import. """ - NOT_STARTED = 'not_started', _('Waiting to stage content') - STAGING = 'staging', _('Staging content for import') + WAITNG_TO_STAGE = _('Waiting to stage content') + STAGING = _('Staging content for import') STAGING_FAILED = _('Failed to stage content') - STAGED = 'staged', _('Content is staged and ready for import') - IMPORTING = 'importing', _('Importing staged content') - IMPORTING_FAILED = 'importing_failed', _('Failed to import staged content') - IMPORTED = 'imported', _('Successfully imported content') - CANCELED = 'canceled', _('Canceled') + STAGED = _('Content is staged and ready for import') + IMPORTING = _('Importing staged content') + IMPORTING_FAILED = _('Failed to import staged content') + IMPORTED = _('Staged content imported successfully') + CANCELED = _('Import canceled') + + FAILED_STATUSES = [ + STAGING_FAILED, + IMPORTING_FAILED, + ] class CompositionLevel(Enum): @@ -50,5 +54,16 @@ def values(cls): """ return [composition_level.value for composition_level in cls] + @classmethod + def choices(cls): + """ + Returns all levels of composition levels as a list of tuples. + """ + return [ + (composition_level.value, composition_level.name) + for composition_level in cls + if not isinstance(composition_level.value, list) + ] + PublishableVersionWithMapping = namedtuple('PublishableVersionWithMapping', ['publishable_version', 'mapping']) diff --git a/cms/djangoapps/import_from_modulestore/helpers.py b/cms/djangoapps/import_from_modulestore/helpers.py index e540e0ff3dba..60b978270c4c 100644 --- a/cms/djangoapps/import_from_modulestore/helpers.py +++ b/cms/djangoapps/import_from_modulestore/helpers.py @@ -90,7 +90,7 @@ def import_from_staged_content(self) -> list[PublishableVersionWithMapping]: if block_to_import is None: return [] - return self._process_import(self.block_usage_key_to_import, block_to_import) + return self._import_complicated_child(block_to_import, self.block_usage_key_to_import) def _process_import(self, usage_key_string, block_to_import) -> list[PublishableVersionWithMapping]: """ @@ -461,6 +461,6 @@ def cancel_incomplete_old_imports(import_event: Import) -> None: target_change=import_event.target_change, source_key=import_event.source_key, staged_content_for_import__isnull=False - ).exclude(uuid=import_event.uuid) + ).exclude(pk=import_event.pk) for incomplete_import in incomplete_user_imports_with_same_target: incomplete_import.set_status(ImportStatus.CANCELED) diff --git a/cms/djangoapps/import_from_modulestore/migrations/0002_remove_import_created_remove_import_modified_and_more.py b/cms/djangoapps/import_from_modulestore/migrations/0002_remove_import_created_remove_import_modified_and_more.py new file mode 100644 index 000000000000..96b2d6ff69e6 --- /dev/null +++ b/cms/djangoapps/import_from_modulestore/migrations/0002_remove_import_created_remove_import_modified_and_more.py @@ -0,0 +1,48 @@ +# Generated by Django 4.2.20 on 2025-05-14 17:31 + +from django.db import migrations, models +import django.db.models.deletion +from openedx.core.djangoapps.content_libraries.api import ContainerType + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_tasks', '0004_url_textfield'), + ('import_from_modulestore', '0001_initial'), + ] + + operations = [ + migrations.RemoveField( + model_name='import', + name='created', + ), + migrations.RemoveField( + model_name='import', + name='modified', + ), + migrations.RemoveField( + model_name='import', + name='status', + ), + migrations.RemoveField( + model_name='import', + name='uuid', + ), + migrations.AddField( + model_name='import', + name='composition_level', + field=models.CharField(choices=[(ContainerType['Section'], 'CHAPTER'), (ContainerType['Subsection'], 'SEQUENTIAL'), (ContainerType['Unit'], 'VERTICAL'), ('component', 'COMPONENT')], default='component', help_text='The composition level of the target learning package', max_length=255), + preserve_default=False, + ), + migrations.AddField( + model_name='import', + name='override', + field=models.BooleanField(default=False, help_text='If true, the import will override any existing content in the target learning package.'), + ), + migrations.AddField( + model_name='import', + name='user_task_status', + field=models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='user_tasks.usertaskstatus'), + ), + ] diff --git a/cms/djangoapps/import_from_modulestore/models.py b/cms/djangoapps/import_from_modulestore/models.py index 5b6122749bba..e8e9fa4e3f96 100644 --- a/cms/djangoapps/import_from_modulestore/models.py +++ b/cms/djangoapps/import_from_modulestore/models.py @@ -1,7 +1,6 @@ """ Models for the course to library import app. """ -import uuid as uuid_tools from django.contrib.auth import get_user_model from django.db import models @@ -14,29 +13,40 @@ ) from openedx_learning.api.authoring_models import LearningPackage, PublishableEntity -from .data import ImportStatus +from .data import CompositionLevel, ImportStatus User = get_user_model() -class Import(TimeStampedModel): +class Import(models.Model): """ Represents the action of a user importing a modulestore-based course or legacy library into a learning-core based learning package (today, that is always a content library). """ - uuid = models.UUIDField(default=uuid_tools.uuid4, editable=False, unique=True) - status = models.CharField( - max_length=100, - choices=ImportStatus.choices, - default=ImportStatus.NOT_STARTED, - db_index=True + user_task_status = models.OneToOneField( + 'user_tasks.UserTaskStatus', + on_delete=models.SET_NULL, + null=True, + blank=True, + related_name='import_event', ) user = models.ForeignKey(User, on_delete=models.CASCADE) # Note: For now, this will always be a course key. In the future, it may be a legacy library key. source_key = LearningContextKeyField(help_text=_('The modulestore course'), max_length=255, db_index=True) target_change = models.ForeignKey(to='oel_publishing.DraftChangeLog', on_delete=models.SET_NULL, null=True) + composition_level = models.CharField( + max_length=255, + choices=CompositionLevel.choices(), + help_text=_('The composition level of the target learning package'), + ) + override = models.BooleanField( + default=False, + help_text=_( + 'If true, the import will override any existing content in the target learning package.' + ), + ) class Meta: verbose_name = _('Import from modulestore') @@ -49,8 +59,13 @@ def set_status(self, status: ImportStatus): """ Set import status. """ - self.status = status - self.save() + if status in ImportStatus.FAILED_STATUSES.value: + self.user_task_status.fail(status) + elif status == ImportStatus.CANCELED: + self.user_task_status.cancel() + else: + self.user_task_status.set_state(status) + self.user_task_status.save() if status in [ImportStatus.IMPORTED, ImportStatus.CANCELED]: self.clean_related_staged_content() diff --git a/cms/djangoapps/import_from_modulestore/tasks.py b/cms/djangoapps/import_from_modulestore/tasks.py index 4644b29e4904..34a377d5e384 100644 --- a/cms/djangoapps/import_from_modulestore/tasks.py +++ b/cms/djangoapps/import_from_modulestore/tasks.py @@ -1,34 +1,90 @@ """ Tasks for course to library import. """ +from typing import Sequence from celery import shared_task from celery.utils.log import get_task_logger from django.db import transaction -from edx_django_utils.monitoring import set_code_owner_attribute +from edx_django_utils.monitoring import set_code_owner_attribute_from_module +from opaque_keys.edx.keys import UsageKey from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import LearningPackage from openedx.core.djangoapps.content_staging import api as content_staging_api +from user_tasks.tasks import UserTask from .constants import IMPORT_FROM_MODULESTORE_STAGING_PURPOSE from .data import ImportStatus from .helpers import get_items_to_import, import_from_staged_content from .models import Import, PublishableEntityImport, StagedContentForImport -from .validators import validate_composition_level log = get_task_logger(__name__) -@shared_task -@set_code_owner_attribute -def save_legacy_content_to_staged_content_task(import_uuid: str) -> None: +class ImportToLibraryTask(UserTask): """ - Save courses to staged content task by sections/chapters. + Base class for import to library tasks. """ - import_event = Import.objects.get(uuid=import_uuid) + @staticmethod + def calculate_total_steps(arguments_dict): + """ + Get number of in-progress steps in importing process, as shown in the UI. + + For reference, these are: + + 1. Staging content + 2. Importing staged content into library + """ + return 2 + + @classmethod + def generate_name(cls, arguments_dict): + """ + Create a name for this particular import task instance. + + Arguments: + arguments_dict (dict): The arguments given to the task function + + Returns: + str: The generated name + """ + library_id = arguments_dict.get('learning_package_id') + import_id = arguments_dict.get('import_pk') + return f'Import course to library (library_id={library_id}, import_id={import_id})' + + +@shared_task(base=ImportToLibraryTask, bind=True) +# Note: The decorator @set_code_owner_attribute cannot be used here because the UserTaskMixin +# does stack inspection and can't handle additional decorators. +def import_to_library_task( + self, + import_pk: int, + usage_key_strings: Sequence[str | UsageKey], + learning_package_id: int, + user_id: int, +) -> None: + """ + Import to library task. + + 1. Save course (for now) to staged content task by sections/chapters. + 2. Import staged content to library task. + """ + set_code_owner_attribute_from_module(__name__) + + # Step 1: Save course to staged content task by sections/chapters. + try: + import_event = Import.objects.get(pk=import_pk) + import_event.user_task_status = self.status + import_event.save(update_fields=['user_task_status']) + except Import.DoesNotExist: + log.info('Import event not found for pk %s', import_pk) + return + + import_event.set_status(ImportStatus.WAITNG_TO_STAGE) import_event.clean_related_staged_content() + import_event.set_status(ImportStatus.STAGING) try: with transaction.atomic(): @@ -36,7 +92,7 @@ def save_legacy_content_to_staged_content_task(import_uuid: str) -> None: for item in items_to_import: staged_content = content_staging_api.stage_xblock_temporarily( item, - import_event.user.id, + user_id, purpose=IMPORT_FROM_MODULESTORE_STAGING_PURPOSE, ) StagedContentForImport.objects.create( @@ -53,26 +109,13 @@ def save_legacy_content_to_staged_content_task(import_uuid: str) -> None: import_event.set_status(ImportStatus.STAGING_FAILED) raise exc + # Step 2: Import staged content to library task. + self.status.increment_completed_steps() -@shared_task -@set_code_owner_attribute -def import_staged_content_to_library_task( - usage_key_strings: list[str], - import_uuid: str, - learning_package_id: int, - user_id: int, - composition_level: str, - override: bool, -) -> None: - """ - Import staged content to a library task. - """ - validate_composition_level(composition_level) - - import_event = Import.objects.get(uuid=import_uuid, status=ImportStatus.STAGED, user_id=user_id) target_learning_package = LearningPackage.objects.get(id=learning_package_id) imported_publishable_versions = [] + import_event.set_status(ImportStatus.IMPORTING) with authoring_api.bulk_draft_changes_for(learning_package_id=learning_package_id) as change_log: try: for usage_key_string in usage_key_strings: @@ -84,18 +127,20 @@ def import_staged_content_to_library_task( usage_key_string, target_learning_package, staged_content_for_import.staged_content, - composition_level, - override, + import_event.composition_level, + import_event.override, ) imported_publishable_versions.extend(publishable_versions) - except: # pylint: disable=bare-except + except Exception as exc: # pylint: disable=broad-except import_event.set_status(ImportStatus.IMPORTING_FAILED) - raise - - import_event.set_status(ImportStatus.IMPORTED) - for imported_component_version in imported_publishable_versions: - PublishableEntityImport.objects.create( - import_event=import_event, - resulting_mapping=imported_component_version.mapping, - resulting_change=change_log.records.get(entity=imported_component_version.mapping.target_entity), - ) + raise exc + else: + import_event.set_status(ImportStatus.IMPORTED) + for imported_component_version in imported_publishable_versions: + PublishableEntityImport.objects.create( + import_event=import_event, + resulting_mapping=imported_component_version.mapping, + resulting_change=change_log.records.get(entity=imported_component_version.mapping.target_entity), + ) + finally: + import_event.clean_related_staged_content() diff --git a/cms/djangoapps/import_from_modulestore/tests/factories.py b/cms/djangoapps/import_from_modulestore/tests/factories.py index 368cc0ed94ff..e8423b3a84b5 100644 --- a/cms/djangoapps/import_from_modulestore/tests/factories.py +++ b/cms/djangoapps/import_from_modulestore/tests/factories.py @@ -1,7 +1,6 @@ """ Factories for Import model. """ - import uuid import factory @@ -22,7 +21,6 @@ class Meta: @factory.lazy_attribute def source_key(self): - return CourseKey.from_string(f'course-v1:edX+DemoX+{self.uuid}') + return CourseKey.from_string(f'course-v1:edX+DemoX+{uuid.uuid4()}') - uuid = factory.LazyFunction(lambda: str(uuid.uuid4())) user = factory.SubFactory(UserFactory) diff --git a/cms/djangoapps/import_from_modulestore/tests/test_api.py b/cms/djangoapps/import_from_modulestore/tests/test_api.py index 62fe2e4159a9..72497200a7f9 100644 --- a/cms/djangoapps/import_from_modulestore/tests/test_api.py +++ b/cms/djangoapps/import_from_modulestore/tests/test_api.py @@ -8,8 +8,7 @@ from organizations.models import Organization from common.djangoapps.student.tests.factories import UserFactory -from cms.djangoapps.import_from_modulestore.api import import_staged_content_to_library, stage_content_for_import -from cms.djangoapps.import_from_modulestore.data import ImportStatus +from cms.djangoapps.import_from_modulestore.api import import_to_library from cms.djangoapps.import_from_modulestore.models import Import from openedx.core.djangoapps.content_libraries import api as content_libraries_api from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -33,26 +32,12 @@ def setUp(self): ) self.library = content_libraries_api.ContentLibrary.objects.get_by_key(_library_metadata.key) - def test_stage_content_for_import(self): - """ - Test stage_content_for_import function. - """ - course_id = "course-v1:edX+DemoX+Demo_Course" - user = UserFactory() - stage_content_for_import(course_id, user.id) - - import_event = Import.objects.get() - assert import_event.source_key == CourseKey.from_string(course_id) - assert import_event.user_id == user.id - assert import_event.status == ImportStatus.NOT_STARTED - def test_import_staged_content_to_library(self): """ Test import_staged_content_to_library function with different override values. """ - import_event = ImportFactory( - source_key=CourseKey.from_string("course-v1:edX+DemoX+Demo_Course"), - ) + user = UserFactory() + source_key = "course-v1:edX+DemoX+Demo_Course" usage_ids = [ "block-v1:edX+DemoX+Demo_Course+type@chapter+block@123", "block-v1:edX+DemoX+Demo_Course+type@chapter+block@456", @@ -60,29 +45,33 @@ def test_import_staged_content_to_library(self): override = False with patch( - "cms.djangoapps.import_from_modulestore.api.import_staged_content_to_library_task" - ) as import_staged_content_to_library_task_mock: - import_staged_content_to_library( - usage_ids, - import_event.uuid, - self.library.learning_package.id, - import_event.user.id, - "xblock", - override + "cms.djangoapps.import_from_modulestore.api.import_to_library_task" + ) as import_to_library_task_mock: + import_to_library( + source_key=source_key, + usage_ids=usage_ids, + target_learning_package_id=self.library.learning_package.id, + user_id=user.id, + composition_level="component", + override=override, ) - import_staged_content_to_library_task_mock.apply_async.assert_called_once_with( + import_event = Import.objects.get() + assert str(import_event.source_key) == source_key + assert import_event.composition_level == "component" + assert import_event.override == override + assert import_event.user_id == user.id + + import_to_library_task_mock.apply_async.assert_called_once_with( kwargs={ + "import_pk": Import.objects.get().pk, "usage_key_strings": usage_ids, - "import_uuid": import_event.uuid, "learning_package_id": self.library.learning_package.id, - "user_id": import_event.user.id, - "composition_level": "xblock", - "override": override, + "user_id": user.id, }, ) - def test_import_staged_content_to_library_invalid_usage_key(self): + def test_import_to_library_invalid_usage_key(self): """ Test import_staged_content_to_library function with not chapter usage keys. """ @@ -95,15 +84,15 @@ def test_import_staged_content_to_library_invalid_usage_key(self): ] with patch( - "cms.djangoapps.import_from_modulestore.api.import_staged_content_to_library_task" - ) as import_staged_content_to_library_task_mock: + "cms.djangoapps.import_from_modulestore.api.import_to_library_task" + ) as import_to_library_task_mock: with self.assertRaises(ValueError): - import_staged_content_to_library( - usage_ids, - import_event.uuid, - self.library.learning_package.id, - import_event.user.id, - "xblock", - False + import_to_library( + source_key=import_event.source_key, + usage_ids=usage_ids, + target_learning_package_id=self.library.learning_package.id, + user_id=import_event.user.id, + composition_level="component", + override=False, ) - import_staged_content_to_library_task_mock.apply_async.assert_not_called() + import_to_library_task_mock.apply_async.assert_not_called() diff --git a/cms/djangoapps/import_from_modulestore/tests/test_helpers.py b/cms/djangoapps/import_from_modulestore/tests/test_helpers.py index 1d2ce26867aa..5e5aad1b1c84 100644 --- a/cms/djangoapps/import_from_modulestore/tests/test_helpers.py +++ b/cms/djangoapps/import_from_modulestore/tests/test_helpers.py @@ -1,6 +1,8 @@ """ Tests for the import_from_modulestore helper functions. """ +import uuid + import ddt from organizations.models import Organization from unittest import mock @@ -8,14 +10,19 @@ from lxml import etree from openedx_learning.api.authoring_models import LearningPackage +from user_tasks.models import UserTaskStatus -from cms.djangoapps.import_from_modulestore import api +from cms.djangoapps.import_from_modulestore import api, constants from cms.djangoapps.import_from_modulestore.helpers import ImportClient +from cms.djangoapps.import_from_modulestore.models import StagedContentForImport +from cms.djangoapps.import_from_modulestore.tests.factories import ImportFactory from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as content_libraries_api +from openedx.core.djangoapps.content_staging import api as content_staging_api from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory +from xmodule.modulestore.django import modulestore @ddt.ddt @@ -50,8 +57,35 @@ def setUp(self): display_name='Video', data="""