🐛(backend) fix numchild when soft deleting/restoring a document

The numchild attribute must be incremented/decremented manually
when we soft delete a document if we want it to remain accurate,
which is important to display the tree structure in the frontend.

src/backend/core/models.py

@@ -874,11 +874,6 @@ 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):
         """
@@ -901,6 +896,11 @@ class Document(MP_Node, BaseModel):
         self.save()
         self.invalidate_nb_accesses_cache()
 
+        if self.depth > 1:
+            self._meta.model.objects.filter(pk=self.get_parent().pk).update(
+                numchild=models.F("numchild") - 1
+            )
+
         # Mark all descendants as soft deleted
         self.get_descendants().filter(ancestors_deleted_at__isnull=True).update(
             ancestors_deleted_at=self.ancestors_deleted_at
@@ -910,8 +910,10 @@ class Document(MP_Node, BaseModel):
     def restore(self):
         """Cancelling a soft delete with checks."""
         # This should not happen
-        if self.deleted_at is None:
-            raise ValidationError({"deleted_at": [_("This document is not deleted.")]})
+        if self._meta.model.objects.filter(
+            pk=self.pk, deleted_at__isnull=True
+        ).exists():
+            raise RuntimeError("This document is not deleted.")
 
         if self.deleted_at < get_trashbin_cutoff():
             raise RuntimeError(
@@ -941,6 +943,11 @@ class Document(MP_Node, BaseModel):
             | models.Q(ancestors_deleted_at__lt=current_deleted_at)
         ).update(ancestors_deleted_at=self.ancestors_deleted_at)
 
+        if self.depth > 1:
+            self._meta.model.objects.filter(pk=self.get_parent().pk).update(
+                numchild=models.F("numchild") + 1
+            )
+
 
 class LinkTrace(BaseModel):
     """

src/backend/core/tests/test_models_documents.py

@@ -958,29 +958,78 @@ def test_models_documents_nb_accesses_cache_is_invalidated_on_document_soft_dele
     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)
+def test_models_documents_numchild_deleted_from_instance():
+    """the "numchild" field should not include documents deleted from the instance."""
+    document = factories.DocumentFactory()
+    child1, _child2 = factories.DocumentFactory.create_batch(2, parent=document)
+    assert document.numchild == 2
 
-    # 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)
+    child1.delete()
 
-    # Delete the parent and check if cache is invalidated
-    parent.delete()
-    assert cache.get(key) is None  # Cache should be invalidated
+    document.refresh_from_db()
+    assert document.numchild == 1
 
-    # 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_numchild_deleted_from_queryset():
+    """the "numchild" field should not include documents deleted from a queryset."""
+    document = factories.DocumentFactory()
+    child1, _child2 = factories.DocumentFactory.create_batch(2, parent=document)
+    assert document.numchild == 2
+
+    models.Document.objects.filter(pk=child1.pk).delete()
+
+    document.refresh_from_db()
+    assert document.numchild == 1
+
+
+def test_models_documents_numchild_soft_deleted_and_restore():
+    """the "numchild" field should not include soft deleted documents."""
+    document = factories.DocumentFactory()
+    child1, _child2 = factories.DocumentFactory.create_batch(2, parent=document)
+
+    assert document.numchild == 2
+
+    child1.soft_delete()
+
+    document.refresh_from_db()
+    assert document.numchild == 1
+
+    child1.restore()
+
+    document.refresh_from_db()
+    assert document.numchild == 2
+
+
+def test_models_documents_soft_delete_tempering_with_instance():
+    """
+    Soft deleting should fail if the document is already deleted in database even though the
+    instance "deleted_at" attributes where tempered with.
+    """
+    document = factories.DocumentFactory()
+    document.soft_delete()
+
+    document.deleted_at = None
+    document.ancestors_deleted_at = None
+    with pytest.raises(
+        RuntimeError, match="This document is already deleted or has deleted ancestors."
+    ):
+        document.soft_delete()
+
+
+def test_models_documents_restore_tempering_with_instance():
+    """
+    Soft deleting should fail if the document is already deleted in database even though the
+    instance "deleted_at" attributes where tempered with.
+    """
+    document = factories.DocumentFactory()
+
+    if random.choice([False, True]):
+        document.deleted_at = timezone.now()
+    else:
+        document.ancestors_deleted_at = timezone.now()
+
+    with pytest.raises(RuntimeError, match="This document is not deleted."):
+        document.restore()
 
 
 def test_models_documents_restore(django_assert_num_queries):
@@ -991,7 +1040,7 @@ def test_models_documents_restore(django_assert_num_queries):
     assert document.deleted_at is not None
     assert document.ancestors_deleted_at == document.deleted_at
 
-    with django_assert_num_queries(6):
+    with django_assert_num_queries(8):
         document.restore()
     document.refresh_from_db()
     assert document.deleted_at is None
@@ -1034,7 +1083,7 @@ def test_models_documents_restore_complex(django_assert_num_queries):
     assert child2.ancestors_deleted_at == document.deleted_at
 
     # Restore the item
-    with django_assert_num_queries(8):
+    with django_assert_num_queries(11):
         document.restore()
     document.refresh_from_db()
     child1.refresh_from_db()
@@ -1084,7 +1133,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries):
 
     # Restoring the grand parent should not restore the document
     # as it was deleted before the grand parent
-    with django_assert_num_queries(7):
+    with django_assert_num_queries(9):
         grand_parent.restore()
 
     grand_parent.refresh_from_db()
@@ -1101,6 +1150,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries):
     assert child1.ancestors_deleted_at == document.deleted_at
     assert child2.ancestors_deleted_at == document.deleted_at
 
+
 @pytest.mark.parametrize(
     "ancestors_links, select_options",
     [