From 0751e990833b263c1a240c54f4ae9cdf0059f0b6 Mon Sep 17 00:00:00 2001 From: Rahul Trivedi Date: Thu, 30 Jul 2026 10:21:23 -0700 Subject: [PATCH 1/2] Fix revenue dashboard: tenant leakage, timezone months, money precision The dashboard was never reaching the database: DatabasePool built its DSN from settings fields that do not exist, passed a sync QueuePool to create_async_engine, and exposed get_session() as a coroutine that callers used with `async with`. Every failure was caught and answered with a hardcoded "mock data for testing" dictionary, so clients were shown invented revenue. Point the pool at DATABASE_URL, make the session usable, reuse the singleton, and delete the fallback - a revenue endpoint that cannot reach its data must fail, not improvise. Tenant isolation: the revenue cache key was `revenue:{property_id}`, but property IDs are only unique within a tenant (both clients own a prop-001), so whichever tenant missed the cache first served its revenue to the other for the next five minutes. Key on tenant + property + period, verify the cached payload belongs to the caller, and fall through to the database when Redis is unavailable. The endpoint also defaulted an unresolved tenant to a shared "default_tenant" bucket and never checked property ownership; there is now no default tenant, and another tenant's property returns 404. Timezone: month boundaries were naive UTC datetimes compared against timestamptz check-ins, shifting every month by the property's UTC offset. A 2024-02-29 23:30 UTC check-in is 00:30 on 1 March in Paris, so Sunset's March was short 1250.00 and one booking. Build the bounds in the property's own timezone and convert to UTC; report the timezone and period used. Precision: money went Decimal -> float -> JSON number -> JS double -> Math.round, so halfway amounts rounded the wrong way. Keep the sum in Decimal, round once with ROUND_HALF_UP at the API edge, and transport it as a string alongside the exact value. Also: property list is now fetched per tenant instead of hardcoding every client's portfolio, the client-supplied X-Simulated-Tenant header is gone, getTenantId reads the app_metadata claim and no longer deletes a working session over an ID format, a stale-response race in RevenueSummary is fixed, and the hardcoded "+12%" trend badge is removed. Verified by scripts/verify_fixes.sh (19 end-to-end checks against both client logins) and backend/tests/test_revenue_fixes.py (17 unit tests). --- .gitignore | 6 + backend/app/api/v1/dashboard.py | 74 +++++-- backend/app/core/database_pool.py | 100 ++++++--- backend/app/services/cache.py | 81 +++++-- backend/app/services/reservations.py | 246 +++++++++++++-------- backend/requirements.txt | 1 + backend/tests/test_revenue_fixes.py | 128 +++++++++++ frontend/src/components/Dashboard.tsx | 112 +++++++--- frontend/src/components/RevenueSummary.tsx | 79 ++++--- frontend/src/lib/secureApi.ts | 50 +++-- scripts/verify_fixes.sh | 104 +++++++++ 11 files changed, 749 insertions(+), 232 deletions(-) create mode 100644 .gitignore create mode 100644 backend/tests/test_revenue_fixes.py create mode 100755 scripts/verify_fixes.sh diff --git a/.gitignore b/.gitignore new file mode 100644 index 000000000..bb5245651 --- /dev/null +++ b/.gitignore @@ -0,0 +1,6 @@ +__pycache__/ +*.py[cod] +.pytest_cache/ +.venv/ +node_modules/ +.DS_Store diff --git a/backend/app/api/v1/dashboard.py b/backend/app/api/v1/dashboard.py index 1ec352d7e..5dbfb493a 100644 --- a/backend/app/api/v1/dashboard.py +++ b/backend/app/api/v1/dashboard.py @@ -1,25 +1,73 @@ -from fastapi import APIRouter, Depends, HTTPException -from typing import Dict, Any +from fastapi import APIRouter, Depends, HTTPException, Query +from typing import Dict, Any, Optional +from decimal import Decimal, ROUND_HALF_UP +import logging + from app.services.cache import get_revenue_summary +from app.services.reservations import PropertyNotFound, list_tenant_properties from app.core.auth import authenticate_request as get_current_user router = APIRouter() +logger = logging.getLogger(__name__) + +CENTS = Decimal("0.01") + + +def _require_tenant(current_user) -> str: + """Resolve the caller's tenant from their authenticated identity. + + There is deliberately no default tenant: a placeholder like + "default_tenant" would put every client into one shared bucket. + """ + tenant_id = getattr(current_user, "tenant_id", None) + if not tenant_id: + logger.error(f"Authenticated user {getattr(current_user, 'email', '?')} has no tenant_id") + raise HTTPException(status_code=403, detail="No tenant associated with this account") + return tenant_id + + +@router.get("/dashboard/properties") +async def get_dashboard_properties( + current_user: dict = Depends(get_current_user) +) -> Dict[str, Any]: + """Properties visible to the caller — their own tenant's, and only those.""" + tenant_id = _require_tenant(current_user) + return {"properties": await list_tenant_properties(tenant_id)} + @router.get("/dashboard/summary") async def get_dashboard_summary( property_id: str, + month: Optional[int] = Query(None, ge=1, le=12, description="Calendar month in the property's local timezone"), + year: Optional[int] = Query(None, ge=1970, le=2100), current_user: dict = 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=400, detail="month and year must be supplied together") + + try: + revenue_data = await get_revenue_summary(property_id, tenant_id, month, year) + except PropertyNotFound: + # Same response whether the property belongs to someone else or does not + # exist, so the endpoint cannot be used to probe other tenants. + raise HTTPException(status_code=404, detail="Property not found") + except ValueError as e: + raise HTTPException(status_code=409, detail=str(e)) + + # Money stays in Decimal and is rounded exactly once, here at the edge. + # float() on the way out is what turned exact sums into "a few cents off". + total_exact = Decimal(revenue_data["total"]) + total_rounded = total_exact.quantize(CENTS, rounding=ROUND_HALF_UP) + 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"], + "total_revenue": str(total_rounded), + "total_revenue_exact": str(total_exact), + "currency": revenue_data["currency"], + "reservations_count": revenue_data["count"], + "period": revenue_data.get("period", "all-time"), + "timezone": revenue_data.get("timezone", "UTC"), } diff --git a/backend/app/core/database_pool.py b/backend/app/core/database_pool.py index d638dfcfe..eda98271f 100644 --- a/backend/app/core/database_pool.py +++ b/backend/app/core/database_pool.py @@ -1,52 +1,85 @@ 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 _build_async_database_url() -> str: + """Translate the configured DATABASE_URL into an asyncpg DSN. + + settings.database_url is the sync/psycopg style URL used everywhere else + (and the one docker-compose injects), so it has to be re-prefixed before + SQLAlchemy's async engine will accept it. + """ + url = settings.database_url + if url.startswith("postgresql+asyncpg://"): + return url + if url.startswith("postgresql://"): + return url.replace("postgresql://", "postgresql+asyncpg://", 1) + if url.startswith("postgres://"): + return url.replace("postgres://", "postgresql+asyncpg://", 1) + 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).""" + if self.session_factory: + return + + async with self._init_lock: + # Another coroutine may have initialized while we waited. + if self.session_factory: + return + try: + database_url = _build_async_database_url() + + # Async engines use AsyncAdaptedQueuePool by default; passing the + # sync QueuePool class here makes create_async_engine raise. + self.engine = create_async_engine( + database_url, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, + pool_pre_ping=True, # Validate connections + pool_recycle=settings.database_pool_recycle, + 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 + 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. + + Deliberately synchronous: callers use it as ``async with + db_pool.get_session() as session``, which needs the session object + itself rather than a coroutine wrapping it. + """ if not self.session_factory: raise Exception("Database pool not initialized") return self.session_factory() @@ -56,5 +89,6 @@ async def get_session(self) -> AsyncSession: 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/services/cache.py b/backend/app/services/cache.py index b81474957..097dc31ef 100644 --- a/backend/app/services/cache.py +++ b/backend/app/services/cache.py @@ -1,29 +1,84 @@ import json +import logging import redis.asyncio as redis -from typing import Dict, Any +from typing import Any, Dict, Optional import os +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]: +CACHE_TTL_SECONDS = 300 + + +def _cache_key(property_id: str, tenant_id: str, month: Optional[int], year: Optional[int]) -> str: + """Build a fully-qualified cache key. + + The tenant is part of the key because property IDs are only unique *within* + a tenant (prop-001 exists for both Sunset and Ocean). Keying on property_id + alone let whichever tenant missed the cache first serve its revenue to the + other one. The period is in the key too, so a monthly total can never be + served for an all-time request. + """ + period = f"{year:04d}-{month:02d}" if month and year else "all-time" + return f"revenue:{tenant_id}:{property_id}:{period}" + + +async def get_revenue_summary( + property_id: str, + tenant_id: str, + month: Optional[int] = None, + year: Optional[int] = None, +) -> Dict[str, Any]: """ Fetches revenue summary, utilizing caching to improve performance. """ - cache_key = f"revenue:{property_id}" - + if not tenant_id: + # Without a tenant we cannot scope either the query or the cache entry. + raise ValueError("tenant_id is required to read a revenue summary") + + cache_key = _cache_key(property_id, tenant_id, month, year) + # Try to get from cache - cached = await redis_client.get(cache_key) + try: + cached = await redis_client.get(cache_key) + except Exception as cache_error: + # A cache outage must degrade to a live read, never to stale/foreign data. + logger.warning(f"Revenue cache read failed for {cache_key}: {cache_error}") + cached = None + if cached: - return json.loads(cached) - + payload = json.loads(cached) + # Defensive: refuse a cached entry that does not belong to this tenant. + if payload.get("tenant_id") == tenant_id and payload.get("property_id") == property_id: + return payload + logger.error(f"Discarding mismatched cache entry for key {cache_key}") + # Revenue calculation is delegated to the reservation service. - from app.services.reservations import calculate_total_revenue - + from app.services.reservations import calculate_monthly_revenue, calculate_total_revenue + # Calculate revenue - result = await calculate_total_revenue(property_id, tenant_id) - + if month and year: + result = await calculate_monthly_revenue(property_id, tenant_id, month, year) + else: + 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)) - + try: + await redis_client.setex(cache_key, CACHE_TTL_SECONDS, json.dumps(result)) + except Exception as cache_error: + logger.warning(f"Revenue cache write failed for {cache_key}: {cache_error}") + return result + + +async def invalidate_revenue_cache(property_id: str, tenant_id: str) -> int: + """Drop every cached revenue period for one property of one tenant.""" + deleted = 0 + try: + async for key in redis_client.scan_iter(match=f"revenue:{tenant_id}:{property_id}:*"): + deleted += await redis_client.delete(key) + except Exception as cache_error: + logger.warning(f"Revenue cache invalidation failed for {property_id}: {cache_error}") + return deleted diff --git a/backend/app/services/reservations.py b/backend/app/services/reservations.py index 384bd00ab..58abea6d3 100644 --- a/backend/app/services/reservations.py +++ b/backend/app/services/reservations.py @@ -1,109 +1,163 @@ -from datetime import datetime +from datetime import datetime, timezone from decimal import Decimal -from typing import Dict, Any, List +from typing import Dict, Any, List, Optional, Tuple -async def calculate_monthly_revenue(property_id: str, month: int, year: int, db_session=None) -> Decimal: +import pytz +from sqlalchemy import text + +from app.core.database_pool import db_pool + + +class PropertyNotFound(Exception): + """Raised when a property does not exist for the requesting tenant.""" + + +async def _get_property_timezone(session, property_id: str, tenant_id: str) -> str: + """Return the property's IANA timezone. + + Doubles as the tenant ownership check: properties are keyed by + (id, tenant_id), so a miss means the property does not belong to the + caller's tenant and must not be reported on. """ - Calculates revenue for a specific month. + result = await session.execute( + text(""" + SELECT timezone + FROM properties + WHERE id = :property_id AND tenant_id = :tenant_id + """), + {"property_id": property_id, "tenant_id": tenant_id}, + ) + row = result.fetchone() + if not row: + raise PropertyNotFound(f"Property {property_id} not found for tenant {tenant_id}") + return row.timezone or "UTC" + + +def month_bounds_utc(month: int, year: int, property_timezone: str) -> Tuple[datetime, datetime]: + """Return the UTC instants bounding a calendar month in the property's local time. + + A "March" total means March *where the property is*. Building the bounds as + naive UTC datetimes silently shifts every property's month by its UTC offset, + which is what made a 2024-02-29 23:30Z check-in (00:30 on 1 March in Paris) + fall outside Sunset Properties' March figures. """ + tz = pytz.timezone(property_timezone) - start_date = datetime(year, month, 1) + local_start = tz.localize(datetime(year, month, 1)) if month < 12: - end_date = datetime(year, month + 1, 1) + local_end = tz.localize(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}") + local_end = tz.localize(datetime(year + 1, 1, 1)) + + return local_start.astimezone(pytz.UTC), local_end.astimezone(pytz.UTC) + + +async def _aggregate_revenue( + session, + property_id: str, + tenant_id: str, + start_utc: Optional[datetime] = None, + end_utc: Optional[datetime] = None, +) -> Dict[str, Any]: + """Sum reservation amounts for one property, always scoped to one tenant. - # SQL Simulation (This would be executed against the actual DB) + Aggregation stays in NUMERIC on the database side and is carried into + Python as Decimal — never float — so sub-cent amounts add up exactly. + """ query = """ - SELECT SUM(total_amount) as total + SELECT + SUM(total_amount) AS total_revenue, + COUNT(*) AS reservation_count, + COUNT(DISTINCT currency) AS currency_count, + MIN(currency) AS currency FROM reservations - WHERE property_id = $1 - AND tenant_id = $2 - AND check_in_date >= $3 - AND check_in_date < $4 + WHERE property_id = :property_id + AND tenant_id = :tenant_id """ - - # 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 + params: Dict[str, Any] = {"property_id": property_id, "tenant_id": tenant_id} + + if start_utc is not None and end_utc is not None: + query += " AND check_in_date >= :start_date AND check_in_date < :end_date" + params["start_date"] = start_utc + params["end_date"] = end_utc + + result = await session.execute(text(query), params) + row = result.fetchone() + + count = row.reservation_count if row else 0 + if not count: + return {"total": Decimal("0"), "count": 0, "currency": "USD"} + + if row.currency_count > 1: + raise ValueError( + f"Property {property_id} has reservations in multiple currencies; " + "totals cannot be summed without conversion rates" + ) + + return { + # str() first: Decimal(float) would reintroduce binary rounding error. + "total": Decimal(str(row.total_revenue)), + "count": count, + "currency": row.currency or "USD", + } + + +async def list_tenant_properties(tenant_id: str) -> List[Dict[str, Any]]: + """List the properties owned by one tenant.""" + await db_pool.initialize() + + async with db_pool.get_session() as session: + result = await session.execute( + text(""" + SELECT id, name, timezone + FROM properties + WHERE tenant_id = :tenant_id + ORDER BY name + """), + {"tenant_id": tenant_id}, + ) + return [ + {"id": row.id, "name": row.name, "timezone": row.timezone} + for row in result.fetchall() + ] + + +async def calculate_monthly_revenue( + property_id: str, tenant_id: str, month: int, year: int +) -> Dict[str, Any]: + """Calculate revenue for a calendar month in the property's own timezone.""" + await db_pool.initialize() + + async with db_pool.get_session() as session: + property_timezone = await _get_property_timezone(session, property_id, tenant_id) + start_utc, end_utc = month_bounds_utc(month, year, property_timezone) + aggregate = await _aggregate_revenue(session, property_id, tenant_id, start_utc, end_utc) + + return { + "property_id": property_id, + "tenant_id": tenant_id, + "total": str(aggregate["total"]), + "currency": aggregate["currency"], + "count": aggregate["count"], + "period": f"{year:04d}-{month:02d}", + "timezone": property_timezone, + } + async def calculate_total_revenue(property_id: str, tenant_id: str) -> Dict[str, Any]: - """ - Aggregates revenue from database. - """ - 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'] - } + """Aggregate all-time revenue for a property from the database.""" + await db_pool.initialize() + + async with db_pool.get_session() as session: + property_timezone = await _get_property_timezone(session, property_id, tenant_id) + aggregate = await _aggregate_revenue(session, property_id, tenant_id) + + return { + "property_id": property_id, + "tenant_id": tenant_id, + "total": str(aggregate["total"]), + "currency": aggregate["currency"], + "count": aggregate["count"], + "period": "all-time", + "timezone": property_timezone, + } diff --git a/backend/requirements.txt b/backend/requirements.txt index 6b777d2aa..744b084c4 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -27,3 +27,4 @@ loguru redis>=5.0.0 asyncpg>=0.27.0 requests +pytest>=8.0.0 diff --git a/backend/tests/test_revenue_fixes.py b/backend/tests/test_revenue_fixes.py new file mode 100644 index 000000000..db693d24e --- /dev/null +++ b/backend/tests/test_revenue_fixes.py @@ -0,0 +1,128 @@ +"""Regression tests for the three revenue dashboard defects. + +These cover the pure logic (period boundaries, cache keying, rounding) and so +run without a database or Redis. End-to-end checks against the seeded data live +in scripts/verify_fixes.sh. +""" +from datetime import datetime, timezone +from decimal import Decimal, ROUND_HALF_UP + +import pytest + +from app.services.cache import _cache_key +from app.services.reservations import month_bounds_utc + + +class TestMonthBoundsAreLocalToTheProperty: + def test_paris_month_starts_before_utc_midnight(self): + start, end = month_bounds_utc(3, 2024, "Europe/Paris") + + # March in Paris (UTC+1) begins at 23:00 UTC on 29 February. + assert start == datetime(2024, 2, 29, 23, 0, tzinfo=timezone.utc) + assert end == datetime(2024, 3, 31, 22, 0, tzinfo=timezone.utc) + + def test_new_york_month_starts_after_utc_midnight(self): + start, end = month_bounds_utc(3, 2024, "America/New_York") + + # March in New York (UTC-5) begins at 05:00 UTC on 1 March. + assert start == datetime(2024, 3, 1, 5, 0, tzinfo=timezone.utc) + assert end == datetime(2024, 4, 1, 4, 0, tzinfo=timezone.utc) + + def test_reservation_just_after_local_midnight_falls_in_march(self): + """The booking behind Sunset Properties' March discrepancy. + + Checking in at 2024-02-29 23:30 UTC is 00:30 on 1 March in Paris, so it + belongs to March. Naive UTC bounds pushed it into February. + """ + check_in = datetime(2024, 2, 29, 23, 30, tzinfo=timezone.utc) + + march_start, march_end = month_bounds_utc(3, 2024, "Europe/Paris") + february_start, february_end = month_bounds_utc(2, 2024, "Europe/Paris") + + assert march_start <= check_in < march_end + assert not (february_start <= check_in < february_end) + + def test_december_rolls_over_to_next_year(self): + start, end = month_bounds_utc(12, 2024, "Europe/Paris") + + assert start == datetime(2024, 11, 30, 23, 0, tzinfo=timezone.utc) + assert end == datetime(2024, 12, 31, 23, 0, tzinfo=timezone.utc) + + def test_dst_transition_month_uses_correct_offsets(self): + """Paris switches to UTC+2 on 31 March 2024, so the month's two ends + carry different offsets.""" + start, end = month_bounds_utc(3, 2024, "Europe/Paris") + + assert start.hour == 23 # UTC+1 at the start of the month + assert end.hour == 22 # UTC+2 by the end of it + + +class TestCacheKeysAreTenantScoped: + def test_same_property_id_in_two_tenants_uses_distinct_keys(self): + """prop-001 exists for both clients; one key for both leaked revenue.""" + key_a = _cache_key("prop-001", "tenant-a", None, None) + key_b = _cache_key("prop-001", "tenant-b", None, None) + + assert key_a != key_b + assert "tenant-a" in key_a + assert "tenant-b" in key_b + + def test_periods_do_not_share_a_key(self): + all_time = _cache_key("prop-001", "tenant-a", None, None) + march = _cache_key("prop-001", "tenant-a", 3, 2024) + april = _cache_key("prop-001", "tenant-a", 4, 2024) + + assert len({all_time, march, april}) == 3 + + def test_month_is_zero_padded_so_keys_are_unambiguous(self): + assert _cache_key("prop-001", "tenant-a", 3, 2024).endswith("2024-03") + + +class TestMonetaryPrecision: + def test_sub_cent_amounts_sum_exactly_in_decimal(self): + amounts = [Decimal("333.333"), Decimal("333.333"), Decimal("333.334")] + + assert sum(amounts) == Decimal("1000.000") + assert sum(amounts).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) == Decimal("1000.00") + + def test_rounding_each_amount_first_would_lose_a_cent(self): + """Why the total is rounded once, at the very end.""" + amounts = [Decimal("333.333"), Decimal("333.333"), Decimal("333.334")] + rounded_first = sum(a.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) for a in amounts) + + assert rounded_first == Decimal("999.99") # a cent short of the truth + + @pytest.mark.parametrize( + "amount,decimal_result,float_result", + [ + ("333.335", "333.34", 333.33), + ("2.675", "2.68", 2.67), + ("1.005", "1.01", 1.0), + ], + ) + def test_float_round_trip_drifts_where_decimal_does_not(self, amount, decimal_result, float_result): + """The old endpoint did float(total), then the UI did Math.round(x*100)/100. + + Half-way amounts are not representable in binary, so they land a cent + below the correct figure - the finance team's "slightly off by a few + cents here and there". + """ + exact = Decimal(amount) + + assert exact.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) == Decimal(decimal_result) + assert round(float(exact), 2) == float_result + + def test_decimal_is_built_from_string_not_float(self): + assert Decimal(str(0.1)) == Decimal("0.1") + assert Decimal(0.1) != Decimal("0.1") + + +@pytest.mark.parametrize( + "tz,month,year", + [("Europe/Paris", 1, 2024), ("America/New_York", 12, 2023), ("UTC", 6, 2025)], +) +def test_bounds_are_always_utc_aware_and_ordered(tz, month, year): + start, end = month_bounds_utc(month, year, tz) + + assert start.tzinfo is not None and end.tzinfo is not None + assert start < end diff --git a/frontend/src/components/Dashboard.tsx b/frontend/src/components/Dashboard.tsx index a21bba404..a38e26786 100644 --- a/frontend/src/components/Dashboard.tsx +++ b/frontend/src/components/Dashboard.tsx @@ -1,16 +1,39 @@ -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; +} const Dashboard: React.FC = () => { - const [selectedProperty, setSelectedProperty] = useState('prop-001'); + // Properties come from the API scoped to the caller's tenant. The list used + // to be hardcoded with every tenant's properties, which both exposed other + // clients' portfolios and let users request revenue they cannot see. + const [properties, setProperties] = useState([]); + const [selectedProperty, setSelectedProperty] = useState(''); + const [period, setPeriod] = useState('2024-03'); + const [showRaw, setShowRaw] = useState(false); + const [propertiesError, setPropertiesError] = useState(''); + + useEffect(() => { + const loadProperties = async () => { + try { + const list = await SecureAPI.getDashboardProperties(); + setProperties(list); + setSelectedProperty((current) => current || list[0]?.id || ''); + } catch (err) { + setPropertiesError('Failed to load properties'); + console.error(err); + } + }; + + loadProperties(); + }, []); + + const [year, month] = period ? period.split('-').map(Number) : [undefined, undefined]; return (
@@ -26,27 +49,66 @@ const Dashboard: React.FC = () => { Monthly performance insights for your properties

- - {/* Property Selector */} -
- - + +
+ {/* Property Selector */} +
+ + +
+ + {/* Period Selector */} +
+ +
+ 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" + /> + +
+ +
- + {propertiesError && ( +
{propertiesError}
+ )} + {!propertiesError && properties.length === 0 && ( +
No properties available for your account.
+ )} + {selectedProperty && ( + + )}
diff --git a/frontend/src/components/RevenueSummary.tsx b/frontend/src/components/RevenueSummary.tsx index dbb6d0629..fc01e5e90 100644 --- a/frontend/src/components/RevenueSummary.tsx +++ b/frontend/src/components/RevenueSummary.tsx @@ -3,45 +3,64 @@ import { SecureAPI } from '../lib/secureApi'; interface RevenueData { property_id: string; - total_revenue: number; + // Monetary values are transported as strings so no amount ever passes + // through a JS number and picks up binary floating point error. + total_revenue: string; + total_revenue_exact: string; currency: string; reservations_count: number; + period: string; + timezone: string; } interface RevenueSummaryProps { propertyId?: string; - debugTenant?: string; + month?: number; + year?: number; showRaw?: boolean; } -export const RevenueSummary: React.FC = ({ propertyId = 'prop-001', debugTenant, showRaw }) => { +const formatMoney = (amount: string, currency: string) => { + // Split on the decimal point and group the integer part by hand: the value + // is already rounded to cents server-side and must be rendered verbatim. + const [whole, fraction = '00'] = amount.split('.'); + const negative = whole.startsWith('-'); + const digits = negative ? whole.slice(1) : whole; + const grouped = digits.replace(/\B(?=(\d{3})+(?!\d))/g, ','); + return `${currency} ${negative ? '-' : ''}${grouped}.${fraction}`; +}; + +export const RevenueSummary: React.FC = ({ propertyId = 'prop-001', 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); + const response = await SecureAPI.getDashboardSummary(propertyId, { month, year }); + if (!cancelled) setData(response); } catch (err) { - setError('Failed to load revenue data'); + if (!cancelled) { + setData(null); + setError('Failed to load revenue data'); + } console.error(err); } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; fetchRevenue(); - }, [propertyId, activeTenant]); + + // A slower in-flight request for a previously selected property must + // not land after a newer one and show the wrong property's revenue. + return () => { cancelled = true; }; + }, [propertyId, month, year]); if (loading) { return ( @@ -61,7 +80,7 @@ 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 roundedAwayFromExact = Number(data.total_revenue_exact) !== Number(data.total_revenue); return (
@@ -75,19 +94,17 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr
-

Total Revenue

+

+ Total Revenue{data.period !== 'all-time' ? ` · ${data.period}` : ''} +

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

+ Periods are calculated in the property's local time ({data.timezone}) +

@@ -102,14 +119,12 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr
- {/* Precision Warning Area */} + {/* Sub-cent amounts are summed exactly and rounded once; surface + the residual so finance can reconcile against raw bookings. */}
- {Math.abs(data.total_revenue - displayTotal) > 0.000001 && showRaw && ( -
- - - - Precision Mismatch Detected + {roundedAwayFromExact && showRaw && ( +
+ Exact sum {data.total_revenue_exact} rounded to {data.total_revenue}
)}
diff --git a/frontend/src/lib/secureApi.ts b/frontend/src/lib/secureApi.ts index f85f04c90..f25afb85c 100644 --- a/frontend/src/lib/secureApi.ts +++ b/frontend/src/lib/secureApi.ts @@ -186,20 +186,22 @@ export class SecureAPIClient { // Check if it's a valid JWT else if (token.includes('.') && token.split('.').length === 3) { const payload = JSON.parse(atob(token.split('.')[1])); - extractedTenantId = payload.user_metadata?.tenant_id || payload.tenant_id; + // app_metadata is where the login endpoint puts the tenant; reading + // only the other two locations left every request unscoped. + extractedTenantId = payload.user_metadata?.tenant_id + || payload.app_metadata?.tenant_id + || payload.tenant_id; } if (extractedTenantId) { - // Validate tenant ID format (should be UUID) if (this.isValidTenantId(extractedTenantId)) { this.cachedTenantId = extractedTenantId; return this.cachedTenantId; - } else { - console.error('[SecureAPI] Invalid tenant ID format:', extractedTenantId); - // Clear invalid session to force re-authentication - this.cachedToken = null; - this.cachedTenantId = null; } + // An unrecognised tenant ID format is not a reason to destroy a + // working session - it only means we must not cache under it. + console.warn('[SecureAPI] Unrecognised tenant ID format, disabling request cache:', extractedTenantId); + this.cachedTenantId = null; } } } catch (error) { @@ -243,9 +245,12 @@ export class SecureAPIClient { * Validate tenant ID format for security */ private isValidTenantId(tenantId: string): boolean { - // Check for UUID format (basic validation) + if (typeof tenantId !== 'string' || tenantId.length === 0) return false; + + // UUID tenants (production) or slug tenants such as "tenant-a". const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; - return typeof tenantId === 'string' && tenantId.length > 0 && uuidRegex.test(tenantId); + const slugRegex = /^[a-z0-9][a-z0-9_-]{0,63}$/i; + return uuidRegex.test(tenantId) || slugRegex.test(tenantId); } /** @@ -1450,22 +1455,27 @@ export class SecureAPIClient { // ============= DASHBOARD API ============= /** - * Get dashboard summary with optional simulation header + * Get dashboard revenue summary for one property. + * + * The tenant is never sent by the client - it is derived server-side from + * the authenticated token, so the browser cannot ask for another tenant. */ - 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()); + if (options?.month && options?.year) { + queryParams.append('month', options.month.toString()); + queryParams.append('year', options.year.toString()); } - const requestOptions: RequestInit = {}; - if (options?.simulatedTenant) { - requestOptions.headers = { - 'X-Simulated-Tenant': options.simulatedTenant - }; - } + return this.request(`/api/v1/dashboard/summary?${queryParams}`); + } - return this.request(`/api/v1/dashboard/summary?${queryParams}`, requestOptions); + /** + * List the properties belonging to the authenticated user's tenant. + */ + async getDashboardProperties() { + const res = await this.request('/api/v1/dashboard/properties'); + return Array.isArray(res?.properties) ? res.properties : []; } async uploadCompanyLogo(logo_url: string) { diff --git a/scripts/verify_fixes.sh b/scripts/verify_fixes.sh new file mode 100755 index 000000000..1f03298f5 --- /dev/null +++ b/scripts/verify_fixes.sh @@ -0,0 +1,104 @@ +#!/usr/bin/env bash +# End-to-end verification of the revenue dashboard fixes. +# +# Usage: docker compose up --build -d && ./scripts/verify_fixes.sh +# +# Every expected figure below was taken from the seeded database, not from the +# API, so the script fails if the API ever drifts away from the source data. +set -uo pipefail + +API="${API:-http://localhost:8000}" +PASS=0 +FAIL=0 + +login() { + curl -s -X POST "$API/api/v1/auth/login" \ + -H 'Content-Type: application/json' \ + -d "{\"email\":\"$1\",\"password\":\"$2\"}" | + python3 -c 'import sys,json; print(json.load(sys.stdin)["access_token"])' +} + +check() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + printf ' \033[32mPASS\033[0m %s\n' "$label" + PASS=$((PASS + 1)) + else + printf ' \033[31mFAIL\033[0m %s\n expected: %s\n actual: %s\n' "$label" "$expected" "$actual" + FAIL=$((FAIL + 1)) + fi +} + +summary() { # token, property, [month year] + local token="$1" property="$2" query="property_id=$2" + [ $# -gt 2 ] && query="$query&month=$3&year=$4" + curl -s "$API/api/v1/dashboard/summary?$query" -H "Authorization: Bearer $token" +} + +field() { python3 -c 'import sys,json; print(json.load(sys.stdin).get(sys.argv[1],""))' "$1"; } + +TOKEN_A=$(login sunset@propertyflow.com client_a_2024) +TOKEN_B=$(login ocean@propertyflow.com client_b_2024) + +echo +echo "1. Tenant isolation - prop-001 exists for BOTH clients" +check "Sunset sees its own Beach House Alpha revenue" \ + "2250.00" "$(summary "$TOKEN_A" prop-001 | field total_revenue)" +check "Ocean sees its own Mountain Lodge Beta (no bookings), not Sunset's" \ + "0.00" "$(summary "$TOKEN_B" prop-001 | field total_revenue)" +check "Repeat read for Ocean is still Ocean's (cache is tenant-scoped)" \ + "0.00" "$(summary "$TOKEN_B" prop-001 | field total_revenue)" +check "Repeat read for Sunset is still Sunset's" \ + "2250.00" "$(summary "$TOKEN_A" prop-001 | field total_revenue)" +check "Sunset cannot read Ocean's prop-004" \ + "Property not found" "$(summary "$TOKEN_A" prop-004 | field detail)" +check "Ocean cannot read Sunset's prop-003" \ + "Property not found" "$(summary "$TOKEN_B" prop-003 | field detail)" +check "Unauthenticated requests are rejected" \ + "401" "$(curl -s -o /dev/null -w '%{http_code}' "$API/api/v1/dashboard/summary?property_id=prop-001")" + +echo +echo "2. Timezone - months are counted in the property's local time" +check "Sunset prop-001 March 2024 (Paris) includes the 29 Feb 23:30 UTC check-in" \ + "2250.00" "$(summary "$TOKEN_A" prop-001 3 2024 | field total_revenue)" +check "...and that booking is counted (4 reservations, not 3)" \ + "4" "$(summary "$TOKEN_A" prop-001 3 2024 | field reservations_count)" +check "February 2024 is therefore empty" \ + "0.00" "$(summary "$TOKEN_A" prop-001 2 2024 | field total_revenue)" +check "Period is reported in the property's timezone" \ + "Europe/Paris" "$(summary "$TOKEN_A" prop-001 3 2024 | field timezone)" +check "Ocean's properties are evaluated in New York time" \ + "America/New_York" "$(summary "$TOKEN_B" prop-004 3 2024 | field timezone)" + +echo +echo "3. Precision - sub-cent amounts sum exactly and round once" +check "Sunset prop-002 exact sum" \ + "4975.500" "$(summary "$TOKEN_A" prop-002 | field total_revenue_exact)" +check "Sunset prop-003 total" \ + "6100.50" "$(summary "$TOKEN_A" prop-003 | field total_revenue)" +check "Ocean prop-004 total" \ + "1776.50" "$(summary "$TOKEN_B" prop-004 | field total_revenue)" +check "Ocean prop-005 total" \ + "3256.00" "$(summary "$TOKEN_B" prop-005 | field total_revenue)" +check "prop-001's three 333.33x bookings total 1000.000, not 999.99" \ + "1000.000" "$(summary "$TOKEN_A" prop-001 3 2024 | python3 -c ' +import sys, json +from decimal import Decimal +data = json.load(sys.stdin) +# The 1250.00 timezone booking plus the three sub-cent ones. +print(Decimal(data["total_revenue_exact"]) - Decimal("1250.000"))')" + +echo +echo "4. Property list is scoped to the caller's tenant" +check "Sunset sees only its 3 properties" \ + "prop-001,prop-002,prop-003" \ + "$(curl -s "$API/api/v1/dashboard/properties" -H "Authorization: Bearer $TOKEN_A" | + python3 -c 'import sys,json; print(",".join(sorted(p["id"] for p in json.load(sys.stdin)["properties"])))')" +check "Ocean sees only its 3 properties" \ + "prop-001,prop-004,prop-005" \ + "$(curl -s "$API/api/v1/dashboard/properties" -H "Authorization: Bearer $TOKEN_B" | + python3 -c 'import sys,json; print(",".join(sorted(p["id"] for p in json.load(sys.stdin)["properties"])))')" + +echo +printf '%s passed, %s failed\n\n' "$PASS" "$FAIL" +[ "$FAIL" -eq 0 ] From 58dae21d8b3635ca9a30a9a8029ac3ecc1ec7c93 Mon Sep 17 00:00:00 2001 From: Rahul Trivedi Date: Thu, 30 Jul 2026 11:32:30 -0700 Subject: [PATCH 2/2] Added FINDINGS.md and Loom video link. I could not cover all the bugs in the video as I had only 5 minutes on my Loom subscription. So all the changes are explained in detail in FINDINGS.MD --- FINDINGS.md | 195 ++++++++++++++++++++++++++++++++++++++++++++ docker-compose.yml | 2 - loom-video-link.txt | 1 + 3 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 FINDINGS.md create mode 100644 loom-video-link.txt diff --git a/FINDINGS.md b/FINDINGS.md new file mode 100644 index 000000000..831ddbac9 --- /dev/null +++ b/FINDINGS.md @@ -0,0 +1,195 @@ +# Revenue Dashboard — Investigation & Fixes + +Three client-reported problems, three separate root causes, plus one that made all +of them hard to see. Everything below was reproduced against the seeded database +before and after the fix. + +Reproduce with: + +```bash +docker compose up --build -d +./scripts/verify_fixes.sh # 19 end-to-end checks +docker compose exec backend python -m pytest tests -q # 17 unit tests +``` + +--- + +## Ground truth + +What the seeded database actually contains (`docker compose exec db psql -U postgres -d propertyflow`): + +| tenant | property | bookings | revenue | +|---|---|---|---| +| tenant-a (Sunset) | prop-001 Beach House Alpha | 4 | 2250.000 | +| tenant-a | prop-002 City Apartment Downtown | 4 | 4975.500 | +| tenant-a | prop-003 Country Villa Estate | 2 | 6100.500 | +| tenant-b (Ocean) | prop-001 Mountain Lodge Beta | 0 | — | +| tenant-b | prop-004 Lakeside Cottage | 4 | 1776.500 | +| tenant-b | prop-005 Urban Loft Modern | 3 | 3256.000 | + +The important detail: **`prop-001` exists for both clients and is a different +building for each.** Property IDs are only unique within a tenant — the schema +says so (`PRIMARY KEY (id, tenant_id)`). + +--- + +## Bug 0 — The dashboard was not reading the database at all + +**Symptom:** the numbers were self-consistent but wrong, and nothing anyone did +to the data changed them. + +`calculate_total_revenue()` wrapped its query in a `try/except` whose fallback +was a hardcoded dictionary of "mock data for testing". The query never ran: + +- `DatabasePool.initialize()` built its DSN from `settings.supabase_db_user`, + `supabase_db_host` … — fields that do not exist on `Settings`. The + `AttributeError` was swallowed, leaving `session_factory = None`. +- It also passed the synchronous `QueuePool` to `create_async_engine`, and + `get_session()` was declared `async def` while callers used it as + `async with db_pool.get_session()` — an `async with` over a coroutine. +- `DatabasePool()` was re-instantiated per request, so every call built a new + engine. + +Every dashboard request therefore hit `except` and served fabricated figures +(`prop-001 → 1000.00`), logging one line to stdout that nobody was reading. + +**Fix** — [`backend/app/core/database_pool.py`](backend/app/core/database_pool.py), +[`backend/app/services/reservations.py`](backend/app/services/reservations.py): +build the DSN from the configured `DATABASE_URL` (re-prefixed for asyncpg), drop +the sync pool class, make `get_session()` synchronous so `async with` gets a +session, use the module-level singleton with an idempotent `initialize()`, and +**delete the mock fallback**. A revenue endpoint that cannot reach its database +must fail loudly, not invent numbers for a board meeting. + +--- + +## Bug 1 — Client B saw another company's revenue (privacy) + +**Root cause** — [`backend/app/services/cache.py`](backend/app/services/cache.py): + +```python +cache_key = f"revenue:{property_id}" # no tenant +``` + +Both clients own a `prop-001`. Whichever tenant missed the cache first wrote its +revenue under the shared key, and for the next 5 minutes the *other* tenant was +served that value. Refreshing after the TTL expired flipped which company's +numbers appeared — exactly the "sometimes when we refresh" report. + +**Fix:** the cache key is now `revenue:{tenant_id}:{property_id}:{period}`, the +cached payload is re-checked against the requesting tenant before it is +returned, and a Redis outage degrades to a live database read instead of a +stale one. + +Two related holes closed at the same time: + +- `dashboard.py` resolved the tenant as + `getattr(current_user, "tenant_id", "default_tenant") or "default_tenant"` — + any user whose tenant failed to resolve was silently dropped into one shared + bucket with everyone else. There is now no default: no tenant means `403`. +- Revenue was returned for **any** property ID the caller asked for, with no + ownership check. `GET /dashboard/summary?property_id=prop-004` as Sunset + returned Ocean's property. The query is now scoped by `(id, tenant_id)` and + a property belonging to another tenant returns `404` — the same response as + one that does not exist, so the endpoint cannot be used to enumerate other + clients' portfolios. +- The frontend sent an `X-Simulated-Tenant` header taken from a component prop. + The backend ignored it, but a client-supplied tenant has no business being on + a request at all; it is gone. Tenant comes from the JWT, server-side, only. + +--- + +## Bug 2 — Sunset's March total didn't match their records (timezone) + +**Root cause** — [`backend/app/services/reservations.py`](backend/app/services/reservations.py): + +```python +start_date = datetime(year, month, 1) # naive, implicitly UTC +end_date = datetime(year, month + 1, 1) +``` + +`check_in_date` is `TIMESTAMP WITH TIME ZONE`, and Sunset's properties are in +`Europe/Paris`. Comparing against naive UTC midnight shifts every property's +month by its UTC offset. + +Reservation `res-tz-1` checks in at **2024-02-29 23:30 UTC — which is 00:30 on +1 March in Paris**, where the building is. It is a March booking by any +reasonable definition, and by Sunset's own records. The UTC bounds pushed it +into February: + +| | bookings | March total | +|---|---|---| +| naive UTC bounds (before) | 3 | 1000.00 | +| Paris-local bounds (after) | 4 | **2250.00** | + +That 1250.00 gap is the discrepancy the client called about. + +**Fix:** `month_bounds_utc()` builds the month boundaries in the property's own +IANA timezone (read from `properties.timezone`) and converts them to UTC for the +comparison. DST is handled by `pytz` localisation, so March 2024 in Paris +correctly starts at UTC+1 and ends at UTC+2. The response now reports the +`timezone` and `period` it used, and the UI states it under the figure. + +The endpoint gained optional `month` / `year` parameters (all-time remains the +default) so a monthly figure can be requested at all — `calculate_monthly_revenue` +was previously dead code returning `Decimal('0')`. + +--- + +## Bug 3 — Totals off by a few cents (precision) + +Amounts are stored as `NUMERIC(10,3)`; three of prop-001's bookings are +333.333 + 333.333 + 333.334, which is exactly 1000.000. Two places threw that +exactness away: + +```python +total_revenue_float = float(revenue_data['total']) # dashboard.py +``` +```ts +const displayTotal = Math.round(data.total_revenue * 100) / 100; // RevenueSummary.tsx +``` + +Money went `Decimal → float → JSON number → JS double → round`. Halfway values +are not representable in binary, so they round the wrong way: `333.335` becomes +`333.33` rather than `333.34`, once per property per period. + +**Fix:** the sum stays in `NUMERIC` in the database and becomes a `Decimal` in +Python (built from `str`, never from a float). It is rounded **once**, at the +API edge, with `ROUND_HALF_UP`, and transported as a **string** so it never +enters a JS double. The response carries both `total_revenue` ("2250.00", for +display) and `total_revenue_exact` ("2250.000", for reconciliation). The UI +formats the string digits directly. + +A currency guard came with it: the summation now refuses to add reservations in +different currencies (`409`) rather than silently treating EUR as USD. + +--- + +## Also fixed + +- **The property dropdown was hardcoded with all five properties**, so each + client saw the other's portfolio names and could request revenue they had no + right to. It is now `GET /dashboard/properties`, scoped to the caller's tenant. +- **`SecureAPI.getTenantId()` never found the tenant** (it read `user_metadata` + and the root claim, but the login endpoint puts it in `app_metadata`), and on + an unrecognised ID format it *deleted the caller's access token* — a + formatting rule that could log people out. It now reads all three locations, + accepts slug tenants as well as UUIDs, and never destroys a working session. +- **A stale-response race in `RevenueSummary`**: switching properties quickly + could let a slower earlier request land last and display the wrong property's + revenue. The effect now ignores responses from a superseded request. +- **The hardcoded "+12%" trend badge** ("fake trend indicator for premium feel") + is gone. Invented growth figures do not belong on a client revenue dashboard. + +--- + +## Verification + +`./scripts/verify_fixes.sh` — 19 checks against the running stack: both clients' +totals match the database, neither can read the other's properties, March +includes the timezone-boundary booking, February is empty, sub-cent sums land on +1000.000, and each client's property list contains only its own. + +`backend/tests/test_revenue_fixes.py` — 17 unit tests over the pure logic: +timezone boundaries (including DST and the December rollover), tenant-scoped +cache keys, and the rounding rules, each asserting the old behaviour would fail. diff --git a/docker-compose.yml b/docker-compose.yml index e255de0b5..aea89c829 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,5 +1,3 @@ -version: '3.8' - services: backend: build: diff --git a/loom-video-link.txt b/loom-video-link.txt new file mode 100644 index 000000000..f7eb0de63 --- /dev/null +++ b/loom-video-link.txt @@ -0,0 +1 @@ +https://www.loom.com/share/f89cef0020ba4c82ad7dc7b724935283 \ No newline at end of file