From badb45da590475f00264b1c91ae359d4ba9c766c Mon Sep 17 00:00:00 2001 From: counterweight Date: Thu, 25 Dec 2025 18:30:26 +0100 Subject: [PATCH] Extract price logic to PriceService - Create PriceService with get_recent_prices() and fetch_and_store_price() - Update routes/audit.py to use PriceService instead of direct queries - Use PriceHistoryMapper consistently - Update test to patch services.price.fetch_btc_eur_price --- REFACTOR_PLAN.md | 244 ++++++++++++++++++++++++++++ backend/routes/audit.py | 58 +------ backend/services/price.py | 70 ++++++++ backend/tests/test_price_history.py | 2 +- 4 files changed, 324 insertions(+), 50 deletions(-) create mode 100644 REFACTOR_PLAN.md create mode 100644 backend/services/price.py diff --git a/REFACTOR_PLAN.md b/REFACTOR_PLAN.md new file mode 100644 index 0000000..76cb3ba --- /dev/null +++ b/REFACTOR_PLAN.md @@ -0,0 +1,244 @@ +# Refactoring Plan: Extract Business Logic from Routes + +## Goal +Remove all business/domain logic from route handlers. Routes should only: +1. Receive HTTP requests +2. Call service methods +3. Map responses using mappers +4. Return HTTP responses + +## Current State Analysis + +### Routes with Business Logic + +#### 1. `routes/auth.py` +**Business Logic:** +- `register()`: Invite validation, user creation, invite marking, role assignment +- `get_default_role()`: Database query (should use repository) + +**Action:** Create `AuthService` with: +- `register_user()` - handles entire registration flow +- `login_user()` - handles authentication and token creation + +#### 2. `routes/invites.py` +**Business Logic:** +- `check_invite()`: Invite validation logic +- `get_my_invites()`: Database query + response building +- `create_invite()`: Invite creation with collision retry logic +- `list_all_invites()`: Query building, filtering, pagination +- `revoke_invite()`: Revocation business logic + +**Action:** Use existing `InviteService` (already exists but not fully used): +- Move `check_invite()` logic to `InviteService.check_invite_validity()` +- Move `create_invite()` logic to `InviteService.create_invite()` +- Move `revoke_invite()` logic to `InviteService.revoke_invite()` +- Add `InviteService.get_user_invites()` for `get_my_invites()` +- Add `InviteService.list_invites()` for `list_all_invites()` + +#### 3. `routes/profile.py` +**Business Logic:** +- `get_godfather_email()`: Database query (should use repository) +- `get_profile()`: Data retrieval and response building +- `update_profile()`: Validation and field updates + +**Action:** Create `ProfileService` with: +- `get_profile()` - retrieves profile with godfather email +- `update_profile()` - validates and updates profile fields + +#### 4. `routes/availability.py` +**Business Logic:** +- `get_availability()`: Query, grouping by date, transformation +- `set_availability()`: Slot overlap validation, time ordering validation, deletion, creation +- `copy_availability()`: Source validation, copying logic, atomic transaction handling + +**Action:** Create `AvailabilityService` with: +- `get_availability_for_range()` - gets and groups availability +- `set_availability_for_date()` - validates slots and replaces availability +- `copy_availability()` - copies availability from one date to others + +#### 5. `routes/audit.py` +**Business Logic:** +- `get_price_history()`: Database query +- `fetch_price_now()`: Price fetching, duplicate timestamp handling +- `_to_price_history_response()`: Mapping (should use mapper) + +**Action:** Create `PriceService` with: +- `get_recent_prices()` - gets recent price history +- `fetch_and_store_price()` - fetches from Bitfinex and stores (handles duplicates) +- Move `_to_price_history_response()` to `PriceHistoryMapper` + +#### 6. `routes/exchange.py` +**Business Logic:** +- `get_available_slots()`: Query, slot expansion logic +- Enum validation (acceptable - this is input validation at route level) + +**Action:** +- Move slot expansion logic to `ExchangeService` or `AvailabilityService` +- Keep enum validation in route (it's input validation, not business logic) + +## Implementation Plan + +### Phase 1: Create Missing Services +1. ✅ `ExchangeService` (already exists) +2. ✅ `InviteService` (already exists, needs expansion) +3. ❌ `AuthService` (needs creation) +4. ❌ `ProfileService` (needs creation) +5. ❌ `AvailabilityService` (needs creation) +6. ❌ `PriceService` (needs creation) + +### Phase 2: Expand Existing Services +1. Expand `InviteService`: + - Add `get_user_invites()` + - Add `list_invites()` with pagination + - Ensure all methods use repositories + +### Phase 3: Update Routes to Use Services +1. `routes/auth.py` → Use `AuthService` +2. `routes/invites.py` → Use `InviteService` consistently +3. `routes/profile.py` → Use `ProfileService` +4. `routes/availability.py` → Use `AvailabilityService` +5. `routes/audit.py` → Use `PriceService` +6. `routes/exchange.py` → Move slot expansion to service + +### Phase 4: Clean Up +1. Remove all direct database queries from routes +2. Remove all business logic from routes +3. Replace all `HTTPException` with custom exceptions +4. Ensure all mappers are used consistently +5. Remove helper functions from routes (move to services/repositories) + +## File Structure After Refactoring + +``` +backend/ +├── routes/ +│ ├── auth.py # Only HTTP handling, calls AuthService +│ ├── invites.py # Only HTTP handling, calls InviteService +│ ├── profile.py # Only HTTP handling, calls ProfileService +│ ├── availability.py # Only HTTP handling, calls AvailabilityService +│ ├── audit.py # Only HTTP handling, calls PriceService +│ └── exchange.py # Only HTTP handling, calls ExchangeService +├── services/ +│ ├── __init__.py +│ ├── auth.py # NEW: Registration, login logic +│ ├── invite.py # EXISTS: Expand with missing methods +│ ├── profile.py # NEW: Profile CRUD operations +│ ├── availability.py # NEW: Availability management +│ ├── price.py # NEW: Price fetching and history +│ └── exchange.py # EXISTS: Already good, minor additions +├── repositories/ +│ └── ... (already good) +└── mappers/ + └── ... (add PriceHistoryMapper) +``` + +## Detailed Service Specifications + +### AuthService +```python +class AuthService: + async def register_user( + self, + email: str, + password: str, + invite_identifier: str + ) -> tuple[User, str]: # Returns (user, token) + """Register new user with invite validation.""" + + async def login_user( + self, + email: str, + password: str + ) -> tuple[User, str]: # Returns (user, token) + """Authenticate user and create token.""" +``` + +### ProfileService +```python +class ProfileService: + async def get_profile(self, user: User) -> ProfileResponse: + """Get user profile with godfather email.""" + + async def update_profile( + self, + user: User, + data: ProfileUpdate + ) -> ProfileResponse: + """Validate and update profile fields.""" +``` + +### AvailabilityService +```python +class AvailabilityService: + async def get_availability_for_range( + self, + from_date: date, + to_date: date + ) -> AvailabilityResponse: + """Get availability grouped by date.""" + + async def set_availability_for_date( + self, + target_date: date, + slots: list[TimeSlot] + ) -> AvailabilityDay: + """Validate and set availability for a date.""" + + async def copy_availability( + self, + source_date: date, + target_dates: list[date] + ) -> AvailabilityResponse: + """Copy availability from source to target dates.""" +``` + +### PriceService +```python +class PriceService: + async def get_recent_prices(self, limit: int = 20) -> list[PriceHistory]: + """Get recent price history.""" + + async def fetch_and_store_price(self) -> PriceHistory: + """Fetch price from Bitfinex and store (handles duplicates).""" +``` + +### InviteService (Expansion) +```python +class InviteService: + # Existing methods... + + async def get_user_invites(self, user_id: int) -> list[Invite]: + """Get all invites for a user.""" + + async def list_invites( + self, + page: int, + per_page: int, + status_filter: str | None = None, + godfather_id: int | None = None + ) -> PaginatedInviteRecords: + """List invites with pagination and filtering.""" +``` + +## Testing Strategy +1. Ensure all existing tests pass after each service creation +2. Add service-level unit tests +3. Keep route tests focused on HTTP concerns (status codes, response formats) +4. Move business logic tests to service tests + +## Migration Order +1. **PriceService** (simplest, least dependencies) +2. **AvailabilityService** (self-contained) +3. **ProfileService** (simple CRUD) +4. **AuthService** (more complex, but isolated) +5. **InviteService expansion** (already exists, just expand) +6. **ExchangeService** (slot expansion logic) + +## Success Criteria +- ✅ No `await db.execute()` calls in routes +- ✅ No business validation logic in routes +- ✅ No data transformation logic in routes +- ✅ All routes are thin wrappers around service calls +- ✅ All tests pass +- ✅ Code is more testable and maintainable + diff --git a/backend/routes/audit.py b/backend/routes/audit.py index 0cc725b..30c44b7 100644 --- a/backend/routes/audit.py +++ b/backend/routes/audit.py @@ -1,36 +1,22 @@ """Audit routes for price history.""" from fastapi import APIRouter, Depends -from sqlalchemy import desc, select -from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from auth import require_permission from database import get_db -from models import Permission, PriceHistory, User -from price_fetcher import PAIR_BTC_EUR, SOURCE_BITFINEX, fetch_btc_eur_price +from mappers import PriceHistoryMapper +from models import Permission, User from schemas import PriceHistoryResponse +from services.price import PriceService router = APIRouter(prefix="/api/audit", tags=["audit"]) -def _to_price_history_response(record: PriceHistory) -> PriceHistoryResponse: - return PriceHistoryResponse( - id=record.id, - source=record.source, - pair=record.pair, - price=record.price, - timestamp=record.timestamp, - created_at=record.created_at, - ) - - # ============================================================================= # Price History Endpoints # ============================================================================= -PRICE_HISTORY_LIMIT = 20 - @router.get("/price-history", response_model=list[PriceHistoryResponse]) async def get_price_history( @@ -38,15 +24,10 @@ async def get_price_history( _current_user: User = Depends(require_permission(Permission.VIEW_AUDIT)), ) -> list[PriceHistoryResponse]: """Get the 20 most recent price history records.""" - query = ( - select(PriceHistory) - .order_by(desc(PriceHistory.timestamp)) - .limit(PRICE_HISTORY_LIMIT) - ) - result = await db.execute(query) - records = result.scalars().all() + service = PriceService(db) + records = await service.get_recent_prices() - return [_to_price_history_response(record) for record in records] + return [PriceHistoryMapper.to_response(record) for record in records] @router.post("/price-history/fetch", response_model=PriceHistoryResponse) @@ -55,28 +36,7 @@ async def fetch_price_now( _current_user: User = Depends(require_permission(Permission.FETCH_PRICE)), ) -> PriceHistoryResponse: """Manually trigger a price fetch from Bitfinex.""" - price, timestamp = await fetch_btc_eur_price() + service = PriceService(db) + record = await service.fetch_and_store_price() - record = PriceHistory( - source=SOURCE_BITFINEX, - pair=PAIR_BTC_EUR, - price=price, - timestamp=timestamp, - ) - db.add(record) - - try: - await db.commit() - await db.refresh(record) - except IntegrityError: - # Duplicate timestamp - return the existing record - await db.rollback() - query = select(PriceHistory).where( - PriceHistory.source == SOURCE_BITFINEX, - PriceHistory.pair == PAIR_BTC_EUR, - PriceHistory.timestamp == timestamp, - ) - result = await db.execute(query) - record = result.scalar_one() - - return _to_price_history_response(record) + return PriceHistoryMapper.to_response(record) diff --git a/backend/services/price.py b/backend/services/price.py new file mode 100644 index 0000000..69901c7 --- /dev/null +++ b/backend/services/price.py @@ -0,0 +1,70 @@ +"""Price service for fetching and managing price history.""" + +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from exceptions import ConflictError +from models import PriceHistory +from price_fetcher import PAIR_BTC_EUR, SOURCE_BITFINEX, fetch_btc_eur_price +from repositories.price import PriceRepository + +PRICE_HISTORY_LIMIT = 20 + + +class PriceService: + """Service for price-related business logic.""" + + def __init__(self, db: AsyncSession): + self.db = db + self.price_repo = PriceRepository(db) + + async def get_recent_prices( + self, limit: int = PRICE_HISTORY_LIMIT + ) -> list[PriceHistory]: + """ + Get recent price history records. + + Args: + limit: Maximum number of records to return (default: 20) + + Returns: + List of PriceHistory records, most recent first + """ + return await self.price_repo.get_recent(limit) + + async def fetch_and_store_price(self) -> PriceHistory: + """ + Fetch price from Bitfinex and store it in the database. + + Handles duplicate timestamp conflicts by returning the existing record. + + Returns: + PriceHistory record (newly created or existing if duplicate) + + Raises: + ConflictError: If unable to fetch or store price after retries + """ + price_value, timestamp = await fetch_btc_eur_price() + + record = PriceHistory( + source=SOURCE_BITFINEX, + pair=PAIR_BTC_EUR, + price=price_value, + timestamp=timestamp, + ) + self.db.add(record) + + try: + await self.db.commit() + await self.db.refresh(record) + return record + except IntegrityError: + # Duplicate timestamp - return the existing record + await self.db.rollback() + existing_record = await self.price_repo.get_by_timestamp( + timestamp, SOURCE_BITFINEX, PAIR_BTC_EUR + ) + if existing_record: + return existing_record + # This should not happen, but handle gracefully + raise ConflictError("Failed to fetch or store price") from None diff --git a/backend/tests/test_price_history.py b/backend/tests/test_price_history.py index 4de7b6f..39861bb 100644 --- a/backend/tests/test_price_history.py +++ b/backend/tests/test_price_history.py @@ -280,7 +280,7 @@ class TestManualFetch: existing_id = existing.id # Mock fetch_btc_eur_price to return the same timestamp - with patch("routes.audit.fetch_btc_eur_price") as mock_fetch: + with patch("services.price.fetch_btc_eur_price") as mock_fetch: mock_fetch.return_value = (95000.0, fixed_timestamp) async with client_factory.create(cookies=admin_user["cookies"]) as authed: