From bc8a7cec467a9bae28452230169c066ef468b467 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Sat, 20 Jun 2026 16:33:54 -0700 Subject: [PATCH] fix(admin): guard Photo resolution column + add project column (#1346) Closes the last Phase 4 item on the admin changelist audit: - Guard Photo.get_resolution_as_str against missing/unreadable image files. Reading picture.width/height opens the file, so one bad file would 500 the whole Photo changelist (same crash class as the News thumbnail guard). Returns 'Unknown' instead of raising. - Add the owning `project` as a changelist column, with list_select_related so the column stays constant-query as rows grow. - Regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) --- website/admin/photo_admin.py | 8 ++++++-- website/models/photo.py | 11 +++++++++-- website/tests/test_admin_changelist.py | 19 ++++++++++++++++++- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/website/admin/photo_admin.py b/website/admin/photo_admin.py index f1695cbc..c4e2ef05 100644 --- a/website/admin/photo_admin.py +++ b/website/admin/photo_admin.py @@ -5,8 +5,12 @@ @admin.register(Photo, site=ml_admin_site) class PhotoAdmin(ImageCroppingMixin, admin.ModelAdmin): - list_display = ('admin_thumbnail', 'caption', 'alt_text', 'get_resolution_as_str', - 'cropping', 'picture') + list_display = ('admin_thumbnail', 'caption', 'alt_text', 'project', + 'get_resolution_as_str', 'cropping', 'picture') + + # The `project` column is an FK; select_related keeps the changelist at a + # constant query count instead of one extra query per row. + list_select_related = ('project',) # Photos had no search box; search caption/alt text and the owning project. search_fields = ['caption', 'alt_text', 'project__name'] diff --git a/website/models/photo.py b/website/models/photo.py index 7d947346..bc2ec231 100644 --- a/website/models/photo.py +++ b/website/models/photo.py @@ -44,8 +44,15 @@ def admin_thumbnail(self): admin_thumbnail.short_description = 'Thumbnail' def get_resolution_as_str(self): - return f"{self.picture.width}x{self.picture.height}" - + # Reading width/height opens the underlying file, so a missing or + # unreadable image raises (FileNotFoundError / OSError). The Photo + # changelist renders this for every row, so one bad file must not 500 + # the whole page (same crash class as the News thumbnail guard, #1346). + try: + return f"{self.picture.width}x{self.picture.height}" + except (FileNotFoundError, OSError, ValueError): + return 'Unknown' + get_resolution_as_str.short_description = 'Resolution' def __str__(self): diff --git a/website/tests/test_admin_changelist.py b/website/tests/test_admin_changelist.py index 5d6cf1db..92f16ed6 100644 --- a/website/tests/test_admin_changelist.py +++ b/website/tests/test_admin_changelist.py @@ -12,7 +12,7 @@ from django.core.files.base import ContentFile -from website.models import News +from website.models import News, Photo from website.admin.admin_site import ml_admin_site from website.admin.news_admin import NewsAdmin from website.admin.keyword_admin import KeywordAdmin @@ -46,6 +46,17 @@ def test_corrupt_image_returns_placeholder_instead_of_raising(self): self.assertEqual(admin.get_display_thumbnail(news), 'No Thumbnail') +class PhotoResolutionRobustnessTests(DatabaseTestCase): + """A missing/unreadable image file must not crash the Photo changelist (#1346).""" + + def test_missing_file_returns_unknown_instead_of_raising(self): + # Point the ImageField at a path with no backing file, so reading + # width/height (which opens the file) raises FileNotFoundError. + photo = Photo(caption="Ghost photo", picture="projects/images/does_not_exist.jpg") + # Must not raise; the guard returns a placeholder. + self.assertEqual(photo.get_resolution_as_str(), 'Unknown') + + class AdminChangelistConfigTests(DatabaseTestCase): """Lock in the Phase 1 search / ordering / date_hierarchy additions.""" @@ -93,6 +104,12 @@ def test_project_umbrella_search_and_ordering(self): def test_photo_search(self): self.assertIn('project__name', PhotoAdmin.search_fields) + def test_photo_project_column_and_select_related(self): + # The owning project is surfaced as a column; select_related keeps the + # changelist constant-query as rows grow (#1346 Phase 4). + self.assertIn('project', PhotoAdmin.list_display) + self.assertEqual(PhotoAdmin.list_select_related, ('project',)) + def test_position_search(self): self.assertIn('person__last_name', PositionAdmin.search_fields)