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)