Use update_fields to limit remove_list_tasks

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.
This commit is contained in:
Mouse Reeve 2022-07-02 13:06:29 -07:00
parent aae02dff9a
commit 3ad0a5d073
7 changed files with 43 additions and 22 deletions

View file

@ -114,15 +114,20 @@ class ListsStream(RedisStore):
@receiver(signals.post_save, sender=models.List) @receiver(signals.post_save, sender=models.List)
# pylint: disable=unused-argument # 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""" """add newly created lists streams"""
if not created: if created:
# the privacy may have changed, so we need to re-do the whole thing # when creating new things, gotta wait on the transaction
remove_list_task.delay(instance.id, re_add=True) transaction.on_commit(lambda: add_list_on_create_command(instance.id))
return return
# when creating new things, gotta wait on the transaction # if update_fields was specified, we can check if privacy was updated, but if
transaction.on_commit(lambda: add_list_on_create_command(instance.id)) # 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) @receiver(signals.post_delete, sender=models.List)

View file

@ -129,7 +129,7 @@ class List(OrderedCollectionMixin, BookWyrmModel):
"""on save, update embed_key and avoid clash with existing code""" """on save, update embed_key and avoid clash with existing code"""
if not self.embed_key: if not self.embed_key:
self.embed_key = uuid.uuid4() self.embed_key = uuid.uuid4()
return super().save(*args, **kwargs) super().save(*args, **kwargs)
class ListItem(CollectionItemMixin, BookWyrmModel): class ListItem(CollectionItemMixin, BookWyrmModel):
@ -156,7 +156,7 @@ class ListItem(CollectionItemMixin, BookWyrmModel):
super().save(*args, **kwargs) super().save(*args, **kwargs)
# tick the updated date on the parent list # tick the updated date on the parent list
self.book_list.updated_date = timezone.now() 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 list_owner = self.book_list.user
model = apps.get_model("bookwyrm.Notification", require_ready=True) model = apps.get_model("bookwyrm.Notification", require_ready=True)

View file

@ -36,7 +36,9 @@ class ListViews(TestCase):
parent_work=work, 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( self.list = models.List.objects.create(
name="Test List", user=self.local_user name="Test List", user=self.local_user
) )

View file

@ -36,7 +36,9 @@ class ListViews(TestCase):
parent_work=work, 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( self.list = models.List.objects.create(
name="Test List", user=self.local_user name="Test List", user=self.local_user
) )

View file

@ -65,7 +65,9 @@ class ListViews(TestCase):
parent_work=work_four, 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( self.list = models.List.objects.create(
name="Test List", user=self.local_user name="Test List", user=self.local_user
) )
@ -244,7 +246,7 @@ class ListViews(TestCase):
with patch( with patch(
"bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" "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) result = view(request, self.list.id)
self.assertEqual(mock.call_count, 1) self.assertEqual(mock.call_count, 1)
@ -596,7 +598,7 @@ class ListViews(TestCase):
def test_add_book_outsider(self): def test_add_book_outsider(self):
"""put a book on a list""" """put a book on a list"""
self.list.curation = "open" self.list.curation = "open"
self.list.save(broadcast=False) self.list.save(broadcast=False, update_fields=["curation"])
request = self.factory.post( request = self.factory.post(
"", "",
{ {
@ -625,7 +627,7 @@ class ListViews(TestCase):
def test_add_book_pending(self): def test_add_book_pending(self):
"""put a book on a list awaiting approval""" """put a book on a list awaiting approval"""
self.list.curation = "curated" self.list.curation = "curated"
self.list.save(broadcast=False) self.list.save(broadcast=False, update_fields=["curation"])
request = self.factory.post( request = self.factory.post(
"", "",
{ {
@ -658,7 +660,7 @@ class ListViews(TestCase):
def test_add_book_self_curated(self): def test_add_book_self_curated(self):
"""put a book on a list automatically approved""" """put a book on a list automatically approved"""
self.list.curation = "curated" self.list.curation = "curated"
self.list.save(broadcast=False) self.list.save(broadcast=False, update_fields=["curation"])
request = self.factory.post( request = self.factory.post(
"", "",
{ {
@ -687,7 +689,7 @@ class ListViews(TestCase):
def test_add_book_permission_denied(self): def test_add_book_permission_denied(self):
"""you can't add to that list""" """you can't add to that list"""
self.list.curation = "closed" self.list.curation = "closed"
self.list.save(broadcast=False) self.list.save(broadcast=False, update_fields=["curation"])
request = self.factory.post( request = self.factory.post(
"", "",
{ {

View file

@ -32,7 +32,9 @@ class ListItemViews(TestCase):
remote_id="https://example.com/book/1", remote_id="https://example.com/book/1",
parent_work=work, 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( self.list = models.List.objects.create(
name="Test List", user=self.local_user name="Test List", user=self.local_user
) )

View file

@ -39,7 +39,9 @@ class ListViews(TestCase):
view = views.Lists.as_view() view = views.Lists.as_view()
with patch( with patch(
"bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" "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="Public list", user=self.local_user)
models.List.objects.create( models.List.objects.create(
name="Private list", privacy="direct", user=self.local_user name="Private list", privacy="direct", user=self.local_user
@ -62,7 +64,9 @@ class ListViews(TestCase):
def test_saved_lists_page(self): def test_saved_lists_page(self):
"""there are so many views, this just makes sure it LOADS""" """there are so many views, this just makes sure it LOADS"""
view = views.SavedLists.as_view() 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( booklist = models.List.objects.create(
name="Public list", user=self.local_user name="Public list", user=self.local_user
) )
@ -82,7 +86,9 @@ class ListViews(TestCase):
def test_saved_lists_page_empty(self): def test_saved_lists_page_empty(self):
"""there are so many views, this just makes sure it LOADS""" """there are so many views, this just makes sure it LOADS"""
view = views.SavedLists.as_view() 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="Public list", user=self.local_user)
models.List.objects.create( models.List.objects.create(
name="Private list", privacy="direct", user=self.local_user name="Private list", privacy="direct", user=self.local_user
@ -108,7 +114,9 @@ class ListViews(TestCase):
def test_user_lists_page(self): def test_user_lists_page(self):
"""there are so many views, this just makes sure it LOADS""" """there are so many views, this just makes sure it LOADS"""
view = views.UserLists.as_view() 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="Public list", user=self.local_user)
models.List.objects.create( models.List.objects.create(
name="Private list", privacy="direct", user=self.local_user name="Private list", privacy="direct", user=self.local_user
@ -146,7 +154,7 @@ class ListViews(TestCase):
request.user = self.local_user request.user = self.local_user
with patch( with patch(
"bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"
) as mock: ) as mock, patch("bookwyrm.lists_stream.remove_list_task.delay"):
result = view(request) result = view(request)
self.assertEqual(mock.call_count, 1) self.assertEqual(mock.call_count, 1)