fixed a bunch of things, hopefully didn't break things

media/models.py

@@ -27,24 +27,16 @@ def photo_upload_path(instance: models.Model, filename: str) -> str:
     if identifier is None:
         identifier = obj.pk  # Use pk instead of id as it's guaranteed to exist
     
-    # Get the next available number for this object
-    existing_photos = Photo.objects.filter(
-        content_type=photo.content_type,
-        object_id=photo.object_id
-    ).count()
-    next_number = existing_photos + 1
-    
-    # Create normalized filename
-    ext = os.path.splitext(filename)[1].lower() or '.jpg'  # Default to .jpg if no extension
-    new_filename = f"{identifier}_{next_number}{ext}"
+    # Create normalized filename - always use .jpg extension
+    base_filename = f"{identifier}.jpg"
     
     # If it's a ride photo, store it under the park's directory
     if content_type == 'ride':
         ride = cast(Ride, obj)
-        return f"park/{ride.park.slug}/{identifier}/{new_filename}"
+        return f"park/{ride.park.slug}/{identifier}/{base_filename}"
     
     # For park photos, store directly in park directory
-    return f"park/{identifier}/{new_filename}"
+    return f"park/{identifier}/{base_filename}"
 
 class Photo(models.Model):
     """Generic photo model that can be attached to any model"""

media/storage.py

@@ -1,16 +1,30 @@
 from django.core.files.storage import FileSystemStorage
 from django.conf import settings
+from django.core.files.base import File
+from django.core.files.move import file_move_safe
+from django.core.files.uploadedfile import UploadedFile, TemporaryUploadedFile
 import os
+import re
+from typing import Optional, Any, Union
 
 class MediaStorage(FileSystemStorage):
-    def __init__(self, *args, **kwargs):
+    _instance = None
+    _counters = {}
+
+    def __init__(self, *args: Any, **kwargs: Any) -> None:
         kwargs['location'] = settings.MEDIA_ROOT
         kwargs['base_url'] = settings.MEDIA_URL
         super().__init__(*args, **kwargs)
     
-    def get_available_name(self, name, max_length=None):
+    @classmethod
+    def reset_counters(cls):
+        """Reset all counters - useful for testing"""
+        cls._counters = {}
+    
+    def get_available_name(self, name: str, max_length: Optional[int] = None) -> str:
         """
         Returns a filename that's free on the target storage system.
+        Ensures proper normalization and uniqueness.
         """
         # Get the directory and filename
         directory = os.path.dirname(name)
@@ -20,19 +34,49 @@ class MediaStorage(FileSystemStorage):
         full_dir = os.path.join(self.location, directory)
         os.makedirs(full_dir, exist_ok=True)
         
-        # Return the name as is since our upload path already handles uniqueness
-        return name
+        # Split filename into root and extension
+        file_root, file_ext = os.path.splitext(filename)
+        
+        # Extract base name without any existing numbers
+        base_root = file_root.rsplit('_', 1)[0]
+        
+        # Use counter for this directory
+        dir_key = os.path.join(directory, base_root)
+        if dir_key not in self._counters:
+            self._counters[dir_key] = 0
+        
+        self._counters[dir_key] += 1
+        counter = self._counters[dir_key]
+        
+        new_name = f"{base_root}_{counter}{file_ext}"
+        return os.path.join(directory, new_name)
     
-    def _save(self, name, content):
+    def _save(self, name: str, content: Union[File, UploadedFile]) -> str:
         """
-        Save with proper permissions
+        Save the file and set proper permissions
         """
-        # Save the file
-        name = super()._save(name, content)
+        # Get the full path where the file will be saved
+        full_path = self.path(name)
+        directory = os.path.dirname(full_path)
+        
+        # Create the directory if it doesn't exist
+        os.makedirs(directory, exist_ok=True)
+        
+        # Save the file using Django's file handling
+        if isinstance(content, TemporaryUploadedFile):
+            # This is a TemporaryUploadedFile
+            file_move_safe(content.temporary_file_path(), full_path)
+        else:
+            # This is an InMemoryUploadedFile or similar
+            with open(full_path, 'wb') as destination:
+                if hasattr(content, 'chunks'):
+                    for chunk in content.chunks():
+                        destination.write(chunk)
+                else:
+                    destination.write(content.read())
         
         # Set proper permissions
-        full_path = self.path(name)
         os.chmod(full_path, 0o644)
-        os.chmod(os.path.dirname(full_path), 0o755)
+        os.chmod(directory, 0o755)
         
         return name

media/tests.py

@@ -3,74 +3,192 @@ from django.core.files.uploadedfile import SimpleUploadedFile
 from django.contrib.auth import get_user_model
 from django.contrib.contenttypes.models import ContentType
 from django.utils import timezone
+from django.conf import settings
+from django.test.utils import override_settings
+from django.db import models
 from datetime import datetime
-from PIL import Image, ExifTags
+from PIL import Image
+import piexif
 import io
+import shutil
+import tempfile
+import os
+import logging
+from typing import Optional, Any, Generator, cast
+from contextlib import contextmanager
 from .models import Photo
