diff --git a/.gitignore b/.gitignore new file mode 100644 index 000000000..ebc7f49bc --- /dev/null +++ b/.gitignore @@ -0,0 +1,10 @@ +# Local Python virtualenv used for running the backend outside Docker +.venv/ +backend/.venv/ + +# Python bytecode +__pycache__/ +*.py[cod] + +# Vite's regenerated dependency cache +frontend/node_modules/.vite/ diff --git a/.vscode/launch.json b/.vscode/launch.json index c07ae42ae..8847897c2 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -1,10 +1,57 @@ { + "version": "0.2.0", "configurations": [ { + // RECOMMENDED. Runs the backend directly under the debugger, against the + // Postgres + Redis containers. Breakpoints work with no attach step. + // Requires: docker compose up -d db redis + "name": "Python: Debug Backend (local)", + "type": "debugpy", + "request": "launch", + "module": "uvicorn", + "args": ["app.main:app", "--host", "0.0.0.0", "--port", "8000"], + "cwd": "${workspaceFolder}/backend", + "python": "${workspaceFolder}/backend/.venv/bin/python", + "env": { + "DATABASE_URL": "postgresql://postgres:postgres@localhost:5433/propertyflow", + "REDIS_URL": "redis://localhost:6380/0", + "SECRET_KEY": "debug_challenge_secret" + }, + "justMyCode": false, + "console": "integratedTerminal" + }, + { + // Alternative: attach to the backend running under debugpy in Docker. + // docker compose -f docker-compose.yml -f docker-compose.debug.yml up + "name": "Python: Attach to Backend (Docker)", + "type": "debugpy", + "request": "attach", + "connect": { "host": "localhost", "port": 5678 }, + "pathMappings": [ + { + "localRoot": "${workspaceFolder}/backend", + "remoteRoot": "/app" + } + ], + "justMyCode": false + }, + { + // Debug the React dashboard in Chrome (vite dev server on :5173). + "name": "Chrome: Debug Frontend", "type": "chrome", - "name": "http://localhost:5173", "request": "launch", - "url": "http://localhost:5173/hr/away" + "url": "http://localhost:5173", + "webRoot": "${workspaceFolder}/frontend/src", + "sourceMaps": true + } + ], + "compounds": [ + { + "name": "Debug Full Stack (backend + frontend)", + "configurations": [ + "Python: Debug Backend (local)", + "Chrome: Debug Frontend" + ] } ] } diff --git a/DEBUGGING.md b/DEBUGGING.md new file mode 100644 index 000000000..8f13c9f82 --- /dev/null +++ b/DEBUGGING.md @@ -0,0 +1,172 @@ +# Debugging Walkthrough + +How each bug was located with the debugger, and how to reproduce that session. + +## Setup + +The backend runs locally against the Postgres and Redis containers. Breakpoints +work immediately, with no attach step. + +```bash +# 1. Data services +docker compose up -d db redis + +# 2. Backend (needs Python 3.10+; the codebase uses `str | None` syntax) +cd backend +python3.12 -m venv .venv +.venv/bin/pip install -r requirements.txt +DATABASE_URL="postgresql://postgres:postgres@localhost:5433/propertyflow" \ +REDIS_URL="redis://localhost:6380/0" \ +SECRET_KEY="debug_challenge_secret" \ +.venv/bin/python -m uvicorn app.main:app --port 8000 + +# 3. Frontend +cd frontend && npm run dev +``` + +In VS Code, hit F5 on **"Python: Debug Backend (local)"** instead of step 2 — it +sets those environment variables for you. + +Frontend: http://localhost:5173 · API docs: http://localhost:8000/docs + +
+Running everything in Docker instead + +```bash +docker compose -f docker-compose.yml -f docker-compose.debug.yml up --build +``` + +Then attach with **"Python: Attach to Backend (Docker)"**. If image pulls fail with +`lookup auth.docker.io: i/o timeout`, Docker Desktop's internal DNS is the problem — +add `"dns": ["8.8.8.8", "1.1.1.1"]` under Settings → Docker Engine and restart. +
+ +| Client | Email | Password | Tenant | +|---|---|---|---| +| Sunset Properties | `sunset@propertyflow.com` | `client_a_2024` | `tenant-a` | +| Ocean Rentals | `ocean@propertyflow.com` | `client_b_2024` | `tenant-b` | + +To see the **broken** behaviour first, stash the fixes, then restore them: + +```bash +git stash # back to the buggy code +git stash pop # fixes restored +``` + +--- + +## Bug 1 — Client B sees another company's revenue + +**Breakpoint:** `backend/app/services/cache.py` → the `cached = await redis_client.get(cache_key)` line. + +**Session:** +1. Log in as **Sunset**, open `prop-001`. Breakpoint hits. + Inspect: `tenant_id == 'tenant-a'`, `cache_key == 'revenue:prop-001'`, `cached is None`. + The result gets written to Redis under that key. Continue. +2. Log in as **Ocean**, open `prop-001`. Breakpoint hits again. + Inspect: `tenant_id == 'tenant-b'` — but `cache_key` is **the same string**, and `cached` is now **populated**. + +**The smoking gun.** Add this watch expression: + +```python +json.loads(cached)['tenant_id'] != tenant_id +``` + +It evaluates to `True` — the cached payload is stamped `tenant-a` while the request is `tenant-b`. Ocean is served Sunset's numbers and returns before any tenant-filtered query runs. + +**Root cause:** `cache_key = f"revenue:{property_id}"` omits the tenant. Property IDs are only unique *per tenant* (`properties` has a composite `(id, tenant_id)` key) and `prop-001` exists for both companies. + +**Fix:** `cache_key = f"revenue:{tenant_id}:{property_id}"`. Re-run: keys become `revenue:tenant-a:prop-001` and `revenue:tenant-b:prop-001`, `cached` is `None` for Ocean, and the leak is gone. + +--- + +## Bug 2 — Client A's March totals don't match + +**Breakpoint:** `backend/app/services/reservations.py` → `calculate_monthly_revenue`, on the line after `end_date` is computed. + +**Session:** Inspect `start_date` and `end_date`. + +- Before: `2024-03-01 00:00:00` — naive, i.e. implicitly **UTC**. +- After: `2024-02-29 23:00:00+00:00` — March 1st *in Paris* (UTC+1) is Feb 29th 23:00 UTC. + +Now step to the query result and compare: + +| Month boundaries | March 2024 total | bookings | +|---|---|---| +| UTC (before) | `1000.000` | 3 | +| `Europe/Paris` (after) | `2250.000` | 4 | + +**The missing booking:** `res-tz-1`, `total_amount = 1250.000`, checking in `2024-02-29 23:30:00+00` UTC — which is `2024-03-01 00:30` in Paris. It *is* a March booking for this property, but UTC bucketing files it under February. + +**Root cause:** month boundaries were built with naive `datetime(year, month, 1)` while `properties.timezone` (here `Europe/Paris`) was ignored. + +**Fix:** compute the boundaries in the property's own timezone and convert to UTC before filtering. + +--- + +## Bug 3 — Finance: totals off by a few cents + +**Breakpoint:** `backend/app/api/v1/dashboard.py` → the line building `total_revenue`. + +**Session:** `total_amount` is `NUMERIC(10,3)`, so sums can carry a third decimal. In the Debug Console, evaluate a half-cent value: + +```python +float('1080.405') # 1080.405 +round(1080.405 * 100) / 100 # 1080.4 <-- loses a cent +Decimal('1080.405').quantize(Decimal('0.01'), ROUND_HALF_UP) # 1080.41 <-- correct +``` + +The old path rounded **twice** in binary floating point — `float()` in `dashboard.py`, then `Math.round(x * 100) / 100` again in `RevenueSummary.tsx`. Because `1080.405` isn't exactly representable in binary, it sits just *below* the true half and rounds down. + +**Fix:** quantize with `Decimal` + `ROUND_HALF_UP` on the backend, and drop the frontend's redundant re-rounding. Frontend breakpoint to confirm: `frontend/src/components/RevenueSummary.tsx`, on the `displayTotal` line. + +--- + +## Bug 4 — The dashboard was never reading the database + +Found while stepping into Bug 1: the tenant-filtered SQL never executed. + +**Breakpoint:** `backend/app/core/database_pool.py` → the `except Exception as e:` block in `initialize()`. + +It is hit on **every** request. Inspect `e`: + +``` +AttributeError: 'Settings' object has no attribute 'supabase_db_user' +``` + +`initialize()` built its URL from `settings.supabase_db_user/password/host/port/name`, none of which exist in `app/config.py` — it only defines `database_url`. So `session_factory` was always `None`. + +**Then set a breakpoint on the old `except` block in `calculate_total_revenue`** (`reservations.py`). Also hit every request, with `e = "Database pool not available"`, about to return from a hardcoded `mock_data` dict keyed **only by `property_id`**: + +```python +mock_data['prop-001'] # {'total': '1000.00', 'count': 3} -- for BOTH tenants +``` + +This was the real engine behind Client B's complaint, and note `1000.00 / 3` is *exactly* the wrong UTC-bucketed March figure from Bug 2 — the buggy answer had been frozen into a constant. + +**Two further defects** surfaced on the way to fixing it: +- `poolclass=QueuePool` → `ArgumentError: Pool class QueuePool cannot be used with asyncio engine`. Needs `AsyncAdaptedQueuePool`. +- `async def get_session()` used as `async with db_pool.get_session()` → `AttributeError: __aenter__`, because calling it returns a coroutine, not a session. + +**Fix:** build the URL from `settings.database_url`, use `AsyncAdaptedQueuePool`, make `get_session()` non-async, reuse the shared `db_pool` singleton instead of constructing a new engine per request, and **delete the fabricated fallback** — the endpoint now returns `503` rather than displaying revenue it cannot attribute to real data. + +--- + +## Verifying the fixes + +Log in as each client and compare `prop-001`: + +| | Sunset (tenant-a) | Ocean (tenant-b) | +|---|---|---| +| Before | `1000.00`, 3 bookings | `1000.00`, 3 bookings ← Sunset's data | +| After | `2250.00`, 4 bookings | `0.00`, 0 bookings | + +Ocean's `prop-001` (*Mountain Lodge Beta*) genuinely has no reservations — `0.00` is the correct answer, and the fact that it previously showed `1000.00` was the privacy breach. + +Confirm cache isolation directly: + +```bash +docker compose exec redis redis-cli KEYS 'revenue:*' +# revenue:tenant-a:prop-001 +# revenue:tenant-b:prop-001 +``` diff --git a/backend/app/api/v1/dashboard.py b/backend/app/api/v1/dashboard.py index 1ec352d7e..0bde1402d 100644 --- a/backend/app/api/v1/dashboard.py +++ b/backend/app/api/v1/dashboard.py @@ -1,25 +1,42 @@ -from fastapi import APIRouter, Depends, HTTPException +import logging +from decimal import Decimal, ROUND_HALF_UP +from fastapi import APIRouter, Depends, HTTPException, status from typing import Dict, Any from app.services.cache import get_revenue_summary from app.core.auth import authenticate_request as get_current_user +logger = logging.getLogger(__name__) + router = APIRouter() +CENTS = Decimal('0.01') + @router.get("/dashboard/summary") async def get_dashboard_summary( property_id: str, 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']) - + + try: + revenue_data = await get_revenue_summary(property_id, tenant_id) + except Exception as e: + # Surface the outage instead of serving substitute figures: a revenue + # dashboard must never display numbers it cannot attribute to real data. + logger.error(f"Revenue lookup failed for {property_id} (tenant: {tenant_id}): {e}") + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Revenue data is temporarily unavailable", + ) + + # Round with Decimal before converting to float so currency (stored with + # sub-cent precision) doesn't drift due to binary floating-point error. + total_revenue_decimal = Decimal(str(revenue_data['total'])).quantize(CENTS, rounding=ROUND_HALF_UP) + return { "property_id": revenue_data['property_id'], - "total_revenue": total_revenue_float, + "total_revenue": float(total_revenue_decimal), "currency": revenue_data['currency'], "reservations_count": revenue_data['count'] } diff --git a/backend/app/core/database_pool.py b/backend/app/core/database_pool.py index d638dfcfe..4cd8bd4e6 100644 --- a/backend/app/core/database_pool.py +++ b/backend/app/core/database_pool.py @@ -1,6 +1,6 @@ import asyncio from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker -from sqlalchemy.pool import QueuePool +from sqlalchemy.pool import AsyncAdaptedQueuePool import logging from ..config import settings @@ -13,13 +13,27 @@ def __init__(self): async def initialize(self): """Initialize database connection pool""" + if self.session_factory: + # Already initialized - reuse the existing pool + return + 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}" - + # Build the async URL from the configured database_url. + # (Previously this referenced settings.supabase_db_* fields that do not + # exist on Settings, so initialize() always raised and every caller + # silently fell back to non-database data.) + database_url = settings.database_url + if database_url.startswith("postgresql+asyncpg://"): + pass + elif database_url.startswith("postgresql://"): + database_url = database_url.replace("postgresql://", "postgresql+asyncpg://", 1) + elif database_url.startswith("postgres://"): + database_url = database_url.replace("postgres://", "postgresql+asyncpg://", 1) + self.engine = create_async_engine( database_url, - poolclass=QueuePool, + # QueuePool is sync-only; asyncio engines require the async-adapted pool. + poolclass=AsyncAdaptedQueuePool, pool_size=20, # Number of connections to maintain max_overflow=30, # Additional connections when needed pool_pre_ping=True, # Validate connections @@ -45,8 +59,13 @@ async def close(self): if self.engine: await self.engine.dispose() - async def get_session(self) -> AsyncSession: - """Get database session from pool""" + def get_session(self) -> AsyncSession: + """Get database session from pool + + Deliberately NOT async: every caller uses `async with db_pool.get_session()`. + An `async def` here returns a coroutine, which has no __aenter__, so that + pattern raised AttributeError before the session was ever opened. + """ if not self.session_factory: raise Exception("Database pool not initialized") return self.session_factory() diff --git a/backend/app/services/cache.py b/backend/app/services/cache.py index b81474957..2e2ef8b0a 100644 --- a/backend/app/services/cache.py +++ b/backend/app/services/cache.py @@ -10,7 +10,10 @@ async def get_revenue_summary(property_id: str, tenant_id: str) -> Dict[str, Any """ Fetches revenue summary, utilizing caching to improve performance. """ - cache_key = f"revenue:{property_id}" + # Must include tenant_id: property IDs are only unique per-tenant + # (see properties table composite key), so two different tenants can + # both have a property "prop-001" and would otherwise share a cache entry. + cache_key = f"revenue:{tenant_id}:{property_id}" # Try to get from cache cached = await redis_client.get(cache_key) diff --git a/backend/app/services/reservations.py b/backend/app/services/reservations.py index 384bd00ab..fd7472c81 100644 --- a/backend/app/services/reservations.py +++ b/backend/app/services/reservations.py @@ -1,109 +1,137 @@ +import logging from datetime import datetime from decimal import Decimal from typing import Dict, Any, List +from zoneinfo import ZoneInfo -async def calculate_monthly_revenue(property_id: str, month: int, year: int, db_session=None) -> Decimal: +logger = logging.getLogger(__name__) + +UTC = ZoneInfo("UTC") + + +async def _get_property_timezone(property_id: str, tenant_id: str) -> str: + """Looks up the property's local timezone (falls back to UTC if unavailable).""" + try: + from app.core.database_pool import db_pool + + await db_pool.initialize() + + if db_pool.session_factory: + async with db_pool.get_session() as session: + from sqlalchemy import text + + query = text( + "SELECT timezone FROM properties WHERE id = :property_id AND tenant_id = :tenant_id" + ) + result = await session.execute(query, {"property_id": property_id, "tenant_id": tenant_id}) + row = result.fetchone() + if row and row.timezone: + return row.timezone + except Exception as e: + logger.warning("Could not resolve timezone for %s (tenant: %s): %s", property_id, tenant_id, e) + + return "UTC" + + +async def calculate_monthly_revenue(property_id: str, tenant_id: str, month: int, year: int, db_session=None) -> Decimal: """ Calculates revenue for a specific month. + + Reservations are stored in UTC (check_in_date), but "month" is a concept + defined in the property's local timezone. Month boundaries are computed + in the property's local timezone and converted to UTC before filtering, + so a check-in that is late-night on the last day of a month locally isn't + miscounted into the wrong month just because it crossed a UTC day boundary. """ + property_timezone = await _get_property_timezone(property_id, tenant_id) + local_tz = ZoneInfo(property_timezone) - start_date = datetime(year, month, 1) + start_date = datetime(year, month, 1, tzinfo=local_tz).astimezone(UTC) if month < 12: - end_date = datetime(year, month + 1, 1) + end_date = datetime(year, month + 1, 1, tzinfo=local_tz).astimezone(UTC) else: - end_date = datetime(year + 1, 1, 1) - - print(f"DEBUG: Querying revenue for {property_id} from {start_date} to {end_date}") + end_date = datetime(year + 1, 1, 1, tzinfo=local_tz).astimezone(UTC) + + logger.debug( + "Querying %s/%s (tz=%s) from %s to %s", tenant_id, property_id, property_timezone, start_date, end_date + ) + + from app.core.database_pool import db_pool + from sqlalchemy import text + + await db_pool.initialize() + + if not db_pool.session_factory: + raise RuntimeError("Database pool not available") - # SQL Simulation (This would be executed against the actual DB) - query = """ + query = text(""" 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 - """ - - # 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 + WHERE property_id = :property_id + AND tenant_id = :tenant_id + AND check_in_date >= :start_date + AND check_in_date < :end_date + """) + + async with db_pool.get_session() as session: + result = await session.execute(query, { + "property_id": property_id, + "tenant_id": tenant_id, + "start_date": start_date, + "end_date": end_date, + }) + total = result.scalar() + + return Decimal(str(total)) if total is not None else Decimal('0') 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}) - + # Use the shared pool singleton - constructing DatabasePool() per call built a + # brand new engine (and connection pool) on every request. + from app.core.database_pool import db_pool + + await db_pool.initialize() + + if not db_pool.session_factory: + raise RuntimeError("Database pool not available") + + 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 + } + + # No reservations for this property within this tenant. return { "property_id": property_id, - "tenant_id": tenant_id, - "total": mock_property_data['total'], + "tenant_id": tenant_id, + "total": "0.00", "currency": "USD", - "count": mock_property_data['count'] + "count": 0 } diff --git a/backend/requirements.txt b/backend/requirements.txt index 6b777d2aa..c9df5c6fb 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -18,7 +18,8 @@ pytz redis>=5.0.0 requests sendgrid -sqlalchemy>=2.0.0 +# [asyncio] pulls in greenlet, which create_async_engine requires at runtime. +sqlalchemy[asyncio]>=2.0.0 supabase tenacity>=8.0.0 uvicorn[standard] diff --git a/docker-compose.debug.yml b/docker-compose.debug.yml new file mode 100644 index 000000000..3ab9c4de9 --- /dev/null +++ b/docker-compose.debug.yml @@ -0,0 +1,15 @@ +# Debug overlay - adds a debugpy listener to the backend without changing the base image. +# +# docker compose -f docker-compose.yml -f docker-compose.debug.yml up +# +# Then attach with the VS Code launch config "Python: Attach to Backend (Docker)". +# debugpy is installed at container start, so no image rebuild is required. +services: + backend: + ports: + - "8000:8000" + - "5678:5678" # debugpy + command: > + sh -c "pip install --no-cache-dir debugpy && + python -m debugpy --listen 0.0.0.0:5678 + -m uvicorn app.main:app --host 0.0.0.0 --port 8000 --reload" diff --git a/frontend/src/components/RevenueSummary.tsx b/frontend/src/components/RevenueSummary.tsx index dbb6d0629..62aa6e1ac 100644 --- a/frontend/src/components/RevenueSummary.tsx +++ b/frontend/src/components/RevenueSummary.tsx @@ -61,7 +61,10 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr if (error) return
{error}
; if (!data) return null; - const displayTotal = Math.round(data.total_revenue * 100) / 100; + // Backend already rounds to cents using Decimal before serializing; + // re-rounding here with float math (x * 100 / 100) can reintroduce the + // exact binary floating-point drift the backend fix eliminates. + const displayTotal = data.total_revenue; return (
@@ -101,18 +104,6 @@ export const RevenueSummary: React.FC = ({ propertyId = 'pr

{data.reservations_count} bookings

- - {/* Precision Warning Area */} -
- {Math.abs(data.total_revenue - displayTotal) > 0.000001 && showRaw && ( -
- - - - Precision Mismatch Detected -
- )} -
);