Skip to content

Commit

Permalink
Placate linters
Browse files Browse the repository at this point in the history
  • Loading branch information
ababic committed Dec 13, 2024
1 parent f3af123 commit b581823
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 58 deletions.
6 changes: 2 additions & 4 deletions cms/private_media/bulk_operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,10 @@ def bulk_set_file_permissions(files: Iterable["FieldFile"], privacy: Privacy) ->

def set_file_permission_and_report(file: "FieldFile") -> None:
storage = file.storage
handler: Callable[[FieldFile], bool] | None
if privacy is Privacy.PRIVATE:
handler = getattr(storage, "make_private", None)
results[file] = storage.make_private(file)
elif privacy is Privacy.PUBLIC:
handler = getattr(storage, "make_public", None)
results[file] = handler(file)
results[file] = storage.make_public(file)

with concurrent.futures.ThreadPoolExecutor(
max_workers=int(settings.PRIVATE_MEDIA_BULK_UPDATE_MAX_WORKERS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@


class Command(BaseCommand):
dry_run: bool

def add_arguments(self, parser: Any) -> None:
parser.add_argument(
"--dry-run",
Expand All @@ -22,7 +24,7 @@ def add_arguments(self, parser: Any) -> None:
)

def handle(self, *args: Any, **options: Any) -> None:
self.dry_run = options["dry_run"] # pylint: disable=attribute-defined-outside-init
self.dry_run = options["dry_run"]
if self.dry_run:
self.stdout.write("This is a dry run.")

Expand Down Expand Up @@ -56,7 +58,7 @@ def update_file_permissions(
for item in model_class.objects.bulk_set_file_permissions( # type: ignore[attr-defined]
items, privacy, save_changes=True
):
if not item.file_permissions_are_outdated():
if not item.has_outdated_file_permissions():
updated_count += 1

self.stdout.write(f"File permissions successfully updated for {updated_count} {privacy} {plural}.")
2 changes: 1 addition & 1 deletion cms/private_media/models/images.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def url(self) -> str:
str: URL for accessing the rendition
"""
image: PrivateImageMixin = self.image # pylint: disable=no-member
if image.is_public and not image.file_permissions_are_outdated():
if image.is_public and not image.has_outdated_file_permissions():
file_url: str = self.file.url
return file_url
return self.serve_url
Expand Down
6 changes: 3 additions & 3 deletions cms/private_media/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,16 @@ def save(self, *args: Any, set_file_permissions: bool = True, **kwargs: Any) ->
super().save(*args, **kwargs)

# Trigger file permission updates after-the-fact
if set_file_permissions and self.file_permissions_are_outdated():
if set_file_permissions and self.has_outdated_file_permissions():
results = bulk_set_file_permissions(self.get_privacy_controlled_files(), self.privacy)
# Only update 'file_permissions_last_set' if all updates were successfull
all_updates_successful = all(results.values())
all_updates_successful = all(results.values())
if all_updates_successful:
self.file_permissions_last_set = timezone.now()
kwargs.update(force_insert=False, update_fields=["file_permissions_last_set"])
super().save(*args, **kwargs)

def file_permissions_are_outdated(self) -> bool:
def has_outdated_file_permissions(self) -> bool:
"""Check if the file permissions are outdated relative to privacy changes."""
return self.file_permissions_last_set is None or self.file_permissions_last_set < self.privacy_last_changed

Expand Down
14 changes: 7 additions & 7 deletions cms/private_media/storages.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from typing import TYPE_CHECKING

from botocore.exceptions import ClientError
from django.core.files.storage import FileSystemStorage, InMemoryStorage
from django.core.files.storage import FileSystemStorage
from storages.backends.s3 import S3Storage

if TYPE_CHECKING:
Expand Down Expand Up @@ -43,10 +43,9 @@ def _set_file_acl(self, file: "FieldFile", acl_name: str) -> bool:


class AccessControlLoggingMixin:
"""
A mixin for storage backends that do not support setting of individual file permissions.
"""
def make_private(self, file: "FieldFile") -> bool: # pylint: disable=unused-argument
"""A mixin for storage backends that do not support setting of individual file permissions."""

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.",
Expand All @@ -55,7 +54,7 @@ def make_private(self, file: "FieldFile") -> bool: # pylint: disable=unused-arg
)
return True

def make_public(self, file: "FieldFile") -> bool: # pylint: disable=unused-argument
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.",
Expand All @@ -67,4 +66,5 @@ def make_public(self, file: "FieldFile") -> bool: # pylint: disable=unused-argu

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."""
file-permission-setting requests, and always reports success.
"""
42 changes: 23 additions & 19 deletions cms/private_media/tests/test_documents.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ def test_private_document(self):
logs.output,
[
(
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual "
f"file permissions to private, so skipping for: {document.file.name}."
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
f"of individual file permissions to private, so skipping for: {document.file.name}."
)
],
)

# File permissions should be considered up-to-date
self.assertFalse(document.file_permissions_are_outdated())
self.assertFalse(document.has_outdated_file_permissions())

# Setting privacy to the same value should not trigger an update to 'privacy_last_changed
value_before = document.privacy_last_changed
Expand All @@ -68,12 +68,12 @@ def test_private_document(self):
self.assertGreater(document.privacy_last_changed, value_before)

# File permissions should now be considered outdated
self.assertTrue(document.file_permissions_are_outdated())
self.assertTrue(document.has_outdated_file_permissions())

# Resaving should trigger an update to file permissions and the 'file_permissions_last_set'
# timestamp, resolving the issue
document.save()
self.assertFalse(document.file_permissions_are_outdated())
self.assertFalse(document.has_outdated_file_permissions())

def test_public_document(self):
"""Test the behaviour of public documents:
Expand All @@ -96,14 +96,14 @@ def test_public_document(self):
logs.output,
[
(
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual "
f"file permissions to public, so skipping for: {document.file.name}."
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
f"of individual file permissions to public, so skipping for: {document.file.name}."
)
],
)

# File permissions should be considered up-to-date
self.assertFalse(document.file_permissions_are_outdated())
self.assertFalse(document.has_outdated_file_permissions())

# Setting privacy to the same value should not trigger an update to 'privacy_last_changed
value_before = document.privacy_last_changed
Expand All @@ -115,12 +115,12 @@ def test_public_document(self):
self.assertGreater(document.privacy_last_changed, value_before)

# File permissions should now be considered outdated
self.assertTrue(document.file_permissions_are_outdated())
self.assertTrue(document.has_outdated_file_permissions())

# Resaving should trigger an update to file permissions and the 'file_permissions_last_set'
# timestamp, resolving the issue
document.save()
self.assertFalse(document.file_permissions_are_outdated())
self.assertFalse(document.has_outdated_file_permissions())

def test_invalid_privacy_value_raises_value_error(self):
"""Verify that attempting to create a document with an invalid privacy value raises ValueError."""
Expand Down Expand Up @@ -170,7 +170,9 @@ def domain_prefixed_url(name: str) -> str:
],
)

