From 56aa69f56ace369e7163e05f28b53d53d69f9930 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Mon, 17 Feb 2025 10:19:06 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20refactor=20list?= =?UTF-8?q?=20view=20to=20allow=20filtering=20other=20views?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the "filter_queryset" method is called in the middle of the "get_object" method. We use the "get_object" in actions like "children", "tree", etc. which start by calling "get_object" but return lists of documents. We would like to apply filters to these views but the it didn't work because the "get_object" method was also impacted by the filters... In a future PR, we should take control of the "get_object" method and decouple all this. We need a quick solution to allow releasing the hierchical documents feature in the frontend. --- CHANGELOG.md | 2 + src/backend/core/api/filters.py | 17 ++- src/backend/core/api/serializers.py | 5 +- src/backend/core/api/viewsets.py | 118 ++++++++++++------ src/backend/core/models.py | 25 ++-- .../test_api_documents_children_create.py | 2 +- .../test_api_documents_children_list.py | 2 +- 7 files changed, 109 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65156052..365cb1fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to ## Fixed - 🐛(frontend) remove scroll listener table content #688 - 🔒️(back) restrict access to favorite_list endpoint #690 +- 🐛(backend) refactor to fix filtering on children + and descendants views #695 ## [2.4.0] - 2025-03-06 diff --git a/src/backend/core/api/filters.py b/src/backend/core/api/filters.py index 2f3b9a9e..731deb15 100644 --- a/src/backend/core/api/filters.py +++ b/src/backend/core/api/filters.py @@ -12,15 +12,26 @@ class DocumentFilter(django_filters.FilterSet): Custom filter for filtering documents. """ + title = django_filters.CharFilter( + field_name="title", lookup_expr="icontains", label=_("Title") + ) + + class Meta: + model = models.Document + fields = ["title"] + + +class ListDocumentFilter(DocumentFilter): + """ + Custom filter for filtering documents. + """ + is_creator_me = django_filters.BooleanFilter( method="filter_is_creator_me", label=_("Creator is me") ) is_favorite = django_filters.BooleanFilter( method="filter_is_favorite", label=_("Favorite") ) - title = django_filters.CharFilter( - field_name="title", lookup_expr="icontains", label=_("Title") - ) class Meta: model = models.Document diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 4b8f526c..d6b557dd 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -177,10 +177,7 @@ class ListDocumentSerializer(serializers.ModelSerializer): request = self.context.get("request") if request: - paths_links_mapping = self.context.get("paths_links_mapping", None) - return document.get_abilities( - request.user, paths_links_mapping=paths_links_mapping - ) + return document.get_abilities(request.user) return {} diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 34dc5903..687deb83 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -31,7 +31,7 @@ from core.services.ai_services import AIService from core.services.collaboration_services import CollaborationService from . import permissions, serializers, utils -from .filters import DocumentFilter +from .filters import DocumentFilter, ListDocumentFilter logger = logging.getLogger(__name__) @@ -316,7 +316,6 @@ class DocumentViewSet( SerializerPerActionMixin, drf.mixins.CreateModelMixin, drf.mixins.DestroyModelMixin, - drf.mixins.ListModelMixin, drf.mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -414,8 +413,6 @@ class DocumentViewSet( - Implements soft delete logic to retain document tree structures. """ - filter_backends = [drf_filters.DjangoFilterBackend] - filterset_class = DocumentFilter metadata_class = DocumentMetadata ordering = ["-updated_at"] ordering_fields = ["created_at", "updated_at", "title"] @@ -502,11 +499,42 @@ class DocumentViewSet( ) def filter_queryset(self, queryset): - """Apply annotations and filters sequentially.""" - filterset = DocumentFilter( + """Override to apply annotations to generic views.""" + queryset = super().filter_queryset(queryset) + queryset = self.annotate_is_favorite(queryset) + queryset = self.annotate_user_roles(queryset) + return queryset + + def get_response_for_queryset(self, queryset): + """Return paginated response for the queryset if requested.""" + page = self.paginate_queryset(queryset) + if page is not None: + serializer = self.get_serializer(page, many=True) + return self.get_paginated_response(serializer.data) + + serializer = self.get_serializer(queryset, many=True) + return drf.response.Response(serializer.data) + + def list(self, request, *args, **kwargs): + """ + Returns a DRF response containing the filtered, annotated and ordered document list. + + 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. + """ + queryset = ( + self.get_queryset() + ) # Not calling filter_queryset. We do our own cooking. + + filterset = ListDocumentFilter( self.request.GET, queryset=queryset, request=self.request ) - filterset.is_valid() + if not filterset.is_valid(): + raise drf.exceptions.ValidationError(filterset.errors) filter_data = filterset.form.cleaned_data # Filter as early as possible on fields that are available on the model @@ -515,22 +543,19 @@ class DocumentViewSet( queryset = self.annotate_user_roles(queryset) - if self.action == "list": - # 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. - root_paths = utils.filter_root_paths( - queryset.order_by("path").values_list("path", flat=True), - skip_sorting=True, - ) - queryset = queryset.filter(path__in=root_paths) + # 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. + root_paths = utils.filter_root_paths( + queryset.order_by("path").values_list("path", flat=True), + skip_sorting=True, + ) + 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 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 = self.annotate_is_favorite(queryset) @@ -539,18 +564,11 @@ class DocumentViewSet( ) # Apply ordering only now that everyting is filtered and annotated - return filters.OrderingFilter().filter_queryset(self.request, queryset, self) + queryset = filters.OrderingFilter().filter_queryset( + self.request, queryset, self + ) - def get_response_for_queryset(self, queryset): - """Return paginated response for the queryset if requested.""" - page = self.paginate_queryset(queryset) - if page is not None: - serializer = self.get_serializer(page, many=True) - result = self.get_paginated_response(serializer.data) - return result - - serializer = self.get_serializer(queryset, many=True) - return drf.response.Response(serializer.data) + return self.get_response_for_queryset(queryset) def retrieve(self, request, *args, **kwargs): """ @@ -604,7 +622,7 @@ class DocumentViewSet( user=user ).values_list("document_id", flat=True) - queryset = self.get_queryset() + queryset = self.filter_queryset(self.get_queryset()) queryset = queryset.filter(id__in=favorite_documents_ids) return self.get_response_for_queryset(queryset) @@ -731,7 +749,6 @@ class DocumentViewSet( detail=True, methods=["get", "post"], ordering=["path"], - url_path="children", ) def children(self, request, *args, **kwargs): """Handle listing and creating children of a document""" @@ -763,10 +780,35 @@ class DocumentViewSet( ) # GET: List children - queryset = document.get_children().filter(deleted_at__isnull=True) + queryset = document.get_children().filter(ancestors_deleted_at__isnull=True) queryset = self.filter_queryset(queryset) - queryset = self.annotate_is_favorite(queryset) - queryset = self.annotate_user_roles(queryset) + + filterset = DocumentFilter(request.GET, queryset=queryset) + if not filterset.is_valid(): + raise drf.exceptions.ValidationError(filterset.errors) + + queryset = filterset.qs + + return self.get_response_for_queryset(queryset) + + @drf.decorators.action( + detail=True, + methods=["get"], + ordering=["path"], + ) + def descendants(self, request, *args, **kwargs): + """Handle listing descendants of a document""" + document = self.get_object() + + queryset = document.get_descendants().filter(ancestors_deleted_at__isnull=True) + queryset = self.filter_queryset(queryset) + + filterset = DocumentFilter(request.GET, queryset=queryset) + if not filterset.is_valid(): + raise drf.exceptions.ValidationError(filterset.errors) + + queryset = filterset.qs + return self.get_response_for_queryset(queryset) @drf.decorators.action( diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 159e954b..6077c0de 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -809,17 +809,18 @@ class Document(MP_Node, BaseModel): Soft delete the document, marking the deletion on descendants. We still keep the .delete() method untouched for programmatic purposes. """ - if self.deleted_at or self.ancestors_deleted_at: + if ( + self._meta.model.objects.filter( + models.Q(deleted_at__isnull=False) + | models.Q(ancestors_deleted_at__isnull=False), + pk=self.pk, + ).exists() + or self.get_ancestors().filter(deleted_at__isnull=False).exists() + ): raise RuntimeError( "This document is already deleted or has deleted ancestors." ) - # Check if any ancestors are deleted - if self.get_ancestors().filter(deleted_at__isnull=False).exists(): - raise RuntimeError( - "Cannot delete this document because one or more ancestors are already deleted." - ) - self.ancestors_deleted_at = self.deleted_at = timezone.now() self.save() @@ -836,14 +837,8 @@ class Document(MP_Node, BaseModel): raise ValidationError({"deleted_at": [_("This document is not deleted.")]}) if self.deleted_at < get_trashbin_cutoff(): - raise ValidationError( - { - "deleted_at": [ - _( - "This document was permanently deleted and cannot be restored." - ) - ] - } + raise RuntimeError( + "This document was permanently deleted and cannot be restored." ) # save the current deleted_at value to exclude it from the descendants update 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 6cfb3764..3a3c3ff9 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 @@ -1,5 +1,5 @@ """ -Tests for Documents API endpoint in impress's core app: create +Tests for Documents API endpoint in impress's core app: children create """ from uuid import uuid4 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 43fa6f4f..0ea49dc8 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 @@ -1,5 +1,5 @@ """ -Tests for Documents API endpoint in impress's core app: retrieve +Tests for Documents API endpoint in impress's core app: children list """ import random