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():