From d4a9022628079faaad98bfa120e195f6d7f9bfba Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 19 Apr 2024 15:38:37 +0200 Subject: [PATCH 1/3] Generic way of logging/archiving deleted data --- abuse/signals.py | 7 ++- common/model_mixins.py | 33 ++++++++++++++- extensions/fields.py | 2 +- extensions/models.py | 4 +- extensions/signals.py | 17 ++++---- extensions/tests/test_delete.py | 75 ++++++++++++++++++++++++++++++++- files/signals.py | 5 ++- ratings/signals.py | 7 ++- reviewers/models.py | 4 +- reviewers/signals.py | 7 ++- teams/models.py | 10 +++-- teams/signals.py | 12 ++++++ 12 files changed, 158 insertions(+), 25 deletions(-) create mode 100644 teams/signals.py 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 af3d6519..b5ceb2e1 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 common.templatetags.common import absolutify from utils import send_mail @@ -75,7 +75,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() diff --git a/teams/models.py b/teams/models.py index eb3f3e2b..f62dd52a 100644 --- a/teams/models.py +++ b/teams/models.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.db import models from django.urls import reverse -from common.model_mixins import CreatedModifiedMixin +from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin from constants.base import ( TEAM_ROLE_CHOICES, TEAM_ROLE_MANAGER, @@ -16,7 +16,9 @@ User = get_user_model() logger = logging.getLogger(__name__) -class Team(CreatedModifiedMixin, models.Model): +class Team(CreatedModifiedMixin, TrackChangesMixin, models.Model): + track_changes_to_fields = {'name'} + slug = models.SlugField(unique=True, null=False, blank=False, editable=True) name = models.CharField(max_length=128, null=False, blank=False) users = models.ManyToManyField(User, through='TeamsUsers', related_name='teams') @@ -37,7 +39,9 @@ class Team(CreatedModifiedMixin, models.Model): return reverse('extensions:by-team', kwargs={'team_slug': self.slug}) -class TeamsUsers(CreatedModifiedMixin, models.Model): +class TeamsUsers(CreatedModifiedMixin, TrackChangesMixin, models.Model): + track_changes_to_fields = {'role'} + class Meta: verbose_name = 'Team member' verbose_name_plural = 'Team members' diff --git a/teams/signals.py b/teams/signals.py new file mode 100644 index 00000000..559826d1 --- /dev/null +++ b/teams/signals.py @@ -0,0 +1,12 @@ +from typing import Union + +from django.db.models.signals import pre_delete +from django.dispatch import receiver + +from teams.models import Team, TeamUser + + +@receiver(pre_delete, sender=Team) +@receiver(pre_delete, sender=TeamUser) +def _log_deletion(sender: object, instance: Union[Team, TeamUser], **kwargs: object) -> None: + instance.record_deletion() -- 2.30.2 From a6628a6cb508bffde6d7af45cdf434c872a061b4 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 19 Apr 2024 15:49:41 +0200 Subject: [PATCH 2/3] Don't change teams yet --- teams/models.py | 10 +++------- teams/signals.py | 12 ------------ 2 files changed, 3 insertions(+), 19 deletions(-) delete mode 100644 teams/signals.py diff --git a/teams/models.py b/teams/models.py index f62dd52a..eb3f3e2b 100644 --- a/teams/models.py +++ b/teams/models.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.db import models from django.urls import reverse -from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin +from common.model_mixins import CreatedModifiedMixin from constants.base import ( TEAM_ROLE_CHOICES, TEAM_ROLE_MANAGER, @@ -16,9 +16,7 @@ User = get_user_model() logger = logging.getLogger(__name__) -class Team(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'name'} - +class Team(CreatedModifiedMixin, models.Model): slug = models.SlugField(unique=True, null=False, blank=False, editable=True) name = models.CharField(max_length=128, null=False, blank=False) users = models.ManyToManyField(User, through='TeamsUsers', related_name='teams') @@ -39,9 +37,7 @@ class Team(CreatedModifiedMixin, TrackChangesMixin, models.Model): return reverse('extensions:by-team', kwargs={'team_slug': self.slug}) -class TeamsUsers(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'role'} - +class TeamsUsers(CreatedModifiedMixin, models.Model): class Meta: verbose_name = 'Team member' verbose_name_plural = 'Team members' diff --git a/teams/signals.py b/teams/signals.py deleted file mode 100644 index 559826d1..00000000 --- a/teams/signals.py +++ /dev/null @@ -1,12 +0,0 @@ -from typing import Union - -from django.db.models.signals import pre_delete -from django.dispatch import receiver - -from teams.models import Team, TeamUser - - -@receiver(pre_delete, sender=Team) -@receiver(pre_delete, sender=TeamUser) -def _log_deletion(sender: object, instance: Union[Team, TeamUser], **kwargs: object) -> None: - instance.record_deletion() -- 2.30.2 From 208f47be5d0ba3dc1782b0571092e1ab187fccf2 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 19 Apr 2024 16:06:48 +0200 Subject: [PATCH 3/3] Add tblib to support --parallel test runner --- requirements_dev.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements_dev.txt b/requirements_dev.txt index 67313f48..e589676c 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -14,5 +14,6 @@ pre-commit==2.20.0 python-dateutil==2.8.2 PyYAML==6.0 responses==0.21.0 +tblib==3.0.0 toml==0.10.2 virtualenv==20.16.2 -- 2.30.2