✨(backend) allow to leave a document

We want to allow users to leave a document where they have an access or
they have visited creating a link_trace. All subdocuments should also be
leaved at the same time.
To know if the user can leave a doc we have to check when computing the
abilities if a record is existing in the LinkTrace table. This is a N+1
query situation. To avoid it, we added an annotation in the
DocumentQueryset like we already do to annotate the user role.
There is one edge case where the annotation is made to soon, it is when the
user is visiting a document for the first time, the `get_object` add the
annotation and in the permission, we compute the abilities. The `leave`
property is False because the entry in the LinkTrace table is not made,
when the serializer ask for the abilities again, it is still False. So
in the `retrieve` method in the viewset we force the
`user_has_link_trace` to the correct value.
This commit is contained in:
Manuel Raynaud
2026-06-01 16:25:50 +02:00
parent 7773a5b9bf
commit 9f98af1387
7 changed files with 404 additions and 11 deletions
+1
View File
@@ -13,6 +13,7 @@ and this project adheres to
- ✨(buildpack) add PaaS deployment support, tested with Scalingo #2293
- 🔧(backend) allow configuring settings OIDC_OP_USER_ENDPOINT_FORMAT
- ⚡️(helm) create a dedicated svc and deployment for yprovider converter #2368
- ✨(backend) allow to leave a document #2365
### Changed
+47 -6
View File
@@ -19,7 +19,7 @@ from django.core.cache import cache
from django.core.exceptions import ValidationError
from django.core.files.storage import default_storage
from django.core.validators import URLValidator
from django.db import connection, transaction
from django.db import DatabaseError, connection, transaction
from django.db import models as db
from django.db.models.expressions import RawSQL
from django.db.models.functions import Greatest, Left, Length
@@ -598,6 +598,8 @@ class DocumentViewSet(
user = self.request.user
queryset = queryset.annotate_is_favorite(user)
queryset = queryset.annotate_user_roles(user)
queryset = queryset.annotate_user_has_link_trace(user)
return queryset
def get_response_for_queryset(self, queryset, context=None):
@@ -638,6 +640,7 @@ class DocumentViewSet(
for parent in (
models.Document.objects.annotate_user_roles(self.request.user)
.annotate_is_favorite(self.request.user)
.annotate_user_has_link_trace(self.request.user)
.filter(path__in=missing_parent_paths)
.iterator()
):
@@ -675,7 +678,7 @@ class DocumentViewSet(
for field in ["is_creator_me", "title", "q"]:
queryset = filterset.filters[field].filter(queryset, filter_data[field])
queryset = queryset.annotate_user_roles(user)
queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user)
# Among the results, we may have documents that are ancestors/descendants
# of each other. In this case we want to keep only the highest ancestors.
@@ -706,7 +709,6 @@ class DocumentViewSet(
"""
user = self.request.user
instance = self.get_object()
serializer = self.get_serializer(instance)
# The `create` query generates 5 db queries which are much less efficient than an
# `exists` query. The user will visit the document many times after the first visit
@@ -717,6 +719,11 @@ class DocumentViewSet(
):
models.LinkTrace.objects.create(document=instance, user=request.user)
# To avoid N+1 query, we force the `user_has_link_trace` normally set by the
# queryset.annotate_user_has_link_trace method. If the user is connected, it must be True.
instance.user_has_link_trace = user.is_authenticated
serializer = self.get_serializer(instance)
return drf.response.Response(serializer.data)
def _apply_uploaded_file_conversion(self, serializer):
@@ -880,7 +887,7 @@ class DocumentViewSet(
queryset = queryset.filter(id__in=favorite_documents_ids)
queryset = queryset.filter(ancestors_deleted_at__isnull=True)
queryset = queryset.order_by("-updated_at")
queryset = queryset.annotate_user_roles(user)
queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user)
queryset = queryset.annotate(
is_favorite=db.Value(True, output_field=db.BooleanField())
)
@@ -922,7 +929,9 @@ class DocumentViewSet(
deleted_at__isnull=False,
deleted_at__gte=models.get_trashbin_cutoff(),
)
queryset = queryset.annotate_user_roles(self.request.user)
queryset = queryset.annotate_user_roles(
self.request.user
).annotate_user_has_link_trace(self.request.user)
return self.get_response_for_queryset(queryset)
@@ -1174,7 +1183,7 @@ class DocumentViewSet(
for field in ["is_creator_me", "title", "q"]:
queryset = filterset.filters[field].filter(queryset, filter_data[field])
queryset = queryset.annotate_user_roles(user)
queryset = queryset.annotate_user_roles(user).annotate_user_has_link_trace(user)
# Annotate favorite status and filter if applicable as late as possible
queryset = queryset.annotate_is_favorite(user)
@@ -1273,6 +1282,7 @@ class DocumentViewSet(
queryset = queryset.order_by("path")
queryset = queryset.annotate_user_roles(user)
queryset = queryset.annotate_is_favorite(user)
queryset = queryset.annotate_user_has_link_trace(user)
# Pass ancestors' links paths mapping to the serializer as a context variable
# in order to allow saving time while computing abilities on the instance
@@ -1590,6 +1600,7 @@ class DocumentViewSet(
.filter(ancestors_deleted_at__isnull=True)
.annotate_user_roles(user)
.annotate_is_favorite(user)
.annotate_user_has_link_trace(user)
)
queryset = filterset.filter_queryset(queryset)
@@ -2499,6 +2510,36 @@ class DocumentViewSet(
}
)
@drf.decorators.action(
detail=True,
methods=["post"],
)
def leave(self, request, *args, **kwargs):
"""
Remove document_accesses if exists and the link_trace related to the current document
for the connected user.
"""
# Check for permissions.
document = self.get_object()
try:
with transaction.atomic():
models.DocumentAccess.objects.filter(
document__path__startswith=document.path, user=request.user
).delete()
models.LinkTrace.objects.filter(
document__path__startswith=document.path, user=request.user
).delete()
except DatabaseError:
logger.error(
"Impossible to leave document %s for user %s",
str(document.id),
str(request.user.id),
)
raise
return drf.response.Response(status=drf.status.HTTP_204_NO_CONTENT)
class DocumentAccessViewSet(
ResourceAccessViewsetMixin,
+37 -1
View File
@@ -852,6 +852,22 @@ class DocumentQuerySet(MP_NodeQuerySet):
user_roles=models.Value([], output_field=output_field),
)
def annotate_user_has_link_trace(self, user):
"""
Annotate document queryset with a boolean to know if the current user
has a link_trace on the current document.
"""
if user.is_authenticated:
link_trace_exists_subquery = LinkTrace.objects.filter(
document_id=models.OuterRef("pk"), user=user
)
return self.annotate(
user_has_link_trace=models.Exists(link_trace_exists_subquery)
)
return self.annotate(user_has_link_trace=models.Value(False))
class DocumentManager(MP_NodeManager.from_queryset(DocumentQuerySet)):
"""
@@ -1150,6 +1166,17 @@ class Document(MP_Node, BaseModel):
return RoleChoices.max(*roles)
def has_link_trace(self, user):
"""Return if the user has a link trace on this document."""
if not user.is_authenticated:
return False
try:
return self.user_has_link_trace
except AttributeError:
return LinkTrace.objects.filter(document=self, user=user).exists()
def compute_ancestors_links_paths_mapping(self):
"""
Compute the ancestors links for the current document up to the highest readable ancestor.
@@ -1227,7 +1254,7 @@ class Document(MP_Node, BaseModel):
"""Actual link role on the document."""
return self.computed_link_definition["link_role"]
def get_abilities(self, user):
def get_abilities(self, user): # pylint: disable=too-many-locals
"""
Compute and return abilities for a given user on the document.
"""
@@ -1248,6 +1275,14 @@ class Document(MP_Node, BaseModel):
is_owner_or_admin or role == RoleChoices.EDITOR
) and not is_deleted
# compute can_leave
# A user can leave a document if it has non privileged role on the document or it has
# access to it with a link_trace
can_leave = user.is_authenticated and (
(has_access_role and not is_owner_or_admin)
or (not has_access_role and self.has_link_trace(user))
)
link_select_options = LinkReachChoices.get_select_options(
**self.ancestors_link_definition
)
@@ -1312,6 +1347,7 @@ class Document(MP_Node, BaseModel):
"favorite": can_get and user.is_authenticated,
"link_configuration": is_owner_or_admin,
"invite_owner": is_owner and not is_deleted,
"leave": can_leave,
"move": is_owner_or_admin and not is_deleted,
"partial_update": can_update,
"restore": is_owner,
@@ -0,0 +1,298 @@
"""Test for the leave document API"""
import pytest
from rest_framework import status
from rest_framework.test import APIClient
from core import factories, models
pytestmark = pytest.mark.django_db
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
def test_api_documents_leave_document_anonymous_user(link_reach):
"""
Anonymous user are not allowed to access the leave feature no matter the document link reach.
"""
document = factories.DocumentFactory(link_reach=link_reach)
client = APIClient()
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_401_UNAUTHORIZED
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
def test_api_documents_leave_connected_user_without_access_nor_link_trace(link_reach):
"""
A connected user with no access or link_trace on the document can not access the leave feature
"""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(link_reach=link_reach, link_traces=other_users)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
assert not models.LinkTrace.objects.filter(document=document, user=user).exists()
assert not models.DocumentAccess.objects.filter(
document=document, user=user
).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 4
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_403_FORBIDDEN
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 4
@pytest.mark.parametrize(
"link_reach",
[models.LinkReachChoices.PUBLIC, models.LinkReachChoices.AUTHENTICATED],
)
def test_api_documents_leave_connected_user_with_link_trace(link_reach):
"""
A connected user with link_trace on a document can leave it.
"""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=[user, *other_users]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
assert models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 4
assert models.DocumentAccess.objects.count() == 4
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_204_NO_CONTENT
assert not models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 4
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
@pytest.mark.parametrize(
"role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES]
)
def test_api_documents_leave_connected_user_with_access(role, link_reach):
"""Connected user with a DocumentAccess can leave it."""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=other_users, users=[(user, role)]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 5
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_204_NO_CONTENT
assert not models.DocumentAccess.objects.filter(
document=document, user=user
).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 4
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
@pytest.mark.parametrize(
"role", [role for role in models.RoleChoices if role in models.PRIVILEGED_ROLES]
)
def test_api_documents_leave_connected_access_with_privileged_role_not_allowed(
role, link_reach
):
"""Connected user with privileged access role can not leave a document."""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 4
assert models.DocumentAccess.objects.count() == 5
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_403_FORBIDDEN
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 4
assert models.DocumentAccess.objects.count() == 5
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
@pytest.mark.parametrize(
"role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES]
)
def test_api_documents_leave_connected_user_with_access_and_link_trace(
role, link_reach
):
"""Connected user with a DocumentAccess can leave it."""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 4
assert models.DocumentAccess.objects.count() == 5
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_204_NO_CONTENT
assert not models.DocumentAccess.objects.filter(
document=document, user=user
).exists()
assert not models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 4
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
@pytest.mark.parametrize(
"role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES]
)
def test_api_documents_leave_connected_accessing_multiple_documents_leave_only_one(
role, link_reach
):
"""Connected user accessing multiple document leaving one should keep access to the others."""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
# Create access to other documents for the same user
factories.UserDocumentAccessFactory.create_batch(4, user=user)
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 4
assert models.DocumentAccess.objects.count() == 9
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_204_NO_CONTENT
assert not models.DocumentAccess.objects.filter(
document=document, user=user
).exists()
assert not models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.count() == 3
assert models.DocumentAccess.objects.count() == 8
@pytest.mark.parametrize(
"link_reach",
models.LinkReachChoices.values,
)
@pytest.mark.parametrize(
"role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES]
)
def test_api_documents_leave_connected_leave_also_sub_documents(role, link_reach):
"""User connected with access and link_trace to a tree should leave all the tree."""
user = factories.UserFactory()
other_users = factories.UserFactory.create_batch(3)
document = factories.DocumentFactory(
link_reach=link_reach, link_traces=[user, *other_users], users=[(user, role)]
)
child = factories.DocumentFactory(parent=document, link_traces=[user, *other_users])
grand_child = factories.DocumentFactory(
parent=child, link_traces=[user, *other_users], users=[(user, role)]
)
factories.UserDocumentAccessFactory.create_batch(4, document=document)
# Create access to other documents for the same user
factories.UserDocumentAccessFactory.create_batch(4, user=user)
assert models.DocumentAccess.objects.filter(document=document, user=user).exists()
assert models.DocumentAccess.objects.filter(
document=grand_child, user=user
).exists()
assert models.LinkTrace.objects.filter(document=document, user=user).exists()
assert models.LinkTrace.objects.filter(document=child, user=user).exists()
assert models.LinkTrace.objects.filter(document=grand_child, user=user).exists()
assert models.LinkTrace.objects.count() == 12
assert models.DocumentAccess.objects.count() == 10
client = APIClient()
client.force_login(user)
response = client.post(f"/api/v1.0/documents/{document.id!s}/leave/")
assert response.status_code == status.HTTP_204_NO_CONTENT
assert not models.DocumentAccess.objects.filter(
document=document, user=user
).exists()
assert not models.DocumentAccess.objects.filter(
document=grand_child, user=user
).exists()
assert not models.LinkTrace.objects.filter(document=document, user=user).exists()
assert not models.LinkTrace.objects.filter(document=child, user=user).exists()
assert not models.LinkTrace.objects.filter(document=grand_child, user=user).exists()
assert models.LinkTrace.objects.count() == 9
assert models.DocumentAccess.objects.count() == 8
@@ -54,6 +54,7 @@ def test_api_documents_retrieve_anonymous_public_standalone():
},
"content_patch": document.link_role == "editor",
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": False,
@@ -132,6 +133,7 @@ def test_api_documents_retrieve_anonymous_public_parent():
),
"content_patch": grand_parent.link_role == "editor",
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": False,
@@ -243,6 +245,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated(
},
"content_patch": document.link_role == "editor",
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"move": False,
@@ -329,6 +332,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea
"move": False,
"content_patch": grand_parent.link_role == "editor",
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"partial_update": grand_parent.link_role == "editor",
@@ -527,6 +531,7 @@ def test_api_documents_retrieve_authenticated_related_parent():
),
"content_patch": access.role not in ["reader", "commenter"],
"content_retrieve": True,
"leave": access.role not in ["administrator", "owner"],
"media_auth": True,
"media_check": True,
"move": access.role in ["administrator", "owner"],
@@ -96,6 +96,7 @@ def test_api_documents_trashbin_format():
},
"content_patch": False,
"content_retrieve": True,
"leave": False,
"media_auth": False,
"media_check": False,
"move": False, # Can't move a deleted document
@@ -173,6 +173,7 @@ def test_models_documents_get_abilities_forbidden(
"invite_owner": False,
"content_patch": False,
"content_retrieve": False,
"leave": False,
"media_auth": False,
"media_check": False,
"move": False,
@@ -192,7 +193,7 @@ def test_models_documents_get_abilities_forbidden(
"versions_retrieve": False,
"search": False,
}
nb_queries = 1 if is_authenticated else 0
nb_queries = 2 if is_authenticated else 0
with django_assert_num_queries(nb_queries):
assert document.get_abilities(user) == expected_abilities
document.soft_delete()
@@ -247,6 +248,7 @@ def test_models_documents_get_abilities_reader(
},
"content_patch": False,
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": False,
@@ -260,7 +262,7 @@ def test_models_documents_get_abilities_reader(
"versions_retrieve": False,
"search": True,
}
nb_queries = 1 if is_authenticated else 0
nb_queries = 2 if is_authenticated else 0
with django_assert_num_queries(nb_queries):
assert document.get_abilities(user) == expected_abilities
@@ -320,6 +322,7 @@ def test_models_documents_get_abilities_commenter(
},
"content_patch": False,
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": False,
@@ -333,7 +336,7 @@ def test_models_documents_get_abilities_commenter(
"versions_retrieve": False,
"search": True,
}
nb_queries = 1 if is_authenticated else 0
nb_queries = 2 if is_authenticated else 0
with django_assert_num_queries(nb_queries):
assert document.get_abilities(user) == expected_abilities
@@ -390,6 +393,7 @@ def test_models_documents_get_abilities_editor(
},
"content_patch": True,
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": False,
@@ -403,7 +407,7 @@ def test_models_documents_get_abilities_editor(
"versions_retrieve": False,
"search": True,
}
nb_queries = 1 if is_authenticated else 0
nb_queries = 2 if is_authenticated else 0
with django_assert_num_queries(nb_queries):
assert document.get_abilities(user) == expected_abilities
document.soft_delete()
@@ -449,6 +453,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries):
},
"content_patch": True,
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": True,
@@ -494,6 +499,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries):
},
"content_patch": False,
"content_retrieve": True,
"leave": False,
"media_auth": False,
"media_check": False,
"move": False,
@@ -543,6 +549,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries)
},
"content_patch": True,
"content_retrieve": True,
"leave": False,
"media_auth": True,
"media_check": True,
"move": True,
@@ -602,6 +609,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries):
},
"content_patch": True,
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"move": False,
@@ -669,6 +677,7 @@ def test_models_documents_get_abilities_reader_user(
},
"content_patch": access_from_link,
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"move": False,
@@ -737,6 +746,7 @@ def test_models_documents_get_abilities_commenter_user(
},
"content_patch": access_from_link,
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"move": False,
@@ -801,6 +811,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries):
},
"content_patch": False,
"content_retrieve": True,
"leave": True,
"media_auth": True,
"media_check": True,
"move": False,