mirror of
https://github.com/pacnpal/thrillwiki_django_no_react.git
synced 2025-12-23 04:51:09 -05:00
feat: Refactor rides app with unique constraints, mixins, and enhanced documentation
- Added migration to convert unique_together constraints to UniqueConstraint for RideModel. - Introduced RideFormMixin for handling entity suggestions in ride forms. - Created comprehensive code standards documentation outlining formatting, docstring requirements, complexity guidelines, and testing requirements. - Established error handling guidelines with a structured exception hierarchy and best practices for API and view error handling. - Documented view pattern guidelines, emphasizing the use of CBVs, FBVs, and ViewSets with examples. - Implemented a benchmarking script for query performance analysis and optimization. - Developed security documentation detailing measures, configurations, and a security checklist. - Compiled a database optimization guide covering indexing strategies, query optimization patterns, and computed fields.
This commit is contained in:
@@ -203,11 +203,12 @@ class Command(BaseCommand):
|
||||
)
|
||||
|
||||
def create_parks(self):
|
||||
"""Create parks with proper operator relationships"""
|
||||
"""Create parks with proper operator relationships."""
|
||||
self.stdout.write("Creating parks...")
|
||||
|
||||
# TODO: Implement park creation - parks_data defined but not used yet
|
||||
parks_data = [ # noqa: F841
|
||||
# Park creation data - will be used to create parks in the database
|
||||
# TODO(THRILLWIKI-111): Complete park creation implementation
|
||||
parks_data = [
|
||||
{
|
||||
"name": "Magic Kingdom",
|
||||
"slug": "magic-kingdom",
|
||||
|
||||
@@ -194,10 +194,9 @@ class Command(BaseCommand):
|
||||
missing_tables = []
|
||||
for model in required_models:
|
||||
try:
|
||||
# Check if the table exists by trying to get the table name
|
||||
table_name = model._meta.db_table
|
||||
with connection.cursor() as cursor:
|
||||
cursor.execute(f"SELECT 1 FROM {table_name} LIMIT 1")
|
||||
# Security: Use Django ORM to check table existence instead of raw SQL
|
||||
# This is safer as it avoids any potential SQL injection via model metadata
|
||||
model.objects.exists()
|
||||
except Exception:
|
||||
missing_tables.append(model._meta.label)
|
||||
|
||||
|
||||
@@ -127,16 +127,17 @@ class ParkQuerySet(StatusQuerySet, ReviewableQuerySet, LocationQuerySet):
|
||||
| Q(location__state__icontains=query)
|
||||
)
|
||||
.select_related("operator", "location")
|
||||
.values(
|
||||
"id",
|
||||
"name",
|
||||
"slug",
|
||||
"location__city",
|
||||
"location__state",
|
||||
"operator__name",
|
||||
.only(
|
||||
"id", "name", "slug",
|
||||
"location__city", "location__state",
|
||||
"operator__name"
|
||||
)[:limit]
|
||||
)
|
||||
|
||||
def with_location(self):
|
||||
"""Always prefetch location for park queries."""
|
||||
return self.select_related("operator").prefetch_related("location")
|
||||
|
||||
|
||||
class ParkManager(StatusManager, ReviewableManager, LocationManager):
|
||||
"""Custom manager for Park model."""
|
||||
@@ -162,6 +163,10 @@ class ParkManager(StatusManager, ReviewableManager, LocationManager):
|
||||
def for_map_display(self, *, bounds=None):
|
||||
return self.get_queryset().for_map_display(bounds=bounds)
|
||||
|
||||
def with_location(self):
|
||||
"""Always prefetch location for park queries."""
|
||||
return self.get_queryset().with_location()
|
||||
|
||||
|
||||
class ParkAreaQuerySet(BaseQuerySet):
|
||||
"""QuerySet for ParkArea model."""
|
||||
@@ -300,3 +305,33 @@ class CompanyManager(BaseManager):
|
||||
|
||||
def major_operators(self, *, min_parks: int = 5):
|
||||
return self.get_queryset().major_operators(min_parks=min_parks)
|
||||
|
||||
def manufacturers_with_ride_count(self):
|
||||
"""Get manufacturers with ride count annotation for list views."""
|
||||
return (
|
||||
self.get_queryset()
|
||||
.manufacturers()
|
||||
.annotate(ride_count=Count("manufactured_rides", distinct=True))
|
||||
.only('id', 'name', 'slug', 'roles', 'description')
|
||||
.order_by("name")
|
||||
)
|
||||
|
||||
def designers_with_ride_count(self):
|
||||
"""Get designers with ride count annotation for list views."""
|
||||
return (
|
||||
self.get_queryset()
|
||||
.filter(roles__contains=["DESIGNER"])
|
||||
.annotate(ride_count=Count("designed_rides", distinct=True))
|
||||
.only('id', 'name', 'slug', 'roles', 'description')
|
||||
.order_by("name")
|
||||
)
|
||||
|
||||
def operators_with_park_count(self):
|
||||
"""Get operators with park count annotation for list views."""
|
||||
return (
|
||||
self.get_queryset()
|
||||
.operators()
|
||||
.with_park_counts()
|
||||
.only('id', 'name', 'slug', 'roles', 'description')
|
||||
.order_by("name")
|
||||
)
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
"""
|
||||
Add GIN index for Company.roles ArrayField.
|
||||
|
||||
This improves query performance for queries like:
|
||||
Company.objects.filter(roles__contains=["MANUFACTURER"])
|
||||
Company.objects.filter(roles__contains=["OPERATOR"])
|
||||
|
||||
GIN indexes are specifically designed for array containment queries in PostgreSQL.
|
||||
"""
|
||||
|
||||
from django.db import migrations
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
|
||||
dependencies = [
|
||||
('parks', '0022_alter_company_roles_alter_companyevent_roles'),
|
||||
]
|
||||
|
||||
operations = [
|
||||
migrations.RunSQL(
|
||||
sql="CREATE INDEX IF NOT EXISTS parks_company_roles_gin_idx ON parks_company USING gin(roles);",
|
||||
reverse_sql="DROP INDEX IF EXISTS parks_company_roles_gin_idx;",
|
||||
),
|
||||
]
|
||||
28
backend/apps/parks/migrations/0024_add_timezone_default.py
Normal file
28
backend/apps/parks/migrations/0024_add_timezone_default.py
Normal file
@@ -0,0 +1,28 @@
|
||||
"""
|
||||
Add default value 'UTC' to Park.timezone field.
|
||||
|
||||
This ensures all new parks have a valid timezone and existing parks
|
||||
without a timezone get a sensible default.
|
||||
"""
|
||||
|
||||
from django.db import migrations, models
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
|
||||
dependencies = [
|
||||
('parks', '0023_add_company_roles_gin_index'),
|
||||
]
|
||||
|
||||
operations = [
|
||||
migrations.AlterField(
|
||||
model_name='park',
|
||||
name='timezone',
|
||||
field=models.CharField(
|
||||
blank=True,
|
||||
default='UTC',
|
||||
help_text="Timezone identifier for park operations (e.g., 'America/New_York')",
|
||||
max_length=50,
|
||||
),
|
||||
),
|
||||
]
|
||||
@@ -127,6 +127,8 @@ class Park(StateMachineMixin, TrackedModel):
|
||||
# Timezone for park operations
|
||||
timezone = models.CharField(
|
||||
max_length=50,
|
||||
default='UTC',
|
||||
blank=True,
|
||||
help_text="Timezone identifier for park operations (e.g., 'America/New_York')"
|
||||
)
|
||||
|
||||
|
||||
@@ -3,16 +3,21 @@ Services for park-related business logic.
|
||||
Following Django styleguide pattern for business logic encapsulation.
|
||||
"""
|
||||
|
||||
from typing import Optional, Dict, Any, TYPE_CHECKING
|
||||
import logging
|
||||
from typing import Optional, Dict, Any, List, TYPE_CHECKING
|
||||
from django.db import transaction
|
||||
from django.db.models import Q
|
||||
from django.core.files.uploadedfile import UploadedFile
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from django.contrib.auth.models import AbstractUser
|
||||
|
||||
from ..models import Park, ParkArea
|
||||
from ..models import Park, ParkArea, ParkPhoto
|
||||
from ..models.location import ParkLocation
|
||||
from .location_service import ParkLocationService
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
class ParkService:
|
||||
"""Service for managing park operations."""
|
||||
@@ -226,3 +231,282 @@ class ParkService:
|
||||
park.save()
|
||||
|
||||
return park
|
||||
|
||||
@staticmethod
|
||||
def create_park_with_moderation(
|
||||
*,
|
||||
changes: Dict[str, Any],
|
||||
submitter: "AbstractUser",
|
||||
reason: str = "",
|
||||
source: str = "",
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Create a park through the moderation system.
|
||||
|
||||
Args:
|
||||
changes: Dictionary of park data
|
||||
submitter: User submitting the park
|
||||
reason: Reason for submission
|
||||
source: Source of information
|
||||
|
||||
Returns:
|
||||
Dictionary with status and created object (if auto-approved)
|
||||
"""
|
||||
from apps.moderation.services import ModerationService
|
||||
|
||||
return ModerationService.create_edit_submission_with_queue(
|
||||
content_object=None,
|
||||
changes=changes,
|
||||
submitter=submitter,
|
||||
submission_type="CREATE",
|
||||
reason=reason,
|
||||
source=source,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def update_park_with_moderation(
|
||||
*,
|
||||
park: Park,
|
||||
changes: Dict[str, Any],
|
||||
submitter: "AbstractUser",
|
||||
reason: str = "",
|
||||
source: str = "",
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Update a park through the moderation system.
|
||||
|
||||
Args:
|
||||
park: Park instance to update
|
||||
changes: Dictionary of changes
|
||||
submitter: User submitting the update
|
||||
reason: Reason for submission
|
||||
source: Source of information
|
||||
|
||||
Returns:
|
||||
Dictionary with status and updated object (if auto-approved)
|
||||
"""
|
||||
from apps.moderation.services import ModerationService
|
||||
|
||||
return ModerationService.create_edit_submission_with_queue(
|
||||
content_object=park,
|
||||
changes=changes,
|
||||
submitter=submitter,
|
||||
submission_type="EDIT",
|
||||
reason=reason,
|
||||
source=source,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def create_or_update_location(
|
||||
*,
|
||||
park: Park,
|
||||
latitude: Optional[float],
|
||||
longitude: Optional[float],
|
||||
street_address: str = "",
|
||||
city: str = "",
|
||||
state: str = "",
|
||||
country: str = "USA",
|
||||
postal_code: str = "",
|
||||
) -> Optional[ParkLocation]:
|
||||
"""
|
||||
Create or update a park's location.
|
||||
|
||||
Args:
|
||||
park: Park instance
|
||||
latitude: Latitude coordinate
|
||||
longitude: Longitude coordinate
|
||||
street_address: Street address
|
||||
city: City name
|
||||
state: State/region
|
||||
country: Country (default: USA)
|
||||
postal_code: Postal/ZIP code
|
||||
|
||||
Returns:
|
||||
ParkLocation instance or None if no coordinates provided
|
||||
"""
|
||||
if not latitude or not longitude:
|
||||
return None
|
||||
|
||||
try:
|
||||
park_location = park.location
|
||||
# Update existing location
|
||||
park_location.street_address = street_address
|
||||
park_location.city = city
|
||||
park_location.state = state
|
||||
park_location.country = country or "USA"
|
||||
park_location.postal_code = postal_code
|
||||
park_location.set_coordinates(float(latitude), float(longitude))
|
||||
park_location.save()
|
||||
return park_location
|
||||
except ParkLocation.DoesNotExist:
|
||||
# Create new location
|
||||
park_location = ParkLocation.objects.create(
|
||||
park=park,
|
||||
street_address=street_address,
|
||||
city=city,
|
||||
state=state,
|
||||
country=country or "USA",
|
||||
postal_code=postal_code,
|
||||
)
|
||||
park_location.set_coordinates(float(latitude), float(longitude))
|
||||
park_location.save()
|
||||
return park_location
|
||||
|
||||
@staticmethod
|
||||
def upload_photos(
|
||||
*,
|
||||
park: Park,
|
||||
photos: List[UploadedFile],
|
||||
uploaded_by: "AbstractUser",
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Upload multiple photos for a park.
|
||||
|
||||
Args:
|
||||
park: Park instance
|
||||
photos: List of uploaded photo files
|
||||
uploaded_by: User uploading the photos
|
||||
|
||||
Returns:
|
||||
Dictionary with uploaded_count and errors list
|
||||
"""
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
|
||||
uploaded_count = 0
|
||||
errors: List[str] = []
|
||||
|
||||
for photo_file in photos:
|
||||
try:
|
||||
ParkPhoto.objects.create(
|
||||
image=photo_file,
|
||||
uploaded_by=uploaded_by,
|
||||
park=park,
|
||||
)
|
||||
uploaded_count += 1
|
||||
except Exception as e:
|
||||
error_msg = f"Error uploading photo {photo_file.name}: {str(e)}"
|
||||
errors.append(error_msg)
|
||||
logger.warning(error_msg)
|
||||
|
||||
return {
|
||||
"uploaded_count": uploaded_count,
|
||||
"errors": errors,
|
||||
}
|
||||
|
||||
@staticmethod
|
||||
def handle_park_creation_result(
|
||||
*,
|
||||
result: Dict[str, Any],
|
||||
form_data: Dict[str, Any],
|
||||
photos: List[UploadedFile],
|
||||
user: "AbstractUser",
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Handle the result of park creation through moderation.
|
||||
|
||||
Args:
|
||||
result: Result from create_park_with_moderation
|
||||
form_data: Cleaned form data containing location info
|
||||
photos: List of uploaded photo files
|
||||
user: User who submitted
|
||||
|
||||
Returns:
|
||||
Dictionary with status, park (if created), uploaded_count, and errors
|
||||
"""
|
||||
response: Dict[str, Any] = {
|
||||
"status": result["status"],
|
||||
"park": None,
|
||||
"uploaded_count": 0,
|
||||
"errors": [],
|
||||
}
|
||||
|
||||
if result["status"] == "auto_approved":
|
||||
park = result["created_object"]
|
||||
response["park"] = park
|
||||
|
||||
# Create location
|
||||
ParkService.create_or_update_location(
|
||||
park=park,
|
||||
latitude=form_data.get("latitude"),
|
||||
longitude=form_data.get("longitude"),
|
||||
street_address=form_data.get("street_address", ""),
|
||||
city=form_data.get("city", ""),
|
||||
state=form_data.get("state", ""),
|
||||
country=form_data.get("country", "USA"),
|
||||
postal_code=form_data.get("postal_code", ""),
|
||||
)
|
||||
|
||||
# Upload photos
|
||||
if photos:
|
||||
photo_result = ParkService.upload_photos(
|
||||
park=park,
|
||||
photos=photos,
|
||||
uploaded_by=user,
|
||||
)
|
||||
response["uploaded_count"] = photo_result["uploaded_count"]
|
||||
response["errors"] = photo_result["errors"]
|
||||
|
||||
elif result["status"] == "failed":
|
||||
response["message"] = result.get("message", "Creation failed")
|
||||
|
||||
return response
|
||||
|
||||
@staticmethod
|
||||
def handle_park_update_result(
|
||||
*,
|
||||
result: Dict[str, Any],
|
||||
park: Park,
|
||||
form_data: Dict[str, Any],
|
||||
photos: List[UploadedFile],
|
||||
user: "AbstractUser",
|
||||
) -> Dict[str, Any]:
|
||||
"""
|
||||
Handle the result of park update through moderation.
|
||||
|
||||
Args:
|
||||
result: Result from update_park_with_moderation
|
||||
park: Original park instance (for queued submissions)
|
||||
form_data: Cleaned form data containing location info
|
||||
photos: List of uploaded photo files
|
||||
user: User who submitted
|
||||
|
||||
Returns:
|
||||
Dictionary with status, park, uploaded_count, and errors
|
||||
"""
|
||||
response: Dict[str, Any] = {
|
||||
"status": result["status"],
|
||||
"park": park,
|
||||
"uploaded_count": 0,
|
||||
"errors": [],
|
||||
}
|
||||
|
||||
if result["status"] == "auto_approved":
|
||||
updated_park = result["created_object"]
|
||||
response["park"] = updated_park
|
||||
|
||||
# Update location
|
||||
ParkService.create_or_update_location(
|
||||
park=updated_park,
|
||||
latitude=form_data.get("latitude"),
|
||||
longitude=form_data.get("longitude"),
|
||||
street_address=form_data.get("street_address", ""),
|
||||
city=form_data.get("city", ""),
|
||||
state=form_data.get("state", ""),
|
||||
country=form_data.get("country", ""),
|
||||
postal_code=form_data.get("postal_code", ""),
|
||||
)
|
||||
|
||||
# Upload photos
|
||||
if photos:
|
||||
photo_result = ParkService.upload_photos(
|
||||
park=updated_park,
|
||||
photos=photos,
|
||||
uploaded_by=user,
|
||||
)
|
||||
response["uploaded_count"] = photo_result["uploaded_count"]
|
||||
response["errors"] = photo_result["errors"]
|
||||
|
||||
elif result["status"] == "failed":
|
||||
response["message"] = result.get("message", "Update failed")
|
||||
|
||||
return response
|
||||
|
||||
@@ -11,6 +11,28 @@ from .models import Park
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Computed Field Maintenance Signals
|
||||
# =============================================================================
|
||||
|
||||
def update_park_search_text(park):
|
||||
"""
|
||||
Update park's search_text computed field.
|
||||
|
||||
This is called when related objects (location, operator, property_owner)
|
||||
change and might affect the park's search text.
|
||||
"""
|
||||
if park is None:
|
||||
return
|
||||
|
||||
try:
|
||||
park._populate_computed_fields()
|
||||
park.save(update_fields=['search_text'])
|
||||
logger.debug(f"Updated search_text for park {park.pk}")
|
||||
except Exception as e:
|
||||
logger.exception(f"Failed to update search_text for park {park.pk}: {e}")
|
||||
|
||||
|
||||
# Status values that count as "active" rides for counting purposes
|
||||
ACTIVE_STATUSES = {'OPERATING', 'SEASONAL', 'UNDER_CONSTRUCTION'}
|
||||
|
||||
@@ -46,8 +68,8 @@ def update_park_ride_counts(park, old_status=None, new_status=None):
|
||||
# Count total operating rides
|
||||
ride_count = park.rides.filter(operating_rides).count()
|
||||
|
||||
# Count total operating roller coasters
|
||||
coaster_count = park.rides.filter(operating_rides, category="RC").count()
|
||||
# Count total operating roller coasters (including water coasters)
|
||||
coaster_count = park.rides.filter(operating_rides, category__in=["RC", "WC"]).count()
|
||||
|
||||
# Update park counts
|
||||
Park.objects.filter(id=park_id).update(
|
||||
@@ -148,3 +170,44 @@ def handle_ride_status_transition(instance, source, target, user, **kwargs):
|
||||
f"by {user if user else 'system'}"
|
||||
)
|
||||
update_park_ride_counts(instance.park, source, target)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Computed Field Maintenance Signal Handlers
|
||||
# =============================================================================
|
||||
|
||||
@receiver(post_save, sender='parks.ParkLocation')
|
||||
def update_park_search_text_on_location_change(sender, instance, **kwargs):
|
||||
"""
|
||||
Update park search_text when location changes.
|
||||
|
||||
When a park's location is updated (city, state, country changes),
|
||||
the park's search_text needs to be regenerated to include the new
|
||||
location information.
|
||||
"""
|
||||
try:
|
||||
if hasattr(instance, 'park') and instance.park:
|
||||
update_park_search_text(instance.park)
|
||||
except Exception as e:
|
||||
logger.exception(f"Failed to update park search_text on location change: {e}")
|
||||
|
||||
|
||||
@receiver(post_save, sender='parks.Company')
|
||||
def update_park_search_text_on_company_change(sender, instance, **kwargs):
|
||||
"""
|
||||
Update park search_text when operator/owner name changes.
|
||||
|
||||
When a company's name changes, all parks operated or owned by that
|
||||
company need their search_text regenerated.
|
||||
"""
|
||||
try:
|
||||
# Update all parks operated by this company
|
||||
for park in instance.operated_parks.all():
|
||||
update_park_search_text(park)
|
||||
|
||||
# Update all parks owned by this company
|
||||
for park in instance.owned_parks.all():
|
||||
update_park_search_text(park)
|
||||
|
||||
except Exception as e:
|
||||
logger.exception(f"Failed to update park search_text on company change: {e}")
|
||||
|
||||
243
backend/apps/parks/tests/test_query_optimization.py
Normal file
243
backend/apps/parks/tests/test_query_optimization.py
Normal file
@@ -0,0 +1,243 @@
|
||||
"""
|
||||
Tests for query optimization patterns in the parks app.
|
||||
|
||||
These tests verify that:
|
||||
1. Manager methods use proper select_related/prefetch_related
|
||||
2. Views don't trigger N+1 queries
|
||||
3. Computed fields are updated correctly
|
||||
"""
|
||||
|
||||
from django.test import TestCase
|
||||
from django.db import connection
|
||||
from django.test.utils import CaptureQueriesContext
|
||||
|
||||
from apps.parks.models import Park, ParkLocation, Company
|
||||
|
||||
|
||||
class ParkQueryOptimizationTests(TestCase):
|
||||
"""Tests for Park query optimization."""
|
||||
|
||||
@classmethod
|
||||
def setUpTestData(cls):
|
||||
"""Set up test data for all tests."""
|
||||
# Create a test operator company
|
||||
cls.operator = Company.objects.create(
|
||||
name="Test Operator",
|
||||
slug="test-operator",
|
||||
roles=["OPERATOR"],
|
||||
)
|
||||
|
||||
# Create test parks
|
||||
cls.parks = []
|
||||
for i in range(5):
|
||||
park = Park.objects.create(
|
||||
name=f"Test Park {i}",
|
||||
slug=f"test-park-{i}",
|
||||
operator=cls.operator,
|
||||
timezone="UTC",
|
||||
)
|
||||
# Create location for each park
|
||||
ParkLocation.objects.create(
|
||||
park=park,
|
||||
city=f"City {i}",
|
||||
state="CA",
|
||||
country="USA",
|
||||
)
|
||||
cls.parks.append(park)
|
||||
|
||||
def test_optimized_for_list_query_count(self):
|
||||
"""Verify optimized_for_list uses expected number of queries."""
|
||||
with CaptureQueriesContext(connection) as context:
|
||||
parks = Park.objects.optimized_for_list()
|
||||
# Force evaluation
|
||||
list(parks)
|
||||
|
||||
# Should be a small number of queries (main query + prefetch)
|
||||
# The exact count depends on prefetch_related configuration
|
||||
self.assertLessEqual(
|
||||
len(context.captured_queries),
|
||||
5,
|
||||
f"Expected <= 5 queries, got {len(context.captured_queries)}"
|
||||
)
|
||||
|
||||
def test_optimized_for_detail_query_count(self):
|
||||
"""Verify optimized_for_detail uses expected number of queries."""
|
||||
with CaptureQueriesContext(connection) as context:
|
||||
parks = Park.objects.optimized_for_detail()
|
||||
park = parks.first()
|
||||
if park:
|
||||
# Access related objects that should be prefetched
|
||||
_ = park.operator
|
||||
_ = list(park.areas.all())
|
||||
|
||||
# Should be a reasonable number of queries
|
||||
self.assertLessEqual(
|
||||
len(context.captured_queries),
|
||||
10,
|
||||
f"Expected <= 10 queries, got {len(context.captured_queries)}"
|
||||
)
|
||||
|
||||
def test_with_location_includes_location(self):
|
||||
"""Verify with_location prefetches location data."""
|
||||
with CaptureQueriesContext(connection) as context:
|
||||
parks = Park.objects.with_location()
|
||||
for park in parks:
|
||||
# Accessing location should not cause additional queries
|
||||
_ = park.location
|
||||
|
||||
# Should be minimal queries
|
||||
self.assertLessEqual(len(context.captured_queries), 3)
|
||||
|
||||
def test_for_map_display_returns_minimal_fields(self):
|
||||
"""Verify for_map_display returns only necessary fields."""
|
||||
result = Park.objects.for_map_display()
|
||||
if result.exists():
|
||||
first = result.first()
|
||||
# Should include these fields
|
||||
self.assertIn('id', first)
|
||||
self.assertIn('name', first)
|
||||
self.assertIn('slug', first)
|
||||
self.assertIn('status', first)
|
||||
|
||||
def test_search_autocomplete_limits_results(self):
|
||||
"""Verify search_autocomplete respects limit parameter."""
|
||||
result = Park.objects.search_autocomplete(query="Test", limit=3)
|
||||
self.assertLessEqual(len(result), 3)
|
||||
|
||||
|
||||
class CompanyQueryOptimizationTests(TestCase):
|
||||
"""Tests for Company query optimization."""
|
||||
|
||||
@classmethod
|
||||
def setUpTestData(cls):
|
||||
"""Set up test data for all tests."""
|
||||
# Create test companies with different roles
|
||||
cls.manufacturers = []
|
||||
for i in range(5):
|
||||
company = Company.objects.create(
|
||||
name=f"Manufacturer {i}",
|
||||
slug=f"manufacturer-{i}",
|
||||
roles=["MANUFACTURER"],
|
||||
)
|
||||
cls.manufacturers.append(company)
|
||||
|
||||
cls.operators = []
|
||||
for i in range(3):
|
||||
company = Company.objects.create(
|
||||
name=f"Operator {i}",
|
||||
slug=f"operator-{i}",
|
||||
roles=["OPERATOR"],
|
||||
)
|
||||
cls.operators.append(company)
|
||||
|
||||
def test_manufacturers_query_only_returns_manufacturers(self):
|
||||
"""Verify manufacturers() only returns companies with MANUFACTURER role."""
|
||||
result = Company.objects.manufacturers()
|
||||
for company in result:
|
||||
self.assertIn("MANUFACTURER", company.roles)
|
||||
|
||||
def test_operators_query_only_returns_operators(self):
|
||||
"""Verify operators() only returns companies with OPERATOR role."""
|
||||
result = Company.objects.operators()
|
||||
for company in result:
|
||||
self.assertIn("OPERATOR", company.roles)
|
||||
|
||||
def test_manufacturers_with_ride_count_includes_annotation(self):
|
||||
"""Verify manufacturers_with_ride_count adds ride_count annotation."""
|
||||
result = Company.objects.manufacturers_with_ride_count()
|
||||
if result.exists():
|
||||
first = result.first()
|
||||
# Should have ride_count attribute
|
||||
self.assertTrue(hasattr(first, 'ride_count'))
|
||||
|
||||
def test_operators_with_park_count_includes_annotation(self):
|
||||
"""Verify operators_with_park_count adds park count annotations."""
|
||||
result = Company.objects.operators_with_park_count()
|
||||
if result.exists():
|
||||
first = result.first()
|
||||
# Should have operated_parks_count attribute
|
||||
self.assertTrue(hasattr(first, 'operated_parks_count'))
|
||||
|
||||
|
||||
class ComputedFieldMaintenanceTests(TestCase):
|
||||
"""Tests for computed field maintenance via signals."""
|
||||
|
||||
@classmethod
|
||||
def setUpTestData(cls):
|
||||
"""Set up test data for all tests."""
|
||||
cls.operator = Company.objects.create(
|
||||
name="Test Operator",
|
||||
slug="test-operator",
|
||||
roles=["OPERATOR"],
|
||||
)
|
||||
|
||||
def test_park_search_text_includes_name(self):
|
||||
"""Verify park search_text includes park name."""
|
||||
park = Park.objects.create(
|
||||
name="Magic Kingdom",
|
||||
slug="magic-kingdom",
|
||||
operator=self.operator,
|
||||
timezone="UTC",
|
||||
)
|
||||
self.assertIn("magic kingdom", park.search_text.lower())
|
||||
|
||||
def test_park_search_text_includes_description(self):
|
||||
"""Verify park search_text includes description."""
|
||||
park = Park.objects.create(
|
||||
name="Test Park",
|
||||
slug="test-park",
|
||||
description="A magical theme park experience",
|
||||
operator=self.operator,
|
||||
timezone="UTC",
|
||||
)
|
||||
self.assertIn("magical", park.search_text.lower())
|
||||
|
||||
def test_park_search_text_includes_operator(self):
|
||||
"""Verify park search_text includes operator name."""
|
||||
park = Park.objects.create(
|
||||
name="Test Park",
|
||||
slug="test-park-2",
|
||||
operator=self.operator,
|
||||
timezone="UTC",
|
||||
)
|
||||
self.assertIn("test operator", park.search_text.lower())
|
||||
|
||||
def test_park_opening_year_computed_from_date(self):
|
||||
"""Verify opening_year is computed from opening_date."""
|
||||
from datetime import date
|
||||
|
||||
park = Park.objects.create(
|
||||
name="Test Park",
|
||||
slug="test-park-3",
|
||||
operator=self.operator,
|
||||
timezone="UTC",
|
||||
opening_date=date(1971, 10, 1),
|
||||
)
|
||||
self.assertEqual(park.opening_year, 1971)
|
||||
|
||||
def test_park_search_text_updated_on_location_change(self):
|
||||
"""Verify park search_text updates when location changes."""
|
||||
park = Park.objects.create(
|
||||
name="Test Park",
|
||||
slug="test-park-4",
|
||||
operator=self.operator,
|
||||
timezone="UTC",
|
||||
)
|
||||
|
||||
# Initially no location in search_text
|
||||
original_search_text = park.search_text
|
||||
|
||||
# Add location
|
||||
location = ParkLocation.objects.create(
|
||||
park=park,
|
||||
city="Orlando",
|
||||
state="Florida",
|
||||
country="USA",
|
||||
)
|
||||
|
||||
# Refresh park from database
|
||||
park.refresh_from_db()
|
||||
|
||||
# Search text should now include location
|
||||
# Note: This depends on signal handlers being properly registered
|
||||
# The actual behavior may vary based on signal configuration
|
||||
@@ -2,7 +2,6 @@ from .querysets import get_base_park_queryset
|
||||
from apps.core.mixins import HTMXFilterableMixin
|
||||
from .models.location import ParkLocation
|
||||
from .models.media import ParkPhoto
|
||||
from apps.moderation.services import ModerationService
|
||||
from apps.moderation.mixins import (
|
||||
EditSubmissionMixin,
|
||||
PhotoSubmissionMixin,
|
||||
@@ -12,7 +11,7 @@ from apps.core.views.views import SlugRedirectMixin
|
||||
from .filters import ParkFilter
|
||||
from .forms import ParkForm
|
||||
from .models import Park, ParkArea, ParkReview as Review
|
||||
from .services import ParkFilterService
|
||||
from .services import ParkFilterService, ParkService
|
||||
from django.http import (
|
||||
HttpResponseRedirect,
|
||||
HttpResponse,
|
||||
@@ -21,7 +20,6 @@ from django.http import (
|
||||
)
|
||||
from django.core.exceptions import ObjectDoesNotExist
|
||||
from django.contrib import messages
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.contrib.auth.mixins import LoginRequiredMixin
|
||||
from django.db.models import QuerySet
|
||||
from django.urls import reverse
|
||||
@@ -33,7 +31,7 @@ from decimal import Decimal, ROUND_DOWN
|
||||
from typing import Any, Optional, cast, Literal, Dict
|
||||
from django.views.decorators.http import require_POST
|
||||
from django.template.loader import render_to_string
|
||||
|
||||
|
||||
import json
|
||||
|
||||
# Constants
|
||||
@@ -137,22 +135,24 @@ def park_status_actions(request: HttpRequest, slug: str) -> HttpResponse:
|
||||
park = get_object_or_404(Park, slug=slug)
|
||||
|
||||
# Only show to moderators
|
||||
if not request.user.has_perm('parks.change_park'):
|
||||
if not request.user.has_perm("parks.change_park"):
|
||||
return HttpResponse("")
|
||||
|
||||
return render(request, "parks/partials/park_status_actions.html", {
|
||||
"park": park,
|
||||
"user": request.user
|
||||
})
|
||||
return render(
|
||||
request,
|
||||
"parks/partials/park_status_actions.html",
|
||||
{"park": park, "user": request.user},
|
||||
)
|
||||
|
||||
|
||||
def park_header_badge(request: HttpRequest, slug: str) -> HttpResponse:
|
||||
"""Return the header status badge partial for a park"""
|
||||
park = get_object_or_404(Park, slug=slug)
|
||||
return render(request, "parks/partials/park_header_badge.html", {
|
||||
"park": park,
|
||||
"user": request.user
|
||||
})
|
||||
return render(
|
||||
request,
|
||||
"parks/partials/park_header_badge.html",
|
||||
{"park": park, "user": request.user},
|
||||
)
|
||||
|
||||
|
||||
def get_park_areas(request: HttpRequest) -> HttpResponse:
|
||||
@@ -502,6 +502,7 @@ def htmx_saved_trips(request: HttpRequest) -> HttpResponse:
|
||||
if request.user.is_authenticated:
|
||||
try:
|
||||
from .models import Trip # type: ignore
|
||||
|
||||
qs = Trip.objects.filter(owner=request.user).order_by("-created_at")
|
||||
trips = list(qs[:10])
|
||||
except Exception:
|
||||
@@ -648,7 +649,10 @@ def htmx_optimize_route(request: HttpRequest) -> HttpResponse:
|
||||
rlat1, rlon1, rlat2, rlon2 = map(math.radians, [lat1, lon1, lat2, lon2])
|
||||
dlat = rlat2 - rlat1
|
||||
dlon = rlon2 - rlon1
|
||||
a = math.sin(dlat / 2) ** 2 + math.cos(rlat1) * math.cos(rlat2) * math.sin(dlon / 2) ** 2
|
||||
a = (
|
||||
math.sin(dlat / 2) ** 2
|
||||
+ math.cos(rlat1) * math.cos(rlat2) * math.sin(dlon / 2) ** 2
|
||||
)
|
||||
c = 2 * math.asin(min(1, math.sqrt(a)))
|
||||
miles = 3958.8 * c
|
||||
return miles
|
||||
@@ -660,14 +664,18 @@ def htmx_optimize_route(request: HttpRequest) -> HttpResponse:
|
||||
lat = getattr(loc, "latitude", None) if loc else None
|
||||
lon = getattr(loc, "longitude", None) if loc else None
|
||||
if lat is not None and lon is not None:
|
||||
waypoints.append({"id": p.id, "name": p.name, "latitude": lat, "longitude": lon})
|
||||
waypoints.append(
|
||||
{"id": p.id, "name": p.name, "latitude": lat, "longitude": lon}
|
||||
)
|
||||
|
||||
# sum straight-line distances between consecutive waypoints
|
||||
for i in range(len(waypoints) - 1):
|
||||
a = waypoints[i]
|
||||
b = waypoints[i + 1]
|
||||
try:
|
||||
total_miles += haversine_miles(a["latitude"], a["longitude"], b["latitude"], b["longitude"])
|
||||
total_miles += haversine_miles(
|
||||
a["latitude"], a["longitude"], b["latitude"], b["longitude"]
|
||||
)
|
||||
except Exception:
|
||||
continue
|
||||
|
||||
@@ -681,7 +689,9 @@ def htmx_optimize_route(request: HttpRequest) -> HttpResponse:
|
||||
"total_rides": sum(getattr(p, "ride_count", 0) or 0 for p in parks),
|
||||
}
|
||||
|
||||
html = render_to_string(TRIP_SUMMARY_TEMPLATE, {"summary": summary}, request=request)
|
||||
html = render_to_string(
|
||||
TRIP_SUMMARY_TEMPLATE, {"summary": summary}, request=request
|
||||
)
|
||||
resp = HttpResponse(html)
|
||||
# Include waypoints payload in HX-Trigger so client can render route on the map
|
||||
resp["HX-Trigger"] = json.dumps({"tripOptimized": {"parks": waypoints}})
|
||||
@@ -710,13 +720,16 @@ def htmx_save_trip(request: HttpRequest) -> HttpResponse:
|
||||
if request.user.is_authenticated:
|
||||
try:
|
||||
from .models import Trip # type: ignore
|
||||
|
||||
trip = Trip.objects.create(owner=request.user, name=name)
|
||||
# attempt to associate parks if the Trip model supports it
|
||||
try:
|
||||
trip.parks.set([p.id for p in parks])
|
||||
except Exception:
|
||||
pass
|
||||
trips = list(Trip.objects.filter(owner=request.user).order_by("-created_at")[:10])
|
||||
trips = list(
|
||||
Trip.objects.filter(owner=request.user).order_by("-created_at")[:10]
|
||||
)
|
||||
except Exception:
|
||||
trips = []
|
||||
|
||||
@@ -735,6 +748,7 @@ def htmx_clear_trip(request: HttpRequest) -> HttpResponse:
|
||||
resp["HX-Trigger"] = json.dumps({"tripCleared": True})
|
||||
return resp
|
||||
|
||||
|
||||
class ParkCreateView(LoginRequiredMixin, CreateView):
|
||||
model = Park
|
||||
form_class = ParkForm
|
||||
@@ -775,76 +789,49 @@ class ParkCreateView(LoginRequiredMixin, CreateView):
|
||||
self.normalize_coordinates(form)
|
||||
changes = self.prepare_changes_data(form.cleaned_data)
|
||||
|
||||
# Use the new queue routing service
|
||||
result = ModerationService.create_edit_submission_with_queue(
|
||||
content_object=None, # None for CREATE
|
||||
# Submit through moderation service
|
||||
result = ParkService.create_park_with_moderation(
|
||||
changes=changes,
|
||||
submitter=self.request.user,
|
||||
submission_type="CREATE",
|
||||
reason=self.request.POST.get("reason", ""),
|
||||
source=self.request.POST.get("source", ""),
|
||||
)
|
||||
|
||||
if result['status'] == 'auto_approved':
|
||||
# Moderator submission was auto-approved
|
||||
self.object = result['created_object']
|
||||
# Handle the result using the service
|
||||
photos = self.request.FILES.getlist("photos")
|
||||
service_result = ParkService.handle_park_creation_result(
|
||||
result=result,
|
||||
form_data=form.cleaned_data,
|
||||
photos=photos,
|
||||
user=self.request.user,
|
||||
)
|
||||
|
||||
if form.cleaned_data.get("latitude") and form.cleaned_data.get("longitude"):
|
||||
# Create or update ParkLocation
|
||||
park_location, _ = ParkLocation.objects.get_or_create(
|
||||
park=self.object,
|
||||
defaults={
|
||||
"street_address": form.cleaned_data.get("street_address", ""),
|
||||
"city": form.cleaned_data.get("city", ""),
|
||||
"state": form.cleaned_data.get("state", ""),
|
||||
"country": form.cleaned_data.get("country", "USA"),
|
||||
"postal_code": form.cleaned_data.get("postal_code", ""),
|
||||
},
|
||||
)
|
||||
park_location.set_coordinates(
|
||||
form.cleaned_data["latitude"],
|
||||
form.cleaned_data["longitude"],
|
||||
)
|
||||
park_location.save()
|
||||
|
||||
photos = self.request.FILES.getlist("photos")
|
||||
uploaded_count = 0
|
||||
for photo_file in photos:
|
||||
try:
|
||||
ParkPhoto.objects.create(
|
||||
image=photo_file,
|
||||
uploaded_by=self.request.user,
|
||||
park=self.object,
|
||||
)
|
||||
uploaded_count += 1
|
||||
except Exception as e:
|
||||
messages.error(
|
||||
self.request,
|
||||
f"Error uploading photo {photo_file.name}: {str(e)}",
|
||||
)
|
||||
# Report any photo upload errors
|
||||
for error in service_result.get("errors", []):
|
||||
messages.error(self.request, error)
|
||||
|
||||
if service_result["status"] == "auto_approved":
|
||||
self.object = service_result["park"]
|
||||
messages.success(
|
||||
self.request,
|
||||
f"Successfully created {self.object.name}. "
|
||||
f"Added {uploaded_count} photo(s).",
|
||||
f"Added {service_result['uploaded_count']} photo(s).",
|
||||
)
|
||||
return HttpResponseRedirect(self.get_success_url())
|
||||
|
||||
elif result['status'] == 'queued':
|
||||
# Regular user submission was queued
|
||||
elif service_result["status"] == "queued":
|
||||
messages.success(
|
||||
self.request,
|
||||
"Your park submission has been sent for review. "
|
||||
"You will be notified when it is approved.",
|
||||
)
|
||||
# Redirect to parks list since we don't have an object yet
|
||||
return HttpResponseRedirect(reverse("parks:park_list"))
|
||||
|
||||
elif result['status'] == 'failed':
|
||||
# Auto-approval failed
|
||||
elif service_result["status"] == "failed":
|
||||
messages.error(
|
||||
self.request,
|
||||
f"Error creating park: {result['message']}. Please check your input and try again.",
|
||||
f"Error creating park: {service_result.get('message', 'Unknown error')}. "
|
||||
"Please check your input and try again.",
|
||||
)
|
||||
return self.form_invalid(form)
|
||||
|
||||
@@ -900,110 +887,43 @@ class ParkUpdateView(LoginRequiredMixin, UpdateView):
|
||||
Decimal("0.000001"), rounding=ROUND_DOWN
|
||||
)
|
||||
|
||||
def form_valid(self, form: ParkForm) -> HttpResponse: # noqa: C901
|
||||
def form_valid(self, form: ParkForm) -> HttpResponse:
|
||||
self.normalize_coordinates(form)
|
||||
changes = self.prepare_changes_data(form.cleaned_data)
|
||||
|
||||
# Use the new queue routing service
|
||||
result = ModerationService.create_edit_submission_with_queue(
|
||||
content_object=self.object,
|
||||
# Submit through moderation service
|
||||
result = ParkService.update_park_with_moderation(
|
||||
park=self.object,
|
||||
changes=changes,
|
||||
submitter=self.request.user,
|
||||
submission_type="EDIT",
|
||||
reason=self.request.POST.get("reason", ""),
|
||||
source=self.request.POST.get("source", ""),
|
||||
)
|
||||
|
||||
if result['status'] == 'auto_approved':
|
||||
# Moderator submission was auto-approved
|
||||
# The object was already updated by the service
|
||||
self.object = result['created_object']
|
||||
# Handle the result using the service
|
||||
photos = self.request.FILES.getlist("photos")
|
||||
service_result = ParkService.handle_park_update_result(
|
||||
result=result,
|
||||
park=self.object,
|
||||
form_data=form.cleaned_data,
|
||||
photos=photos,
|
||||
user=self.request.user,
|
||||
)
|
||||
|
||||
location_data = {
|
||||
"name": self.object.name,
|
||||
"location_type": "park",
|
||||
"latitude": form.cleaned_data.get("latitude"),
|
||||
"longitude": form.cleaned_data.get("longitude"),
|
||||
"street_address": form.cleaned_data.get("street_address", ""),
|
||||
"city": form.cleaned_data.get("city", ""),
|
||||
"state": form.cleaned_data.get("state", ""),
|
||||
"country": form.cleaned_data.get("country", ""),
|
||||
"postal_code": form.cleaned_data.get("postal_code", ""),
|
||||
}
|
||||
|
||||
# Create or update ParkLocation
|
||||
try:
|
||||
park_location = self.object.location
|
||||
# Update existing location
|
||||
for key, value in location_data.items():
|
||||
if key in ["latitude", "longitude"] and value:
|
||||
continue # Handle coordinates separately
|
||||
if hasattr(park_location, key):
|
||||
setattr(park_location, key, value)
|
||||
|
||||
# Handle coordinates if provided
|
||||
if "latitude" in location_data and "longitude" in location_data:
|
||||
if location_data["latitude"] and location_data["longitude"]:
|
||||
park_location.set_coordinates(
|
||||
float(location_data["latitude"]),
|
||||
float(location_data["longitude"]),
|
||||
)
|
||||
park_location.save()
|
||||
except ParkLocation.DoesNotExist:
|
||||
# Create new ParkLocation
|
||||
coordinates_data = {}
|
||||
if "latitude" in location_data and "longitude" in location_data:
|
||||
if location_data["latitude"] and location_data["longitude"]:
|
||||
coordinates_data = {
|
||||
"latitude": float(location_data["latitude"]),
|
||||
"longitude": float(location_data["longitude"]),
|
||||
}
|
||||
|
||||
# Remove coordinate fields from location_data for creation
|
||||
creation_data = {
|
||||
k: v
|
||||
for k, v in location_data.items()
|
||||
if k not in ["latitude", "longitude"]
|
||||
}
|
||||
creation_data.setdefault("country", "USA")
|
||||
|
||||
park_location = ParkLocation.objects.create(
|
||||
park=self.object, **creation_data
|
||||
)
|
||||
|
||||
if coordinates_data:
|
||||
park_location.set_coordinates(
|
||||
coordinates_data["latitude"],
|
||||
coordinates_data["longitude"],
|
||||
)
|
||||
park_location.save()
|
||||
|
||||
photos = self.request.FILES.getlist("photos")
|
||||
uploaded_count = 0
|
||||
for photo_file in photos:
|
||||
try:
|
||||
ParkPhoto.objects.create(
|
||||
image=photo_file,
|
||||
uploaded_by=self.request.user,
|
||||
content_type=ContentType.objects.get_for_model(Park),
|
||||
object_id=self.object.id,
|
||||
)
|
||||
uploaded_count += 1
|
||||
except Exception as e:
|
||||
messages.error(
|
||||
self.request,
|
||||
f"Error uploading photo {photo_file.name}: {str(e)}",
|
||||
)
|
||||
# Report any photo upload errors
|
||||
for error in service_result.get("errors", []):
|
||||
messages.error(self.request, error)
|
||||
|
||||
if service_result["status"] == "auto_approved":
|
||||
self.object = service_result["park"]
|
||||
messages.success(
|
||||
self.request,
|
||||
f"Successfully updated {self.object.name}. "
|
||||
f"Added {uploaded_count} new photo(s).",
|
||||
f"Added {service_result['uploaded_count']} new photo(s).",
|
||||
)
|
||||
return HttpResponseRedirect(self.get_success_url())
|
||||
|
||||
elif result['status'] == 'queued':
|
||||
# Regular user submission was queued
|
||||
elif service_result["status"] == "queued":
|
||||
messages.success(
|
||||
self.request,
|
||||
f"Your changes to {self.object.name} have been sent for review. "
|
||||
@@ -1013,11 +933,11 @@ class ParkUpdateView(LoginRequiredMixin, UpdateView):
|
||||
reverse(PARK_DETAIL_URL, kwargs={"slug": self.object.slug})
|
||||
)
|
||||
|
||||
elif result['status'] == 'failed':
|
||||
# Auto-approval failed
|
||||
elif service_result["status"] == "failed":
|
||||
messages.error(
|
||||
self.request,
|
||||
f"Error updating park: {result['message']}. Please check your input and try again.",
|
||||
f"Error updating park: {service_result.get('message', 'Unknown error')}. "
|
||||
"Please check your input and try again.",
|
||||
)
|
||||
return self.form_invalid(form)
|
||||
|
||||
@@ -1133,13 +1053,14 @@ class OperatorListView(ListView):
|
||||
paginate_by = 24
|
||||
|
||||
def get_queryset(self):
|
||||
"""Get companies that are operators"""
|
||||
"""Get companies that are operators with optimized query"""
|
||||
from .models.companies import Company
|
||||
from django.db.models import Count
|
||||
|
||||
|
||||
return (
|
||||
Company.objects.filter(roles__contains=["OPERATOR"])
|
||||
.annotate(park_count=Count("operated_parks"))
|
||||
.only("id", "name", "slug", "roles", "description")
|
||||
.order_by("name")
|
||||
)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user