From fc40fbcdfd9a51dd52825f48c18d5e781c61bb08 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 20:15:09 +0200 Subject: [PATCH 1/6] Version: drop file field, use files instead --- extensions/admin.py | 21 ++++++---- extensions/forms.py | 8 ++-- .../migrations/0041_remove_version_file.py | 41 +++++++++++++++++++ extensions/models.py | 35 ++++++++-------- extensions/signals.py | 33 ++++++--------- extensions/tests/test_approve.py | 6 ++- extensions/tests/test_delete.py | 8 +++- extensions/tests/test_manifest.py | 2 +- extensions/tests/test_models.py | 2 - extensions/tests/test_submit.py | 18 ++++---- extensions/views/api.py | 2 +- extensions/views/manage.py | 2 +- extensions/views/public.py | 8 ++-- files/admin.py | 13 +++--- reviewers/views.py | 4 +- users/tests/test_tasks.py | 1 + 16 files changed, 125 insertions(+), 79 deletions(-) create mode 100644 extensions/migrations/0041_remove_version_file.py diff --git a/extensions/admin.py b/extensions/admin.py index 1330b2db..e6e3bad1 100644 --- a/extensions/admin.py +++ b/extensions/admin.py @@ -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') - raw_id_fields = ('file',) + fields = ('version', 'blender_version_min', 'blender_version_max') show_change_link = True extra = 0 +class VersionFilesInline(admin.TabularInline): + model = VersionFiles + 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', - 'file__user__full_name', - 'file__user__username', + 'files__user__email', + 'files__user__full_name', + '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 = (VersionFilesInline,) def get_urls(self): def wrap(view): diff --git a/extensions/forms.py b/extensions/forms.py index 1325af1d..3ed15cab 100644 --- a/extensions/forms.py +++ b/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): diff --git a/extensions/migrations/0041_remove_version_file.py b/extensions/migrations/0041_remove_version_file.py new file mode 100644 index 00000000..636242ad --- /dev/null +++ b/extensions/migrations/0041_remove_version_file.py @@ -0,0 +1,41 @@ +# 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() + if v.file is None: + print(v) + to_update.append(v) + Version.objects.bulk_update(to_update, ['file']) + + +class Migration(migrations.Migration): + + dependencies = [ + ('extensions', '0040_version_add_files_field'), + ] + + operations = [ + # 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', + ), + ] diff --git a/extensions/models.py b/extensions/models.py index 30cf670a..b9f349ad 100644 --- a/extensions/models.py +++ b/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,22 +544,11 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'permissions', '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='legacy_version', - on_delete=models.CASCADE, - null=False, - blank=False, - ) + extension = models.ForeignKey(Extension, related_name='versions', on_delete=models.CASCADE) files = models.ManyToManyField( 'files.File', through='VersionFiles', @@ -676,6 +666,17 @@ 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): + return self.files.first() + @property def is_listed(self): # To be public, version file must have a public status. diff --git a/extensions/signals.py b/extensions/signals.py index 3aa5d71a..48a9cf9f 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -29,20 +29,6 @@ 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(post_delete, sender=extensions.models.VersionFiles) def _delete_versionfiles_file( sender: object, instance: extensions.models.VersionFiles, **kwargs: object @@ -53,11 +39,14 @@ def _delete_versionfiles_file( # 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() + file = instance.file + logger.info('Deleting File pk=%s of VersionFiles pk=%s', file.pk, instance.pk) + 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'): - extension = instance.legacy_version.extension + version = instance.version.first() + 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 diff --git a/extensions/tests/test_approve.py b/extensions/tests/test_approve.py index 33a074dd..f438f316 100644 --- a/extensions/tests/test_approve.py +++ b/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 - new_version.file.save() + file = new_version.file + 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) diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 67353428..1bde9773 100644 --- a/extensions/tests/test_delete.py +++ b/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 VersionFiles 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.VersionFiles.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, diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 860be014..bb41eb85 100644 --- a/extensions/tests/test_manifest.py +++ b/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 Ж') diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index aab17672..c2f17e5e 100644 --- a/extensions/tests/test_models.py +++ b/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) diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index ad6185db..74d062ad 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.legacy_version.extension.get_draft_url()) - extension = file.legacy_version.extension + self.assertEqual(response['Location'], file.version.first().extension.get_draft_url()) + 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)) diff --git a/extensions/views/api.py b/extensions/views/api.py index 4148e24f..b6c52f48 100644 --- a/extensions/views/api.py +++ b/extensions/views/api.py @@ -140,7 +140,7 @@ class ExtensionsAPIView(APIView): 'authors', 'team', 'versions', - 'versions__file', + 'versions__files', 'versions__licenses', 'versions__permissions', 'versions__platforms', diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 5becacf9..35ff0fda 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -61,7 +61,7 @@ class ManageListView(LoginRequiredMixin, ListView): 'ratings', 'team', 'versions', - 'versions__file', + 'versions__files', 'versions__tags', ) diff --git a/extensions/views/public.py b/extensions/views/public.py index 5b4b4265..ef92cdba 100644 --- a/extensions/views/public.py +++ b/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__file__validation', + 'versions__files', + 'versions__files__validation', 'versions__permissions', 'versions__platforms', ).distinct() diff --git a/files/admin.py b/files/admin.py index 8ba35a73..ee96035c 100644 --- a/files/admin.py +++ b/files/admin.py @@ -76,7 +76,7 @@ class FileAdmin(admin.ModelAdmin): 'date_created', 'date_modified', 'date_status_changed', - ('legacy_version', admin.EmptyFieldListFilter), + 'versionfiles__version', ('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('versionfiles__version', 'version of'), link_to('icon_of'), link_to('featured_image_of'), link_to('preview_set.extension', 'preview of'), @@ -95,15 +95,16 @@ class FileAdmin(admin.ModelAdmin): ) list_select_related = ( - 'legacy_version__extension', + 'versionfiles__version__extension', 'user', - 'legacy_version', + 'versionfiles__version', 'validation', ) list_prefetch_related = ( 'icon_of', 'featured_image_of', 'preview_set__extension', + 'versionfiles__version', ) autocomplete_fields = ['user'] @@ -123,8 +124,8 @@ class FileAdmin(admin.ModelAdmin): 'content_type', ) search_fields = ( - 'legacy_version__extension__slug', - 'legacy_version__extension__name', + 'version__extension__slug', + 'version__extension__name', 'extensions__slug', 'extensions__name', 'original_name', diff --git a/reviewers/views.py b/reviewers/views.py index bb9097e4..f9ad2ef7 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -31,8 +31,8 @@ class ApprovalQueueView(ListView): 'extension__authors', 'extension__icon', 'extension__versions', - 'extension__versions__file', - 'extension__versions__file__validation', + 'extension__versions__files', + 'extension__versions__files__validation', ).order_by('-id') by_extension = {} by_date_created = [] diff --git a/users/tests/test_tasks.py b/users/tests/test_tasks.py index c923a99b..12567b39 100644 --- a/users/tests/test_tasks.py +++ b/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): -- 2.30.2 From 83c51dc51b782216b400d97a2c4967bd1a9ca848 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 20:19:43 +0200 Subject: [PATCH 2/6] use prefetched files --- extensions/models.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/extensions/models.py b/extensions/models.py index b9f349ad..285399a3 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -675,7 +675,11 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): # if it isn't cached, then the save call is applied to a different, newly fetched File instance @cached_property def file(self): - return self.files.first() + files = list(self.files.all()) + if files: + return files[0] + else: + return None @property def is_listed(self): -- 2.30.2 From 15fc36658072e6a001a9f479788b6659e23b9da8 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 20:36:01 +0200 Subject: [PATCH 3/6] remove debug print --- extensions/migrations/0041_remove_version_file.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/extensions/migrations/0041_remove_version_file.py b/extensions/migrations/0041_remove_version_file.py index 636242ad..97728305 100644 --- a/extensions/migrations/0041_remove_version_file.py +++ b/extensions/migrations/0041_remove_version_file.py @@ -8,8 +8,6 @@ def populate_file(apps, schema_editor): to_update = [] for v in Version.objects.all(): v.file = v.files.first() - if v.file is None: - print(v) to_update.append(v) Version.objects.bulk_update(to_update, ['file']) -- 2.30.2 From 29fdab5495501153324bd68d6ebc612ae2a9ea30 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 21 Jun 2024 10:44:52 +0200 Subject: [PATCH 4/6] fix file admin --- files/admin.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/files/admin.py b/files/admin.py index ee96035c..e7648a1a 100644 --- a/files/admin.py +++ b/files/admin.py @@ -76,7 +76,7 @@ class FileAdmin(admin.ModelAdmin): 'date_created', 'date_modified', 'date_status_changed', - 'versionfiles__version', + ('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('versionfiles__version', 'version of'), + link_to('version', 'version of'), link_to('icon_of'), link_to('featured_image_of'), link_to('preview_set.extension', 'preview of'), @@ -95,16 +95,15 @@ class FileAdmin(admin.ModelAdmin): ) list_select_related = ( - 'versionfiles__version__extension', 'user', - 'versionfiles__version', 'validation', ) list_prefetch_related = ( 'icon_of', 'featured_image_of', 'preview_set__extension', - 'versionfiles__version', + 'version', + 'version__extension', ) autocomplete_fields = ['user'] -- 2.30.2 From 004b60c25a97823ebe60f6e0fd400d3e072c65d4 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 21 Jun 2024 10:51:13 +0200 Subject: [PATCH 5/6] fix admin: VersionFilesInline --- extensions/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/admin.py b/extensions/admin.py index e6e3bad1..7a64a178 100644 --- a/extensions/admin.py +++ b/extensions/admin.py @@ -32,8 +32,8 @@ class VersionInline(NoAddDeleteMixin, admin.TabularInline): extra = 0 -class VersionFilesInline(admin.TabularInline): - model = VersionFiles +class VersionFilesInline(NoAddDeleteMixin, admin.TabularInline): + model = Version.files.through autocomplete_fields = ('file',) extra = 0 -- 2.30.2 From bb926d8ed6b1432592983965314798933457ef15 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 21 Jun 2024 10:58:38 +0200 Subject: [PATCH 6/6] rename VersionFiles -> VersionFile --- extensions/admin.py | 10 +++++----- extensions/migrations/0041_remove_version_file.py | 1 + extensions/models.py | 4 ++-- extensions/signals.py | 8 ++++---- extensions/tests/test_delete.py | 4 ++-- extensions/views/manage.py | 2 +- 6 files changed, 15 insertions(+), 14 deletions(-) diff --git a/extensions/admin.py b/extensions/admin.py index 7a64a178..17df006e 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, Tag, Version, VersionFiles +from extensions.models import Extension, Maintainer, Tag, Version, VersionFile log = logging.getLogger(__name__) @@ -32,7 +32,7 @@ class VersionInline(NoAddDeleteMixin, admin.TabularInline): extra = 0 -class VersionFilesInline(NoAddDeleteMixin, admin.TabularInline): +class VersionFileInline(NoAddDeleteMixin, admin.TabularInline): model = Version.files.through autocomplete_fields = ('file',) extra = 0 @@ -208,7 +208,7 @@ class VersionAdmin(admin.ModelAdmin): }, ), ) - inlines = (VersionFilesInline,) + inlines = (VersionFileInline,) def get_urls(self): def wrap(view): @@ -248,8 +248,8 @@ class TagAdmin(admin.ModelAdmin): return () -class VersionFilesAdmin(admin.ModelAdmin): - model = VersionFiles +class VersionFileAdmin(admin.ModelAdmin): + model = VersionFile list_display = 'file' readonly_fields = 'file' diff --git a/extensions/migrations/0041_remove_version_file.py b/extensions/migrations/0041_remove_version_file.py index 97728305..d5f2ed93 100644 --- a/extensions/migrations/0041_remove_version_file.py +++ b/extensions/migrations/0041_remove_version_file.py @@ -19,6 +19,7 @@ class Migration(migrations.Migration): ] operations = [ + migrations.RenameModel('VersionFiles', 'VersionFile'), # this alter is only needed to make a reverse migration possible migrations.AlterField( model_name='version', diff --git a/extensions/models.py b/extensions/models.py index 285399a3..870ae1ea 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -551,7 +551,7 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): extension = models.ForeignKey(Extension, related_name='versions', on_delete=models.CASCADE) files = models.ManyToManyField( 'files.File', - through='VersionFiles', + through='VersionFile', related_name='version', ) @@ -771,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) diff --git a/extensions/signals.py b/extensions/signals.py index 48a9cf9f..f088cfbd 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -29,18 +29,18 @@ def _log_deletion( instance.record_deletion() -@receiver(post_delete, sender=extensions.models.VersionFiles) +@receiver(post_delete, sender=extensions.models.VersionFile) 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 file = instance.file - logger.info('Deleting File pk=%s of VersionFiles pk=%s', file.pk, instance.pk) + logger.info('Deleting File pk=%s of VersionFile pk=%s', file.pk, instance.pk) file.delete() if instance.version.files.count() == 0: diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 1bde9773..fda39d5e 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -65,7 +65,7 @@ class DeleteTest(TestCase): ) # 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 VersionFiles when the check for the last file is done + # 2. it happens in post_delete for VersionFile when the check for the last file is done object_reprs = list( map( repr, @@ -102,7 +102,7 @@ class DeleteTest(TestCase): 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.VersionFiles.objects.filter(version__pk=version.pk).first() + 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()) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 35ff0fda..9862761c 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -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 -- 2.30.2