diff --git a/bookwyrm/activitypub/verbs.py b/bookwyrm/activitypub/verbs.py index 442e38583..c2cbfea31 100644 --- a/bookwyrm/activitypub/verbs.py +++ b/bookwyrm/activitypub/verbs.py @@ -16,11 +16,10 @@ class Verb(ActivityObject): def action(self): """ usually we just want to update and save """ - obj = self.object - # it may return None if the object is invalid in an expected way + # self.object may return None if the object is invalid in an expected way # ie, Question type - if obj: - obj.to_model() + if self.object: + self.object.to_model() @dataclass(init=False) @@ -43,7 +42,16 @@ class Delete(Verb): def action(self): """ find and delete the activity object """ - obj = self.object.to_model(save=False, allow_create=False) + if not self.object: + return + + if isinstance(self.object, str): + # Deleted users are passed as strings. Not wild about this fix + model = apps.get_model("bookwyrm.User") + obj = model.find_existing_by_remote_id(self.object) + else: + obj = self.object.to_model(save=False, allow_create=False) + if obj: obj.delete() # if we can't find it, we don't need to delete it because we don't have it @@ -58,8 +66,7 @@ class Update(Verb): def action(self): """ update a model instance from the dataclass """ - obj = self.object - if obj: + if self.object: self.object.to_model(allow_create=False) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index c6ba249c5..ce6245b21 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -241,7 +241,9 @@ class ObjectMixin(ActivitypubMixin): return # is this a deletion? - if hasattr(self, "deleted") and self.deleted: + if (hasattr(self, "deleted") and self.deleted) or ( + hasattr(self, "is_active") and not self.is_active + ): activity = self.to_delete_activity(user) else: activity = self.to_update_activity(user) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 15ceb19bd..9585a4c4a 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -205,6 +205,9 @@ class User(OrderedCollectionPageMixin, AbstractUser): def to_activity(self, **kwargs): """override default AP serializer to add context object idk if this is the best way to go about this""" + if not self.is_active: + return self.remote_id + activity_object = super().to_activity(**kwargs) activity_object["@context"] = [ "https://www.w3.org/ns/activitystreams", @@ -283,6 +286,12 @@ class User(OrderedCollectionPageMixin, AbstractUser): editable=False, ).save(broadcast=False) + def delete(self, *args, **kwargs): + """ deactivate rather than delete a user """ + self.is_active = False + # skip the logic in this class's save() + super().save(*args, **kwargs) + @property def local_path(self): """ this model doesn't inherit bookwyrm model, so here we are """ diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index bd5255cef..883ef669e 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -1,4 +1,5 @@ """ testing models """ +import json from unittest.mock import patch from django.test import TestCase import responses @@ -152,3 +153,17 @@ class User(TestCase): self.assertEqual(server.server_name, DOMAIN) self.assertIsNone(server.application_type) self.assertIsNone(server.application_version) + + def test_delete_user(self): + """ deactivate a user """ + self.assertTrue(self.user.is_active) + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.delay" + ) as broadcast_mock: + self.user.delete() + + self.assertEqual(broadcast_mock.call_count, 1) + activity = json.loads(broadcast_mock.call_args[0][1]) + self.assertEqual(activity["type"], "Delete") + self.assertEqual(activity["object"], self.user.remote_id) + self.assertFalse(self.user.is_active) diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 65a754266..03598b88d 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/bookwyrm/tests/views/inbox/test_inbox_delete.py @@ -49,7 +49,7 @@ class InboxActivities(TestCase): } models.SiteSettings.objects.create() - def test_handle_delete_status(self): + def test_delete_status(self): """ remove a status """ self.assertFalse(self.status.deleted) activity = { @@ -70,7 +70,7 @@ class InboxActivities(TestCase): self.assertTrue(status.deleted) self.assertIsInstance(status.deleted_date, datetime) - def test_handle_delete_status_notifications(self): + def test_delete_status_notifications(self): """ remove a status with related notifications """ models.Notification.objects.create( related_status=self.status, @@ -104,3 +104,34 @@ class InboxActivities(TestCase): # notifications should be truly deleted self.assertEqual(models.Notification.objects.count(), 1) self.assertEqual(models.Notification.objects.get(), notif) + + def test_delete_user(self): + """ delete a user """ + self.assertTrue(models.User.objects.get(username="rat@example.com").is_active) + activity = { + "@context": "https://www.w3.org/ns/activitystreams", + "id": "https://example.com/users/test-user#delete", + "type": "Delete", + "actor": "https://example.com/users/test-user", + "to": ["https://www.w3.org/ns/activitystreams#Public"], + "object": self.remote_user.remote_id, + } + + views.inbox.activity_task(activity) + self.assertFalse(models.User.objects.get(username="rat@example.com").is_active) + + def test_delete_user_unknown(self): + """ don't worry about it if we don't know the user """ + self.assertEqual(models.User.objects.filter(is_active=True).count(), 2) + activity = { + "@context": "https://www.w3.org/ns/activitystreams", + "id": "https://example.com/users/test-user#delete", + "type": "Delete", + "actor": "https://example.com/users/test-user", + "to": ["https://www.w3.org/ns/activitystreams#Public"], + "object": "https://example.com/users/test-user", + } + + # nothing happens. + views.inbox.activity_task(activity) + self.assertEqual(models.User.objects.filter(is_active=True).count(), 2)