+from .storage import MediaStorage
 from parks.models import Park
 
 User = get_user_model()
+logger = logging.getLogger(__name__)
 
+@override_settings(MEDIA_ROOT=tempfile.mkdtemp())
 class PhotoModelTests(TestCase):
-    def setUp(self):
-        # Create a test user
-        self.user = User.objects.create_user(
+    test_media_root: str
+    user: models.Model
+    park: Park
+    content_type: ContentType
+    
+    @classmethod
+    def setUpClass(cls) -> None:
+        super().setUpClass()
+        cls.test_media_root = settings.MEDIA_ROOT
+        
+    @classmethod
+    def tearDownClass(cls) -> None:
+        try:
+            shutil.rmtree(cls.test_media_root, ignore_errors=True)
+        except Exception as e:
+            logger.warning(f"Failed to clean up test media directory: {e}")
+        super().tearDownClass()
+
+    def setUp(self) -> None:
+        self.user = self._create_test_user()
+        self.park = self._create_test_park()
+        self.content_type = ContentType.objects.get_for_model(Park)
+        self._setup_test_directory()
+        
+    def tearDown(self) -> None:
+        self._cleanup_test_directory()
+        Photo.objects.all().delete()
+        with self._reset_storage_state():
+            pass
+
+    def _create_test_user(self) -> models.Model:
+        """Create a test user for the tests"""
+        return User.objects.create_user(
             username='testuser',
             password='testpass123'
         )
-        
-        # Create a test park for photo association
-        self.park = Park.objects.create(
+
+    def _create_test_park(self) -> Park:
+        """Create a test park for the tests"""
+        return Park.objects.create(
             name='Test Park',
             slug='test-park'
         )
-        
-        self.content_type = ContentType.objects.get_for_model(Park)
 
-    def create_test_image_with_exif(self, date_taken=None):
+    def _setup_test_directory(self) -> None:
+        """Set up test directory and clean any existing test files"""
+        try:
+            # Clean up any existing test park directory
+            test_park_dir = os.path.join(settings.MEDIA_ROOT, 'park', 'test-park')
+            if os.path.exists(test_park_dir):
+                shutil.rmtree(test_park_dir, ignore_errors=True)
+            
+            # Create necessary directories
+            os.makedirs(test_park_dir, exist_ok=True)
+            
+        except Exception as e:
+            logger.warning(f"Failed to set up test directory: {e}")
+            raise
+
+    def _cleanup_test_directory(self) -> None:
+        """Clean up test directories and files"""
+        try:
+            test_park_dir = os.path.join(settings.MEDIA_ROOT, 'park', 'test-park')
+            if os.path.exists(test_park_dir):
+                shutil.rmtree(test_park_dir, ignore_errors=True)
+        except Exception as e:
+            logger.warning(f"Failed to clean up test directory: {e}")
+
+    @contextmanager
+    def _reset_storage_state(self) -> Generator[None, None, None]:
+        """Safely reset storage state"""
+        try:
+            MediaStorage.reset_counters()
+            yield
+        finally:
+            MediaStorage.reset_counters()
+
+    def create_test_image_with_exif(self, date_taken: Optional[datetime] = None, filename: str = 'test.jpg') -> SimpleUploadedFile:
         """Helper method to create a test image with EXIF data"""
-        # Create a test image
         image = Image.new('RGB', (100, 100), color='red')
         image_io = io.BytesIO()
         
-        # Add EXIF data if date_taken is provided
+        # Save image first without EXIF
+        image.save(image_io, 'JPEG')
+        image_io.seek(0)
+        
         if date_taken:
+            # Create EXIF data
             exif_dict = {
                 "0th": {},
                 "Exif": {
-                    ExifTags.Base.DateTimeOriginal: date_taken.strftime("%Y:%m:%d %H:%M:%S").encode()
+                    piexif.ExifIFD.DateTimeOriginal: date_taken.strftime("%Y:%m:%d %H:%M:%S").encode()
                 }
             }
-            image.save(image_io, 'JPEG', exif=exif_dict)
+            exif_bytes = piexif.dump(exif_dict)
+            
+            # Insert EXIF into image
+            image_with_exif = io.BytesIO()
+            piexif.insert(exif_bytes, image_io.getvalue(), image_with_exif)
+            image_with_exif.seek(0)
+            image_data = image_with_exif.getvalue()
         else:
-            image.save(image_io, 'JPEG')
+            image_data = image_io.getvalue()
         
-        image_io.seek(0)
         return SimpleUploadedFile(
-            'test.jpg',
-            image_io.getvalue(),
+            filename,
+            image_data,
             content_type='image/jpeg'
         )
 
-    def test_photo_creation(self):
-        """Test basic photo creation"""
-        photo = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
-            caption='Test Caption',
-            uploaded_by=self.user,
-            content_type=self.content_type,
-            object_id=self.park.pk
-        )
-        
-        self.assertEqual(photo.caption, 'Test Caption')
-        self.assertEqual(photo.uploaded_by, self.user)
-        self.assertIsNone(photo.date_taken)
+    def test_filename_normalization(self) -> None:
+        """Test that filenames are properly normalized"""
+        with self._reset_storage_state():
+            # Test with various problematic filenames
+            test_cases = [
+                ('test with spaces.jpg', 'test-park_1.jpg'),
+                ('TEST_UPPER.JPG', 'test-park_2.jpg'),
+                ('special@#chars.jpeg', 'test-park_3.jpg'),
+                ('no-extension', 'test-park_4.jpg'),
+                ('multiple...dots.jpg', 'test-park_5.jpg'),
+                ('très_açaí.jpg', 'test-park_6.jpg'),  # Unicode characters
+            ]
 
-    def test_exif_date_extraction(self):
+            for input_name, expected_suffix in test_cases:
+                photo = Photo.objects.create(
+                    image=self.create_test_image_with_exif(filename=input_name),
+                    uploaded_by=self.user,
+                    content_type=self.content_type,
+                    object_id=self.park.pk
+                )
+                
+                # Check that the filename follows the normalized pattern
+                self.assertTrue(
+                    photo.image.name.endswith(expected_suffix),
+                    f"Expected filename to end with {expected_suffix}, got {photo.image.name}"
+                )
+                
+                # Verify the path structure
+                expected_path = f"park/{self.park.slug}/"
+                self.assertTrue(
+                    photo.image.name.startswith(expected_path),
+                    f"Expected path to start with {expected_path}, got {photo.image.name}"
+                )
+
+    def test_sequential_filename_numbering(self) -> None:
+        """Test that sequential files get proper numbering"""
+        with self._reset_storage_state():
+            # Create multiple photos and verify numbering
+            for i in range(1, 4):
+                photo = Photo.objects.create(
+                    image=self.create_test_image_with_exif(),
+                    uploaded_by=self.user,
+                    content_type=self.content_type,
+                    object_id=self.park.pk
+                )
+                
+                expected_name = f"park/{self.park.slug}/test-park_{i}.jpg"
+                self.assertEqual(
+                    photo.image.name,
+                    expected_name,
+                    f"Expected {expected_name}, got {photo.image.name}"
+                )
+
+    def test_exif_date_extraction(self) -> None:
         """Test EXIF date extraction from uploaded photos"""
         test_date = datetime(2024, 1, 1, 12, 0, 0)
         image_file = self.create_test_image_with_exif(test_date)
@@ -90,9 +208,9 @@ class PhotoModelTests(TestCase):
         else:
             self.skipTest("EXIF data extraction not supported in test environment")
 
-    def test_photo_without_exif(self):
+    def test_photo_without_exif(self) -> None:
         """Test photo upload without EXIF data"""
-        image_file = self.create_test_image_with_exif()  # No date provided
+        image_file = self.create_test_image_with_exif()
         
         photo = Photo.objects.create(
             image=image_file,
@@ -103,31 +221,22 @@ class PhotoModelTests(TestCase):
         
         self.assertIsNone(photo.date_taken)
 
-    def test_default_caption(self):
+    def test_default_caption(self) -> None:
         """Test default caption generation"""
         photo = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
+            image=self.create_test_image_with_exif(),
             uploaded_by=self.user,
             content_type=self.content_type,
             object_id=self.park.pk
         )
         
-        expected_prefix = f"Uploaded by {self.user.username} on"
+        expected_prefix = f"Uploaded by {cast(Any, self.user).username} on"
         self.assertTrue(photo.caption.startswith(expected_prefix))
 
-    def test_primary_photo_toggle(self):
+    def test_primary_photo_toggle(self) -> None:
         """Test primary photo functionality"""
-        # Create two photos
         photo1 = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test1.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
+            image=self.create_test_image_with_exif(),
             uploaded_by=self.user,
             content_type=self.content_type,
             object_id=self.park.pk,
@@ -135,51 +244,24 @@ class PhotoModelTests(TestCase):
         )
         
         photo2 = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test2.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
