Skip to content

Feat/assignation event#645

Merged
jbpenrath merged 1 commit into
developmentfrom
feat/assignation-event
May 5, 2026
Merged

Feat/assignation event#645
jbpenrath merged 1 commit into
developmentfrom
feat/assignation-event

Conversation

@jbpenrath
Copy link
Copy Markdown
Contributor

@jbpenrath jbpenrath commented Apr 27, 2026

Purpose

Assign users to a thread and add mailbox filters to quickly find unassigned and self-assigned threads.

For all previous comments take a look at this PR... #639

Summary by CodeRabbit

  • New Features

    • Assign/unassign users to threads (quick-assign popover, assignees widget, avatar group) and show assigned users in thread lists/details
    • Thread filters & stats for "Assigned to me" and "Unassigned"
    • Collapsed grouping for runs of assignment/unassignment system events
  • Improvements

    • Enhanced share/modal and access UI (per-access user lists, role dropdowns, assign-from-share flows, visibility rules)
    • Idempotent assignment/unassignment, undo window, and mention reconciliation behaviors
  • Documentation

    • Permissions and event semantics updated (viewer/editor roles clarified)

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 403a39a9-a519-470d-b1ba-012462eaf42f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Introduces assign/unassign thread events and per-user UserEvent rows, centralizes mention/assignment reconciliation into a thread_events service, adds DB uniqueness for assign user events, routes admin/write changes into cleanup, extends APIs/serializers/OpenAPI, and adds frontend UI, hooks, timeline collapsing, and comprehensive tests.

Changes

Thread assignment system

