make document post_delete signal transaction aware

wagtail/wagtaildocs/signal_handlers.py

@@ -1,15 +1,25 @@
 from __future__ import absolute_import, unicode_literals
 
+from django import VERSION as DJANGO_VERSION
+from django.db import transaction
 from django.db.models.signals import post_delete
 
 from wagtail.wagtaildocs.models import Document
 
 
-# Receive the post_delete signal and delete the file associated with the model instance.
-def post_delete_document_file_cleanup(sender, instance, **kwargs):
+TRANSACTION_ON_COMMIT_AVAILABLE = (1, 8) < DJANGO_VERSION[:2]
+if TRANSACTION_ON_COMMIT_AVAILABLE:
+    def on_commit_handler(f):
+        return transaction.on_commit(f)
+else:
+    def on_commit_handler(f):
+        return f()
+
+
+def post_delete_file_cleanup(instance, **kwargs):
     # Pass false so FileField doesn't save the model.
-    instance.file.delete(False)
+    on_commit_handler(lambda: instance.file.delete(False))
 
 
 def register_signal_handlers():
-    post_delete.connect(post_delete_document_file_cleanup, sender=Document)
+    post_delete.connect(post_delete_file_cleanup, sender=Document)

wagtail/wagtaildocs/tests/test_models.py

@@ -1,12 +1,17 @@
 from __future__ import absolute_import, unicode_literals
 
+import unittest
+
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import Group, Permission
 from django.core.files.base import ContentFile
-from django.test import TestCase
+from django.db import transaction
+from django.test import TestCase, TransactionTestCase
 
 from wagtail.wagtailcore.models import Collection, GroupCollectionPermission
-from wagtail.wagtaildocs import models
+from wagtail.wagtaildocs import models, signal_handlers
+from wagtail.wagtaildocs.models import get_document_model
+from wagtail.wagtailimages.tests.utils import get_test_image_file
 
 
 class TestDocumentQuerySet(TestCase):
@@ -96,3 +101,54 @@ class TestDocumentFilenameProperties(TestCase):
     def tearDown(self):
         self.document.delete()
         self.extensionless_document.delete()
+
+
+class TestFilesDeletedForDefaultModels(TransactionTestCase):
+    '''
+    Because we expect file deletion to only happen once a transaction is
+    successfully committed, we must run these tests using TransactionTestCase
+    per the following documentation:
+
+        Django's TestCase class wraps each test in a transaction and rolls back that
+        transaction after each test, in order to provide test isolation. This means
+        that no transaction is ever actually committed, thus your on_commit()
+        callbacks will never be run. If you need to test the results of an
+        on_commit() callback, use a TransactionTestCase instead.
+        https://docs.djangoproject.com/en/1.10/topics/db/transactions/#use-in-tests
+    '''
+    def setUp(self):
+        # Required to create root collection because the TransactionTestCase
+        # does not make initial data loaded in migrations available and
+        # serialized_rollback=True causes other problems in the test suite.
+        # ref: https://docs.djangoproject.com/en/1.10/topics/testing/overview/#rollback-emulation
+        Collection.objects.get_or_create(
+            name="Root",
+            path='0001',
+            depth=1,
+            numchild=0,
+        )
+
+    def test_oncommit_available(self):
+        self.assertEqual(hasattr(transaction, 'on_commit'), signal_handlers.TRANSACTION_ON_COMMIT_AVAILABLE)
+
+    @unittest.skipUnless(signal_handlers.TRANSACTION_ON_COMMIT_AVAILABLE, 'is required for this test')
+    def test_document_file_deleted_oncommit(self):
+        with transaction.atomic():
+            document = get_document_model().objects.create(title="Test Image", file=get_test_image_file())
+            self.assertTrue(document.file.storage.exists(document.file.name))
+            document.delete()
+            self.assertTrue(document.file.storage.exists(document.file.name))
+        self.assertFalse(document.file.storage.exists(document.file.name))
+
+    @unittest.skipIf(signal_handlers.TRANSACTION_ON_COMMIT_AVAILABLE, 'duplicate')
+    def test_document_file_deleted(self):
+        '''
+            this test duplicates `test_image_file_deleted_oncommit` for
+            django 1.8 support and can be removed once django 1.8 is no longer
+            supported
+        '''
+        with transaction.atomic():
+            document = get_document_model().objects.create(title="Test Image", file=get_test_image_file())
+            self.assertTrue(document.file.storage.exists(document.file.name))
+            document.delete()
+        self.assertFalse(document.file.storage.exists(document.file.name))