From ee6e3ed7eb3ef6be989cd4bd594901d68be7b8a0 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 20:25:51 -0800 Subject: [PATCH] Adds a database field for is_deleted on user --- .../migrations/0183_auto_20231105_1607.py | 22 +++------ .../migrations/0184_auto_20231106_0421.py | 49 +++++++++++++++++++ bookwyrm/models/user.py | 34 +++++-------- .../templates/snippets/user_active_tag.html | 2 +- .../migrations/{test_0183.py => test_0184.py} | 17 +++---- bookwyrm/tests/models/test_user_model.py | 37 -------------- 6 files changed, 75 insertions(+), 86 deletions(-) create mode 100644 bookwyrm/migrations/0184_auto_20231106_0421.py rename bookwyrm/tests/migrations/{test_0183.py => test_0184.py} (92%) diff --git a/bookwyrm/migrations/0183_auto_20231105_1607.py b/bookwyrm/migrations/0183_auto_20231105_1607.py index 2716a0737..0c8376adc 100644 --- a/bookwyrm/migrations/0183_auto_20231105_1607.py +++ b/bookwyrm/migrations/0183_auto_20231105_1607.py @@ -1,18 +1,6 @@ # Generated by Django 3.2.20 on 2023-11-05 16:07 -from django.db import migrations -from bookwyrm.models import User - - -def erase_deleted_user_data(apps, schema_editor): - """Retroactively clear user data""" - for user in User.get_permanently_deleted_users(): - user.erase_user_data() - user.save( - broadcast=False, - update_fields=["email", "avatar", "preview_image", "summary", "name"], - ) - user.erase_user_statuses(broadcast=False) +from django.db import migrations, models class Migration(migrations.Migration): @@ -22,7 +10,9 @@ class Migration(migrations.Migration): ] operations = [ - migrations.RunPython( - erase_deleted_user_data, reverse_code=migrations.RunPython.noop - ) + migrations.AddField( + model_name="user", + name="is_deleted", + field=models.BooleanField(default=False), + ), ] diff --git a/bookwyrm/migrations/0184_auto_20231106_0421.py b/bookwyrm/migrations/0184_auto_20231106_0421.py new file mode 100644 index 000000000..e8197dea1 --- /dev/null +++ b/bookwyrm/migrations/0184_auto_20231106_0421.py @@ -0,0 +1,49 @@ +# Generated by Django 3.2.20 on 2023-11-06 04:21 + +from django.db import migrations +from bookwyrm.models import User + + +def update_deleted_users(apps, schema_editor): + """Find all the users who are deleted, not just inactive, and set deleted""" + users = apps.get_model("bookwyrm", "User") + db_alias = schema_editor.connection.alias + users.objects.using(db_alias).filter( + is_active=False, + deactivation_reason__in=[ + "self_deletion", + "moderator_deletion", + ], + ).update(is_deleted=True) + + # differente rules for remote users + users.objects.using(db_alias).filter(is_active=False, local=False,).exclude( + deactivation_reason="moderator_deactivation", + ).update(is_deleted=True) + + +def erase_deleted_user_data(apps, schema_editor): + """Retroactively clear user data""" + for user in User.objects.filter(is_deleted=True): + user.erase_user_data() + user.save( + broadcast=False, + update_fields=["email", "avatar", "preview_image", "summary", "name"], + ) + user.erase_user_statuses(broadcast=False) + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookwyrm", "0183_auto_20231105_1607"), + ] + + operations = [ + migrations.RunPython( + update_deleted_users, reverse_code=migrations.RunPython.noop + ), + migrations.RunPython( + erase_deleted_user_data, reverse_code=migrations.RunPython.noop + ), + ] diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 19006f772..75ca1d527 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -8,7 +8,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.postgres.fields import ArrayField, CICharField from django.core.exceptions import PermissionDenied, ObjectDoesNotExist from django.dispatch import receiver -from django.db import models, transaction +from django.db import models, transaction, IntegrityError from django.utils import timezone from django.utils.translation import gettext_lazy as _ from model_utils import FieldTracker @@ -54,6 +54,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): username = fields.UsernameField() email = models.EmailField(unique=True, null=True) + is_deleted = models.BooleanField(default=False) key_pair = fields.OneToOneField( "KeyPair", @@ -263,17 +264,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): is_active=True, ).distinct() - @classmethod - def get_permanently_deleted_users(cls): - """a list of users who are permanently deleted""" - return cls.objects.filter( - is_active=False, - deactivation_reason__in=[ - "self_deletion", - "moderator_deletion", - ], - ).distinct() - def update_active_date(self): """this user is here! they are doing things!""" self.last_active_date = timezone.now() @@ -407,6 +397,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): # pylint: disable=attribute-defined-outside-init self.is_active = False self.allow_reactivation = False + self.is_deleted = True self.erase_user_data() self.erase_user_statuses() @@ -419,6 +410,11 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_data(self): """Wipe a user's custom data""" + if not self.is_deleted: + raise IntegrityError( + "Trying to erase user data on user that is not deleted" + ) + # mangle email address self.email = f"{uuid4()}@deleted.user" @@ -431,18 +427,14 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_statuses(self, broadcast=True): """Wipe the data on all the user's statuses""" + if not self.is_deleted: + raise IntegrityError( + "Trying to erase user data on user that is not deleted" + ) + for status in self.status_set.all(): status.delete(broadcast=broadcast) - @property - def is_permanently_deleted(self): - """is this user inactive, or really truly deleted?""" - return not self.is_active and self.deactivation_reason in [ - "self_deletion", - "moderator_deletion", - "activitypub_deletion", - ] - def deactivate(self): """Disable the user but allow them to reactivate""" # pylint: disable=attribute-defined-outside-init diff --git a/bookwyrm/templates/snippets/user_active_tag.html b/bookwyrm/templates/snippets/user_active_tag.html index c3f067b43..1d85ae68b 100644 --- a/bookwyrm/templates/snippets/user_active_tag.html +++ b/bookwyrm/templates/snippets/user_active_tag.html @@ -8,7 +8,7 @@ {% trans "Active" as text %} {% include "snippets/user_active_tag_item.html" with icon="check" text=text level="success" %} {% endif %} -{% elif user.is_permanently_deleted %} +{% elif user.is_deleted %} {% trans "Deleted" as text %} {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="danger" deactivation_reason=user.get_deactivation_reason_display %} {% else %} diff --git a/bookwyrm/tests/migrations/test_0183.py b/bookwyrm/tests/migrations/test_0184.py similarity index 92% rename from bookwyrm/tests/migrations/test_0183.py rename to bookwyrm/tests/migrations/test_0184.py index 8c8f27d5a..4bf1b66c9 100644 --- a/bookwyrm/tests/migrations/test_0183.py +++ b/bookwyrm/tests/migrations/test_0184.py @@ -1,23 +1,20 @@ """ testing migrations """ -import json from unittest.mock import patch -from django.apps import apps from django.test import TestCase from django.db.migrations.executor import MigrationExecutor from django.db import connection -import responses from bookwyrm import models from bookwyrm.management.commands import initdb -from bookwyrm.settings import USE_HTTPS, DOMAIN +from bookwyrm.settings import DOMAIN # pylint: disable=missing-class-docstring # pylint: disable=missing-function-docstring class EraseDeletedUserDataMigration(TestCase): - migrate_from = "0182_auto_20231027_1122" - migrate_to = "0183_auto_20231105_1607" + migrate_from = "0183_auto_20231105_1607" + migrate_to = "0184_auto_20231106_0421" # pylint: disable=invalid-name def setUp(self): @@ -68,11 +65,6 @@ class EraseDeletedUserDataMigration(TestCase): initdb.init_groups() initdb.init_permissions() - assert ( - self.migrate_from and self.migrate_to - ), "TestCase '{}' must define migrate_from and migrate_to properties".format( - type(self).__name__ - ) self.migrate_from = [("bookwyrm", self.migrate_from)] self.migrate_to = [("bookwyrm", self.migrate_to)] executor = MigrationExecutor(connection) @@ -104,12 +96,14 @@ class EraseDeletedUserDataMigration(TestCase): self.deleted_status.refresh_from_db() self.assertTrue(self.active_user.is_active) + self.assertFalse(self.active_user.is_deleted) self.assertEqual(self.active_user.name, "a name") self.assertNotEqual(self.deleted_user.email, "activeuser@activeuser.activeuser") self.assertFalse(self.active_status.deleted) self.assertEqual(self.active_status.content, "don't delete me") self.assertFalse(self.inactive_user.is_active) + self.assertFalse(self.inactive_user.is_deleted) self.assertEqual(self.inactive_user.name, "name name") self.assertNotEqual( self.deleted_user.email, "inactiveuser@inactiveuser.inactiveuser" @@ -118,6 +112,7 @@ class EraseDeletedUserDataMigration(TestCase): self.assertEqual(self.inactive_status.content, "also don't delete me") self.assertFalse(self.deleted_user.is_active) + self.assertTrue(self.deleted_user.is_deleted) self.assertIsNone(self.deleted_user.name) self.assertNotEqual( self.deleted_user.email, "deleteduser@deleteduser.deleteduser" diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 3576417de..30d7918c0 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -325,40 +325,3 @@ class User(TestCase): results = models.User.admins() self.assertEqual(results.count(), 1) self.assertEqual(results.first(), self.user) - - def test_get_permanently_deleted_users(self): - - with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( - "bookwyrm.activitystreams.populate_stream_task.delay" - ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): - models.User.objects.create_user( - f"activeuser@{DOMAIN}", - "activeuser@activeuser.activeuser", - "activeuserword", - local=True, - localname="active", - ) - models.User.objects.create_user( - f"deleteduser@{DOMAIN}", - "deleteduser@deleteduser.deleteduser", - "deleteduserword", - local=True, - localname="deleted", - is_active=False, - deactivation_reason="self_deletion", - ) - models.User.objects.create_user( - f"inactiveuser@{DOMAIN}", - "inactiveuser@inactiveuser.inactiveuser", - "inactiveuserword", - local=True, - localname="inactive", - is_active=False, - deactivation_reason="self_deactivation", - ) - - deleted_users = models.User.get_permanently_deleted_users() - - self.assertTrue(deleted_users.filter(localname="deleted").exists()) - self.assertFalse(deleted_users.filter(localname="active").exists()) - self.assertFalse(deleted_users.filter(localname="inactive").exists())