extensions/admin.py

@@ -5,7 +5,7 @@ from django.contrib import admin
 
 from . import models
 from common.admin import NoAddDeleteMixin
-from extensions.models import Extension, Maintainer, Tag, Version, VersionFiles
+from extensions.models import Extension, Maintainer, Tag, Version, VersionFile
 
 
 log = logging.getLogger(__name__)
@@ -27,12 +27,17 @@ class PreviewInline(NoAddDeleteMixin, admin.TabularInline):
 
 class VersionInline(NoAddDeleteMixin, admin.TabularInline):
     model = Version
-    fields = ('version', 'blender_version_min', 'blender_version_max', 'file')
+    fields = ('version', 'blender_version_min', 'blender_version_max')
-    raw_id_fields = ('file',)
     show_change_link = True
     extra = 0
 
 
+class VersionFileInline(NoAddDeleteMixin, admin.TabularInline):
+    model = Version.files.through
+    autocomplete_fields = ('file',)
+    extra = 0
+
+
 class ExtensionAdmin(admin.ModelAdmin):
     save_on_top = True
     date_hierarchy = 'date_created'
@@ -148,7 +153,7 @@ class VersionAdmin(admin.ModelAdmin):
         'download_count',
     )
     list_filter = (
-        'file__status',
+        'files__status',
         'blender_version_min',
         'blender_version_max',
         'permissions',
@@ -163,11 +168,11 @@ class VersionAdmin(admin.ModelAdmin):
         'extension__slug',
         'extension__name',
         'extension__extension_id',
-        'file__user__email',
+        'files__user__email',
-        'file__user__full_name',
+        'files__user__full_name',
-        'file__user__username',
+        'files__user__username',
     )
-    autocomplete_fields = ('extension', 'file')
+    autocomplete_fields = ('extension', 'files')
     readonly_fields = (
         'id',
         'tagline',
@@ -191,7 +196,6 @@ class VersionAdmin(admin.ModelAdmin):
                     'release_notes',
                     'licenses',
                     'tags',
-                    'file',
                     'permissions',
                     'platforms',
                 ),
@@ -204,6 +208,7 @@ class VersionAdmin(admin.ModelAdmin):
             },
         ),
     )
+    inlines = (VersionFileInline,)
 
     def get_urls(self):
         def wrap(view):
@@ -243,8 +248,8 @@ class TagAdmin(admin.ModelAdmin):
             return ()
 
 
-class VersionFilesAdmin(admin.ModelAdmin):
+class VersionFileAdmin(admin.ModelAdmin):
-    model = VersionFiles
+    model = VersionFile
     list_display = 'file'
     readonly_fields = 'file'
 

extensions/forms.py

@@ -303,21 +303,21 @@ class ExtensionDeleteForm(forms.ModelForm):
 class VersionForm(forms.ModelForm):
     class Meta:
         model = extensions.models.Version
-        fields = {'file', 'release_notes'}
+        fields = {'files', 'release_notes'}
 
     def __init__(self, *args, **kwargs):
         """Limit 'file' choices to the initial file value."""
         super().__init__(*args, **kwargs)
 
         # Mark 'file' field as disabled so that Django form allows using its initial value.
-        self.fields['file'].disabled = True
+        self.fields['files'].disabled = True
 
-    def clean_file(self, *args, **kwargs):
+    def clean_files(self, *args, **kwargs):
         """Return file that was passed to the form via the initial values.
 
         This ensures that it doesn't have to be supplied by the form data.
         """
-        return self.initial['file']
+        return self.initial['files']
 
 
 class VersionUpdateForm(forms.ModelForm):

extensions/migrations/0041_remove_version_file.py

