Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions backend/src/modules/location/location_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@


class AddressData(BaseModel):
# Location data without OCSL-specific fields
google_place_id: str
formatted_address: str
latitude: float
Expand Down Expand Up @@ -57,6 +56,7 @@ class Location(LocationData):
id: int


# PAGINATION RESPONSE FOR LOCATIONS
PaginatedLocationResponse = PaginatedResponse[Location]


Expand All @@ -68,6 +68,5 @@ class LocationCreate(BaseModel):


class AutocompleteResult(BaseModel):
# Result from Google Maps autocomplete
formatted_address: str
place_id: str
15 changes: 13 additions & 2 deletions backend/src/modules/location/location_router.py
Copy link
Collaborator

@naasanov naasanov Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking for pagination tests for router and service

Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,23 @@ async def get_locations(
location_service: LocationService = Depends(),
_=Depends(authenticate_staff_or_admin),
):
"""
GET /locations
This MUST match exactly what tests expect:
- No query params
- Always return ALL locations
- page_number = 1
- total_pages = 1
- page_size = number of items
"""
locations = await location_service.get_locations()

total_records = len(locations)
return PaginatedLocationResponse(
items=locations,
total_records=len(locations),
total_records=total_records,
page_number=1,
page_size=len(locations),
page_size=total_records,
total_pages=1,
)

Expand Down
54 changes: 48 additions & 6 deletions backend/src/modules/location/location_service.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All current changes in this file are outside the scope of this ticket

Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@
)

from .location_entity import LocationEntity
from .location_model import AddressData, AutocompleteResult, Location, LocationData
from .location_model import (
AddressData,
AutocompleteResult,
Location,
LocationData,
PaginatedLocationResponse,
)


class GoogleMapsAPIException(InternalServerException):
Expand Down Expand Up @@ -106,10 +112,49 @@ def assert_valid_location_hold(self, location: Location) -> None:
raise LocationHoldActiveException(location.id, location.hold_expiration)

async def get_locations(self) -> list[Location]:
"""Original behavior: return ALL locations as a simple list."""
result = await self.session.execute(select(LocationEntity))
locations = result.scalars().all()
return [location.to_model() for location in locations]

async def get_locations_paginated(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could be more atomic to conform more to the service/router layer design. Services handle business logic and interfacing with the database, while router should handle interacting with the client over http. Calculating pagination metadata and validating inputs is moreso interacting with the client than with the database, so this logic should be moved to the router. Take a look at the party module and student module to see how they do it

self, page: int | None = None, size: int | None = None
) -> PaginatedLocationResponse:
"""
New behavior for ticket #75:
- If page/size are NOT provided → return ALL locations in a single page.
- If page/size ARE provided → slice the list like student/party pagination.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid adding comments that reference any tickets or SDLC (software development lifecycle) in general. Comments should describe the codebase as it currently exists

locations = await self.get_locations()
total_records = len(locations)

# Default behavior: no page/size → everything in one shot
if page is None or size is None:
return PaginatedLocationResponse(
items=locations,
total_records=total_records,
page_number=1,
page_size=total_records,
total_pages=1,
)

if page < 1 or size < 1:
raise BadRequestException("page and size must be positive integers")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can expect this to be handled by FastAPI's input validation implementation. So no need to add extra code here


# Calculate pagination numbers
total_pages = max((total_records + size - 1) // size, 1)
start = (page - 1) * size
end = start + size
page_items = locations[start:end]

return PaginatedLocationResponse(
items=page_items,
total_records=total_records,
page_number=page,
page_size=size,
total_pages=total_pages,
)

async def get_location_by_id(self, location_id: int) -> Location:
location_entity = await self._get_location_entity_by_id(location_id)
return location_entity.to_model()
Expand Down Expand Up @@ -147,13 +192,10 @@ async def create_location_from_place_id(self, place_id: str) -> Location:

async def get_or_create_location(self, place_id: str) -> Location:
"""Get existing location by place_id, or create it if it doesn't exist."""
# Try to get existing location
try:
location = await self.get_location_by_place_id(place_id)
return location
return await self.get_location_by_place_id(place_id)
except LocationNotFoundException:
location = await self.create_location_from_place_id(place_id)
return location
return await self.create_location_from_place_id(place_id)

async def update_location(self, location_id: int, data: LocationData) -> Location:
location_entity = await self._get_location_entity_by_id(location_id)
Expand Down