+            image=self.create_test_image_with_exif(),
             uploaded_by=self.user,
             content_type=self.content_type,
             object_id=self.park.pk,
             is_primary=True
         )
         
-        # Refresh from database
         photo1.refresh_from_db()
         photo2.refresh_from_db()
         
-        # Verify only photo2 is primary
         self.assertFalse(photo1.is_primary)
         self.assertTrue(photo2.is_primary)
 
-    @override_settings(MEDIA_ROOT='test_media/')
-    def test_photo_upload_path(self):
-        """Test photo upload path generation"""
-        photo = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
-            uploaded_by=self.user,
-            content_type=self.content_type,
-            object_id=self.park.pk
-        )
-        
-        expected_path = f"park/{self.park.slug}/"
-        self.assertTrue(photo.image.name.startswith(expected_path))
-
-    def test_date_taken_field(self):
+    def test_date_taken_field(self) -> None:
         """Test date_taken field functionality"""
         test_date = timezone.now()
         photo = Photo.objects.create(
-            image=SimpleUploadedFile(
-                'test.jpg',
-                b'dummy image data',
-                content_type='image/jpeg'
-            ),
+            image=self.create_test_image_with_exif(),
             uploaded_by=self.user,
             content_type=self.content_type,
             object_id=self.park.pk,