@@ -0,0 +1,40 @@
+# Generated by Django 4.2.11 on 2024-06-17 14:42
+
+from django.db import migrations, models
+
+
+def populate_file(apps, schema_editor):
+    Version = apps.get_model('extensions', 'Version')
+    to_update = []
+    for v in Version.objects.all():
+        v.file = v.files.first()
+        to_update.append(v)
+    Version.objects.bulk_update(to_update, ['file'])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('extensions', '0040_version_add_files_field'),
+    ]
+
+    operations = [
+        migrations.RenameModel('VersionFiles', 'VersionFile'),
+        # this alter is only needed to make a reverse migration possible
+        migrations.AlterField(
+            model_name='version',
+            name='file',
+            field=models.OneToOneField(
+                'files.File',
+                related_name='legacy_version',
+                on_delete=models.CASCADE,
+                null=True,
+                blank=True,
+            ),
+        ),
+        migrations.RunPython(migrations.RunPython.noop, populate_file),
+        migrations.RemoveField(
+            model_name='version',
+            name='file',
+        ),
+    ]

extensions/models.py

@@ -6,6 +6,7 @@ from django.core.exceptions import ObjectDoesNotExist, BadRequest
 from django.db import models, transaction
 from django.db.models import Q, Count
 from django.urls import reverse
+from django.utils.functional import cached_property
 
 from common.fields import FilterableManyToManyField
 from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin, TrackChangesMixin
@@ -207,7 +208,7 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model):
             permissions = list(permissions.keys())
         platforms = fields.pop('platforms', [])
         tags = fields.pop('tags', [])
-        version = Version(**fields, extension=self, release_notes=release_notes, file=file)
+        version = Version(**fields, extension=self, release_notes=release_notes)
         with transaction.atomic():
             version.save()
             version.files.add(file)
@@ -432,7 +433,7 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model):
 
     @transaction.atomic
     def update_latest_version(self, skip_version=None):
-        versions = self.versions.select_related('file').order_by('-date_created')
+        versions = self.versions.prefetch_related('files').order_by('-date_created')
         latest_version = None
         for version in versions:
             if skip_version and version == skip_version:
@@ -449,7 +450,7 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model):
     def update_is_listed(self):
         should_be_listed = (
             self.status == self.STATUSES.APPROVED
-            and self.versions.filter(file__status=File.STATUSES.APPROVED).count() > 0
+            and self.versions.filter(files__status=File.STATUSES.APPROVED).count() > 0
         )
 
         # this method is called from post_save signal, this early return should prevent a loop
@@ -529,11 +530,11 @@ class Tag(CreatedModifiedMixin, models.Model):
 class VersionManager(models.Manager):
     @property
     def listed(self):
-        return self.filter(file__status=FILE_STATUS_CHOICES.APPROVED)
+        return self.filter(files__status=FILE_STATUS_CHOICES.APPROVED)
 
     @property
     def unlisted(self):
-        return self.exclude(file__status=FILE_STATUS_CHOICES.APPROVED)
+        return self.exclude(files__status=FILE_STATUS_CHOICES.APPROVED)
 
 
 class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
@@ -543,25 +544,14 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
         'permissions',
         'version',
         'licenses',
-        'file',
         'files',
         'tagline',
     }
 
-    extension = models.ForeignKey(
+    extension = models.ForeignKey(Extension, related_name='versions', on_delete=models.CASCADE)
-        'extensions.Extension', related_name='versions', on_delete=models.CASCADE
-    )
-    # TODO drop once data is migrated to `files`
-    file = models.OneToOneField(
-        'files.File',
-        related_name='legacy_version',
-        on_delete=models.CASCADE,
-        null=False,
-        blank=False,
-    )
     files = models.ManyToManyField(
         'files.File',
-        through='VersionFiles',
+        through='VersionFile',
         related_name='version',
     )
 
@@ -676,6 +666,21 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
     def __str__(self) -> str:
         return f'{self.extension} v{self.version}'
 
+    # TODO get rid of this once all places are aware of multi-file reality
+    #
+    # the fact that it is cached is important, this allows to use version.file as if it still was a
+    # model field, i.e. write things like
+    #   version.file.status = ...
+    #   version.file.save()
+    # if it isn't cached, then the save call is applied to a different, newly fetched File instance
+    @cached_property
+    def file(self):
+        files = list(self.files.all())
+        if files:
+            return files[0]
+        else:
+            return None
+
     @property
     def is_listed(self):
         # To be public, version file must have a public status.
