diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index fa1213ce..82044fe2 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -132,7 +132,8 @@ class ListDocumentSerializer(serializers.ModelSerializer): """Serialize documents with limited fields for display in lists.""" is_favorite = serializers.BooleanField(read_only=True) - nb_accesses = serializers.IntegerField(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) abilities = serializers.SerializerMethodField(read_only=True) @@ -148,7 +149,8 @@ class ListDocumentSerializer(serializers.ModelSerializer): "is_favorite", "link_role", "link_reach", - "nb_accesses", + "nb_accesses_ancestors", + "nb_accesses_direct", "numchild", "path", "title", @@ -165,7 +167,8 @@ class ListDocumentSerializer(serializers.ModelSerializer): "is_favorite", "link_role", "link_reach", - "nb_accesses", + "nb_accesses_ancestors", + "nb_accesses_direct", "numchild", "path", "updated_at", @@ -217,7 +220,8 @@ class DocumentSerializer(ListDocumentSerializer): "is_favorite", "link_role", "link_reach", - "nb_accesses", + "nb_accesses_ancestors", + "nb_accesses_direct", "numchild", "path", "title", @@ -233,7 +237,8 @@ class DocumentSerializer(ListDocumentSerializer): "is_favorite", "link_role", "link_reach", - "nb_accesses", + "nb_accesses_ancestors", + "nb_accesses_direct", "numchild", "path", "updated_at", diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 1874f565..ba976a82 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -654,20 +654,39 @@ class Document(MP_Node, BaseModel): """Generate a unique cache key for each document.""" return f"document_{self.id!s}_nb_accesses" - @property - def nb_accesses(self): - """Calculate the number of accesses.""" + def get_nb_accesses(self): + """ + Calculate the number of accesses: + - directly attached to the document + - attached to any of the document's ancestors + """ cache_key = self.get_nb_accesses_cache_key() nb_accesses = cache.get(cache_key) if nb_accesses is None: - nb_accesses = DocumentAccess.objects.filter( - document__path=Left(models.Value(self.path), Length("document__path")), - ).count() + nb_accesses = ( + DocumentAccess.objects.filter(document=self).count(), + DocumentAccess.objects.filter( + document__path=Left( + models.Value(self.path), Length("document__path") + ), + document__ancestors_deleted_at__isnull=True, + ).count(), + ) cache.set(cache_key, nb_accesses) return nb_accesses + @property + def nb_accesses_direct(self): + """Returns the number of accesses related to the document or one of its ancestors.""" + return self.get_nb_accesses()[0] + + @property + def nb_accesses_ancestors(self): + """Returns the number of accesses related to the document or one of its ancestors.""" + return self.get_nb_accesses()[1] + def invalidate_nb_accesses_cache(self): """ Invalidate the cache for number of accesses, including on affected descendants. @@ -855,6 +874,11 @@ class Document(MP_Node, BaseModel): self.send_email(subject, [email], context, language) + def delete(self, *args, **kwargs): + """Invalidate cache for number of accesses when deleting a document.""" + super().delete(*args, **kwargs) + self.invalidate_nb_accesses_cache() + @transaction.atomic def soft_delete(self): """ @@ -875,6 +899,7 @@ class Document(MP_Node, BaseModel): self.ancestors_deleted_at = self.deleted_at = timezone.now() self.save() + self.invalidate_nb_accesses_cache() # Mark all descendants as soft deleted self.get_descendants().filter(ancestors_deleted_at__isnull=True).update( @@ -909,6 +934,7 @@ class Document(MP_Node, BaseModel): ) self.ancestors_deleted_at = ancestors_deleted_at self.save(update_fields=["deleted_at", "ancestors_deleted_at"]) + self.invalidate_nb_accesses_cache() self.get_descendants().exclude( models.Q(deleted_at__isnull=False) 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 0ea49dc8..96e1d9b4 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 @@ -39,7 +39,8 @@ def test_api_documents_children_list_anonymous_public_standalone(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -56,7 +57,8 @@ def test_api_documents_children_list_anonymous_public_standalone(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -100,7 +102,8 @@ def test_api_documents_children_list_anonymous_public_parent(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -117,7 +120,8 @@ def test_api_documents_children_list_anonymous_public_parent(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -179,7 +183,8 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -196,7 +201,8 @@ def test_api_documents_children_list_authenticated_unrelated_public_or_authentic "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -244,7 +250,8 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -261,7 +268,8 @@ def test_api_documents_children_list_authenticated_public_or_authenticated_paren "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -331,7 +339,8 @@ def test_api_documents_children_list_authenticated_related_direct(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 3, + "nb_accesses_ancestors": 3, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -348,7 +357,8 @@ def test_api_documents_children_list_authenticated_related_direct(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -399,7 +409,8 @@ def test_api_documents_children_list_authenticated_related_parent(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -416,7 +427,8 @@ def test_api_documents_children_list_authenticated_related_parent(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -514,7 +526,8 @@ def test_api_documents_children_list_authenticated_related_team_members( "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -531,7 +544,8 @@ def test_api_documents_children_list_authenticated_related_team_members( "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), 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 719db378..302af231 100644 --- a/src/backend/core/tests/documents/test_api_documents_descendants.py +++ b/src/backend/core/tests/documents/test_api_documents_descendants.py @@ -41,7 +41,8 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -58,7 +59,8 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -75,7 +77,8 @@ def test_api_documents_descendants_list_anonymous_public_standalone(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -121,7 +124,8 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -138,7 +142,8 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -155,7 +160,8 @@ def test_api_documents_descendants_list_anonymous_public_parent(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -220,7 +226,8 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -237,7 +244,8 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -254,7 +262,8 @@ def test_api_documents_descendants_list_authenticated_unrelated_public_or_authen "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -304,7 +313,8 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -321,7 +331,8 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -338,7 +349,8 @@ def test_api_documents_descendants_list_authenticated_public_or_authenticated_pa "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -411,7 +423,8 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 3, + "nb_accesses_ancestors": 3, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -428,7 +441,8 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 3, + "nb_accesses_ancestors": 3, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -445,7 +459,8 @@ def test_api_documents_descendants_list_authenticated_related_direct(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -498,7 +513,8 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 1, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -515,7 +531,8 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -532,7 +549,8 @@ def test_api_documents_descendants_list_authenticated_related_parent(): "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), @@ -602,7 +620,6 @@ def test_api_documents_descendants_list_authenticated_related_team_members( mock_user_teams.return_value = ["myteam"] user = factories.UserFactory() - client = APIClient() client.force_login(user) @@ -632,7 +649,8 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "link_reach": child1.link_reach, "link_role": child1.link_role, "numchild": 1, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child1.path, "title": child1.title, "updated_at": child1.updated_at.isoformat().replace("+00:00", "Z"), @@ -649,7 +667,8 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "link_reach": grand_child.link_reach, "link_role": grand_child.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": grand_child.path, "title": grand_child.title, "updated_at": grand_child.updated_at.isoformat().replace("+00:00", "Z"), @@ -666,7 +685,8 @@ def test_api_documents_descendants_list_authenticated_related_team_members( "link_reach": child2.link_reach, "link_role": child2.link_role, "numchild": 0, - "nb_accesses": 1, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 0, "path": child2.path, "title": child2.title, "updated_at": child2.updated_at.isoformat().replace("+00:00", "Z"), 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 46baa8e4..8791a6bf 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 @@ -12,7 +12,7 @@ def test_api_document_favorite_list_anonymous(): """Anonymous users should receive a 401 error.""" client = APIClient() - response = client.get(f"/api/v1.0/documents/favorite_list/") + response = client.get("/api/v1.0/documents/favorite_list/") assert response.status_code == 401 @@ -23,7 +23,7 @@ def test_api_document_favorite_list_authenticated_no_favorite(): client = APIClient() client.force_login(user) - response = client.get(f"/api/v1.0/documents/favorite_list/") + response = client.get("/api/v1.0/documents/favorite_list/") assert response.status_code == 200 assert response.json() == { @@ -65,9 +65,11 @@ def test_api_document_favorite_list_authenticated_with_favorite(): "depth": document.depth, "excerpt": document.excerpt, "id": str(document.id), + "is_favorite": True, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": document.nb_accesses, + "nb_accesses_ancestors": 1, + "nb_accesses_direct": 1, "numchild": document.numchild, "path": document.path, "title": document.title, 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 4aa5c6c1..1120123e 100644 --- a/src/backend/core/tests/documents/test_api_documents_list.py +++ b/src/backend/core/tests/documents/test_api_documents_list.py @@ -70,7 +70,8 @@ def test_api_documents_list_format(): "is_favorite": True, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": 3, + "nb_accesses_ancestors": 3, + "nb_accesses_direct": 3, "numchild": 0, "path": document.path, "title": document.title, @@ -147,7 +148,7 @@ def test_api_documents_list_authenticated_direct(django_assert_num_queries): str(child4_with_access.id), } - with django_assert_num_queries(8): + with django_assert_num_queries(12): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached @@ -185,7 +186,7 @@ def test_api_documents_list_authenticated_via_team( expected_ids = {str(document.id) for document in documents_team1 + documents_team2} - with django_assert_num_queries(9): + with django_assert_num_queries(14): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached @@ -218,7 +219,7 @@ def test_api_documents_list_authenticated_link_reach_restricted( other_document = factories.DocumentFactory(link_reach="public") models.LinkTrace.objects.create(document=other_document, user=user) - with django_assert_num_queries(5): + with django_assert_num_queries(6): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached @@ -267,7 +268,7 @@ 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(7): + with django_assert_num_queries(10): response = client.get("/api/v1.0/documents/") # nb_accesses should now be cached @@ -391,7 +392,7 @@ def test_api_documents_list_favorites_no_extra_queries(django_assert_num_queries factories.DocumentFactory.create_batch(2, users=[user]) url = "/api/v1.0/documents/" - with django_assert_num_queries(9): + with django_assert_num_queries(14): response = client.get(url) # nb_accesses should now be cached 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 e88ad035..8b587f06 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -64,7 +64,8 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "is_favorite": False, "link_reach": "public", "link_role": document.link_role, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "numchild": 0, "path": document.path, "title": document.title, @@ -124,7 +125,8 @@ def test_api_documents_retrieve_anonymous_public_parent(): "is_favorite": False, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "numchild": 0, "path": document.path, "title": document.title, @@ -220,7 +222,8 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "is_favorite": False, "link_reach": reach, "link_role": document.link_role, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "numchild": 0, "path": document.path, "title": document.title, @@ -287,7 +290,8 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "is_favorite": False, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": 0, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 0, "numchild": 0, "path": document.path, "title": document.title, @@ -396,7 +400,8 @@ def test_api_documents_retrieve_authenticated_related_direct(): "is_favorite": False, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 2, "numchild": 0, "path": document.path, "title": document.title, @@ -463,7 +468,8 @@ def test_api_documents_retrieve_authenticated_related_parent(): "is_favorite": False, "link_reach": "restricted", "link_role": document.link_role, - "nb_accesses": 2, + "nb_accesses_ancestors": 2, + "nb_accesses_direct": 0, "numchild": 0, "path": document.path, "title": document.title, @@ -491,7 +497,8 @@ def test_api_documents_retrieve_authenticated_related_nb_accesses(): f"/api/v1.0/documents/{document.id!s}/", ) assert response.status_code == 200 - assert response.json()["nb_accesses"] == 3 + assert response.json()["nb_accesses_ancestors"] == 3 + assert response.json()["nb_accesses_direct"] == 1 factories.UserDocumentAccessFactory(document=grand_parent) @@ -499,7 +506,8 @@ def test_api_documents_retrieve_authenticated_related_nb_accesses(): f"/api/v1.0/documents/{document.id!s}/", ) assert response.status_code == 200 - assert response.json()["nb_accesses"] == 4 + assert response.json()["nb_accesses_ancestors"] == 4 + assert response.json()["nb_accesses_direct"] == 1 def test_api_documents_retrieve_authenticated_related_child(): @@ -580,12 +588,10 @@ def test_api_documents_retrieve_authenticated_related_team_members( mock_user_teams.return_value = teams user = factories.UserFactory() - client = APIClient() client.force_login(user) document = factories.DocumentFactory(link_reach="restricted") - factories.TeamDocumentAccessFactory( document=document, team="readers", role="reader" ) @@ -614,7 +620,8 @@ def test_api_documents_retrieve_authenticated_related_team_members( "is_favorite": False, "link_reach": "restricted", "link_role": document.link_role, - "nb_accesses": 5, + "nb_accesses_ancestors": 5, + "nb_accesses_direct": 5, "numchild": 0, "path": document.path, "title": document.title, @@ -675,7 +682,8 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( "is_favorite": False, "link_reach": "restricted", "link_role": document.link_role, - "nb_accesses": 5, + "nb_accesses_ancestors": 5, + "nb_accesses_direct": 5, "numchild": 0, "path": document.path, "title": document.title, @@ -736,7 +744,8 @@ def test_api_documents_retrieve_authenticated_related_team_owners( "is_favorite": False, "link_reach": "restricted", "link_role": document.link_role, - "nb_accesses": 5, + "nb_accesses_ancestors": 5, + "nb_accesses_direct": 5, "numchild": 0, "path": document.path, "title": document.title, @@ -770,7 +779,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(11): + with django_assert_max_num_queries(12): response = client.get(f"/api/v1.0/documents/{document.id!s}/") assert response.status_code == 200 @@ -787,7 +796,7 @@ def test_api_documents_retrieve_numqueries_with_link_trace(django_assert_num_que document = factories.DocumentFactory(users=[user], link_traces=[user]) - with django_assert_num_queries(4): + with django_assert_num_queries(5): response = client.get(f"/api/v1.0/documents/{document.id!s}/") with django_assert_num_queries(3): 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 b2fcd4b6..6e78f17b 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -105,7 +105,8 @@ def test_api_documents_trashbin_format(): "excerpt": document.excerpt, "link_reach": document.link_reach, "link_role": document.link_role, - "nb_accesses": 3, + "nb_accesses_ancestors": 0, + "nb_accesses_direct": 3, "numchild": 0, "path": document.path, "title": document.title, @@ -154,7 +155,7 @@ def test_api_documents_trashbin_authenticated_direct(django_assert_num_queries): expected_ids = {str(document1.id), str(document2.id), str(document3.id)} - with django_assert_num_queries(7): + with django_assert_num_queries(10): response = client.get("/api/v1.0/documents/trashbin/") with django_assert_num_queries(4): @@ -196,7 +197,7 @@ def test_api_documents_trashbin_authenticated_via_team( expected_ids = {str(deleted_document_team1.id), str(deleted_document_team2.id)} - with django_assert_num_queries(5): + with django_assert_num_queries(7): response = client.get("/api/v1.0/documents/trashbin/") with django_assert_num_queries(3): 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 321912be..33fa614b 100644 --- a/src/backend/core/tests/documents/test_api_documents_tree.py +++ b/src/backend/core/tests/documents/test_api_documents_tree.py @@ -23,7 +23,7 @@ def test_api_documents_tree_list_anonymous_public_standalone(django_assert_num_q ) child = factories.DocumentFactory(link_reach="public", parent=document) - with django_assert_num_queries(9): + with django_assert_num_queries(14): APIClient().get(f"/api/v1.0/documents/{document.id!s}/tree/") with django_assert_num_queries(4): @@ -332,7 +332,7 @@ def test_api_documents_tree_list_authenticated_unrelated_public_or_authenticated document, sibling = factories.DocumentFactory.create_batch(2, parent=parent) child = factories.DocumentFactory(link_reach="public", parent=document) - with django_assert_num_queries(9): + with django_assert_num_queries(13): client.get(f"/api/v1.0/documents/{document.id!s}/tree/") with django_assert_num_queries(5): diff --git a/src/backend/core/tests/documents/test_api_documents_update.py b/src/backend/core/tests/documents/test_api_documents_update.py index 8eaba224..fefa0ae1 100644 --- a/src/backend/core/tests/documents/test_api_documents_update.py +++ b/src/backend/core/tests/documents/test_api_documents_update.py @@ -275,7 +275,8 @@ def test_api_documents_update_authenticated_editor_administrator_or_owner( "depth", "link_reach", "link_role", - "nb_accesses", + "nb_accesses_ancestors", + "nb_accesses_direct", "numchild", "path", ]: diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index ae529a27..9562d02f 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -1,6 +1,7 @@ """ Unit tests for the Document model """ +# pylint: disable=too-many-lines import random import smtplib @@ -819,40 +820,89 @@ def test_models_documents__email_invitation__failed(mock_logger, _mock_send_mail # Document number of accesses -def test_models_documents_nb_accesses_cache_is_set_and_retrieved( +def test_models_documents_nb_accesses_cache_is_set_and_retrieved_ancestors( django_assert_num_queries, ): - """Test that nb_accesses is cached after the first computation.""" - document = factories.DocumentFactory() + """Test that nb_accesses is cached when calling nb_accesses_ancestors.""" + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) key = f"document_{document.id!s}_nb_accesses" - nb_accesses = random.randint(1, 4) - factories.UserDocumentAccessFactory.create_batch(nb_accesses, document=document) + nb_accesses_parent = random.randint(1, 4) + factories.UserDocumentAccessFactory.create_batch( + nb_accesses_parent, document=parent + ) + nb_accesses_direct = random.randint(1, 4) + factories.UserDocumentAccessFactory.create_batch( + nb_accesses_direct, document=document + ) factories.UserDocumentAccessFactory() # An unrelated access should not be counted # Initially, the nb_accesses should not be cached assert cache.get(key) is None # Compute the nb_accesses for the first time (this should set the cache) - with django_assert_num_queries(1): - assert document.nb_accesses == nb_accesses + nb_accesses_ancestors = nb_accesses_parent + nb_accesses_direct + with django_assert_num_queries(2): + assert document.nb_accesses_ancestors == nb_accesses_ancestors # Ensure that the nb_accesses is now cached with django_assert_num_queries(0): - assert document.nb_accesses == nb_accesses - assert cache.get(key) == nb_accesses + assert document.nb_accesses_ancestors == nb_accesses_ancestors + assert cache.get(key) == (nb_accesses_direct, nb_accesses_ancestors) # The cache value should be invalidated when a document access is created models.DocumentAccess.objects.create( document=document, user=factories.UserFactory(), role="reader" ) assert cache.get(key) is None # Cache should be invalidated - with django_assert_num_queries(1): - new_nb_accesses = document.nb_accesses - assert new_nb_accesses == nb_accesses + 1 - assert cache.get(key) == new_nb_accesses # Cache should now contain the new value + with django_assert_num_queries(2): + assert document.nb_accesses_ancestors == nb_accesses_ancestors + 1 + assert cache.get(key) == (nb_accesses_direct + 1, nb_accesses_ancestors + 1) +def test_models_documents_nb_accesses_cache_is_set_and_retrieved_direct( + django_assert_num_queries, +): + """Test that nb_accesses is cached when calling nb_accesses_direct.""" + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + key = f"document_{document.id!s}_nb_accesses" + nb_accesses_parent = random.randint(1, 4) + factories.UserDocumentAccessFactory.create_batch( + nb_accesses_parent, document=parent + ) + nb_accesses_direct = random.randint(1, 4) + factories.UserDocumentAccessFactory.create_batch( + nb_accesses_direct, document=document + ) + factories.UserDocumentAccessFactory() # An unrelated access should not be counted + + # Initially, the nb_accesses should not be cached + assert cache.get(key) is None + + # Compute the nb_accesses for the first time (this should set the cache) + nb_accesses_ancestors = nb_accesses_parent + nb_accesses_direct + with django_assert_num_queries(2): + assert document.nb_accesses_direct == nb_accesses_direct + + # Ensure that the nb_accesses is now cached + with django_assert_num_queries(0): + assert document.nb_accesses_direct == nb_accesses_direct + assert cache.get(key) == (nb_accesses_direct, nb_accesses_ancestors) + + # The cache value should be invalidated when a document access is created + models.DocumentAccess.objects.create( + document=document, user=factories.UserFactory(), role="reader" + ) + assert cache.get(key) is None # Cache should be invalidated + with django_assert_num_queries(2): + assert document.nb_accesses_direct == nb_accesses_direct + 1 + assert cache.get(key) == (nb_accesses_direct + 1, nb_accesses_ancestors + 1) + + +@pytest.mark.parametrize("field", ["nb_accesses_ancestors", "nb_accesses_direct"]) def test_models_documents_nb_accesses_cache_is_invalidated_on_access_removal( + field, django_assert_num_queries, ): """Test that the cache is invalidated when a document access is deleted.""" @@ -861,18 +911,76 @@ def test_models_documents_nb_accesses_cache_is_invalidated_on_access_removal( access = factories.UserDocumentAccessFactory(document=document) # Initially, the nb_accesses should be cached - assert document.nb_accesses == 1 - assert cache.get(key) == 1 + assert getattr(document, field) == 1 + assert cache.get(key) == (1, 1) # Remove the access and check if cache is invalidated access.delete() assert cache.get(key) is None # Cache should be invalidated # Recompute the nb_accesses (this should trigger a cache set) - with django_assert_num_queries(1): - new_nb_accesses = document.nb_accesses + with django_assert_num_queries(2): + new_nb_accesses = getattr(document, field) assert new_nb_accesses == 0 - assert cache.get(key) == 0 # Cache should now contain the new value + assert cache.get(key) == (0, 0) # Cache should now contain the new value + + +@pytest.mark.parametrize("field", ["nb_accesses_ancestors", "nb_accesses_direct"]) +def test_models_documents_nb_accesses_cache_is_invalidated_on_document_soft_delete_restore( + field, + django_assert_num_queries, +): + """Test that the cache is invalidated when a document access is deleted.""" + document = factories.DocumentFactory() + key = f"document_{document.id!s}_nb_accesses" + factories.UserDocumentAccessFactory(document=document) + + # Initially, the nb_accesses should be cached + assert getattr(document, field) == 1 + assert cache.get(key) == (1, 1) + + # Soft delete the document and check if cache is invalidated + document.soft_delete() + assert cache.get(key) is None # Cache should be invalidated + + # Recompute the nb_accesses (this should trigger a cache set) + with django_assert_num_queries(2): + new_nb_accesses = getattr(document, field) + assert new_nb_accesses == (1 if field == "nb_accesses_direct" else 0) + assert cache.get(key) == (1, 0) # Cache should now contain the new value + + document.restore() + + # Recompute the nb_accesses (this should trigger a cache set) + with django_assert_num_queries(2): + new_nb_accesses = getattr(document, field) + assert new_nb_accesses == 1 + assert cache.get(key) == (1, 1) # Cache should now contain the new value + + +def test_models_documents_nb_accesses_cache_is_invalidated_on_document_delete( + django_assert_num_queries, +): + """Test that the cache is invalidated when a document is deleted.""" + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + key = f"document_{document.id!s}_nb_accesses" + factories.UserDocumentAccessFactory(document=parent) + + # Initially, the nb_accesses should be cached + assert document.nb_accesses_ancestors == 1 + assert document.nb_accesses_direct == 0 + assert cache.get(key) == (0, 1) + + # Delete the parent and check if cache is invalidated + parent.delete() + assert cache.get(key) is None # Cache should be invalidated + + # Recompute the nb_accesses (this should trigger a cache set) + with django_assert_num_queries(2): + assert document.nb_accesses_ancestors == 0 + assert document.nb_accesses_direct == 0 + assert cache.get(key) == (0, 0) # Cache should now contain the new value def test_models_documents_restore(django_assert_num_queries):