Layer / File(s) Summary
Data Model & Constraints
src/backend/core/enums.py, src/backend/core/models.py, src/backend/core/migrations/0026_userevent_usrevt_user_thread_assign_uniq.py
Added ASSIGN/UNASSIGN event types and UserEvent ASSIGN kind; added conditional unique constraint enforcing one active ASSIGN per (user, thread).
JSON Schema Validation Utility
src/backend/core/utils.py, src/backend/core/models.py
Added validate_json_schema(...); factored ThreadEvent DATA_SCHEMAS and added ThreadEvent.validate_data(); model clean() delegates to it.
Service Layer (core logic)
src/backend/core/services/thread_events.py
New atomic APIs: assign_users, unassign_users, sync_im_mentions; helpers for parsing/deduping, validation, user-event create/delete; undo-window absorption (120s); and access-cleanup entry points (revoke_*, downgrade_*).
Admin & Write Hooks
src/backend/core/admin.py, src/backend/core/factories.py, src/backend/core/signals.py
Admin save/delete paths now call thread_events service helpers; factory post-generation sync mirrors service effects; removed old ThreadEvent post-save signal for mention syncing.
Serializers & OpenAPI
src/backend/core/api/serializers.py, src/backend/core/api/openapi.json
Polymorphic OpenAPI serializers for ThreadEvent.data; ThreadMentionableUserSerializer; Mailbox/Thread/ThreadAccess schema changes (is_shared, assigned_users, users); added has_assigned_to_me/has_unassigned query params and updated event contract wording.
Permissions & Viewsets (backend API)
src/backend/core/api/permissions.py, src/backend/core/api/viewsets/thread_event.py, src/backend/core/api/viewsets/thread_access.py, src/backend/core/api/viewsets/mailbox_access.py, src/backend/core/api/viewsets/thread.py, src/backend/core/api/viewsets/thread_user.py
Extracted author-mutation guard helper; ThreadEventViewSet.create delegates ASSIGN/UNASSIGN to service and syncs IM mentions on edits; mailbox/thread access viewsets call service revoke/downgrade on role changes/deletes; thread queryset annotated with _has_assigned_to_me/_has_unassigned and prefetches assigned user events; ThreadUserViewSet returns ThreadMentionableUser with can_post_comments annotation.
Frontend: assignment UX & integration
src/frontend/src/features/... assignees-widget/*, quick-assign-popover/*, ThreadAccessesWidget, share-modal-extensions/*, thread-action-bar, thread-item, thread-panel/*
Added AssigneesWidget, QuickAssignPopover, share-modal extensions, AccessRoleDropdown, per-access users list, assignment flows in ThreadAccessesWidget (including viewer→editor elevation), assigned avatars in thread list, and filter UI for assigned/unassigned.
Frontend: timeline rendering & helpers
src/frontend/src/features/... thread-event/*, assignment-message.ts, group-system-events.test.ts, use-assigned-users.ts, use-thread-assignment.ts, use-is-shared-context.ts
Added grouping that collapses runs of 3+ assign/unassign events; SystemEventLine, CollapsedEventsGroup, buildAssignmentMessage; hooks to derive/mutate assignment state; IM input visibility now uses shared-context gating.
Frontend: styles, locales, utilities
src/frontend/src/features/ui/components/assignees-avatar-group/*, src/frontend/public/locales/common/*, src/frontend/src/features/utils/date-helper.ts, src/frontend/src/styles/main.scss
Added assignees avatar group component and SCSS for widgets/collapsed events, many en/fr locale strings for assignment flows, and date/timestamp helpers for timeline labels.
Tests
src/backend/core/tests/..., src/frontend/..., src/e2e/...
Extensive backend unit tests for service behavior (assign/unassign/mentions/cleanup), model constraint tests, API tests for ASSIGN/UNASSIGN (validation, idempotency, undo-window), thread-list/stats filters and assigned_users field tests, serializer tests, query-count regression tests, frontend unit tests for components/hooks, and e2e Playwright coverage for assign flows.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Frontend
    participant ThreadEventViewSet as API
    participant ThreadEventsService as Service
    participant Database
    participant Cache

    User->>Frontend: Click "Assign user X"
    Frontend->>API: POST /threads/{id}/events (type=assign)
    API->>Service: assign_users(thread, author, assignees)
    Service->>Database: Validate assignee IDs and edit rights
    alt new assignments present
        Service->>Database: Create ThreadEvent(type=assign)
        Service->>Database: Bulk create UserEvent(type=assign) rows (ignore_conflicts)
        API->>Frontend: 201 Created (serialized ThreadEvent)
        API->>Cache: Invalidate thread events / threads list / stats
    else all requested assignees already active
        API->>Frontend: 204 No Content (no-op)
    end

    User->>Frontend: Click "Unassign user X"
    Frontend->>API: POST /threads/{id}/events (type=unassign)
    API->>Service: unassign_users(thread, author, assignees)
    alt within undo window and same author
        Service->>Database: Lock & amend/delete prior ASSIGN ThreadEvent and delete absorbed UserEvent rows
        API->>Frontend: 204 No Content (absorbed)
        API->>Cache: Invalidate relevant caches
    else regular unassign
        Service->>Database: Create ThreadEvent(type=unassign)
        Service->>Database: Delete matching UserEvent(type=assign) rows
        API->>Frontend: 201 Created
        API->>Cache: Invalidate thread events / threads list / stats
    end

    User->>Frontend: Open thread timeline
    Frontend->>Frontend: groupSystemEvents(threadItems)
    Frontend->>User: Render timeline with collapsed group for runs of assign/unassign
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • sylvinus

@jbpenrath jbpenrath force-pushed the feat/assignation-event branch 3 times, most recently from bc2f4e4 to 12e6bed Compare May 4, 2026 08:45
thread=thread, author=request.user, assignees_data=assignees_data
)
except ValueError as exc:
raise ValidationError(str(exc)) from exc
@jbpenrath jbpenrath changed the base branch from main to development May 4, 2026 10:22
@jbpenrath jbpenrath force-pushed the feat/assignation-event branch from 12e6bed to e08c6b6 Compare May 4, 2026 14:55
@jbpenrath jbpenrath marked this pull request as ready for review May 4, 2026 14:55
@jbpenrath
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/backend/core/api/viewsets/thread.py (1)

420-445: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject _unread variants for the new assignment annotations.

has_assigned_to_me and has_unassigned are annotations, not model fields. Because they were added to valid_base_fields but not to annotation_fields, stats_fields=has_unassigned_unread currently passes validation and then builds Q(has_unassigned=True), which will blow up with a FieldError.

Suggested fix
-        annotation_fields = {"has_mention", "has_unread_mention"}
+        annotation_fields = {
+            "has_mention",
+            "has_unread_mention",
+            "has_assigned_to_me",
+            "has_unassigned",
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/viewsets/thread.py` around lines 420 - 445, The
validation allows "_unread" suffixes for annotations because has_assigned_to_me
and has_unassigned were left only in valid_base_fields; update the
annotation_fields set to include "has_assigned_to_me" and "has_unassigned" so
their "_unread" variants are rejected during stats_fields parsing (preventing
building Q(has_unassigned=True) for a non-model column). Modify the
annotation_fields definition near valid_base_fields/special_fields to add these
two symbols.
src/backend/core/api/openapi.json (2)

5575-5621: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Document the 204 no-op branch in responses.

The description now says this endpoint can return 204 for ASSIGN/UNASSIGN no-ops, but the schema still advertises only 201. That leaves generated clients unable to model a documented success path.

📝 Suggested schema update
                 "responses": {
                     "201": {
                         "content": {
                             "application/json": {
                                 "schema": {
                                     "$ref": "#/components/schemas/ThreadEvent"
                                 }
                             }
                         },
                         "description": ""
+                    },
+                    "204": {
+                        "description": "No response body. Returned when the requested ASSIGN/UNASSIGN is a no-op."
                     }
                 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/openapi.json` around lines 5575 - 5621, The OpenAPI
operation threads_events_create currently documents a 201 response only while
its description states it may return 204 for no-op ASSIGN/UNASSIGN cases; update
the operation's responses object to include a "204" response entry (no content)
with a clear description (e.g., "No-op: nothing changed, no content") so
generated clients can model this success path alongside the existing 201 ->
ThreadEvent schema; ensure the new "204" response has no response body/schema.

6031-6046: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the new stats_fields values consistent with the _unread docs.

The updated description says boolean stats fields support an _unread suffix and now includes has_assigned_to_me / has_unassigned, but the enum only allows the base names. As written, the spec documents values that generated clients cannot send.

📝 Suggested enum fix
                             "enum": [
                                 "all",
                                 "all_unread",
                                 "has_assigned_to_me",
+                                "has_assigned_to_me_unread",
                                 "has_delivery_failed",
                                 "has_delivery_pending",
                                 "has_mention",
                                 "has_unassigned",
+                                "has_unassigned_unread",
                                 "has_unread_mention"
                             ]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/api/openapi.json` around lines 6031 - 6046, The openapi enum
for the query parameter stats_fields is missing the documented "_unread"
variants, causing clients to be unable to send the values described in the
description; update the enum in openapi.json for the stats_fields schema to
include all "_unread" counterparts for the boolean fields (add entries such as
has_trashed_unread, has_draft_unread, has_starred_unread,
has_attachments_unread, has_archived_unread, has_sender_unread,
has_active_unread, has_delivery_pending_unread, has_delivery_failed_unread,
is_spam_unread, has_messages_unread, has_unread_mention_unread if intended or
correct that particular name, has_mention_unread, has_assigned_to_me_unread,
has_unassigned_unread) while keeping the existing "all" and "all_unread" values
so the enum matches the description for stats_fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend/core/admin.py`:
- Around line 666-680: The save_model currently only detects EDITOR→other role
changes and calls thread_events_service.downgrade_thread_access, but it doesn't
handle when an existing ThreadAccess is edited to point to a different
mailbox/thread (identity change), leaving the old access unrevoked; update
save_model (and mirror the same fix already used in
_cleanup_thread_access_formset) to detect when change is True and obj.pk exists
and the stored previous row has a different (mailbox_id, thread_id, identity
fields used to represent the access tuple) than the new obj, then call
thread_events_service.revoke_thread_access(...) (or the appropriate revoke
function) for the previous tuple before saving (or immediately after saving) so
assignment cleanup runs for the old access; reference ThreadAccess, save_model,
_cleanup_thread_access_formset, revoke_thread_access, and
downgrade_thread_access when implementing this logic.
- Around line 608-623: When saving an existing MailboxAccess in save_model,
you're only comparing role and thus miss cases where user_id or mailbox_id
changed; take a snapshot of the existing record (user_id, mailbox_id, role)
before calling super().save_model, then after saving compare the previous
snapshot to the new obj: if user_id or mailbox_id changed, call
thread_events_service.revoke_mailbox_access(...) for the previous grant (use the
previous snapshot or re-fetch the previous object state to construct/pass the
old access info), and also keep the existing logic that checks was_editor vs
is_editor to call
thread_events_service.downgrade_mailbox_access(mailbox_access=obj) when role was
downgraded; follow the same snapshot/compare pattern used in
_cleanup_mailbox_access_formset to locate where to capture
previous_user_id/previous_mailbox_id/previous_role and perform the appropriate
revoke/downgrade calls.

In `@src/backend/core/api/openapi.json`:
- Around line 9254-9256: Add an OpenAPI discriminator to the ThreadEventData and
ThreadEventDataRequest oneOf schemas so the top-level "type" value
deterministically selects the matching data variant; update the
ThreadEventRequest spec to reference that discriminator-backed schema mapping
(use the existing variant schema names from the diff, e.g. the
IM/Assign/Owners/etc. variant schemas) and add "additionalProperties": false to
each variant schema to prevent mixed-shape payloads; ensure the
discriminator.mapping maps each type string (e.g. "im", "assign") to the
corresponding component schema name and that the "type" property is required on
the ThreadEvent/ThreadEventRequest wrapper so clients can resolve the correct
data branch.

In `@src/backend/core/api/serializers.py`:
- Around line 1200-1216: The get_users helper currently only filters by mailbox
roles but must also exclude thread accesses with role VIEWER so the UI doesn't
list users the ASSIGN endpoint will reject; update the comprehension in
get_users (which iterates instance.mailbox.accesses.all()) to require both that
access.role is in enums.MAILBOX_ROLES_CAN_BE_ASSIGNED and that access.role is
not the thread-viewer role (e.g. check access.role !=
enums.ThreadAccess.Role.VIEWER or the equivalent enum constant used in your
codebase), then continue sorting and serializing as before.

In `@src/backend/core/services/thread_events.py`:
- Around line 335-345: The code is directly calling uuid.UUID(...) on incoming
assignee IDs which can raise ValueError and bypass intended validation; replace
the manual parsing with the existing helper _parse_and_dedupe_user_ids() to
validate and dedupe incoming IDs, use its returned set/list instead of
assignee_ids when building already_assigned and when filtering assignees_data to
form new_assignees (so you don't reparse inside the comprehensions), and apply
the same change in both assign_users() and unassign_users() code paths (also
update the block currently creating already_assigned and new_assignees and the
similar block around lines 387-399).

In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 698-703: A class declaration (TestThreadEventAssign) was inserted
before the body of test_read_mention_viewer_access, causing that test and
several subsequent tests (test_im_viewer_forbidden,
test_read_mention_unauthenticated) to have their assertions attached to the
wrong test functions; fix by moving the TestThreadEventAssign class declaration
below the complete body of test_read_mention_viewer_access (and below the
related read-mention PATCH/assertion block), restore the intended bodies so the
read-mention assertions belong to test_read_mention_viewer_access, ensure the IM
unauthenticated POST check remains in test_read_mention_unauthenticated and the
viewer-forbidden IM assertions are in test_im_viewer_forbidden, and update any
test names if their behavior was changed so each test focuses on a single use
case.

In `@src/backend/core/tests/api/test_threads_list.py`:
- Around line 1879-1886: Tests currently risk creating duplicate UserEvent
ASSIGN rows because ThreadEventFactory has a _sync_user_events post-generation
hook that auto-creates UserEvent rows for ASSIGN-type ThreadEvents; update this
test to avoid manual creation when the ThreadEvent is ASSIGN by either removing
the explicit UserEventFactory(...) call and relying on ThreadEventFactory to
generate assignee UserEvent(s), or guard the manual creation with a check on
thread_event.type != enums.ThreadEventTypeChoices.ASSIGN so you only call
UserEventFactory when the ThreadEventFactory will not auto-create them.

In `@src/frontend/public/locales/common/fr-FR.json`:
- Around line 639-640: The "Search teammates" translation key in the fr-FR
locale is empty which causes a blank label; update the "Search teammates" entry
in src/frontend/public/locales/common/fr-FR.json to provide an appropriate
French translation (e.g., "Rechercher des coéquipiers" or "Rechercher des
collaborateurs") or remove the key so the app falls back to the English string;
ensure you edit the exact JSON key "Search teammates" to avoid introducing
syntax errors.

In
`@src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/_index.scss`:
- Around line 128-138: The visually-hidden utility
.quick-assign-popover__sr-only uses the deprecated clip property; replace it
with a modern clip-path declaration to maintain the same behavior: remove the
line using clip: rect(0, 0, 0, 0); and add clip-path: inset(50%); (optionally
include -webkit-clip-path: inset(50%) for broader browser support) while keeping
the existing position, size, padding, margin, overflow, white-space and border
rules to preserve the accessibility pattern.

In
`@src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/index.tsx`:
- Around line 154-168: Dialog does not accept the onKeyDownCapture prop and the
build fails; remove the onKeyDownCapture from the Dialog element and move the
Escape interception logic into a descendant focusable/container element (e.g., a
wrapper div or the Dialog's content element) that supports keyboard events,
attaching an onKeyDown or onKeyDownCapture handler there which checks e.key ===
"Escape", calls e.preventDefault()/e.stopPropagation() and then calls
onOpenChange(false); ensure the handler references the same onOpenChange prop
and keep the explanatory comment near the new handler.

In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss`:
- Around line 43-90: Remove the empty `//` comment lines inside the top comment
block and inside the `::before` block (replace with non-empty comment or delete
them) to fix scss/comment-no-empty, and remove the blank lines that precede
declarations inside the pseudo-element blocks so they no longer trigger
declaration-empty-line-before; specifically update the selectors
`&__row--assigned .c__avatar, &__share-member-item--assigned .c__avatar` and
their `&::after` (and the corresponding `::before` block) to eliminate empty
comment lines and extra blank lines between property groups.

In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx`:
- Around line 45-53: The Unassign Button in the assigned user row (the JSX
Button that calls onUnassign(user.id)) does not respect the unassign mutation's
in-flight state, allowing double-clicks to enqueue multiple UNASSIGN mutations;
fix by wiring the hook's in-flight flag into this Button (e.g., compute a
per-user boolean like isUnassigning = isMutatingForUser(user.id) or use the
hook's global isMutating when appropriate) and set the Button to
disabled={isUnassigning} (and/or show loading) when true so the onClick handler
cannot fire while the mutation is pending; update the code near canUpdate,
Button, onUnassign and user.id to use that flag.

In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx`:
- Around line 272-321: The filter that builds searchMemberResult uses reference
equality (!pendingInvitationUsers.includes(user)) which breaks when server
returns new object instances; change it to compare ids: build a Set of pending
ids (e.g. const pendingIds = new Set(pendingInvitationUsers.map(u => u.id))) and
then filter with (user) => !pendingIds.has(user.id); update the
searchMemberResult declaration in the same memo block (and ensure user.id is
used consistently, e.g. in the isInvitationMode check and endActions newUser.id)
so duplicates are removed by id rather than object identity.

In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx`:
- Around line 95-103: The rendered rows should come from the patched /accesses/
cache rather than the stale accesses prop: update enrichedAccesses and
normalizedAccesses to read from the threadsAccessesList cache (use
queryClient.getQueryData(getThreadsAccessesListQueryKey(selectedThread.id)) or a
useQuery keyed to that cache) and fall back to props.accesses only if the cache
is empty; keep patchAccessesCache as-is but ensure guards like hasOnlyOneEditor
and can_delete compute against that cached list (not accesses) so role
changes/deletes reflect immediately before invalidateThreadMessages refetches.

In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.ts`:
- Around line 57-65: When handling system UNASSIGN in the component branch using
isSystem, isAssign, selfInAssignees and assignees, don't return the singular
string t("You were unassigned") whenever the current user is in assignees; only
use that string when assignees.length === 1. If selfInAssignees and there are
multiple assignees, render a combined message that preserves all names by
mapping the assignees array to a display list that replaces the current user's
entry with a localized "You" (via t("You")) and then pass that joined list and
count into t("{{assignees}} was unassigned") so other users affected by the same
event are still shown.

In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 54-84: groupSystemEvents is currently buffering every non-IM
system event which causes CollapsedEventsGroup (which only knows how to render
assign/unassign) to drop unrelated events when a collapsed group is expanded;
change the grouping predicate so the buffer only accumulates assignment-related
events (e.g., check event.type or a helper like isAssignmentEvent for
ThreadEventType values such as 'assign'/'unassign') and leave other system
events to be flushed immediately (preserve behavior for message events and IM
events), keep using COLLAPSE_THRESHOLD and flushBuffer as-is; add a regression
test that constructs a timeline containing a mix of assign/unassign and another
system event type in a 3+ sequence and assert that only contiguous
assign/unassign events are collapsed while other events remain as individual
entries.

In `@src/frontend/src/features/message/use-thread-assignment.ts`:
- Around line 57-61: dispatchAssignEvent exits early when selectedThread?.id is
falsy without calling options?.onSettled, leaving assigningUserId (set by
assignUser) stuck and UI disabled; update dispatchAssignEvent to call
options?.onSettled() before returning in the if (!selectedThread?.id) branch so
the pending state always clears, and ensure callers (assignUser) still set
assigningUserId as before while relying on the settled callback to reset UI
state.

---

Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5575-5621: The OpenAPI operation threads_events_create currently
documents a 201 response only while its description states it may return 204 for
no-op ASSIGN/UNASSIGN cases; update the operation's responses object to include
a "204" response entry (no content) with a clear description (e.g., "No-op:
nothing changed, no content") so generated clients can model this success path
alongside the existing 201 -> ThreadEvent schema; ensure the new "204" response
has no response body/schema.
- Around line 6031-6046: The openapi enum for the query parameter stats_fields
is missing the documented "_unread" variants, causing clients to be unable to
send the values described in the description; update the enum in openapi.json
for the stats_fields schema to include all "_unread" counterparts for the
boolean fields (add entries such as has_trashed_unread, has_draft_unread,
has_starred_unread, has_attachments_unread, has_archived_unread,
has_sender_unread, has_active_unread, has_delivery_pending_unread,
has_delivery_failed_unread, is_spam_unread, has_messages_unread,
has_unread_mention_unread if intended or correct that particular name,
has_mention_unread, has_assigned_to_me_unread, has_unassigned_unread) while
keeping the existing "all" and "all_unread" values so the enum matches the
description for stats_fields.

In `@src/backend/core/api/viewsets/thread.py`:
- Around line 420-445: The validation allows "_unread" suffixes for annotations
because has_assigned_to_me and has_unassigned were left only in
valid_base_fields; update the annotation_fields set to include
"has_assigned_to_me" and "has_unassigned" so their "_unread" variants are
rejected during stats_fields parsing (preventing building Q(has_unassigned=True)
for a non-model column). Modify the annotation_fields definition near
valid_base_fields/special_fields to add these two symbols.
🪄 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

Run ID: 55348f92-085c-47d9-8fdd-1e7bc2680c03

📥 Commits

Reviewing files that changed from the base of the PR and between 5ca5932 and e08c6b6.

⛔ Files ignored due to path filters (32)
  • src/frontend/src/features/api/gen/models/index.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/mailbox.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/mailbox_light.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/paginated_thread_access_list.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/patched_thread_event_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/patched_thread_event_request_data_one_of.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/patched_thread_event_request_data_one_of_mentions_item.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_access.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_assignees_data.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_assignees_data_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_data.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_data_one_of.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_data_one_of_mentions_item.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_data_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_im_data.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_im_data_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_request_data_one_of.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_type_enum.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_user.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_event_user_request.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_mentionable_user.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/thread_mentionable_user_custom_attributes.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/threads_accesses_list_params.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/threads_list_params.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/thread-access/thread-access.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/thread-events/thread-events.ts is excluded by !**/gen/**
  • src/frontend/src/features/api/gen/thread-users/thread-users.ts is excluded by !**/gen/**
📒 Files selected for processing (68)
  • docs/permissions.md
  • src/backend/core/admin.py
  • src/backend/core/api/openapi.json
  • src/backend/core/api/permissions.py
  • src/backend/core/api/serializers.py
  • src/backend/core/api/viewsets/mailbox_access.py
  • src/backend/core/api/viewsets/thread.py
  • src/backend/core/api/viewsets/thread_access.py
  • src/backend/core/api/viewsets/thread_event.py
  • src/backend/core/api/viewsets/thread_user.py
  • src/backend/core/enums.py
  • src/backend/core/factories.py
  • src/backend/core/migrations/0026_userevent_usrevt_user_thread_assign_uniq.py
  • src/backend/core/models.py
  • src/backend/core/services/thread_events.py
  • src/backend/core/signals.py
  • src/backend/core/tests/api/test_mailboxes.py
  • src/backend/core/tests/api/test_provisioning_mailbox.py
  • src/backend/core/tests/api/test_thread_access.py
  • src/backend/core/tests/api/test_thread_event.py
  • src/backend/core/tests/api/test_thread_filter_assignment.py
  • src/backend/core/tests/api/test_thread_user.py
  • src/backend/core/tests/api/test_threads_list.py
  • src/backend/core/tests/models/test_thread_event.py
  • src/backend/core/tests/models/test_user_event.py
  • src/backend/core/tests/services/test_thread_events.py
  • src/backend/core/utils.py
  • src/frontend/public/locales/common/en-US.json
  • src/frontend/public/locales/common/fr-FR.json
  • src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scss
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsx
  • src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx
  • src/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.ts
  • src/frontend/src/features/layouts/components/thread-view/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/index.ts
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/index.tsx
  • src/frontend/src/features/message/use-assigned-users.ts
  • src/frontend/src/features/message/use-thread-assignment.ts
  • src/frontend/src/features/providers/mailbox.tsx
  • src/frontend/src/features/ui/components/assignees-avatar-group/_index.scss
  • src/frontend/src/features/ui/components/assignees-avatar-group/index.test.tsx
  • src/frontend/src/features/ui/components/assignees-avatar-group/index.tsx
  • src/frontend/src/features/utils/date-helper.ts
  • src/frontend/src/hooks/use-is-shared-context.ts
  • src/frontend/src/styles/main.scss
💤 Files with no reviewable changes (1)
  • src/backend/core/signals.py

Comment thread src/backend/core/admin.py
Comment thread src/backend/core/admin.py
Comment thread src/backend/core/api/openapi.json
Comment thread src/backend/core/api/serializers.py
Comment thread src/backend/core/services/thread_events.py
Comment thread src/frontend/src/features/message/use-thread-assignment.ts Outdated
@jbpenrath
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx (1)

329-339: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the patched access list for the trigger count.

The modal rows now come from enrichedAccesses, but the badge/tooltip still read accesses.length. After an add/remove, the trigger can keep showing the old member count until the parent thread payload refreshes.

♻️ Minimal fix
-            <Tooltip content={t('See members of this thread ({{count}} members)', { count: accesses.length })}>
+            <Tooltip content={t('See members of this thread ({{count}} members)', { count: enrichedAccesses.length })}>
                 <Button
                     variant="tertiary"
                     size="nano"
-                    aria-label={t('See members of this thread ({{count}} members)', { count: accesses.length })}
+                    aria-label={t('See members of this thread ({{count}} members)', { count: enrichedAccesses.length })}
                     className="thread-accesses-widget"
                     onClick={() => setIsShareModalOpen(true)}
                     icon={<Icon name="group" type={IconType.FILLED} />}
                 >
-                    {accesses.length}
+                    {enrichedAccesses.length}
                 </Button>
             </Tooltip>
🤖 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-accesses-widget/index.tsx`
around lines 329 - 339, The tooltip and badge are using accesses.length which
can be stale; switch them to use enrichedAccesses.length so the trigger reflects
the patched access list immediately. Update the Tooltip content and Button
aria-label and children to reference enrichedAccesses.length instead of
accesses.length (same JSX around Tooltip/Button with className
"thread-accesses-widget" and onClick setIsShareModalOpen) so the displayed count
matches the modal rows sourced from enrichedAccesses after add/remove.
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss (1)

49-49: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Three Stylelint [error]-level violations remain from the previous review — these will still fail CI.

The ::after blank-line fixes were applied, but the following three errors are still present:

Line Rule Violation
49 scss/comment-no-empty Bare // inside the "assigned avatar badge" comment block
111 scss/comment-no-empty Bare // inside the "shared avatar" comment block
138 declaration-empty-line-before Empty line before position: absolute inside ::before
🔧 Proposed fix
-    //
-    // Implemented via `::after` on the upstream `.c__avatar` so we don't
+    // Implemented via `::after` on the upstream `.c__avatar` so we don't
-    //
-    // Two anchors trigger the treatment:
+    // Two anchors trigger the treatment:
             white-space: nowrap;
-
             position: absolute;

Also applies to: 111-111, 138-138

🤖 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/share-modal-extensions/_index.scss`
at line 49, Remove the empty single-line SCSS comments and the extra blank line:
replace the bare `//` placeholders in the "assigned avatar badge" and "shared
avatar" comment blocks with either meaningful comments (e.g., `/* assigned
avatar badge */`) or delete them entirely, and delete the empty line before the
`position: absolute` declaration inside the `::before` rule so there is no blank
line preceding that declaration; target the comment locations described as the
assigned avatar badge and shared avatar comment blocks and the `::before`
pseudo-element where `position: absolute` is declared.
🤖 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/tests/api/test_thread_event.py`:
- Around line 1121-1126: The tests hardcode a 10-minute offset to push an ASSIGN
outside the undo window; instead import and use UNDO_WINDOW_SECONDS from the
thread_events module and replace the fixed timedelta with
timedelta(seconds=UNDO_WINDOW_SECONDS + 1) when calling _force_created_at on the
ThreadEvent (e.g., for the ASSIGN event looked up via
models.ThreadEvent.objects.get(thread=thread,
type=enums.ThreadEventTypeChoices.ASSIGN)); apply the same change to the other
occurrences mentioned so the test derives the "outside undo window" timestamp
from the production constant.
- Around line 666-690: Split the combined assertions into separate unit tests
that directly verify serializer-level dispatch: create one test per event type
(e.g., test_serializer_validate_dispatches_for_im and
test_serializer_validate_dispatches_for_assign) and in each test either patch
models.ThreadEvent.validate_data to assert it was called with the serializer
input or instantiate ThreadEventSerializer with sample data and call
serializer.is_valid() to exercise ThreadEventSerializer.validate() in isolation;
ensure each test asserts that validate_data was invoked (or that
serializer.errors include the expected field like "content" or "assignees")
rather than relying on the full API 400 response, and reference
ThreadEventSerializer.validate and models.ThreadEvent.validate_data when
locating the code to modify.
- Around line 882-893: Extend the test_create_assign_self_assign test to assert
that the self-assign created the expected UserEvent record: after posting via
get_thread_event_url, query the UserEvent model for an active ASSIGN event for
the current user and thread (use the UserEvent class and its ASSIGN constant or
enum) and assert it exists (and optionally its assignees/owner matches user.id
and thread.id). This ensures the self-assignment path actually creates the
active UserEvent row rather than relying solely on the 201 response.

