common/admin.py

@@ -65,10 +65,17 @@ def link_to(field_name, title=None):
 
     @admin.display(description=title, ordering=field_name)
     def _raw(obj):
-        if related_field_name:
-            target_obj = getattr(getattr(obj, field_name), related_field_name)
-        else:
         target_obj = getattr(obj, field_name)
+        if isinstance(target_obj, models.Manager):
+            admin_urls = []
+            for _obj in target_obj.all():
+                if related_field_name:
+                    _obj = getattr(_obj, related_field_name)
+                admin_urls.append(get_admin_change_url(_obj))
+            return format_html('<br>'.join(admin_urls))
+
+        if related_field_name:
+            target_obj = getattr(target_obj, related_field_name)
 
         admin_url = get_admin_change_url(target_obj)
         return admin_url

common/tests/factories/extensions.py

@@ -108,10 +108,7 @@ class VersionFactory(DjangoModelFactory):
 
 def create_version(**kwargs) -> 'Version':
     version = VersionFactory(**kwargs)
-    file = version.file
+    version.extension.authors.add(version.file.user)
-    file.extension = version.extension
-    file.save(update_fields={'extension'})
-    file.extension.authors.add(version.file.user)
     return version
 
 

extensions/forms.py

@@ -64,18 +64,16 @@ class AddPreviewFileForm(files.forms.BaseMediaFileForm):
         """Save Preview from the cleaned form data."""
         instance = super().save(*args, **kwargs)
 
-        # Create extension preview and save caption to it
+        # Create extension preview and save caption to it, ignore duplicate records
-        extensions.models.Preview.objects.create(
+        extensions.models.Preview.objects.get_or_create(
             file=instance,
-            caption=self.cleaned_data['caption'],
             extension=self.extension,
+            defaults={'caption': self.cleaned_data['caption']},
         )
         return instance
 
 
 class AddPreviewModelFormSet(forms.BaseModelFormSet):
-    msg_duplicate_file = _('Please select another file instead of the duplicate')
-
     def __init__(self, *args, **kwargs):
         self.request = kwargs.pop('request')
         self.extension = kwargs.pop('extension')
@@ -89,14 +87,6 @@ class AddPreviewModelFormSet(forms.BaseModelFormSet):
         form_kwargs['extension'] = self.extension
         return form_kwargs
 
-    def get_unique_error_message(self, unique_check):
-        """Replace duplicate `original_hash`/`hash` message with a more meaningful one."""
-        if len(unique_check) == 1:
-            field = unique_check[0]
-            if field in ('original_hash', 'hash'):
-                return self.msg_duplicate_file
-        return super().get_unique_error_message(unique_check)
-
 
 AddPreviewFormSet = forms.modelformset_factory(
     files.models.File,
@@ -116,7 +106,6 @@ class ExtensionUpdateForm(forms.ModelForm):
         'An extension can be converted to draft only while it is Awating Review'
     )
     msg_need_previews = _('Please add at least one preview.')
-    msg_duplicate_file = _('Please select another file instead of the duplicate.')
 
     class Meta:
         model = extensions.models.Extension
@@ -198,16 +187,17 @@ class ExtensionUpdateForm(forms.ModelForm):
             self.featured_image_form,
             self.icon_form,
         ]
-        seen_hashes = set()
+        seen_hashes = {}
+        # Ignore duplicate files in the formsets: point all forms sharing the hash to the same
+        # file instance, when forms call .save() it'll be on the same instance.
         for f in new_file_forms:
             hash = f.instance.original_hash
-            if hash:
+            if not hash:
+                continue
             if hash in seen_hashes:
-                    f.add_error('source', self.msg_duplicate_file)
+                f.instance = seen_hashes[hash]
-                    is_valid_flags.append(False)
+            else:
-                    break
+                seen_hashes[hash] = f.instance
-                seen_hashes.add(hash)
-
         return all(is_valid_flags)
 
     def clean_team(self):
@@ -352,9 +342,9 @@ class IconForm(files.forms.BaseMediaFileForm):
     }
     expected_size_px = 256
 
-    def clean_source(self):
+    def clean_source(self, *args, **kwargs):
         """Check image resolution."""
-        source = self.cleaned_data.get('source')
+        source = super().clean_source(*args, **kwargs)
         if not source:
             return
         image = getattr(source, 'image', None)

extensions/migrations/0034_alter_extension_featured_image_alter_extension_icon_and_more.py

