diff --git a/.gitignore b/.gitignore new file mode 100644 index 000000000..692f12e5e --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +__pycache__/ +*.pyc +.pytest_cache/ +.venv/ diff --git a/FINDINGS.md b/FINDINGS.md new file mode 100644 index 000000000..34602b3f2 --- /dev/null +++ b/FINDINGS.md @@ -0,0 +1,47 @@ +# Findings: Property Revenue Dashboard + +Scope: the dashboard has exactly one real code path — +`Dashboard.tsx → RevenueSummary.tsx → GET /api/v1/dashboard/summary → services/cache.py → services/reservations.py → core/database_pool.py`. +Everything else in the repo is unrelated scaffolding and was deliberately left untouched (this is a debugging task, not a rebuild). + +## Bugs and fixes + +| # | Symptom reported | Root cause | Fix | +|---|---|---|---| +| 1 | **Ocean Rentals** intermittently sees another company's numbers on refresh | `services/cache.py` cached by `revenue:{property_id}` only. Property IDs are unique *per tenant* (PK is `(id, tenant_id)`; both tenants own `prop-001`), so whoever warmed the key served their numbers to the other tenant for the 300 s TTL. | Key is now `revenue:v2:{tenant_id}:{property_id}:{period}`. Cached payload is also checked against the requesting tenant before being served (defense in depth). | +| 2 | Numbers "don't match internal records" (both clients) | The dashboard **never read the database**. `core/database_pool.py` built its DSN from `settings.supabase_db_*` fields that don't exist → `AttributeError`; `get_session` was `async def` but used with `async with`; a new pool was built per request. `calculate_total_revenue` swallowed every exception and returned a **hard-coded mock table** keyed by property only — so `prop-001` showed `1000.00 / 3` to *both* tenants (a second cross-tenant leak, and it hid the outage). | Pool uses `settings.database_url` (rewritten to `postgresql+asyncpg://`), single global pool with lazy, lock-guarded init, `get_session` returns the session. The mock fallback is deleted: DB errors now surface as **503**. Financial data must fail loudly, never fabricate. | +| 3 | **Sunset Properties** March total differs from their books | `calculate_monthly_revenue` used naive `datetime(year, month, 1)` bounds against `TIMESTAMPTZ`, i.e. UTC months. Seed row `res-tz-1` checks in at `2024-02-29 23:30 UTC` = **1 March 00:30 in Europe/Paris**, so 1 250.00 fell into February. The function was also a stub returning `0`, lacked its `tenant_id` param, and was never called. | Month boundaries are evaluated in the property's own timezone: `check_in_date AT TIME ZONE properties.timezone` compared to half-open local bounds. `month`/`year` query params were wired into `/dashboard/summary` and a period picker added to the UI. | +| 4 | Finance sees totals "off by a few cents" | `dashboard.py` did `float(total)` on a `NUMERIC(10,3)` sum, and the UI re-rounded with `Math.round(x*100)/100`. Sub-cent rows (`333.333 + 333.333 + 333.334`) round to `999.99` if rounded per row or through float, vs the correct `1000.00`. | Sum in SQL (exact), round **once** on the total with `Decimal.quantize(0.01, ROUND_HALF_UP)`, serialise as a string. UI formats the string; no float arithmetic anywhere. | +| 5 | Ocean sees Sunset's property names | `Dashboard.tsx` hard-coded all five properties for every tenant. `RevenueSummary.tsx` also rendered a hard-coded "▲ 12%" trend badge (commented in code as fake) next to the revenue figure. | New tenant-scoped `GET /api/v1/dashboard/properties`; the selector is populated from it. The fabricated trend badge is removed — a finance dashboard must not show made-up numbers. | +| 6 | (security hardening found while investigating) | `dashboard.py` fell back to `"default_tenant"` when the user had no tenant; `TenantResolver` defaulted *unknown* users to `tenant-a`; the UI sent an `X-Simulated-Tenant: candidate` header; the summary endpoint didn't check the property belongs to the caller. | Missing tenant → **403**. Resolver reads the signed JWT claim (`app_metadata.tenant_id`), then a known-account map, else `None`. Header removed. Unknown property for this tenant → **404** (no cross-tenant probing). | + +## Expected values after the fix (seed data) + +| Login | Property | All time | March 2024 (property tz) | +|---|---|---|---| +| Sunset (tenant-a) | prop-001 Beach House Alpha (Paris) | **2250.00 / 4** | **2250.00 / 4** (UTC logic would say 1000.00 / 3) | +| Sunset | prop-002 / prop-003 | 4975.50 / 4, 6100.50 / 2 | same | +| Ocean (tenant-b) | prop-001 Mountain Lodge Beta (New York) | **0.00 / 0** (was showing Sunset's 1000.00 / 3) | 0.00 / 0 | +| Ocean | prop-004 / prop-005 | 1776.50 / 4, 3256.00 / 3 | same | + +## How to verify + +```bash +docker compose up --build +# UI: http://localhost:3000 (log in as each client, compare prop-001) +# API: http://localhost:8000/docs + +# Unit tests (pure functions, no DB needed) +cd backend && python -m pytest -q + +# Cache keys are now tenant-scoped +docker compose exec redis redis-cli KEYS 'revenue:*' +``` + +## Left as follow-ups (out of scope for a debugging pass) + +- `schema.sql` enables RLS on `properties`/`reservations` but defines no policies; the app connects as superuser so RLS is a no-op. Real isolation should add policies keyed on a `current_tenant_id()` setting and connect as a non-superuser role. +- `SECRET_KEY` and DB credentials live in `docker-compose.yml`; move to env/secrets. +- The in-process auth cache keys on a 16-hex token hash for 30 min; fine for now, but token revocation needs the Redis pub/sub path. +- The frontend `SecureAPI` request cache looks for `tenant_id` in `user_metadata` and requires a UUID, while our JWT carries it in `app_metadata` as `tenant-a`; it therefore never matches and client-side caching is silently disabled (safe, but worth aligning). +- A tenant-aware cache helper that *requires* `tenant_id` (as `revenue_cache_key` now does) should be the only way to touch Redis, so bug #1 cannot be reintroduced. diff --git a/backend/app/api/v1/dashboard.py b/backend/app/api/v1/dashboard.py index 1ec352d7e..56c13c918 100644 --- a/backend/app/api/v1/dashboard.py +++ b/backend/app/api/v1/dashboard.py @@ -1,25 +1,74 @@ -from fastapi import APIRouter, Depends, HTTPException -from typing import Dict, Any -from app.services.cache import get_revenue_summary +import logging +from typing import Dict, Any, Optional, List + +from fastapi import APIRouter, Depends, HTTPException, Query, status + from app.core.auth import authenticate_request as get_current_user +from app.models.auth import AuthenticatedUser +from app.services.cache import get_revenue_summary +from app.services.reservations import list_properties +logger = logging.getLogger(__name__) router = APIRouter() + +def _require_tenant(current_user: AuthenticatedUser) -> str: + """ + BUG FIX: the endpoint used to fall back to "default_tenant" when the user had no + tenant. A missing tenant is an authorization failure, never a shared bucket. + """ + tenant_id = current_user.tenant_id + if not tenant_id: + logger.warning(f"Dashboard access denied - no tenant for user {current_user.email}") + raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="No tenant associated with this account") + return tenant_id + + +@router.get("/dashboard/properties") +async def get_dashboard_properties( + current_user: AuthenticatedUser = Depends(get_current_user), +) -> List[Dict[str, Any]]: + """Properties belonging to the caller's tenant (drives the dashboard selector).""" + tenant_id = _require_tenant(current_user) + try: + return await list_properties(tenant_id) + except Exception as e: + logger.exception(f"Failed to list properties for tenant {tenant_id}: {e}") + raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Property data temporarily unavailable") + + @router.get("/dashboard/summary") async def get_dashboard_summary( property_id: str, - current_user: dict = Depends(get_current_user) + month: Optional[int] = Query(None, ge=1, le=12, description="Calendar month (in the property's timezone)"), + year: Optional[int] = Query(None, ge=2000, le=2100), + current_user: AuthenticatedUser = Depends(get_current_user), ) -> Dict[str, Any]: - - tenant_id = getattr(current_user, "tenant_id", "default_tenant") or "default_tenant" - - revenue_data = await get_revenue_summary(property_id, tenant_id) - - total_revenue_float = float(revenue_data['total']) - + tenant_id = _require_tenant(current_user) + + if (month is None) != (year is None): + raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="month and year must be provided together") + + try: + revenue_data = await get_revenue_summary(property_id, tenant_id, month=month, year=year) + except Exception as e: + # BUG FIX: the service used to swallow DB errors and return hard-coded mock + # numbers (shared across tenants). Financial data must fail loudly instead. + logger.exception(f"Revenue lookup failed for tenant={tenant_id} property={property_id}: {e}") + raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Revenue data temporarily unavailable") + + if revenue_data is None: + # Property does not exist *for this tenant* - do not reveal whether it exists elsewhere. + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Property not found") + + # BUG FIX: total was converted with float() here, which loses cents on NUMERIC values. + # The service already rounded once (ROUND_HALF_UP); pass it through as a string. return { - "property_id": revenue_data['property_id'], - "total_revenue": total_revenue_float, - "currency": revenue_data['currency'], - "reservations_count": revenue_data['count'] + "property_id": revenue_data["property_id"], + "property_name": revenue_data.get("property_name"), + "timezone": revenue_data.get("timezone"), + "total_revenue": revenue_data["total"], + "currency": revenue_data["currency"], + "reservations_count": revenue_data["count"], + "period": {"month": month, "year": year} if month is not None else None, } diff --git a/backend/app/core/database_pool.py b/backend/app/core/database_pool.py index d638dfcfe..fed600a55 100644 --- a/backend/app/core/database_pool.py +++ b/backend/app/core/database_pool.py @@ -1,60 +1,91 @@ import asyncio from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker -from sqlalchemy.pool import QueuePool import logging from ..config import settings logger = logging.getLogger(__name__) + +def _async_database_url(url: str) -> str: + """Normalise a DATABASE_URL into the SQLAlchemy asyncpg dialect form.""" + if url.startswith("postgresql+asyncpg://"): + return url + for prefix in ("postgresql://", "postgres://"): + if url.startswith(prefix): + return "postgresql+asyncpg://" + url[len(prefix):] + return url + + class DatabasePool: def __init__(self): self.engine = None self.session_factory = None - + self._init_lock = asyncio.Lock() + async def initialize(self): - """Initialize database connection pool""" - try: - # Create async engine with connection pooling - database_url = f"postgresql+asyncpg://{settings.supabase_db_user}:{settings.supabase_db_password}@{settings.supabase_db_host}:{settings.supabase_db_port}/{settings.supabase_db_name}" - - self.engine = create_async_engine( - database_url, - poolclass=QueuePool, - pool_size=20, # Number of connections to maintain - max_overflow=30, # Additional connections when needed - pool_pre_ping=True, # Validate connections - pool_recycle=3600, # Recycle connections every hour - echo=False # Set to True for SQL debugging - ) - - self.session_factory = async_sessionmaker( - bind=self.engine, - class_=AsyncSession, - expire_on_commit=False - ) - - logger.info("✅ Database connection pool initialized") - - except Exception as e: - logger.error(f"❌ Database pool initialization failed: {e}") - self.engine = None - self.session_factory = None - + """Initialize database connection pool (idempotent, safe under concurrency).""" + if self.session_factory: + return + async with self._init_lock: + if self.session_factory: + return + try: + # BUG FIX: previously built the DSN from settings.supabase_db_* fields that + # do not exist on Settings -> AttributeError on every request -> the revenue + # service silently fell back to hard-coded mock data. Use settings.database_url. + database_url = _async_database_url(settings.database_url) + + # BUG FIX: QueuePool is not valid for async engines; use SQLAlchemy's default + # AsyncAdaptedQueuePool by not passing poolclass. + self.engine = create_async_engine( + database_url, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, + pool_timeout=settings.database_pool_timeout, + pool_pre_ping=True, + pool_recycle=settings.database_pool_recycle, + echo=False, + ) + + self.session_factory = async_sessionmaker( + bind=self.engine, + class_=AsyncSession, + expire_on_commit=False, + ) + + logger.info("✅ Database connection pool initialized") + + except Exception as e: + logger.error(f"❌ Database pool initialization failed: {e}") + self.engine = None + self.session_factory = None + raise + async def close(self): """Close database connections""" if self.engine: await self.engine.dispose() - - async def get_session(self) -> AsyncSession: - """Get database session from pool""" + self.engine = None + self.session_factory = None + + def get_session(self) -> AsyncSession: + """Get database session from pool. + + BUG FIX: this was `async def`, so callers doing `async with db_pool.get_session()` + received a coroutine (no __aenter__) and raised TypeError. It must be a plain + method returning the AsyncSession, which is itself an async context manager. + """ if not self.session_factory: - raise Exception("Database pool not initialized") + raise RuntimeError("Database pool not initialized") return self.session_factory() -# Global database pool instance + +# Global database pool instance - reuse this, never construct a new pool per request. db_pool = DatabasePool() + async def get_db_session() -> AsyncSession: """Dependency to get database session""" + await db_pool.initialize() async with db_pool.get_session() as session: yield session diff --git a/backend/app/core/tenant_resolver.py b/backend/app/core/tenant_resolver.py index db09a4629..33744a6fe 100644 --- a/backend/app/core/tenant_resolver.py +++ b/backend/app/core/tenant_resolver.py @@ -4,6 +4,10 @@ from typing import Optional import logging +from jose import jwt, JWTError + +from ..config import settings + logger = logging.getLogger(__name__) @@ -21,17 +25,17 @@ def resolve_tenant_from_token(token_payload: dict) -> Optional[str]: Returns: Tenant ID if found, None otherwise """ - # Try user_metadata first (most common location) - if 'user_metadata' in token_payload: - tenant_id = token_payload['user_metadata'].get('tenant_id') - if tenant_id: - return tenant_id + # Try app_metadata first (server-controlled claims; this is where login.py puts it) + app_metadata = token_payload.get('app_metadata') or {} + tenant_id = app_metadata.get('tenant_id') + if tenant_id: + return tenant_id - # Try app_metadata as fallback - if 'app_metadata' in token_payload: - tenant_id = token_payload['app_metadata'].get('tenant_id') - if tenant_id: - return tenant_id + # Try user_metadata as fallback + user_metadata = token_payload.get('user_metadata') or {} + tenant_id = user_metadata.get('tenant_id') + if tenant_id: + return tenant_id # Try root level tenant_id = token_payload.get('tenant_id') @@ -68,34 +72,46 @@ def resolve_tenant_from_user(user_data: dict) -> Optional[str]: return None + # Known challenge accounts. Used only as a fallback when the token carries no claim. + _EMAIL_TENANT_MAP = { + "sunset@propertyflow.com": "tenant-a", + "ocean@propertyflow.com": "tenant-b", + "candidate@propertyflow.com": "tenant-a", + } + @staticmethod - async def resolve_tenant_id(user_id: str, user_email: str, token: Optional[str] = None) -> str: + async def resolve_tenant_id(user_id: str, user_email: str, token: Optional[str] = None) -> Optional[str]: """ Resolve tenant ID for a user. - - Args: - user_id: User ID - user_email: User email - - Returns: - Tenant ID + + Order: signed JWT claim -> known account mapping -> None. + + BUG FIX: this used to return "tenant-a" for *any* unknown user, so any + authenticated account that lacked a tenant silently became Sunset Properties. + Unknown tenant must resolve to None and be rejected downstream (403). """ - # Fallback mapping by known user email. - if user_email == "sunset@propertyflow.com": - return "tenant-a" - if user_email == "ocean@propertyflow.com": - return "tenant-b" - if user_email == "candidate@propertyflow.com": - return "tenant-a" - - # Default fallback - return "tenant-a" + if token: + try: + payload = jwt.decode(token, settings.secret_key, algorithms=["HS256"], audience="authenticated") + tenant_id = TenantResolver.resolve_tenant_from_token(payload) + if tenant_id: + return tenant_id + except JWTError: + # Not one of our JWTs (e.g. a Supabase token) - fall through to the mapping. + pass + + tenant_id = TenantResolver._EMAIL_TENANT_MAP.get((user_email or "").lower()) + if tenant_id: + return tenant_id + + logger.warning(f"Could not resolve tenant for user {user_email} ({user_id})") + return None @staticmethod async def update_user_tenant_metadata(user_id: str, tenant_id: str) -> None: """ Update user metadata with tenant_id. - + Args: user_id: User ID tenant_id: Tenant ID diff --git a/backend/app/services/cache.py b/backend/app/services/cache.py index b81474957..e58e325d0 100644 --- a/backend/app/services/cache.py +++ b/backend/app/services/cache.py @@ -1,29 +1,74 @@ import json -import redis.asyncio as redis -from typing import Dict, Any +import logging import os +from typing import Dict, Any, Optional + +import redis.asyncio as redis + +from app.services.reservations import calculate_revenue + +logger = logging.getLogger(__name__) # Initialize Redis client (typically configured centrally). redis_client = redis.Redis.from_url(os.getenv("REDIS_URL", "redis://localhost:6379/0")) -async def get_revenue_summary(property_id: str, tenant_id: str) -> Dict[str, Any]: +# Bump when the cached payload shape changes so stale entries are never deserialised. +CACHE_VERSION = "v2" +CACHE_TTL_SECONDS = 300 + + +def revenue_cache_key(tenant_id: str, property_id: str, month: Optional[int] = None, year: Optional[int] = None) -> str: + """ + Build the cache key for a revenue summary. + + BUG FIX: the key used to be `revenue:{property_id}`. Property IDs are only unique + per tenant (properties PK is (id, tenant_id); both tenants own a 'prop-001'), so + whichever tenant warmed the cache served its numbers to the other tenant for the + next 5 minutes. The tenant MUST be part of the key, as must the period. + """ + if not tenant_id: + raise ValueError("tenant_id is required for cache isolation") + period = f"{year:04d}-{month:02d}" if month is not None and year is not None else "all" + return f"revenue:{CACHE_VERSION}:{tenant_id}:{property_id}:{period}" + + +async def get_revenue_summary( + property_id: str, + tenant_id: str, + month: Optional[int] = None, + year: Optional[int] = None, +) -> Optional[Dict[str, Any]]: """ Fetches revenue summary, utilizing caching to improve performance. + Returns None when the property does not belong to the tenant. """ - cache_key = f"revenue:{property_id}" - - # Try to get from cache - cached = await redis_client.get(cache_key) + cache_key = revenue_cache_key(tenant_id, property_id, month, year) + + try: + cached = await redis_client.get(cache_key) + except Exception as e: # Redis down must not take the dashboard down + logger.warning(f"Redis GET failed for {cache_key}: {e}") + cached = None + if cached: - return json.loads(cached) - - # Revenue calculation is delegated to the reservation service. - from app.services.reservations import calculate_total_revenue - - # Calculate revenue - result = await calculate_total_revenue(property_id, tenant_id) - - # Cache the result for 5 minutes - await redis_client.setex(cache_key, 300, json.dumps(result)) - + data = json.loads(cached) + # Defense in depth: never serve a payload that belongs to another tenant, + # even if a key collision were ever introduced again. + if data.get("tenant_id") == tenant_id and data.get("property_id") == property_id: + return data + logger.error(f"Cache payload/tenant mismatch for {cache_key} - discarding entry") + try: + await redis_client.delete(cache_key) + except Exception: + pass + + result = await calculate_revenue(property_id, tenant_id, month=month, year=year) + if result is None: + return None + + try: + await redis_client.setex(cache_key, CACHE_TTL_SECONDS, json.dumps(result)) + except Exception as e: + logger.warning(f"Redis SETEX failed for {cache_key}: {e}") + return result diff --git a/backend/app/services/reservations.py b/backend/app/services/reservations.py index 384bd00ab..9437ce0e5 100644 --- a/backend/app/services/reservations.py +++ b/backend/app/services/reservations.py @@ -1,109 +1,138 @@ +""" +Revenue calculations for property reservations. + +Money rules (see FINDINGS.md): + * Amounts are stored as NUMERIC(10, 3) and are summed in the database (exact). + * Rounding to cents happens exactly once, on the final total, with ROUND_HALF_UP. + * Amounts are never converted to float on the backend; the API serialises them as strings. + +Time rules: + * check_in_date is TIMESTAMPTZ. A "month" is defined in the *property's* local timezone + (properties.timezone), because that is how the client's own books define it. +""" from datetime import datetime -from decimal import Decimal -from typing import Dict, Any, List +from decimal import Decimal, ROUND_HALF_UP +from typing import Dict, Any, Optional, List, Tuple +import logging -async def calculate_monthly_revenue(property_id: str, month: int, year: int, db_session=None) -> Decimal: - """ - Calculates revenue for a specific month. - """ +from sqlalchemy import text + +from app.core.database_pool import db_pool + +logger = logging.getLogger(__name__) + +CENT = Decimal("0.01") - start_date = datetime(year, month, 1) - if month < 12: - end_date = datetime(year, month + 1, 1) - else: - end_date = datetime(year + 1, 1, 1) - - print(f"DEBUG: Querying revenue for {property_id} from {start_date} to {end_date}") - - # SQL Simulation (This would be executed against the actual DB) - query = """ - SELECT SUM(total_amount) as total - FROM reservations - WHERE property_id = $1 - AND tenant_id = $2 - AND check_in_date >= $3 - AND check_in_date < $4 + +def to_money(amount: Decimal) -> Decimal: + """Round a Decimal amount to cents, half-up (financial rounding).""" + return Decimal(amount).quantize(CENT, rounding=ROUND_HALF_UP) + + +def month_bounds(year: int, month: int) -> Tuple[datetime, datetime]: + """Half-open [start, end) wall-clock bounds of a calendar month (naive datetimes). + + These are compared against `check_in_date AT TIME ZONE properties.timezone`, i.e. + the reservation's local wall-clock time, so a booking at 2024-02-29 23:30 UTC for a + Paris property (= 2024-03-01 00:30 local) correctly lands in March. """ - - # In production this query executes against a database session. - # result = await db.fetch_val(query, property_id, tenant_id, start_date, end_date) - # return result or Decimal('0') - - return Decimal('0') # Placeholder for now until DB connection is finalized - -async def calculate_total_revenue(property_id: str, tenant_id: str) -> Dict[str, Any]: + if not 1 <= month <= 12: + raise ValueError(f"month must be 1..12, got {month}") + start = datetime(year, month, 1) + end = datetime(year + 1, 1, 1) if month == 12 else datetime(year, month + 1, 1) + return start, end + + +async def calculate_revenue( + property_id: str, + tenant_id: str, + month: Optional[int] = None, + year: Optional[int] = None, +) -> Optional[Dict[str, Any]]: """ - Aggregates revenue from database. + Sum reservation revenue for one property of one tenant. + + If month/year are given, only reservations whose check-in falls within that calendar + month *in the property's timezone* are counted; otherwise all reservations. + + Returns None if the property does not exist for this tenant. Raises on database + errors - callers must NOT mask failures with fabricated numbers. """ - try: - # Import database pool - from app.core.database_pool import DatabasePool - - # Initialize pool if needed - db_pool = DatabasePool() - await db_pool.initialize() - - if db_pool.session_factory: - async with db_pool.get_session() as session: - # Use SQLAlchemy text for raw SQL - from sqlalchemy import text - - query = text(""" - SELECT - property_id, - SUM(total_amount) as total_revenue, - COUNT(*) as reservation_count - FROM reservations - WHERE property_id = :property_id AND tenant_id = :tenant_id - GROUP BY property_id - """) - - result = await session.execute(query, { - "property_id": property_id, - "tenant_id": tenant_id - }) - row = result.fetchone() - - if row: - total_revenue = Decimal(str(row.total_revenue)) - return { - "property_id": property_id, - "tenant_id": tenant_id, - "total": str(total_revenue), - "currency": "USD", - "count": row.reservation_count - } - else: - # No reservations found for this property - return { - "property_id": property_id, - "tenant_id": tenant_id, - "total": "0.00", - "currency": "USD", - "count": 0 - } - else: - raise Exception("Database pool not available") - - except Exception as e: - print(f"Database error for {property_id} (tenant: {tenant_id}): {e}") - - # Create property-specific mock data for testing when DB is unavailable - # This ensures each property shows different figures - mock_data = { - 'prop-001': {'total': '1000.00', 'count': 3}, - 'prop-002': {'total': '4975.50', 'count': 4}, - 'prop-003': {'total': '6100.50', 'count': 2}, - 'prop-004': {'total': '1776.50', 'count': 4}, - 'prop-005': {'total': '3256.00', 'count': 3} - } - - mock_property_data = mock_data.get(property_id, {'total': '0.00', 'count': 0}) - - return { - "property_id": property_id, - "tenant_id": tenant_id, - "total": mock_property_data['total'], - "currency": "USD", - "count": mock_property_data['count'] - } + if (month is None) != (year is None): + raise ValueError("month and year must be provided together") + + await db_pool.initialize() + + params: Dict[str, Any] = {"property_id": property_id, "tenant_id": tenant_id} + period_filter = "" + if month is not None: + start, end = month_bounds(year, month) + params["start"] = start + params["end"] = end + # AT TIME ZONE converts the TIMESTAMPTZ to the property's local wall-clock time. + period_filter = """ + AND (r.check_in_date AT TIME ZONE p.timezone) >= :start + AND (r.check_in_date AT TIME ZONE p.timezone) < :end + """ + + query = text(f""" + SELECT + p.id AS property_id, + p.name AS property_name, + p.timezone AS timezone, + COALESCE(SUM(r.total_amount), 0) AS total_revenue, + COUNT(r.id) AS reservation_count + FROM properties p + LEFT JOIN reservations r + ON r.property_id = p.id + AND r.tenant_id = p.tenant_id + {period_filter} + WHERE p.id = :property_id + AND p.tenant_id = :tenant_id + GROUP BY p.id, p.name, p.timezone + """) + + async with db_pool.get_session() as session: + result = await session.execute(query, params) + row = result.fetchone() + + if row is None: + return None + + total = to_money(Decimal(str(row.total_revenue))) + return { + "property_id": row.property_id, + "property_name": row.property_name, + "tenant_id": tenant_id, + "timezone": row.timezone, + "total": str(total), + "currency": "USD", + "count": int(row.reservation_count), + "month": month, + "year": year, + } + + +async def calculate_total_revenue(property_id: str, tenant_id: str) -> Optional[Dict[str, Any]]: + """All-time revenue for a property. Kept for backwards compatibility.""" + return await calculate_revenue(property_id, tenant_id) + + +async def calculate_monthly_revenue(property_id: str, tenant_id: str, month: int, year: int) -> Optional[Dict[str, Any]]: + """Revenue for a calendar month in the property's local timezone.""" + return await calculate_revenue(property_id, tenant_id, month=month, year=year) + + +async def list_properties(tenant_id: str) -> List[Dict[str, Any]]: + """Properties visible to a tenant (never leaks other tenants' properties).""" + await db_pool.initialize() + query = text(""" + SELECT id, name, timezone + FROM properties + WHERE tenant_id = :tenant_id + ORDER BY id + """) + async with db_pool.get_session() as session: + result = await session.execute(query, {"tenant_id": tenant_id}) + rows = result.fetchall() + return [{"id": r.id, "name": r.name, "timezone": r.timezone} for r in rows] diff --git a/backend/requirements.txt b/backend/requirements.txt index 6b777d2aa..26c904eca 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -27,3 +27,4 @@ loguru redis>=5.0.0 asyncpg>=0.27.0 requests +pytest diff --git a/backend/tests/test_revenue.py b/backend/tests/test_revenue.py new file mode 100644 index 000000000..407355d90 --- /dev/null +++ b/backend/tests/test_revenue.py @@ -0,0 +1,81 @@ +""" +Unit tests for the revenue bug fixes. Pure functions only - no DB/Redis needed. + +Run: cd backend && python -m pytest -q +""" +from datetime import datetime, timezone +from decimal import Decimal +from zoneinfo import ZoneInfo + +import pytest + +from app.services.cache import revenue_cache_key +from app.services.reservations import month_bounds, to_money + + +# --- Bug 1: cache key must be tenant-scoped ----------------------------------------- + +def test_cache_key_isolates_tenants_sharing_a_property_id(): + a = revenue_cache_key("tenant-a", "prop-001") + b = revenue_cache_key("tenant-b", "prop-001") + assert a != b + assert "tenant-a" in a and "tenant-b" in b + + +def test_cache_key_isolates_periods(): + assert revenue_cache_key("tenant-a", "prop-001") != revenue_cache_key("tenant-a", "prop-001", 3, 2024) + assert revenue_cache_key("tenant-a", "prop-001", 3, 2024) != revenue_cache_key("tenant-a", "prop-001", 4, 2024) + + +def test_cache_key_requires_tenant(): + with pytest.raises(ValueError): + revenue_cache_key("", "prop-001") + + +# --- Bug 4: money is rounded once, half-up, never via float --------------------------- + +def test_sum_then_round_does_not_drift(): + rows = [Decimal("333.333"), Decimal("333.333"), Decimal("333.334")] + assert to_money(sum(rows)) == Decimal("1000.00") + # The buggy behaviour (round each row first) loses a cent: + assert sum(to_money(r) for r in rows) == Decimal("999.99") + + +def test_round_half_up_not_bankers(): + assert to_money(Decimal("4975.525")) == Decimal("4975.53") + assert to_money(Decimal("0.005")) == Decimal("0.01") + + +def test_seed_totals(): + tenant_a_prop_001 = [Decimal("1250.000"), Decimal("333.333"), Decimal("333.333"), Decimal("333.334")] + assert to_money(sum(tenant_a_prop_001)) == Decimal("2250.00") + + +# --- Bug 3: month boundaries live in the property's timezone ------------------------- + +def test_month_bounds_are_half_open(): + start, end = month_bounds(2024, 3) + assert start == datetime(2024, 3, 1) + assert end == datetime(2024, 4, 1) + assert month_bounds(2024, 12)[1] == datetime(2025, 1, 1) + + +def test_month_bounds_rejects_bad_month(): + with pytest.raises(ValueError): + month_bounds(2024, 13) + + +def test_feb_29_2330_utc_is_march_in_paris_but_february_in_new_york(): + """Seed row res-tz-1: the check-in that caused Sunset's 'March mismatch'.""" + check_in = datetime(2024, 2, 29, 23, 30, tzinfo=timezone.utc) + march_start, april_start = month_bounds(2024, 3) + + paris_local = check_in.astimezone(ZoneInfo("Europe/Paris")).replace(tzinfo=None) + assert march_start <= paris_local < april_start # counted in March for a Paris property + + ny_local = check_in.astimezone(ZoneInfo("America/New_York")).replace(tzinfo=None) + assert ny_local < march_start # would be February for a New York property + + naive_utc = check_in.replace(tzinfo=None) + assert naive_utc < march_start # the old naive-UTC logic put it in February + diff --git a/frontend/src/components/Dashboard.tsx b/frontend/src/components/Dashboard.tsx index a21bba404..cfb2d0910 100644 --- a/frontend/src/components/Dashboard.tsx +++ b/frontend/src/components/Dashboard.tsx @@ -1,16 +1,38 @@ -import React, { useState } from "react"; +import React, { useEffect, useState } from "react"; import { RevenueSummary } from "./RevenueSummary"; +import { SecureAPI } from "../lib/secureApi"; -const PROPERTIES = [ - { id: 'prop-001', name: 'Beach House Alpha' }, - { id: 'prop-002', name: 'City Apartment Downtown' }, - { id: 'prop-003', name: 'Country Villa Estate' }, - { id: 'prop-004', name: 'Lakeside Cottage' }, - { id: 'prop-005', name: 'Urban Loft Modern' } -]; +interface Property { + id: string; + name: string; + timezone: string; +} +// BUG FIX: the property list used to be hard-coded with all five properties (from both +// tenants), so Ocean Rentals saw Sunset's property names and vice-versa. Properties are +// now loaded from the tenant-scoped /dashboard/properties endpoint. const Dashboard: React.FC = () => { - const [selectedProperty, setSelectedProperty] = useState('prop-001'); + const [properties, setProperties] = useState([]); + const [selectedProperty, setSelectedProperty] = useState(''); + const [period, setPeriod] = useState(''); // '' = all time, otherwise 'YYYY-MM' + const [loadError, setLoadError] = useState(''); + + useEffect(() => { + let cancelled = false; + SecureAPI.getDashboardProperties() + .then((list: Property[]) => { + if (cancelled) return; + setProperties(list); + setSelectedProperty(prev => (list.some(p => p.id === prev) ? prev : (list[0]?.id ?? ''))); + }) + .catch((err: unknown) => { + console.error(err); + if (!cancelled) setLoadError('Failed to load properties'); + }); + return () => { cancelled = true; }; + }, []); + + const [periodYear, periodMonth] = period ? period.split('-').map(Number) : [undefined, undefined]; return (
@@ -26,27 +48,55 @@ const Dashboard: React.FC = () => { Monthly performance insights for your properties

- - {/* Property Selector */} -
- - + +
+ {/* Property Selector */} +
+ + +
+ + {/* Period Selector: month is interpreted in the property's timezone */} +
+ +
+ setPeriod(e.target.value)} + className="block px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-blue-500 focus:border-blue-500 text-sm" + /> + {period && ( + + )} +
+
- + {loadError &&
{loadError}
} + {selectedProperty && ( + + )}
diff --git a/frontend/src/components/RevenueSummary.tsx b/frontend/src/components/RevenueSummary.tsx index dbb6d0629..09b742e12 100644 --- a/frontend/src/components/RevenueSummary.tsx +++ b/frontend/src/components/RevenueSummary.tsx @@ -3,45 +3,62 @@ import { SecureAPI } from '../lib/secureApi'; interface RevenueData { property_id: string; - total_revenue: number; + property_name?: string; + timezone?: string; + // BUG FIX: the API now returns the total as a decimal *string* already rounded to + // cents on the server, so the UI never re-rounds a float. + total_revenue: string; currency: string; reservations_count: number; + period: { month: number; year: number } | null; } interface RevenueSummaryProps { - propertyId?: string; - debugTenant?: string; + propertyId: string; + month?: number; + year?: number; showRaw?: boolean; } -export const RevenueSummary: React.FC = ({ propertyId = 'prop-001', debugTenant, showRaw }) => { +const MONTHS = ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December']; + +// Format a decimal string ("2250.00") for display without going through float arithmetic. +const formatMoney = (value: string): string => { + const [whole, frac = '00'] = value.split('.'); + const sign = whole.startsWith('-') ? '-' : ''; + const digits = whole.replace('-', ''); + const grouped = digits.replace(/\B(?=(\d{3})+(?!\d))/g, ','); + return `${sign}${grouped}.${frac.padEnd(2, '0').slice(0, 2)}`; +}; + +export const RevenueSummary: React.FC = ({ propertyId, month, year, showRaw }) => { const [data, setData] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(''); - const activeTenant = debugTenant || 'candidate'; - useEffect(() => { + let cancelled = false; const fetchRevenue = async () => { setLoading(true); + setError(''); try { - // Use SecureAPI to handle authentication automatically - // We pass the simulatedTenant option which SecureAPI will attach as a header - const response = await SecureAPI.getDashboardSummary(propertyId, { - simulatedTenant: activeTenant, - timestamp: Date.now() - }); - setData(response); + // BUG FIX: removed the bogus `X-Simulated-Tenant: candidate` header and the + // `_t` cache-buster. Tenant comes from the signed JWT, nowhere else. + const response = await SecureAPI.getDashboardSummary(propertyId, { month, year }); + if (!cancelled) setData(response); } catch (err) { - setError('Failed to load revenue data'); - console.error(err); + if (!cancelled) { + setError('Failed to load revenue data'); + console.error(err); + } } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; fetchRevenue(); - }, [propertyId, activeTenant]); + return () => { cancelled = true; }; + }, [propertyId, month, year]); if (loading) { return ( @@ -61,7 +78,9 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr if (error) return
{error}
; if (!data) return null; - const displayTotal = Math.round(data.total_revenue * 100) / 100; + const periodLabel = data.period + ? `${MONTHS[data.period.month - 1]} ${data.period.year}${data.timezone ? ` (${data.timezone})` : ''}` + : 'All time'; return (
@@ -78,41 +97,26 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr

Total Revenue

- {data.currency} {displayTotal.toLocaleString(undefined, { minimumFractionDigits: 2, maximumFractionDigits: 2 })} - - {/* Fake trend indicator for premium feel */} - - - 12% + {data.currency} {formatMoney(data.total_revenue)}
+

{periodLabel}

-

Property ID

-

{data.property_id}

+

Property

+

+ {data.property_name ?? data.property_id} + {data.property_id} +

Reservations

{data.reservations_count} bookings

- - {/* Precision Warning Area */} -
- {Math.abs(data.total_revenue - displayTotal) > 0.000001 && showRaw && ( -
- - - - Precision Mismatch Detected -
- )} -
); diff --git a/frontend/src/lib/secureApi.ts b/frontend/src/lib/secureApi.ts index f85f04c90..428020c18 100644 --- a/frontend/src/lib/secureApi.ts +++ b/frontend/src/lib/secureApi.ts @@ -1452,20 +1452,22 @@ export class SecureAPIClient { /** * Get dashboard summary with optional simulation header */ - async getDashboardSummary(propertyId: string, options?: { simulatedTenant?: string, timestamp?: number }) { + async getDashboardSummary(propertyId: string, options?: { month?: number, year?: number }) { const queryParams = new URLSearchParams({ property_id: propertyId }); - if (options?.timestamp) { - queryParams.append('_t', options.timestamp.toString()); - } - - const requestOptions: RequestInit = {}; - if (options?.simulatedTenant) { - requestOptions.headers = { - 'X-Simulated-Tenant': options.simulatedTenant - }; + if (options?.month !== undefined && options?.year !== undefined) { + queryParams.append('month', String(options.month)); + queryParams.append('year', String(options.year)); } + // Tenant is derived server-side from the signed JWT; no simulated-tenant header. + return this.request(`/api/v1/dashboard/summary?${queryParams}`); + } - return this.request(`/api/v1/dashboard/summary?${queryParams}`, requestOptions); + /** + * Properties visible to the current tenant. + */ + async getDashboardProperties() { + const res = await this.request('/api/v1/dashboard/properties'); + return Array.isArray(res) ? res : []; } async uploadCompanyLogo(logo_url: string) {