fixup! feat: add library restore endpoint

Author: Taylor Payne

openedx/core/djangoapps/content_libraries/rest_api/libraries.py

@@ -100,7 +100,7 @@
     user_can_create_organizations
 )
 from cms.djangoapps.contentstore.storage import course_import_export_storage
-from openedx.core.djangoapps.content_libraries.tasks import restore_library, LibraryRestoreTask
+from openedx.core.djangoapps.content_libraries.tasks import restore_library
 
 from openedx.core.djangoapps.content_libraries import api, permissions
 from openedx.core.djangoapps.content_libraries.api.libraries import get_backup_task_status
@@ -117,6 +117,7 @@
     LibraryBackupTaskStatusSerializer,
     LibraryRestoreFileSerializer,
     LibraryRestoreTaskSerializer,
+    LibraryRestoreTaskResultSerializer,
     LibraryXBlockCreationSerializer,
     LibraryXBlockMetadataSerializer,
     LibraryXBlockTypeSerializer,
@@ -854,7 +855,7 @@ def post(self, request):
                 description="The ID of the restore library task to retrieve."
             ),
         ],
-        responses={200: LibraryRestoreTaskSerializer}
+        responses={200: LibraryRestoreTaskResultSerializer}
     )
     def get(self, request):
         """
@@ -867,18 +868,10 @@ def get(self, request):
 
         # get task status and related artifact
         task_status = get_object_or_404(UserTaskStatus, task_id=task_id, user=request.user)
-        artifact_name = LibraryRestoreTask.ARTIFACT_NAMES.get(task_status.state, '')
-        artifact = task_status.artifacts.filter(name=artifact_name).first()
 
-        try:
-            result = json.loads(artifact.text) if artifact else {'message': 'No artifact for the current task status.'}
-        except json.JSONDecodeError:
-            result = {'error': f'Could not decode artifact JSON. Artifact Text: {artifact.text}'}
-
-        return Response(LibraryRestoreTaskSerializer({
-            'status': task_status.state,
-            'result': result,
-        }).data)
+        # serialize and return result
+        result_serializer = LibraryRestoreTaskResultSerializer.from_task_status(task_status, request)
+        return Response(result_serializer.data)
 
 
 # LTI 1.3 Views

openedx/core/djangoapps/content_libraries/rest_api/serializers.py

@@ -2,13 +2,18 @@
 Serializers for the content libraries REST API
 """
 # pylint: disable=abstract-method
+import json
+import logging
+
 from django.core.validators import validate_unicode_slug
 from opaque_keys import InvalidKeyError, OpaqueKey
 from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2
 from openedx_learning.api.authoring_models import Collection, LearningPackage
 from rest_framework import serializers
 from rest_framework.exceptions import ValidationError
+from user_tasks.models import UserTaskStatus
 
