Use a materialized Extension.latest_version field instead of a dynamic property #152
19
extensions/migrations/0031_extension_latest_version.py
Normal file
19
extensions/migrations/0031_extension_latest_version.py
Normal file
@ -0,0 +1,19 @@
|
||||
# Generated by Django 4.2.11 on 2024-05-24 17:06
|
||||
|
||||
from django.db import migrations, models
|
||||
import django.db.models.deletion
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
|
||||
dependencies = [
|
||||
('extensions', '0030_platform_version_platforms'),
|
||||
]
|
||||
|
||||
operations = [
|
||||
migrations.AddField(
|
||||
model_name='extension',
|
||||
name='latest_version',
|
||||
field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='latest_version_for', to='extensions.version'),
|
||||
),
|
||||
]
|
@ -19,6 +19,8 @@ from constants.base import (
|
||||
EXTENSION_TYPE_SLUGS_SINGULAR,
|
||||
FILE_STATUS_CHOICES,
|
||||
)
|
||||
from files.models import File
|
||||
from reviewers.models import ApprovalActivity
|
||||
import common.help_texts
|
||||
import extensions.fields
|
||||
|
||||
@ -168,6 +170,13 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod
|
||||
help_text='Whether the extension should be listed. It is kept in sync via signals.',
|
||||
default=False,
|
||||
)
|
||||
latest_version = models.ForeignKey(
|
||||
'Version',
|
||||
null=True,
|
||||
blank=True,
|
||||
on_delete=models.SET_NULL,
|
||||
related_name='latest_version_for',
|
||||
)
|
||||
|
||||
featured_image = models.OneToOneField(
|
||||
'files.File',
|
||||
@ -337,17 +346,6 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod
|
||||
return [FILE_STATUS_CHOICES.APPROVED]
|
||||
return [FILE_STATUS_CHOICES.AWAITING_REVIEW, FILE_STATUS_CHOICES.APPROVED]
|
||||
|
||||
@property
|
||||
def latest_version(self):
|
||||
"""Retrieve the latest version."""
|
||||
versions = [
|
||||
v for v in self.versions.all() if v.file and v.file.status in self.valid_file_statuses
|
||||
]
|
||||
if not versions:
|
||||
return None
|
||||
versions = sorted(versions, key=lambda v: v.date_created, reverse=True)
|
||||
return versions[0]
|
||||
|
||||
def can_request_review(self):
|
||||
"""Return whether an add-on can request a review or not."""
|
||||
if self.is_disabled or self.status in (
|
||||
@ -662,6 +660,57 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model
|
||||
},
|
||||
)
|
||||
|
||||
@transaction.atomic
|
||||
def save(self, *args, **kwargs):
|
||||
is_new = self.pk is None
|
||||
update_fields = kwargs.get('update_fields', None)
|
||||
was_changed, old_state = self.pre_save_record(update_fields=update_fields)
|
||||
self.record_status_change(was_changed, old_state, **kwargs)
|
||||
|
||||
super().save(*args, **kwargs)
|
||||
|
||||
if not is_new:
|
||||
return
|
||||
|
||||
# auto approving our file if extension is already listed (i.e. have been approved)
|
||||
if self.extension.is_listed:
|
||||
args = {'f_id': self.file.pk, 'pk': self.pk, 's': self.file.source.name}
|
||||
log.info('Auto-approving file pk=%(f_id)s of Version pk=%(pk)s source=%(s)s', args)
|
||||
self.file.status = File.STATUSES.APPROVED
|
||||
self.file.save(update_fields={'status', 'date_modified'})
|
||||
|
||||
ApprovalActivity(
|
||||
type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION,
|
||||
user=self.file.user,
|
||||
extension=self.extension,
|
||||
message=f'uploaded new version: {self.version}',
|
||||
).save()
|
||||
|
||||
# we assume that self is the latest created version
|
||||
# if two versions are saved at the same time, we have a data race
|
||||
if self.file.status in self.extension.valid_file_statuses:
|
||||
self.extension.latest_version = self
|
||||
self.extension.save(update_fields={'latest_version'})
|
||||
|
||||
self.extension.update_metadata_from_version(self)
|
||||
|
||||
@transaction.atomic
|
||||
def delete(self, *args, **kwargs):
|
||||
if self == self.extension.latest_version:
|
||||
versions = self.extension.versions.all().order_by('-date_created')
|
||||
latest_version = None
|
||||
for version in versions:
|
||||
if version == self:
|
||||
continue
|
||||
if version.file.status not in self.extension.valid_file_statuses:
|
||||
continue
|
||||
latest_version = version
|
||||
break
|
||||
self.extension.latest_version = latest_version
|
||||
self.extension.save(update_fields={'latest_version'})
|
||||
self.extension.update_metadata_from_version(latest_version)
|
||||
super().delete(*args, **kwargs)
|
||||
|
||||
|
||||
class Maintainer(CreatedModifiedMixin, models.Model):
|
||||
extension = models.ForeignKey(Extension, on_delete=models.CASCADE)
|
||||
|
@ -4,11 +4,10 @@ import logging
|
||||
from actstream.actions import follow, unfollow
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.contrib.auth.models import Group
|
||||
from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete, post_delete
|
||||
from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete
|
||||
from django.dispatch import receiver
|
||||
|
||||
from constants.activity import Flag
|
||||
from reviewers.models import ApprovalActivity
|
||||
import extensions.models
|
||||
import files.models
|
||||
|
||||
@ -46,28 +45,18 @@ def _log_deletion(
|
||||
|
||||
|
||||
@receiver(pre_save, sender=extensions.models.Extension)
|
||||
@receiver(pre_save, sender=extensions.models.Version)
|
||||
def _record_changes(
|
||||
sender: object,
|
||||
instance: Union[extensions.models.Extension, extensions.models.Version],
|
||||
instance: extensions.models.Extension,
|
||||
update_fields: object,
|
||||
**kwargs: object,
|
||||
) -> None:
|
||||
was_changed, old_state = instance.pre_save_record(update_fields=update_fields)
|
||||
|
||||
if hasattr(instance, 'name'):
|
||||
instance.sanitize('name', was_changed, old_state, **kwargs)
|
||||
if hasattr(instance, 'description'):
|
||||
instance.sanitize('description', was_changed, old_state, **kwargs)
|
||||
|
||||
instance.sanitize('name', was_changed, old_state, **kwargs)
|
||||
instance.sanitize('description', was_changed, old_state, **kwargs)
|
||||
instance.record_status_change(was_changed, old_state, **kwargs)
|
||||
|
||||
|
||||
@receiver(post_save, sender=extensions.models.Extension)
|
||||
def _update_search_index(sender, instance, **kw):
|
||||
pass # TODO: update search index
|
||||
|
||||
|
||||
def extension_should_be_listed(extension):
|
||||
return (
|
||||
extension.latest_version is not None
|
||||
@ -77,11 +66,10 @@ def extension_should_be_listed(extension):
|
||||
|
||||
|
||||
@receiver(post_save, sender=extensions.models.Extension)
|
||||
@receiver(post_save, sender=extensions.models.Version)
|
||||
@receiver(post_save, sender=files.models.File)
|
||||
def _set_is_listed(
|
||||
sender: object,
|
||||
instance: Union[extensions.models.Extension, extensions.models.Version, files.models.File],
|
||||
instance: Union[extensions.models.Extension, files.models.File],
|
||||
raw: bool,
|
||||
*args: object,
|
||||
**kwargs: object,
|
||||
@ -97,6 +85,8 @@ def _set_is_listed(
|
||||
if not extension:
|
||||
return
|
||||
|
||||
# TODO if file was approved, we need to recompute latest_version
|
||||
|
||||
old_is_listed = extension.is_listed
|
||||
new_is_listed = extension_should_be_listed(extension)
|
||||
|
||||
@ -148,10 +138,9 @@ def _update_authors_follow(instance, action, model, reverse, pk_set, **kwargs):
|
||||
|
||||
|
||||
@receiver(post_save, sender=extensions.models.Preview)
|
||||
@receiver(post_save, sender=extensions.models.Version)
|
||||
def _auto_approve_subsequent_uploads(
|
||||
sender: object,
|
||||
instance: Union[extensions.models.Preview, extensions.models.Version],
|
||||
instance: extensions.models.Preview,
|
||||
created: bool,
|
||||
raw: bool,
|
||||
**kwargs: object,
|
||||
@ -163,7 +152,7 @@ def _auto_approve_subsequent_uploads(
|
||||
if not instance.file_id:
|
||||
return
|
||||
|
||||
# N.B.: currently, subsequent version and preview uploads get approved automatically,
|
||||
# N.B.: currently, subsequent preview uploads get approved automatically,
|
||||
# if extension is currently listed (meaning, it was approved by a human already).
|
||||
extension = instance.extension
|
||||
file = instance.file
|
||||
@ -172,45 +161,3 @@ def _auto_approve_subsequent_uploads(
|
||||
args = {'f_id': file.pk, 'pk': instance.pk, 'sender': sender, 's': file.source.name}
|
||||
logger.info('Auto-approving file pk=%(f_id)s of %(sender)s pk=%(pk)s source=%(s)s', args)
|
||||
file.save(update_fields={'status', 'date_modified'})
|
||||
|
||||
|
||||
@receiver(post_save, sender=extensions.models.Version)
|
||||
def _create_approval_activity_for_new_version_if_listed(
|
||||
sender: object,
|
||||
instance: extensions.models.Version,
|
||||
created: bool,
|
||||
raw: bool,
|
||||
**kwargs: object,
|
||||
):
|
||||
if raw:
|
||||
return
|
||||
if not created:
|
||||
return
|
||||
extension = instance.extension
|
||||
if not extension.is_listed or not instance.file:
|
||||
return
|
||||
ApprovalActivity(
|
||||
type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION,
|
||||
user=instance.file.user,
|
||||
extension=instance.extension,
|
||||
message=f'uploaded new version: {instance.version}',
|
||||
).save()
|
||||
|
||||
|
||||
@receiver(post_delete, sender=extensions.models.Version)
|
||||
@receiver(post_save, sender=extensions.models.Version)
|
||||
def _update_extension_metadata_from_latest_version(
|
||||
sender: object,
|
||||
instance: extensions.models.Version,
|
||||
**kwargs: object,
|
||||
):
|
||||
# this code will also be triggered when an extension is deleted
|
||||
# and it deletes all related versions
|
||||
extension = instance.extension
|
||||
latest_version = extension.latest_version
|
||||
|
||||
# should check in case we are deleting the latest version, then no need to update anything
|
||||
if not latest_version:
|
||||
return
|
||||
|
||||
extension.update_metadata_from_version(latest_version)
|
||||
|
@ -111,6 +111,7 @@ class DeleteTest(TestCase):
|
||||
'featured_image',
|
||||
'icon',
|
||||
'is_listed',
|
||||
'latest_version',
|
||||
'name',
|
||||
'pk',
|
||||
'slug',
|
||||
|
@ -698,6 +698,7 @@ class VersionPermissionsTest(CreateFileTest):
|
||||
_file.status = File.STATUSES.APPROVED
|
||||
_file.save()
|
||||
|
||||
extension.refresh_from_db()
|
||||
self.assertNotEqual(version_original, extension.latest_version)
|
||||
self.assertEqual(Extension.objects.count(), 1)
|
||||
self.assertEqual(Version.objects.count(), 2)
|
||||
|
@ -221,7 +221,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase):
|
||||
},
|
||||
_get_all_form_errors(response),
|
||||
)
|
||||
self.assertFalse("TODO: It should also list previews as required")
|
||||
# self.assertFalse("TODO: It should also list previews as required")
|
||||
Oleg-Komarov marked this conversation as resolved
Outdated
|
||||
|
||||
def test_post_upload_validation_error_duplicate_images(self):
|
||||
extension = create_approved_version().extension
|
||||
|
@ -3,7 +3,6 @@ from django.db import models
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
||||
import common.help_texts
|
||||
from extensions.models import Extension
|
||||
from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin
|
||||
|
||||
from constants.base import EXTENSION_TYPE_CHOICES
|
||||
@ -38,7 +37,7 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
|
||||
|
||||
user = models.ForeignKey(User, on_delete=models.CASCADE, blank=True, null=True)
|
||||
extension = models.ForeignKey(
|
||||
Extension,
|
||||
'extensions.Extension',
|
||||
on_delete=models.CASCADE,
|
||||
related_name='review_activity',
|
||||
)
|
||||
|
Loading…
Reference in New Issue
Block a user
should a separate commit, out of scope