From a1914c62592a84e9ef861de0ad4a0da976a3e991 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Mon, 24 Mar 2025 10:46:40 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(backend)=20compute=20ancestor=5Fli?= =?UTF-8?q?nks=20in=20get=5Fabilities=20if=20needed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refactor made in the tree view caching the ancestors_links to not compute them again in the document.get_abilities method lead to a bug. If the get_abilities method is called without ancestors_links, then they are computed on all the ancestors but not from the highest readable ancestor for the current user. We have to compute them with this constraint. --- CHANGELOG.md | 4 ++ src/backend/core/api/viewsets.py | 16 ++++++- src/backend/core/models.py | 28 +++++++++++- .../documents/test_api_documents_retrieve.py | 2 +- .../core/tests/test_models_documents.py | 44 +++++++++++++++++++ 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c979a118..ed4f587b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to - ♻️(frontend) Integrate UI kit #783 +## Fixed + +- 🐛(backend) compute ancestor_links in get_abilities if needed #725 + ## [2.6.0] - 2025-03-21 ## Added diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 8544aed9..71b030d3 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -846,14 +846,15 @@ class DocumentViewSet( ) # Get the highest readable ancestor - highest_readable = ancestors.readable_per_se(request.user).only("depth").first() + highest_readable = ( + ancestors.readable_per_se(request.user).only("depth", "path").first() + ) if highest_readable is None: raise ( drf.exceptions.PermissionDenied() if request.user.is_authenticated else drf.exceptions.NotAuthenticated() ) - paths_links_mapping = {} ancestors_links = [] children_clause = db.Q() @@ -876,6 +877,17 @@ class DocumentViewSet( 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 = self.annotate_user_roles(queryset) queryset = self.annotate_is_favorite(queryset) diff --git a/src/backend/core/models.py b/src/backend/core/models.py index ef276d02..e66bde1b 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -754,6 +754,32 @@ class Document(MP_Node, BaseModel): return dict(links_definitions) # Convert defaultdict back to a normal dict + def compute_ancestors_links(self, user): + """ + Compute the ancestors links for the current document up to the highest readable ancestor. + """ + ancestors = ( + (self.get_ancestors() | self._meta.model.objects.filter(pk=self.pk)) + .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): + 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 ancestors_links + def get_abilities(self, user, ancestors_links=None): """ Compute and return abilities for a given user on the document. @@ -761,7 +787,7 @@ class Document(MP_Node, BaseModel): if self.depth <= 1 or getattr(self, "is_highest_ancestor_for_user", False): ancestors_links = [] elif ancestors_links is None: - ancestors_links = self.get_ancestors().values("link_reach", "link_role") + ancestors_links = self.compute_ancestors_links(user=user) roles = set( self.get_roles(user) 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 1148d7c1..38d66cd4 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -789,7 +789,7 @@ def test_api_documents_retrieve_user_roles(django_assert_max_num_queries): ) expected_roles = {access.role for access in accesses} - with django_assert_max_num_queries(12): + with django_assert_max_num_queries(14): response = client.get(f"/api/v1.0/documents/{document.id!s}/") assert response.status_code == 200 diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index e1037815..3f2b8d6e 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -1305,3 +1305,47 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): def test_models_documents_get_select_options(ancestors_links, select_options): """Validate that the "get_select_options" method operates as expected.""" assert models.LinkReachChoices.get_select_options(ancestors_links) == 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( + django_assert_num_queries, +): + """Test the compute_ancestors_links method.""" + 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( + parent=root, + link_reach="authenticated", + link_role="editor", + users=[user, other_user], + ) + child3 = factories.DocumentFactory( + parent=child2, + 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}, + ] + + with django_assert_num_queries(2): + assert child3.compute_ancestors_links(user=other_user) == [ + {"link_reach": child2.link_reach, "link_role": child2.link_role}, + ]