@@ -0,0 +1,30 @@
+# Generated by Django 4.2.11 on 2024-06-04 08:54
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('files', '0010_remove_file_extension_alter_file_hash_and_more'),
+        ('extensions', '0033_extensions_fts_20240603_1918'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='extension',
+            name='featured_image',
+            field=models.ForeignKey(help_text='Shown by social networks when this extension is shared (used as `og:image` metadata field).Should have resolution of at least 1920 x 1080 and aspect ratio of 16:9.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='featured_image_of', to='files.file'),
+        ),
+        migrations.AlterField(
+            model_name='extension',
+            name='icon',
+            field=models.ForeignKey(help_text='A 256 x 256 PNG icon representing this extension.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='icon_of', to='files.file'),
+        ),
+        migrations.AlterField(
+            model_name='preview',
+            name='file',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='files.file'),
+        ),
+    ]

extensions/models.py

@@ -178,24 +178,24 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod
         related_name='latest_version_of',
     )
 
-    featured_image = models.OneToOneField(
+    featured_image = models.ForeignKey(
         'files.File',
         related_name='featured_image_of',
         null=True,
         blank=False,
-        on_delete=models.SET_NULL,
+        on_delete=models.PROTECT,
         help_text=(
             "Shown by social networks when this extension is shared"
             " (used as `og:image` metadata field)."
             "Should have resolution of at least 1920 x 1080 and aspect ratio of 16:9."
         ),
     )
-    icon = models.OneToOneField(
+    icon = models.ForeignKey(
         'files.File',
         related_name='icon_of',
         null=True,
         blank=False,
-        on_delete=models.SET_NULL,
+        on_delete=models.PROTECT,
         help_text="A 256 x 256 PNG icon representing this extension.",
     )
     previews = FilterableManyToManyField('files.File', through='Preview', related_name='extensions')
@@ -748,12 +748,14 @@ class Maintainer(CreatedModifiedMixin, models.Model):
 
 class Preview(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
     extension = models.ForeignKey(Extension, on_delete=models.CASCADE)
-    file = models.OneToOneField('files.File', on_delete=models.CASCADE)
+    # Files can potentially be referenced by different extensions
+    file = models.ForeignKey('files.File', on_delete=models.PROTECT)
     caption = models.CharField(max_length=255, default='', null=False, blank=True)
     position = models.IntegerField(default=0)
 
     class Meta:
         ordering = ('position', 'date_created')
+        # We don't want to have duplicate previews on the same extension
         unique_together = [['extension', 'file']]
 
     @property

extensions/signals.py

@@ -5,7 +5,7 @@ from actstream.actions import follow, unfollow
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import Group
 from django.db import transaction
-from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete
+from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete, post_delete
 from django.dispatch import receiver
 
 from constants.activity import Flag
@@ -16,22 +16,6 @@ logger = logging.getLogger(__name__)
 User = get_user_model()
 
 
-@receiver(pre_save, sender=extensions.models.Preview)
-def _set_extension(
-    sender: object, instance: extensions.models.Preview, raw: bool, **kwargs: object
-) -> None:
-    if raw:
-        return
-
-    file = instance.file
-    if not file:
-        return
-
-    if not file.extension_id:
-        file.extension_id = instance.extension_id
-        file.save(update_fields={'extension_id'})
-
-
 @receiver(pre_delete, sender=extensions.models.Extension)
 @receiver(pre_delete, sender=extensions.models.Preview)
 @receiver(pre_delete, sender=extensions.models.Version)
@@ -45,6 +29,20 @@ def _log_deletion(
     instance.record_deletion()
 
 
+@receiver(post_delete, sender=extensions.models.Version)
+def _delete_version_file(
+    sender: object, instance: extensions.models.Version, **kwargs: object
+) -> None:
+    # **N.B.**: this isn't part of an overloaded `Version.delete()` method because
+    # that method isn't called when `Extension.delete()` cascades to deleting the versions:
+    #
+    # delete() method for an object is not necessarily called ... as a result of a cascading delete
+    # https://docs.djangoproject.com/en/4.2/topics/db/models/#overriding-predefined-model-methods
+    version_file = instance.file
+    logger.info('Deleting file pk=%s of Version pk=%s', version_file.pk, instance.pk)
+    version_file.delete()
+
+
 @receiver(pre_save, sender=extensions.models.Extension)
 def _record_changes(
     sender: object,

extensions/tests/test_delete.py

@@ -21,23 +21,38 @@ class DeleteTest(TestCase):
 
     def test_unlisted_unrated_extension_can_be_deleted_by_author(self):
         self.maxDiff = None
+        reused_image = FileFactory(
+            type=files.models.File.TYPES.IMAGE,
+            original_name='extension_feature_image.png',
+            source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png',
+        )
         version = create_version(
             file__status=files.models.File.STATUSES.AWAITING_REVIEW,
             ratings=[],
+            extension__icon=FileFactory(
+                type=files.models.File.TYPES.IMAGE,
+                original_name='extension_icon_final.png',
+                source='images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png',
+            ),
+            extension__featured_image=reused_image,
             extension__previews=[
+                reused_image,
                 FileFactory(
                     type=files.models.File.TYPES.IMAGE,
+                    original_name='extension_preview_001.png',
                     source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png',
-                )
+                ),
             ],
         )
         extension = version.extension
         version_file = version.file
+        icon = extension.icon
+        featured_image = extension.featured_image
         self.assertEqual(version_file.get_status_display(), 'Awaiting Review')
         self.assertEqual(extension.get_status_display(), 'Draft')
         self.assertFalse(extension.is_listed)
         self.assertEqual(extension.cannot_be_deleted_reasons, [])
-        preview_file = extension.previews.first()
+        preview_file = extension.previews.last()
         self.assertIsNotNone(preview_file)
         # Create some ApprovalActivity as well
         moderator = create_moderator()
@@ -55,11 +70,11 @@ class DeleteTest(TestCase):
                 repr,
                 [
                     version_file,
-                    preview_file,
+                    file_validation,
                     extension,
                     approval_activity,
-                    file_validation,
+                    preview_file.preview_set.first(),
-                    preview_file.preview,
+                    reused_image.preview_set.first(),
                     version,
                 ],
             )
@@ -78,12 +93,16 @@ class DeleteTest(TestCase):
             version.refresh_from_db()
         with self.assertRaises(files.models.File.DoesNotExist):
             version_file.refresh_from_db()
-        with self.assertRaises(files.models.File.DoesNotExist):
+        # Preview files aren't deleted: they might be re-uploaded shortly and should be looked up by hash
         preview_file.refresh_from_db()
+        icon.refresh_from_db()
+        featured_image.refresh_from_db()
         self.assertIsNone(extensions.models.Extension.objects.filter(pk=extension.pk).first())
         self.assertIsNone(extensions.models.Version.objects.filter(pk=version.pk).first())
         self.assertIsNone(files.models.File.objects.filter(pk=version_file.pk).first())
-        self.assertIsNone(files.models.File.objects.filter(pk=preview_file.pk).first())
+        self.assertIsNotNone(files.models.File.objects.filter(pk=preview_file.pk).first())
+        self.assertIsNotNone(files.models.File.objects.filter(pk=icon.pk).first())
+        self.assertIsNotNone(files.models.File.objects.filter(pk=featured_image.pk).first())
 
         # Check that each of the deleted records was logged
         deletion_log_entries_q = LogEntry.objects.filter(action_flag=DELETION)

extensions/tests/test_manifest.py

@@ -1,5 +1,5 @@
 from django.test import TestCase
-from django.urls import reverse
+from django.urls import reverse_lazy
 from django.core.exceptions import ValidationError
 import factory
 
@@ -35,6 +35,8 @@ META_DATA = {
 
 
 class CreateFileTest(TestCase):
+    submit_url = reverse_lazy('extensions:submit')
+
     def setUp(self):
         super().setUp()
 
@@ -52,10 +54,6 @@ class CreateFileTest(TestCase):
         super().tearDown()
         shutil.rmtree(self.temp_directory)
 
-    @classmethod
-    def _get_submit_url(cls):
-        return reverse('extensions:submit')
-
     def _create_valid_extension(self, extension_id):
         return create_approved_version(
             extension__name='Blender Kitsu',
@@ -120,9 +118,7 @@ class ValidateManifestTest(CreateFileTest):
 
         bad_file = self._create_file_from_data("theme.zip", file_data, self.user)
         with open(bad_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 200)
         error = response.context['form'].errors.get('source')[0]
@@ -140,9 +136,7 @@ class ValidateManifestTest(CreateFileTest):
 
         bad_file = self._create_file_from_data("theme.zip", file_data, self.user)
         with open(bad_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 200)
         error = response.context['form'].errors.get('source')[0]
@@ -165,9 +159,7 @@ class ValidateManifestTest(CreateFileTest):
 
         extension_file = self._create_file_from_data("theme.zip", kitsu_1_5, self.user)
         with open(extension_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 200)
         error = response.context['form'].errors.get('source')[0]
@@ -191,9 +183,7 @@ class ValidateManifestTest(CreateFileTest):
 
         extension_file = self._create_file_from_data("theme.zip", kitsu_1_5, self.user)
         with open(extension_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 200)
         error = response.context['form'].errors.get('source')[0]
@@ -317,9 +307,7 @@ class ValidateManifestTest(CreateFileTest):
 
         bad_file = self._create_file_from_data("theme.zip", file_data, self.user)
         with open(bad_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 200)
         error = response.context['form'].errors.get('source')
@@ -338,13 +326,11 @@ class ValidateManifestTest(CreateFileTest):
 
         extension_file = self._create_file_from_data("theme.zip", file_data, self.user)
         with open(extension_file, 'rb') as fp:
-            response = self.client.post(
+            response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True})
-                self._get_submit_url(), {'source': fp, 'agreed_with_terms': True}
-            )
 
         self.assertEqual(response.status_code, 302)
         file = File.objects.first()
-        extension = file.extension
+        extension = file.version.extension
         self.assertEqual(extension.slug, 'an-id')
         self.assertEqual(extension.name, 'Name. - With  Extra spaces and other characters Ж')
 

extensions/tests/test_submit.py

@@ -137,8 +137,8 @@ class SubmitFileTest(TestCase):
         self.assertEqual(response.status_code, 302)
         self.assertEqual(File.objects.count(), 1)
         file = File.objects.first()
-        self.assertEqual(response['Location'], file.get_submit_url())
+        self.assertEqual(response['Location'], file.version.extension.get_draft_url())
-        extension = file.extension
+        extension = file.version.extension
         self.assertEqual(extension.slug, slug)
         self.assertEqual(extension.name, name)
         self.assertEqual(file.original_name, file_name)
@@ -203,8 +203,7 @@ class SubmitFileTest(TestCase):
         self.assertEqual(response.status_code, 302, _get_all_form_errors(response))
         self.assertEqual(File.objects.count(), 1)
         file = File.objects.first()
-        self.assertIsNotNone(file.extension_id)
+        self.assertEqual(response['Location'], file.version.extension.get_draft_url())
-        self.assertEqual(response['Location'], file.get_submit_url())
         self.assertEqual(file.user, user)
         self.assertEqual(file.original_name, 'theme.zip')
         self.assertEqual(file.size_bytes, 5895)
@@ -285,18 +284,19 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase):
         )
 
     def test_get_finalise_addon_redirects_if_anonymous(self):
-        response = self.client.post(self.file.get_submit_url(), {})
+        response = self.client.post(self.file.version.extension.get_draft_url(), {})
 
         self.assertEqual(response.status_code, 302)
         self.assertEqual(
-            response['Location'], f'/oauth/login?next=/add-ons/{self.file.extension.slug}/draft/'
+            response['Location'],
+            f'/oauth/login?next=/add-ons/{self.file.version.extension.slug}/draft/',
         )
 
     def test_get_finalise_addon_not_allowed_if_different_user(self):
         user = UserFactory()
         self.client.force_login(user)
 
-        response = self.client.post(self.file.get_submit_url(), {})
+        response = self.client.post(self.file.version.extension.get_draft_url(), {})
 
         # Technically this could (should) be a 403, but changing this means changing
         # the MaintainedExtensionMixin which is used in multiple places.
@@ -305,7 +305,7 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase):
     def test_post_finalise_addon_validation_errors(self):
         self.client.force_login(self.file.user)
         data = {**POST_DATA, 'submit_draft': ''}
