✨(backend) per-user read tracking for shared mailboxes - #770
Open
nicolasaunai wants to merge 2 commits into
Open
✨(backend) per-user read tracking for shared mailboxes#770nicolasaunai wants to merge 2 commits into
nicolasaunai wants to merge 2 commits into
Conversation
Adds a MessageRead model to track which user has read which message in shared mailboxes. The existing read/unread flag (based on ThreadAccess.read_at) is unchanged, but each message now exposes a read_by field listing who has read it. - New MessageRead model (message, user, read_at) - Migration 0035 for messages_messageread table - Flag endpoint creates/deletes MessageRead on mark-as-read/unread - read_by field in MessageSerializer - Prefetch reads in MessageViewSet to avoid N+1 - ReadOnly inline in MessageAdmin - Frontend: ReadByUser type + 'Vu par' display below each message
The read_by serializer field was missing from the generated OpenAPI schema and frontend client, causing the CI API-update check to fail. - Regenerated openapi.json with Message.read_by - Generated MessageReadByItem type from schema - Updated Message model import to use auto-generated type - Removed hand-written ReadByUser type from message.ts Signed-off-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
📝 WalkthroughWalkthroughAdds per-user message read tracking, synchronizes records from read-state updates, exposes reader attribution through the message API, displays it in thread messages, and adds read-only Django admin visibility. ChangesMessage read tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChangeFlagView
participant MessageRead
participant MessageViewSet
participant ThreadMessage
User->>ChangeFlagView: submit read_at
ChangeFlagView->>MessageRead: create or delete read records
MessageViewSet->>MessageRead: prefetch readers
MessageViewSet->>ThreadMessage: return message with read_by
ThreadMessage->>User: display reader names
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 4
🤖 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/api/serializers.py`:
- Around line 1151-1163: Update the read_by schema in
src/backend/core/api/serializers.py#L1151-L1163 by adding an item-level required
declaration for user_id, user_name, and read_at, then regenerate
src/backend/core/api/openapi.json#L9518-L9536 so the generated item schema
includes the same required fields.
In `@src/backend/core/api/viewsets/flag.py`:
- Around line 393-421: The read_at branch in the message-read update flow must
remove this user’s MessageRead rows for messages with created_at greater than
the lowered watermark before creating missing rows. Update the logic around the
existing messages_to_mark and bulk_create operations to delete those
out-of-watermark reads for thread_pks and user, while preserving creation of
missing reads up to read_at.
In `@src/backend/core/migrations/0035_messageread.py`:
- Around line 17-22: Wrap the generated field declarations in the migration’s
MessageRead model, especially the UUIDField, DateTimeField, and ForeignKey
calls, so every line stays within the repository’s 100-character limit while
preserving all existing field options and migration behavior.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx`:
- Around line 395-399: Replace the hard-coded 'Vu par ' label in the thread
message read-by block with the component’s existing t(...) localization helper,
using the appropriate translation key and preserving the mapped reader names and
spacing.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98092042-7777-44e0-a4b0-b937779d3fc0
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_read_by_item.tsis excluded by!**/gen/**
📒 Files selected for processing (9)
src/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/flag.pysrc/backend/core/api/viewsets/message.pysrc/backend/core/migrations/0035_messageread.pysrc/backend/core/models.pysrc/frontend/src/features/layouts/components/thread-view/components/thread-message/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx
Comment on lines
+1151
to
+1163
| @extend_schema_field( | ||
| { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "user_id": {"type": "string", "format": "uuid"}, | ||
| "user_name": {"type": "string"}, | ||
| "read_at": {"type": "string", "format": "date-time"}, | ||
| }, | ||
| }, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== serializer around read_by annotation =="
sed -n '1120,1185p' src/backend/core/api/serializers.py
echo
echo "== openapi read_by section =="
sed -n '9495,9545p' src/backend/core/api/openapi.json
echo
echo "== definitions/usages of get_read_by and read_by field =="
rg -n "def get_read_by|read_by|get_read_by|annotate.*read_by" src/backend/core -SRepository: suitenumerique/messages
Length of output: 5740
Declare the read_by item fields as required.
get_read_by() always returns user_id, user_name, and read_at, but the schema marks them as optional. Add the item-level required array and regenerate the OpenAPI artifact.
src/backend/core/api/serializers.py#L1151-L1163: addrequired: ["user_id", "user_name", "read_at"]to the item object.src/backend/core/api/openapi.json#L9518-L9536: regenerate the schema so the item object includes the same required fields.
📍 Affects 2 files
src/backend/core/api/serializers.py#L1151-L1163(this comment)src/backend/core/api/openapi.json#L9518-L9536
🤖 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/api/serializers.py` around lines 1151 - 1163, Update the
read_by schema in src/backend/core/api/serializers.py#L1151-L1163 by adding an
item-level required declaration for user_id, user_name, and read_at, then
regenerate src/backend/core/api/openapi.json#L9518-L9536 so the generated item
schema includes the same required fields.
Comment on lines
+393
to
+421
| if read_at is not None: | ||
| # Mark all messages up to read_at as read by this user | ||
| messages_to_mark = list( | ||
| models.Message.objects.filter( | ||
| thread_id__in=thread_pks, | ||
| created_at__lte=read_at, | ||
| ) | ||
| .exclude( | ||
| reads__user=user, | ||
| ) | ||
| .values_list("id", flat=True) | ||
| ) | ||
| if messages_to_mark: | ||
| models.MessageRead.objects.bulk_create( | ||
| [ | ||
| models.MessageRead( | ||
| message_id=mid, user=user, read_at=read_at | ||
| ) | ||
| for mid in messages_to_mark | ||
| ], | ||
| ignore_conflicts=True, | ||
| batch_size=500, | ||
| ) | ||
| else: | ||
| # Mark as unread: remove MessageRead for this user in these threads | ||
| models.MessageRead.objects.filter( | ||
| message__thread_id__in=thread_pks, | ||
| user=user, | ||
| ).delete() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove reads beyond a lowered watermark.
When read_at moves from a later timestamp to an earlier one, this only creates missing rows; it retains this user’s MessageRead rows for messages after the new watermark. ThreadAccess.read_at then marks those messages unread while read_by still reports them as read. Delete this user’s rows with message__created_at__gt=read_at in these threads before creating missing rows.
🤖 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/api/viewsets/flag.py` around lines 393 - 421, The read_at
branch in the message-read update flow must remove this user’s MessageRead rows
for messages with created_at greater than the lowered watermark before creating
missing rows. Update the logic around the existing messages_to_mark and
bulk_create operations to delete those out-of-watermark reads for thread_pks and
user, while preserving creation of missing reads up to read_at.
Comment on lines
+17
to
+22
| ('id', models.UUIDField(default=uuid.uuid4, editable=False, help_text='primary key for the record as UUID', primary_key=True, serialize=False, verbose_name='id')), | ||
| ('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created on')), | ||
| ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')), | ||
| ('read_at', models.DateTimeField(db_index=True, default=django.utils.timezone.now, verbose_name='read at')), | ||
| ('message', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='reads', to='core.message')), | ||
| ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='message_reads', to='core.user')), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the generated field declarations.
Lines 17–22 exceed the repository’s 100-character limit. As per coding guidelines, src/backend/**/*.py must follow Django/PEP 8 with a 100-character line limit.
🤖 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/migrations/0035_messageread.py` around lines 17 - 22, Wrap
the generated field declarations in the migration’s MessageRead model,
especially the UUIDField, DateTimeField, and ForeignKey calls, so every line
stays within the repository’s 100-character limit while preserving all existing
field options and migration behavior.
Source: Coding guidelines
Comment on lines
+395
to
+399
| {message.read_by && message.read_by.length > 0 && ( | ||
| <div className="thread-message__read-by"> | ||
| {'Vu par '} | ||
| {message.read_by.map((r) => r.user_name).join(', ')} | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the read-by label.
'Vu par ' is hard-coded even though this component already uses t(...); users in other locales will see a French label.
Proposed fix
- {'Vu par '}
+ {t('Seen by')}{' '}📝 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
| {message.read_by && message.read_by.length > 0 && ( | |
| <div className="thread-message__read-by"> | |
| {'Vu par '} | |
| {message.read_by.map((r) => r.user_name).join(', ')} | |
| </div> | |
| {message.read_by && message.read_by.length > 0 && ( | |
| <div className="thread-message__read-by"> | |
| {t('Seen by')}{' '} | |
| {message.read_by.map((r) => r.user_name).join(', ')} | |
| </div> |
🤖 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/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx`
around lines 395 - 399, Replace the hard-coded 'Vu par ' label in the thread
message read-by block with the component’s existing t(...) localization helper,
using the appropriate translation key and preserving the mapped reader names and
spacing.
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.
Adds a
MessageReadmodel to track which user has read which message in shared mailboxes. The existing read/unread flag (based onThreadAccess.read_at) is unchanged, but each message now exposes aread_byfield listing who has read it.Backend
MessageReadmodel (message, user, read_at) with unique_togethermessages_messagereadtableMessageSerializer.read_by— list of {user_id, user_name, read_at}ChangeFlagViewcreates/deletesMessageReadon mark-as-read/unreadMessageViewSet(select_related user) to avoid N+1MessageAdminFrontend
MessageReadByItemtype via OrvalSummary by CodeRabbit