Repository navigation
fix: isolate tenant revenue data, apply property timezones and exact decimal totals #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
32b854c
7d9e8d7
d9d9814
17fee5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,60 +1,46 @@ | ||
| import asyncio | ||
| from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker | ||
| from sqlalchemy.pool import QueuePool | ||
| import asyncpg | ||
| import logging | ||
| from ..config import settings | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| class DatabasePool: | ||
| def __init__(self): | ||
| self.engine = None | ||
| self.session_factory = None | ||
| self.pool = None | ||
|
|
||
| async def initialize(self): | ||
| """Initialize database connection pool""" | ||
| 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}" | ||
|
|
||
| self.engine = create_async_engine( | ||
| database_url, | ||
| poolclass=QueuePool, | ||
| pool_size=20, # Number of connections to maintain | ||
| max_overflow=30, # Additional connections when needed | ||
| pool_pre_ping=True, # Validate connections | ||
| pool_recycle=3600, # Recycle connections every hour | ||
| echo=False # Set to True for SQL debugging | ||
| ) | ||
|
|
||
| self.session_factory = async_sessionmaker( | ||
| bind=self.engine, | ||
| class_=AsyncSession, | ||
| expire_on_commit=False | ||
| self.pool = await asyncpg.create_pool( | ||
| settings.database_url, | ||
| min_size=1, | ||
| max_size=20 # Number of connections to maintain | ||
| ) | ||
|
|
||
| logger.info("✅ Database connection pool initialized") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Database pool initialization failed: {e}") | ||
| self.engine = None | ||
| self.session_factory = None | ||
| self.pool = None | ||
| raise | ||
|
|
||
| async def close(self): | ||
| """Close database connections""" | ||
| if self.engine: | ||
| await self.engine.dispose() | ||
| if self.pool: | ||
| await self.pool.close() | ||
| self.pool = None | ||
|
|
||
| async def get_session(self) -> AsyncSession: | ||
| """Get database session from pool""" | ||
| if not self.session_factory: | ||
| def get_session(self): | ||
| """Get database connection from pool (use with `async with`)""" | ||
| if not self.pool: | ||
| raise Exception("Database pool not initialized") | ||
| return self.session_factory() | ||
| return self.pool.acquire() | ||
|
|
||
| # Global database pool instance | ||
| db_pool = DatabasePool() | ||
|
|
||
| async def get_db_session() -> AsyncSession: | ||
| async def get_db_session(): | ||
| """Dependency to get database session""" | ||
| async with db_pool.get_session() as session: | ||
| yield session |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,109 +1,72 @@ | ||
| from datetime import datetime | ||
| from decimal import Decimal | ||
| from decimal import Decimal, ROUND_HALF_UP | ||
| from typing import Dict, Any, List | ||
|
|
||
| async def calculate_monthly_revenue(property_id: str, month: int, year: int, db_session=None) -> Decimal: | ||
| 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. | ||
| Calculates revenue for a specific month, using the property's local timezone | ||
| to decide which month a check-in belongs to. | ||
| """ | ||
| print(f"DEBUG: Querying revenue for {property_id} for {year}-{month:02d}") | ||
|
|
||
| start_date = datetime(year, month, 1) | ||
| if month < 12: | ||
| end_date = datetime(year, month + 1, 1) | ||
| else: | ||
| end_date = datetime(year + 1, 1, 1) | ||
|
|
||
| print(f"DEBUG: Querying revenue for {property_id} from {start_date} to {end_date}") | ||
|
|
||
| # SQL Simulation (This would be executed against the actual DB) | ||
| query = """ | ||
| 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 | ||
| SELECT SUM(r.total_amount) as total | ||
| FROM reservations r | ||
| JOIN properties p ON p.id = r.property_id AND p.tenant_id = r.tenant_id | ||
| WHERE r.property_id = $1 | ||
| AND r.tenant_id = $2 | ||
| AND (r.check_in_date AT TIME ZONE p.timezone) >= make_timestamp($3, $4, 1, 0, 0, 0) | ||
| AND (r.check_in_date AT TIME ZONE p.timezone) < make_timestamp($3, $4, 1, 0, 0, 0) + interval '1 month' | ||
| """ | ||
|
|
||
| # 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) | ||
|
Comment on lines
19
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The new Suggested FixUpdate Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
| 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() | ||
| # Import database pool | ||
| from app.core.database_pool import db_pool | ||
|
|
||
| # Initialize pool if needed | ||
| if db_pool.pool is None: | ||
| 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} | ||
|
|
||
| async with db_pool.get_session() as conn: | ||
| query = """ | ||
| SELECT | ||
| property_id, | ||
| SUM(total_amount) as total_revenue, | ||
| COUNT(*) as reservation_count | ||
| FROM reservations | ||
| WHERE property_id = $1 AND tenant_id = $2 | ||
| GROUP BY property_id | ||
| """ | ||
|
|
||
| row = await conn.fetchrow(query, property_id, tenant_id) | ||
|
|
||
| if row: | ||
| total_revenue = Decimal(str(row["total_revenue"])).quantize(Decimal("0.01"), rounding=ROUND_HALF_UP) | ||
| return { | ||
| "property_id": property_id, | ||
| "tenant_id": tenant_id, | ||
| "total": str(total_revenue), | ||
| "currency": "USD", | ||
| "count": row["reservation_count"] | ||
| } | ||
|
|
||
| mock_property_data = mock_data.get(property_id, {'total': '0.00', 'count': 0}) | ||
|
|
||
| else: | ||
| # No reservations found for this property | ||
| 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 | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| """Shared test configuration. | ||
|
|
||
| DB-backed tests run against a real Postgres loaded with database/schema.sql and | ||
| database/seed.sql. Point TEST_DATABASE_URL at it (default: localhost:5544, see | ||
| README of the test run command in the task notes). | ||
| """ | ||
| import os | ||
| from datetime import datetime, timezone | ||
|
|
||
| TEST_DATABASE_URL = os.getenv( | ||
| "TEST_DATABASE_URL", "postgresql://postgres:postgres@localhost:5544/propertyflow" | ||
| ) | ||
| # Must be set before any `app.*` import reads settings. | ||
| os.environ["DATABASE_URL"] = TEST_DATABASE_URL | ||
| os.environ.setdefault("REDIS_URL", "redis://localhost:6379/0") | ||
|
|
||
| import fakeredis | ||
| import pytest_asyncio | ||
|
|
||
| from app.core.database_pool import db_pool | ||
|
|
||
|
|
||
| @pytest_asyncio.fixture | ||
| async def fake_redis(monkeypatch): | ||
| from app.services import cache | ||
|
|
||
| client = fakeredis.FakeAsyncRedis() | ||
| monkeypatch.setattr(cache, "redis_client", client) | ||
| yield client | ||
| await client.aclose() | ||
|
|
||
|
|
||
| @pytest_asyncio.fixture(autouse=True) | ||
| async def reset_db_pool(): | ||
| """Each test runs on its own event loop, so drop the shared pool after it.""" | ||
| yield | ||
| await db_pool.close() | ||
| db_pool.pool = None | ||
|
|
||
|
|
||
| @pytest_asyncio.fixture | ||
| async def extra_reservation(): | ||
| """Insert a reservation for the test and delete it afterwards.""" | ||
| inserted = [] | ||
|
|
||
| async def _insert(res_id, property_id, tenant_id, amount, currency="USD", check_in=datetime(2024, 3, 10, 12, tzinfo=timezone.utc)): | ||
| if db_pool.pool is None: | ||
| await db_pool.initialize() | ||
| async with db_pool.get_session() as conn: | ||
| await conn.execute( | ||
| "INSERT INTO reservations (id, property_id, tenant_id, check_in_date, check_out_date," | ||
| " total_amount, currency) VALUES ($1, $2, $3, $4::timestamptz, $4::timestamptz + interval '2 days'," | ||
| " $5::numeric, $6)", | ||
| res_id, property_id, tenant_id, check_in, amount, currency, | ||
| ) | ||
| inserted.append(res_id) | ||
|
|
||
| yield _insert | ||
|
|
||
| if inserted: | ||
| if db_pool.pool is None: | ||
| await db_pool.initialize() | ||
| async with db_pool.get_session() as conn: | ||
| await conn.execute("DELETE FROM reservations WHERE id = ANY($1::text[])", inserted) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import pytest | ||
| from app.services.cache import get_revenue_summary | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_cached_revenue_is_isolated_per_tenant_for_same_property_id(fake_redis): | ||
| tenant_a = await get_revenue_summary("prop-001", "tenant-a") | ||
| tenant_b = await get_revenue_summary("prop-001", "tenant-b") | ||
|
|
||
| assert tenant_a["tenant_id"] == "tenant-a" | ||
| assert tenant_b["tenant_id"] == "tenant-b" | ||
| assert tenant_b["count"] == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The change to
resolve_tenant_idcan result inuser.tenant_idbeingNone. Thelist_departmentsendpoint doesn't filter by tenant in this case, leaking all departments.Severity: HIGH
Suggested Fix
The
authenticate_requestandverify_token_wsfunctions inauth.pyshould ensure a valid tenant ID is always resolved for authenticated users, or endpoints likelist_departmentsshould explicitly reject requests whereuser.tenant_idisNoneinstead of querying without a filter.Prompt for AI Agent
Did we get this right? 👍 / 👎 to inform future reviews.