-        response = self.client.post(self.file.get_submit_url(), data)
+        response = self.client.post(self.file.version.extension.get_draft_url(), data)
 
         self.assertEqual(response.status_code, 200)
         self.assertDictEqual(
@@ -371,7 +371,9 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase):
                 'icon-source': fp3,
                 'featured-image-source': fp4,
             }
-            response = self.client.post(self.file.get_submit_url(), {**data, **files})
+            response = self.client.post(
+                self.file.version.extension.get_draft_url(), {**data, **files}
+            )
 
         self.assertEqual(response.status_code, 302, _get_all_form_errors(response))
         self.assertEqual(response['Location'], '/add-ons/edit-breakdown/manage/')

extensions/tests/test_update.py

@@ -81,7 +81,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
         self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), 1)
         self.assertEqual(extension.previews.count(), 1)
         file1 = extension.previews.all()[0]
-        self.assertEqual(file1.preview.caption, 'First Preview Caption Text')
+        self.assertEqual(file1.preview_set.first().caption, 'First Preview Caption Text')
         self.assertEqual(
             file1.original_hash,
             'sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
@@ -125,7 +125,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
         extension.refresh_from_db()
         self.assertEqual(extension.previews.count(), 1)
         video_file = extension.previews.all()[0]
-        self.assertEqual(video_file.preview.caption, 'First Preview Caption Text')
+        self.assertEqual(video_file.preview_set.first().caption, 'First Preview Caption Text')
         self._test_file_properties(
             video_file,
             content_type='video/mp4',
@@ -169,8 +169,8 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
         self.assertEqual(extension.previews.count(), 2)
         file1 = extension.previews.all()[0]
         file2 = extension.previews.all()[1]
-        self.assertEqual(file1.preview.caption, 'First Preview Caption Text')
+        self.assertEqual(file1.preview_set.first().caption, 'First Preview Caption Text')
-        self.assertEqual(file2.preview.caption, 'Second Preview Caption Text')
+        self.assertEqual(file2.preview_set.first().caption, 'Second Preview Caption Text')
         self.assertEqual(
             file1.original_hash,
             'sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
@@ -222,8 +222,10 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             _get_all_form_errors(response),
         )
 
-    def test_post_upload_validation_error_duplicate_images(self):
+    def test_post_upload_duplicate_preview_files_ignored(self):
         extension = create_approved_version().extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertEqual(extension.previews.count(), 0)
 
         data = {
             **POST_DATA,
@@ -243,31 +245,24 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             }
             response = self.client.post(url, {**data, **files})
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
-        self.maxDiff = None
+        # One image was uploaded successfully
         self.assertEqual(
-            [
+            File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before + 1
-                response.context['add_preview_formset'].forms[0].errors,
-                response.context['add_preview_formset'].forms[1].errors,
-                response.context['add_preview_formset'].non_form_errors(),
-            ],
-            [
-                {},
-                {
-                    '__all__': ['Please correct the duplicate values below.'],
-                    'source': ['Please select another file instead of the duplicate.'],
-                },
-                ['Please select another file instead of the duplicate'],
-            ],
         )
+        self.assertEqual(extension.previews.count(), 1)
 
-    def test_post_upload_validation_error_image_already_exists(self):
+    def test_post_upload_existing_image_file_linked_to_extension(self):
-        FileFactory(
+        file = FileFactory(
+            type=File.TYPES.IMAGE,
             original_hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
             hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
             source='file/original_image_source.jpg',
         )
         extension = create_approved_version().extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertEqual(extension.previews.count(), 0)
+        old_user = file.user
 
         data = {
             **POST_DATA,
@@ -281,15 +276,56 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             files = {'form-0-source': fp1}
             response = self.client.post(url, {**data, **files})
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
-        self.maxDiff = None
+        # No new files were created: the existing one was linked to the extension instead
-        self.assertEqual(
+        self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before)
-            response.context['add_preview_formset'].forms[0].errors,
+        self.assertEqual(extension.previews.count(), 1)
-            {'source': ['File with this Original hash already exists.']},
+        self.assertEqual(extension.previews.first().original_hash, file.original_hash)
-        )
+        self.assertEqual(extension.previews.first().pk, file.pk)
+        file.refresh_from_db()
+        self.assertEqual(file.user, old_user)
 
-    def test_post_upload_validation_error_duplicate_across_forms(self):
+    def test_post_upload_existing_preview_file_linked_to_extension(self):
+        file = FileFactory(
+            type=File.TYPES.IMAGE,
+            original_hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
+            hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340',
+            source='file/original_image_source.jpg',
+        )
         extension = create_approved_version().extension
+        another_extension = create_approved_version(extension__previews=[file]).extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertEqual(extension.previews.count(), 0)
+        self.assertEqual(another_extension.previews.count(), 1)
+        old_user = file.user
+
+        data = {
+            **POST_DATA,
+            'form-TOTAL_FORMS': ['1'],
+        }
+        file_name1 = 'test_preview_image_0001.png'
+        url = extension.get_manage_url()
+        user = extension.authors.first()
+        self.client.force_login(user)
+        with open(TEST_FILES_DIR / file_name1, 'rb') as fp1:
+            files = {'form-0-source': fp1}
+            response = self.client.post(url, {**data, **files})
+
+        self.assertEqual(response.status_code, 302)
+        # No new files were created: the existing one was linked to the extension instead
+        self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before)
+        self.assertEqual(extension.previews.count(), 1)
+        self.assertEqual(extension.previews.first().original_hash, file.original_hash)
+        self.assertEqual(extension.previews.first().pk, file.pk)
+        # File is referenced as a preview by both extensions
+        self.assertEqual(file.preview_set.count(), 2)
+        file.refresh_from_db()
+        self.assertEqual(file.user, old_user)
+
+    def test_post_upload_duplicates_are_ignored_across_forms(self):
+        extension = create_approved_version().extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertEqual(extension.previews.count(), 0)
 
         data = {
             **POST_DATA,
@@ -304,12 +340,12 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             files = {'form-0-source': fp, 'icon-source': fp1}
             response = self.client.post(url, {**data, **files})
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
-        self.maxDiff = None
+        # One image was uploaded successfully
         self.assertEqual(
-            response.context['icon_form'].errors,
+            File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before + 1
-            {'source': ['Please select another file instead of the duplicate.']},
         )
+        self.assertEqual(extension.previews.count(), 1)
 
     def test_post_upload_validation_error_unexpected_preview_format_gif(self):
         extension = create_approved_version().extension
@@ -419,6 +455,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             original_name='old_icon.png',
             size_bytes=1234,
         )
+        old_icon = extension.icon
 
         self.client.force_login(extension.authors.first())
         url = extension.get_manage_url()
@@ -427,7 +464,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             response = self.client.post(url, {**POST_DATA, **files})
 
         self.assertEqual(response.status_code, 302)
-        extension.icon.refresh_from_db()
+        old_icon.refresh_from_db()
+        extension.refresh_from_db()
+        self.assertNotEqual(extension.icon_id, old_icon.pk)
         self._test_file_properties(
             extension.icon,
             content_type='image/png',
@@ -440,6 +479,68 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             size_bytes=30177,
         )
 
+    def test_update_icon_existing_file_linked(self):
+        file = FileFactory(
+            type=File.TYPES.IMAGE,
+            original_hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472',
+            hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472',
+            source='test_icon_0001.png',
+        )
+        extension = create_approved_version().extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertIsNone(extension.icon)
+        self.assertEqual(file.icon_of.count(), 0)
+        old_user = file.user
+
+        url = extension.get_manage_url()
+        user = extension.authors.first()
+        self.client.force_login(user)
+        with open(TEST_FILES_DIR / 'test_icon_0001.png', 'rb') as fp:
+            files = {'icon-source': fp}
+            response = self.client.post(url, {**POST_DATA, **files})
+
+        self.assertEqual(response.status_code, 302)
+        # No new files were created: the existing one was linked to the extension instead
+        self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before)
+        extension.refresh_from_db()
+        self.assertEqual(extension.icon, file)
+        self.assertEqual(file.icon_of.count(), 1)
+        file.refresh_from_db()
+        self.assertEqual(file.user, old_user)
+
+    def test_update_icon_existing_file_linked_to_multiple_extensions(self):
+        file = FileFactory(
+            type=File.TYPES.IMAGE,
+            original_hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472',
+            hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472',
+            source='test_icon_0001.png',
+        )
+        extension = create_approved_version().extension
+        another_extension = create_approved_version(extension__icon=file).extension
+        images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count()
+        self.assertIsNone(extension.icon)
+        self.assertEqual(another_extension.icon_id, file.pk)
+        old_user = file.user
+
+        url = extension.get_manage_url()
+        user = extension.authors.first()
+        self.client.force_login(user)
+        with open(TEST_FILES_DIR / 'test_icon_0001.png', 'rb') as fp:
+            files = {'icon-source': fp}
+            response = self.client.post(url, {**POST_DATA, **files})
+
+        self.assertEqual(response.status_code, 302)
+        # No new files were created: the existing one was linked to the extension instead
+        self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before)
+        extension.refresh_from_db()
+        another_extension.refresh_from_db()
+        self.assertEqual(extension.icon, file)
+        self.assertEqual(another_extension.icon, file)
+        # File is referenced as a preview by both extensions
+        self.assertEqual(file.icon_of.count(), 2)
+        file.refresh_from_db()
+        self.assertEqual(file.user, old_user)
+
     def test_update_featured_image_changes_expected_file_fields(self):
         extension = create_approved_version(
             extension__featured_image=ImageFactory(
@@ -456,6 +557,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             original_name='old_featured_image.png',
             size_bytes=1234,
         )
+        old_featured_image = extension.featured_image
 
         self.client.force_login(extension.authors.first())
         url = extension.get_manage_url()
@@ -464,7 +566,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
             response = self.client.post(url, {**POST_DATA, **files})
 
         self.assertEqual(response.status_code, 302)
-        extension.featured_image.refresh_from_db()
+        old_featured_image.refresh_from_db()
+        extension.refresh_from_db()
+        self.assertNotEqual(extension.featured_image_id, old_featured_image.pk)
         self._test_file_properties(
             extension.featured_image,
             content_type='image/png',

extensions/views/submit.py

@@ -64,7 +64,6 @@ class UploadFileView(LoginRequiredMixin, CreateView):
         # Need to save the form to be able to use the file to create the version.
         self.object = self.file = form.save()
 
-        self.file.extension = self.extension
         Version.objects.update_or_create(
             extension=self.extension, file=self.file, **self.file.parsed_version_fields
         )[0]

files/admin.py

@@ -41,6 +41,10 @@ class FileAdmin(admin.ModelAdmin):
     class Media:
         css = {'all': ('files/admin/file.css',)}
 
+    def get_queryset(self, *args, **kwargs):
+        q = super().get_queryset(*args, **kwargs)
+        return q.prefetch_related(*self.list_prefetch_related)
+
     def thumbnails(self, obj):
         if not obj or not (obj.is_image or obj.is_video):
             return ''
@@ -72,7 +76,6 @@ class FileAdmin(admin.ModelAdmin):
         'date_created',
         'date_modified',
         'date_status_changed',
-        ('extension', admin.EmptyFieldListFilter),
         ('version', admin.EmptyFieldListFilter),
         ('icon_of', admin.EmptyFieldListFilter),
         ('featured_image_of', admin.EmptyFieldListFilter),
@@ -80,7 +83,6 @@ class FileAdmin(admin.ModelAdmin):
     )
     list_display = (
         'original_name',
-        link_to('extension'),
         link_to('user'),
         'date_created',
         'type',
@@ -88,19 +90,20 @@ class FileAdmin(admin.ModelAdmin):
         link_to('version'),
         link_to('icon_of'),
         link_to('featured_image_of'),
-        link_to('preview.extension', 'preview of'),
+        link_to('preview_set.extension', 'preview of'),
         'is_ok',
     )
 
     list_select_related = (
         'version__extension',
         'user',
-        'extension',
         'version',
         'validation',
+    )
+    list_prefetch_related = (
         'icon_of',
         'featured_image_of',
-        'preview__extension',
+        'preview_set__extension',
     )
 
     autocomplete_fields = ['user']

files/forms.py

@@ -6,7 +6,6 @@ import tempfile
 from django import forms
 from django.utils.safestring import mark_safe
 from django.utils.translation import gettext_lazy as _
-import django.core.exceptions
 
 from .validators import (
     ExtensionIDManifestValidator,
@@ -44,7 +43,7 @@ class FileForm(forms.ModelForm):
 
     class Meta:
         model = files.models.File
-        fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user', 'extension')
+        fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user')
 
     source = forms.FileField(
         allow_empty_file=False,
@@ -123,7 +122,6 @@ class FileForm(forms.ModelForm):
                 'size_bytes': source.size,
                 'original_hash': hash_,
                 'hash': hash_,
-                'extension': self.extension,
             }
         )
 
@@ -180,8 +178,7 @@ class FileFormSkipAgreed(FileForm):
 class BaseMediaFileForm(forms.ModelForm):
     class Meta:
         model = files.models.File
-        fields = ('source', 'original_hash')
+        fields = ('source',)
-        widgets = {'original_hash': forms.HiddenInput()}
 
     source = forms.ImageField(widget=forms.FileInput, allow_empty_file=False)
 
@@ -210,22 +207,23 @@ class BaseMediaFileForm(forms.ModelForm):
 
         super().__init__(*args, **kwargs)
 
-        self.instance.user = self.request.user
+    def clean_source(self, *args, **kwargs):
-
+        """Calculate original hash of the uploaded file, reuse existing record matching it."""
-    def clean_original_hash(self, *args, **kwargs):
-        """Calculate original hash of the uploaded file."""
         source = self.cleaned_data.get('source')
         if 'source' in self.changed_data and source:
-            return files.models.File.generate_hash(source)
+            original_hash = files.models.File.generate_hash(source)
-
+            instance = files.models.File.objects.filter(original_hash=original_hash).first()
-    def add_error(self, field, error):
+            if instance:
-        """Add hidden `original_hash` errors to the visible `source` field instead."""
+                # File with this hash exists already, make sure it's reused here
-        if isinstance(error, django.core.exceptions.ValidationError):
+                if instance.pk != self.instance.pk:
-            if getattr(error, 'error_dict', None):
+                    self.instance = instance
-                hash_error = error.error_dict.pop('original_hash', None)
+            else:
-                if hash_error:
+                previous_hash = self.instance.hash
-                    error.error_dict['source'] = hash_error
+                if previous_hash and original_hash != previous_hash and self.instance.pk:
-        super(forms.ModelForm, self).add_error(field, error)
+                    # Create a new file instead of changing the existing one
+                    self.instance.pk = None
+            self.instance.original_hash = original_hash
+        return source
 
     def save(self, *args, **kwargs):
         """Save as `to_field` on the parent object (Extension)."""
@@ -235,7 +233,9 @@ class BaseMediaFileForm(forms.ModelForm):
             self.instance.original_name = source.name
             self.instance.size_bytes = source.size
 
-        self.instance.extension = self.extension
+        if not self.instance.user_id:
+            self.instance.user = self.request.user
+
         instance = super().save(*args, **kwargs)
 
         if hasattr(self, 'to_field'):

files/migrations/0010_remove_file_extension_alter_file_hash_and_more.py

@@ -0,0 +1,27 @@
+# Generated by Django 4.2.11 on 2024-06-03 14:16
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('files', '0009_file_extension'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='file',
+            name='extension',
+        ),
+        migrations.AlterField(
+            model_name='file',
+            name='hash',
+            field=models.CharField(blank=True, editable=False, max_length=255, unique=True),
+        ),
+        migrations.AlterField(
+            model_name='file',
+            name='original_hash',
+            field=models.CharField(blank=True, editable=False, help_text='The original hash of the file before we repackage it any way.', max_length=255, unique=True),
+        ),
+    ]

