From 8b939debe4df60d9012d2d1f1d0cda85667fee0d Mon Sep 17 00:00:00 2001 From: Anthony LC Date: Wed, 7 Oct 2026 16:06:29 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20send=20thread=20reply=20no?= =?UTF-8?q?tification=20asynchronously?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queue a Celery task when a comment is added to an existing thread, so the SMTP round-trips stay out of the request. Queuing is best effort and a failure to send never fails the creation of the comment. The first comment of a thread is created with the thread by another code path and is not notified. Signed-off-by: Anthony LC --- CHANGELOG.md | 4 + src/backend/core/api/viewsets.py | 19 +- src/backend/core/tasks/mail.py | 15 ++ ...st_api_documents_comments_notifications.py | 212 ++++++++++++++++++ 4 files changed, 248 insertions(+), 2 deletions(-) create mode 100644 src/backend/core/tests/documents/test_api_documents_comments_notifications.py diff --git a/CHANGELOG.md b/CHANGELOG.md index aeae72747..78358bcf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to ## [Unreleased] +### Added + +-✨(backend) email thread participants on reply #2802 + ## [v6.0.0] - 2026-10-05 ### Added diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 1d91a5d66..25acc2f67 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -71,7 +71,11 @@ from core.services.search_indexers import ( from core.services.yhub_services import YHubError, YHubService from core.tasks.access import reset_service_connections_on_commit from core.tasks.documents import sync_service_restorations_in_cascade -from core.tasks.mail import send_ask_for_access_mail, send_mention_notification_mail +from core.tasks.mail import ( + send_ask_for_access_mail, + send_mention_notification_mail, + send_thread_reply_notification_mail, +) from core.tasks.search import trigger_batch_document_indexer from core.utils.analytics import PosthogEventName, posthog_capture from core.utils.dicts import lowercase_keys @@ -3282,7 +3286,9 @@ class CommentViewSet( return context def perform_create(self, serializer): - """Attach the request user as the comment author.""" + """Attach the request user as the comment author and notify the + participants of the thread, a comment here always replies to an existing + thread (the first comment of a thread is created with the thread).""" user = self.request.user if self.request.user.is_authenticated else None comment = serializer.save(user=user) @@ -3293,6 +3299,15 @@ class CommentViewSet( document=self.get_document_or_404(), ) + # Notifying is best effort: the comment is already saved and a broker + # outage must not turn its creation into an error + try: + send_thread_reply_notification_mail.delay(str(comment.id)) + except Exception: # pylint: disable=broad-exception-caught + logger.exception( + "unable to queue the notification of comment %s", comment.id + ) + @drf.decorators.action( detail=True, methods=["post", "delete"], diff --git a/src/backend/core/tasks/mail.py b/src/backend/core/tasks/mail.py index 51602d969..719cf1267 100644 --- a/src/backend/core/tasks/mail.py +++ b/src/backend/core/tasks/mail.py @@ -45,3 +45,18 @@ def send_mention_notification_mail(mention_id): }, document=mention.document, ) + + +@app.task +def send_thread_reply_notification_mail(comment_id): + """Notify the participants of a thread of a new reply, outside the + request/response cycle.""" + try: + comment = models.Comment.objects.select_related( + "user", "thread__creator", "thread__document" + ).get(id=comment_id) + except models.Comment.DoesNotExist: + # The comment was deleted before the task ran, nothing to announce + return + + comment.notify_thread_participants() diff --git a/src/backend/core/tests/documents/test_api_documents_comments_notifications.py b/src/backend/core/tests/documents/test_api_documents_comments_notifications.py new file mode 100644 index 000000000..3c41ec0e2 --- /dev/null +++ b/src/backend/core/tests/documents/test_api_documents_comments_notifications.py @@ -0,0 +1,212 @@ +""" +Tests for the email notification of the participants of a thread when a +comment is added to it, through the comments API and the Celery task. +""" + +from unittest import mock + +from django.core import mail + +import pytest +from rest_framework.test import APIClient + +from core import factories, models +from core.tasks.mail import send_thread_reply_notification_mail + +pytestmark = pytest.mark.django_db + + +def _post_comment(user, document, thread, body="reply"): + """Post a comment on a thread as the given user.""" + client = APIClient() + client.force_login(user) + return client.post( + f"/api/v1.0/documents/{document.id!s}/threads/{thread.id!s}/comments/", + {"body": body}, + ) + + +def _setup(): + """Return a document, a thread with a first comment and the replying user.""" + document = factories.DocumentFactory(link_reach="restricted", title="My doc") + creator = factories.UserFactory(language="en-us") + factories.UserDocumentAccessFactory( + document=document, user=creator, role="commenter" + ) + thread = factories.ThreadFactory(document=document, creator=creator) + factories.CommentFactory(thread=thread, user=creator) + replier = factories.UserFactory(full_name="Replying User") + factories.UserDocumentAccessFactory(document=document, user=replier, role="editor") + return document, thread, creator, replier + + +def test_api_comments_reply_notifies_participants(): + """Replying in a thread emails the participants, not the author of the reply.""" + document, thread, creator, replier = _setup() + earlier = factories.UserFactory(language="en-us") + factories.UserDocumentAccessFactory(document=document, user=earlier, role="editor") + factories.CommentFactory(thread=thread, user=earlier) + # Collaborator who did not take part in the thread + bystander = factories.UserFactory() + factories.UserDocumentAccessFactory( + document=document, user=bystander, role="editor" + ) + + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + # pylint: disable=no-member + assert sorted(email.to[0] for email in mail.outbox) == sorted( + [creator.email, earlier.email] + ) + email = next(email for email in mail.outbox if email.to == [creator.email]) + assert email.subject.lower() == ( + 'replying user replied in a comment thread in "my doc"' + ) + assert f"docs/{document.id!s}/" in email.body + + +def test_api_comments_reply_does_not_notify_on_thread_creation(): + """Creating a thread (and its first comment) does not notify anybody.""" + document = factories.DocumentFactory(link_reach="restricted") + author = factories.UserFactory() + factories.UserDocumentAccessFactory(document=document, user=author, role="editor") + other = factories.UserFactory() + factories.UserDocumentAccessFactory(document=document, user=other, role="editor") + factories.ThreadFactory(document=document, creator=other) + + client = APIClient() + client.force_login(author) + response = client.post( + f"/api/v1.0/documents/{document.id!s}/threads/", {"body": "first"} + ) + + assert response.status_code == 201 + assert models.Comment.objects.filter(thread_id=response.json()["id"]).count() == 1 + # pylint: disable-next=no-member + assert len(mail.outbox) == 0 + + +def test_api_comments_reply_nobody_to_notify(): + """Replying to your own thread notifies nobody.""" + document, thread, creator, _replier = _setup() + + response = _post_comment(creator, document, thread) + + assert response.status_code == 201 + # pylint: disable-next=no-member + assert len(mail.outbox) == 0 + + +def test_api_comments_reply_skips_reader_and_user_without_access(): + """Participants who cannot see the thread anymore are not notified.""" + document, thread, creator, replier = _setup() + reader = factories.UserFactory() + factories.UserDocumentAccessFactory(document=document, user=reader, role="reader") + former = factories.UserFactory() + factories.CommentFactory(thread=thread, user=reader) + factories.CommentFactory(thread=thread, user=former) + + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + # pylint: disable=no-member + assert [email.to for email in mail.outbox] == [[creator.email]] + + +def test_api_comments_reply_with_mention_sends_a_single_email(): + """ + A user mentioned by the replier is emailed once: by the mention + notification, not again by the reply notification. + """ + document, thread, creator, replier = _setup() + mentioned = factories.UserFactory() + factories.UserDocumentAccessFactory( + document=document, user=mentioned, role="commenter" + ) + factories.CommentFactory(thread=thread, user=mentioned) + + client = APIClient() + client.force_login(replier) + response = client.post( + f"/api/v1.0/documents/{document.id!s}/mention/", + { + "anchor_id": "3f2b0f4e-5c55-4f0b-9a3e-2d1f5c1b0a11", + "mentioned_user_id": str(mentioned.id), + "thread_id": str(thread.id), + }, + ) + assert response.status_code == 201 + + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + # pylint: disable=no-member + recipients = [email.to[0] for email in mail.outbox] + assert sorted(recipients) == sorted([mentioned.email, creator.email]) + assert recipients.count(mentioned.email) == 1 + + +def test_api_comments_reply_notification_is_asynchronous(): + """The notification is queued, the request does not wait for the emails.""" + document, thread, _creator, replier = _setup() + + with mock.patch( + "core.api.viewsets.send_thread_reply_notification_mail.delay" + ) as mock_delay: + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + mock_delay.assert_called_once_with(response.json()["id"]) + # pylint: disable-next=no-member + assert len(mail.outbox) == 0 + + +def test_api_comments_reply_queue_failure_does_not_fail_the_creation(): + """A broker outage must not turn the creation of a comment into an error.""" + document, thread, _creator, replier = _setup() + + with mock.patch( + "core.api.viewsets.send_thread_reply_notification_mail.delay", + side_effect=ConnectionError("broker down"), + ): + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + assert models.Comment.objects.filter(id=response.json()["id"]).exists() + + +def test_api_comments_reply_mail_failure_does_not_fail_the_creation(): + """A failure while sending an email must not fail the creation of a comment.""" + document, thread, _creator, replier = _setup() + + with mock.patch.object(models.Document, "send_email", side_effect=RuntimeError): + response = _post_comment(replier, document, thread) + + assert response.status_code == 201 + assert models.Comment.objects.filter(id=response.json()["id"]).exists() + + +def test_tasks_send_thread_reply_notification_mail(): + """The task emails the participants of the thread of the comment.""" + document, thread, creator, replier = _setup() + reply = factories.CommentFactory(thread=thread, user=replier) + + send_thread_reply_notification_mail(str(reply.id)) + + # pylint: disable=no-member + assert [email.to for email in mail.outbox] == [[creator.email]] + assert document.title.lower() in mail.outbox[0].subject.lower() + + +def test_tasks_send_thread_reply_notification_mail_deleted_comment(): + """The task does nothing if the comment was deleted in the meantime.""" + _document, thread, _creator, replier = _setup() + reply = factories.CommentFactory(thread=thread, user=replier) + comment_id = str(reply.id) + reply.delete() + + send_thread_reply_notification_mail(comment_id) + + # pylint: disable-next=no-member + assert len(mail.outbox) == 0