From aefb0c9b14e5ff24c120d48a2371546d86edafc7 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Thu, 24 Mar 2022 12:43:01 -0700 Subject: [PATCH 1/5] Re-consider list privacy on edit Please run ci?? --- bookwyrm/lists_stream.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bookwyrm/lists_stream.py b/bookwyrm/lists_stream.py index f6a35cc25..c40d2b534 100644 --- a/bookwyrm/lists_stream.py +++ b/bookwyrm/lists_stream.py @@ -115,9 +115,12 @@ class ListsStream(RedisStore): @receiver(signals.post_save, sender=models.List) # pylint: disable=unused-argument def add_list_on_create(sender, instance, created, *args, **kwargs): - """add newly created lists streamsstreams""" + """add newly created lists streams""" if not created: + # the privacy may have changed, so we need to re-do the whole thing + remove_list_task.delay(instance.id, re_add=True) return + # when creating new things, gotta wait on the transaction transaction.on_commit(lambda: add_list_on_create_command(instance.id)) @@ -217,7 +220,7 @@ def populate_lists_task(user_id): @app.task(queue=MEDIUM) -def remove_list_task(list_id): +def remove_list_task(list_id, re_add=False): """remove a list from any stream it might be in""" stores = models.User.objects.filter(local=True, is_active=True).values_list( "id", flat=True @@ -227,6 +230,9 @@ def remove_list_task(list_id): stores = [ListsStream().stream_id(idx) for idx in stores] ListsStream().remove_object_from_related_stores(list_id, stores=stores) + if re_add: + add_list_task.delay(list_id) + @app.task(queue=HIGH) def add_list_task(list_id): From 3ad0a5d07357d60597667605ec12e43f7606b90a Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 2 Jul 2022 13:06:29 -0700 Subject: [PATCH 2/5] Use update_fields to limit `remove_list_task`s If we know what fields were updated, we can avoid running this task. This also adds some mocks where they are needed for the list view. --- bookwyrm/lists_stream.py | 17 +++++++++++------ bookwyrm/models/list.py | 4 ++-- bookwyrm/tests/views/lists/test_curate.py | 4 +++- bookwyrm/tests/views/lists/test_embed.py | 4 +++- bookwyrm/tests/views/lists/test_list.py | 14 ++++++++------ bookwyrm/tests/views/lists/test_list_item.py | 4 +++- bookwyrm/tests/views/lists/test_lists.py | 18 +++++++++++++----- 7 files changed, 43 insertions(+), 22 deletions(-) diff --git a/bookwyrm/lists_stream.py b/bookwyrm/lists_stream.py index c40d2b534..0977ad8c2 100644 --- a/bookwyrm/lists_stream.py +++ b/bookwyrm/lists_stream.py @@ -114,15 +114,20 @@ class ListsStream(RedisStore): @receiver(signals.post_save, sender=models.List) # pylint: disable=unused-argument -def add_list_on_create(sender, instance, created, *args, **kwargs): +def add_list_on_create(sender, instance, created, *args, update_fields=None, **kwargs): """add newly created lists streams""" - if not created: - # the privacy may have changed, so we need to re-do the whole thing - remove_list_task.delay(instance.id, re_add=True) + if created: + # when creating new things, gotta wait on the transaction + transaction.on_commit(lambda: add_list_on_create_command(instance.id)) return - # when creating new things, gotta wait on the transaction - transaction.on_commit(lambda: add_list_on_create_command(instance.id)) + # if update_fields was specified, we can check if privacy was updated, but if + # it wasn't specified (ie, by an activitypub update), there's no way to know + if update_fields and "privacy" not in update_fields: + return + + # the privacy may have changed, so we need to re-do the whole thing + remove_list_task.delay(instance.id, re_add=True) @receiver(signals.post_delete, sender=models.List) diff --git a/bookwyrm/models/list.py b/bookwyrm/models/list.py index ea524cc54..0195020e0 100644 --- a/bookwyrm/models/list.py +++ b/bookwyrm/models/list.py @@ -129,7 +129,7 @@ class List(OrderedCollectionMixin, BookWyrmModel): """on save, update embed_key and avoid clash with existing code""" if not self.embed_key: self.embed_key = uuid.uuid4() - return super().save(*args, **kwargs) + super().save(*args, **kwargs) class ListItem(CollectionItemMixin, BookWyrmModel): @@ -156,7 +156,7 @@ class ListItem(CollectionItemMixin, BookWyrmModel): super().save(*args, **kwargs) # tick the updated date on the parent list self.book_list.updated_date = timezone.now() - self.book_list.save(broadcast=False) + self.book_list.save(broadcast=False, update_fields=["updated_date"]) list_owner = self.book_list.user model = apps.get_model("bookwyrm.Notification", require_ready=True) diff --git a/bookwyrm/tests/views/lists/test_curate.py b/bookwyrm/tests/views/lists/test_curate.py index 9be8c2a15..9f3427b2c 100644 --- a/bookwyrm/tests/views/lists/test_curate.py +++ b/bookwyrm/tests/views/lists/test_curate.py @@ -36,7 +36,9 @@ class ListViews(TestCase): parent_work=work, ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): self.list = models.List.objects.create( name="Test List", user=self.local_user ) diff --git a/bookwyrm/tests/views/lists/test_embed.py b/bookwyrm/tests/views/lists/test_embed.py index dc61736d3..4191ffe0d 100644 --- a/bookwyrm/tests/views/lists/test_embed.py +++ b/bookwyrm/tests/views/lists/test_embed.py @@ -36,7 +36,9 @@ class ListViews(TestCase): parent_work=work, ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): self.list = models.List.objects.create( name="Test List", user=self.local_user ) diff --git a/bookwyrm/tests/views/lists/test_list.py b/bookwyrm/tests/views/lists/test_list.py index bcec0822f..98b0a461a 100644 --- a/bookwyrm/tests/views/lists/test_list.py +++ b/bookwyrm/tests/views/lists/test_list.py @@ -65,7 +65,9 @@ class ListViews(TestCase): parent_work=work_four, ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): self.list = models.List.objects.create( name="Test List", user=self.local_user ) @@ -244,7 +246,7 @@ class ListViews(TestCase): with patch( "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" - ) as mock: + ) as mock, patch("bookwyrm.lists_stream.remove_list_task.delay"): result = view(request, self.list.id) self.assertEqual(mock.call_count, 1) @@ -596,7 +598,7 @@ class ListViews(TestCase): def test_add_book_outsider(self): """put a book on a list""" self.list.curation = "open" - self.list.save(broadcast=False) + self.list.save(broadcast=False, update_fields=["curation"]) request = self.factory.post( "", { @@ -625,7 +627,7 @@ class ListViews(TestCase): def test_add_book_pending(self): """put a book on a list awaiting approval""" self.list.curation = "curated" - self.list.save(broadcast=False) + self.list.save(broadcast=False, update_fields=["curation"]) request = self.factory.post( "", { @@ -658,7 +660,7 @@ class ListViews(TestCase): def test_add_book_self_curated(self): """put a book on a list automatically approved""" self.list.curation = "curated" - self.list.save(broadcast=False) + self.list.save(broadcast=False, update_fields=["curation"]) request = self.factory.post( "", { @@ -687,7 +689,7 @@ class ListViews(TestCase): def test_add_book_permission_denied(self): """you can't add to that list""" self.list.curation = "closed" - self.list.save(broadcast=False) + self.list.save(broadcast=False, update_fields=["curation"]) request = self.factory.post( "", { diff --git a/bookwyrm/tests/views/lists/test_list_item.py b/bookwyrm/tests/views/lists/test_list_item.py index 50be3c286..b95282bef 100644 --- a/bookwyrm/tests/views/lists/test_list_item.py +++ b/bookwyrm/tests/views/lists/test_list_item.py @@ -32,7 +32,9 @@ class ListItemViews(TestCase): remote_id="https://example.com/book/1", parent_work=work, ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): self.list = models.List.objects.create( name="Test List", user=self.local_user ) diff --git a/bookwyrm/tests/views/lists/test_lists.py b/bookwyrm/tests/views/lists/test_lists.py index f65d2e4c2..c2263b933 100644 --- a/bookwyrm/tests/views/lists/test_lists.py +++ b/bookwyrm/tests/views/lists/test_lists.py @@ -39,7 +39,9 @@ class ListViews(TestCase): view = views.Lists.as_view() with patch( "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" - ), patch("bookwyrm.lists_stream.add_list_task.delay"): + ), patch("bookwyrm.lists_stream.add_list_task.delay"), patch( + "bookwyrm.lists_stream.remove_list_task.delay" + ): models.List.objects.create(name="Public list", user=self.local_user) models.List.objects.create( name="Private list", privacy="direct", user=self.local_user @@ -62,7 +64,9 @@ class ListViews(TestCase): def test_saved_lists_page(self): """there are so many views, this just makes sure it LOADS""" view = views.SavedLists.as_view() - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): booklist = models.List.objects.create( name="Public list", user=self.local_user ) @@ -82,7 +86,9 @@ class ListViews(TestCase): def test_saved_lists_page_empty(self): """there are so many views, this just makes sure it LOADS""" view = views.SavedLists.as_view() - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): models.List.objects.create(name="Public list", user=self.local_user) models.List.objects.create( name="Private list", privacy="direct", user=self.local_user @@ -108,7 +114,9 @@ class ListViews(TestCase): def test_user_lists_page(self): """there are so many views, this just makes sure it LOADS""" view = views.UserLists.as_view() - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): models.List.objects.create(name="Public list", user=self.local_user) models.List.objects.create( name="Private list", privacy="direct", user=self.local_user @@ -146,7 +154,7 @@ class ListViews(TestCase): request.user = self.local_user with patch( "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" - ) as mock: + ) as mock, patch("bookwyrm.lists_stream.remove_list_task.delay"): result = view(request) self.assertEqual(mock.call_count, 1) From 495bf203b073bc0bf8c7538ae74fc4ce239f1e74 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 2 Jul 2022 13:45:53 -0700 Subject: [PATCH 3/5] Mocks inbox and search tests --- bookwyrm/tests/views/inbox/test_inbox_remove.py | 4 +++- bookwyrm/tests/views/inbox/test_inbox_update.py | 7 +++++-- bookwyrm/tests/views/test_search.py | 4 +++- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/bookwyrm/tests/views/inbox/test_inbox_remove.py b/bookwyrm/tests/views/inbox/test_inbox_remove.py index 53288e0d3..d7b3f6778 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_remove.py +++ b/bookwyrm/tests/views/inbox/test_inbox_remove.py @@ -75,7 +75,9 @@ class InboxRemove(TestCase): def test_handle_remove_book_from_list(self): """listing a book""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): booklist = models.List.objects.create( name="test list", user=self.local_user, diff --git a/bookwyrm/tests/views/inbox/test_inbox_update.py b/bookwyrm/tests/views/inbox/test_inbox_update.py index 18b2e5d59..e8593c2be 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_update.py +++ b/bookwyrm/tests/views/inbox/test_inbox_update.py @@ -50,7 +50,9 @@ class InboxUpdate(TestCase): def test_update_list(self): """a new list""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="hi", remote_id="https://example.com/list/22", user=self.local_user ) @@ -69,7 +71,8 @@ class InboxUpdate(TestCase): "curation": "curated", "@context": "https://www.w3.org/ns/activitystreams", } - views.inbox.activity_task(activity) + with patch("bookwyrm.lists_stream.remove_list_task.delay"): + views.inbox.activity_task(activity) book_list.refresh_from_db() self.assertEqual(book_list.name, "Test List") self.assertEqual(book_list.curation, "curated") diff --git a/bookwyrm/tests/views/test_search.py b/bookwyrm/tests/views/test_search.py index 51166f2fa..d6e00edb6 100644 --- a/bookwyrm/tests/views/test_search.py +++ b/bookwyrm/tests/views/test_search.py @@ -140,7 +140,9 @@ class Views(TestCase): def test_search_lists(self): """searches remote connectors""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): booklist = models.List.objects.create( user=self.local_user, name="test list" ) From 46421f9672d73d8414d638cac391faa90b3c698a Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 2 Jul 2022 13:48:15 -0700 Subject: [PATCH 4/5] Mocks for lists model --- bookwyrm/tests/models/test_list.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/bookwyrm/tests/models/test_list.py b/bookwyrm/tests/models/test_list.py index e4ecfe897..de6957b57 100644 --- a/bookwyrm/tests/models/test_list.py +++ b/bookwyrm/tests/models/test_list.py @@ -23,7 +23,9 @@ class List(TestCase): def test_remote_id(self, _): """shelves use custom remote ids""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="Test List", user=self.local_user ) @@ -32,7 +34,9 @@ class List(TestCase): def test_to_activity(self, _): """jsonify it""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="Test List", user=self.local_user ) @@ -46,7 +50,9 @@ class List(TestCase): def test_list_item(self, _): """a list entry""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="Test List", user=self.local_user, privacy="unlisted" ) @@ -64,7 +70,9 @@ class List(TestCase): def test_list_item_pending(self, _): """a list entry""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="Test List", user=self.local_user ) @@ -84,7 +92,9 @@ class List(TestCase): def test_embed_key(self, _): """embed_key should never be empty""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): book_list = models.List.objects.create( name="Test List", user=self.local_user ) From 6e70ceb094ed5ea0139b8ac7d689e476eca3350f Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sat, 2 Jul 2022 19:43:59 -0700 Subject: [PATCH 5/5] More mocks --- bookwyrm/tests/lists_stream/test_signals.py | 14 ++++++++------ bookwyrm/tests/lists_stream/test_stream.py | 1 + bookwyrm/tests/lists_stream/test_tasks.py | 4 +++- bookwyrm/tests/models/test_group.py | 9 ++++++--- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/bookwyrm/tests/lists_stream/test_signals.py b/bookwyrm/tests/lists_stream/test_signals.py index f82dba3a4..96f1ae231 100644 --- a/bookwyrm/tests/lists_stream/test_signals.py +++ b/bookwyrm/tests/lists_stream/test_signals.py @@ -32,9 +32,10 @@ class ListsStreamSignals(TestCase): def test_add_list_on_create_command(self, _): """a new lists has entered""" - book_list = models.List.objects.create( - user=self.remote_user, name="hi", privacy="public" - ) + with patch("bookwyrm.lists_stream.remove_list_task.delay"): + book_list = models.List.objects.create( + user=self.remote_user, name="hi", privacy="public" + ) with patch("bookwyrm.lists_stream.add_list_task.delay") as mock: lists_stream.add_list_on_create_command(book_list.id) self.assertEqual(mock.call_count, 1) @@ -43,9 +44,10 @@ class ListsStreamSignals(TestCase): def test_remove_list_on_delete(self, _): """delete a list""" - book_list = models.List.objects.create( - user=self.remote_user, name="hi", privacy="public" - ) + with patch("bookwyrm.lists_stream.remove_list_task.delay"): + book_list = models.List.objects.create( + user=self.remote_user, name="hi", privacy="public" + ) with patch("bookwyrm.lists_stream.remove_list_task.delay") as mock: lists_stream.remove_list_on_delete(models.List, book_list) args = mock.call_args[0] diff --git a/bookwyrm/tests/lists_stream/test_stream.py b/bookwyrm/tests/lists_stream/test_stream.py index 4d8aa52b2..0e87c7436 100644 --- a/bookwyrm/tests/lists_stream/test_stream.py +++ b/bookwyrm/tests/lists_stream/test_stream.py @@ -11,6 +11,7 @@ from bookwyrm import lists_stream, models @patch("bookwyrm.activitystreams.add_book_statuses_task.delay") @patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") @patch("bookwyrm.activitystreams.populate_stream_task.delay") +@patch("bookwyrm.lists_stream.remove_list_task.delay") class ListsStream(TestCase): """using redis to build activity streams""" diff --git a/bookwyrm/tests/lists_stream/test_tasks.py b/bookwyrm/tests/lists_stream/test_tasks.py index 1da36b71b..55c5d98c8 100644 --- a/bookwyrm/tests/lists_stream/test_tasks.py +++ b/bookwyrm/tests/lists_stream/test_tasks.py @@ -35,7 +35,9 @@ class Activitystreams(TestCase): inbox="https://example.com/users/rat/inbox", outbox="https://example.com/users/rat/outbox", ) - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): self.list = models.List.objects.create( user=self.local_user, name="hi", privacy="public" ) diff --git a/bookwyrm/tests/models/test_group.py b/bookwyrm/tests/models/test_group.py index 8739f7fee..86cafaa39 100644 --- a/bookwyrm/tests/models/test_group.py +++ b/bookwyrm/tests/models/test_group.py @@ -80,7 +80,9 @@ class Group(TestCase): """follower-only group booklists should not be excluded from group booklist listing for group members who do not follower list owner""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): followers_list = models.List.objects.create( name="Followers List", curation="group", @@ -101,8 +103,9 @@ class Group(TestCase): """private group booklists should not be excluded from group booklist listing for group members""" - with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): - + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.lists_stream.remove_list_task.delay"): private_list = models.List.objects.create( name="Private List", privacy="direct",