Improve album upload error handling (#3649)

* Improve album upload error handling

* Delete upload if album is gone

website/photos/admin.py

@@ -74,7 +74,11 @@ def save_model(self, request, obj, form, change):
             # function call. In a real deployment, it has to be called only when the current db
             # transaction is committed,
             transaction.on_commit(
-                process_album_upload.s(archive.temporary_upload.upload_id, obj.id).delay
+                process_album_upload.s(
+                    archive.temporary_upload.upload_id,
+                    obj.id,
+                    uploader_id=request.user.id,
+                ).delay
             )
 
             self.message_user(

website/photos/services.py

@@ -64,48 +64,67 @@ def get_annotated_accessible_albums(request, albums):
     return albums
 
 
-def extract_archive(album, archive):
+def extract_archive(album, archive) -> tuple[dict[str, str], int]:
     """Extract zip and tar files."""
+    warnings, count = {}, 0
     if is_zipfile(archive):
         archive.seek(0)
         with ZipFile(archive) as zip_file:
             for photo in sorted(zip_file.namelist()):
                 if not _has_photo_extension(photo):
+                    warnings[photo] = "has an unknown extension."
                     continue
 
                 with zip_file.open(photo) as file:
-                    _try_save_photo(album, file, photo)
-        return
+                    if warning := _try_save_photo(album, file, photo):
+                        warnings[photo] = warning
+                    else:
+                        count += 1
+        return warnings, count
 
     archive.seek(0)
     # is_tarfile only supports filenames, so we cannot use that
     try:
         with tarfile.open(fileobj=archive) as tar_file:
             for photo in sorted(tar_file.getnames()):
                 if not _has_photo_extension(photo):
+                    warnings[photo] = "has an unknown extension."
                     continue
 
                 with tar_file.extractfile(photo) as file:
-                    _try_save_photo(album, file, photo)
+                    if warning := _try_save_photo(album, file, photo):
+                        warnings[photo] = warning
+                    else:
+                        count += 1
     except tarfile.ReadError as e:
         raise ValueError(_("The uploaded file is not a zip or tar file.")) from e
 
+    return warnings, count
+
 
 def _has_photo_extension(filename):
     """Check if the filename has a photo extension."""
     __, extension = os.path.splitext(filename)
     return extension.lower() in (".jpg", ".jpeg", ".png", ".webp")
 
 
-def _try_save_photo(album, file, filename):
-    """Try to save a photo to an album."""
+def _try_save_photo(album, file, filename) -> str | None:
+    """Try to save a photo to an album.
+
+    Returns None, or a string describing a reason for failure.
+    """
     instance = Photo(album=album)
     instance.file = File(file, filename)
     try:
         with transaction.atomic():
             instance.full_clean()
             instance.save()
-    except ValidationError:
-        pass
-    except UnidentifiedImageError:
-        pass
+    except ValidationError as e:
+        logger.warning(f"Photo '{filename}' could not be read: {e}", exc_info=e)
+        return "could not be read."
+    except UnidentifiedImageError as e:
+        logger.warning(f"Photo '{filename}' could not be read: {e}", exc_info=e)
+        return "could not be read."
+    except OSError as e:
+        logger.warning(f"Photo '{filename}' could not be read: {e}", exc_info=e)
+        return "could not be read."

website/photos/tasks.py

@@ -1,36 +1,67 @@
+import logging
+
 from django.db import transaction
 from django.dispatch import Signal
 
 from celery import shared_task
 from django_drf_filepond.models import TemporaryUpload
 
+from mailinglists.services import get_member_email_addresses
+from members.models.member import Member
 from photos.models import Album
+from utils.snippets import send_email
 
 from .services import extract_archive
 
+logger = logging.getLogger(__name__)
 album_uploaded = Signal()
 
 
 @shared_task
-def process_album_upload(archive_upload_id: str, album_id: int):
+def process_album_upload(
+    archive_upload_id: str, album_id: int, uploader_id: int | None = None
+):
+    upload = TemporaryUpload.objects.get(upload_id=archive_upload_id)
+    archive = upload.file
+
     try:
         album = Album.objects.get(id=album_id)
     except Album.DoesNotExist:
-        return
+        logger.exception("Album %s does not exist", album_id)
+        archive.delete()
+        upload.delete()
+
+    uploader = Member.objects.get(id=uploader_id) if uploader_id is not None else None
 
-    upload = TemporaryUpload.objects.get(upload_id=archive_upload_id)
-    archive = upload.file
     try:
         with transaction.atomic():
             # We make the upload atomic separately, so we can keep using the db if it fails.
             # See https://docs.djangoproject.com/en/4.2/topics/db/transactions/#handling-exceptions-within-postgresql-transactions.
-            extract_archive(album, archive)
+            warnings, count = extract_archive(album, archive)
             album.is_processing = False
             album.save()
 
             # Send signal to notify that an album has been uploaded. This is used
             # by facedetection, and possibly in the future to notify the uploader.
             album_uploaded.send(sender=None, album=album)
+
+            if uploader is not None:
+                # Notify uploader of the upload result.
+                send_email(
+                    to=get_member_email_addresses(uploader),
+                    subject=("Album upload processed completed."),
+                    txt_template="photos/email/upload-processed.txt",
+                    context={
+                        "name": uploader.first_name,
+                        "album": album,
+                        "upload_name": upload.file.name,
+                        "warnings": warnings,
+                        "num_processed": count,
+                    },
+                )
+    except Exception as e:
+        logger.exception(f"Failed to process album upload: {e}", exc_info=e)
+
     finally:
         archive.delete()
         upload.delete()

website/photos/templates/photos/emails/upload-processed.txt

@@ -0,0 +1,16 @@
+Hi {{ name }},
+
+You have recently uploaded an archive ('{{ upload_name }}') to this album: {{ album }}
+You can see it here: {% url "photos:album" album.slug %}
+
+{{ num_processed }} photos were successfully processed.
+{% if warnings %}
+The following warnings occurred while processing the upload:{% for file, warning in warnings%}
+{{ file }}: {{ warning }}
+{% endfor %}{% else %}
+There were no issues while processing the upload.</p>
+{% endif %}
+
+Kisses,
+
+The website