+from openedx.core.djangoapps.content_libraries.tasks import LibraryRestoreTask
 from openedx.core.djangoapps.content_libraries.api.containers import ContainerType
 from openedx.core.djangoapps.content_libraries.constants import ALL_RIGHTS_RESERVED, LICENSE_OPTIONS
 from openedx.core.djangoapps.content_libraries.models import (
@@ -22,6 +27,8 @@
 
 DATETIME_FORMAT = '%Y-%m-%dT%H:%M:%SZ'
 
+log = logging.getLogger(__name__)
+
 
 class ContentLibraryMetadataSerializer(serializers.Serializer):
     """
@@ -453,9 +460,76 @@ class LibraryRestoreTaskSerializer(serializers.Serializer):
     """
     Serializer for result of a library restore task.
     """
-    # input only fields
-    task_id = serializers.CharField(write_only=True, help_text="The ID of the restore task to check.")
+    task_id = serializers.UUIDField(write_only=True, help_text="The ID of the restore task to check.")
 
-    # output only fields
-    status = serializers.UUIDField(read_only=True)
-    result = serializers.JSONField(read_only=True)
+
+class RestoreSuccessDataSerializer(serializers.Serializer):
+    """
+    Serializer for the data returned upon successful restoration of a library.
+    """
+    learning_package_id = serializers.IntegerField(source="lp_restored_data.id")
+    title = serializers.CharField(source="lp_restored_data.title")
+    org = serializers.CharField(source="lp_restored_data.archive_org_key")
+    slug = serializers.CharField(source="lp_restored_data.archive_slug")
+    key = serializers.CharField(source="lp_restored_data.key")
+    archive_key = serializers.CharField(source="lp_restored_data.archive_lp_key")
+
+    containers = serializers.IntegerField(source="lp_restored_data.num_containers")
+    components = serializers.IntegerField(source="lp_restored_data.num_components")
+    collections = serializers.IntegerField(source="lp_restored_data.num_collections")
+    sections = serializers.IntegerField(source="lp_restored_data.num_sections")
+    subsections = serializers.IntegerField(source="lp_restored_data.num_subsections")
+    units = serializers.IntegerField(source="lp_restored_data.num_units")
+
+    created_on_server = serializers.CharField(source="backup_metadata.original_server", required=False)
+    created_at = serializers.DateTimeField(source="backup_metadata.created_at", format=DATETIME_FORMAT)
+    created_by = serializers.SerializerMethodField()
+
+    def get_created_by(self, obj):
+        username = obj["backup_metadata"].get("created_by")
+        email = obj["backup_metadata"].get("created_by_email")
+        return {"username": username, "email": email}
+
+
+class LibraryRestoreTaskResultSerializer(serializers.Serializer):
+    """
+    Serializer for the result of a library restore task.
+    """
+    state = serializers.CharField()
+    result = RestoreSuccessDataSerializer(required=False, allow_null=True, default=None)
+    error = serializers.CharField(required=False, allow_blank=True, default=None)
+    error_log = serializers.FileField(source='error_log_url', allow_null=True, use_url=True, default=None)
+
+    @classmethod
+    def from_task_status(cls, task_status, request):
+        """Build serializer input from task status object."""
+
+        # If the task did not complete, just return the state.
+        if task_status.state not in {UserTaskStatus.SUCCEEDED, UserTaskStatus.FAILED}:
+            return cls({
+                "state": task_status.state,
+            })
+
+        artifact_name = LibraryRestoreTask.ARTIFACT_NAMES.get(task_status.state, '')
+        artifact = task_status.artifacts.filter(name=artifact_name).first()
+
+        # If the task failed, include the log artifact if it exists
+        if task_status.state == UserTaskStatus.FAILED:
+            return cls({
+                "state": UserTaskStatus.FAILED,
+                "error": "Library restore failed. See error log for details.",
+                "error_log_url": artifact.file if artifact else None,
+            }, context={'request': request})
+
+        if task_status.state == UserTaskStatus.SUCCEEDED:
+            input_data = {
+                "state": UserTaskStatus.SUCCEEDED,
+            }
+            try:
+                result = json.loads(artifact.text) if artifact else {}
+                input_data["result"] = result
+            except json.JSONDecodeError:
+                log.error("Failed to decode JSON from artifact (%s): %s", artifact.id, artifact.text)
+                input_data["error"] = f'Could not decode artifact JSON. Artifact Text: {artifact.text}'
+
+            return cls(input_data)

openedx/core/djangoapps/content_libraries/tasks.py

@@ -26,6 +26,7 @@
 
 from django.core.files.base import ContentFile
 from django.contrib.auth import get_user_model
+from django.core.serializers.json import DjangoJSONEncoder
 from celery import shared_task
 from celery.utils.log import get_task_logger
 from celery_utils.logged_task import LoggedTask
@@ -572,10 +573,12 @@ class LibraryRestoreTask(UserTask):
     """
 
     ARTIFACT_NAMES = {
-        UserTaskStatus.FAILED: 'Error',
+        UserTaskStatus.FAILED: 'Error log',
         UserTaskStatus.SUCCEEDED: 'Library Restore',
     }
 
+    ERROR_LOG_ARTIFACT_NAME = 'Error log'
+
     @classmethod
     def generate_name(cls, arguments_dict):
         storage_path = arguments_dict['storage_path']
@@ -592,13 +595,11 @@ def fail_with_error_log(self, logfile) -> None:
         error_log_file = ContentFile(logfile.getvalue().encode("utf-8"))
 
         # Save the error log as an artifact
-        artifact = UserTaskArtifact(status=self.status, name='Error log')
+        artifact = UserTaskArtifact(status=self.status, name=self.ERROR_LOG_ARTIFACT_NAME)
         artifact.file.save(name=f'{self.status.task_id}-error.log', content=error_log_file)
         artifact.save()
 
-        # Fail the task with a reference to the error log
-        url = artifact.file.storage.url(artifact.file.name)
-        self.status.fail(json.dumps({'error': 'Error(s) restoring learning package', 'log_file': url}))
+        self.status.fail(json.dumps({'error': 'Error(s) restoring learning package'}))
 
     def load_learning_package(self, storage_path, user):
         """
@@ -651,39 +652,15 @@ def restore_library(self, user_id, storage_path):
         user = User.objects.get(id=user_id)
         result = self.load_learning_package(storage_path, user=user)
         learning_package_data = result.get("lp_restored_data", {})
-        backup_metadata = result.get("backup_metadata", {})
 
         TASK_LOGGER.info(
             'Restored learning package (id: %s) with key %s',
             learning_package_data.get('id'),
             learning_package_data.get('key')
         )
 
-        # Ensure any datetime value is formatted correctly
-        if backup_created_at := backup_metadata.get("created_at"):
-            backup_created_at = backup_created_at.strftime(DATETIME_FORMAT)
-
         # Save the restore details as an artifact in JSON format
-        restore_data = json.dumps({
-            "learning_package_id": learning_package_data.get("id"),
-            "title": learning_package_data.get("title"),
-            "org": learning_package_data.get("archive_org_key"),
-            "slug": learning_package_data.get("archive_slug"),
-            "key": learning_package_data.get("key"),
-            "archive_key": learning_package_data.get("archive_lp_key"),
-            "containers": learning_package_data.get("num_containers", -1),
-            "components": learning_package_data.get("num_components", -1),
-            "collections": learning_package_data.get("num_collections", -1),
-            "sections": learning_package_data.get("num_sections", -1),
-            "subsections": learning_package_data.get("num_subsections", -1),
-            "units": learning_package_data.get("num_units", -1),
-            "created_on_server": backup_metadata.get("original_server"),
-            "created_at": backup_created_at,
-            "created_by": {
-                "username": backup_metadata.get("created_by"),
-                "email": backup_metadata.get("created_by_email"),
-            },
-        })
+        restore_data = json.dumps(result, cls=DjangoJSONEncoder)
 
         UserTaskArtifact.objects.create(
             status=self.status,

openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py

@@ -992,8 +992,8 @@ def test_restore_library_success(self):
         with self.as_user(self.admin_user):
             response_data = self._get_library_restore_task(response_data['task_id'])
 
-        self.assertIn('status', response_data)
-        self.assertEqual(response_data['status'], 'Succeeded')
+        self.assertIn('state', response_data)
+        self.assertEqual(response_data['state'], 'Succeeded')
 
         self.assertIn('result', response_data)
         task_result = response_data.get('result', {})
@@ -1039,8 +1039,8 @@ def test_create_content_library_from_restore(self):
         with self.as_user(self.admin_user):
             response_data = self._get_library_restore_task(response_data['task_id'])
 
-        self.assertIn('status', response_data)
-        self.assertEqual(response_data['status'], 'Succeeded')
+        self.assertIn('state', response_data)
+        self.assertEqual(response_data['state'], 'Succeeded')
 
         task_result = response_data.get('result', {})
         self.assertIn('learning_package_id', task_result)
@@ -1094,10 +1094,10 @@ def test_get_restore_task_unfinished(self):
             response_data = self._get_library_restore_task(pending_task_status.task_id)
 
         expected = {
-            "status": UserTaskStatus.PENDING,
-            "result": {
-                "message": "No artifact for the current task status."
-            }
+            "state": UserTaskStatus.PENDING,
+            "result": None,
+            "error": None,
+            "error_log": None,
         }
 
         self.assertEqual(response_data, expected)
@@ -1110,7 +1110,7 @@ def test_get_restore_task_unfinished(self):
         ):
             response_data = self._get_library_restore_task(in_progress_task_status.task_id)
 
-        expected["status"] = UserTaskStatus.IN_PROGRESS
+        expected["state"] = UserTaskStatus.IN_PROGRESS
         self.assertEqual(response_data, expected)
 
     def test_task_user_mismatch(self):
@@ -1136,7 +1136,7 @@ def test_task_artifact_text_not_json(self):
         UserTaskArtifact.objects.create(
             status=task_status,
             text=artifact_text,
-            name=LibraryRestoreTask.ARTIFACT_NAMES.get(task_status.state, '')
+            name=LibraryRestoreTask.ARTIFACT_NAMES[task_status.state],
         )
 
         with patch(
@@ -1146,10 +1146,10 @@ def test_task_artifact_text_not_json(self):
             response_data = self._get_library_restore_task(task_status.task_id)
 
         expected = {
-            "status": UserTaskStatus.SUCCEEDED,
-            "result": {
-                'error': f'Could not decode artifact JSON. Artifact Text: {artifact_text}'
-            }
+            "state": UserTaskStatus.SUCCEEDED,
+            "result": None,
+            "error": ANY,
+            "error_log": None,
         }
 
         self.assertEqual(response_data, expected)
@@ -1159,8 +1159,10 @@ def test_failed_task_with_error_log(self):
         If a task fails with an error log, include the url to the log
         """
         error_result = {
-            'log_file_error': StringIO("Library restore failed: An unexpected error occurred during processing."),
             'status': 'error',
+            'log_file_error': StringIO("Library restore failed: An unexpected error occurred during processing."),
+            'lp_restore_data': None,
+            'backup_metadata': None,
         }
 
         with self.as_user(self.admin_user):
@@ -1174,11 +1176,10 @@ def test_failed_task_with_error_log(self):
             task_data = self._get_library_restore_task(response['task_id'])
 
         expected = {
-            'status': 'Failed',
-            'result': {
-                'error': ANY,
-                'log_file': ANY,
-            }
+            'state': 'Failed',
+            'error': ANY,
+            'error_log': ANY,
+            'result': None,
         }
 
         self.assertEqual(task_data, expected)
@@ -1198,11 +1199,10 @@ def test_uncaught_error_creates_error_log(self):
             task_data = self._get_library_restore_task(response['task_id'])
 
         expected = {
-            'status': 'Failed',
-            'result': {
-                'error': ANY,
-                'log_file': ANY,
-            }
+            'state': 'Failed',
+            'error': ANY,
+            'error_log': ANY,
+            'result': None,
         }
 
         self.assertEqual(task_data, expected)