(backend/frontend) move access/invitation cleanup from frontend to backend on document move

[maboukerfa]

Purpose

Move the access and invitation cleanup logic from the frontend to the backend during document move operations.

Problems with the current approach:

1. No atomicity: The frontend deletes accesses and invitations via separate HTTP requests after the move succeeds. If the user loses their connection mid-process, the move completes but the old permissions remain, leaving the document in
   an inconsistent state.
2. API consumers bypass cleanup entirely: A direct API call to POST /documents/{id}/move/ moves the document without touching its permissions. This creates sub-documents with their own direct accesses that differ from their parent's,
   breaking the inheritance model.
3. Race condition on deletion: After the move, the frontend fetches the document's accesses to delete them. But since the document has already moved to a new parent, the permissions it retrieves are the inherited ones from the new tree not the old direct accesses. Attempting to delete these returns 404s.

Screen.Recording.2026-04-16.at.17.29.32.mov

Proposal

• Backend: When a document leaves its current permission scope (root document being moved, cross-tree move, or sub-document promoted to root), its direct accesses and pending invitations are deleted within the same @transaction.atomic. block as the move. If the move fails, everything rolls back.
• Frontend: The useMoveDoc hook is simplified to a plain move call, no more manual fetch-and-delete logic.

[coderabbitai]

Walkthrough

The backend DocumentViewSet.move now materializes owner_accesses into a list when moving under a root target, detects permission-scope changes (root status, becoming a sibling root, or moving between trees), and—on scope change—deletes direct document.accesses and pending document.invitations inside the same @transaction.atomic block before calling document.move. Ownership-repair uses the precomputed owner_accesses when applicable. Frontend useMoveDoc dropped the deleteAccessOnMove parameter and removed invitation/access deletion side effects; its call sites were updated. Tests were expanded to cover root/cross-tree/same-tree moves, promotions to root, sibling-root moves, owner preservation, and transactional rollback on move failure.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main change: moving access/invitation cleanup logic from frontend to backend during document moves.
Description check	✅ Passed	The description is directly related to the changeset, explaining the purpose, problems being solved, and the proposed backend/frontend implementation.
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between e59d8a4 and 0c0f025.

📒 Files selected for processing (5)

• src/backend/core/api/viewsets.py
• src/backend/core/tests/documents/test_api_documents_move.py
• src/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsx
• src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx
• src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 99bfe78 and ffe8b59.

📒 Files selected for processing (2)

• src/backend/core/api/viewsets.py
• src/backend/core/tests/documents/test_api_documents_move.py

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm that the move method invoked by the viewset is indeed Document.move
# (inherited from treebeard) and not a custom override or a differently-named method.
ast-grep --pattern $'class Document($$$):
  $$$
  def move($$$):
    $$$
  $$$'

# Also confirm the viewset call site
rg -nP --type=py -C1 '\bdocument\.move\s*\(' src/backend/core/api/viewsets.py

Repository: suitenumerique/docs

Length of output: 130

---

🏁 Script executed:

# Check Document class definition and inheritance
rg -nP --type=py 'class Document\(' src/backend/core/models.py | head -20

Repository: suitenumerique/docs

Length of output: 103

---

🏁 Script executed:

# Read the test code around line 697 for context
head -n 710 src/backend/core/tests/documents/test_api_documents_move.py | tail -n 30

Repository: suitenumerique/docs

Length of output: 1253

---

Consider documenting the move method contract assumption in the test.

The monkeypatch target models.Document.move is correct—Document inherits move from treebeard's MP_Node and does not override it. The test properly exercises the rollback path by injecting a failure at the inherited method level. To future-proof this test, add a brief comment clarifying that the API view ultimately calls Document.move (inherited from treebeard) and document the dependency contract, so a future refactor that redirects to a differently-named method won't silently break the test's intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/backend/core/tests/documents/test_api_documents_move.py` at 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.

[lunika]

The backend part is ok for me.
I asked @AntoLC a review
I would like to discuss this new behavior with @virgile-dev when he will come back.