✨(backend) add user mentions with email notifications#2447
Open
maboukerfa wants to merge 5 commits into
Open
Conversation
Return all accesses on the document and its ancestors to any user with access, keeping the limited user details serializer and adding the user id to it, so that collaborators allowed to comment can identify and mention each other. Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
We need this so mention notification emails can replace the default share link with a link to the anchor of the mention.
Allow users with at least the commenter role on a document to mention
other collaborators in the document body or in a comment thread via
POST /documents/{id}/mention/. The mentioned user must already have
access to the document.
The mention record is always created, and the mentioned user is
notified by email with a link to the anchor, unless a notification
was already sent in the same context (document body or specific
thread) within a configurable cooldown period (15 minutes by default).
Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
Move the mention email off the request/response cycle into a Celery task so the SMTP round-trip no longer delays the API response. Signed-off-by: BOUKERFA Mohamed El Amine <boukerfa.ma@gmail.com>
Add a dedicated "mention" throttle scope limiting calls to the document mention endpoint to 30 requests per minute. Signed-off-by: BOUKERFA Mohamed El Amine <boukerfa.ma@gmail.com>
WalkthroughThis PR adds a user mention feature to the document collaboration backend. A new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/models.py (1)
1439-1449:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not mark
notified_atwhen email delivery fails.
Document.send_email()swallowsSMTPException(Line 1439-1449), butMention.notify()still setsnotified_at(Line 2159-2160). That records false delivery and suppresses future retries.Proposed fix
def send_email(self, subject, emails, context=None, language=None): @@ try: send_mail( @@ fail_silently=False, ) except smtplib.SMTPException as exception: logger.error("invitation to %s was not sent: %s", emails, exception) + return False + return True @@ - self.document.send_email(subject, [user.email], context, language) + sent = self.document.send_email(subject, [user.email], context, language) + if not sent: + return False self.notified_at = timezone.now() self.save(update_fields=["notified_at", "updated_at"])Also applies to: 2157-2160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/models.py` around lines 1439 - 1449, The `Document.send_email()` method catches `smtplib.SMTPException` and logs it but does not re-raise the exception or communicate failure to the caller. This allows `Mention.notify()` to unconditionally set the `notified_at` field even when email delivery fails, falsely marking notifications as sent and preventing retries. Fix this by either re-raising the exception after logging it in the `send_email()` method, or returning a success/failure status that `Mention.notify()` can check before setting `notified_at`. Ensure `notified_at` is only set when email delivery actually succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/models.py`:
- Around line 2125-2160: The notification sending logic has a race condition
where multiple concurrent workers can pass the is_notification_in_cooldown()
check simultaneously before either saves the notified_at field, resulting in
duplicate emails being sent. To fix this, use database-level locking to make the
cooldown check and the notified_at update atomic. Fetch the current object
instance using select_for_update() before performing the
is_notification_in_cooldown() check to ensure only one worker can proceed at a
time, preventing race conditions between the cooldown validation and the
notified_at timestamp update in the save() call.
- Line 2112: The cooldown check at line 2112 uses `created_at__gt` to filter
notifications, but cooldown semantics should be based on when notifications were
actually sent, not when they were created. With async workers, there can be a
delay between creation and sending, causing cooldown suppression to be bypassed
prematurely. Replace the `created_at__gt` field with the appropriate sent or
delivered timestamp field (such as `sent_at` or similar) to ensure cooldown
decisions are based on actual notification delivery times rather than creation
times.
In `@src/backend/core/tasks/mail.py`:
- Line 35: The mention.notify() call has a race condition where concurrent tasks
can simultaneously check cooldown, both see no prior notification, and send
duplicate emails. Before calling mention.notify(), implement a locking mechanism
on the mention context or use an atomic idempotency key to serialize the
cooldown decision. Additionally, add a check to skip mentions that already have
a notified_at timestamp set before executing notify() to prevent redundant
processing of already-notified mentions.
- Around line 31-33: The query in the mail task uses `.get()` on the Mention
model which will raise an exception if the mention has been deleted before the
worker processes the queued task (since Mention objects are cascade-deleted).
Replace the `.get(id=mention_id)` call with `.filter(id=mention_id).first()` to
gracefully return None when the mention no longer exists, then add a check to
return early or skip processing if the mention is None instead of crashing on a
missing database row.
- Around line 40-43: In the dictionary construction where `mentioned_user_id` is
being set, apply a conditional check similar to the one already used for
`thread_id` to preserve null values in analytics. Instead of always calling
`str(mention.mentioned_user_id)`, conditionally convert it to string only if the
value is not None, otherwise pass None directly. This prevents null
`mentioned_user_id` values from being converted to the literal string "None" in
the PostHog analytics data.
In `@src/backend/core/tests/documents/test_api_documents_mention.py`:
- Around line 516-518: Replace the inline mutation of
settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] with Django's
override_settings context manager to ensure proper cleanup even if the test
exits early. Create a copy of the existing DEFAULT_THROTTLE_RATES dictionary,
modify the mention key in the copy to "3/minute", and wrap the test logic in a
with override_settings() block that merges this modified throttle rates dict
back into REST_FRAMEWORK. Apply this pattern to all occurrences where throttle
rates are being mutated (at the locations mentioned in the review) to maintain
proper test isolation and avoid state leakage.
---
Outside diff comments:
In `@src/backend/core/models.py`:
- Around line 1439-1449: The `Document.send_email()` method catches
`smtplib.SMTPException` and logs it but does not re-raise the exception or
communicate failure to the caller. This allows `Mention.notify()` to
unconditionally set the `notified_at` field even when email delivery fails,
falsely marking notifications as sent and preventing retries. Fix this by either
re-raising the exception after logging it in the `send_email()` method, or
returning a success/failure status that `Mention.notify()` can check before
setting `notified_at`. Ensure `notified_at` is only set when email delivery
actually succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 348f36be-a93d-4b8b-9cf6-6edfbddef963
📒 Files selected for processing (18)
CHANGELOG.mdsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets.pysrc/backend/core/factories.pysrc/backend/core/migrations/0033_mention.pysrc/backend/core/models.pysrc/backend/core/tasks/mail.pysrc/backend/core/tests/documents/test_api_document_accesses.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/core/tests/documents/test_api_documents_mention.pysrc/backend/core/tests/documents/test_api_documents_retrieve.pysrc/backend/core/tests/documents/test_api_documents_threads.pysrc/backend/core/tests/documents/test_api_documents_trashbin.pysrc/backend/core/tests/serializers/test_user_light_serializer.pysrc/backend/core/tests/test_models_documents.pysrc/backend/core/tests/test_models_mentions.pysrc/backend/core/utils/analytics.pysrc/backend/impress/settings.py
| mentioned_user_id=self.mentioned_user_id, | ||
| thread_id=self.thread_id, | ||
| notified_at__isnull=False, | ||
| created_at__gt=cooldown_start, |
There was a problem hiding this comment.
Use notification timestamp for cooldown decisions.
Line 2112 uses created_at__gt, but cooldown semantics are about sent notifications. With async workers, delayed sends can bypass suppression too early.
Proposed fix
- created_at__gt=cooldown_start,
+ notified_at__gt=cooldown_start,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| created_at__gt=cooldown_start, | |
| notified_at__gt=cooldown_start, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/models.py` at line 2112, The cooldown check at line 2112
uses `created_at__gt` to filter notifications, but cooldown semantics should be
based on when notifications were actually sent, not when they were created. With
async workers, there can be a delay between creation and sending, causing
cooldown suppression to be bypassed prematurely. Replace the `created_at__gt`
field with the appropriate sent or delivered timestamp field (such as `sent_at`
or similar) to ensure cooldown decisions are based on actual notification
delivery times rather than creation times.
Comment on lines
+2125
to
+2160
| if user is None or not user.email or self.is_notification_in_cooldown(): | ||
| return False | ||
|
|
||
| sender = self.mentioned_by_user | ||
| language = ( | ||
| language or user.language or sender.language or settings.LANGUAGE_CODE | ||
| ) | ||
| sender_name = sender.full_name or sender.email | ||
| domain = settings.EMAIL_URL_APP or Site.objects.get_current().domain | ||
|
|
||
| with override(language): | ||
| title = self.document.title or str(_("Untitled Document")) | ||
| if self.thread_id: | ||
| subject = _( | ||
| "You were mentioned in a comment on the document {title}" | ||
| ).format(title=title) | ||
| message = _( | ||
| "{name} mentioned you in a comment on the following document:" | ||
| ).format(name=sender_name) | ||
| else: | ||
| subject = _("You were mentioned in the document {title}").format( | ||
| title=title | ||
| ) | ||
| message = _("{name} mentioned you in the following document:").format( | ||
| name=sender_name | ||
| ) | ||
| context = { | ||
| "title": subject, | ||
| "message": message, | ||
| "link": f"{domain}/docs/{self.document_id}/#{self.anchor_id}", | ||
| } | ||
|
|
||
| self.document.send_email(subject, [user.email], context, language) | ||
|
|
||
| self.notified_at = timezone.now() | ||
| self.save(update_fields=["notified_at", "updated_at"]) |
There was a problem hiding this comment.
Cooldown check and update are race-prone under concurrent workers.
Two workers can pass is_notification_in_cooldown() before either writes notified_at, causing duplicate emails for the same context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/models.py` around lines 2125 - 2160, The notification
sending logic has a race condition where multiple concurrent workers can pass
the is_notification_in_cooldown() check simultaneously before either saves the
notified_at field, resulting in duplicate emails being sent. To fix this, use
database-level locking to make the cooldown check and the notified_at update
atomic. Fetch the current object instance using select_for_update() before
performing the is_notification_in_cooldown() check to ensure only one worker can
proceed at a time, preventing race conditions between the cooldown validation
and the notified_at timestamp update in the save() call.
Comment on lines
+31
to
+33
| mention = models.Mention.objects.select_related( | ||
| "document", "mentioned_user", "mentioned_by_user" | ||
| ).get(id=mention_id) |
There was a problem hiding this comment.
Handle mentions deleted before the worker runs.
Because Mention is cascade-deleted with its document/thread/sender, a queued task can legitimately find no row and currently fails on .get().
Proposed fix
- mention = models.Mention.objects.select_related(
- "document", "mentioned_user", "mentioned_by_user"
- ).get(id=mention_id)
+ try:
+ mention = models.Mention.objects.select_related(
+ "document", "mentioned_user", "mentioned_by_user"
+ ).get(id=mention_id)
+ except models.Mention.DoesNotExist:
+ return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tasks/mail.py` around lines 31 - 33, The query in the mail
task uses `.get()` on the Mention model which will raise an exception if the
mention has been deleted before the worker processes the queued task (since
Mention objects are cascade-deleted). Replace the `.get(id=mention_id)` call
with `.filter(id=mention_id).first()` to gracefully return None when the mention
no longer exists, then add a check to return early or skip processing if the
mention is None instead of crashing on a missing database row.
| "document", "mentioned_user", "mentioned_by_user" | ||
| ).get(id=mention_id) | ||
|
|
||
| notified = mention.notify() |
There was a problem hiding this comment.
Serialize the cooldown decision before sending.
Mention.notify() checks cooldown and then sends/sets notified_at without locking; concurrent tasks in the same document/thread/user context can both see no prior notification and send duplicate emails, bypassing the cooldown. Lock the mention context or use an atomic idempotency key before calling notify(), and also skip already-notified mentions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tasks/mail.py` at line 35, The mention.notify() call has a
race condition where concurrent tasks can simultaneously check cooldown, both
see no prior notification, and send duplicate emails. Before calling
mention.notify(), implement a locking mechanism on the mention context or use an
atomic idempotency key to serialize the cooldown decision. Additionally, add a
check to skip mentions that already have a notified_at timestamp set before
executing notify() to prevent redundant processing of already-notified mentions.
Comment on lines
+40
to
+43
| { | ||
| "mention_id": str(mention.id), | ||
| "mentioned_user_id": str(mention.mentioned_user_id), | ||
| "thread_id": str(mention.thread_id) if mention.thread_id else None, |
There was a problem hiding this comment.
Preserve null for deleted mentioned users in analytics.
When mentioned_user_id is null, str(mention.mentioned_user_id) emits the literal string "None", mixing UUID strings with a sentinel string in PostHog.
Proposed fix
- "mentioned_user_id": str(mention.mentioned_user_id),
+ "mentioned_user_id": (
+ str(mention.mentioned_user_id)
+ if mention.mentioned_user_id
+ else None
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "mention_id": str(mention.id), | |
| "mentioned_user_id": str(mention.mentioned_user_id), | |
| "thread_id": str(mention.thread_id) if mention.thread_id else None, | |
| { | |
| "mention_id": str(mention.id), | |
| "mentioned_user_id": ( | |
| str(mention.mentioned_user_id) | |
| if mention.mentioned_user_id | |
| else None | |
| ), | |
| "thread_id": str(mention.thread_id) if mention.thread_id else None, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tasks/mail.py` around lines 40 - 43, In the dictionary
construction where `mentioned_user_id` is being set, apply a conditional check
similar to the one already used for `thread_id` to preserve null values in
analytics. Instead of always calling `str(mention.mentioned_user_id)`,
conditionally convert it to string only if the value is not None, otherwise pass
None directly. This prevents null `mentioned_user_id` values from being
converted to the literal string "None" in the PostHog analytics data.
Comment on lines
+516
to
+518
| current_rate = settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] | ||
| settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] = "3/minute" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Harden throttle-rate test setup/teardown to avoid state leakage.
Mutating settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] inline is fragile if the test exits early. Use override_settings (or try/finally) around a copied throttle map to keep isolation robust.
Proposed refactor
- current_rate = settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"]
- settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] = "3/minute"
+ throttle_rates = dict(settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"])
+ throttle_rates["mention"] = "3/minute"
+ with pytest.raises(Exception):
+ pass# Example shape
throttle_rates = dict(settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"])
throttle_rates["mention"] = "3/minute"
with override_settings(
REST_FRAMEWORK={**settings.REST_FRAMEWORK, "DEFAULT_THROTTLE_RATES": throttle_rates}
):
...Also applies to: 540-541, 549-551, 572-573
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/documents/test_api_documents_mention.py` around lines
516 - 518, Replace the inline mutation of
settings.REST_FRAMEWORK["DEFAULT_THROTTLE_RATES"]["mention"] with Django's
override_settings context manager to ensure proper cleanup even if the test
exits early. Create a copy of the existing DEFAULT_THROTTLE_RATES dictionary,
modify the mention key in the copy to "3/minute", and wrap the test logic in a
with override_settings() block that merges this modified throttle rates dict
back into REST_FRAMEWORK. Apply this pattern to all occurrences where throttle
rates are being mutated (at the locations mentioned in the review) to maintain
proper test isolation and avoid state leakage.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add a mention feature: when a user is mentioned in a document body or a comment thread, they receive an email notification linking straight to the anchor of the mention.
Proposal
POST /api/v1.0/documents/{id}/mention/endpoint that records the mention and sends an email notification to the mentioned user.Notes
the same context (document body or thread) within the cooldown window
(
MENTION_NOTIFICATION_COOLDOWN_MINUTES, default 15).Email content