@override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}})
@override_settings(
STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
)
def test_file_permission_setting_success(self):
"""Test successful file permission setting using AccessControlLoggingFileSystemStorage:
- Verify permissions are set correctly for both public and private documents.
Expand All @@ -180,10 +182,12 @@ def test_file_permission_setting_success(self):
private_document = DocumentFactory(collection=self.root_collection)
public_document = DocumentFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection)

self.assertFalse(private_document.file_permissions_are_outdated())
self.assertFalse(public_document.file_permissions_are_outdated())
self.assertFalse(private_document.has_outdated_file_permissions())
self.assertFalse(public_document.has_outdated_file_permissions())

@override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}})
@override_settings(
STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
)
@mock.patch(
"cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_private",
return_value=False,
Expand All @@ -203,9 +207,9 @@ def test_file_permission_setting_failure(self, mock_make_public, mock_make_priva
public_document = DocumentFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection)

mock_make_private.assert_called_once_with(private_document.file)
self.assertTrue(private_document.file_permissions_are_outdated())
self.assertTrue(private_document.has_outdated_file_permissions())
mock_make_public.assert_called_once_with(public_document.file)
self.assertTrue(public_document.file_permissions_are_outdated())
self.assertTrue(public_document.has_outdated_file_permissions())


@override_settings(
Expand Down Expand Up @@ -248,7 +252,7 @@ def test_bulk_make_public(self):
# Verify all images are now public
for obj in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"):
self.assertIs(obj.privacy, Privacy.PUBLIC)
self.assertFalse(obj.file_permissions_are_outdated())
self.assertFalse(obj.has_outdated_file_permissions())

# Another attempt should result in no updates
with self.assertNumQueries(1):
Expand All @@ -271,7 +275,7 @@ def test_bulk_make_private(self):
# Verify all images are now private
for image in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"):
self.assertIs(image.privacy, Privacy.PRIVATE)
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

# Another attempt should result in no updates
with self.assertNumQueries(1):
Expand Down Expand Up @@ -319,6 +323,6 @@ def test_serve_public_document_with_outdated_file_permissions(self):
"""Test the serve view behaviour for public documents with outdated file permissions."""
self.model.objects.filter(id=self.public_document.id).update(file_permissions_last_set=None)
self.public_document.refresh_from_db()
self.assertTrue(self.public_document.file_permissions_are_outdated())
self.assertTrue(self.public_document.has_outdated_file_permissions())
response = self.client.get(self.public_document.url)
self.assertEqual(response.status_code, 200)
44 changes: 24 additions & 20 deletions cms/private_media/tests/test_images.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ def test_private_image(self):
logs.output,
[
(
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual "
f"file permissions to private, so skipping for: {image.file.name}."
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
f"of individual file permissions to private, so skipping for: {image.file.name}."
)
],
)

# File permissions should be considered up-to-date
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

# Setting privacy to the same value should not trigger an update to 'privacy_last_changed
value_before = image.privacy_last_changed
Expand All @@ -73,12 +73,12 @@ def test_private_image(self):
self.assertGreater(image.privacy_last_changed, value_before)

# File permissions should now be considered outdated
self.assertTrue(image.file_permissions_are_outdated())
self.assertTrue(image.has_outdated_file_permissions())

# Resaving should trigger an update to file permissions and the 'file_permissions_last_set'
# timestamp, resolving the issue
image.save()
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

def test_private_image_renditions(self):
"""Test that private image renditions use serve URLs instead of direct file URLs."""
Expand Down Expand Up @@ -114,14 +114,14 @@ def test_public_image(self):
logs.output,
[
(
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting of individual "
f"file permissions to public, so skipping for: {image.file.name}."
"INFO:cms.private_media.storages:AccessControlLoggingFileSystemStorage does not support setting "
f"of individual file permissions to public, so skipping for: {image.file.name}."
)
],
)

# File permissions should be considered up-to-date
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

# Setting privacy to the same value should not trigger an update to 'privacy_last_changed
value_before = image.privacy_last_changed
Expand All @@ -133,12 +133,12 @@ def test_public_image(self):
self.assertGreater(image.privacy_last_changed, value_before)

# File permissions should now be considered outdated
self.assertTrue(image.file_permissions_are_outdated())
self.assertTrue(image.has_outdated_file_permissions())

# Resaving should trigger an update to file permissions and the 'file_permissions_last_set'
# timestamp, resolving the issue
image.save()
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

def test_public_image_renditions(self):
"""Test rendition.url behaviour for public image renditions:
Expand All @@ -157,7 +157,7 @@ def test_public_image_renditions(self):

