Repository navigation
Fix/revenue dashboard bugs - #220
Open
ansab-rehman wants to merge 6 commits into
Open
ansab-rehman wants to merge 6 commits into
ansab-rehman wants to merge 6 commits into
Conversation
Both tenants seed a property with id prop-001, so a key of
revenue:{property_id} let whichever tenant loaded first serve its
totals to the other for the 300s TTL.
DatabasePool read settings.supabase_db_* which config.py never defines, so initialize() swallowed an AttributeError and left session_factory as None. Also drops QueuePool (invalid for create_async_engine), makes get_session an async context manager to match its call sites, and adds the sqlalchemy[asyncio] extra so greenlet is present.
calculate_total_revenue caught every DB error and returned a hardcoded, tenant-blind mock_data table, so the dashboard showed invented figures while the connection pool was broken. Errors now raise RevenueDataUnavailable and surface as 503.
Month boundaries were naive UTC datetimes compared against TIMESTAMPTZ, ignoring properties.timezone. A 2024-02-29 23:30Z check-in is 1 March in Europe/Paris, so 1250.00 fell out of March and prop-001 reported 1000.00 instead of 2250.00. calculate_monthly_revenue was also a stub with no tenant_id parameter; it now executes and is reachable via optional month/year query params.
total_amount is NUMERIC(10,3), so a total can land on a half cent. float(total) on the API and Math.round(x*100)/100 on the client both resolved those downwards: 0.005 displayed as 0.00. Totals are now quantized once with ROUND_HALF_UP and sent as an exact string.
resolve_tenant_id returned 'tenant-a' for any unknown user and the dashboard fell back to 'default_tenant', so an unrecognised account received Sunset Properties' revenue. Both now refuse to guess and the endpoint returns 403. Also removes the client-controlled X-Simulated-Tenant header the backend rightly ignored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Production Bug Investigation & Resolution
Investigated three client reports: Sunset’s March revenue mismatch, Ocean seeing another company’s figures, and Finance reporting discrepancies of a few cents.
Identified six defects and fixed them across six commits. The broken connection pool and the mock-data fallback that concealed it were treated as separate issues and addressed independently.
Key finding: Every dashboard figure was fabricated before this branch. The database was never successfully queried because an exception handler swallowed the connection failure and returned a hardcoded table of plausible-looking numbers. This masked the underlying defects and made the reports difficult to diagnose.
No rebuild was required. All changes were surgical edits to existing functions, preserving the current structure and coding patterns.
Defects identified and fixed
1. Revenue cache key ignored the tenant
Files:
cache.pySymptom: Ocean intermittently saw Sunset’s revenue, creating a tenant privacy issue.
Root cause: The cache key was
f"revenue:{property_id}", ignoring the existingtenant_idparameter. Property IDs were unique only within each tenant, so both tenants could useprop-001. Whichever tenant populated the cache first could serve its figures to the other for the entire 300-second TTL.Fix: Updated the key to include
tenant_idand the requested period, preventing cross-tenant and cross-period collisions. Redis failures are now non-fatal and fall back to the database.2. Database connection pool was unusable
Files:
database_pool.py,requirements.txtFour independent issues prevented database access:
The connection URL referenced undefined settings (
supabase_db_user,_password,_host,_port,_name) instead of the existingsettings.database_url.QueuePool, a synchronous pool, was passed tocreate_async_engine, which rejects it.get_sessionwas a plainasync def, while callers usedasync with.SQLAlchemy was installed without the asyncio extra, leaving
greenletunavailable.Fix: The engine now uses
settings.database_url, normalized to thepostgresql+asyncpg://scheme. Pool sizing comes from configuration, the incompatible explicit pool class was removed, andget_sessionis now an@asynccontextmanager. Initialization is idempotent, protected by a lock, and re-raises errors instead of swallowing them. Dependencies now specifysqlalchemy[asyncio]>=2.0.0.3. Database failures returned fabricated revenue
File:
reservations.pyRoot cause:
calculate_total_revenuecaught every exception and returned hardcodedmock_datakeyed only byproperty_id, without tenant isolation. For example, it returned1000.00forprop-001, matching the incorrect result produced by the timezone defect.Fix: Removed the mock data. Failures now raise
RevenueDataUnavailable, which the API maps to HTTP503with a neutral error message.A financial dashboard must never present invented numbers as verified revenue when the database is unavailable.
4. Monthly revenue calculations ignored property timezones
Files:
reservations.py,dashboard.pySymptom: Sunset’s March revenue was understated.
Root cause: Naive datetime boundaries were compared against a
TIMESTAMPTZcolumn without consideringproperties.timezone. A reservation stored at2024-02-29 23:30:00+00corresponds to2024-03-01 00:30inEurope/Paris. Its1250.000was excluded from March, producing1000.00instead of2250.00.An additional issue was that
calculate_monthly_revenuewas an unreachable stub returningDecimal('0'), with its query commented out and notenant_idparameter.Fix: The query now converts reservation timestamps using each property's timezone and applies a half-open monthly interval. The monthly calculation is implemented, tenant-scoped, and exposed through optional
monthandyearquery parameters. Inputs are validated (month: 1–12;year: 2000–2100), and requests supplying only one parameter are rejected with HTTP400.Using each property's timezone avoids correcting one client's totals while breaking another's.
5. Monetary calculations used binary floating point
Files:
dashboard.py,RevenueSummary.tsxSymptom: Finance reported revenue discrepancies of a few cents.
Root cause: The API converted
Decimalvalues tofloat, and the frontend rounded usingMath.round(data.total_revenue * 100) / 100. Binary floating-point representation can place values just below their intended midpoint, causing half-cent values such as0.005to round down to0.00.Fix: Revenue remains a
Decimaland is quantized once using explicitROUND_HALF_UP. The API serializes the result as a string, and the frontend now typestotal_revenueasstringand formats it without converting it to a floating-point number.6. Unknown users could be assigned a real tenant
Files:
tenant_resolver.py,dashboard.py,secureApi.tsRoot cause:
resolve_tenant_iddefaulted unknown accounts to"tenant-a". The dashboard endpoint had a correspondingdefault_tenantfallback, potentially exposing Sunset's live revenue to unrecognized users.Fix: Tenant resolution now returns
Nonefor unknown accounts, and the endpoint responds with HTTP403instead of guessing. The client-suppliedX-Simulated-Tenantheader was also removed fromsecureApi.ts. The backend derives the tenant from the bearer token server-side.Verification results
All figures were verified against the live application using
docker compose, covering both tenants after the fixes.|
Property
|
Sunset (
tenant-a)|
Ocean (
tenant-b)|
| --- | --- | --- |
|
prop-001|
2250.00/ 4|
0.00/ 0|
|
prop-002|
4975.50/ 4|
—
|
|
prop-003|
6100.50/ 2|
—
|
|
prop-004|
0.00/ 0|
1776.50/ 4|
|
prop-005|
—
|
3256.00/ 3|
Additional checks passed:
prop-001, March 2024:2250.00/ 4 reservations; February 2024:0.00/ 0.Side-by-side SQL comparison confirmed
march_naive_utc = 1000.000versusmarch_property_tz = 2250.000.Nine tenant-scoped Redis keys verified, with no cross-tenant collisions.
Stopping the database using
docker compose stop dbresulted in HTTP503with a neutral message. Service recovered after restart throughpool_pre_ping.Unknown accounts received HTTP
403, including requests with a validly signed JWT claimingtenant_id: tenant-a.Ocean continued receiving its own
0.00when sendingX-Simulated-Tenant: tenant-a.npx tsc --noEmitcompleted successfully.Out of scope: Issues flagged for follow-up
The following issues were identified but deliberately left unchanged because they were outside the scope of these fixes:
Database RLS:
database/schema.sqlenables row-level security but defines no policies. Tenant isolation currently depends on application-level enforcement.Hardcoded secret:
SECRET_KEYis set todebug_challenge_secretindocker-compose.yml. The current isolation checks assume trustworthy token signing, but this key is not suitable for production.Hardcoded frontend properties:
Dashboard.tsxcontains a non-tenant-scopedPROPERTIESlist and currently relies on the API returning zeros for properties a tenant does not own.Legacy health endpoint:
GET /healthreportsdegradeddue to a stubbed Supabase-style client inbackend/app/database.py. The dashboard path does not use this component, so it was left untouched.Missing
.gitignore:__pycache__andnode_modulesappear as untracked files.