diff --git a/abuse/migrations/0006_remove_abusereport_date_deleted.py b/abuse/migrations/0006_remove_abusereport_date_deleted.py new file mode 100644 index 00000000..cc451fff --- /dev/null +++ b/abuse/migrations/0006_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', '0005_alter_abusereport_type'), + ] + + operations = [ + migrations.RemoveField( + model_name='abusereport', + name='date_deleted', + ), + ] diff --git a/abuse/models.py b/abuse/models.py index 687328d2..a508e0d9 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_RATING -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/common/model_mixins.py b/common/model_mixins.py index 3bc59d4f..3cd95568 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 @@ -152,43 +152,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/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/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/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/extensions/models.py b/extensions/models.py index 8919a424..bee30ff5 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: @@ -236,12 +224,19 @@ class Extension( self.status = self.STATUSES.APPROVED self.save() - @transaction.atomic - def delete(self, hard=False): - versions = self.versions.filter(date_deleted__isnull=True) - for v in versions: - v.delete(hard=hard) - super().delete(hard=hard) + @property + def cannot_be_deleted_reasons(self) -> List[str]: + """Return a list of reasons why this extension cannot be deleted.""" + reasons = [] + 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 def get_absolute_url(self): return reverse('extensions:detail', args=[self.type_slug, self.slug]) @@ -295,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, ) @@ -312,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 @@ -322,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 @@ -360,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. @@ -430,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 @@ -457,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', @@ -582,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.is_listed: + reasons.append('version_is_listed') + if self.ratings.count() > 0: + reasons.append('version_has_ratings') + return reasons @property def pending_rejection(self): @@ -656,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) @@ -680,6 +663,11 @@ class Preview(CreatedModifiedMixin, models.Model): ordering = ('position', 'date_created') unique_together = [['extension', 'file']] + @property + def cannot_be_deleted_reasons(self) -> List[str]: + """Return a list of reasons why this preview cannot be deleted.""" + return [] + class ExtensionReviewerFlags(models.Model): extension = models.OneToOneField( diff --git a/extensions/signals.py b/extensions/signals.py index 7738ca76..6ba69b8d 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -1,21 +1,42 @@ from typing import Union +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, post_delete +from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete, post_delete from django.dispatch import receiver from constants.activity import Flag import extensions.models import files.models +logger = logging.getLogger(__name__) User = get_user_model() +@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 + 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}) + 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)) + + @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) @@ -45,7 +66,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 ) @@ -80,6 +100,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() @@ -118,3 +139,30 @@ def _update_authors_follow(instance, action, model, reverse, pk_set, **kwargs): unfollow(user, extension, send_action=False, flag=Flag.AUTHOR) elif action == 'post_add': follow(user, extension, send_action=False, flag=Flag.AUTHOR) + + +@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'}) 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/draft_finalise.html b/extensions/templates/extensions/draft_finalise.html index 2a1a9928..989282d0 100644 --- a/extensions/templates/extensions/draft_finalise.html +++ b/extensions/templates/extensions/draft_finalise.html @@ -90,6 +90,15 @@ {% trans 'Submit for Approval' %} + + {% with cannot_be_deleted_reasons=extension.cannot_be_deleted_reasons %} + {% if not cannot_be_deleted_reasons %} + + + {% trans 'Delete Extension' %} + + {% endif %} + {% endwith %}
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 %}

diff --git a/extensions/templates/extensions/version_list.html b/extensions/templates/extensions/version_list.html index e312ec91..bc44bd41 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.all %} {% if version.is_listed or is_maintainer %}
@@ -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/tests/test_delete.py b/extensions/tests/test_delete.py index 093b2c72..c1e64eb4 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,14 +1,34 @@ 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): fixtures = ['dev', 'licenses'] - def test_happy_path(self): - extension = create_approved_version().extension + 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( + type=files.models.File.TYPES.IMAGE, + source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', + ) + ], + ) + extension = version.extension + version_file = version.file + 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) url = extension.get_delete_url() user = extension.authors.first() @@ -16,9 +36,71 @@ class DeleteTest(TestCase): 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())) + # 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()) + # 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 @@ -30,5 +112,3 @@ class DeleteTest(TestCase): 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())) 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_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 c52bfd24..91cc1866 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() @@ -135,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() @@ -201,25 +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_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) - def test_moderate_only_version(self): self.version.file.status = File.STATUSES.DISABLED self.version.file.save() diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 79c19006..1af1b643 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -180,13 +180,21 @@ 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 len(cannot_be_deleted_reasons) > 0: + return False + return True class VersionDeleteView( LoginRequiredMixin, MaintainedExtensionMixin, + UserPassesTestMixin, DeleteView, ): model = Version @@ -226,6 +234,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, @@ -355,8 +371,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 5060a492..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 @@ -51,9 +50,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) @@ -62,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) 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/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/files/migrations/0007_alter_file_status.py b/files/migrations/0007_alter_file_status.py new file mode 100644 index 00000000..5dff9aff --- /dev/null +++ b/files/migrations/0007_alter_file_status.py @@ -0,0 +1,47 @@ +# 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): + + 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), + ), + migrations.RunPython(change_first_uploads_to_awaiting_review), + ] diff --git a/files/models.py b/files/models.py index 1d6f2d02..728d0428 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 @@ -76,7 +76,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode '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 @@ -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 ee4175f3..49491434 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 @@ -32,3 +32,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/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', + ), + ] 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..d8f7952b 100644 --- a/ratings/tests/test_views.py +++ b/ratings/tests/test_views.py @@ -27,12 +27,6 @@ class RatingsViewTest(TestCase): text='this rating is also approved', status=Rating.STATUSES.APPROVED, ), - RatingFactory( - version=version, - date_deleted='2022-01-01 01:01:01Z', - 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 a938ec80..dd80c1ff 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -17,14 +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.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) - # Page is visible for every extension and does not require authentication def test_visibility(self): r = self.client.get( 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'