Simplify logging in AccessControlLoggingFileSystemStorage and use alt…

…ernate in-memory storages to reduce need for mocking in tests
ONSdigital · Dec 13, 2024 · faefcac · faefcac
1 parent d5f2ecc
commit faefcac
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 58 deletions.
diff --git a/cms/private_media/storages.py b/cms/private_media/storages.py
@@ -2,7 +2,7 @@
 from typing import TYPE_CHECKING
 
 from botocore.exceptions import ClientError
-from django.core.files.storage import FileSystemStorage
+from django.core.files.storage import FileSystemStorage, InMemoryStorage
 from storages.backends.s3 import S3Storage
 
 if TYPE_CHECKING:
@@ -42,29 +42,45 @@ def _set_file_acl(self, file: "FieldFile", acl_name: str) -> bool:
         return True
 
 
-class AccessControlLoggingMixin:
-    """A mixin for storage backends that do not support setting of individual file permissions."""
+class AccessControlLoggingFileSystemStorage(FileSystemStorage):
+    """A version of Django's `FileSystemStorage` backend for local development and tests, which logs
+    file-permission-setting requests, and always reports success.
+    """
 
     def make_private(self, file: "FieldFile") -> bool:
         """Pretend to make the provided file private."""
-        logger.info(
-            "%s does not support setting of individual file permissions to private, so skipping for: %s.",
-            self.__class__.__name__,
-            file.name,
-        )
+        logger.info("Skipping private file permission setting for '%s'.", file.name)
         return True
 
     def make_public(self, file: "FieldFile") -> bool:
         """Pretend to make the provided file public."""
-        logger.info(
-            "%s does not support setting of individual file permissions to public, so skipping for: %s.",
-            self.__class__.__name__,
-            file.name,
-        )
+        logger.info("Skipping public file permission setting for '%s'.", file.name)
         return True
 
 
-class AccessControlLoggingFileSystemStorage(AccessControlLoggingMixin, FileSystemStorage):
-    """A version of Django's `FileSystemStorage` backend for local development and tests, which logs
-    file-permission-setting requests, and always reports success.
+class ReliableAccessControlInMemoryStorage(InMemoryStorage):
+    """A version of Django's `InMemoryStorage` backend for unit tests, that always reports success
+    for file-permission-setting requests.
+    """
+
+    def make_private(self, file: "FieldFile") -> bool:  # pylint: disable=unused-argument
+        """Report success in making the provided file private."""
+        return True
+
+    def make_public(self, file: "FieldFile") -> bool:  # pylint: disable=unused-argument
+        """Report success in making the provided file public."""
+        return True
+
+
+class FlakyAccessControlInMemoryStorage(InMemoryStorage):
+    """A version of Django's `InMemoryStorage` backend for unit tests, that always reports failure
+    for file-permission-setting requests.
     """
+
+    def make_private(self, file: "FieldFile") -> bool:  # pylint: disable=unused-argument
+        """Report failure in making the provided file private."""
+        return False
+
+    def make_public(self, file: "FieldFile") -> bool:  # pylint: disable=unused-argument
+        """Report failure in making the provided file public."""
+        return False
diff --git a/cms/private_media/tests/test_documents.py b/cms/private_media/tests/test_documents.py
@@ -47,12 +47,7 @@ def test_private_document(self):
         # since the default file backend doesn't support it
         self.assertEqual(
             logs.output,
-            [
-                (
-                    "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
-                    f"of individual file permissions to private, so skipping for: {document.file.name}."
-                )
-            ],
+            [(f"INFO:cms.private_media.storages:Skipping private file permission setting for '{document.file.name}'.")],
         )
 
         # File permissions should be considered up-to-date
@@ -94,12 +89,7 @@ def test_public_document(self):
         # since the default file backend doesn't support it
         self.assertEqual(
             logs.output,
-            [
-                (
-                    "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
-                    f"of individual file permissions to public, so skipping for: {document.file.name}."
-                )
-            ],
+            [(f"INFO:cms.private_media.storages:Skipping public file permission setting for '{document.file.name}'.")],
         )
 
         # File permissions should be considered up-to-date

diff --git a/cms/private_media/tests/test_images.py b/cms/private_media/tests/test_images.py
@@ -37,7 +37,6 @@ def test_private_image(self):
         - Verify that images are created as PRIVATE by default.
         - Verify that file permission states are updated on save to reflect image privacy.
         - Verify that privacy change tracking works correctly.
-        - Graceful handling of unsupported storage backends.
         """
         # Attempts to set file permissions on save should have failed gracefully,
         # since the default file backend doesn't support it
@@ -52,12 +51,7 @@ def test_private_image(self):
         # Attempts to set file permissions on save should have failed gracefully
         self.assertEqual(
             logs.output,
-            [
-                (
-                    "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
-                    f"of individual file permissions to private, so skipping for: {image.file.name}."
-                )
-            ],
+            [(f"INFO:cms.private_media.storages:Skipping private file permission setting for '{image.file.name}'.")],
         )
 
         # File permissions should be considered up-to-date
@@ -112,12 +106,7 @@ def test_public_image(self):
         # Attempts to set file permissions on save should have failed gracefully
         self.assertEqual(
             logs.output,
-            [
-                (
-                    "INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
-                    f"of individual file permissions to public, so skipping for: {image.file.name}."
-                )
-            ],
+            [(f"INFO:cms.private_media.storages:Skipping public file permission setting for '{image.file.name}'.")],
         )
 
         # File permissions should be considered up-to-date
@@ -211,7 +200,7 @@ def test_invalid_privacy_value_raises_value_error(self):
             ImageFactory(_privacy="invalid", collection=self.root_collection)
 
     @override_settings(
-        STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
+        STORAGES={"default": {"BACKEND": "cms.private_media.storages.ReliableAccessControlInMemoryStorage"}}
     )
     def test_file_permission_setting_success(self):
         """Test successful file permission setting using a storage backend that supports it."""
@@ -223,29 +212,17 @@ def test_file_permission_setting_success(self):
         self.assertFalse(public_image.has_outdated_file_permissions())
 
     @override_settings(
-        STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
-    )
-    @mock.patch(
-        "cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_private",
-        return_value=False,
+        STORAGES={"default": {"BACKEND": "cms.private_media.storages.FlakyAccessControlInMemoryStorage"}}
     )
-    @mock.patch(
-        "cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_public",
-        return_value=False,
-    )
-    def test_file_permission_setting_failure(self, mock_make_public, mock_make_private):
+    def test_file_permission_setting_failure(self):
         """Test graceful handling of file permission setting failures.
 
         Verifies that the system correctly tracks failed permission updates and
         maintains the outdated state when storage operations fail.
         """
-        with self.assertNoLogs("cms.private_media.bulk_operations"):
-            private_image = ImageFactory(collection=self.root_collection)
-            public_image = ImageFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection)
-
-        mock_make_private.assert_called_once_with(private_image.file)
+        private_image = ImageFactory(collection=self.root_collection)
+        public_image = ImageFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection)
         self.assertTrue(private_image.has_outdated_file_permissions())
-        mock_make_public.assert_called_once_with(public_image.file)
         self.assertTrue(public_image.has_outdated_file_permissions())