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}, + ], + }