From 611ba496d2538751faad14c088ea30b64b2a13b3 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Fri, 25 Apr 2025 08:03:12 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20simplify=20roles?= =?UTF-8?q?=20by=20returning=20only=20the=20max=20role?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were returning the list of roles a user has on a document (direct and inherited). Now that we introduced priority on roles, we are able to determine what is the max role and return only this one. This commit also changes the role that is returned for the restricted reach: we now return None because the role is not relevant in this case. --- CHANGELOG.md | 5 +- src/backend/core/api/serializers.py | 44 ++--- src/backend/core/api/viewsets.py | 61 +++--- src/backend/core/choices.py | 40 +++- src/backend/core/models.py | 178 +++++++++--------- .../test_api_documents_children_create.py | 4 +- .../test_api_documents_children_list.py | 125 +++++++----- .../test_api_documents_descendants.py | 42 ++--- .../test_api_documents_favorite_list.py | 2 +- .../documents/test_api_documents_list.py | 10 +- .../documents/test_api_documents_retrieve.py | 85 ++++----- .../documents/test_api_documents_trashbin.py | 3 +- .../documents/test_api_documents_tree.py | 73 +++---- .../core/tests/test_models_documents.py | 85 +++++---- 14 files changed, 406 insertions(+), 351 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 085a1d5b..48847c3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to ### Changed +- ♻️(backend) simplify roles by ranking them and return only the max role #846 - 📌(yjs) stop pinning node to minor version on yjs docker image #1005 - 🧑‍💻(docker) add .next to .dockerignore #1055 - 🧑‍💻(docker) handle frontend development images with docker compose #1033 @@ -114,10 +115,6 @@ and this project adheres to - 🐛(backend) race condition create doc #633 - 🐛(frontend) fix breaklines in custom blocks #908 -## Fixed - -- 🐛(backend) fix link definition select options linked to ancestors #846 - ## [3.1.0] - 2025-04-07 ## Added diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 2a21b670..cbbfb3b4 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -171,7 +171,7 @@ class ListDocumentSerializer(serializers.ModelSerializer): is_favorite = serializers.BooleanField(read_only=True) nb_accesses_ancestors = serializers.IntegerField(read_only=True) nb_accesses_direct = serializers.IntegerField(read_only=True) - user_roles = serializers.SerializerMethodField(read_only=True) + user_role = serializers.SerializerMethodField(read_only=True) abilities = serializers.SerializerMethodField(read_only=True) class Meta: @@ -192,7 +192,7 @@ class ListDocumentSerializer(serializers.ModelSerializer): "path", "title", "updated_at", - "user_roles", + "user_role", ] read_only_fields = [ "id", @@ -209,34 +209,36 @@ class ListDocumentSerializer(serializers.ModelSerializer): "numchild", "path", "updated_at", - "user_roles", + "user_role", ] - def get_abilities(self, document) -> dict: + def to_representation(self, instance): + """Precompute once per instance""" + paths_links_mapping = self.context.get("paths_links_mapping") + + if paths_links_mapping is not None: + links = paths_links_mapping.get(instance.path[: -instance.steplen], []) + instance.ancestors_link_definition = choices.get_equivalent_link_definition( + links + ) + + return super().to_representation(instance) + + def get_abilities(self, instance) -> dict: """Return abilities of the logged-in user on the instance.""" request = self.context.get("request") + if not request: + return {} - if request: - paths_links_mapping = self.context.get("paths_links_mapping", None) - # Retrieve ancestor links from paths_links_mapping (if provided) - ancestors_links = ( - paths_links_mapping.get(document.path[: -document.steplen]) - if paths_links_mapping - else None - ) - return document.get_abilities(request.user, ancestors_links=ancestors_links) + return instance.get_abilities(request.user) - return {} - - def get_user_roles(self, document): + def get_user_role(self, instance): """ Return roles of the logged-in user for the current document, taking into account ancestors. """ request = self.context.get("request") - if request: - return document.get_roles(request.user) - return [] + return instance.get_role(request.user) if request else None class DocumentSerializer(ListDocumentSerializer): @@ -264,7 +266,7 @@ class DocumentSerializer(ListDocumentSerializer): "path", "title", "updated_at", - "user_roles", + "user_role", "websocket", ] read_only_fields = [ @@ -281,7 +283,7 @@ class DocumentSerializer(ListDocumentSerializer): "numchild", "path", "updated_at", - "user_roles", + "user_role", ] def get_fields(self): diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index efc06fe7..70288449 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -443,14 +443,15 @@ class DocumentViewSet( queryset = queryset.annotate_user_roles(user) return queryset - def get_response_for_queryset(self, queryset): + def get_response_for_queryset(self, queryset, context=None): """Return paginated response for the queryset if requested.""" + context = context or self.get_serializer_context() page = self.paginate_queryset(queryset) if page is not None: - serializer = self.get_serializer(page, many=True) + serializer = self.get_serializer(page, many=True, context=context) return self.get_paginated_response(serializer.data) - serializer = self.get_serializer(queryset, many=True) + serializer = self.get_serializer(queryset, many=True, context=context) return drf.response.Response(serializer.data) def list(self, request, *args, **kwargs): @@ -460,9 +461,6 @@ class DocumentViewSet( This method applies filtering based on request parameters using `ListDocumentFilter`. It performs early filtering on model fields, annotates user roles, and removes descendant documents to keep only the highest ancestors readable by the current user. - - Additional annotations (e.g., `is_highest_ancestor_for_user`, favorite status) are - applied before ordering and returning the response. """ user = self.request.user @@ -490,12 +488,6 @@ class DocumentViewSet( ) queryset = queryset.filter(path__in=root_paths) - # Annotate the queryset with an attribute marking instances as highest ancestor - # in order to save some time while computing abilities on the instance - queryset = queryset.annotate( - is_highest_ancestor_for_user=db.Value(True, output_field=db.BooleanField()) - ) - # Annotate favorite status and filter if applicable as late as possible queryset = queryset.annotate_is_favorite(user) queryset = filterset.filters["is_favorite"].filter( @@ -827,7 +819,17 @@ class DocumentViewSet( queryset = filterset.qs - return self.get_response_for_queryset(queryset) + # Pass ancestors' links paths mapping to the serializer as a context variable + # in order to allow saving time while computing abilities on the instance + paths_links_mapping = document.compute_ancestors_links_paths_mapping() + + return self.get_response_for_queryset( + queryset, + context={ + "request": request, + "paths_links_mapping": paths_links_mapping, + }, + ) @drf.decorators.action( detail=True, @@ -886,13 +888,6 @@ class DocumentViewSet( ancestors_links = [] children_clause = db.Q() for ancestor in ancestors: - if ancestor.depth < highest_readable.depth: - continue - - children_clause |= db.Q( - path__startswith=ancestor.path, depth=ancestor.depth + 1 - ) - # Compute cache for ancestors links to avoid many queries while computing # abilities for his documents in the tree! ancestors_links.append( @@ -900,25 +895,21 @@ class DocumentViewSet( ) paths_links_mapping[ancestor.path] = ancestors_links.copy() + if ancestor.depth < highest_readable.depth: + continue + + children_clause |= db.Q( + path__startswith=ancestor.path, depth=ancestor.depth + 1 + ) + children = self.queryset.filter(children_clause, deleted_at__isnull=True) queryset = ancestors.filter(depth__gte=highest_readable.depth) | children queryset = queryset.order_by("path") - # Annotate if the current document is the highest ancestor for the user - queryset = queryset.annotate( - is_highest_ancestor_for_user=db.Case( - db.When( - path=db.Value(highest_readable.path), - then=db.Value(True), - ), - default=db.Value(False), - output_field=db.BooleanField(), - ) - ) queryset = queryset.annotate_user_roles(user) queryset = queryset.annotate_is_favorite(user) - # Pass ancestors' links definitions to the serializer as a context variable + # Pass ancestors' links paths mapping to the serializer as a context variable # in order to allow saving time while computing abilities on the instance serializer = self.get_serializer( queryset, @@ -1520,8 +1511,8 @@ class DocumentAccessViewSet( except models.Document.DoesNotExist: return drf.response.Response([]) - roles = set(document.get_roles(user)) - if not roles: + role = document.get_role(user) + if role is None: return drf.response.Response([]) ancestors = ( @@ -1539,7 +1530,7 @@ class DocumentAccessViewSet( document__in=ancestors.filter(depth__gte=highest_readable.depth) ) - is_privileged = bool(roles.intersection(set(choices.PRIVILEGED_ROLES))) + is_privileged = role in choices.PRIVILEGED_ROLES if is_privileged: serializer_class = serializers.DocumentAccessSerializer else: diff --git a/src/backend/core/choices.py b/src/backend/core/choices.py index ff618f21..f1a0e298 100644 --- a/src/backend/core/choices.py +++ b/src/backend/core/choices.py @@ -11,10 +11,11 @@ class PriorityTextChoices(TextChoices): """ @classmethod - def get_priority(cls, value): - """Returns the priority of the given value based on its order in the class.""" + def get_priority(cls, role): + """Returns the priority of the given role based on its order in the class.""" + members = list(cls.__members__.values()) - return members.index(value) + 1 if value in members else 0 + return members.index(role) + 1 if role in members else 0 @classmethod def max(cls, *roles): @@ -22,7 +23,6 @@ class PriorityTextChoices(TextChoices): Return the highest-priority role among the given roles, using get_priority(). If no valid roles are provided, returns None. """ - valid_roles = [role for role in roles if cls.get_priority(role) is not None] if not valid_roles: return None @@ -61,7 +61,6 @@ class LinkReachChoices(PriorityTextChoices): ) # Any authenticated user can access the document PUBLIC = "public", _("Public") # Even anonymous users can access the document - @classmethod def get_select_options(cls, link_reach, link_role): """ @@ -110,3 +109,34 @@ class LinkReachChoices(PriorityTextChoices): reach: sorted(roles, key=LinkRoleChoices.get_priority) if roles else roles for reach, roles in result.items() } + + +def get_equivalent_link_definition(ancestors_links): + """ + Return the (reach, role) pair with: + 1. Highest reach + 2. Highest role among links having that reach + """ + if not ancestors_links: + return {"link_reach": None, "link_role": None} + + # 1) Find the highest reach + max_reach = max( + ancestors_links, + key=lambda link: LinkReachChoices.get_priority(link["link_reach"]), + )["link_reach"] + + # 2) Among those, find the highest role (ignore role if RESTRICTED) + if max_reach == LinkReachChoices.RESTRICTED: + max_role = None + else: + max_role = max( + ( + link["link_role"] + for link in ancestors_links + if link["link_reach"] == max_reach + ), + key=LinkRoleChoices.get_priority, + ) + + return {"link_reach": max_reach, "link_role": max_role} diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 2913fa88..64979b46 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -6,7 +6,6 @@ Declare and configure the models for the impress core application import hashlib import smtplib import uuid -from collections import defaultdict from datetime import timedelta from logging import getLogger @@ -33,7 +32,13 @@ from rest_framework.exceptions import ValidationError from timezone_field import TimeZoneField from treebeard.mp_tree import MP_Node, MP_NodeManager, MP_NodeQuerySet -from .choices import PRIVILEGED_ROLES, LinkReachChoices, LinkRoleChoices, RoleChoices +from .choices import ( + PRIVILEGED_ROLES, + LinkReachChoices, + LinkRoleChoices, + RoleChoices, + get_equivalent_link_definition, +) logger = getLogger(__name__) @@ -284,9 +289,9 @@ class BaseAccess(BaseModel): class Meta: abstract = True - def _get_roles(self, resource, user): + def _get_role(self, resource, user): """ - Get the roles a user has on a resource. + Get the role a user has on a resource. """ roles = [] if user.is_authenticated: @@ -301,23 +306,20 @@ class BaseAccess(BaseModel): except (self._meta.model.DoesNotExist, IndexError): roles = [] - return roles + return RoleChoices.max(*roles) def _get_abilities(self, resource, user): """ Compute and return abilities for a given user taking into account the current state of the object. """ - roles = self._get_roles(resource, user) + role = self._get_role(resource, user) + is_owner_or_admin = role in (RoleChoices.OWNER, RoleChoices.ADMIN) - is_owner_or_admin = bool( - set(roles).intersection({RoleChoices.OWNER, RoleChoices.ADMIN}) - ) if self.role == RoleChoices.OWNER: - can_delete = ( - RoleChoices.OWNER in roles - and resource.accesses.filter(role=RoleChoices.OWNER).count() > 1 - ) + can_delete = (role == RoleChoices.OWNER) and resource.accesses.filter( + role=RoleChoices.OWNER + ).count() > 1 set_role_to = ( [RoleChoices.ADMIN, RoleChoices.EDITOR, RoleChoices.READER] if can_delete @@ -326,7 +328,7 @@ class BaseAccess(BaseModel): else: can_delete = is_owner_or_admin set_role_to = [] - if RoleChoices.OWNER in roles: + if role == RoleChoices.OWNER: set_role_to.append(RoleChoices.OWNER) if is_owner_or_admin: set_role_to.extend( @@ -343,7 +345,7 @@ class BaseAccess(BaseModel): "destroy": can_delete, "update": bool(set_role_to), "partial_update": bool(set_role_to), - "retrieve": bool(roles), + "retrieve": bool(role), "set_role_to": set_role_to, } @@ -419,6 +421,7 @@ class DocumentManager(MP_NodeManager.from_queryset(DocumentQuerySet)): return self._queryset_class(self.model).order_by("path") +# pylint: disable=too-many-public-methods class Document(MP_Node, BaseModel): """Pad document carrying the content.""" @@ -486,6 +489,11 @@ class Document(MP_Node, BaseModel): def __str__(self): return str(self.title) if self.title else str(_("Untitled Document")) + def __init__(self, *args, **kwargs): + """Initialize cache property.""" + super().__init__(*args, **kwargs) + self._ancestors_link_definition = None + def save(self, *args, **kwargs): """Write content to object storage only if _content has changed.""" super().save(*args, **kwargs) @@ -673,37 +681,22 @@ class Document(MP_Node, BaseModel): cache_key = document.get_nb_accesses_cache_key() cache.delete(cache_key) - def get_roles(self, user): + def get_role(self, user): """Return the roles a user has on a document.""" if not user.is_authenticated: - return [] + return None try: roles = self.user_roles or [] except AttributeError: - try: - roles = DocumentAccess.objects.filter( - models.Q(user=user) | models.Q(team__in=user.teams), - document__path=Left( - models.Value(self.path), Length("document__path") - ), - ).values_list("role", flat=True) - except (models.ObjectDoesNotExist, IndexError): - roles = [] - return roles + roles = DocumentAccess.objects.filter( + models.Q(user=user) | models.Q(team__in=user.teams), + document__path=Left(models.Value(self.path), Length("document__path")), + ).values_list("role", flat=True) - def get_ancestors_links_definitions(self, ancestors_links): - """Get links reach/role definitions for ancestors of the current document.""" + return RoleChoices.max(*roles) - ancestors_links_definitions = defaultdict(set) - for ancestor in ancestors_links: - ancestors_links_definitions[ancestor["link_reach"]].add( - ancestor["link_role"] - ) - - return ancestors_links_definitions - - def compute_ancestors_links(self, user): + def compute_ancestors_links_paths_mapping(self): """ Compute the ancestors links for the current document up to the highest readable ancestor. """ @@ -712,73 +705,87 @@ class Document(MP_Node, BaseModel): .filter(ancestors_deleted_at__isnull=True) .order_by("path") ) - highest_readable = ancestors.readable_per_se(user).only("depth").first() - - if highest_readable is None: - return [] - ancestors_links = [] paths_links_mapping = {} - for ancestor in ancestors.filter(depth__gte=highest_readable.depth): + + for ancestor in ancestors: ancestors_links.append( {"link_reach": ancestor.link_reach, "link_role": ancestor.link_role} ) paths_links_mapping[ancestor.path] = ancestors_links.copy() - ancestors_links = paths_links_mapping.get(self.path[: -self.steplen], []) + return paths_links_mapping - return ancestors_links + @property + def ancestors_link_definition(self): + """Link defintion equivalent to all document's ancestors.""" + if getattr(self, "_ancestors_link_definition", None) is None: + if self.depth <= 1: + ancestors_links = [] + else: + mapping = self.compute_ancestors_links_paths_mapping() + ancestors_links = mapping.get(self.path[: -self.steplen], []) + self._ancestors_link_definition = get_equivalent_link_definition( + ancestors_links + ) - def get_abilities(self, user, ancestors_links=None): + return self._ancestors_link_definition + + @ancestors_link_definition.setter + def ancestors_link_definition(self, definition): + """Cache the ancestors_link_definition.""" + self._ancestors_link_definition = definition + + @property + def ancestors_link_reach(self): + """Link reach equivalent to all document's ancestors.""" + return self.ancestors_link_definition["link_reach"] + + @property + def ancestors_link_role(self): + """Link role equivalent to all document's ancestors.""" + return self.ancestors_link_definition["link_role"] + + def get_abilities(self, user): """ Compute and return abilities for a given user on the document. """ - if self.depth <= 1 or getattr(self, "is_highest_ancestor_for_user", False): - ancestors_links = [] - elif ancestors_links is None: - ancestors_links = self.compute_ancestors_links(user=user) - - roles = set( - self.get_roles(user) - ) # at this point only roles based on specific access + # First get the role based on specific access + role = self.get_role(user) # Characteristics that are based only on specific access - is_owner = RoleChoices.OWNER in roles + is_owner = role == RoleChoices.OWNER is_deleted = self.ancestors_deleted_at and not is_owner - is_owner_or_admin = (is_owner or RoleChoices.ADMIN in roles) and not is_deleted + is_owner_or_admin = (is_owner or role == RoleChoices.ADMIN) and not is_deleted # Compute access roles before adding link roles because we don't # want anonymous users to access versions (we wouldn't know from # which date to allow them anyway) # Anonymous users should also not see document accesses - has_access_role = bool(roles) and not is_deleted + has_access_role = bool(role) and not is_deleted can_update_from_access = ( - is_owner_or_admin or RoleChoices.EDITOR in roles + is_owner_or_admin or role == RoleChoices.EDITOR ) and not is_deleted - # Add roles provided by the document link, taking into account its ancestors - ancestors_links_definitions = self.get_ancestors_links_definitions( - ancestors_links + link_select_options = LinkReachChoices.get_select_options( + **self.ancestors_link_definition + ) + link_definition = get_equivalent_link_definition( + [ + self.ancestors_link_definition, + {"link_reach": self.link_reach, "link_role": self.link_role}, + ] ) - public_roles = ancestors_links_definitions.get( - LinkReachChoices.PUBLIC, set() - ) | ({self.link_role} if self.link_reach == LinkReachChoices.PUBLIC else set()) - authenticated_roles = ( - ancestors_links_definitions.get(LinkReachChoices.AUTHENTICATED, set()) - | ( - {self.link_role} - if self.link_reach == LinkReachChoices.AUTHENTICATED - else set() - ) - if user.is_authenticated - else set() - ) - roles = roles | public_roles | authenticated_roles + link_reach = link_definition["link_reach"] + if link_reach == LinkReachChoices.PUBLIC or ( + link_reach == LinkReachChoices.AUTHENTICATED and user.is_authenticated + ): + role = RoleChoices.max(role, link_definition["link_role"]) - can_get = bool(roles) and not is_deleted + can_get = bool(role) and not is_deleted can_update = ( - is_owner_or_admin or RoleChoices.EDITOR in roles + is_owner_or_admin or role == RoleChoices.EDITOR ) and not is_deleted ai_allow_reach_from = settings.AI_ALLOW_REACH_FROM @@ -816,12 +823,7 @@ class Document(MP_Node, BaseModel): "restore": is_owner, "retrieve": can_get, "media_auth": can_get, - "ancestors_links_definitions": { - k: list(v) for k, v in ancestors_links_definitions.items() - }, - "link_select_options": LinkReachChoices.get_select_options( - ancestors_links_definitions - ), + "link_select_options": link_select_options, "tree": can_get, "update": can_update, "versions_destroy": is_owner_or_admin, @@ -1082,11 +1084,11 @@ class DocumentAccess(BaseAccess): """ Compute and return abilities for a given user on the document access. """ - roles = self._get_roles(self.document, user) - is_owner_or_admin = bool(set(roles).intersection(set(PRIVILEGED_ROLES))) + role = self._get_role(self.document, user) + is_owner_or_admin = role in PRIVILEGED_ROLES if self.role == RoleChoices.OWNER: can_delete = ( - RoleChoices.OWNER in roles + role == RoleChoices.OWNER and self.document.accesses.filter(role=RoleChoices.OWNER).count() > 1 ) set_role_to = ( @@ -1097,7 +1099,7 @@ class DocumentAccess(BaseAccess): else: can_delete = is_owner_or_admin set_role_to = [] - if RoleChoices.OWNER in roles: + if role == RoleChoices.OWNER: set_role_to.append(RoleChoices.OWNER) if is_owner_or_admin: set_role_to.extend( diff --git a/src/backend/core/tests/documents/test_api_documents_children_create.py b/src/backend/core/tests/documents/test_api_documents_children_create.py index 5aea1b60..c5b6f2bf 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_create.py +++ b/src/backend/core/tests/documents/test_api_documents_children_create.py @@ -98,7 +98,9 @@ def test_api_documents_children_create_authenticated_success(reach, role, depth) if i == 0: document = factories.DocumentFactory(link_reach=reach, link_role=role) else: - document = factories.DocumentFactory(parent=document, link_role="reader") + document = factories.DocumentFactory( + parent=document, link_reach="restricted" + ) response = client.post( f"/api/v1.0/documents/{document.id!s}/children/", diff --git a/src/backend/core/tests/documents/test_api_documents_children_list.py b/src/backend/core/tests/documents/test_api_documents_children_list.py index 96e1d9b4..0267345b 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_list.py +++ b/src/backend/core/tests/documents/test_api_documents_children_list.py @@ -14,13 +14,18 @@ from core import factories pytestmark = pytest.mark.django_db -def test_api_documents_children_list_anonymous_public_standalone(): +def test_api_documents_children_list_anonymous_public_standalone( + django_assert_num_queries, +): """Anonymous users should be allowed to retrieve the children of a public document.""" document = factories.DocumentFactory(link_reach="public") child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(8): + APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(4): + response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") assert response.status_code == 200 assert response.json() == { @@ -44,7 +49,7 @@ def test_api_documents_children_list_anonymous_public_standalone(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -62,13 +67,13 @@ def test_api_documents_children_list_anonymous_public_standalone(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } -def test_api_documents_children_list_anonymous_public_parent(): +def test_api_documents_children_list_anonymous_public_parent(django_assert_num_queries): """ Anonymous users should be allowed to retrieve the children of a document who has a public ancestor. @@ -83,7 +88,10 @@ def test_api_documents_children_list_anonymous_public_parent(): child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(9): + APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(5): + response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") assert response.status_code == 200 assert response.json() == { @@ -107,7 +115,7 @@ def test_api_documents_children_list_anonymous_public_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -125,7 +133,7 @@ def test_api_documents_children_list_anonymous_public_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -149,7 +157,7 @@ def test_api_documents_children_list_anonymous_restricted_or_authenticated(reach @pytest.mark.parametrize("reach", ["public", "authenticated"]) def test_api_documents_children_list_authenticated_unrelated_public_or_authenticated( - reach, + reach, django_assert_num_queries ): """ Authenticated users should be able to retrieve the children of a public/authenticated @@ -163,9 +171,13 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = client.get( - f"/api/v1.0/documents/{document.id!s}/children/", - ) + with django_assert_num_queries(9): + client.get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(5): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/children/", + ) + assert response.status_code == 200 assert response.json() == { "count": 2, @@ -188,7 +200,7 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(user), @@ -206,7 +218,7 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -214,7 +226,7 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic @pytest.mark.parametrize("reach", ["public", "authenticated"]) def test_api_documents_children_list_authenticated_public_or_authenticated_parent( - reach, + reach, django_assert_num_queries ): """ Authenticated users should be allowed to retrieve the children of a document who @@ -231,7 +243,11 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(10): + client.get(f"/api/v1.0/documents/{document.id!s}/children/") + + with django_assert_num_queries(6): + response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") assert response.status_code == 200 assert response.json() == { @@ -255,7 +271,7 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(user), @@ -273,13 +289,15 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } -def test_api_documents_children_list_authenticated_unrelated_restricted(): +def test_api_documents_children_list_authenticated_unrelated_restricted( + django_assert_num_queries, +): """ Authenticated users should not be allowed to retrieve the children of a document that is restricted and to which they are not related. @@ -293,16 +311,20 @@ def test_api_documents_children_list_authenticated_unrelated_restricted(): child1, _child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = client.get( - f"/api/v1.0/documents/{document.id!s}/children/", - ) + with django_assert_num_queries(2): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/children/", + ) + assert response.status_code == 403 assert response.json() == { "detail": "You do not have permission to perform this action." } -def test_api_documents_children_list_authenticated_related_direct(): +def test_api_documents_children_list_authenticated_related_direct( + django_assert_num_queries, +): """ Authenticated users should be allowed to retrieve the children of a document to which they are directly related whatever the role. @@ -319,9 +341,11 @@ def test_api_documents_children_list_authenticated_related_direct(): child1, child2 = factories.DocumentFactory.create_batch(2, parent=document) factories.UserDocumentAccessFactory(document=child1) - response = client.get( - f"/api/v1.0/documents/{document.id!s}/children/", - ) + with django_assert_num_queries(9): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/children/", + ) + assert response.status_code == 200 assert response.json() == { "count": 2, @@ -344,7 +368,7 @@ def test_api_documents_children_list_authenticated_related_direct(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": child2.get_abilities(user), @@ -362,13 +386,15 @@ def test_api_documents_children_list_authenticated_related_direct(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], } -def test_api_documents_children_list_authenticated_related_parent(): +def test_api_documents_children_list_authenticated_related_parent( + django_assert_num_queries, +): """ Authenticated users should be allowed to retrieve the children of a document if they are related to one of its ancestors whatever the role. @@ -389,9 +415,11 @@ def test_api_documents_children_list_authenticated_related_parent(): document=grand_parent, user=user ) - response = client.get( - f"/api/v1.0/documents/{document.id!s}/children/", - ) + with django_assert_num_queries(10): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/children/", + ) + assert response.status_code == 200 assert response.json() == { "count": 2, @@ -414,7 +442,7 @@ def test_api_documents_children_list_authenticated_related_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [grand_parent_access.role], + "user_role": grand_parent_access.role, }, { "abilities": child2.get_abilities(user), @@ -432,13 +460,15 @@ def test_api_documents_children_list_authenticated_related_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [grand_parent_access.role], + "user_role": grand_parent_access.role, }, ], } -def test_api_documents_children_list_authenticated_related_child(): +def test_api_documents_children_list_authenticated_related_child( + django_assert_num_queries, +): """ Authenticated users should not be allowed to retrieve all the children of a document as a result of being related to one of its children. @@ -454,16 +484,20 @@ def test_api_documents_children_list_authenticated_related_child(): factories.UserDocumentAccessFactory(document=child1, user=user) factories.UserDocumentAccessFactory(document=document) - response = client.get( - f"/api/v1.0/documents/{document.id!s}/children/", - ) + with django_assert_num_queries(2): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/children/", + ) + assert response.status_code == 403 assert response.json() == { "detail": "You do not have permission to perform this action." } -def test_api_documents_children_list_authenticated_related_team_none(mock_user_teams): +def test_api_documents_children_list_authenticated_related_team_none( + mock_user_teams, django_assert_num_queries +): """ Authenticated users should not be able to retrieve the children of a restricted document related to teams in which the user is not. @@ -480,7 +514,9 @@ def test_api_documents_children_list_authenticated_related_team_none(mock_user_t factories.TeamDocumentAccessFactory(document=document, team="myteam") - response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(2): + response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") + assert response.status_code == 403 assert response.json() == { "detail": "You do not have permission to perform this action." @@ -488,7 +524,7 @@ def test_api_documents_children_list_authenticated_related_team_none(mock_user_t def test_api_documents_children_list_authenticated_related_team_members( - mock_user_teams, + mock_user_teams, django_assert_num_queries ): """ Authenticated users should be allowed to retrieve the children of a document to which they @@ -506,7 +542,8 @@ def test_api_documents_children_list_authenticated_related_team_members( access = factories.TeamDocumentAccessFactory(document=document, team="myteam") - response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") + with django_assert_num_queries(9): + response = client.get(f"/api/v1.0/documents/{document.id!s}/children/") # pylint: disable=R0801 assert response.status_code == 200 @@ -531,7 +568,7 @@ def test_api_documents_children_list_authenticated_related_team_members( "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": child2.get_abilities(user), @@ -549,7 +586,7 @@ def test_api_documents_children_list_authenticated_related_team_members( "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], } diff --git a/src/backend/core/tests/documents/test_api_documents_descendants.py b/src/backend/core/tests/documents/test_api_documents_descendants.py index 302af231..c3e7000b 100644 --- a/src/backend/core/tests/documents/test_api_documents_descendants.py +++ b/src/backend/core/tests/documents/test_api_documents_descendants.py @@ -46,7 +46,7 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": grand_child.get_abilities(AnonymousUser()), @@ -64,7 +64,7 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -82,7 +82,7 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -129,7 +129,7 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": grand_child.get_abilities(AnonymousUser()), @@ -147,7 +147,7 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(AnonymousUser()), @@ -165,7 +165,7 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -231,7 +231,7 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": grand_child.get_abilities(user), @@ -249,7 +249,7 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(user), @@ -267,7 +267,7 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -318,7 +318,7 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": grand_child.get_abilities(user), @@ -336,7 +336,7 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": child2.get_abilities(user), @@ -354,7 +354,7 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], } @@ -428,7 +428,7 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": grand_child.get_abilities(user), @@ -446,7 +446,7 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": child2.get_abilities(user), @@ -464,7 +464,7 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], } @@ -518,7 +518,7 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [grand_parent_access.role], + "user_role": grand_parent_access.role, }, { "abilities": grand_child.get_abilities(user), @@ -536,7 +536,7 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [grand_parent_access.role], + "user_role": grand_parent_access.role, }, { "abilities": child2.get_abilities(user), @@ -554,7 +554,7 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [grand_parent_access.role], + "user_role": grand_parent_access.role, }, ], } @@ -654,7 +654,7 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": grand_child.get_abilities(user), @@ -672,7 +672,7 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": child2.get_abilities(user), @@ -690,7 +690,7 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], } diff --git a/src/backend/core/tests/documents/test_api_documents_favorite_list.py b/src/backend/core/tests/documents/test_api_documents_favorite_list.py index 8791a6bf..28ed3164 100644 --- a/src/backend/core/tests/documents/test_api_documents_favorite_list.py +++ b/src/backend/core/tests/documents/test_api_documents_favorite_list.py @@ -74,7 +74,7 @@ def test_api_document_favorite_list_authenticated_with_favorite(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": ["reader"], + "user_role": "reader", } ], } diff --git a/src/backend/core/tests/documents/test_api_documents_list.py b/src/backend/core/tests/documents/test_api_documents_list.py index 1120123e..0fc14e6d 100644 --- a/src/backend/core/tests/documents/test_api_documents_list.py +++ b/src/backend/core/tests/documents/test_api_documents_list.py @@ -76,7 +76,7 @@ def test_api_documents_list_format(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } @@ -148,11 +148,11 @@ def test_api_documents_list_authenticated_direct(django_assert_num_queries): str(child4_with_access.id), } - with django_assert_num_queries(12): + with django_assert_num_queries(14): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached - with django_assert_num_queries(4): + with django_assert_num_queries(6): response = client.get("/api/v1.0/documents/") assert response.status_code == 200 @@ -268,11 +268,11 @@ def test_api_documents_list_authenticated_link_reach_public_or_authenticated( expected_ids = {str(document1.id), str(document2.id), str(visible_child.id)} - with django_assert_num_queries(10): + with django_assert_num_queries(11): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached - with django_assert_num_queries(4): + with django_assert_num_queries(5): response = client.get("/api/v1.0/documents/") assert response.status_code == 200 diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index af5f25e0..de7ccaac 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -12,7 +12,7 @@ from django.utils import timezone import pytest from rest_framework.test import APIClient -from core import factories, models +from core import choices, factories, models pytestmark = pytest.mark.django_db @@ -44,7 +44,6 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "favorite": False, "invite_owner": False, "link_configuration": False, - "ancestors_links_definitions": {}, "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], @@ -76,7 +75,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -94,7 +93,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): assert response.status_code == 200 links = document.get_ancestors().values("link_reach", "link_role") - links_definitions = document.get_ancestors_links_definitions(links) + links_definition = choices.get_equivalent_link_definition(links) assert response.json() == { "id": str(document.id), "abilities": { @@ -102,10 +101,6 @@ def test_api_documents_retrieve_anonymous_public_parent(): "accesses_view": False, "ai_transform": False, "ai_translate": False, - "ancestors_links_definitions": { - "public": [grand_parent.link_role], - parent.link_reach: [parent.link_role], - }, "attachment_upload": grand_parent.link_role == "editor", "can_edit": grand_parent.link_role == "editor", "children_create": False, @@ -120,7 +115,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "invite_owner": False, "link_configuration": False, "link_select_options": models.LinkReachChoices.get_select_options( - links_definitions + **links_definition ), "media_auth": True, "media_check": True, @@ -148,7 +143,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -206,7 +201,6 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "accesses_view": False, "ai_transform": document.link_role == "editor", "ai_translate": document.link_role == "editor", - "ancestors_links_definitions": {}, "attachment_upload": document.link_role == "editor", "can_edit": document.link_role == "editor", "children_create": document.link_role == "editor", @@ -250,7 +244,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } assert ( models.LinkTrace.objects.filter(document=document, user=user).exists() is True @@ -276,7 +270,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea assert response.status_code == 200 links = document.get_ancestors().values("link_reach", "link_role") - links_definitions = document.get_ancestors_links_definitions(links) + links_definition = choices.get_equivalent_link_definition(links) assert response.json() == { "id": str(document.id), "abilities": { @@ -284,10 +278,6 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "accesses_view": False, "ai_transform": grand_parent.link_role == "editor", "ai_translate": grand_parent.link_role == "editor", - "ancestors_links_definitions": { - grand_parent.link_reach: [grand_parent.link_role], - "restricted": [parent.link_role], - }, "attachment_upload": grand_parent.link_role == "editor", "can_edit": grand_parent.link_role == "editor", "children_create": grand_parent.link_role == "editor", @@ -301,7 +291,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "invite_owner": False, "link_configuration": False, "link_select_options": models.LinkReachChoices.get_select_options( - links_definitions + **links_definition ), "move": False, "media_auth": True, @@ -329,7 +319,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -439,7 +429,7 @@ def test_api_documents_retrieve_authenticated_related_direct(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } @@ -465,8 +455,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): ) assert response.status_code == 200 links = document.get_ancestors().values("link_reach", "link_role") - links_definitions = document.get_ancestors_links_definitions(links) - ancestors_roles = list({grand_parent.link_role, parent.link_role}) + link_definition = choices.get_equivalent_link_definition(links) assert response.json() == { "id": str(document.id), "abilities": { @@ -474,7 +463,6 @@ def test_api_documents_retrieve_authenticated_related_parent(): "accesses_view": True, "ai_transform": access.role != "reader", "ai_translate": access.role != "reader", - "ancestors_links_definitions": {"restricted": ancestors_roles}, "attachment_upload": access.role != "reader", "can_edit": access.role != "reader", "children_create": access.role != "reader", @@ -488,7 +476,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "invite_owner": access.role == "owner", "link_configuration": access.role in ["administrator", "owner"], "link_select_options": models.LinkReachChoices.get_select_options( - links_definitions + **link_definition ), "media_auth": True, "media_check": True, @@ -516,7 +504,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } @@ -612,16 +600,16 @@ def test_api_documents_retrieve_authenticated_related_team_none(mock_user_teams) @pytest.mark.parametrize( - "teams,roles", + "teams,role", [ - [["readers"], ["reader"]], - [["unknown", "readers"], ["reader"]], - [["editors"], ["editor"]], - [["unknown", "editors"], ["editor"]], + [["readers"], "reader"], + [["unknown", "readers"], "reader"], + [["editors"], "editor"], + [["unknown", "editors"], "editor"], ], ) def test_api_documents_retrieve_authenticated_related_team_members( - teams, roles, mock_user_teams + teams, role, mock_user_teams ): """ Authenticated users should be allowed to retrieve a document to which they @@ -668,20 +656,20 @@ def test_api_documents_retrieve_authenticated_related_team_members( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": roles, + "user_role": role, } @pytest.mark.parametrize( - "teams,roles", + "teams,role", [ - [["administrators"], ["administrator"]], - [["editors", "administrators"], ["administrator", "editor"]], - [["unknown", "administrators"], ["administrator"]], + [["administrators"], "administrator"], + [["editors", "administrators"], "administrator"], + [["unknown", "administrators"], "administrator"], ], ) def test_api_documents_retrieve_authenticated_related_team_administrators( - teams, roles, mock_user_teams + teams, role, mock_user_teams ): """ Authenticated users should be allowed to retrieve a document to which they @@ -730,21 +718,21 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": roles, + "user_role": role, } @pytest.mark.parametrize( - "teams,roles", + "teams,role", [ - [["owners"], ["owner"]], - [["owners", "administrators"], ["owner", "administrator"]], - [["members", "administrators", "owners"], ["owner", "administrator"]], - [["unknown", "owners"], ["owner"]], + [["owners"], "owner"], + [["owners", "administrators"], "owner"], + [["members", "administrators", "owners"], "owner"], + [["unknown", "owners"], "owner"], ], ) def test_api_documents_retrieve_authenticated_related_team_owners( - teams, roles, mock_user_teams + teams, role, mock_user_teams ): """ Authenticated users should be allowed to retrieve a restricted document to which @@ -792,11 +780,11 @@ def test_api_documents_retrieve_authenticated_related_team_owners( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": roles, + "user_role": role, } -def test_api_documents_retrieve_user_roles(django_assert_max_num_queries): +def test_api_documents_retrieve_user_role(django_assert_max_num_queries): """ Roles should be annotated on querysets taking into account all documents ancestors. """ @@ -819,15 +807,14 @@ def test_api_documents_retrieve_user_roles(django_assert_max_num_queries): factories.UserDocumentAccessFactory(document=parent, user=user), factories.UserDocumentAccessFactory(document=document, user=user), ) - expected_roles = {access.role for access in accesses} + expected_role = choices.RoleChoices.max(*[access.role for access in accesses]) with django_assert_max_num_queries(14): response = client.get(f"/api/v1.0/documents/{document.id!s}/") assert response.status_code == 200 - user_roles = response.json()["user_roles"] - assert set(user_roles) == expected_roles + assert response.json()["user_role"] == expected_role def test_api_documents_retrieve_numqueries_with_link_trace(django_assert_num_queries): diff --git a/src/backend/core/tests/documents/test_api_documents_trashbin.py b/src/backend/core/tests/documents/test_api_documents_trashbin.py index 91cdf073..cafbbd6b 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -74,7 +74,6 @@ def test_api_documents_trashbin_format(): "accesses_view": True, "ai_transform": True, "ai_translate": True, - "ancestors_links_definitions": {}, "attachment_upload": True, "can_edit": True, "children_create": True, @@ -116,7 +115,7 @@ def test_api_documents_trashbin_format(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": ["owner"], + "user_role": "owner", } diff --git a/src/backend/core/tests/documents/test_api_documents_tree.py b/src/backend/core/tests/documents/test_api_documents_tree.py index 33fa614b..32de7c4b 100644 --- a/src/backend/core/tests/documents/test_api_documents_tree.py +++ b/src/backend/core/tests/documents/test_api_documents_tree.py @@ -57,7 +57,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": document.created_at.isoformat().replace("+00:00", "Z"), @@ -74,7 +74,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": sibling1.get_abilities(AnonymousUser()), @@ -93,7 +93,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q "path": sibling1.path, "title": sibling1.title, "updated_at": sibling1.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": sibling2.get_abilities(AnonymousUser()), @@ -112,7 +112,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q "path": sibling2.path, "title": sibling2.title, "updated_at": sibling2.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -129,7 +129,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -163,7 +163,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): response = APIClient().get(f"/api/v1.0/documents/{document.id!s}/tree/") assert response.status_code == 200 - assert response.json() == { + expected_tree = { "abilities": grand_parent.get_abilities(AnonymousUser()), "children": [ { @@ -193,7 +193,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": document.created_at.isoformat().replace( @@ -214,7 +214,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): "updated_at": document.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, { "abilities": document_sibling.get_abilities(AnonymousUser()), @@ -237,7 +237,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): "updated_at": document_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -254,7 +254,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": parent_sibling.get_abilities(AnonymousUser()), @@ -277,7 +277,7 @@ def test_api_documents_tree_list_anonymous_public_parent(): "updated_at": parent_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": grand_parent.created_at.isoformat().replace("+00:00", "Z"), @@ -294,8 +294,9 @@ def test_api_documents_tree_list_anonymous_public_parent(): "path": grand_parent.path, "title": grand_parent.title, "updated_at": grand_parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } + assert response.json() == expected_tree @pytest.mark.parametrize("reach", ["restricted", "authenticated"]) @@ -366,7 +367,7 @@ def test_api_documents_tree_list_authenticated_unrelated_public_or_authenticated "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": document.created_at.isoformat().replace("+00:00", "Z"), @@ -383,7 +384,7 @@ def test_api_documents_tree_list_authenticated_unrelated_public_or_authenticated "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": sibling.get_abilities(user), @@ -402,7 +403,7 @@ def test_api_documents_tree_list_authenticated_unrelated_public_or_authenticated "path": sibling.path, "title": sibling.title, "updated_at": sibling.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -419,7 +420,7 @@ def test_api_documents_tree_list_authenticated_unrelated_public_or_authenticated "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -488,7 +489,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": document.created_at.isoformat().replace( @@ -509,7 +510,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "updated_at": document.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, { "abilities": document_sibling.get_abilities(user), @@ -532,7 +533,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "updated_at": document_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -549,7 +550,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, }, { "abilities": parent_sibling.get_abilities(user), @@ -572,7 +573,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "updated_at": parent_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [], + "user_role": None, }, ], "created_at": grand_parent.created_at.isoformat().replace("+00:00", "Z"), @@ -589,7 +590,7 @@ def test_api_documents_tree_list_authenticated_public_or_authenticated_parent( "path": grand_parent.path, "title": grand_parent.title, "updated_at": grand_parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [], + "user_role": None, } @@ -664,7 +665,7 @@ def test_api_documents_tree_list_authenticated_related_direct(): "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": document.created_at.isoformat().replace("+00:00", "Z"), @@ -681,7 +682,7 @@ def test_api_documents_tree_list_authenticated_related_direct(): "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": sibling.get_abilities(user), @@ -700,7 +701,7 @@ def test_api_documents_tree_list_authenticated_related_direct(): "path": sibling.path, "title": sibling.title, "updated_at": sibling.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -717,7 +718,7 @@ def test_api_documents_tree_list_authenticated_related_direct(): "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } @@ -790,7 +791,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": document.created_at.isoformat().replace( @@ -811,7 +812,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "updated_at": document.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": document_sibling.get_abilities(user), @@ -834,7 +835,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "updated_at": document_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -851,7 +852,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": parent_sibling.get_abilities(user), @@ -874,7 +875,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "updated_at": parent_sibling.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": grand_parent.created_at.isoformat().replace("+00:00", "Z"), @@ -891,7 +892,7 @@ def test_api_documents_tree_list_authenticated_related_parent(): "path": grand_parent.path, "title": grand_parent.title, "updated_at": grand_parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } @@ -974,7 +975,7 @@ def test_api_documents_tree_list_authenticated_related_team_members( "updated_at": child.updated_at.isoformat().replace( "+00:00", "Z" ), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": document.created_at.isoformat().replace("+00:00", "Z"), @@ -991,7 +992,7 @@ def test_api_documents_tree_list_authenticated_related_team_members( "path": document.path, "title": document.title, "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, { "abilities": sibling.get_abilities(user), @@ -1010,7 +1011,7 @@ def test_api_documents_tree_list_authenticated_related_team_members( "path": sibling.path, "title": sibling.title, "updated_at": sibling.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, }, ], "created_at": parent.created_at.isoformat().replace("+00:00", "Z"), @@ -1027,5 +1028,5 @@ def test_api_documents_tree_list_authenticated_related_team_members( "path": parent.path, "title": parent.title, "updated_at": parent.updated_at.isoformat().replace("+00:00", "Z"), - "user_roles": [access.role], + "user_role": access.role, } diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index a56af68b..7542d375 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -154,7 +154,6 @@ def test_models_documents_get_abilities_forbidden( "accesses_view": False, "ai_transform": False, "ai_translate": False, - "ancestors_links_definitions": {}, "attachment_upload": False, "can_edit": False, "children_create": False, @@ -217,7 +216,6 @@ def test_models_documents_get_abilities_reader( "accesses_view": False, "ai_transform": False, "ai_translate": False, - "ancestors_links_definitions": {}, "attachment_upload": False, "can_edit": False, "children_create": False, @@ -256,7 +254,7 @@ def test_models_documents_get_abilities_reader( assert all( value is False for key, value in document.get_abilities(user).items() - if key not in ["link_select_options", "ancestors_links_definitions"] + if key not in ["link_select_options", "ancestors_links_definition"] ) @@ -282,7 +280,6 @@ def test_models_documents_get_abilities_editor( "accesses_view": False, "ai_transform": is_authenticated, "ai_translate": is_authenticated, - "ancestors_links_definitions": {}, "attachment_upload": True, "can_edit": True, "children_create": is_authenticated, @@ -320,7 +317,7 @@ def test_models_documents_get_abilities_editor( assert all( value is False for key, value in document.get_abilities(user).items() - if key not in ["link_select_options", "ancestors_links_definitions"] + if key not in ["link_select_options", "ancestors_links_definition"] ) @@ -336,7 +333,6 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): "accesses_view": True, "ai_transform": True, "ai_translate": True, - "ancestors_links_definitions": {}, "attachment_upload": True, "can_edit": True, "children_create": True, @@ -387,7 +383,6 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) "accesses_view": True, "ai_transform": True, "ai_translate": True, - "ancestors_links_definitions": {}, "attachment_upload": True, "can_edit": True, "children_create": True, @@ -425,7 +420,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) assert all( value is False for key, value in document.get_abilities(user).items() - if key not in ["link_select_options", "ancestors_links_definitions"] + if key not in ["link_select_options", "ancestors_links_definition"] ) @@ -441,7 +436,6 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): "accesses_view": True, "ai_transform": True, "ai_translate": True, - "ancestors_links_definitions": {}, "attachment_upload": True, "can_edit": True, "children_create": True, @@ -479,7 +473,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): assert all( value is False for key, value in document.get_abilities(user).items() - if key not in ["link_select_options", "ancestors_links_definitions"] + if key not in ["link_select_options", "ancestors_links_definition"] ) @@ -502,7 +496,6 @@ def test_models_documents_get_abilities_reader_user( # You should not access AI if it's restricted to users with specific access "ai_transform": access_from_link and ai_access_setting != "restricted", "ai_translate": access_from_link and ai_access_setting != "restricted", - "ancestors_links_definitions": {}, "attachment_upload": access_from_link, "can_edit": access_from_link, "children_create": access_from_link, @@ -542,7 +535,7 @@ def test_models_documents_get_abilities_reader_user( assert all( value is False for key, value in document.get_abilities(user).items() - if key not in ["link_select_options", "ancestors_links_definitions"] + if key not in ["link_select_options", "ancestors_links_definition"] ) @@ -561,7 +554,6 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): "accesses_view": True, "ai_transform": False, "ai_translate": False, - "ancestors_links_definitions": {}, "attachment_upload": False, "can_edit": False, "children_create": False, @@ -1269,45 +1261,60 @@ def test_models_documents_get_select_options(reach, role, select_options): assert models.LinkReachChoices.get_select_options(reach, role) == select_options -def test_models_documents_compute_ancestors_links_no_highest_readable(): - """Test the compute_ancestors_links method.""" - document = factories.DocumentFactory(link_reach="public") - assert document.compute_ancestors_links(user=AnonymousUser()) == [] - - -def test_models_documents_compute_ancestors_links_highest_readable( +def test_models_documents_compute_ancestors_links_paths_mapping_single( django_assert_num_queries, ): - """Test the compute_ancestors_links method.""" + """Test the compute_ancestors_links_paths_mapping method on a single document.""" + document = factories.DocumentFactory(link_reach="public") + with django_assert_num_queries(1): + assert document.compute_ancestors_links_paths_mapping() == { + document.path: [{"link_reach": "public", "link_role": document.link_role}] + } + + +def test_models_documents_compute_ancestors_links_paths_mapping_structure( + django_assert_num_queries, +): + """Test the compute_ancestors_links_paths_mapping method on a tree of documents.""" user = factories.UserFactory() other_user = factories.UserFactory() - root = factories.DocumentFactory( - link_reach="restricted", link_role="reader", users=[user] - ) - factories.DocumentFactory( - parent=root, link_reach="public", link_role="reader", users=[user] - ) - child2 = factories.DocumentFactory( + root = factories.DocumentFactory(link_reach="restricted", users=[user]) + document = factories.DocumentFactory( parent=root, link_reach="authenticated", link_role="editor", users=[user, other_user], ) - child3 = factories.DocumentFactory( - parent=child2, + sibling = factories.DocumentFactory(parent=root, link_reach="public", users=[user]) + child = factories.DocumentFactory( + parent=document, link_reach="authenticated", link_role="reader", users=[user, other_user], ) - with django_assert_num_queries(2): - assert child3.compute_ancestors_links(user=user) == [ - {"link_reach": root.link_reach, "link_role": root.link_role}, - {"link_reach": child2.link_reach, "link_role": child2.link_role}, - ] + # Child + with django_assert_num_queries(1): + assert child.compute_ancestors_links_paths_mapping() == { + root.path: [{"link_reach": "restricted", "link_role": root.link_role}], + document.path: [ + {"link_reach": "restricted", "link_role": root.link_role}, + {"link_reach": document.link_reach, "link_role": document.link_role}, + ], + child.path: [ + {"link_reach": "restricted", "link_role": root.link_role}, + {"link_reach": document.link_reach, "link_role": document.link_role}, + {"link_reach": child.link_reach, "link_role": child.link_role}, + ], + } - with django_assert_num_queries(2): - assert child3.compute_ancestors_links(user=other_user) == [ - {"link_reach": child2.link_reach, "link_role": child2.link_role}, - ] + # Sibling + with django_assert_num_queries(1): + assert sibling.compute_ancestors_links_paths_mapping() == { + root.path: [{"link_reach": "restricted", "link_role": root.link_role}], + sibling.path: [ + {"link_reach": "restricted", "link_role": root.link_role}, + {"link_reach": sibling.link_reach, "link_role": sibling.link_role}, + ], + }