Merge pull request #2380 from bookwyrm-social/status-activity-tasks

Generate fewer add_status_tasks

bookwyrm/activitystreams.py

@@ -298,6 +298,12 @@ def add_status_on_create(sender, instance, created, *args, **kwargs):
         remove_status_task.delay(instance.id)
         return
 
+    # To avoid creating a zillion unnecessary tasks caused by re-saving the model,
+    # check if it's actually ready to send before we go. We're trusting this was
+    # set correctly by the inbox or view
+    if not instance.ready:
+        return
+
     # when creating new things, gotta wait on the transaction
     transaction.on_commit(
         lambda: add_status_on_create_command(sender, instance, created)

bookwyrm/migrations/0164_status_ready.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.2.16 on 2022-11-15 21:40
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("bookwyrm", "0163_merge_0160_auto_20221101_2251_0162_importjob_task_id"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="status",
+            name="ready",
+            field=models.BooleanField(default=True),
+        ),
+    ]

bookwyrm/models/status.py

@@ -63,6 +63,9 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
         activitypub_field="inReplyTo",
     )
     thread_id = models.IntegerField(blank=True, null=True)
+    # statuses get saved a few times, this indicates if they're set
+    ready = models.BooleanField(default=True)
+
     objects = InheritanceManager()
 
     activity_serializer = activitypub.Note
@@ -83,8 +86,7 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel):
 
         if not self.reply_parent:
             self.thread_id = self.id
-
-        super().save(broadcast=False, update_fields=["thread_id"])
+            super().save(broadcast=False, update_fields=["thread_id"])
 
     def delete(self, *args, **kwargs):  # pylint: disable=unused-argument
         """ "delete" a status"""
@@ -399,7 +401,7 @@ class ReviewRating(Review):
     def save(self, *args, **kwargs):
         if not self.rating:
             raise ValueError("ReviewRating object must include a numerical rating")
-        return super().save(*args, **kwargs)
+        super().save(*args, **kwargs)
 
     @property
     def pure_content(self):

bookwyrm/tests/activitystreams/test_tasks.py

@@ -7,6 +7,7 @@ from bookwyrm import activitystreams, models
 class Activitystreams(TestCase):
     """using redis to build activity streams"""
 
