diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 8298f211f0..9c876f0f51 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -947,8 +947,8 @@ def move(self, request, *args, **kwargs): "as a child to this target document." ) elif target_document.is_root(): - owner_accesses = document.get_root().accesses.filter( - role=models.RoleChoices.OWNER + owner_accesses = list( + document.get_root().accesses.filter(role=models.RoleChoices.OWNER) ) elif not target_document.get_parent().get_abilities(user).get("move"): message = ( @@ -970,6 +970,30 @@ def move(self, request, *args, **kwargs): status=status.HTTP_400_BAD_REQUEST, ) + # A move changes the document's permission scope in any of these cases: + # - it is currently a root (it carries its own scope), + # - it is moving into a different tree (different current root than target's), + # - it is being promoted to root as a sibling of its own current root. + # In all these cases, direct accesses and pending invitations must be wiped so + # the document inherits the new scope. Deletions and the move share the same + # atomic transaction, so a failure rolls everything back. + becomes_sibling_root = ( + position + not in [ + enums.MoveNodePositionChoices.FIRST_CHILD, + enums.MoveNodePositionChoices.LAST_CHILD, + ] + and target_document.is_root() + ) + scope_changes = ( + document.is_root() + or becomes_sibling_root + or document.get_root() != target_document.get_root() + ) + if scope_changes: + document.accesses.all().delete() + document.invitations.all().delete() + # Make sure we have at least one owner if ( owner_accesses diff --git a/src/backend/core/tests/documents/test_api_documents_move.py b/src/backend/core/tests/documents/test_api_documents_move.py index 4cd3727b0a..ebab88c20d 100644 --- a/src/backend/core/tests/documents/test_api_documents_move.py +++ b/src/backend/core/tests/documents/test_api_documents_move.py @@ -237,10 +237,10 @@ def test_api_documents_move_authenticated_no_owner_user_and_team(): document.refresh_from_db() assert list(document.get_children()) == [child] - assert document.accesses.count() == 3 + + assert document.accesses.count() == 2 assert document.accesses.get(user__isnull=False, role="owner").user == parent_owner assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" - assert document.accesses.get(role="administrator").user == user def test_api_documents_move_authenticated_no_owner_same_user(): @@ -304,8 +304,10 @@ def test_api_documents_move_authenticated_no_owner_same_team(): document.refresh_from_db() assert list(document.get_children()) == [child] - assert document.accesses.count() == 2 - assert document.accesses.get(user__isnull=False, role="administrator").user == user + + # The user's direct administrator access was wiped during the cross-tree move; + # only the "lasuite" team remains, re-added as owner from the previous root. + assert document.accesses.count() == 1 assert document.accesses.get(user__isnull=True, role="owner").team == "lasuite" @@ -527,3 +529,269 @@ def test_api_documents_move_to_self(position): # Ensure document has not moved document.refresh_from_db() assert document.is_root() is True + + +def test_api_documents_move_root_deletes_accesses_and_invitations(): + """ + Moving a root document should automatically delete all its direct accesses and + invitations so it inherits the target's permissions. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + factories.InvitationFactory(document=document) + + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 2 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 0 + assert document.invitations.count() == 0 + + +def test_api_documents_move_cross_tree_deletes_accesses_and_invitations(): + """ + Moving a non-root document to a different tree should delete its direct accesses + and invitations because it is leaving its current permission scope. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + source_root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=source_root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + target_root = factories.DocumentFactory(users=[(user, "owner")]) + target = factories.DocumentFactory(parent=target_root, users=[(user, "owner")]) + + assert not document.is_root() + assert document.get_root() != target.get_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 0 + assert document.invitations.count() == 0 + + +def test_api_documents_move_same_tree_keeps_accesses_and_invitations(): + """ + Moving a non-root document within the same tree should preserve its direct + accesses and invitations since it stays in the same permission scope. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + target = factories.DocumentFactory(parent=root, users=[(user, "owner")]) + + assert not document.is_root() + assert document.get_root() == target.get_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-child"}, + ) + + assert response.status_code == 200 + assert response.json() == {"message": "Document moved successfully."} + + document.refresh_from_db() + assert document.is_child_of(target) + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_sub_document_to_root_deletes_accesses_and_invitations( + position, +): + """ + Moving a sub-document to root level (as a sibling of a root document) changes its + permission scope. Its direct accesses and invitations must be deleted. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + parent = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=parent, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + # target is a root document; moving as its sibling promotes document to root level + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert not document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": position}, + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.is_root() + # Direct accesses and invitations are wiped; the backend then ensures at least one + # owner exists by inheriting the owner(s) from the previous root. + assert document.accesses.count() == 1 + assert document.accesses.filter(role="owner").exists() + assert document.invitations.count() == 0 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_sub_document_as_sibling_of_its_own_root_deletes_accesses( + position, +): + """ + Moving a sub-document as a sibling of its current root promotes it to a + new root. Even though before the move both share the same root, the document is + leaving its permission scope and its direct accesses/invitations must be deleted. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + root = factories.DocumentFactory(users=[(user, "owner")]) + document = factories.DocumentFactory( + parent=root, users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + assert not document.is_root() + assert document.get_root() == root + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(root.id), "position": position}, + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.is_root() + # Original accesses wiped; owner re-inherited from previous root ensures non-orphaned. + assert document.accesses.count() == 1 + assert document.accesses.filter(role="owner").exists() + assert document.invitations.count() == 0 + + +@pytest.mark.parametrize("position", ["first-sibling", "last-sibling", "left", "right"]) +def test_api_documents_move_root_as_sibling_of_root_preserves_owner(position): + """ + Moving a root document as sibling of another root, the owners + collected from the previous root (which is the document itself) must survive the + pre-move access deletion, so the document keeps at least one owner afterwards. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert target.is_root() + + response = client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": position}, + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.is_root() + # The original editor access and invitation are wiped; the previous root owner + # is re-added so the document still has at least one owner. + assert document.accesses.count() == 1 + assert document.accesses.get(role="owner").user == user + assert document.invitations.count() == 0 + + +def test_api_documents_move_scope_change_deletion_is_atomic(monkeypatch): + """ + When accesses/invitations are to be deleted (root or cross-tree move), both + deletions and the tree move are atomic: if the move fails, deletions roll back. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + other_user = factories.UserFactory() + document = factories.DocumentFactory( + users=[(user, "owner"), (other_user, "editor")] + ) + factories.InvitationFactory(document=document) + target = factories.DocumentFactory(users=[(user, "owner")]) + + assert document.is_root() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 + + # Force Document.move to fail *after* the deletion block has already run, + # so we actually exercise the rollback path rather than bailing out earlier. + def failing_move(self, *args, **kwargs): + raise RuntimeError("Simulated move failure for atomicity test") + + monkeypatch.setattr(models.Document, "move", failing_move) + + with pytest.raises(RuntimeError, match="Simulated move failure"): + client.post( + f"/api/v1.0/documents/{document.id!s}/move/", + data={"target_document_id": str(target.id), "position": "first-sibling"}, + ) + + # Accesses and invitations must still exist due to transaction rollback + document.refresh_from_db() + assert document.accesses.count() == 2 + assert document.invitations.count() == 1 diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsx index dce4e5ed42..c0e732259b 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useMoveDoc.tsx @@ -2,12 +2,6 @@ import { TreeViewMoveModeEnum } from '@gouvfr-lasuite/ui-kit'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import { APIError, errorCauses, fetchAPI } from '@/api'; -import { - getDocAccesses, - getDocInvitations, - useDeleteDocAccess, - useDeleteDocInvitation, -} from '@/docs/doc-share'; import { KEY_LIST_DOC } from './useDocs'; @@ -37,46 +31,15 @@ export const moveDoc = async ({ return response.json() as Promise; }; -export function useMoveDoc(deleteAccessOnMove = false) { +export function useMoveDoc() { const queryClient = useQueryClient(); - const { mutate: handleDeleteInvitation } = useDeleteDocInvitation(); - const { mutate: handleDeleteAccess } = useDeleteDocAccess(); return useMutation({ mutationFn: moveDoc, - async onSuccess(_data, variables, _onMutateResult, _context) { - if (!deleteAccessOnMove) { - return; - } - + onSuccess() { void queryClient.invalidateQueries({ queryKey: [KEY_LIST_DOC], }); - const accesses = await getDocAccesses({ - docId: variables.sourceDocumentId, - }); - - const invitationsResponse = await getDocInvitations({ - docId: variables.sourceDocumentId, - page: 1, - }); - - const invitations = invitationsResponse.results; - - await Promise.all([ - ...invitations.map((invitation) => - handleDeleteInvitation({ - docId: variables.sourceDocumentId, - invitationId: invitation.id, - }), - ), - ...accesses.map((access) => - handleDeleteAccess({ - docId: variables.sourceDocumentId, - accessId: access.id, - }), - ), - ]); }, }); } diff --git a/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx b/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx index 3d04afa8c0..03fea5ac06 100644 --- a/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx +++ b/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx @@ -61,7 +61,7 @@ type DocGridContentListProps = { export const DraggableDocGridContentList = ({ docs, }: DocGridContentListProps) => { - const { mutateAsync: handleMove, isError } = useMoveDoc(true); + const { mutateAsync: handleMove, isError } = useMoveDoc(); const modalConfirmation = useModal(); const onDragData = useRef(null); const { untitledDocument } = useTrans(); diff --git a/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx b/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx index ee43f5c1bb..e8cbf593f4 100644 --- a/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx +++ b/src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx @@ -87,7 +87,7 @@ export const DocMoveModal = ({ const docTargetTitle = docSelected?.title || untitledDocument; const modalConfirmation = useModal(); const modalRequest = useModal(); - const { mutate: moveDoc } = useMoveDoc(true); + const { mutate: moveDoc } = useMoveDoc(); const [search, setSearch] = useState(''); const { isDesktop, isTablet, isMobile } = useResponsiveStore(); const isModal = (isDesktop || isTablet) && !isMobile;