@@ -766,7 +771,7 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
         super().delete(*args, **kwargs)
 
 
-class VersionFiles(CreatedModifiedMixin, models.Model):
+class VersionFile(CreatedModifiedMixin, models.Model):
     version = models.ForeignKey(Version, on_delete=models.CASCADE)
     file = models.OneToOneField(File, on_delete=models.CASCADE)
 

extensions/signals.py

@@ -29,35 +29,24 @@ def _log_deletion(
     instance.record_deletion()
 
 
-@receiver(post_delete, sender=extensions.models.Version)
+@receiver(post_delete, sender=extensions.models.VersionFile)
-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(post_delete, sender=extensions.models.VersionFiles)
 def _delete_versionfiles_file(
-    sender: object, instance: extensions.models.VersionFiles, **kwargs: object
+    sender: object, instance: extensions.models.VersionFile, **kwargs: object
 ) -> None:
-    # **N.B.**: this isn't part of an overloaded `VersionFiles.delete()` method because
+    # **N.B.**: this isn't part of an overloaded `VersionFile.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
 
-    # FIXME: uncomment when the signal above is cleaned up
+    file = instance.file
-    return
+    logger.info('Deleting File pk=%s of VersionFile pk=%s', file.pk, instance.pk)
-    version_file = instance.file
+    file.delete()
-    logger.info('Deleting file pk=%s of VersionFiles pk=%s', version_file.pk, instance.pk)
+
-    version_file.delete()
+    if instance.version.files.count() == 0:
+        # this was the last file, clean up the version
+        logger.info('Deleting Version pk=%s because its last file was deleted', instance.version.pk)
+        instance.version.delete()
 
 
 @receiver(pre_save, sender=extensions.models.Extension)
@@ -86,8 +75,10 @@ def _update_version(
     if raw:
         return
 
-    if hasattr(instance, 'legacy_version'):
+    version = instance.version.first()
-        extension = instance.legacy_version.extension
+    if version:
+        # TODO double-check if not just version.extension
+        extension = version.extension
         with transaction.atomic():
             # it's important to update is_listed before computing latest_version
             # because latest_version for listed and unlisted extensions are defined differently

extensions/tests/test_approve.py

@@ -26,8 +26,10 @@ class ApproveExtensionTest(TestCase):
 
         # latest_version of approved extension must be listed
         # check that we rollback latest_version when file is not approved
-        new_version.file.status = File.STATUSES.AWAITING_REVIEW
+        file = new_version.file
-        new_version.file.save()
+        file.status = File.STATUSES.AWAITING_REVIEW
+        file.save(update_fields={'status'})
+        new_version.refresh_from_db()
         self.assertFalse(new_version.is_listed)
         extension.refresh_from_db()
         self.assertEqual(first_version, extension.latest_version)

extensions/tests/test_delete.py

@@ -63,10 +63,14 @@ class DeleteTest(TestCase):
         file_validation = files.models.FileValidation.objects.create(
             file=version_file, results={'deadbeef': 'foobar'}
         )
+        # FIXME? version is listed twice, because unfortunately we generate duplicate log entry:
+        # 1. it happens due to Extension deleting its Version objects via CASCADE
+        # 2. it happens in post_delete for VersionFile when the check for the last file is done
         object_reprs = list(
             map(
                 repr,
                 [
+                    version,
                     version_file,
                     file_validation,
                     extension,
@@ -97,6 +101,9 @@ class DeleteTest(TestCase):
         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(
+            extensions.models.VersionFile.objects.filter(version__pk=version.pk).first()
+        )
         self.assertIsNone(files.models.File.objects.filter(pk=version_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())
@@ -104,7 +111,6 @@ class DeleteTest(TestCase):
 
         # Check that each of the deleted records was logged
         deletion_log_entries_q = LogEntry.objects.filter(action_flag=DELETION)
-        self.assertEqual(deletion_log_entries_q.count(), 7)
         self.assertEqual(
             [_.object_repr for _ in deletion_log_entries_q],
             object_reprs,

extensions/tests/test_manifest.py

@@ -308,7 +308,7 @@ class ValidateManifestTest(CreateFileTest):
 
         self.assertEqual(response.status_code, 302)
         file = File.objects.first()
-        extension = file.legacy_version.extension
+        extension = file.version.first().extension
         self.assertEqual(extension.slug, 'an-id')
         self.assertEqual(extension.name, 'Name. - With  Extra spaces and other characters Ж')
 

extensions/tests/test_models.py

@@ -80,8 +80,6 @@ class VersionTest(TestCase):
             metadata__website='https://example.com/',
         )
         self.assertEqual(entries_for(self.version).count(), 0)
-        # FIXME remove when dropping Version.file field
-        self.assertEqual(self.version.file, self.version.files.first())
 
     def test_admin_change_view(self):
         path = get_admin_change_path(obj=self.version)

extensions/tests/test_submit.py

@@ -154,8 +154,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.legacy_version.extension.get_draft_url())
+        self.assertEqual(response['Location'], file.version.first().extension.get_draft_url())
-        extension = file.legacy_version.extension
+        extension = file.version.first().extension
         self.assertEqual(extension.slug, slug)
         self.assertEqual(extension.name, name)
         self.assertEqual(file.original_name, file_name)
@@ -166,7 +166,7 @@ class SubmitFileTest(TestCase):
         self.assertEqual(file.metadata['version'], version_str)
         self.assertEqual(file.metadata.get('permissions'), other_metadata.get('permissions'))
         self.assertEqual(
-            [p.slug for p in file.legacy_version.platforms.all()],
+            [p.slug for p in file.version.first().platforms.all()],
             other_metadata.get('platforms', []),
         )
 
@@ -225,7 +225,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.assertEqual(response['Location'], file.legacy_version.extension.get_draft_url())
+        self.assertEqual(response['Location'], file.version.first().extension.get_draft_url())
         self.assertEqual(file.user, user)
         self.assertEqual(file.original_name, 'theme.zip')
         self.assertEqual(file.size_bytes, 5895)
@@ -297,19 +297,19 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase):
         self.version = create_version(file=self.file)
 
     def test_get_finalise_addon_redirects_if_anonymous(self):
-        response = self.client.post(self.file.legacy_version.extension.get_draft_url(), {})
+        response = self.client.post(self.file.version.first().extension.get_draft_url(), {})
 
         self.assertEqual(response.status_code, 302)
         self.assertEqual(
             response['Location'],
-            f'/oauth/login?next=/add-ons/{self.file.legacy_version.extension.slug}/draft/',
+            f'/oauth/login?next=/add-ons/{self.file.version.first().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.legacy_version.extension.get_draft_url(), {})
+        response = self.client.post(self.file.version.first().extension.get_draft_url(), {})
 
         # Technically this could (should) be a 403, but changing this means changing
         # the MaintainedExtensionMixin which is used in multiple places.
@@ -318,7 +318,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.legacy_version.extension.get_draft_url(), data)
+        response = self.client.post(self.file.version.first().extension.get_draft_url(), data)
 
         self.assertEqual(response.status_code, 200)
         self.assertDictEqual(
@@ -385,7 +385,7 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase):
                 'featured-image-source': fp4,
             }
             response = self.client.post(
-                self.file.legacy_version.extension.get_draft_url(), {**data, **files}
+                self.file.version.first().extension.get_draft_url(), {**data, **files}
             )
 
         self.assertEqual(response.status_code, 302, _get_all_form_errors(response))

extensions/views/api.py

@@ -140,7 +140,7 @@ class ExtensionsAPIView(APIView):
             'authors',
             'team',
             'versions',
-            'versions__file',
+            'versions__files',
             'versions__licenses',
             'versions__permissions',
             'versions__platforms',

extensions/views/manage.py

@@ -61,7 +61,7 @@ class ManageListView(LoginRequiredMixin, ListView):
             'ratings',
             'team',
             'versions',
-            'versions__file',
+            'versions__files',
             'versions__tags',
         )
 
@@ -244,7 +244,7 @@ class NewVersionFinalizeView(LoginRequiredMixin, OwnsFileMixin, CreateView):
 
     def get_form_kwargs(self):
         form_kwargs = super().get_form_kwargs()
-        # this lookup via VersionFiles ManyToManyManager returns the version that was created on
+        # this lookup via VersionFile ManyToManyManager returns the version that was created on
         # the previous step by create_version_from_file
         form_kwargs['instance'] = self.file.version.first()
         return form_kwargs

extensions/views/public.py

@@ -28,7 +28,7 @@ class ListedExtensionsView(ListView):
     model = Extension
     queryset = Extension.objects.listed.prefetch_related(
         'authors',
-        'latest_version__file',
+        'latest_version__files',
         'latest_version__tags',
         'preview_set',
         'preview_set__file',
@@ -226,7 +226,7 @@ class SearchView(ListedExtensionsView):
 class ExtensionDetailView(DetailView):
     queryset = Extension.objects.listed.prefetch_related(
         'authors',
-        'latest_version__file',
+        'latest_version__files',
         'latest_version__tags',
         'preview_set',
         'preview_set__file',
@@ -236,8 +236,8 @@ class ExtensionDetailView(DetailView):
         'ratings__user',
         'team',
         'versions',
-        'versions__file',
+        'versions__files',
-        'versions__file__validation',
+        'versions__files__validation',
         'versions__permissions',
         'versions__platforms',
     ).distinct()

files/admin.py

@@ -76,7 +76,7 @@ class FileAdmin(admin.ModelAdmin):
         'date_created',
         'date_modified',
         'date_status_changed',
-        ('legacy_version', admin.EmptyFieldListFilter),
+        ('version', admin.EmptyFieldListFilter),
         ('icon_of', admin.EmptyFieldListFilter),
         ('featured_image_of', admin.EmptyFieldListFilter),
         ('preview', admin.EmptyFieldListFilter),
@@ -87,7 +87,7 @@ class FileAdmin(admin.ModelAdmin):
         'date_created',
         'type',
         'status',
-        link_to('legacy_version'),
+        link_to('version', 'version of'),
         link_to('icon_of'),
         link_to('featured_image_of'),
         link_to('preview_set.extension', 'preview of'),
@@ -95,15 +95,15 @@ class FileAdmin(admin.ModelAdmin):
     )
 
     list_select_related = (
-        'legacy_version__extension',
         'user',
-        'legacy_version',
         'validation',
     )
     list_prefetch_related = (
         'icon_of',
         'featured_image_of',
         'preview_set__extension',
+        'version',
+        'version__extension',
     )
 
     autocomplete_fields = ['user']
@@ -123,8 +123,8 @@ class FileAdmin(admin.ModelAdmin):
         'content_type',
     )
     search_fields = (
-        'legacy_version__extension__slug',
+        'version__extension__slug',
-        'legacy_version__extension__name',
+        'version__extension__name',
         'extensions__slug',
         'extensions__name',
         'original_name',

reviewers/views.py

@@ -31,8 +31,8 @@ class ApprovalQueueView(ListView):
             'extension__authors',
             'extension__icon',
             'extension__versions',
-            'extension__versions__file',
+            'extension__versions__files',
-            'extension__versions__file__validation',
+            'extension__versions__files__validation',
         ).order_by('-id')
         by_extension = {}
         by_date_created = []

users/tests/test_tasks.py

@@ -34,6 +34,7 @@ class TestTasks(TestCase):
         self.report3 = AbuseReportFactory(user=user)
         self.report4 = AbuseReportFactory(user=user)
         self.authored_unlisted_extension = create_version(user=user).extension
+        self.assertEqual(self.authored_unlisted_extension.authors.first(), user)
         self.assertFalse(self.authored_unlisted_extension.is_listed)
 
     def create_account_data_that_cannot_be_deleted(self, user):