+    # pylint: disable=invalid-name
     def setUp(self):
         """use a test csv"""
         with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(

bookwyrm/tests/views/inbox/test_inbox_create.py

@@ -3,13 +3,69 @@ import json
 import pathlib
 from unittest.mock import patch
 
-from django.test import TestCase
+from django.test import TestCase, TransactionTestCase
 
 from bookwyrm import models, views
 from bookwyrm.activitypub import ActivitySerializerError
 
 
-# pylint: disable=too-many-public-methods
+# pylint: disable=too-many-public-methods, invalid-name
+class TransactionInboxCreate(TransactionTestCase):
+    """readthrough tests"""
+
+    def setUp(self):
+        """basic user and book data"""
+        with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(
+            "bookwyrm.activitystreams.populate_stream_task.delay"
+        ), patch("bookwyrm.lists_stream.populate_lists_task.delay"):
+            self.local_user = models.User.objects.create_user(
+                "mouse@example.com",
+                "mouse@mouse.com",
+                "mouseword",
+                local=True,
+                localname="mouse",
+            )
+        self.local_user.remote_id = "https://example.com/user/mouse"
+        self.local_user.save(broadcast=False, update_fields=["remote_id"])
+        with patch("bookwyrm.models.user.set_remote_server.delay"):
+            self.remote_user = models.User.objects.create_user(
+                "rat",
+                "rat@rat.com",
+                "ratword",
+                local=False,
+                remote_id="https://example.com/users/rat",
+                inbox="https://example.com/users/rat/inbox",
+                outbox="https://example.com/users/rat/outbox",
+            )
+
+        self.create_json = {
+            "id": "hi",
+            "type": "Create",
+            "actor": "hi",
+            "to": ["https://www.w3.org/ns/activitystreams#public"],
+            "cc": ["https://example.com/user/mouse/followers"],
+            "object": {},
+        }
+        models.SiteSettings.objects.create()
+
+    def test_create_status_transaction(self, *_):
+        """the "it justs works" mode"""
+        datafile = pathlib.Path(__file__).parent.joinpath(
+            "../../data/ap_quotation.json"
+        )
+        status_data = json.loads(datafile.read_bytes())
+
+        models.Edition.objects.create(
+            title="Test Book", remote_id="https://example.com/book/1"
+        )
+        activity = self.create_json
+        activity["object"] = status_data
+
+        with patch("bookwyrm.activitystreams.add_status_task.apply_async") as mock:
+            views.inbox.activity_task(activity)
+        self.assertEqual(mock.call_count, 2)
+
+
 @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
 @patch("bookwyrm.activitystreams.add_book_statuses_task.delay")
 class InboxCreate(TestCase):

bookwyrm/tests/views/test_status.py

@@ -2,21 +2,74 @@
 import json
 from unittest.mock import patch
 from django.core.exceptions import PermissionDenied
-from django.test import TestCase
+from django.test import TestCase, TransactionTestCase
 from django.test.client import RequestFactory
 
 from bookwyrm import forms, models, views
 from bookwyrm.views.status import find_mentions
 from bookwyrm.settings import DOMAIN
+
 from bookwyrm.tests.validate_html import validate_html
 
+# pylint: disable=invalid-name
+@patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
+class StatusTransactions(TransactionTestCase):
+    """Test full database transactions"""
+
+    def setUp(self):
+        """we need basic test data and mocks"""
+        self.factory = RequestFactory()
+        with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch(
+            "bookwyrm.activitystreams.populate_stream_task.delay"
+        ), patch("bookwyrm.lists_stream.populate_lists_task.delay"):
+            self.local_user = models.User.objects.create_user(
+                "mouse@local.com",
+                "mouse@mouse.com",
+                "mouseword",
+                local=True,
+                localname="mouse",
+                remote_id="https://example.com/users/mouse",
+            )
+            self.another_user = models.User.objects.create_user(
+                f"nutria@{DOMAIN}",
+                "nutria@nutria.com",
+                "password",
+                local=True,
+                localname="nutria",
+            )
+
+        work = models.Work.objects.create(title="Test Work")
+        self.book = models.Edition.objects.create(
+            title="Example Edition",
+            remote_id="https://example.com/book/1",
+            parent_work=work,
+        )
+
+    def test_create_status_saves(self, *_):
+        """This view calls save multiple times"""
+        view = views.CreateStatus.as_view()
+        form = forms.CommentForm(
+            {
+                "content": "hi",
+                "user": self.local_user.id,
+                "book": self.book.id,
+                "privacy": "public",
+            }
+        )
+        request = self.factory.post("", form.data)
+        request.user = self.local_user
+
+        with patch("bookwyrm.activitystreams.add_status_task.apply_async") as mock:
+            view(request, "comment")
+
+        self.assertEqual(mock.call_count, 2)
+
 
 @patch("bookwyrm.suggested_users.rerank_suggestions_task.delay")
 @patch("bookwyrm.activitystreams.populate_stream_task.delay")
 @patch("bookwyrm.lists_stream.populate_lists_task.delay")
 @patch("bookwyrm.activitystreams.remove_status_task.delay")
 @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async")
-# pylint: disable=invalid-name
 # pylint: disable=too-many-public-methods
 class StatusViews(TestCase):
     """viewing and creating statuses"""

bookwyrm/views/status.py

@@ -82,13 +82,16 @@ class CreateStatus(View):
                 return HttpResponseBadRequest()
             return redirect("/")
 
-        status = form.save(request)
+        status = form.save(request, commit=False)
+        status.ready = False
         # save the plain, unformatted version of the status for future editing
         status.raw_content = status.content
         if hasattr(status, "quote"):
             status.raw_quote = status.quote
 
         status.sensitive = status.content_warning not in [None, ""]
+        # the status has to be saved now before we can add many to many fields
+        # like mentions
         status.save(broadcast=False)
 
         # inspect the text for user tags
@@ -119,6 +122,7 @@ class CreateStatus(View):
         if hasattr(status, "quote"):
             status.quote = to_markdown(status.quote)
 
+        status.ready = True
         status.save(created=created)
 
         # update a readthrough, if needed