From 286ed3cc03db9c5e342a45b3e20811877be63914 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 24 May 2024 20:03:01 +0200 Subject: [PATCH 01/12] Refactor dynamic property into a materialized model field --- .../0031_extension_latest_version.py | 19 +++++ extensions/models.py | 71 ++++++++++++++++--- extensions/signals.py | 71 +++---------------- extensions/tests/test_delete.py | 1 + extensions/tests/test_manifest.py | 1 + extensions/tests/test_update.py | 2 +- reviewers/models.py | 3 +- 7 files changed, 92 insertions(+), 76 deletions(-) create mode 100644 extensions/migrations/0031_extension_latest_version.py diff --git a/extensions/migrations/0031_extension_latest_version.py b/extensions/migrations/0031_extension_latest_version.py new file mode 100644 index 00000000..61696436 --- /dev/null +++ b/extensions/migrations/0031_extension_latest_version.py @@ -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'), + ), + ] diff --git a/extensions/models.py b/extensions/models.py index 15eeba63..60ab92f7 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -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) diff --git a/extensions/signals.py b/extensions/signals.py index 928d22a4..2d2473f6 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -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) diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 91fefc19..f19b2e6b 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -111,6 +111,7 @@ class DeleteTest(TestCase): 'featured_image', 'icon', 'is_listed', + 'latest_version', 'name', 'pk', 'slug', diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 4a706fb3..f48e718d 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -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) diff --git a/extensions/tests/test_update.py b/extensions/tests/test_update.py index f9065def..7dab5eea 100644 --- a/extensions/tests/test_update.py +++ b/extensions/tests/test_update.py @@ -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") def test_post_upload_validation_error_duplicate_images(self): extension = create_approved_version().extension diff --git a/reviewers/models.py b/reviewers/models.py index e943d4c4..47069b79 100644 --- a/reviewers/models.py +++ b/reviewers/models.py @@ -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', ) -- 2.30.2 From 3df8ec048ab125518f0775640ef360a397532c2e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 14:43:25 +0200 Subject: [PATCH 02/12] decouple signals, pass approve test --- .../0031_extension_latest_version.py | 4 +- extensions/models.py | 38 ++++++----- extensions/signals.py | 65 +++++++++++-------- 3 files changed, 61 insertions(+), 46 deletions(-) diff --git a/extensions/migrations/0031_extension_latest_version.py b/extensions/migrations/0031_extension_latest_version.py index 61696436..d4155ba2 100644 --- a/extensions/migrations/0031_extension_latest_version.py +++ b/extensions/migrations/0031_extension_latest_version.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.11 on 2024-05-24 17:06 +# Generated by Django 4.2.11 on 2024-05-27 12:42 from django.db import migrations, models import django.db.models.deletion @@ -14,6 +14,6 @@ class Migration(migrations.Migration): 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'), + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='latest_version_of', to='extensions.version'), ), ] diff --git a/extensions/models.py b/extensions/models.py index 60ab92f7..69a8faa0 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -175,7 +175,7 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod null=True, blank=True, on_delete=models.SET_NULL, - related_name='latest_version_for', + related_name='latest_version_of', ) featured_image = models.OneToOneField( @@ -237,10 +237,6 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod def status_slug(self) -> str: return utils.slugify(EXTENSION_STATUS_CHOICES[self.status - 1][1]) - def save(self, *args, **kwargs): - self.clean() - return super().save(*args, **kwargs) - def update_metadata_from_version(self, version): update_fields = set() metadata = version.file.metadata @@ -411,6 +407,22 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod lookup_field = 'slug' return lookup_field + @transaction.atomic + def update_latest_version(self, skip_version=None): + versions = self.versions.all().order_by('-date_created') + latest_version = None + for version in versions: + if skip_version and version == skip_version: + continue + if version.file.status not in self.valid_file_statuses: + continue + latest_version = version + break + self.latest_version = latest_version + self.save(update_fields={'latest_version'}) + if latest_version: + self.update_metadata_from_version(latest_version) + class VersionPermission(CreatedModifiedMixin, models.Model): name = models.CharField(max_length=128, null=False, blank=False, unique=True) @@ -603,7 +615,7 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model @property def is_listed(self): # To be public, version file must have a public status. - return self.file is not None and self.file.status == self.file.STATUSES.APPROVED + return self.file.status == self.file.STATUSES.APPROVED @property def cannot_be_deleted_reasons(self) -> List[str]: @@ -697,18 +709,8 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model @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) + # make sure self is not a candidate for latest_version, since it's being deleted + self.extension.update_latest_version(skip_version=self) super().delete(*args, **kwargs) diff --git a/extensions/signals.py b/extensions/signals.py index 2d2473f6..d2bfb3f3 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -59,17 +59,33 @@ def _record_changes( def extension_should_be_listed(extension): return ( - extension.latest_version is not None - and extension.latest_version.is_listed - and extension.status == extension.STATUSES.APPROVED + extension.status == extension.STATUSES.APPROVED + and extension.versions.filter(file__status=files.models.File.STATUSES.APPROVED).count() > 0 ) -@receiver(post_save, sender=extensions.models.Extension) +def update_is_listed(extension): + old_is_listed = extension.is_listed + new_is_listed = extension_should_be_listed(extension) + + if old_is_listed == new_is_listed: + return + + extension.is_listed = new_is_listed + update_fields = {'is_listed'} + if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: + extension.status = extensions.models.Extension.STATUSES.DRAFT + update_fields.add('status') + + extension.save(update_fields=update_fields) + + +# TODO? split this out into version.approve that would take care of updating file.status and +# recomputing extension.latest_version @receiver(post_save, sender=files.models.File) -def _set_is_listed( +def _update_version( sender: object, - instance: Union[extensions.models.Extension, files.models.File], + instance: files.models.File, raw: bool, *args: object, **kwargs: object, @@ -77,28 +93,25 @@ def _set_is_listed( if raw: return - if isinstance(instance, extensions.models.Extension): - extension = instance - else: - # Since signals is called very early on, we can't assume file.extension will be available. - extension = instance.extension - if not extension: + if hasattr(instance, 'version'): + extension = instance.version.extension + update_is_listed(extension) + extension.update_latest_version() + + +@receiver(post_save, sender=extensions.models.Extension) +def _set_is_listed( + sender: object, + instance: extensions.models.Extension, + raw: bool, + *args: object, + **kwargs: object, +) -> None: + if raw: 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) - - if old_is_listed == new_is_listed: - return - - if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: - extension.status = extensions.models.Extension.STATUSES.DRAFT - - logger.info('Extension pk=%s becomes listed', extension.pk) - extension.is_listed = new_is_listed - extension.save() + extension = instance + update_is_listed(extension) @receiver(post_save, sender=extensions.models.Extension) -- 2.30.2 From 16b1fa32b6bc18cbae5117be5b5a95b3bed83314 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 15:03:59 +0200 Subject: [PATCH 03/12] move method to model --- extensions/models.py | 17 +++++++++++++++++ extensions/signals.py | 28 ++-------------------------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index 69a8faa0..5055c9f6 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -423,6 +423,23 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod if latest_version: self.update_metadata_from_version(latest_version) + def update_is_listed(self): + should_be_listed = ( + self.status == self.STATUSES.APPROVED + and self.versions.filter(file__status=File.STATUSES.APPROVED).count() > 0 + ) + + if self.is_listed == should_be_listed: + return + + self.is_listed = should_be_listed + update_fields = {'is_listed'} + if self.status == self.STATUSES.APPROVED and not should_be_listed: + self.status = self.STATUSES.DRAFT + update_fields.add('status') + + self.save(update_fields=update_fields) + class VersionPermission(CreatedModifiedMixin, models.Model): name = models.CharField(max_length=128, null=False, blank=False, unique=True) diff --git a/extensions/signals.py b/extensions/signals.py index d2bfb3f3..f23be262 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -57,29 +57,6 @@ def _record_changes( instance.record_status_change(was_changed, old_state, **kwargs) -def extension_should_be_listed(extension): - return ( - extension.status == extension.STATUSES.APPROVED - and extension.versions.filter(file__status=files.models.File.STATUSES.APPROVED).count() > 0 - ) - - -def update_is_listed(extension): - old_is_listed = extension.is_listed - new_is_listed = extension_should_be_listed(extension) - - if old_is_listed == new_is_listed: - return - - extension.is_listed = new_is_listed - update_fields = {'is_listed'} - if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: - extension.status = extensions.models.Extension.STATUSES.DRAFT - update_fields.add('status') - - extension.save(update_fields=update_fields) - - # TODO? split this out into version.approve that would take care of updating file.status and # recomputing extension.latest_version @receiver(post_save, sender=files.models.File) @@ -95,7 +72,7 @@ def _update_version( if hasattr(instance, 'version'): extension = instance.version.extension - update_is_listed(extension) + extension.update_is_listed() extension.update_latest_version() @@ -110,8 +87,7 @@ def _set_is_listed( if raw: return - extension = instance - update_is_listed(extension) + instance.update_is_listed() @receiver(post_save, sender=extensions.models.Extension) -- 2.30.2 From c18e423c181cdd1ccb7cc48a5d6d949d006ca986 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 15:38:18 +0200 Subject: [PATCH 04/12] simplify version.save method --- extensions/models.py | 8 +------- extensions/signals.py | 10 ++++++---- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index 5055c9f6..967fbac9 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -715,13 +715,7 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model 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) + self.extension.update_latest_version() @transaction.atomic def delete(self, *args, **kwargs): diff --git a/extensions/signals.py b/extensions/signals.py index f23be262..f9bab5cf 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -4,6 +4,7 @@ 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 import transaction from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete from django.dispatch import receiver @@ -57,8 +58,8 @@ def _record_changes( instance.record_status_change(was_changed, old_state, **kwargs) -# TODO? split this out into version.approve that would take care of updating file.status and -# recomputing extension.latest_version +# TODO? move this out into version.approve that would take care of updating file.status and +# recomputing extension's is_listed and latest_version fields @receiver(post_save, sender=files.models.File) def _update_version( sender: object, @@ -72,8 +73,9 @@ def _update_version( if hasattr(instance, 'version'): extension = instance.version.extension - extension.update_is_listed() - extension.update_latest_version() + with transaction.atomic(): + extension.update_is_listed() + extension.update_latest_version() @receiver(post_save, sender=extensions.models.Extension) -- 2.30.2 From 047e703a3c0e45fb55aee0670eaf6da3e5ef58b5 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 16:00:01 +0200 Subject: [PATCH 05/12] migration: populate the new Extension.latest_version field --- .../0031_extension_latest_version.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/extensions/migrations/0031_extension_latest_version.py b/extensions/migrations/0031_extension_latest_version.py index d4155ba2..34f05e24 100644 --- a/extensions/migrations/0031_extension_latest_version.py +++ b/extensions/migrations/0031_extension_latest_version.py @@ -3,6 +3,34 @@ from django.db import migrations, models import django.db.models.deletion +from constants.base import ( + EXTENSION_STATUS_CHOICES, + FILE_STATUS_CHOICES, +) + + +def valid_file_statuses(self): + if self.status == EXTENSION_STATUS_CHOICES.APPROVED: + return [FILE_STATUS_CHOICES.APPROVED] + return [FILE_STATUS_CHOICES.AWAITING_REVIEW, FILE_STATUS_CHOICES.APPROVED] + + +def populate_latest_version(apps, schema_editor): + Extension = apps.get_model('extensions', 'Extension') + for extension in Extension.objects.prefetch_related( + 'versions', + 'versions__file', + ).all(): + versions = extension.versions.all().order_by('-date_created') + latest_version = None + for version in versions: + if version.file.status not in valid_file_statuses(extension): + continue + latest_version = version + break + extension.latest_version = latest_version + extension.save(update_fields={'latest_version'}) + class Migration(migrations.Migration): @@ -16,4 +44,5 @@ class Migration(migrations.Migration): name='latest_version', field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='latest_version_of', to='extensions.version'), ), + migrations.RunPython(populate_latest_version, reverse_code=migrations.RunPython.noop), ] -- 2.30.2 From 00711657afe85955c5739bf6d264cb5b4fc3cabb Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 16:14:26 +0200 Subject: [PATCH 06/12] update prefetches --- extensions/views/public.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/extensions/views/public.py b/extensions/views/public.py index 036cf6b8..478591d9 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -37,13 +37,12 @@ class HomeView(ListedExtensionsView): .get_queryset() .prefetch_related( 'authors', + 'latest_version__file', + 'latest_version__tags', 'preview_set', 'preview_set__file', 'ratings', 'team', - 'versions', - 'versions__file', - 'versions__tags', ) ) context['addons'] = q.filter(type=EXTENSION_TYPE_CHOICES.BPY)[:8] @@ -100,13 +99,13 @@ class SearchView(ListedExtensionsView): queryset = queryset.filter(search_query).distinct() return queryset.prefetch_related( 'authors', + 'latest_version__file', + 'latest_version__tags', 'preview_set', 'preview_set__file', 'ratings', 'team', 'versions', - 'versions__file', - 'versions__tags', ) def get_context_data(self, **kwargs): -- 2.30.2 From b34648a8adf1ac75242e69b50f41962d1099a7a7 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 16:21:32 +0200 Subject: [PATCH 07/12] update search by tags to use latest_version --- extensions/views/public.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/extensions/views/public.py b/extensions/views/public.py index 478591d9..f08d4f58 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -76,7 +76,9 @@ class SearchView(ListedExtensionsView): def get_queryset(self): queryset = super().get_queryset() if self.kwargs.get('tag_slug'): - queryset = queryset.filter(versions__tags__slug=self.kwargs['tag_slug']).distinct() + queryset = queryset.filter( + latest_version__tags__slug=self.kwargs['tag_slug'] + ).distinct() if self.kwargs.get('team_slug'): queryset = queryset.filter(team__slug=self.kwargs['team_slug']) if self.kwargs.get('user_id'): @@ -94,7 +96,7 @@ class SearchView(ListedExtensionsView): Q(slug__icontains=token) | Q(name__icontains=token) | Q(description__icontains=token) - | Q(versions__tags__name__icontains=token) + | Q(latest_version__tags__name__icontains=token) ) queryset = queryset.filter(search_query).distinct() return queryset.prefetch_related( -- 2.30.2 From e336a0516669671fdbc6290aceca995848f08278 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 17:00:20 +0200 Subject: [PATCH 08/12] remove forgotten prefetch --- extensions/views/public.py | 1 - 1 file changed, 1 deletion(-) diff --git a/extensions/views/public.py b/extensions/views/public.py index f08d4f58..22e5abd2 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -107,7 +107,6 @@ class SearchView(ListedExtensionsView): 'preview_set__file', 'ratings', 'team', - 'versions', ) def get_context_data(self, **kwargs): -- 2.30.2 From b42350b4e97def15eff5ae06037f713894f084be Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 17:00:36 +0200 Subject: [PATCH 09/12] more comments in parts that are still not good enough --- extensions/models.py | 1 + extensions/signals.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/extensions/models.py b/extensions/models.py index 967fbac9..f7151a86 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -429,6 +429,7 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod and self.versions.filter(file__status=File.STATUSES.APPROVED).count() > 0 ) + # this method is called from post_save signal, this early return above should prevent a loop if self.is_listed == should_be_listed: return diff --git a/extensions/signals.py b/extensions/signals.py index f9bab5cf..a25ac85b 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -74,6 +74,8 @@ def _update_version( if hasattr(instance, 'version'): extension = instance.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 extension.update_is_listed() extension.update_latest_version() -- 2.30.2 From 6f6f5d7959c28db6b2338241c5ff12c97f1fdef8 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 17:42:19 +0200 Subject: [PATCH 10/12] review fixes --- extensions/migrations/0031_extension_latest_version.py | 4 +++- extensions/models.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/extensions/migrations/0031_extension_latest_version.py b/extensions/migrations/0031_extension_latest_version.py index 34f05e24..515ad5f5 100644 --- a/extensions/migrations/0031_extension_latest_version.py +++ b/extensions/migrations/0031_extension_latest_version.py @@ -17,6 +17,7 @@ def valid_file_statuses(self): def populate_latest_version(apps, schema_editor): Extension = apps.get_model('extensions', 'Extension') + to_update = [] for extension in Extension.objects.prefetch_related( 'versions', 'versions__file', @@ -29,7 +30,8 @@ def populate_latest_version(apps, schema_editor): latest_version = version break extension.latest_version = latest_version - extension.save(update_fields={'latest_version'}) + to_update.append(extension) + Extension.objects.bulk_update(to_update, ['latest_version']) class Migration(migrations.Migration): diff --git a/extensions/models.py b/extensions/models.py index f7151a86..cdb67468 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -409,7 +409,7 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod @transaction.atomic def update_latest_version(self, skip_version=None): - versions = self.versions.all().order_by('-date_created') + versions = self.versions.select_related('file').all().order_by('-date_created') latest_version = None for version in versions: if skip_version and version == skip_version: @@ -429,7 +429,7 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod and self.versions.filter(file__status=File.STATUSES.APPROVED).count() > 0 ) - # this method is called from post_save signal, this early return above should prevent a loop + # this method is called from post_save signal, this early return should prevent a loop if self.is_listed == should_be_listed: return -- 2.30.2 From ff56a381a66f394e748adb44a36060c6c81cb048 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 17:52:50 +0200 Subject: [PATCH 11/12] add latest_version in admin --- extensions/admin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions/admin.py b/extensions/admin.py index 4b3bfbc0..c572114a 100644 --- a/extensions/admin.py +++ b/extensions/admin.py @@ -83,6 +83,7 @@ class ExtensionAdmin(admin.ModelAdmin): 'website', 'icon', 'featured_image', + 'latest_version', ) autocomplete_fields = ('team',) @@ -104,6 +105,7 @@ class ExtensionAdmin(admin.ModelAdmin): 'description', ('icon', 'featured_image'), 'status', + 'latest_version', ), }, ), -- 2.30.2 From bbb7c9fc85d0d425fc69207cf73e21c9e039d153 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 27 May 2024 17:57:06 +0200 Subject: [PATCH 12/12] remove unnecessary .all() --- extensions/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/models.py b/extensions/models.py index cdb67468..bc85fdf7 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -409,7 +409,7 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod @transaction.atomic def update_latest_version(self, skip_version=None): - versions = self.versions.select_related('file').all().order_by('-date_created') + versions = self.versions.select_related('file').order_by('-date_created') latest_version = None for version in versions: if skip_version and version == skip_version: -- 2.30.2