From 9148f36719f31f21e8cee8419ea69d1c16010f16 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 22 Apr 2021 18:16:00 -0700 Subject: [PATCH 1/4] Fixes duplicate boosts --- bookwyrm/models/activitypub_mixin.py | 6 +++++- bookwyrm/tests/views/test_interaction.py | 27 ++++++++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 82a45ac86..2e248fbe3 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -204,7 +204,11 @@ class ObjectMixin(ActivitypubMixin): created = created or not bool(self.id) # first off, we want to save normally no matter what super().save(*args, **kwargs) - if not broadcast: + if ( + not broadcast + or hasattr(self, "status_type") + and self.status_type == "Announce" + ): return # this will work for objects owned by a user (lists, shelves) diff --git a/bookwyrm/tests/views/test_interaction.py b/bookwyrm/tests/views/test_interaction.py index 8d2c63ffc..f767edfaa 100644 --- a/bookwyrm/tests/views/test_interaction.py +++ b/bookwyrm/tests/views/test_interaction.py @@ -1,4 +1,5 @@ """ test for app action functionality """ +import json from unittest.mock import patch from django.test import TestCase from django.test.client import RequestFactory @@ -39,7 +40,7 @@ class InteractionViews(TestCase): parent_work=work, ) - def test_handle_favorite(self, _): + def test_favorite(self, _): """ create and broadcast faving a status """ view = views.Favorite.as_view() request = self.factory.post("") @@ -57,7 +58,7 @@ class InteractionViews(TestCase): self.assertEqual(notification.user, self.local_user) self.assertEqual(notification.related_user, self.remote_user) - def test_handle_unfavorite(self, _): + def test_unfavorite(self, _): """ unfav a status """ view = views.Unfavorite.as_view() request = self.factory.post("") @@ -74,7 +75,7 @@ class InteractionViews(TestCase): self.assertEqual(models.Favorite.objects.count(), 0) self.assertEqual(models.Notification.objects.count(), 0) - def test_handle_boost(self, _): + def test_boost(self, _): """ boost a status """ view = views.Boost.as_view() request = self.factory.post("") @@ -85,6 +86,7 @@ class InteractionViews(TestCase): view(request, status.id) boost = models.Boost.objects.get() + self.assertEqual(boost.boosted_status, status) self.assertEqual(boost.user, self.remote_user) self.assertEqual(boost.privacy, "public") @@ -95,7 +97,7 @@ class InteractionViews(TestCase): self.assertEqual(notification.related_user, self.remote_user) self.assertEqual(notification.related_status, status) - def test_handle_self_boost(self, _): + def test_self_boost(self, _): """ boost your own status """ view = views.Boost.as_view() request = self.factory.post("") @@ -103,7 +105,14 @@ class InteractionViews(TestCase): with patch("bookwyrm.activitystreams.ActivityStream.add_status"): status = models.Status.objects.create(user=self.local_user, content="hi") - view(request, status.id) + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.delay" + ) as broadcast_mock: + view(request, status.id) + + self.assertEqual(broadcast_mock.call_count, 1) + activity = json.loads(broadcast_mock.call_args[0][1]) + self.assertEqual(activity["type"], "Announce") boost = models.Boost.objects.get() self.assertEqual(boost.boosted_status, status) @@ -112,7 +121,7 @@ class InteractionViews(TestCase): self.assertFalse(models.Notification.objects.exists()) - def test_handle_boost_unlisted(self, _): + def test_boost_unlisted(self, _): """ boost a status """ view = views.Boost.as_view() request = self.factory.post("") @@ -127,7 +136,7 @@ class InteractionViews(TestCase): boost = models.Boost.objects.get() self.assertEqual(boost.privacy, "unlisted") - def test_handle_boost_private(self, _): + def test_boost_private(self, _): """ boost a status """ view = views.Boost.as_view() request = self.factory.post("") @@ -140,7 +149,7 @@ class InteractionViews(TestCase): view(request, status.id) self.assertFalse(models.Boost.objects.exists()) - def test_handle_boost_twice(self, _): + def test_boost_twice(self, _): """ boost a status """ view = views.Boost.as_view() request = self.factory.post("") @@ -152,7 +161,7 @@ class InteractionViews(TestCase): view(request, status.id) self.assertEqual(models.Boost.objects.count(), 1) - def test_handle_unboost(self, _): + def test_unboost(self, _): """ undo a boost """ view = views.Unboost.as_view() request = self.factory.post("") From b457446f2ff69f850e927ae40e4bb025c3d9c260 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 22 Apr 2021 19:36:27 -0700 Subject: [PATCH 2/4] Don't save duplicate boosts --- bookwyrm/models/activitypub_mixin.py | 6 ++---- bookwyrm/models/status.py | 8 ++++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 2e248fbe3..02cfafc0e 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -204,10 +204,8 @@ class ObjectMixin(ActivitypubMixin): created = created or not bool(self.id) # first off, we want to save normally no matter what super().save(*args, **kwargs) - if ( - not broadcast - or hasattr(self, "status_type") - and self.status_type == "Announce" + if not broadcast or ( + hasattr(self, "status_type") and self.status_type == "Announce" ): return diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 360288e93..65bf71d28 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -351,6 +351,14 @@ class Boost(ActivityMixin, Status): def save(self, *args, **kwargs): """ save and notify """ + # This constraint can't work as it would cross tables. + # class Meta: + # unique_together = ('user', 'boosted_status') + if Boost.objects.filter( + boosted_status=self.boosted_status, user=self.user + ).exists(): + return + super().save(*args, **kwargs) if not self.boosted_status.user.local or self.boosted_status.user == self.user: return From 32e694032b2f4d24623da0d25db1e7744efc08c8 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 23 Apr 2021 10:49:17 -0700 Subject: [PATCH 3/4] Fixes duplicate boost model verification --- bookwyrm/models/status.py | 2 +- .../tests/views/inbox/test_inbox_announce.py | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 65bf71d28..3784680fb 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -356,7 +356,7 @@ class Boost(ActivityMixin, Status): # unique_together = ('user', 'boosted_status') if Boost.objects.filter( boosted_status=self.boosted_status, user=self.user - ).exists(): + ).exclude(id=self.id).exists(): return super().save(*args, **kwargs) diff --git a/bookwyrm/tests/views/inbox/test_inbox_announce.py b/bookwyrm/tests/views/inbox/test_inbox_announce.py index 954d4e647..dbe4ec568 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_announce.py +++ b/bookwyrm/tests/views/inbox/test_inbox_announce.py @@ -51,7 +51,7 @@ class InboxActivities(TestCase): models.SiteSettings.objects.create() @patch("bookwyrm.activitystreams.ActivityStream.add_status") - def test_handle_boost(self, _): + def test_boost(self, redis_mock): """ boost a status """ self.assertEqual(models.Notification.objects.count(), 0) activity = { @@ -66,16 +66,23 @@ class InboxActivities(TestCase): with patch("bookwyrm.models.status.Status.ignore_activity") as discarder: discarder.return_value = False views.inbox.activity_task(activity) + + # boost added to redis activitystreams + self.assertTrue(redis_mock.called) + + # boost created of correct status boost = models.Boost.objects.get() self.assertEqual(boost.boosted_status, self.status) + + # notification sent to original poster notification = models.Notification.objects.get() self.assertEqual(notification.user, self.local_user) self.assertEqual(notification.related_status, self.status) @responses.activate @patch("bookwyrm.activitystreams.ActivityStream.add_status") - def test_handle_boost_remote_status(self, redis_mock): - """ boost a status """ + def test_boost_remote_status(self, redis_mock): + """ boost a status from a remote server """ work = models.Work.objects.create(title="work title") book = models.Edition.objects.create( title="Test", @@ -123,7 +130,7 @@ class InboxActivities(TestCase): self.assertEqual(boost.boosted_status.comment.book, book) @responses.activate - def test_handle_discarded_boost(self): + def test_discarded_boost(self): """ test a boost of a mastodon status that will be discarded """ status = models.Status( content="hi", @@ -146,7 +153,7 @@ class InboxActivities(TestCase): views.inbox.activity_task(activity) self.assertEqual(models.Boost.objects.count(), 0) - def test_handle_unboost(self): + def test_unboost(self): """ undo a boost """ with patch("bookwyrm.activitystreams.ActivityStream.add_status"): boost = models.Boost.objects.create( @@ -175,7 +182,7 @@ class InboxActivities(TestCase): self.assertTrue(redis_mock.called) self.assertFalse(models.Boost.objects.exists()) - def test_handle_unboost_unknown_boost(self): + def test_unboost_unknown_boost(self): """ undo a boost """ activity = { "type": "Undo", From 79424f7bfb6b4315c440a95b83ec0ab5eb115f0c Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 23 Apr 2021 10:56:17 -0700 Subject: [PATCH 4/4] Python formatting --- bookwyrm/models/status.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 3784680fb..0dee0b756 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -354,9 +354,11 @@ class Boost(ActivityMixin, Status): # This constraint can't work as it would cross tables. # class Meta: # unique_together = ('user', 'boosted_status') - if Boost.objects.filter( - boosted_status=self.boosted_status, user=self.user - ).exclude(id=self.id).exists(): + if ( + Boost.objects.filter(boosted_status=self.boosted_status, user=self.user) + .exclude(id=self.id) + .exists() + ): return super().save(*args, **kwargs)