From 5c7a4d1703d440c4a90220c2e398a8dd3004e27e Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 15 Apr 2024 19:21:02 +0200 Subject: [PATCH 01/18] Make possible fully deleting unrated/unlisted extensions --- extensions/models.py | 41 ++++++++++++++++++- .../templates/extensions/draft_finalise.html | 5 +++ extensions/tests/test_delete.py | 41 ++++++++++++++++++- 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index 8919a424..6a45e4f6 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -236,11 +236,32 @@ class Extension( self.status = self.STATUSES.APPROVED self.save() + @property + def cannot_be_deleted_reasons(self) -> List[str]: + """Return a list of reasons why this extension cannot be fully deleted.""" + reasons = [] + if self.ratings.count() > 0: + reasons.append('has_ratings') + if self.is_listed: + reasons.append('is_listed') + return reasons + @transaction.atomic - def delete(self, hard=False): - versions = self.versions.filter(date_deleted__isnull=True) + def delete(self): + hard = self.cannot_be_deleted_reasons == [] + versions = self.versions.all() + previews = self.previews.all() + # When soft-deleting, filter out already soft-deleted versions and previews + if not hard: + previews = self.previews.filter(date_deleted__isnull=True) + versions = self.versions.filter(date_deleted__isnull=True) + mode = not hard and 'soft-' or '' + for p in previews: + p.delete(hard=hard) + log.warning('%(mode)sdeleting preview file pk=% source=%s', mode, p.pk, p.source.name) for v in versions: v.delete(hard=hard) + log.warning('%(mode)sdeleting extension pk=%s%sdeleted', mode, self.pk) super().delete(hard=hard) def get_absolute_url(self): @@ -542,6 +563,22 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMi def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + def delete(self, hard=False): + file = self.file + mode = not hard and 'soft-' or '' + args = { + 'extension_id': self.extension_id, + 'file_id': file.pk, + 'mode': mode, + 'source': file.source.name, + 'version_id': self.pk, + 'version': self.version, + } + log.warning('%(mode)sdeleting file pk=%(file_id)s source=%(source)s', args) + file.delete(hard=hard) + log.warning('%(mode)sdeleting version pk=%(version_id)s "%(version)s"', args) + super().delete(hard=hard) + def set_initial_permissions(self, _permissions): if not _permissions: return diff --git a/extensions/templates/extensions/draft_finalise.html b/extensions/templates/extensions/draft_finalise.html index 2a1a9928..27d21d4f 100644 --- a/extensions/templates/extensions/draft_finalise.html +++ b/extensions/templates/extensions/draft_finalise.html @@ -90,6 +90,11 @@ {% trans 'Submit for Approval' %} + + + + {% trans 'Delete Extension' %} + diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 093b2c72..11708f14 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,7 +1,10 @@ from django.test import TestCase -from common.tests.factories.extensions import create_approved_version +from common.tests.factories.extensions import create_approved_version, create_version +from common.tests.factories.files import FileFactory from common.tests.factories.users import UserFactory +import extensions.models +import files.models class DeleteTest(TestCase): @@ -32,3 +35,39 @@ class DeleteTest(TestCase): extension.refresh_from_db() self.assertIsNone(extension.date_deleted) self.assertTrue(all(v.date_deleted is None for v in extension.versions.all())) + + def test_incomplete_unrated_extension_can_be_fully_deleted_by_author(self): + version = create_version( + ratings=[], + extension__previews=[ + FileFactory( + type=files.models.File.TYPES.IMAGE, + source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', + ) + ], + ) + extension = version.extension + version_file = version.file + self.assertEqual(extension.status, extension.STATUSES.INCOMPLETE) + preview_file = extension.previews.first() + self.assertIsNotNone(preview_file) + + url = extension.get_delete_url() + user = extension.authors.first() + self.client.force_login(user) + response = self.client.post(url) + + self.assertEqual(response.status_code, 302) + # All relevant records should have been deleted + with self.assertRaises(extensions.models.Extension.DoesNotExist): + extension.refresh_from_db() + with self.assertRaises(extensions.models.Version.DoesNotExist): + version.refresh_from_db() + with self.assertRaises(files.models.File.DoesNotExist): + version_file.refresh_from_db() + with self.assertRaises(files.models.File.DoesNotExist): + preview_file.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(files.models.File.objects.filter(pk=version_file.pk).first()) + self.assertIsNone(files.models.File.objects.filter(pk=preview_file.pk).first()) -- 2.30.2 From 783af257149b6cc0632b4c185f3c7bd8cc97af1c Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 15 Apr 2024 19:22:24 +0200 Subject: [PATCH 02/18] Typo --- extensions/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/models.py b/extensions/models.py index 6a45e4f6..af4be532 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -261,7 +261,7 @@ class Extension( log.warning('%(mode)sdeleting preview file pk=% source=%s', mode, p.pk, p.source.name) for v in versions: v.delete(hard=hard) - log.warning('%(mode)sdeleting extension pk=%s%sdeleted', mode, self.pk) + log.warning('%(mode)sdeleting extension pk=%s', mode, self.pk) super().delete(hard=hard) def get_absolute_url(self): -- 2.30.2 From 0c3fbbc9801a5f5c989af700c4e447471cdcc43f Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Mon, 15 Apr 2024 19:24:25 +0200 Subject: [PATCH 03/18] Another typo --- extensions/models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index af4be532..d7c2f512 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -258,7 +258,7 @@ class Extension( mode = not hard and 'soft-' or '' for p in previews: p.delete(hard=hard) - log.warning('%(mode)sdeleting preview file pk=% source=%s', mode, p.pk, p.source.name) + log.warning('%(mode)sdeleting preview file pk=%s source=%s', mode, p.pk, p.source.name) for v in versions: v.delete(hard=hard) log.warning('%(mode)sdeleting extension pk=%s', mode, self.pk) @@ -567,7 +567,6 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMi file = self.file mode = not hard and 'soft-' or '' args = { - 'extension_id': self.extension_id, 'file_id': file.pk, 'mode': mode, 'source': file.source.name, -- 2.30.2 From 7949be8e2ec25c248e2427222ccda162dd0b0175 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Tue, 16 Apr 2024 20:03:20 +0200 Subject: [PATCH 04/18] Remove soft-deletion from everywhere; check for possibility of deletion instead --- abuse/models.py | 4 +- blender_extensions/settings.py | 7 +- common/model_mixins.py | 42 +----- extensions/admin.py | 9 +- extensions/forms.py | 3 - extensions/models.py | 129 ++++++------------ extensions/signals.py | 32 ++++- .../templates/extensions/version_list.html | 2 +- extensions/tests/test_delete.py | 101 ++++++++++---- extensions/tests/test_models.py | 1 - extensions/tests/test_views.py | 27 ---- extensions/views/manage.py | 9 +- extensions/views/mixins.py | 4 +- extensions/views/public.py | 3 - files/admin.py | 3 - files/models.py | 9 +- files/signals.py | 7 +- files/tests/test_models.py | 1 - ratings/admin.py | 30 ++-- ratings/models.py | 39 +----- ratings/tests/test_views.py | 1 - reviewers/tests/test_views.py | 2 - reviewers/views.py | 6 +- 23 files changed, 187 insertions(+), 284 deletions(-) diff --git a/abuse/models.py b/abuse/models.py index 634ef2eb..762e14ca 100644 --- a/abuse/models.py +++ b/abuse/models.py @@ -9,13 +9,13 @@ from extended_choices import Choices from geoip2.errors import GeoIP2Error from constants.base import ABUSE_TYPE, ABUSE_TYPE_EXTENSION, ABUSE_TYPE_REVIEW -from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin +from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin import extensions.fields User = get_user_model() -class AbuseReport(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Model): +class AbuseReport(CreatedModifiedMixin, TrackChangesMixin, models.Model): TYPE = ABUSE_TYPE REASONS = Choices( diff --git a/blender_extensions/settings.py b/blender_extensions/settings.py index 4c75234f..5e62b24f 100644 --- a/blender_extensions/settings.py +++ b/blender_extensions/settings.py @@ -1,5 +1,4 @@ -""" -Django settings for blender_extensions project. +"""Django settings for blender_extensions project. Generated by 'django-admin startproject' using Django 4.0.6. @@ -192,9 +191,9 @@ LOGGING = { }, }, 'loggers': { - 'django': {'level': 'WARNING'}, + 'django': {'level': 'INFO'}, }, - 'root': {'level': 'WARNING', 'handlers': ['console']}, + 'root': {'level': 'INFO', 'handlers': ['console']}, } PIPELINE = { diff --git a/common/model_mixins.py b/common/model_mixins.py index 9d74d8c6..1cb6efa0 100644 --- a/common/model_mixins.py +++ b/common/model_mixins.py @@ -3,7 +3,7 @@ import copy import logging from django.contrib.contenttypes.models import ContentType -from django.db import models, transaction +from django.db import models from django.shortcuts import reverse from django.utils import timezone @@ -147,43 +147,3 @@ class TrackChangesMixin(models.Model): self.name = markdown.sanitize(self.name) if update_fields is not None: kwargs['update_fields'] = kwargs['update_fields'].union({'name'}) - - -class SoftDeleteMixin(models.Model): - """Model with soft-deletion functionality.""" - - class Meta: - abstract = True - - date_deleted = models.DateTimeField(null=True, blank=True, editable=False) - - @property - def is_deleted(self) -> bool: - return self.date_deleted is not None - - @transaction.atomic - def delete(self, hard=False): - if hard: - super().delete() - else: - if hasattr(self, 'file'): - # TODO(oleg) move to an archived directory, add random suffix - self.file.delete() - self.date_deleted = timezone.now() - self.save() - - logger.info('%r pk=%r deleted', self.__class__, self.pk) - - def delete_queryset(self, request, queryset): - """Given a queryset, soft-delete it from the database.""" - queryset.update(date_deleted=timezone.now()) - - def undelete(self, save=True): - if not self.date_deleted: - logger.warning('%r pk=%r is not deleted, cannot undelete', self.__class__, self.pk) - return - self.date_deleted = None - if save: - self.save() - - logger.info('%r pk=%r undeleted', self.__class__, self.pk) diff --git a/extensions/admin.py b/extensions/admin.py index a36730eb..37b99080 100644 --- a/extensions/admin.py +++ b/extensions/admin.py @@ -53,7 +53,6 @@ class ExtensionAdmin(admin.ModelAdmin): 'date_created', 'date_status_changed', 'date_approved', - 'date_deleted', 'date_modified', 'average_score', 'text_ratings_count', @@ -76,7 +75,6 @@ class ExtensionAdmin(admin.ModelAdmin): 'date_status_changed', 'date_approved', 'date_modified', - 'date_deleted', ), 'name', 'slug', @@ -135,7 +133,6 @@ class VersionAdmin(admin.ModelAdmin): 'tagline', 'date_created', 'date_modified', - 'date_deleted', 'average_score', 'download_count', ) @@ -147,7 +144,7 @@ class VersionAdmin(admin.ModelAdmin): 'fields': ( 'id', 'tagline', - ('date_created', 'date_modified', 'date_deleted'), + ('date_created', 'date_modified'), 'extension', 'version', 'blender_version_min', @@ -185,8 +182,8 @@ class VersionAdmin(admin.ModelAdmin): class MaintainerAdmin(admin.ModelAdmin): model = Maintainer - list_display = ('extension', 'user', 'date_deleted') - readonly_fields = ('extension', 'user', 'date_deleted') + list_display = ('extension', 'user') + readonly_fields = ('extension', 'user') class LicenseAdmin(admin.ModelAdmin): diff --git a/extensions/forms.py b/extensions/forms.py index 4c72103c..32f4a938 100644 --- a/extensions/forms.py +++ b/extensions/forms.py @@ -76,9 +76,6 @@ class AddPreviewFileForm(forms.ModelForm): ): logger.warning('Found an existing %s pk=%s', model, existing_image.pk) self.instance = existing_image - # Undelete the instance, if necessary - if self.instance.is_deleted: - self.instance.undelete(save=False) # Fill in missing fields from request and the source file self.instance.user = self.request.user diff --git a/extensions/models.py b/extensions/models.py index d7c2f512..5de6d417 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -10,7 +10,7 @@ from django.db.models import F, Q, Count from django.urls import reverse from common.fields import FilterableManyToManyField -from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin +from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin from constants.base import ( AUTHOR_ROLE_CHOICES, AUTHOR_ROLE_DEV, @@ -106,43 +106,33 @@ class License(CreatedModifiedMixin, models.Model): class ExtensionManager(models.Manager): - @property - def exclude_deleted(self): - return self.filter(date_deleted__isnull=True) - @property def listed(self): - return self.exclude_deleted.filter( + return self.filter( status=self.model.STATUSES.APPROVED, is_listed=True, ) @property def unlisted(self): - return self.exclude_deleted.exclude(status=self.model.STATUSES.APPROVED) + return self.exclude(status=self.model.STATUSES.APPROVED) def authored_by(self, user_id: int): - return self.exclude_deleted.filter( - maintainer__user_id=user_id, maintainer__date_deleted__isnull=True - ) + return self.filter(maintainer__user_id=user_id) def listed_or_authored_by(self, user_id: int): - return self.exclude_deleted.filter( - Q(status=self.model.STATUSES.APPROVED) - | Q(maintainer__user_id=user_id, maintainer__date_deleted__isnull=True) + return self.filter( + Q(status=self.model.STATUSES.APPROVED) | Q(maintainer__user_id=user_id) ).distinct() -class Extension( - CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMixin, models.Model -): +class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model): track_changes_to_fields = { 'status', 'name', 'description', 'support', 'website', - 'date_deleted', } TYPES = EXTENSION_TYPE_CHOICES STATUSES = EXTENSION_STATUS_CHOICES @@ -176,7 +166,6 @@ class Extension( User, through='Maintainer', related_name='extensions', - q_filter=Q(maintainer__date_deleted__isnull=True), ) team = models.ForeignKey('teams.Team', null=True, blank=True, on_delete=models.SET_NULL) @@ -190,8 +179,7 @@ class Extension( ordering = ['-average_score', '-date_created', 'name'] def __str__(self): - label_deleted = f'{self.date_deleted and " (DELETED ❌)" or ""}' - return f'{self.get_type_display()} "{self.name}"{label_deleted}' + return f'{self.get_type_display()} "{self.name}"' @property def type_slug(self) -> str: @@ -238,32 +226,18 @@ class Extension( @property def cannot_be_deleted_reasons(self) -> List[str]: - """Return a list of reasons why this extension cannot be fully deleted.""" + """Return a list of reasons why this extension cannot be deleted.""" reasons = [] + if self.abusereport_set.count() > 0: + reasons.append('has_abuse_reports') if self.ratings.count() > 0: reasons.append('has_ratings') if self.is_listed: reasons.append('is_listed') + for v in self.versions.all(): + reasons.extend(v.cannot_be_deleted_reasons) return reasons - @transaction.atomic - def delete(self): - hard = self.cannot_be_deleted_reasons == [] - versions = self.versions.all() - previews = self.previews.all() - # When soft-deleting, filter out already soft-deleted versions and previews - if not hard: - previews = self.previews.filter(date_deleted__isnull=True) - versions = self.versions.filter(date_deleted__isnull=True) - mode = not hard and 'soft-' or '' - for p in previews: - p.delete(hard=hard) - log.warning('%(mode)sdeleting preview file pk=%s source=%s', mode, p.pk, p.source.name) - for v in versions: - v.delete(hard=hard) - log.warning('%(mode)sdeleting extension pk=%s', mode, self.pk) - super().delete(hard=hard) - def get_absolute_url(self): return reverse('extensions:detail', args=[self.type_slug, self.slug]) @@ -316,7 +290,6 @@ class Extension( """Retrieve the latest version.""" return ( self.versions.filter( - date_deleted__isnull=True, file__status__in=self.valid_file_statuses, file__isnull=False, ) @@ -333,7 +306,7 @@ class Extension( If the add-on has not been created yet or is deleted, it returns None. """ - if not self.id or self.is_deleted: + if not self.id: return None try: return self.version @@ -343,14 +316,9 @@ class Extension( def can_request_review(self): """Return whether an add-on can request a review or not.""" - if ( - self.is_deleted - or self.is_disabled - or self.status - in ( - self.STATUSES.APPROVED, - self.STATUSES.AWAITING_REVIEW, - ) + if self.is_disabled or self.status in ( + self.STATUSES.APPROVED, + self.STATUSES.AWAITING_REVIEW, ): return False @@ -381,9 +349,7 @@ class Extension( """Return True if given user is listed as a maintainer.""" if user is None or user.is_anonymous: return False - return self.authors.filter( - maintainer__user_id=user.pk, maintainer__date_deleted__isnull=True - ).exists() + return self.authors.filter(maintainer__user_id=user.pk).exists() def can_rate(self, user) -> bool: """Return True if given user can rate this extension. @@ -451,17 +417,13 @@ class Tag(CreatedModifiedMixin, models.Model): class VersionManager(models.Manager): - @property - def exclude_deleted(self): - return self.filter(date_deleted__isnull=True) - @property def listed(self): - return self.exclude_deleted.filter(file__status=FILE_STATUS_CHOICES.APPROVED) + return self.filter(file__status=FILE_STATUS_CHOICES.APPROVED) @property def unlisted(self): - return self.exclude_deleted.exclude(file__status=FILE_STATUS_CHOICES.APPROVED) + return self.exclude(file__status=FILE_STATUS_CHOICES.APPROVED) def update_or_create(self, *args, **kwargs): # Stash the ManyToMany to be created after the Version has a valid ID already @@ -478,11 +440,10 @@ class VersionManager(models.Manager): return version, result -class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMixin, models.Model): +class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model): track_changes_to_fields = { 'blender_version_min', 'blender_version_max', - 'date_deleted', 'permissions', 'version', 'licenses', @@ -563,21 +524,6 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMi def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - def delete(self, hard=False): - file = self.file - mode = not hard and 'soft-' or '' - args = { - 'file_id': file.pk, - 'mode': mode, - 'source': file.source.name, - 'version_id': self.pk, - 'version': self.version, - } - log.warning('%(mode)sdeleting file pk=%(file_id)s source=%(source)s', args) - file.delete(hard=hard) - log.warning('%(mode)sdeleting version pk=%(version_id)s "%(version)s"', args) - super().delete(hard=hard) - def set_initial_permissions(self, _permissions): if not _permissions: return @@ -618,21 +564,22 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMi self.tags.add(tag) def __str__(self) -> str: - label_deleted = f'{self.date_deleted and " (DELETED ❌)" or ""}' - return f'{self.extension} v{self.version}{label_deleted}' + return f'{self.extension} v{self.version}' + @property def is_listed(self): - # To be public, a version must not be deleted, must belong to a public - # extension, and its attached file must have a public status. - try: - return ( - not self.is_deleted - and self.extension.is_listed - and self.file is not None - and self.file.status == self.file.STATUSES.APPROVED - ) - except models.ObjectDoesNotExist: - return False + # To be public, version file must have a public status. + return self.file is not None and self.file.status == self.file.STATUSES.APPROVED + + @property + def cannot_be_deleted_reasons(self) -> List[str]: + """Return a list of reasons why this version cannot be deleted.""" + reasons = [] + if self.ratings.count() > 0: + reasons.append('version_has_ratings') + if self.is_listed: + reasons.append('version_is_listed') + return reasons @property def pending_rejection(self): @@ -692,7 +639,7 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, SoftDeleteMi ) -class Maintainer(CreatedModifiedMixin, SoftDeleteMixin, models.Model): +class Maintainer(CreatedModifiedMixin, models.Model): extension = models.ForeignKey(Extension, on_delete=models.CASCADE) user = models.ForeignKey(User, on_delete=models.CASCADE) role = models.SmallIntegerField(default=AUTHOR_ROLE_DEV, choices=AUTHOR_ROLE_CHOICES) @@ -716,6 +663,10 @@ class Preview(CreatedModifiedMixin, models.Model): ordering = ('position', 'date_created') unique_together = [['extension', 'file']] + @property + def cannot_be_deleted_reasons(self) -> List[str]: + return [] + class ExtensionReviewerFlags(models.Model): extension = models.OneToOneField( diff --git a/extensions/signals.py b/extensions/signals.py index 780abdd9..5d5d66ae 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,19 +1,43 @@ from typing import Union +import logging -from django.db.models.signals import pre_save, post_save, post_delete +from django.core.exceptions import ValidationError +from django.db.models.signals import pre_save, post_save, pre_delete, post_delete from django.dispatch import receiver import django.dispatch import extensions.models import files.models +logger = logging.getLogger(__name__) + version_changed = django.dispatch.Signal() version_uploaded = django.dispatch.Signal() +@receiver(pre_delete, sender=extensions.models.Extension) +@receiver(pre_delete, sender=extensions.models.Preview) +@receiver(pre_delete, sender=extensions.models.Version) +def _log_extension_delete(sender: object, instance: object, **kwargs: object) -> None: + cannot_be_deleted_reasons = instance.cannot_be_deleted_reasons + # FIXME: remove this temporary sanity check: + # this would break the assumption that admin deletion is "normal" deletion, + # which we'd like to stick to. + if cannot_be_deleted_reasons != []: + # This shouldn't happen: prior validation steps should have taken care of this. + raise ValidationError({'__all__': cannot_be_deleted_reasons}) + + logger.info('Deleting %s pk=%s "%s"', sender, instance.pk, str(instance)) + + @receiver(post_delete, sender=extensions.models.Preview) -def _delete_file(sender: object, instance: extensions.models.Preview, **kwargs: object) -> None: - instance.file.delete() +@receiver(post_delete, sender=extensions.models.Version) +def _delete_file(sender: object, instance: object, **kwargs: object) -> None: + f = instance.file + args = {'f_id': f.pk, 'h': f.hash, 'pk': instance.pk, 'sender': sender, 's': f.source.name} + logger.info('Deleting file pk=%(f_id)s s=%(s)s hash=%(h)s linked to %(sender)s pk=%(pk)s', args) + f.delete() + # TODO: this doesn't mean that the file was deleted from disk @receiver(pre_save, sender=extensions.models.Extension) @@ -46,7 +70,6 @@ def extension_should_be_listed(extension): return ( extension.latest_version is not None and extension.latest_version.is_listed - and extension.latest_version.date_deleted is None and extension.status == extension.STATUSES.APPROVED ) @@ -81,6 +104,7 @@ def _set_is_listed( if extension.status == extensions.models.Extension.STATUSES.APPROVED and not new_is_listed: extension.status = extensions.models.Extension.STATUSES.INCOMPLETE + logger.info('Extension pk=%s becomes listed', extension.pk) extension.is_listed = new_is_listed extension.save() diff --git a/extensions/templates/extensions/version_list.html b/extensions/templates/extensions/version_list.html index e312ec91..41bc43b3 100644 --- a/extensions/templates/extensions/version_list.html +++ b/extensions/templates/extensions/version_list.html @@ -21,7 +21,7 @@
- {% for version in extension.versions.exclude_deleted %} + {% for version in extension.versions %} {% if version.is_listed or is_maintainer %}
diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 11708f14..13c84114 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,4 +1,4 @@ -from django.test import TestCase +from django.test import TransactionTestCase from common.tests.factories.extensions import create_approved_version, create_version from common.tests.factories.files import FileFactory @@ -7,37 +7,12 @@ import extensions.models import files.models -class DeleteTest(TestCase): +class DeleteTest(TransactionTestCase): fixtures = ['dev', 'licenses'] - def test_happy_path(self): - extension = create_approved_version().extension - - url = extension.get_delete_url() - user = extension.authors.first() - self.client.force_login(user) - response = self.client.post(url) - - self.assertEqual(response.status_code, 302) - extension.refresh_from_db() - self.assertIsNotNone(extension.date_deleted) - self.assertTrue(all(v.date_deleted is not None for v in extension.versions.all())) - - def test_random_user_cant_delete(self): - extension = create_approved_version().extension - - url = extension.get_delete_url() - user = UserFactory() - self.client.force_login(user) - response = self.client.post(url) - - self.assertEqual(response.status_code, 403) - extension.refresh_from_db() - self.assertIsNone(extension.date_deleted) - self.assertTrue(all(v.date_deleted is None for v in extension.versions.all())) - - def test_incomplete_unrated_extension_can_be_fully_deleted_by_author(self): + def test_unlisted_unrated_extension_can_be_deleted_by_author(self): version = create_version( + file__status=files.models.File.STATUSES.AWAITING_REVIEW, ratings=[], extension__previews=[ FileFactory( @@ -48,7 +23,10 @@ class DeleteTest(TestCase): ) extension = version.extension version_file = version.file - self.assertEqual(extension.status, extension.STATUSES.INCOMPLETE) + self.assertEqual(version_file.get_status_display(), 'Awaiting Review') + self.assertEqual(extension.get_status_display(), 'Incomplete') + self.assertFalse(extension.is_listed) + self.assertEqual(extension.cannot_be_deleted_reasons, []) preview_file = extension.previews.first() self.assertIsNotNone(preview_file) @@ -71,3 +49,66 @@ class DeleteTest(TestCase): self.assertIsNone(extensions.models.Version.objects.filter(pk=version.pk).first()) self.assertIsNone(files.models.File.objects.filter(pk=version_file.pk).first()) self.assertIsNone(files.models.File.objects.filter(pk=preview_file.pk).first()) + # TODO: check that files were deleted from storage (create a temp one prior to the check) + + def test_publicly_listed_extension_cannot_be_deleted(self): + version = create_approved_version(ratings=[]) + self.assertTrue(version.is_listed) + extension = version.extension + self.assertTrue(extension.is_listed) + self.assertEqual(extension.get_status_display(), 'Approved') + + self.assertEqual(version.cannot_be_deleted_reasons, ['version_is_listed']) + self.assertEqual(extension.cannot_be_deleted_reasons, ['is_listed', 'version_is_listed']) + + url = extension.get_delete_url() + user = extension.authors.first() + self.client.force_login(user) + response = self.client.post(url) + + self.assertEqual(response.status_code, 403) + + def test_rated_extension_cannot_be_deleted(self): + version = create_version(file__status=files.models.File.STATUSES.AWAITING_REVIEW) + self.assertFalse(version.is_listed) + extension = version.extension + self.assertFalse(extension.is_listed) + self.assertEqual(extension.get_status_display(), 'Incomplete') + + self.assertEqual(version.cannot_be_deleted_reasons, ['version_has_ratings']) + self.assertEqual( + extension.cannot_be_deleted_reasons, ['has_ratings', 'version_has_ratings'] + ) + + url = extension.get_delete_url() + user = extension.authors.first() + self.client.force_login(user) + response = self.client.post(url) + + self.assertEqual(response.status_code, 403) + + def test_reported_extension_cannot_be_deleted(self): # TODO + pass + + def test_extension_with_ratings_cannot_be_deleted(self): + version = create_approved_version() + extension = version.extension + self.assertEqual(extension.status, extension.STATUSES.APPROVED) + + url = extension.get_delete_url() + user = extension.authors.first() + self.client.force_login(user) + response = self.client.post(url) + + self.assertEqual(response.status_code, 403) + + def test_random_user_cant_delete(self): + extension = create_approved_version().extension + + url = extension.get_delete_url() + user = UserFactory() + self.client.force_login(user) + response = self.client.post(url) + + self.assertEqual(response.status_code, 403) + extension.refresh_from_db() diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index 9f0b336e..91c573e5 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -47,7 +47,6 @@ class ExtensionTest(TestCase): 'name': 'Extension name', 'status': 1, 'support': 'https://example.com/', - 'date_deleted': None, }, } }, diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index c52bfd24..2a6c81d8 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -98,27 +98,6 @@ class ExtensionDetailViewTest(_BaseTestCase): self._check_detail_page(response, extension) - def test_cannot_view_deleted_extension_anonymously(self): - extension = _create_extension() - extension.delete() - self.assertTrue(extension.is_deleted) - - url = extension.get_absolute_url() - response = self.client.get(url) - - self.assertEqual(response.status_code, 404) - - def test_can_view_deleted_extension_if_staff(self): - staff_user = UserFactory(is_staff=True) - extension = _create_extension() - extension.delete() - self.assertTrue(extension.is_deleted) - - self.client.force_login(staff_user) - response = self.client.get(extension.get_absolute_url()) - - self._check_detail_page(response, extension) - def test_can_view_unlisted_extension_if_maintaner(self): extension = _create_extension() @@ -210,12 +189,6 @@ class ListedExtensionsTest(_BaseTestCase): self.extension.save() self.assertEqual(self._listed_extensions_count(), 0) - def test_soft_delete_only_version(self): - self.version.date_deleted = '1994-01-02 0:0:0+00:00' - self.version.save() - self.assertFalse(self.extension.is_listed) - self.assertEqual(self._listed_extensions_count(), 0) - def test_delete_only_version(self): self.version.delete() self.assertEqual(self._listed_extensions_count(), 0) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index eac467e9..9fddbb12 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -179,8 +179,15 @@ class DeleteExtensionView( return context def test_func(self) -> bool: + obj = self.get_object() # Only maintainers allowed - return self.get_object().has_maintainer(self.request.user) + if not obj.has_maintainer(self.request.user): + return False + # Unless this extension cannot be deleted anymore + cannot_be_deleted_reasons = obj.cannot_be_deleted_reasons + if cannot_be_deleted_reasons != []: + return False + return True class VersionDeleteView( diff --git a/extensions/views/mixins.py b/extensions/views/mixins.py index 5060a492..f6117fe8 100644 --- a/extensions/views/mixins.py +++ b/extensions/views/mixins.py @@ -51,9 +51,7 @@ class ExtensionMixin: """Fetch an extension by slug in the URL before dispatching the view.""" def dispatch(self, *args, **kwargs): - self.extension = get_object_or_404( - Extension.objects.exclude_deleted, slug=self.kwargs['slug'] - ) + self.extension = get_object_or_404(Extension, slug=self.kwargs['slug']) return super().dispatch(*args, **kwargs) diff --git a/extensions/views/public.py b/extensions/views/public.py index db731ddc..29c98e96 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -2,7 +2,6 @@ import logging from django.contrib.auth import get_user_model from django.db.models import Q -from django.http import Http404 from django.shortcuts import get_object_or_404, redirect from django.views.generic.list import ListView @@ -54,8 +53,6 @@ class HomeView(ListedExtensionsView): def extension_version_download(request, type_slug, slug, version): """Download an extension version and count downloads.""" extension_version = get_object_or_404(Version, extension__slug=slug, version=version) - if extension_version.date_deleted is not None: - raise Http404("This extension version has been deleted") ExtensionDownload.create_from_request(request, object_id=extension_version.extension_id) VersionDownload.create_from_request(request, object_id=extension_version.pk) return redirect(extension_version.downloadable_signed_url) diff --git a/files/admin.py b/files/admin.py index 8c8fa1bc..b794f70a 100644 --- a/files/admin.py +++ b/files/admin.py @@ -36,7 +36,6 @@ class FileAdmin(admin.ModelAdmin): 'status', 'date_status_changed', 'date_approved', - 'date_deleted', ) list_display = ('original_name', 'extension', 'user', 'date_created', 'type', 'status', 'is_ok') @@ -45,7 +44,6 @@ class FileAdmin(admin.ModelAdmin): readonly_fields = ( 'id', 'date_created', - 'date_deleted', 'date_modified', 'date_approved', 'date_status_changed', @@ -84,7 +82,6 @@ class FileAdmin(admin.ModelAdmin): 'date_modified', 'date_status_changed', 'date_approved', - 'date_deleted', ) }, ), diff --git a/files/models.py b/files/models.py index 1d6f2d02..31d4b3be 100644 --- a/files/models.py +++ b/files/models.py @@ -5,7 +5,7 @@ import logging from django.contrib.auth import get_user_model from django.db import models -from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin +from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin from files.utils import get_sha256, guess_mimetype_from_ext from constants.base import ( FILE_STATUS_CHOICES, @@ -48,8 +48,8 @@ def thumbnail_upload_to(instance, filename): return path -class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Model): - track_changes_to_fields = {'status', 'size_bytes', 'hash', 'date_deleted'} +class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): + track_changes_to_fields = {'status', 'size_bytes', 'hash'} TYPES = FILE_TYPE_CHOICES STATUSES = FILE_STATUS_CHOICES @@ -103,8 +103,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode objects = FileManager() def __str__(self) -> str: - label_deleted = f'{self.date_deleted and " (DELETED ❌)" or ""}' - return f'{self.original_name} ({self.get_status_display()}){label_deleted}' + return f'{self.original_name} ({self.get_status_display()})' @property def has_been_validated(self): diff --git a/files/signals.py b/files/signals.py index 3388efad..7ee71de2 100644 --- a/files/signals.py +++ b/files/signals.py @@ -1,6 +1,6 @@ import logging -from django.db.models.signals import pre_save, post_save +from django.db.models.signals import pre_save, post_save, pre_delete from django.dispatch import receiver import files.models @@ -30,3 +30,8 @@ def _scan_new_file( return schedule_scan(instance) + + +@receiver(pre_delete, sender=files.models.File) +def _log_file_delete(sender: object, instance: files.models.File, **kwargs: object) -> None: + logger.info('Deleting file pk=%s source=%s', instance.pk, instance.source.name) diff --git a/files/tests/test_models.py b/files/tests/test_models.py index e8066179..cb92e646 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -45,7 +45,6 @@ class FileTest(TestCase): 'status': 2, 'hash': 'foobar', 'size_bytes': 7149, - 'date_deleted': None, }, } }, diff --git a/ratings/admin.py b/ratings/admin.py index 754a1936..e537c0f3 100644 --- a/ratings/admin.py +++ b/ratings/admin.py @@ -16,12 +16,12 @@ class RatingTypeFilter(admin.SimpleListFilter): parameter_name = 'type' def lookups(self, request, model_admin): - """ - Returns a list of tuples. The first element in each - tuple is the coded value for the option that will - appear in the URL query. The second element is the - human-readable name for the option that will appear - in the right sidebar. + """Return a list of lookup option tuples. + + The first element in each tuple is the coded value + for the option that will appear in the URL query. + The second element is the human-readable name for + the option that will appear in the right sidebar. """ return ( ('rating', 'User Rating'), @@ -29,10 +29,10 @@ class RatingTypeFilter(admin.SimpleListFilter): ) def queryset(self, request, queryset): - """ - Returns the filtered queryset based on the value - provided in the query string and retrievable via - `self.value()`. + """Return the filtered queryset. + + Filter based on the value provided in the query string + and retrievable via `self.value()`. """ if self.value() == 'rating': return queryset.filter(reply_to__isnull=True) @@ -53,7 +53,6 @@ class RatingAdmin(admin.ModelAdmin): readonly_fields = ( 'date_created', 'date_modified', - 'date_deleted', 'extension', 'version', 'text', @@ -63,7 +62,6 @@ class RatingAdmin(admin.ModelAdmin): fields = ('status',) + readonly_fields list_display = ( 'date_created', - 'is_deleted', 'user', 'ip_address', 'score', @@ -71,7 +69,7 @@ class RatingAdmin(admin.ModelAdmin): 'status', 'truncated_text', ) - list_filter = ('status', RatingTypeFilter, 'score', 'date_deleted') + list_filter = ('status', RatingTypeFilter, 'score') actions = ('delete_selected',) list_select_related = ('user',) # For extension/reply_to see get_queryset() @@ -94,11 +92,5 @@ class RatingAdmin(admin.ModelAdmin): is_reply.boolean = True is_reply.admin_order_field = 'reply_to' - def is_deleted(self, obj): - return bool(obj.date_deleted) - - is_deleted.boolean = True - is_deleted.admin_order_field = 'date_deleted' - admin.site.register(Rating, RatingAdmin) diff --git a/ratings/models.py b/ratings/models.py index 27749a6e..f0fe383a 100644 --- a/ratings/models.py +++ b/ratings/models.py @@ -1,12 +1,12 @@ import logging from django.contrib.auth import get_user_model -from django.db import models, transaction +from django.db import models from django.template.defaultfilters import truncatechars from django.utils.translation import gettext_lazy as _ from django.urls import reverse -from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin +from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin from common.templatetags import common from constants.base import RATING_STATUS_CHOICES, RATING_SCORE_CHOICES from utils import send_mail @@ -17,28 +17,20 @@ log = logging.getLogger(__name__) class RatingManager(models.Manager): # TODO: figure out how to retrieve reviews "annotated" with replies, if any - @property - def exclude_deleted(self): - return self.filter(date_deleted__isnull=True) - @property def listed(self): - return self.exclude_deleted.filter( - status=self.model.STATUSES.APPROVED, reply_to__isnull=True - ) + return self.filter(status=self.model.STATUSES.APPROVED, reply_to__isnull=True) @property def unlisted(self): - return self.exclude_deleted.exclude( - status=self.models.STATUSES.APPROVED, reply_to__isnull=True - ) + return self.exclude(status=self.models.STATUSES.APPROVED, reply_to__isnull=True) @property def listed_texts(self): return self.listed.filter(text__isnull=False) -class Rating(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Model): +class Rating(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = {'status'} STATUSES = RATING_STATUS_CHOICES @@ -107,7 +99,7 @@ class Rating(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mo @classmethod def get_for(cls, user_id: int, extension_id: int): """Get rating left by a given user for a given extension.""" - return cls.objects.exclude_deleted.filter( + return cls.objects.filter( reply_to=None, user_id=user_id, extension_id=extension_id, @@ -141,25 +133,6 @@ class Rating(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mo # user_responsible=user. self.save() - @transaction.atomic - def delete(self, user_responsible=None, send_post_save_signal=True): - if user_responsible is None: - user_responsible = self.user - - for flag in self.ratingflag_set.all(): - flag.delete() - - SoftDeleteMixin.delete(self) - log.warning( - 'Rating deleted: user pk=%s (%s) deleted id:%s by pk=%s (%s) ("%s")', - user_responsible.pk, - str(user_responsible), - self.pk, - self.user.pk, - str(self.user), - str(self.text), - ) - @classmethod def get_replies(cls, ratings): ratings = [r.id for r in ratings] diff --git a/ratings/tests/test_views.py b/ratings/tests/test_views.py index e1326569..b1b278a8 100644 --- a/ratings/tests/test_views.py +++ b/ratings/tests/test_views.py @@ -29,7 +29,6 @@ class RatingsViewTest(TestCase): ), RatingFactory( version=version, - date_deleted='2022-01-01 01:01:01Z', text='this rating is deleted', status=Rating.STATUSES.APPROVED, ), diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index a938ec80..6bfee106 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -18,9 +18,7 @@ class CommentsViewTest(TestCase): self.assertEqual(len(r.context['object_list']), 1) # Deleted extensions don't show up in the approval queue - self.assertIsNone(self.default_version.extension.date_deleted) self.default_version.extension.delete() - self.assertIsNotNone(self.default_version.extension.date_deleted) r = self.client.get(reverse('reviewers:approval-queue')) self.assertEqual(r.status_code, 200) self.assertEqual(len(r.context['object_list']), 0) diff --git a/reviewers/views.py b/reviewers/views.py index d8393132..1b5e1811 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -18,10 +18,8 @@ class ApprovalQueueView(ListView): paginate_by = 100 def get_queryset(self): - return ( - Extension.objects.exclude_deleted - .exclude(status=Extension.STATUSES.APPROVED) - .order_by('-date_created') + return Extension.objects.exclude(status=Extension.STATUSES.APPROVED).order_by( + '-date_created' ) template_name = 'reviewers/extensions_review_list.html' -- 2.30.2 From 2bf06df9eb2a806a0888c0dba3fafa5f9ebe2256 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 11:00:14 +0200 Subject: [PATCH 05/18] Migrations --- .../0005_remove_abusereport_date_deleted.py | 17 +++++++++++++ ..._remove_extension_date_deleted_and_more.py | 25 +++++++++++++++++++ .../0006_remove_file_date_deleted.py | 17 +++++++++++++ .../0005_remove_rating_date_deleted.py | 17 +++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 abuse/migrations/0005_remove_abusereport_date_deleted.py create mode 100644 extensions/migrations/0026_remove_extension_date_deleted_and_more.py create mode 100644 files/migrations/0006_remove_file_date_deleted.py create mode 100644 ratings/migrations/0005_remove_rating_date_deleted.py diff --git a/abuse/migrations/0005_remove_abusereport_date_deleted.py b/abuse/migrations/0005_remove_abusereport_date_deleted.py new file mode 100644 index 00000000..fbf62964 --- /dev/null +++ b/abuse/migrations/0005_remove_abusereport_date_deleted.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.11 on 2024-04-18 08:59 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('abuse', '0004_abusereport_rating_abusereport_type'), + ] + + operations = [ + migrations.RemoveField( + model_name='abusereport', + name='date_deleted', + ), + ] diff --git a/extensions/migrations/0026_remove_extension_date_deleted_and_more.py b/extensions/migrations/0026_remove_extension_date_deleted_and_more.py new file mode 100644 index 00000000..4b017507 --- /dev/null +++ b/extensions/migrations/0026_remove_extension_date_deleted_and_more.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.11 on 2024-04-18 08:59 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('extensions', '0025_alter_tag_type'), + ] + + operations = [ + migrations.RemoveField( + model_name='extension', + name='date_deleted', + ), + migrations.RemoveField( + model_name='maintainer', + name='date_deleted', + ), + migrations.RemoveField( + model_name='version', + name='date_deleted', + ), + ] diff --git a/files/migrations/0006_remove_file_date_deleted.py b/files/migrations/0006_remove_file_date_deleted.py new file mode 100644 index 00000000..5d790366 --- /dev/null +++ b/files/migrations/0006_remove_file_date_deleted.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.11 on 2024-04-18 08:59 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0005_rename_validation_filevalidation_results_and_more'), + ] + + operations = [ + migrations.RemoveField( + model_name='file', + name='date_deleted', + ), + ] diff --git a/ratings/migrations/0005_remove_rating_date_deleted.py b/ratings/migrations/0005_remove_rating_date_deleted.py new file mode 100644 index 00000000..7acd928b --- /dev/null +++ b/ratings/migrations/0005_remove_rating_date_deleted.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.11 on 2024-04-18 08:59 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('ratings', '0004_alter_rating_status'), + ] + + operations = [ + migrations.RemoveField( + model_name='rating', + name='date_deleted', + ), + ] -- 2.30.2 From e98ac907f615c787bd18e3e239233e87d92b83ae Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 11:52:09 +0200 Subject: [PATCH 06/18] Minor change --- extensions/signals.py | 2 +- extensions/views/manage.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/signals.py b/extensions/signals.py index 5d5d66ae..dd678bac 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -23,7 +23,7 @@ def _log_extension_delete(sender: object, instance: object, **kwargs: object) -> # FIXME: remove this temporary sanity check: # this would break the assumption that admin deletion is "normal" deletion, # which we'd like to stick to. - if cannot_be_deleted_reasons != []: + if len(cannot_be_deleted_reasons) > 0: # This shouldn't happen: prior validation steps should have taken care of this. raise ValidationError({'__all__': cannot_be_deleted_reasons}) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 9fddbb12..f4ac8224 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -185,7 +185,7 @@ class DeleteExtensionView( return False # Unless this extension cannot be deleted anymore cannot_be_deleted_reasons = obj.cannot_be_deleted_reasons - if cannot_be_deleted_reasons != []: + if len(cannot_be_deleted_reasons) > 0: return False return True -- 2.30.2 From 051657aa5bff6dc671d8219dbfb95c47dd88904f Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 12:24:41 +0200 Subject: [PATCH 07/18] File's default status should be 'awaiting approval' --- files/migrations/0007_alter_file_status.py | 18 ++++++++++++++++++ files/models.py | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 files/migrations/0007_alter_file_status.py diff --git a/files/migrations/0007_alter_file_status.py b/files/migrations/0007_alter_file_status.py new file mode 100644 index 00000000..7680310d --- /dev/null +++ b/files/migrations/0007_alter_file_status.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.11 on 2024-04-18 10:10 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0006_remove_file_date_deleted'), + ] + + operations = [ + migrations.AlterField( + model_name='file', + name='status', + field=models.PositiveSmallIntegerField(choices=[(2, 'Awaiting Review'), (3, 'Approved'), (4, 'Disabled by staff'), (5, 'Disabled by author')], default=2), + ), + ] diff --git a/files/models.py b/files/models.py index 31d4b3be..728d0428 100644 --- a/files/models.py +++ b/files/models.py @@ -76,7 +76,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'as guessed from its contents.' ), ) - status = models.PositiveSmallIntegerField(choices=STATUSES, default=STATUSES.APPROVED) + status = models.PositiveSmallIntegerField(choices=STATUSES, default=STATUSES.AWAITING_REVIEW) user = models.ForeignKey( User, related_name='files', null=False, blank=False, on_delete=models.CASCADE -- 2.30.2 From 62931d361e563998db89af9aaf2ac27c1a56c019 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 12:25:49 +0200 Subject: [PATCH 08/18] Only display delete button if extension can be deleted --- extensions/templates/extensions/draft_finalise.html | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/extensions/templates/extensions/draft_finalise.html b/extensions/templates/extensions/draft_finalise.html index 27d21d4f..989282d0 100644 --- a/extensions/templates/extensions/draft_finalise.html +++ b/extensions/templates/extensions/draft_finalise.html @@ -91,10 +91,14 @@ {% trans 'Submit for Approval' %} - - - {% trans 'Delete Extension' %} - + {% with cannot_be_deleted_reasons=extension.cannot_be_deleted_reasons %} + {% if not cannot_be_deleted_reasons %} + + + {% trans 'Delete Extension' %} + + {% endif %} + {% endwith %}
-- 2.30.2 From 97eda95184e53fe1953a9ae4f6b843607f6113f2 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 12:27:32 +0200 Subject: [PATCH 09/18] Test versions page --- extensions/templates/extensions/version_list.html | 2 +- extensions/tests/test_views.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/extensions/templates/extensions/version_list.html b/extensions/templates/extensions/version_list.html index 41bc43b3..810446b1 100644 --- a/extensions/templates/extensions/version_list.html +++ b/extensions/templates/extensions/version_list.html @@ -21,7 +21,7 @@
- {% for version in extension.versions %} + {% for version in extension.versions.all %} {% if version.is_listed or is_maintainer %}
diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index 2a6c81d8..8afa2ae3 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -114,6 +114,14 @@ class ExtensionDetailViewTest(_BaseTestCase): self._check_detail_page(response, extension) + def test_can_view_publicly_listed_extension_versions_anonymously(self): + extension = _create_extension() + extension.approve() + + response = self.client.get(extension.get_versions_url()) + + self._check_detail_page(response, extension) + def test_can_view_publicly_listed_extension_ratings_anonymously(self): extension = _create_extension() extension.approve() -- 2.30.2 From f3205d44291e57f9c8816fdc68ba1a6298488547 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 12:28:18 +0200 Subject: [PATCH 10/18] Extension draft page should not break when there are no versions --- extensions/views/manage.py | 5 +++-- extensions/views/mixins.py | 4 ---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index f4ac8224..c9f2b0a0 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -361,8 +361,9 @@ class DraftExtensionView( def get_initial(self): """Return initial values for the version, based on the file.""" initial = super().get_initial() - initial['file'] = self.version.file - initial.update(**self.version.file.parsed_version_fields) + if self.version: + initial['file'] = self.version.file + initial.update(**self.version.file.parsed_version_fields) return initial def get_context_data(self, form=None, extension_form=None, add_preview_formset=None, **kwargs): diff --git a/extensions/views/mixins.py b/extensions/views/mixins.py index f6117fe8..d06f06a0 100644 --- a/extensions/views/mixins.py +++ b/extensions/views/mixins.py @@ -1,6 +1,5 @@ from django.contrib.auth.mixins import UserPassesTestMixin from django.shortcuts import get_object_or_404, redirect -from django.core.exceptions import BadRequest from extensions.models import Extension from files.models import File @@ -60,9 +59,6 @@ class DraftVersionMixin: def dispatch(self, *args, **kwargs): self.version = self.extension.versions.first() - if self.version is None: - error_message = f'Extension "{self.extension.name}" has no versions.' - raise BadRequest(error_message) return super().dispatch(*args, **kwargs) -- 2.30.2 From 9329c8c63d4d7e5a34e6797030860ecd52e4dca9 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 12:33:47 +0200 Subject: [PATCH 11/18] Only display version delete btn if can delete this version --- extensions/templates/extensions/version_list.html | 12 ++++++++---- extensions/views/manage.py | 9 +++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/extensions/templates/extensions/version_list.html b/extensions/templates/extensions/version_list.html index 810446b1..bc44bd41 100644 --- a/extensions/templates/extensions/version_list.html +++ b/extensions/templates/extensions/version_list.html @@ -103,10 +103,14 @@ Edit - - - {% trans "Delete Version" %} - + {% with cannot_be_deleted_reasons=version.cannot_be_deleted_reasons %} + {% if not cannot_be_deleted_reasons %} + + + {% trans "Delete Version" %} + + {% endif %} + {% endwith %} {% endif %}
diff --git a/extensions/views/manage.py b/extensions/views/manage.py index c9f2b0a0..acd2ba65 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -193,6 +193,7 @@ class DeleteExtensionView( class VersionDeleteView( LoginRequiredMixin, MaintainedExtensionMixin, + UserPassesTestMixin, DeleteView, ): model = Version @@ -232,6 +233,14 @@ class VersionDeleteView( context['confirm_url'] = version.get_delete_url() return context + def test_func(self) -> bool: + obj = self.get_object() + # Unless this version cannot be deleted anymore + cannot_be_deleted_reasons = obj.cannot_be_deleted_reasons + if len(cannot_be_deleted_reasons) > 0: + return False + return True + class ManageVersionsView( LoginRequiredMixin, -- 2.30.2 From 9cee4249f0ac4e2831ee5bac4e7fd5522e2bdd72 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 14:08:49 +0200 Subject: [PATCH 12/18] Update migration to "unapprove" files of unlisted extensions --- extensions/signals.py | 27 ++++++++++++++++++++ files/migrations/0007_alter_file_status.py | 29 ++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/extensions/signals.py b/extensions/signals.py index dd678bac..2f57aabb 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -109,4 +109,31 @@ def _set_is_listed( extension.save() +@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], + created: bool, + raw: bool, + **kwargs: object, +): + if raw: + return + if not created: + return + if not instance.file_id: + return + + # N.B.: currently, subsequent version and preview uploads get approved automatically, + # if extension is currently listed (meaning, it was approved by a human already). + extension = instance.extension + file = instance.file + if extension.is_listed: + file.status = files.models.File.STATUSES.APPROVED + 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'}) + + version_uploaded.connect(send_notifications, dispatch_uid='send_notifications') diff --git a/files/migrations/0007_alter_file_status.py b/files/migrations/0007_alter_file_status.py index 7680310d..5dff9aff 100644 --- a/files/migrations/0007_alter_file_status.py +++ b/files/migrations/0007_alter_file_status.py @@ -1,7 +1,35 @@ # Generated by Django 4.2.11 on 2024-04-18 10:10 +import logging from django.db import migrations, models +logger = logging.getLogger(__name__) + + +def change_first_uploads_to_awaiting_review(apps, schema_editor): + from constants.base import FILE_STATUS_CHOICES + + File = apps.get_model('files', 'File') + to_update = [] + for obj in File.objects.filter(status=FILE_STATUS_CHOICES.APPROVED): + # Unless this is a subsequent upload of a version/preview of an already approved extension, + # it should have a default status (Awaiting Review). + extension = ( + obj.extension_preview.first() and obj.extension_preview.first().extension + or (hasattr(obj, 'version') and obj.version.extension) + ) + if extension.is_listed: + continue + logger.info( + 'Will change file pk=%s status from %s to %s', obj.pk, obj.get_status_display(), + 'Awaiting Review', + ) + obj.status = FILE_STATUS_CHOICES.AWAITING_REVIEW + to_update.append(obj) + + logger.info('Updating %s files', len(to_update)) + File.objects.bulk_update(to_update, batch_size=500, fields={'status'}) + class Migration(migrations.Migration): @@ -15,4 +43,5 @@ class Migration(migrations.Migration): name='status', field=models.PositiveSmallIntegerField(choices=[(2, 'Awaiting Review'), (3, 'Approved'), (4, 'Disabled by staff'), (5, 'Disabled by author')], default=2), ), + migrations.RunPython(change_first_uploads_to_awaiting_review), ] -- 2.30.2 From 25ff5b4efefac63ce64872755dde62625b184a6d Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 15:11:48 +0200 Subject: [PATCH 13/18] Remove tests depending on soft-deletion --- common/tests/factories/extensions.py | 1 + extensions/signals.py | 9 ++++----- extensions/tests/test_submit.py | 2 +- extensions/tests/test_views.py | 8 -------- ratings/tests/test_views.py | 5 ----- reviewers/tests/test_views.py | 6 ------ 6 files changed, 6 insertions(+), 25 deletions(-) diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 6e8bf1ac..76dd4ec4 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -94,4 +94,5 @@ def create_version(**kwargs) -> 'Version': def create_approved_version(**kwargs) -> 'Version': version = create_version(**kwargs) version.extension.approve() + version.refresh_from_db() return version diff --git a/extensions/signals.py b/extensions/signals.py index 2f57aabb..e5f2ffd9 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,7 +1,7 @@ from typing import Union import logging -from django.core.exceptions import ValidationError +# from django.core.exceptions import ValidationError from django.db.models.signals import pre_save, post_save, pre_delete, post_delete from django.dispatch import receiver import django.dispatch @@ -20,12 +20,11 @@ version_uploaded = django.dispatch.Signal() @receiver(pre_delete, sender=extensions.models.Version) def _log_extension_delete(sender: object, instance: object, **kwargs: object) -> None: cannot_be_deleted_reasons = instance.cannot_be_deleted_reasons - # FIXME: remove this temporary sanity check: - # this would break the assumption that admin deletion is "normal" deletion, - # which we'd like to stick to. if len(cannot_be_deleted_reasons) > 0: # This shouldn't happen: prior validation steps should have taken care of this. - raise ValidationError({'__all__': cannot_be_deleted_reasons}) + # raise ValidationError({'__all__': cannot_be_deleted_reasons}) + args = {'sender': sender, 'pk': instance.pk, 'reasons': cannot_be_deleted_reasons} + logger.error("%(sender)s pk=%(pk)s is being deleted but it %(reasons)s", args) logger.info('Deleting %s pk=%s "%s"', sender, instance.pk, str(instance)) diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index ee076187..2dde6a4f 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -352,7 +352,7 @@ class SubmitFinaliseTest(TestCase): self.assertEqual(version.blender_version_max, None) self.assertEqual(version.schema_version, '1.0.0') self.assertEqual(version.release_notes, data['release_notes']) - self.assertEqual(version.file.get_status_display(), 'Approved') + self.assertEqual(version.file.get_status_display(), 'Awaiting Review') # We cannot check for the ManyToMany yet (tags, licences, permissions) # Check that author can access the page they are redirected to diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index 8afa2ae3..91cc1866 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -188,19 +188,11 @@ class ListedExtensionsTest(_BaseTestCase): create_approved_version(extension=self.extension) self.assertEqual(self._listed_extensions_count(), 1) - def test_delete_extension(self): - self.extension.delete() - self.assertEqual(self._listed_extensions_count(), 0) - def test_moderate_extension(self): self.extension.status = Extension.STATUSES.DISABLED self.extension.save() self.assertEqual(self._listed_extensions_count(), 0) - def test_delete_only_version(self): - self.version.delete() - self.assertEqual(self._listed_extensions_count(), 0) - def test_moderate_only_version(self): self.version.file.status = File.STATUSES.DISABLED self.version.file.save() diff --git a/ratings/tests/test_views.py b/ratings/tests/test_views.py index b1b278a8..d8f7952b 100644 --- a/ratings/tests/test_views.py +++ b/ratings/tests/test_views.py @@ -27,11 +27,6 @@ class RatingsViewTest(TestCase): text='this rating is also approved', status=Rating.STATUSES.APPROVED, ), - RatingFactory( - version=version, - text='this rating is deleted', - status=Rating.STATUSES.APPROVED, - ), ] url = version.extension.get_ratings_url() diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index 6bfee106..dd80c1ff 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -17,12 +17,6 @@ class CommentsViewTest(TestCase): self.assertEqual(r.status_code, 200) self.assertEqual(len(r.context['object_list']), 1) - # Deleted extensions don't show up in the approval queue - self.default_version.extension.delete() - r = self.client.get(reverse('reviewers:approval-queue')) - self.assertEqual(r.status_code, 200) - self.assertEqual(len(r.context['object_list']), 0) - # Page is visible for every extension and does not require authentication def test_visibility(self): r = self.client.get( -- 2.30.2 From 439b2f70ca63193318ae3c915d17b1fe00ee87c9 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 15:12:40 +0200 Subject: [PATCH 14/18] Admin: allow deletion of LogEntry (otherwise it blocks deletion of other objects) --- common/admin.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/common/admin.py b/common/admin.py index fde17d98..ca19b840 100644 --- a/common/admin.py +++ b/common/admin.py @@ -212,9 +212,6 @@ class LogEntryAdmin(admin.ModelAdmin): def has_change_permission(self, request, obj=None): return False - def has_delete_permission(self, request, obj=None): - return False - def has_view_permission(self, request, obj=None): return request.user.is_superuser -- 2.30.2 From b06768a15274b7470a57952934f86887ad26e005 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 15:13:09 +0200 Subject: [PATCH 15/18] Fake data: set logging level from verbosity --- common/management/commands/generate_fake_data.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/common/management/commands/generate_fake_data.py b/common/management/commands/generate_fake_data.py index bf90b76f..37191b15 100644 --- a/common/management/commands/generate_fake_data.py +++ b/common/management/commands/generate_fake_data.py @@ -1,3 +1,4 @@ +import logging import random from django.core.management.base import BaseCommand @@ -61,6 +62,15 @@ class Command(BaseCommand): help = 'Generate fake data with extensions, users and versions using test factories.' def handle(self, *args, **options): + verbosity = int(options['verbosity']) + root_logger = logging.getLogger('root') + if verbosity > 2: + root_logger.setLevel(logging.DEBUG) + elif verbosity > 1: + root_logger.setLevel(logging.INFO) + else: + root_logger.setLevel(logging.WARNING) + tags = { type_id: list(Tag.objects.filter(type=type_id).values_list('name', flat=True)) for type_id, _ in Extension.TYPES -- 2.30.2 From 6a66e164f7164046c6b56aedf8119c2a3d2e951b Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 16:19:48 +0200 Subject: [PATCH 16/18] Update migrations --- ..._date_deleted.py => 0006_remove_abusereport_date_deleted.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename abuse/migrations/{0005_remove_abusereport_date_deleted.py => 0006_remove_abusereport_date_deleted.py} (82%) diff --git a/abuse/migrations/0005_remove_abusereport_date_deleted.py b/abuse/migrations/0006_remove_abusereport_date_deleted.py similarity index 82% rename from abuse/migrations/0005_remove_abusereport_date_deleted.py rename to abuse/migrations/0006_remove_abusereport_date_deleted.py index fbf62964..cc451fff 100644 --- a/abuse/migrations/0005_remove_abusereport_date_deleted.py +++ b/abuse/migrations/0006_remove_abusereport_date_deleted.py @@ -6,7 +6,7 @@ from django.db import migrations class Migration(migrations.Migration): dependencies = [ - ('abuse', '0004_abusereport_rating_abusereport_type'), + ('abuse', '0005_alter_abusereport_type'), ] operations = [ -- 2.30.2 From c0a20f121eebab3899250e36e3c42344d5e86134 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 18 Apr 2024 19:03:16 +0200 Subject: [PATCH 17/18] Update confirmation message --- extensions/templates/extensions/confirm_delete.html | 3 +-- extensions/templates/extensions/version_confirm_delete.html | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/extensions/templates/extensions/confirm_delete.html b/extensions/templates/extensions/confirm_delete.html index b5c7e4f2..344bbb3c 100644 --- a/extensions/templates/extensions/confirm_delete.html +++ b/extensions/templates/extensions/confirm_delete.html @@ -9,8 +9,7 @@

{% blocktranslate with extension_name=extension_name %} - By deleting {{ extension_name }} you will lose the files, - download count, reviews as well ratings for the whole extension. + All extension previews and versions files will be deleted. {% endblocktranslate %}

diff --git a/extensions/templates/extensions/version_confirm_delete.html b/extensions/templates/extensions/version_confirm_delete.html index f20f8e04..9a8375fe 100644 --- a/extensions/templates/extensions/version_confirm_delete.html +++ b/extensions/templates/extensions/version_confirm_delete.html @@ -8,9 +8,8 @@ {% blocktranslate with extension_name=extension_name %} Delete {{ extension_name }} {{ version }}?{% endblocktranslate %}

- {% blocktranslate with extension_name=extension_name version=version.version %} - By deleting {{ extension_name }} {{ version }} you will lose the files, - download count, reviews as well ratings for this version. + {% blocktranslate with extension_name=extension_name %} + Files belonging to this version will be deleted. {% endblocktranslate %}

-- 2.30.2 From b375f9d86505c65e20f1392bdd8fcdf927dabb7e Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 19 Apr 2024 10:29:16 +0200 Subject: [PATCH 18/18] Minor changes --- extensions/models.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/extensions/models.py b/extensions/models.py index 5de6d417..bee30ff5 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -228,12 +228,12 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod def cannot_be_deleted_reasons(self) -> List[str]: """Return a list of reasons why this extension cannot be deleted.""" reasons = [] - if self.abusereport_set.count() > 0: - reasons.append('has_abuse_reports') - if self.ratings.count() > 0: - reasons.append('has_ratings') if self.is_listed: reasons.append('is_listed') + if self.ratings.count() > 0: + reasons.append('has_ratings') + if self.abusereport_set.count() > 0: + reasons.append('has_abuse_reports') for v in self.versions.all(): reasons.extend(v.cannot_be_deleted_reasons) return reasons @@ -575,10 +575,10 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model def cannot_be_deleted_reasons(self) -> List[str]: """Return a list of reasons why this version cannot be deleted.""" reasons = [] - if self.ratings.count() > 0: - reasons.append('version_has_ratings') if self.is_listed: reasons.append('version_is_listed') + if self.ratings.count() > 0: + reasons.append('version_has_ratings') return reasons @property @@ -665,6 +665,7 @@ class Preview(CreatedModifiedMixin, models.Model): @property def cannot_be_deleted_reasons(self) -> List[str]: + """Return a list of reasons why this preview cannot be deleted.""" return [] -- 2.30.2