From 48662ceecb98babce7f5ae4286c08a0bdf840025 Mon Sep 17 00:00:00 2001
From: Samuel Paccoud - DINUM <samuel.paccoud@mail.numerique.gouv.fr>
Date: Tue, 17 Dec 2024 07:35:09 +0100
Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20list=20only=20the=20first?=
 =?UTF-8?q?=20visible=20parent=20document=20for=20a=20user?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now that we have a tree structure, we should only include parents of
a visible subtree in list results.
---
 src/backend/core/api/viewsets.py              | 20 +++++++++
 .../documents/test_api_documents_list.py      | 43 ++++++++++++++++---
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py
index b5359af5..052e2321 100644
--- a/src/backend/core/api/viewsets.py
+++ b/src/backend/core/api/viewsets.py
@@ -15,12 +15,14 @@ from django.db import models as db
 from django.db.models import (
     Count,
     Exists,
+    F,
     OuterRef,
     Q,
     Subquery,
     Value,
 )
 from django.db.models.expressions import RawSQL
+from django.db.models.functions import Left, Length
 from django.http import Http404
 
 import rest_framework as drf
@@ -395,6 +397,24 @@ class DocumentViewSet(
                     & ~db.Q(link_reach=models.LinkReachChoices.RESTRICTED)
                 )
             )
+
+            # Among the results, we may have documents that are ancestors/children of each other
+            # In this case we want to keep only the highest ancestor. Let's annotate, each document
+            # with the path of its highest ancestor within results so we can use it to filter
+            shortest_path = Subquery(
+                queryset.filter(path=Left(OuterRef("path"), Length("path")))
+                .order_by("path")  # Get the shortest (root) path
+                .values("path")[:1]
+            )
+            queryset = queryset.annotate(root_path=shortest_path)
+
+            # Filter documents based on their shortest path (root path)
+            queryset = queryset.filter(
+                root_path=F(
+                    "path"
+                )  # Keep only documents who are the annotated highest ancestor
+            )
+
         else:
             queryset = queryset.none()
 
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 727ae266..93e35237 100644
--- a/src/backend/core/tests/documents/test_api_documents_list.py
+++ b/src/backend/core/tests/documents/test_api_documents_list.py
@@ -2,6 +2,7 @@
 Tests for Documents API endpoint in impress's core app: list
 """
 
+import random
 from unittest import mock
 
 import pytest
@@ -82,7 +83,7 @@ def test_api_documents_list_authenticated_direct(django_assert_num_queries):
     client = APIClient()
     client.force_login(user)
 
-    documents = [
+    document1, document2 = [
         access.document
         for access in factories.UserDocumentAccessFactory.create_batch(2, user=user)
     ]
@@ -92,14 +93,29 @@ def test_api_documents_list_authenticated_direct(django_assert_num_queries):
         for role in models.LinkRoleChoices:
             factories.DocumentFactory(link_reach=reach, link_role=role)
 
-    expected_ids = {str(document.id) for document in documents}
+    # Children of visible documents should not get listed even with a specific access
+    factories.DocumentFactory(parent=document1)
+
+    child1_with_access = factories.DocumentFactory(parent=document1)
+    factories.UserDocumentAccessFactory(user=user, document=child1_with_access)
+
+    middle_document = factories.DocumentFactory(parent=document2)
+    child2_with_access = factories.DocumentFactory(parent=middle_document)
+    factories.UserDocumentAccessFactory(user=user, document=child2_with_access)
+
+    # Children of hidden documents should get listed when visible by the logged-in user
+    hidden_root = factories.DocumentFactory()
+    child3_with_access = factories.DocumentFactory(parent=hidden_root)
+    factories.UserDocumentAccessFactory(user=user, document=child3_with_access)
+
+    expected_ids = {str(document1.id), str(document2.id), str(child3_with_access.id)}
 
     with django_assert_num_queries(3):
         response = client.get("/api/v1.0/documents/")
 
     assert response.status_code == 200
     results = response.json()["results"]
-    assert len(results) == 2
+    assert len(results) == 3
     results_id = {result["id"] for result in results}
     assert expected_ids == results_id
 
@@ -183,12 +199,27 @@ def test_api_documents_list_authenticated_link_reach_public_or_authenticated(
     client = APIClient()
     client.force_login(user)
 
-    documents = [
+    document1, document2 = [
         factories.DocumentFactory(link_traces=[user], link_reach=reach)
         for reach in models.LinkReachChoices
         if reach != "restricted"
     ]
-    expected_ids = {str(document.id) for document in documents}
+    factories.DocumentFactory(
+        link_reach=random.choice(["public", "authenticated"]),
+        link_traces=[user],
+        parent=document1,
+    )
+
+    hidden_document = factories.DocumentFactory(
+        link_reach=random.choice(["public", "authenticated"])
+    )
+    visible_child = factories.DocumentFactory(
+        link_traces=[user],
+        link_reach=random.choice(["public", "authenticated"]),
+        parent=hidden_document,
+    )
+
+    expected_ids = {str(document1.id), str(document2.id), str(visible_child.id)}
 
     with django_assert_num_queries(3):
         response = client.get(
@@ -197,7 +228,7 @@ def test_api_documents_list_authenticated_link_reach_public_or_authenticated(
 
     assert response.status_code == 200
     results = response.json()["results"]
-    assert len(results) == 2
+    assert len(results) == 3
     results_id = {result["id"] for result in results}
     assert expected_ids == results_id