Repository navigation
Fix revenue dashboard: tenant data leak, DB connection, money precision and timezone-aware months - #225
Fix revenue dashboard: tenant data leak, DB connection, money precision and timezone-aware months#225krowslyare wants to merge 12 commits into
Conversation
The pool never initialized, so every dashboard request silently fell back to mock figures. It read settings.supabase_db_* (undefined), used the sync-only QueuePool with an async engine, was used with `async with` on a coroutine, and greenlet (required by SQLAlchemy asyncio) was not installed. It was also re-created on every request; it is now initialized once at startup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the query failed, calculate_total_revenue returned a hardcoded table keyed only by property_id, ignoring the tenant. Clients saw plausible but fake totals: Ocean's prop-001 (no reservations) showed 1000.00, and Sunset could load prop-004 and see Ocean's real total. Errors are now logged with the traceback and surfaced as 503, and are never cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The key was `revenue:{property_id}`, but property IDs repeat across tenants.
Whichever client loaded prop-001 first populated the entry, and the other
client was served that payload (it even carried the other tenant_id) for the
next 5 minutes, which is why the leak only showed up "sometimes on refresh".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The summary endpoint answered for any property_id: Sunset could request prop-004 (Ocean's) and get a 200. Ownership is now checked against properties(id, tenant_id) before aggregating, and unknown and foreign properties get the same 404 so the response doesn't reveal which IDs exist. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TenantResolver mapped any unrecognised email to tenant-a, and the dashboard fell back to "default_tenant", so a user without a tenant would read another client's data. The tenant now comes from the verified token's app_metadata, then the known-user mapping, and the dashboard returns 403 when none exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
total_amount is NUMERIC(10,3) (sub-cent precision), but the API converted the
sum to float and the browser rounded it again with Math.round(x*100)/100.
Floats can't represent most cent values exactly, so double rounding can drift
by a cent. Totals now stay Decimal, are rounded once half-up at the API
boundary and returned as a decimal string ("2250.00"); the UI only formats it.
Rounding once on the total matters: prop-001's three 333.333 / 333.333 /
333.334 bookings sum to 1000.00, while rounding each booking first gives 999.99.
Also reports the reservations' real currency and rejects mixed-currency sums.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dashboard is labelled "monthly" but only exposed all-time totals, and calculate_monthly_revenue was an unused stub that returned 0 and built its range from naive (UTC) datetimes. A check-in at 2024-02-29 23:30 UTC is 1 March 00:30 in Paris, so a UTC month boundary moves that booking into February and March for prop-001 drops from 2250.00 (4 bookings) to 1000.00 (3). /dashboard/summary now accepts optional year+month and filters check-ins with half-open [start, end) bounds in the property's timezone; the period is part of the cache key. The unused stub is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The property selector was a hardcoded list of all five properties, so each client saw the other client's property names, and Ocean's prop-001 (Mountain Lodge Beta) was labelled "Beach House Alpha", which is Sunset's prop-001. It is now loaded from a new tenant-scoped GET /dashboard/properties. Also removes two misleading UI details: a hardcoded "12%" trend badge and the X-Simulated-Tenant header (always "candidate"), which the backend never reads; the tenant comes only from the auth token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dashboard is labelled "Monthly performance insights" but had no way to pick a month. Adds a month picker (property local time) that passes year/month to /dashboard/summary; leaving it empty shows the all-time total. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If loading the property list failed the dashboard rendered nothing, with no message; it now shows an error, or "No properties" for an empty account. Quickly switching property/month could let a slower, outdated response overwrite the current one; stale responses are now ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers the rounding policy (one half-up rounding on the total: 1000.00 vs 999.99 when rounding per booking) and month bounds in the property's timezone, including the Paris check-in that falls on 1 March local time and the DST month. Pure functions, no database needed: cd backend && uv run --no-project --with pytest pytest -q -p no:cacheprovider Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| database_url = settings.database_url.replace( | ||
| "postgresql://", "postgresql+asyncpg://", 1 | ||
| ) |
There was a problem hiding this comment.
Bug: String replacement in database_pool.py fails to handle postgres:// URLs, causing async engine creation to fail and permanently disabling the database session factory.
Severity: MEDIUM
Suggested Fix
Normalize database URLs using URL parsing (e.g. urllib.parse or sqlalchemy.engine.make_url) or handle prefixes like postgres:// and postgresql:// explicitly before passing the URL with dialect postgresql+asyncpg:// to create_async_engine.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: backend/app/core/database_pool.py#L17-L19
Potential issue: Database pool initialization relies on replacing `postgresql://` with
`postgresql+asyncpg://`. If `settings.database_url` uses the legacy `postgres://`
dialect prefix or an empty/malformed URL, the string replacement does not occur, leaving
the URI unchanged. Passing an unsupported scheme to `create_async_engine()` raises an
exception that is swallowed by a generic catch block, leaving `session_factory` as
`None` for the application lifecycle and causing subsequent database-dependent requests
to fail with 503 errors.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Good catch, fixed in 80e26e2: the URL is now parsed with make_url and the asyncpg driver is set explicitly, so postgres://, postgresql:// and postgresql+psycopg2:// all work.
The pool built its URL with a string replace of "postgresql://", so a legacy "postgres://" (or "postgresql+psycopg2://") DATABASE_URL would keep an unsupported driver and the pool would never start. Parse the URL and set the asyncpg driver explicitly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
The dashboard never reached the database (every request fell back to tenant-agnostic
mock data) and the Redis cache key ignored the tenant. Together this caused Sunset's
wrong March totals and Ocean seeing another company's numbers. One commit per bug.
Bugs fixed
revenue:{property_id}; IDs repeat across tenants/dashboard/properties; removed fake "12%" badge and unused headerTesting
cd backend && uv run --no-project --with pytest pytest -q -p no:cacheproviderRemaining lint errors already exist on
main.Out of scope (found, not fixed)
Hardcoded
SECRET_KEY,candidate@login without password check, RLS withoutpolicies, frontend session lost on reload.
How I used AI
Claude Code explored the code, reproduced bugs, implemented each fix and ran adversarial
reviews. I directed the plan, made the key decisions (money as decimal string, 404 vs 403,
month selector) and verified each fix with SQL, curl and the UI.