files/models.py

@@ -197,32 +197,28 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model):
     def get_submit_url(self) -> str:
         return self.extension.get_draft_url()
 
-    @property
+    def get_thumbnail_of_size(self, size_key: str) -> str:
-    def thumbnail_l_url(self) -> str:
+        """Return absolute path portion of the URL of a thumbnail of this file.
-        """Return absolute path portion of the URL of the large thumbnail of this file.
 
         Fall back to the source file, if no thumbnail is stored.
         Log absence of the thumbnail file instead of exploding somewhere in the templates.
         """
+        if not self.is_image and not self.is_video:
+            return ''
         try:
-            return self.thumbnail.url
+            path = self.metadata['thumbnails'][size_key]['path']
-        except ValueError:
+            return self.thumbnail.storage.url(path)
-            log.exception(f'File pk={self.pk} is missing a large thumbnail')
+        except (KeyError, TypeError):
+            log.exception(f'File pk={self.pk} is missing thumbnail "{size_key}": {self.metadata}')
         return self.source.url
 
+    @property
+    def thumbnail_l_url(self) -> str:
+        return self.get_thumbnail_of_size('l')
+
     @property
     def thumbnail_s_url(self) -> str:
-        """Return absolute path portion of the URL of the small thumbnail of this file.
+        return self.get_thumbnail_of_size('s')
-
-        Fall back to the source file, if no thumbnail is stored.
-        Log absence of the thumbnail file instead of exploding somewhere in the templates.
-        """
-        try:
-            s = self.metadata['thumbnails']['s']['path']
-            return self.thumbnail.storage.url(s)
-        except (KeyError, TypeError):
-            log.exception(f'File pk={self.pk} is missing a small thumbnail: {self.metadata}')
-        return self.source.url
 
 
 class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model):

files/tasks.py

@@ -42,21 +42,24 @@ def make_thumbnails(file_id: int) -> None:
     assert file.is_image or file.is_video, f'File pk={file_id} is neither image nor video'
 
     # For an image, source of the thumbnails is the original image
-    source_path = os.path.join(settings.MEDIA_ROOT, file.source.path)
+    source_path = file.source.path
+    thumbnail_field = file.thumbnail
+    unchanged_thumbnail = thumbnail_field.name
 
     if file.is_video:
-        f_path = os.path.join(settings.MEDIA_ROOT, files.utils.get_thumbnail_upload_to(file.hash))
+        frame_path = files.utils.get_thumbnail_upload_to(file.hash)
         # For a video, source of the thumbnails is a frame extracted with ffpeg
-        files.utils.extract_frame(source_path, f_path)
+        files.utils.extract_frame(source_path, frame_path)
-        source_path = f_path
+        thumbnail_field.name = frame_path
+        source_path = frame_path
 
     thumbnails = files.utils.make_thumbnails(source_path, file.hash)
-    thumbnail_default_path = thumbnails['l']['path']
+
+    if not thumbnail_field.name:
+        thumbnail_field.name = thumbnails['l']['path']
 
     update_fields = set()
-    thumbnail_field = file.thumbnail
+    if thumbnail_field.name != unchanged_thumbnail:
-    if thumbnail_default_path != thumbnail_field.name:
-        thumbnail_field.name = thumbnail_default_path
         update_fields.add('thumbnail')
     if file.metadata.get('thumbnails') != thumbnails:
         file.metadata.update({'thumbnails': thumbnails})

files/tests/test_tasks.py

@@ -12,11 +12,33 @@ TEST_MEDIA_DIR = Path(__file__).resolve().parent / 'media'
 
 @override_settings(MEDIA_ROOT=TEST_MEDIA_DIR)
 class TasksTest(TestCase):
+    def test_make_thumbnails_fails_when_no_validation(self):
+        file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg')
+
+        with self.assertRaises(files.models.File.validation.RelatedObjectDoesNotExist):
+            make_thumbnails.task_function(file_id=file.pk)
+
+    def test_make_thumbnails_fails_when_validation_not_ok(self):
+        file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg')
+        files.models.FileValidation.objects.create(file=file, is_ok=False, results={})
+
+        with self.assertRaises(AssertionError):
+            make_thumbnails.task_function(file_id=file.pk)
+
+    def test_make_thumbnails_fails_when_not_image_or_video(self):
+        file = FileFactory(
+            original_hash='foobar', source='file/source.zip', type=files.models.File.TYPES.THEME
+        )
+
+        make_thumbnails.task_function(file_id=file.pk)
+
     @patch('files.utils.resize_image')
     @patch('files.utils.Image')
     def test_make_thumbnails_for_image(self, mock_image, mock_resize_image):
         file = FileFactory(original_hash='foobar', source='file/original_image_source.jpg')
         files.models.FileValidation.objects.create(file=file, is_ok=True, results={})
+        self.assertIsNone(file.thumbnail.name)
+        self.assertEqual(file.metadata, {})
 
         make_thumbnails.task_function(file_id=file.pk)
 
@@ -24,7 +46,9 @@ class TasksTest(TestCase):
             str(TEST_MEDIA_DIR / 'file' / 'original_image_source.jpg')
         )
         mock_image.open.return_value.close.assert_called_once()
+
         file.refresh_from_db()
+        self.assertEqual(file.thumbnail.name, 'thumbnails/fo/foobar_1920x1080.png')
         self.assertEqual(
             file.metadata,
             {
@@ -34,7 +58,6 @@ class TasksTest(TestCase):
                 },
             },
         )
-        self.assertEqual(file.thumbnail.name, 'thumbnails/fo/foobar_1920x1080.png')
 
     @patch('files.utils.resize_image')
     @patch('files.utils.Image')
@@ -44,6 +67,8 @@ class TasksTest(TestCase):
             original_hash='deadbeef', source='file/path.mp4', type=files.models.File.TYPES.VIDEO
         )
         files.models.FileValidation.objects.create(file=file, is_ok=True, results={})
+        self.assertIsNone(file.thumbnail.name)
+        self.assertEqual(file.metadata, {})
 
         make_thumbnails.task_function(file_id=file.pk)
 
@@ -52,7 +77,11 @@ class TasksTest(TestCase):
             str(TEST_MEDIA_DIR / 'thumbnails' / 'de' / 'deadbeef.png')
         )
         mock_image.open.return_value.close.assert_called_once()
+
         file.refresh_from_db()
+        # Check that the extracted frame is stored instead of the large thumbnail
+        self.assertEqual(file.thumbnail.name, 'thumbnails/de/deadbeef.png')
+        # Check that File metadata and thumbnail fields were updated
         self.assertEqual(
             file.metadata,
             {
@@ -62,4 +91,3 @@ class TasksTest(TestCase):
                 },
             },
         )
-        self.assertEqual(file.thumbnail.name, 'thumbnails/de/deadbeef_1920x1080.png')

files/utils.py

@@ -11,6 +11,7 @@ import typing
 import zipfile
 
 from PIL import Image
+from django.conf import settings
 from django.core.files.storage import default_storage
 from ffmpeg import FFmpeg, FFmpegFileNotFound, FFmpegInvalidCommand, FFmpegError
 from lxml import etree
@@ -225,19 +226,22 @@ def resize_image(image: Image, size: tuple, output, output_format: str = 'PNG',
     source_image.save(output, output_format, **output_params)
 
 
-def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT) -> dict:
+def make_thumbnails(
+    source_path: str, file_hash: str, output_format: str = THUMBNAIL_FORMAT
+) -> dict:
     """Generate thumbnail files for given file and a predefined list of dimensions.
 
     Resulting thumbnail paths a derived from the given file hash and thumbnail sizes.
     Return a dict of size keys to output paths of generated thumbnail images.
     """
     thumbnails = {}
-    image = Image.open(file_path)
+    abs_path = os.path.join(settings.MEDIA_ROOT, source_path)
+    image = Image.open(abs_path)
     for size_key, (w, h) in THUMBNAIL_SIZES.items():
         output_path = get_thumbnail_upload_to(file_hash, width=w, height=h)
         size = (w, h)
         with tempfile.TemporaryFile() as f:
-            logger.info('Resizing %s to %s (%s)', file_path, size, output_format)
+            logger.info('Resizing %s to %s (%s)', abs_path, size, output_format)
             resize_image(
                 image,
                 size,
@@ -261,13 +265,14 @@ def make_thumbnails(file_path: str, file_hash: str, output_format: str = THUMBNA
 def extract_frame(source_path: str, output_path: str, at_time: str = '00:00:00.01'):
     """Extract a single frame of a video at a given path, write it to the given output path."""
     try:
+        abs_path = os.path.join(settings.MEDIA_ROOT, output_path)
         ffmpeg = (
             FFmpeg()
             .option('y')
             .input(source_path)
-            .output(output_path, {'ss': at_time, 'frames:v': 1, 'update': 'true'})
+            .output(abs_path, {'ss': at_time, 'frames:v': 1, 'update': 'true'})
         )
-        output_dir = os.path.dirname(output_path)
+        output_dir = os.path.dirname(abs_path)
         if not os.path.isdir(output_dir):
             os.mkdir(output_dir)
         ffmpeg.execute()