From e16085cb8abd07406a0cb2a508092bc9e950b4f9 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 17 Jun 2024 13:21:26 +0200 Subject: [PATCH] Multi-platform support: add Version.files field This is step 1 of moving from Version.file to Version.files: 1. start double-writing into Version.files + update existing records 2. change all reads from Version.file to Version.files, expecting a single file, then drop Version.file 3. implement support for multiple uploads per Version File.version is renamed to File.legacy_version to simplify further cleanup in step 2. --- common/tests/factories/extensions.py | 12 +++++ extensions/admin.py | 8 +++- .../0040_version_add_files_field.py | 45 +++++++++++++++++++ extensions/models.py | 20 ++++++++- extensions/signals.py | 21 ++++++++- extensions/tests/test_delete.py | 2 +- extensions/tests/test_manifest.py | 2 +- extensions/tests/test_models.py | 2 + extensions/tests/test_submit.py | 19 ++++---- files/admin.py | 12 ++--- 10 files changed, 122 insertions(+), 21 deletions(-) create mode 100644 extensions/migrations/0040_version_add_files_field.py diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 35a095ba..7dd0203c 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -84,6 +84,18 @@ class VersionFactory(DjangoModelFactory): RatingFactory, size=lambda: random.randint(0, 5), factory_related_name='version' ) + @factory.post_generation + def files(self, create, extracted, **kwargs): + if not create: + return + + if not extracted: + self.files.add(self.file) + return + + for file in extracted: + self.files.add(file) + @factory.post_generation def platforms(self, create, extracted, **kwargs): if not create: diff --git a/extensions/admin.py b/extensions/admin.py index 2e025e01..9dbed1d0 100644 --- a/extensions/admin.py +++ b/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, Version, Tag +from extensions.models import Extension, Maintainer, Tag, Version, VersionFiles log = logging.getLogger(__name__) @@ -241,6 +241,12 @@ class TagAdmin(admin.ModelAdmin): return () +class VersionFilesAdmin(admin.ModelAdmin): + model = VersionFiles + list_display = 'file' + readonly_fields = 'file' + + class VersionPermissionAdmin(admin.ModelAdmin): list_display = ('name', 'slug') diff --git a/extensions/migrations/0040_version_add_files_field.py b/extensions/migrations/0040_version_add_files_field.py new file mode 100644 index 00000000..15511e14 --- /dev/null +++ b/extensions/migrations/0040_version_add_files_field.py @@ -0,0 +1,45 @@ +# Generated by Django 4.2.11 on 2024-06-17 10:13 + +from django.db import migrations, models +import django.db.models.deletion + + +def populate_files(apps, schema_editor): + Version = apps.get_model('extensions', 'Version') + for v in Version.objects.all(): + v.files.add(v.file) + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0010_remove_file_extension_alter_file_hash_and_more'), + ('extensions', '0039_alter_extension_status'), + ] + + operations = [ + migrations.AlterField( + model_name='version', + name='file', + field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='legacy_version', to='files.file'), + ), + migrations.CreateModel( + name='VersionFiles', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('date_created', models.DateTimeField(auto_now_add=True)), + ('date_modified', models.DateTimeField(auto_now=True)), + ('file', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='files.file')), + ('version', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='extensions.version')), + ], + options={ + 'abstract': False, + }, + ), + migrations.AddField( + model_name='version', + name='files', + field=models.ManyToManyField(related_name='version', through='extensions.VersionFiles', to='files.file'), + ), + migrations.RunPython(populate_files, migrations.RunPython.noop) + ] diff --git a/extensions/models.py b/extensions/models.py index b43367bc..e76d036e 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -496,10 +496,12 @@ class VersionManager(models.Manager): permissions = list(permissions.keys()) platforms = kwargs.pop('platforms', []) tags = kwargs.pop('tags', []) + file = kwargs.get('file') # FIXME legacy_version: replace get with pop version, result = super().update_or_create(*args, **kwargs) # Add the ManyToMany to the already initialized Version + version.files.add(file) version.set_initial_licenses(licenses) version.set_initial_permissions(permissions) version.set_initial_platforms(platforms) @@ -515,14 +517,25 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'version', 'licenses', 'file', + 'files', 'tagline', } extension = models.ForeignKey( 'extensions.Extension', related_name='versions', on_delete=models.CASCADE ) + # TODO drop once data is migrated to `files` file = models.OneToOneField( - 'files.File', related_name='version', on_delete=models.CASCADE, null=False, blank=False + 'files.File', + related_name='legacy_version', + on_delete=models.CASCADE, + null=False, + blank=False, + ) + files = models.ManyToManyField( + 'files.File', + through='VersionFiles', + related_name='version', ) version = extensions.fields.VersionStringField( @@ -746,6 +759,11 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): super().delete(*args, **kwargs) +class VersionFiles(CreatedModifiedMixin, models.Model): + version = models.ForeignKey(Version, on_delete=models.CASCADE) + file = models.OneToOneField(File, on_delete=models.CASCADE) + + class Maintainer(CreatedModifiedMixin, models.Model): extension = models.ForeignKey(Extension, on_delete=models.CASCADE) user = models.ForeignKey(User, on_delete=models.CASCADE) diff --git a/extensions/signals.py b/extensions/signals.py index cb94f041..3aa5d71a 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -43,6 +43,23 @@ def _delete_version_file( version_file.delete() +@receiver(post_delete, sender=extensions.models.VersionFiles) +def _delete_versionfiles_file( + sender: object, instance: extensions.models.VersionFiles, **kwargs: object +) -> None: + # **N.B.**: this isn't part of an overloaded `VersionFiles.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 + return + version_file = instance.file + logger.info('Deleting file pk=%s of VersionFiles pk=%s', version_file.pk, instance.pk) + version_file.delete() + + @receiver(pre_save, sender=extensions.models.Extension) def _record_changes( sender: object, @@ -69,8 +86,8 @@ def _update_version( if raw: return - if hasattr(instance, 'version'): - extension = instance.version.extension + if hasattr(instance, 'legacy_version'): + extension = instance.legacy_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 diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 6506291e..51e1670d 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -73,10 +73,10 @@ class DeleteTest(TestCase): version_file, file_validation, extension, + version, approval_activity, preview_file.preview_set.first(), reused_image.preview_set.first(), - version, ], ) ) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index f8905b52..bb3177f0 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -330,7 +330,7 @@ class ValidateManifestTest(CreateFileTest): self.assertEqual(response.status_code, 302) file = File.objects.first() - extension = file.version.extension + extension = file.legacy_version.extension self.assertEqual(extension.slug, 'an-id') self.assertEqual(extension.name, 'Name. - With Extra spaces and other characters Ж') diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index daf0d0ed..d3ab76ca 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -98,6 +98,8 @@ class VersionTest(TestCase): extension__support='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 _check_change_message(self): entries = entries_for(self.version) diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 3520a4df..7ec75a6b 100644 --- a/extensions/tests/test_submit.py +++ b/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.version.extension.get_draft_url()) - extension = file.version.extension + self.assertEqual(response['Location'], file.legacy_version.extension.get_draft_url()) + extension = file.legacy_version.extension self.assertEqual(extension.slug, slug) self.assertEqual(extension.name, name) self.assertEqual(file.original_name, file_name) @@ -166,7 +166,8 @@ 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.version.platforms.all()], other_metadata.get('platforms', []) + [p.slug for p in file.legacy_version.platforms.all()], + other_metadata.get('platforms', []), ) def test_not_allowed_anonymous(self): @@ -224,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.version.extension.get_draft_url()) + self.assertEqual(response['Location'], file.legacy_version.extension.get_draft_url()) self.assertEqual(file.user, user) self.assertEqual(file.original_name, 'theme.zip') self.assertEqual(file.size_bytes, 5895) @@ -305,19 +306,19 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): ) def test_get_finalise_addon_redirects_if_anonymous(self): - response = self.client.post(self.file.version.extension.get_draft_url(), {}) + response = self.client.post(self.file.legacy_version.extension.get_draft_url(), {}) self.assertEqual(response.status_code, 302) self.assertEqual( response['Location'], - f'/oauth/login?next=/add-ons/{self.file.version.extension.slug}/draft/', + f'/oauth/login?next=/add-ons/{self.file.legacy_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.version.extension.get_draft_url(), {}) + response = self.client.post(self.file.legacy_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. @@ -326,7 +327,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.version.extension.get_draft_url(), data) + response = self.client.post(self.file.legacy_version.extension.get_draft_url(), data) self.assertEqual(response.status_code, 200) self.assertDictEqual( @@ -393,7 +394,7 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): 'featured-image-source': fp4, } response = self.client.post( - self.file.version.extension.get_draft_url(), {**data, **files} + self.file.legacy_version.extension.get_draft_url(), {**data, **files} ) self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) diff --git a/files/admin.py b/files/admin.py index 47491858..8ba35a73 100644 --- a/files/admin.py +++ b/files/admin.py @@ -76,7 +76,7 @@ class FileAdmin(admin.ModelAdmin): 'date_created', 'date_modified', 'date_status_changed', - ('version', admin.EmptyFieldListFilter), + ('legacy_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('version'), + link_to('legacy_version'), link_to('icon_of'), link_to('featured_image_of'), link_to('preview_set.extension', 'preview of'), @@ -95,9 +95,9 @@ class FileAdmin(admin.ModelAdmin): ) list_select_related = ( - 'version__extension', + 'legacy_version__extension', 'user', - 'version', + 'legacy_version', 'validation', ) list_prefetch_related = ( @@ -123,8 +123,8 @@ class FileAdmin(admin.ModelAdmin): 'content_type', ) search_fields = ( - 'version__extension__slug', - 'version__extension__name', + 'legacy_version__extension__slug', + 'legacy_version__extension__name', 'extensions__slug', 'extensions__name', 'original_name', -- 2.30.2