Repository navigation
Conversation
The DB pool built its URL from nonexistent supabase_db_* settings, so every query failed and calculate_total_revenue silently returned hardcoded mock totals keyed only by property_id. Build the asyncpg pool from settings.database_url, share it, and let DB errors surface.
The revenue cache key was revenue:{property_id}, and prop-001 exists in two
tenants, so one client could see the other's cached totals. Unresolved
tenants also defaulted to tenant-a. Key the cache by tenant and reject
users without a tenant with 403.
calculate_monthly_revenue used naive UTC month bounds and returned a placeholder 0. Query the real data and evaluate month bounds in each property's local timezone, so res-tz-1 (Feb 29 23:30 UTC) counts in March for a Paris property.
Amounts are NUMERIC(10,3) and the total was returned unrounded. Round the Decimal total once with ROUND_HALF_UP so cent-level totals match finance.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+91
to
+92
| # Unknown users must never be silently assigned to another client's tenant. | ||
| return None |
There was a problem hiding this comment.
Bug: The change to resolve_tenant_id can result in user.tenant_id being None. The list_departments endpoint doesn't filter by tenant in this case, leaking all departments.
Severity: HIGH
Suggested Fix
The authenticate_request and verify_token_ws functions in auth.py should ensure a valid tenant ID is always resolved for authenticated users, or endpoints like list_departments should explicitly reject requests where user.tenant_id is None instead of querying without a filter.
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/tenant_resolver.py#L91-L92
Potential issue: The `TenantResolver.resolve_tenant_id` function now returns `None` for
any authenticated user not matching three specific emails. Previously, these users
received a default tenant ID. Endpoints like `GET /departments` are not prepared for a
`None` tenant ID. The `list_departments` function only applies its tenant filter if
`user.tenant_id` is truthy. For a user with a `None` tenant ID (like an admin), the
query runs unfiltered, returning departments from all tenants. This introduces a new
cross-tenant data leak. Other endpoints may fail or write incorrect data.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
19
to
+29
| """ | ||
|
|
||
| # 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 | ||
|
|
||
| from app.core.database_pool import db_pool | ||
|
|
||
| if db_pool.pool is None: | ||
| await db_pool.initialize() | ||
|
|
||
| async with db_pool.get_session() as conn: | ||
| total = await conn.fetchval(query, property_id, tenant_id, year, month) | ||
|
|
||
| return Decimal(str(total or 0)).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) |
There was a problem hiding this comment.
Bug: The new calculate_monthly_revenue function is not called by the dashboard endpoint it's intended to fix, leaving the original timezone bug in place. The fix is dead code.
Severity: MEDIUM
Suggested Fix
Update get_revenue_summary in dashboard.py to call the new calculate_monthly_revenue function instead of calculate_total_revenue. This will apply the intended timezone-aware, monthly revenue calculation to the dashboard summary.
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/services/reservations.py#L4-L29
Potential issue: The new timezone-aware function `calculate_monthly_revenue` is not
actually used by any production code path. The `/dashboard/summary` endpoint, which this
change is intended to fix, still calls the old `get_revenue_summary` function, which in
turn uses `calculate_total_revenue`. This old function does not filter by month or
handle timezones. As a result, the dashboard's revenue calculation remains incorrect and
the fix for the reported timezone issue is not applied.
Did we get this right? 👍 / 👎 to inform future reviews.
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
1 participant
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.
Fix revenue dashboard: tenant data leak, timezone month bounds and cent-level precision
Debugging fixes only. Every change maps to a reported symptom; no frontend changes, no dependency changes, no new endpoints.
supabase_db_*settings that do not exist; every query failed and a hardcoded mock dict keyed only byproperty_idwas returnedsettings.database_urlwith asyncpg; mock fallback removed so DB errors surfacerevenue:{property_id}ignores the tenant andprop-001exists for both tenants; unresolved tenants defaulted totenant-acalculate_monthly_revenuewas a stub returning 0 and used naive UTC month bounds, ignoringproperties.timezone(res-tz-1checks in 2024-02-29 23:30 UTC = 2024-03-01 00:30 Paris)AT TIME ZONE p.timezoneNUMERICamounts were never rounded, then converted to float and rounded again in the browserDecimalrounded once in the backend withROUND_HALF_UPCommits
fix: query the real database instead of mock datafix: isolate revenue cache and tenant resolution per tenantfix: compute monthly revenue in the property's timezonefix: round revenue totals to cents half-up onceVerification
backend/tests/(10 tests, written test-first) run against the real schema and seed; Redis is faked.prop-001= 2250.00 (4 reservations), Oceanprop-001= 0.00 (0 reservations).Run the tests inside the stack: