diff --git a/abuse/signals.py b/abuse/signals.py index 129a5c77..3e266676 100644 --- a/abuse/signals.py +++ b/abuse/signals.py @@ -1,7 +1,7 @@ import logging from actstream import action -from django.db.models.signals import post_save +from django.db.models.signals import post_save, pre_delete from django.dispatch import receiver from abuse.models import AbuseReport @@ -45,3 +45,8 @@ def _create_action_from_report( target=instance.extension, action_object=instance, ) + + +@receiver(pre_delete, sender=AbuseReport) +def _log_deletion(sender: object, instance: AbuseReport, **kwargs: object) -> None: + instance.record_deletion() diff --git a/common/model_mixins.py b/common/model_mixins.py index 3cd95568..6b2a32c4 100644 --- a/common/model_mixins.py +++ b/common/model_mixins.py @@ -2,7 +2,9 @@ from typing import Set, Tuple, Mapping, Any import copy import logging +from django.contrib.admin.models import DELETION from django.contrib.contenttypes.models import ContentType +from django.core import serializers from django.db import models from django.shortcuts import reverse from django.utils import timezone @@ -45,7 +47,36 @@ class CreatedModifiedMixin(models.Model): return f'{path}?{query}' -class TrackChangesMixin(models.Model): +class RecordDeletionMixin: + def serialise(self) -> dict: + data = serializers.serialize('python', [self])[0] + data['fields']['pk'] = data['pk'] + return data['fields'] + + def record_deletion(self): + """Create a LogEntry describing a deletion of this object.""" + msg_args = {'type': type(self), 'pk': self.pk} + logger.info('Deleting %(type)s pk=%(pk)s', msg_args) + if hasattr(self, 'cannot_be_deleted_reasons'): + cannot_be_deleted_reasons = self.cannot_be_deleted_reasons + if len(cannot_be_deleted_reasons) > 0: + # This shouldn't happen: prior validation steps should have taken care of this. + msg_args['reasons'] = cannot_be_deleted_reasons + logger.error("%(type)s pk=%(pk)s is being deleted but it %(reasons)s", msg_args) + state = self.serialise() + message = [ + { + 'deleted': { + 'name': str(self._meta.verbose_name), + 'object': repr(self), + 'old_state': state, + }, + } + ] + attach_log_entry(self, message, action_flag=DELETION) + + +class TrackChangesMixin(RecordDeletionMixin, models.Model): """Tracks changes of Django models. Tracks which fields have changed in the save() function, so that diff --git a/extensions/fields.py b/extensions/fields.py index 5fdbe6be..06707d84 100644 --- a/extensions/fields.py +++ b/extensions/fields.py @@ -22,7 +22,7 @@ class VersionStringField(SemanticVersionField): return str(value) def value_to_string(self, obj): - value = self._get_val_from_obj(obj) + value = self.value_from_object(obj) return self.get_prep_value(value) def from_json(self, json_str): diff --git a/extensions/models.py b/extensions/models.py index bee30ff5..a29c8479 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 +from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin, TrackChangesMixin from constants.base import ( AUTHOR_ROLE_CHOICES, AUTHOR_ROLE_DEV, @@ -651,7 +651,7 @@ class Maintainer(CreatedModifiedMixin, models.Model): ] -class Preview(CreatedModifiedMixin, models.Model): +class Preview(CreatedModifiedMixin, RecordDeletionMixin, models.Model): extension = models.ForeignKey(Extension, on_delete=models.CASCADE) file = models.ForeignKey( 'files.File', related_name='extension_preview', on_delete=models.CASCADE diff --git a/extensions/signals.py b/extensions/signals.py index 6ba69b8d..85ae6dbd 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -18,15 +18,14 @@ 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)) +def _log_deletion( + sender: object, + instance: Union[ + extensions.models.Extension, extensions.models.Version, extensions.models.Preview + ], + **kwargs: object, +) -> None: + instance.record_deletion() @receiver(post_delete, sender=extensions.models.Preview) diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index c1e64eb4..c4c54f2c 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,16 +1,21 @@ -from django.test import TestCase +import json + +from django.contrib.admin.models import LogEntry, DELETION +from django.test import TestCase # , TransactionTestCase 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 +from common.tests.factories.users import UserFactory, create_moderator import extensions.models import files.models +import reviewers.models class DeleteTest(TestCase): fixtures = ['dev', 'licenses'] def test_unlisted_unrated_extension_can_be_deleted_by_author(self): + self.maxDiff = None version = create_version( file__status=files.models.File.STATUSES.AWAITING_REVIEW, ratings=[], @@ -29,6 +34,31 @@ class DeleteTest(TestCase): self.assertEqual(extension.cannot_be_deleted_reasons, []) preview_file = extension.previews.first() self.assertIsNotNone(preview_file) + # Create some ApprovalActivity as well + moderator = create_moderator() + approval_activity = reviewers.models.ApprovalActivity.objects.create( + extension=extension, + user=moderator, + message='This is a message in approval activity', + ) + # Create a file validation record + file_validation = files.models.FileValidation.objects.create( + file=version_file, results={'deadbeef': 'foobar'} + ) + object_reprs = list( + map( + repr, + [ + preview_file, + version_file, + file_validation, + extension, + approval_activity, + preview_file.extension_preview.first(), + version, + ], + ) + ) url = extension.get_delete_url() user = extension.authors.first() @@ -49,6 +79,47 @@ 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()) + + # Check that each of the deleted records was logged + deletion_log_entries_q = LogEntry.objects.filter(action_flag=DELETION) + self.assertEqual(deletion_log_entries_q.count(), 7) + self.assertEqual( + [_.object_repr for _ in deletion_log_entries_q], + object_reprs, + ) + log_entry = deletion_log_entries_q.filter(object_repr__contains='Extension').first() + change_message_data = json.loads(log_entry.change_message) + self.assertEqual( + change_message_data[0]['deleted']['object'], f'' + ) + self.assertEqual( + set(change_message_data[0]['deleted']['old_state'].keys()), + { + 'average_score', + 'date_approved', + 'date_created', + 'date_modified', + 'date_status_changed', + 'description', + 'download_count', + 'extension_id', + 'is_listed', + 'name', + 'pk', + 'slug', + 'status', + 'support', + 'team', + 'type', + 'view_count', + 'website', + }, + ) + self.assertEqual( + log_entry.get_change_message(), + f'Deleted extension “”.', + ) + # 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): diff --git a/files/signals.py b/files/signals.py index 41fe8a05..fceb3086 100644 --- a/files/signals.py +++ b/files/signals.py @@ -36,5 +36,6 @@ def _scan_new_file( @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) +@receiver(pre_delete, sender=files.models.FileValidation) +def _log_deletion(sender: object, instance: files.models.File, **kwargs: object) -> None: + instance.record_deletion() diff --git a/ratings/signals.py b/ratings/signals.py index 5ff6c271..933900ee 100644 --- a/ratings/signals.py +++ b/ratings/signals.py @@ -1,5 +1,5 @@ from actstream import action -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 from constants.activity import Verb @@ -40,3 +40,8 @@ def _create_action_from_rating( action_object=instance, target=instance.extension, ) + + +@receiver(pre_delete, sender=Rating) +def _log_deletion(sender: object, instance: Rating, **kwargs: object) -> None: + instance.record_deletion() diff --git a/reviewers/models.py b/reviewers/models.py index e8377a83..3daacd78 100644 --- a/reviewers/models.py +++ b/reviewers/models.py @@ -9,7 +9,7 @@ from django.utils.translation import gettext_lazy as _ import common.help_texts from extensions.models import Extension -from common.model_mixins import CreatedModifiedMixin +from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin from utils import absolutify, send_mail from constants.base import EXTENSION_TYPE_CHOICES @@ -74,7 +74,7 @@ class ReviewerSubscription(CreatedModifiedMixin, models.Model): ) -class ApprovalActivity(CreatedModifiedMixin, models.Model): +class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model): class ActivityType(models.TextChoices): COMMENT = "COM", _("Comment") APPROVED = "APR", _("Approved") diff --git a/reviewers/signals.py b/reviewers/signals.py index 4e143c05..1e1b6a06 100644 --- a/reviewers/signals.py +++ b/reviewers/signals.py @@ -1,6 +1,6 @@ from actstream import action from actstream.actions import follow -from django.db.models.signals import post_save +from django.db.models.signals import post_save, pre_delete from django.dispatch import receiver from constants.activity import Flag, Verb @@ -37,3 +37,8 @@ def _create_action_from_review_and_follow( action_object=instance, target=instance.extension, ) + + +@receiver(pre_delete, sender=ApprovalActivity) +def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None: + instance.record_deletion()