(backend/frontend) move access/invitation cleanup from frontend to backend on document move#2223
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughBackend: DocumentViewSet.move now materializes owner_accesses into a list for root-target moves, detects permission-scope changes (root status changes, cross-root moves, or promotion-to-root as a sibling), and when scope changes deletes direct document.accesses and document.invitations inside the same transaction before completing the move; ownership-repair uses the precomputed owner_accesses. Frontend: useMoveDoc removes the deleteAccessOnMove parameter and client-side deletion side effects; its call sites were updated. Tests: new and updated tests cover root, cross-tree, same-tree moves, promotions to root, sibling-root moves, owner preservation, and transactional rollback on failure. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 3
🤖 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/api/viewsets.py`:
- Around line 965-971: Materialize the owner_accesses queryset before deleting
direct accesses: before calling document.accesses.all().delete() (and
document.invitations.all().delete()), evaluate owner_accesses into a concrete
list (e.g. owner_accesses = list(owner_accesses)) so the later owner-recreation
loop can iterate over the preserved items; update the code that computes
owner_accesses to materialize it and then proceed with the deletes and the owner
recreation logic.
- Around line 969-971: The cleanup condition incorrectly only compares current
roots; instead compute the prospective root after the move and use that to
decide whether to drop direct accesses/invitations. Change the logic around
document.is_root(), document.get_root(), and target_document.get_root() to
determine the new root post-move (e.g., if moving as a child use
target_document.get_root(), if moving as a sibling use
target_document.get_parent() and its root or None if that parent is root) and
then run document.accesses.all().delete() and
document.invitations.all().delete() when document.is_root() or
document.get_root() != prospective_root (i.e., when the root membership changes
after the move).
In `@src/backend/core/tests/documents/test_api_documents_move.py`:
- Around line 594-620: The test currently fails before the deletion block
because the target lookup uses a random UUID; change it to use a real target
document and force Document.move to raise after the deletes so rollback can be
observed. Concretely: create a real target_document (e.g. via
factories.DocumentFactory()) and pass its id in the POST, and monkeypatch or
temporarily stub Document.move (the method on the Document model) to raise an
exception when called; then call client.post on the move endpoint and assert the
response indicates failure and that document.accesses.count() and
document.invitations.count() remain unchanged (verifying the deletion roll
back).
🪄 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: CHILL
Plan: Pro
Run ID: 56acc976-7a4f-4750-b14f-c62dbde5d92e
📒 Files selected for processing (5)
src/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_move.pysrc/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx
maboukerfa
force-pushed
the
feat/move-delete-accesses-backend
branch
from
0c0f025 to
99bfe78
Compare
maboukerfa
force-pushed
the
feat/move-delete-accesses-backend
branch
from
99bfe78 to
ffe8b59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/documents/test_api_documents_move.py`:
- Line 697: Add a short comment above the monkeypatch line explaining the test's
contract: that the API view under test invokes models.Document.move (inherited
from treebeard.MP_Node) and therefore the test injects failure via
monkeypatch.setattr(models.Document, "move", failing_move) to exercise rollback;
note that if the view later calls a differently-named method the test intent
must be updated. This documents the dependency on models.Document.move and the
purpose of the failing_move injection so future refactors don't silently break
the test.
🪄 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: 9403005d-2240-4fea-9cae-5a823958856d
📒 Files selected for processing (2)
src/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_move.py
|
The backend part is ok for me. |
AntoLC
left a comment
AntoLC
left a comment
There was a problem hiding this comment.
✅ Frontend side seems good.
lunika
force-pushed
the
feat/move-delete-accesses-backend
branch
from
ffe8b59 to
3714d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/api/viewsets.py (1)
965-995:⚠️ Potential issue | 🔴 CriticalGuard cleanup against sibling no-op moves.
For
LEFT/RIGHT/FIRST_SIBLING/LAST_SIBLING, treebeard does not raise when the target is the document itself or one of its descendants; it leaves the node in place. In that case this block still treats a root document as a scope change and wipes its direct accesses/invitations even though nothing actually moved. Short-circuit those no-op sibling cases before running the cleanup.Based on learnings: In the
suitenumerique/docsproject (Django/treebeard), when moving a document to a sibling position relative to itself or one of its descendants, treebeard performs a no-op and no exception is raised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets.py` around lines 965 - 995, Detect and skip the "no-op" sibling moves before wiping access/invitations: if the position is a sibling move (not FIRST_CHILD/LAST_CHILD — i.e. enums.MoveNodePositionChoices.LEFT/RIGHT/FIRST_SIBLING/LAST_SIBLING) and the target_document is the document itself or a descendant of the document (so the treebeard move will be a no-op), short-circuit and return a successful/no-op response (or skip the cleanup) instead of treating becomes_sibling_root/scope_changes as true; update the logic around becomes_sibling_root, scope_changes and the subsequent document.accesses.all().delete()/document.invitations.all().delete() so cleanup only runs for actual moves (use document vs target_document identity/descendant check before performing deletes).
♻️ Duplicate comments (1)
src/backend/core/tests/documents/test_api_documents_move.py (1)
781-792:⚠️ Potential issue | 🟡 MinorThis rollback test still fails before the delete path runs.
DocumentViewSet.move()now callsdocument.move(...)beforedocument.accesses.all().delete()/document.invitations.all().delete(). Patchingmodels.Document.movehere makes the request abort before the cleanup block, so the final count assertions stay green even if those deletes are no longer transactional. Make the failure happen after cleanup instead.
🤖 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/api/viewsets.py`:
- Around line 993-995: The bulk delete block that runs when scope_changes is
true currently calls document.accesses.all().delete() and
document.invitations.all().delete() which bypasses
DocumentAccessViewSet.perform_destroy() and therefore doesn't notify the
collaboration server; after performing these deletes wrap a call to queue a
document-level reset/notification inside transaction.on_commit(...) so the
collaboration server is informed only after the DB commit — locate the
scope_changes branch in viewsets.py and add a transaction.on_commit(lambda:
<call to existing collaboration reset/notify helper>) so active websocket
sessions are reset/updated after the bulk cleanup.
---
Outside diff comments:
In `@src/backend/core/api/viewsets.py`:
- Around line 965-995: Detect and skip the "no-op" sibling moves before wiping
access/invitations: if the position is a sibling move (not
FIRST_CHILD/LAST_CHILD — i.e.
enums.MoveNodePositionChoices.LEFT/RIGHT/FIRST_SIBLING/LAST_SIBLING) and the
target_document is the document itself or a descendant of the document (so the
treebeard move will be a no-op), short-circuit and return a successful/no-op
response (or skip the cleanup) instead of treating
becomes_sibling_root/scope_changes as true; update the logic around
becomes_sibling_root, scope_changes and the subsequent
document.accesses.all().delete()/document.invitations.all().delete() so cleanup
only runs for actual moves (use document vs target_document identity/descendant
check before performing deletes).
🪄 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: 39f6d9cf-4b5d-44b1-b9c1-3170ed208a5a
📒 Files selected for processing (5)
src/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_move.pysrc/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx
| if scope_changes: | ||
| document.accesses.all().delete() | ||
| document.invitations.all().delete() |
There was a problem hiding this comment.
Reset collaboration sessions after bulk access cleanup.
This bulk delete bypasses the normal access-removal path in DocumentAccessViewSet.perform_destroy(), so the collaboration server is never notified that users lost direct access during the move. Existing websocket sessions can stay alive with stale permissions until they reconnect. Queue a document-level reset in transaction.on_commit(...) when scope_changes is true.
🛠️ Suggested follow-up
if scope_changes:
document.accesses.all().delete()
document.invitations.all().delete()
+ transaction.on_commit(
+ lambda: CollaborationService().reset_connections(str(document.id))
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets.py` around lines 993 - 995, The bulk delete
block that runs when scope_changes is true currently calls
document.accesses.all().delete() and document.invitations.all().delete() which
bypasses DocumentAccessViewSet.perform_destroy() and therefore doesn't notify
the collaboration server; after performing these deletes wrap a call to queue a
document-level reset/notification inside transaction.on_commit(...) so the
collaboration server is informed only after the DB commit — locate the
scope_changes branch in viewsets.py and add a transaction.on_commit(lambda:
<call to existing collaboration reset/notify helper>) so active websocket
sessions are reset/updated after the bulk cleanup.
Access and invitation cleanup on document move is now handled atomically by the backend. The frontend no longer needs to fetch and delete accesses/invitations after a successful move. Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
When a document is moved outside its current permission scope (root document, cross-tree move, or promotion to root), its direct accesses and pending invitations are now deleted server-side within the same atomic transaction as the move itself. This ensures consistency: if the move fails, deletions are rolled back. Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
lunika
force-pushed
the
feat/move-delete-accesses-backend
branch
from
3714d3b to
61a6dd8
Compare
3 participants
Purpose
Move the access and invitation cleanup logic from the frontend to the backend during document move operations.
Problems with the current approach:
an inconsistent state.
breaking the inheritance model.
Screen.Recording.2026-04-16.at.17.29.32.mov
Proposal