In `@src/frontend/public/locales/common/fr-FR.json`:
- Line 640: The "Search users" localization key has an empty French value
causing a blank label; update the fr-FR translation for the "Search users" key
(same pattern as the existing "Search teammates" fix) by providing the proper
French string (e.g., "Rechercher des utilisateurs") so French UI falls back
correctly and no blank placeholder appears.

In
`@src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/index.tsx`:
- Around line 121-139: The live-announcement calls are executed before server
confirmation; remove the setAnnouncement(...) calls from the immediate toggle
loop around assignUser and unassignUser and instead trigger announcements only
after the mutation confirms success (e.g., in the mutation's onSuccess callback)
or derive announcements from the post-invalidation/updated assignedUserIds
state; specifically, stop calling setAnnouncement inside the loop that uses
assignUser/unassignUser and move the success-message logic to the mutation
success handler or a useEffect that compares previous vs current assignedUserIds
(use symbols: assignUser, unassignUser, setAnnouncement, assignedUserIds,
newKeys, users to locate the code to change).
- Around line 111-145: The handler allows starting a second toggle while
mutatingUserId is set, which can overwrite that in-flight id; to fix, make the
menu disable all toggles while a mutation is pending by computing disabledKeys
from the users list when mutatingUserId is truthy (e.g., set disabledKeys =
mutatingUserId ? new Set(users.map(u => u.id)) : undefined), leaving
handleSelectionChange, assignUser, unassignUser and setAnnouncement logic
unchanged so only one mutation can be in flight at a time.

In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx`:
- Around line 219-248: Add a cleanup to clear the pending debounce timer when
the component unmounts: in this component, use an effect with no deps that in
its cleanup checks searchUserTimeoutRef.current and calls clearTimeout(...) and
sets it to null to avoid firing onSearchUsers after unmount; reference the
existing searchUserTimeoutRef and onSearchUser function to locate where to add
the useEffect cleanup.

In `@src/frontend/src/features/message/use-thread-assignment.ts`:
- Line 61: assignedUserIds is recreated on every render; wrap its creation in
React's useMemo to avoid repeated Set construction by computing it only when
assignedUsers changes (e.g., memoize new Set(assignedUsers.map(u => u.id)) with
a dependency on assignedUsers or on assignedUsers' ids). Update the module to
import useMemo if not already present and replace the inline Set creation
(assignedUserIds) with the useMemo-backed value so consumers use the memoized
Set.

---

Outside diff comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx`:
- Around line 329-339: The tooltip and badge are using accesses.length which can
be stale; switch them to use enrichedAccesses.length so the trigger reflects the
patched access list immediately. Update the Tooltip content and Button
aria-label and children to reference enrichedAccesses.length instead of
accesses.length (same JSX around Tooltip/Button with className
"thread-accesses-widget" and onClick setIsShareModalOpen) so the displayed count
matches the modal rows sourced from enrichedAccesses after add/remove.

---

Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss`:
- Line 49: Remove the empty single-line SCSS comments and the extra blank line:
replace the bare `//` placeholders in the "assigned avatar badge" and "shared
avatar" comment blocks with either meaningful comments (e.g., `/* assigned
avatar badge */`) or delete them entirely, and delete the empty line before the
`position: absolute` declaration inside the `::before` rule so there is no blank
line preceding that declaration; target the comment locations described as the
assigned avatar badge and shared avatar comment blocks and the `::before`
pseudo-element where `position: absolute` is declared.
🪄 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

Run ID: 14ecd9da-16b2-4413-bd73-278727ff9507

📥 Commits

Reviewing files that changed from the base of the PR and between e08c6b6 and 93013e9.

📒 Files selected for processing (17)
  • src/backend/core/admin.py
  • src/backend/core/tests/api/test_thread_event.py
  • src/e2e/src/__tests__/thread-event.spec.ts
  • src/frontend/public/locales/common/en-US.json
  • src/frontend/public/locales/common/fr-FR.json
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.ts
  • src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
  • src/frontend/src/features/message/use-thread-assignment.ts

Comment thread src/backend/core/tests/api/test_thread_event.py Outdated
Comment thread src/backend/core/tests/api/test_thread_event.py
Comment thread src/backend/core/tests/api/test_thread_event.py Outdated
Comment thread src/frontend/public/locales/common/fr-FR.json Outdated
Comment thread src/frontend/src/features/message/use-thread-assignment.ts Outdated
@jbpenrath
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/tests/api/test_thread_event.py`:
- Around line 691-701: Add a new unit test mirroring
test_serializer_validate_dispatches_validate_data_for_assign but asserting that
ThreadEventSerializer forwards the literal "unassign" type to
models.ThreadEvent.validate_data: create payload = {"type": "unassign", "data":
{"assignees":[...]}}, patch models.ThreadEvent.validate_data, instantiate
ThreadEventSerializer(data=payload), call serializer.is_valid() and then assert
mock_validate.assert_called_once_with("unassign", {"assignees":[assignee]});
reference the existing
test_serializer_validate_dispatches_validate_data_for_assign and
ThreadEventSerializer and models.ThreadEvent.validate_data to locate where to
add this new test.
- Around line 156-167: The test currently uses a random UUID for the first
assignee which may trigger "user not assignable/not found" before the malformed
UUID is validated; update the first assignee in the data payload used in
test_thread_event.py so it references a known valid assignable user (e.g., the
test fixture user or thread.creator.id as a string) so the request proceeds past
existence checks and properly exercises the malformed UUID path for the second
assignee ("not-a-uuid"); keep the rest of the payload and the assertion for
status.HTTP_400_BAD_REQUEST unchanged.
🪄 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

Run ID: d861db82-3384-40d4-8b1f-124f9ca9f3ed

📥 Commits

Reviewing files that changed from the base of the PR and between 93013e9 and 654bb09.

📒 Files selected for processing (9)
  • src/backend/core/tests/api/test_thread_event.py
  • src/e2e/src/__tests__/thread-event.spec.ts
  • src/frontend/public/locales/common/fr-FR.json
  • src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/quick-assign-popover/index.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx
  • src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx
  • src/frontend/src/features/message/use-thread-assignment.ts

Comment on lines +156 to +167
data = {
"type": "assign",
"data": {
"assignees": [
{"id": str(uuid.uuid4()), "name": "Alice"},
{"id": "not-a-uuid", "name": "Broken"},
]
},
}

response = api_client.post(get_thread_event_url(thread.id), data, format="json")
assert response.status_code == status.HTTP_400_BAD_REQUEST
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Make the control assignee valid so this actually exercises the malformed UUID path.

Right now the first assignees[].id is just a random UUID, so this can return 400 for "user not assignable / not found" before "not-a-uuid" is ever checked. That makes the regression guard a false positive for the FormatChecker bug described in the docstring.

Possible minimal fix
-        user, _mailbox, thread = setup_user_with_thread_access()
+        user, mailbox, thread = setup_user_with_thread_access()
         api_client.force_authenticate(user=user)
+        assignee = factories.UserFactory()
+        factories.MailboxAccessFactory(
+            mailbox=mailbox, user=assignee, role=enums.MailboxRoleChoices.ADMIN
+        )

         data = {
             "type": "assign",
             "data": {
                 "assignees": [
-                    {"id": str(uuid.uuid4()), "name": "Alice"},
+                    {"id": str(assignee.id), "name": "Alice"},
                     {"id": "not-a-uuid", "name": "Broken"},
                 ]
             },
         }
As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases".
🤖 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/api/test_thread_event.py` around lines 156 - 167, The
test currently uses a random UUID for the first assignee which may trigger "user
not assignable/not found" before the malformed UUID is validated; update the
first assignee in the data payload used in test_thread_event.py so it references
a known valid assignable user (e.g., the test fixture user or thread.creator.id
as a string) so the request proceeds past existence checks and properly
exercises the malformed UUID path for the second assignee ("not-a-uuid"); keep
the rest of the payload and the assertion for status.HTTP_400_BAD_REQUEST
unchanged.

Comment on lines +691 to +701
def test_serializer_validate_dispatches_validate_data_for_assign(self):
"""Same wiring for ASSIGN; covers UNASSIGN by code-path equivalence
(both share ``_ASSIGNEES_SCHEMA`` in ``ThreadEvent.DATA_SCHEMAS``).
"""
assignee = {"id": str(uuid.uuid4()), "name": "Jane"}
payload = {"type": "assign", "data": {"assignees": [assignee]}}
with patch.object(models.ThreadEvent, "validate_data") as mock_validate:
serializer = ThreadEventSerializer(data=payload)
assert serializer.is_valid(), serializer.errors

mock_validate.assert_called_once_with("assign", {"assignees": [assignee]})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add a dedicated serializer dispatch test for unassign.

Using the same schema as assign does not prove ThreadEventSerializer.validate() forwards the literal "unassign" type. A hardcoded "assign" here would still pass this test and most of the API happy-path coverage.

Suggested addition
     def test_serializer_validate_dispatches_validate_data_for_assign(self):
         """Same wiring for ASSIGN; covers UNASSIGN by code-path equivalence
         (both share ``_ASSIGNEES_SCHEMA`` in ``ThreadEvent.DATA_SCHEMAS``).
         """
         assignee = {"id": str(uuid.uuid4()), "name": "Jane"}
         payload = {"type": "assign", "data": {"assignees": [assignee]}}
         with patch.object(models.ThreadEvent, "validate_data") as mock_validate:
             serializer = ThreadEventSerializer(data=payload)
             assert serializer.is_valid(), serializer.errors

         mock_validate.assert_called_once_with("assign", {"assignees": [assignee]})
+
+    def test_serializer_validate_dispatches_validate_data_for_unassign(self):
+        """UNASSIGN should be forwarded to ``validate_data`` with its own type."""
+        assignee = {"id": str(uuid.uuid4()), "name": "Jane"}
+        payload = {"type": "unassign", "data": {"assignees": [assignee]}}
+        with patch.object(models.ThreadEvent, "validate_data") as mock_validate:
+            serializer = ThreadEventSerializer(data=payload)
+            assert serializer.is_valid(), serializer.errors
+
+        mock_validate.assert_called_once_with("unassign", {"assignees": [assignee]})
As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases".
🤖 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/api/test_thread_event.py` around lines 691 - 701, Add
a new unit test mirroring
test_serializer_validate_dispatches_validate_data_for_assign but asserting that
ThreadEventSerializer forwards the literal "unassign" type to
models.ThreadEvent.validate_data: create payload = {"type": "unassign", "data":
{"assignees":[...]}}, patch models.ThreadEvent.validate_data, instantiate
ThreadEventSerializer(data=payload), call serializer.is_valid() and then assert
mock_validate.assert_called_once_with("unassign", {"assignees":[assignee]});
reference the existing
test_serializer_validate_dispatches_validate_data_for_assign and
ThreadEventSerializer and models.ThreadEvent.validate_data to locate where to
add this new test.

@jbpenrath jbpenrath force-pushed the feat/assignation-event branch from 654bb09 to 4e754b9 Compare May 4, 2026 20:08
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
@jbpenrath jbpenrath force-pushed the feat/assignation-event branch from 4e754b9 to 962fa58 Compare May 4, 2026 20:34
@jbpenrath jbpenrath merged commit 5a5d15d into development May 5, 2026
9 checks passed
@jbpenrath jbpenrath deleted the feat/assignation-event branch May 5, 2026 08:59
@coderabbitai coderabbitai Bot mentioned this pull request May 5, 2026
jbpenrath added a commit that referenced this pull request May 5, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 6, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 7, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 7, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 7, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 7, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 11, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath added a commit that referenced this pull request May 12, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
@coderabbitai coderabbitai Bot mentioned this pull request May 12, 2026
jbpenrath added a commit that referenced this pull request May 19, 2026
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent
models a feature to assign users to a thread.
We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned).
The thread share modal has been forked from ui-kit to be able to list users of each
mailbox and add a cta to assign them to the thread. A section above shows assigned users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants