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
This commit is contained in:
parent
168b67acee
commit
badb45da59
4 changed files with 324 additions and 50 deletions
244
REFACTOR_PLAN.md
Normal file
244
REFACTOR_PLAN.md
Normal file
|
|
@ -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
|
||||||
|
|
||||||
|
|
@ -1,36 +1,22 @@
|
||||||
"""Audit routes for price history."""
|
"""Audit routes for price history."""
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends
|
from fastapi import APIRouter, Depends
|
||||||
from sqlalchemy import desc, select
|
|
||||||
from sqlalchemy.exc import IntegrityError
|
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
from auth import require_permission
|
from auth import require_permission
|
||||||
from database import get_db
|
from database import get_db
|
||||||
from models import Permission, PriceHistory, User
|
from mappers import PriceHistoryMapper
|
||||||
from price_fetcher import PAIR_BTC_EUR, SOURCE_BITFINEX, fetch_btc_eur_price
|
from models import Permission, User
|
||||||
from schemas import PriceHistoryResponse
|
from schemas import PriceHistoryResponse
|
||||||
|
from services.price import PriceService
|
||||||
|
|
||||||
router = APIRouter(prefix="/api/audit", tags=["audit"])
|
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 Endpoints
|
||||||
# =============================================================================
|
# =============================================================================
|
||||||
|
|
||||||
PRICE_HISTORY_LIMIT = 20
|
|
||||||
|
|
||||||
|
|
||||||
@router.get("/price-history", response_model=list[PriceHistoryResponse])
|
@router.get("/price-history", response_model=list[PriceHistoryResponse])
|
||||||
async def get_price_history(
|
async def get_price_history(
|
||||||
|
|
@ -38,15 +24,10 @@ async def get_price_history(
|
||||||
_current_user: User = Depends(require_permission(Permission.VIEW_AUDIT)),
|
_current_user: User = Depends(require_permission(Permission.VIEW_AUDIT)),
|
||||||
) -> list[PriceHistoryResponse]:
|
) -> list[PriceHistoryResponse]:
|
||||||
"""Get the 20 most recent price history records."""
|
"""Get the 20 most recent price history records."""
|
||||||
query = (
|
service = PriceService(db)
|
||||||
select(PriceHistory)
|
records = await service.get_recent_prices()
|
||||||
.order_by(desc(PriceHistory.timestamp))
|
|
||||||
.limit(PRICE_HISTORY_LIMIT)
|
|
||||||
)
|
|
||||||
result = await db.execute(query)
|
|
||||||
records = result.scalars().all()
|
|
||||||
|
|
||||||
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)
|
@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)),
|
_current_user: User = Depends(require_permission(Permission.FETCH_PRICE)),
|
||||||
) -> PriceHistoryResponse:
|
) -> PriceHistoryResponse:
|
||||||
"""Manually trigger a price fetch from Bitfinex."""
|
"""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(
|
return PriceHistoryMapper.to_response(record)
|
||||||
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)
|
|
||||||
|
|
|
||||||
70
backend/services/price.py
Normal file
70
backend/services/price.py
Normal file
|
|
@ -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
|
||||||
|
|
@ -280,7 +280,7 @@ class TestManualFetch:
|
||||||
existing_id = existing.id
|
existing_id = existing.id
|
||||||
|
|
||||||
# Mock fetch_btc_eur_price to return the same timestamp
|
# 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)
|
mock_fetch.return_value = (95000.0, fixed_timestamp)
|
||||||
|
|
||||||
async with client_factory.create(cookies=admin_user["cookies"]) as authed:
|
async with client_factory.create(cookies=admin_user["cookies"]) as authed:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue