Version: drop file field, use files instead #193

Merged
Oleg-Komarov merged 6 commits from version-drop-file into main 2024-06-21 11:29:22 +02:00
16 changed files with 125 additions and 79 deletions
Showing only changes of commit fc40fbcdfd - Show all commits

View File

@ -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):

View File

@ -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):

View File

@ -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',
),
]

View File

@ -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.

View File

@ -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

View File

@ -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)

View File

@ -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,

View File

@ -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 Ж')

View File

@ -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)

View File

@ -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))

View File

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

View File

@ -61,7 +61,7 @@ class ManageListView(LoginRequiredMixin, ListView):
'ratings',
'team',
'versions',
'versions__file',
'versions__files',
'versions__tags',
)

View File

@ -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()

View File

@ -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',

View File

@ -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 = []

View File

@ -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):