# However, if the file permissions are outdated, rendition.url should return a serve URL
with mock.patch(
"cms.private_media.models.PrivateMediaMixin.file_permissions_are_outdated",
"cms.private_media.models.PrivateMediaMixin.has_outdated_file_permissions",
return_value=True,
):
for rendition in renditions.values():
Expand Down Expand Up @@ -210,17 +210,21 @@ def test_invalid_privacy_value_raises_value_error(self):
with self.assertRaises(ValueError):
ImageFactory(_privacy="invalid", collection=self.root_collection)

@override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}})
@override_settings(
STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
)
def test_file_permission_setting_success(self):
"""Test successful file permission setting using a storage backend that supports it."""
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)

self.assertFalse(private_image.file_permissions_are_outdated())
self.assertFalse(public_image.file_permissions_are_outdated())
self.assertFalse(private_image.has_outdated_file_permissions())
self.assertFalse(public_image.has_outdated_file_permissions())

@override_settings(STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}})
@override_settings(
STORAGES={"default": {"BACKEND": "cms.private_media.storages.AccessControlLoggingFileSystemStorage"}}
)
@mock.patch(
"cms.private_media.storages.AccessControlLoggingFileSystemStorage.make_private",
return_value=False,
Expand All @@ -240,9 +244,9 @@ def test_file_permission_setting_failure(self, mock_make_public, mock_make_priva
public_image = ImageFactory(_privacy=Privacy.PUBLIC, collection=self.root_collection)

mock_make_private.assert_called_once_with(private_image.file)
self.assertTrue(private_image.file_permissions_are_outdated())
self.assertTrue(private_image.has_outdated_file_permissions())
mock_make_public.assert_called_once_with(public_image.file)
self.assertTrue(public_image.file_permissions_are_outdated())
self.assertTrue(public_image.has_outdated_file_permissions())


@override_settings(
Expand Down Expand Up @@ -289,7 +293,7 @@ def test_bulk_make_public(self):
# Verify all images are now public
for obj in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"):
self.assertIs(obj.privacy, Privacy.PUBLIC)
self.assertFalse(obj.file_permissions_are_outdated())
self.assertFalse(obj.has_outdated_file_permissions())

# Another attempt should result in no updates
with self.assertNumQueries(1):
Expand All @@ -314,7 +318,7 @@ def test_bulk_make_private(self):
# Verify all images are now private
for image in self.model.objects.only("_privacy", "file_permissions_last_set", "privacy_last_changed"):
self.assertIs(image.privacy, Privacy.PRIVATE)
self.assertFalse(image.file_permissions_are_outdated())
self.assertFalse(image.has_outdated_file_permissions())

# Another attempt should result in no updates
with self.assertNumQueries(1):
Expand Down Expand Up @@ -372,7 +376,7 @@ def test_serve_public_image_with_outdated_file_permissions(self):
"""Test the serve view behaviour for public image renditions with outdated file permissions."""
self.model.objects.filter(id=self.public_image.id).update(file_permissions_last_set=None)
self.public_image.refresh_from_db()
self.assertTrue(self.public_image.file_permissions_are_outdated())
self.assertTrue(self.public_image.has_outdated_file_permissions())
for rendition in self.public_image_renditions.values():
response = self.client.get(rendition.serve_url)
self.assertEqual(response.status_code, 200)
Loading

0 comments on commit b581823

Please sign in to comment.