files/models.py

@@ -49,12 +49,6 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model):
 
     date_approved = models.DateTimeField(null=True, blank=True, editable=False)
     date_status_changed = models.DateTimeField(null=True, blank=True, editable=False)
-    extension = models.ForeignKey(
-        'extensions.Extension',
-        null=True,
-        blank=True,
-        on_delete=models.CASCADE,
-    )
 
     source = models.FileField(null=False, blank=False, upload_to=file_upload_to)
     thumbnail = models.ImageField(
@@ -82,12 +76,13 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model):
         User, related_name='files', null=False, blank=False, on_delete=models.CASCADE
     )
     size_bytes = models.PositiveBigIntegerField(default=0, editable=False)
-    hash = models.CharField(max_length=255, null=False, blank=True, unique=True)
+    hash = models.CharField(max_length=255, null=False, blank=True, unique=True, editable=False)
     original_name = models.CharField(max_length=255, blank=True, null=False)
     original_hash = models.CharField(
         max_length=255,
         null=False,
         blank=True,
+        editable=False,
         unique=True,
         help_text='The original hash of the file before we repackage it any way.',
     )
@@ -196,9 +191,6 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model):
             'tags': data.get('tags'),
         }
 
-    def get_submit_url(self) -> str:
-        return self.extension.get_draft_url()
-
     def get_thumbnail_of_size(self, size_key: str) -> str:
         """Return absolute path portion of the URL of a thumbnail of this file.
 

notifications/tests/test_follow_logic.py

@@ -159,7 +159,7 @@ class TestTasks(TestCase):
                 'form-0-source': fp1,
                 'form-1-source': fp2,
             }
-            self.client.post(file.get_submit_url(), {**data, **files})
+            self.client.post(file.version.extension.get_draft_url(), {**data, **files})
         new_notification_nr = Notification.objects.filter(recipient=moderator).count()
         self.assertEqual(new_notification_nr, notification_nr + 1)