From dc75d5493b231b4600c1c8a98b25b23a9df779eb Mon Sep 17 00:00:00 2001 From: Mohamed El Amine BOUKERFA Date: Thu, 11 Jun 2026 23:47:00 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82(backend)=20make=20document=20acces?= =?UTF-8?q?s=20list=20visible=20to=20all=20collaborators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Return all accesses on the document and its ancestors to any user with access, keeping the limited user details serializer and adding the user id to it, so that collaborators allowed to comment can identify and mention each other. Signed-off-by: Mohamed El Amine BOUKERFA --- CHANGELOG.md | 1 + src/backend/core/api/serializers.py | 4 ++-- src/backend/core/api/viewsets.py | 6 ++--- .../documents/test_api_document_accesses.py | 24 ++++++++++--------- .../test_api_document_accesses_me.py | 6 ++++- .../documents/test_api_documents_comments.py | 7 ++++++ .../documents/test_api_documents_threads.py | 4 ++++ .../serializers/test_user_light_serializer.py | 2 ++ 8 files changed, 37 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb5f0a818..c6e9bde2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -263,6 +263,7 @@ and this project adheres to ### Changed +- 🛂(backend) make document access list visible to all collaborators - 👷(CI) remove test-e2e-other-browser job #2404 - ♿️(frontend) use heading element for pinned documents section title #2380 - ♿️(frontend) use anchor links for table of contents entries #2390 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index ff48656db..75290127b 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -73,8 +73,8 @@ class UserLightSerializer(UserSerializer): class Meta: model = models.User - fields = ["full_name", "short_name"] - read_only_fields = ["full_name", "short_name"] + fields = ["id", "full_name", "short_name"] + read_only_fields = ["id", "full_name", "short_name"] class ListDocumentSerializer(serializers.ModelSerializer): diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index b374c75a6..3118ee701 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -2599,11 +2599,11 @@ class DocumentAccessViewSet( | models.Document.objects.filter(pk=self.document.pk) ).filter(ancestors_deleted_at__isnull=True) + # All users with access see the full list of accesses (with limited + # user details for unprivileged roles) so that any collaborator + # allowed to comment can mention the others. queryset = self.get_queryset().filter(document__in=ancestors) - if role not in choices.PRIVILEGED_ROLES: - queryset = queryset.filter(role__in=choices.PRIVILEGED_ROLES) - accesses = list(queryset.order_by("document__path")) # Annotate more information on roles diff --git a/src/backend/core/tests/documents/test_api_document_accesses.py b/src/backend/core/tests/documents/test_api_document_accesses.py index 2f4393881..43a7f044c 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -97,8 +97,9 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( via, role, mock_user_teams, django_assert_num_queries ): """ - Authenticated users with no privileged role should only be able to list document - accesses associated with privileged roles for a document, including from ancestors. + Authenticated users with no privileged role should be able to list all document + accesses, including from ancestors, but with limited user information, so that + any collaborator allowed to comment can mention the others. """ user = factories.UserFactory() client = APIClient() @@ -125,18 +126,20 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( factories.UserDocumentAccessFactory(document=child) if via == USER: - models.DocumentAccess.objects.create( + user_access = models.DocumentAccess.objects.create( document=document, user=user, role=role, ) elif via == TEAM: mock_user_teams.return_value = ["lasuite", "unknown"] - models.DocumentAccess.objects.create( + user_access = models.DocumentAccess.objects.create( document=document, team="lasuite", role=role, ) + else: + raise RuntimeError() # Accesses for other documents to which the user is related should not be listed either other_access = factories.UserDocumentAccessFactory(user=user) @@ -148,11 +151,9 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( assert response.status_code == 200 content = response.json() - # Make sure only privileged roles are returned - privileged_accesses = [ - acc for acc in accesses if acc.role in choices.PRIVILEGED_ROLES - ] - assert len(content) == len(privileged_accesses) + # All accesses on the document and its ancestors are returned + all_accesses = [*accesses, user_access] + assert len(content) == len(all_accesses) assert sorted(content, key=lambda x: x["id"]) == sorted( [ @@ -164,6 +165,7 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( "depth": access.document.depth, }, "user": { + "id": str(access.user.id), "full_name": access.user.full_name, "short_name": access.user.short_name, } @@ -176,14 +178,14 @@ def test_api_document_accesses_list_authenticated_related_non_privileged( "abilities": { "destroy": False, "partial_update": False, - "retrieve": False, + "retrieve": access.user is not None and access.user.id == user.id, "set_role_to": [], "update": False, }, "updated_at": access.updated_at.isoformat().replace("+00:00", "Z"), "created_at": access.created_at.isoformat().replace("+00:00", "Z"), } - for access in privileged_accesses + for access in all_accesses ], key=lambda x: x["id"], ) diff --git a/src/backend/core/tests/documents/test_api_document_accesses_me.py b/src/backend/core/tests/documents/test_api_document_accesses_me.py index 71ac47ce2..3794a86c6 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses_me.py +++ b/src/backend/core/tests/documents/test_api_document_accesses_me.py @@ -48,7 +48,11 @@ def expected_access(access, user, via): }, "user": None if via == TEAM - else {"full_name": user.full_name, "short_name": user.short_name}, + else { + "full_name": user.full_name, + "short_name": user.short_name, + "id": str(user.id), + }, "team": "lasuite" if via == TEAM else "", "role": access.role, "max_ancestors_role": None, diff --git a/src/backend/core/tests/documents/test_api_documents_comments.py b/src/backend/core/tests/documents/test_api_documents_comments.py index e9a3b2eeb..722120243 100644 --- a/src/backend/core/tests/documents/test_api_documents_comments.py +++ b/src/backend/core/tests/documents/test_api_documents_comments.py @@ -1,5 +1,6 @@ """Test API for comments on documents.""" +# pylint: disable=too-many-lines import random from unittest import mock @@ -41,6 +42,7 @@ def test_list_comments_anonymous_user_public_document(): "created_at": comment1.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment1.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(comment1.user.id), "full_name": comment1.user.full_name, "short_name": comment1.user.short_name, }, @@ -53,6 +55,7 @@ def test_list_comments_anonymous_user_public_document(): "created_at": comment2.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment2.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(comment2.user.id), "full_name": comment2.user.full_name, "short_name": comment2.user.short_name, }, @@ -110,6 +113,7 @@ def test_list_comments_authenticated_user_accessible_document(): "created_at": comment1.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment1.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(comment1.user.id), "full_name": comment1.user.full_name, "short_name": comment1.user.short_name, }, @@ -122,6 +126,7 @@ def test_list_comments_authenticated_user_accessible_document(): "created_at": comment2.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment2.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(comment2.user.id), "full_name": comment2.user.full_name, "short_name": comment2.user.short_name, }, @@ -263,6 +268,7 @@ def test_create_comment_authenticated_user_accessible_document(): "created_at": response.json()["created_at"], "updated_at": response.json()["updated_at"], "user": { + "id": str(user.id), "full_name": user.full_name, "short_name": user.short_name, }, @@ -317,6 +323,7 @@ def test_retrieve_comment_anonymous_user_public_document(): "created_at": comment.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(comment.user.id), "full_name": comment.user.full_name, "short_name": comment.user.short_name, }, diff --git a/src/backend/core/tests/documents/test_api_documents_threads.py b/src/backend/core/tests/documents/test_api_documents_threads.py index 34bd34d12..b10ec2b2b 100644 --- a/src/backend/core/tests/documents/test_api_documents_threads.py +++ b/src/backend/core/tests/documents/test_api_documents_threads.py @@ -175,6 +175,7 @@ def test_api_documents_threads_restricted_document_editor(role): "created_at": thread.created_at.isoformat().replace("+00:00", "Z"), "updated_at": thread.updated_at.isoformat().replace("+00:00", "Z"), "creator": { + "id": str(user.id), "full_name": user.full_name, "short_name": user.short_name, }, @@ -185,6 +186,7 @@ def test_api_documents_threads_restricted_document_editor(role): "created_at": comment.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(user.id), "full_name": user.full_name, "short_name": user.short_name, }, @@ -296,6 +298,7 @@ def test_api_documents_threads_authenticated_document(link_role): "created_at": thread.created_at.isoformat().replace("+00:00", "Z"), "updated_at": thread.updated_at.isoformat().replace("+00:00", "Z"), "creator": { + "id": str(user.id), "full_name": user.full_name, "short_name": user.short_name, }, @@ -306,6 +309,7 @@ def test_api_documents_threads_authenticated_document(link_role): "created_at": comment.created_at.isoformat().replace("+00:00", "Z"), "updated_at": comment.updated_at.isoformat().replace("+00:00", "Z"), "user": { + "id": str(user.id), "full_name": user.full_name, "short_name": user.short_name, }, diff --git a/src/backend/core/tests/serializers/test_user_light_serializer.py b/src/backend/core/tests/serializers/test_user_light_serializer.py index 05bd33b4f..f2f75478d 100644 --- a/src/backend/core/tests/serializers/test_user_light_serializer.py +++ b/src/backend/core/tests/serializers/test_user_light_serializer.py @@ -16,8 +16,10 @@ def test_user_light_serializer(): short_name="John", ) serializer = UserLightSerializer(user) + assert serializer.data["id"] == str(user.id) assert serializer.data["full_name"] == "John Doe" assert serializer.data["short_name"] == "John" + assert "email" not in serializer.data def test_user_light_serializer_no_full_name():