Repository navigation
Fix per-tenant revenue - #229
radenpranantya wants to merge 5 commits into
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| total, count = await _monthly_totals(property_id, tenant_id, year, month) | ||
| return { | ||
| "property_id": property_id, | ||
| "tenant_id": tenant_id, | ||
| "total": mock_property_data['total'], | ||
| "tenant_id": tenant_id, | ||
| "total": f"{total:.2f}", | ||
| "currency": "USD", | ||
| "count": mock_property_data['count'] | ||
| "count": count, | ||
| } | ||
| except Exception as e: | ||
| print(f"Database error for {property_id} (tenant: {tenant_id}): {e}") | ||
| return _empty_summary(property_id, tenant_id) |
There was a problem hiding this comment.
Bug: Database exceptions are caught and fallback zero-revenue summaries are cached in Redis for 300 seconds instead of skipping cache on failure.
Severity: MEDIUM
Suggested Fix
Do not cache fallback or error responses in get_revenue_summary. Alternatively, re-raise exceptions from calculate_total_revenue or return an indicator to bypass redis_client.setex on failures.
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#L99-L110
Potential issue: In `calculate_total_revenue`, failures from `_monthly_totals` (such as
database connectivity issues, timeouts, or pool errors) are caught by a broad
`try/except Exception` block that logs the error and returns
`_empty_summary(property_id, tenant_id)`. Because `_empty_summary` populates the
matching `tenant_id`, the tenant verification check in `get_revenue_summary` passes. The
fallback summary with zeroed totals is then cached in Redis via `setex` for 300 seconds,
serving stale zero values to users even after database recovery.
Also affects:
backend/app/services/cache.py:35~45
Did we get this right? 👍 / 👎 to inform future reviews.
| year: int = REPORT_YEAR, | ||
| month: int = REPORT_MONTH, |
There was a problem hiding this comment.
Bug: Lack of parameter validation on year and month causes invalid inputs to swallow ValueError and return HTTP 200 with zero revenue.
Severity: LOW
Suggested Fix
Add parameter validation constraints (e.g. Query(ge=1, le=12) for month and valid range constraints for year) on the endpoint in backend/app/api/v1/dashboard.py.
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/api/v1/dashboard.py#L27-L28
Potential issue: The `/dashboard/summary` endpoint accepts `year` and `month` query
parameters without validating their ranges. Passing invalid values such as `month=13`
triggers a `ValueError` in `datetime` construction within `_month_window`. Because
`calculate_total_revenue` catches all exceptions, the error is swallowed and an empty
summary with zero revenue is returned with HTTP 200 and cached for 5 minutes instead of
returning a 4xx validation error.
Also affects:
backend/app/services/reservations.py:31~38
Did we get this right? 👍 / 👎 to inform future reviews.
|
https://www.loom.com/share/66c33e3c2d8a419e8771cbcdcdbf50c7 the upload is error, so I put the loom in here. Thank you! |
Summary
Test plan