Property Management System Review and Bug fixs - #154
Open
MichaelGetu-git wants to merge 4 commits into
Open
Conversation
The pool built its URL from supabase_db_* settings that do not exist in config, and passed QueuePool to an asyncio engine. Both raised, so every request silently fell back to hardcoded mock totals: prop-001 reported 1000.00 across 3 bookings where the database holds 2250.000 across 4. get_session was also a coroutine, so 'async with' could not use it.
prop-001 exists for both tenant-a and tenant-b as different properties. Keying the cache on property_id alone meant whichever tenant requested first served their totals to the other for the full 5 minute TTL. Ocean Rentals saw 2250.00 across 4 bookings belonging to Sunset Properties, where their own prop-001 has no reservations at all.
Amounts are NUMERIC(10,3) and the API converted them with float(), which snaps to the nearest binary double. Values like 333.333 have no exact binary form, so totals drifted by fractions of a cent. The value is now quantized to 2dp with ROUND_HALF_UP and serialized as a string.
Month boundaries were built as naive datetimes and compared against timestamptz check_in dates, so every month ran midnight to midnight UTC regardless of where the property is. Beach House Alpha is Europe/Paris, where a booking checking in 2024-03-01 00:30 local is 2024-02-29 23:30 UTC, so March dropped it and reported 1000.000 instead of 2250.000. The function also referenced tenant_id without accepting it and returned a hardcoded zero.
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.
Summary
Investigated the two client reports and the finance team's rounding complaint.
Found four bugs. Three of them were compounding, and one was masking the other
two, so the dashboard looked healthy while serving fabricated numbers.
The bugs
1. Revenue queries never reached the database
DatabasePool.initialize()built its connection string fromsettings.supabase_db_user,supabase_db_password,supabase_db_host,supabase_db_portandsupabase_db_name.None of those exist on the
Settingsobject. The setting that does exist isdatabase_url, already pointing at the right Postgres instance.The pool therefore threw on every initialization, and
calculate_total_revenuecaught the exception and returned a hardcoded dictionary of mock revenue. No error
reached the client, nothing was logged that operations would see, and the numbers
looked plausible. The dashboard had never read the database.
Fixed by building the async URL from
database_urlwith the asyncpg driver.2. Cache key was not scoped by tenant
property_idis only unique within a tenant. The schema makes this explicit with acomposite primary key of
(id, tenant_id), and the seed data hasprop-001asBeach House Alpha in Paris for tenant-a and Mountain Lodge Beta in New York for
tenant-b. The revenue cache keyed on
revenue:{property_id}alone, so whicheverclient requested a shared ID first wrote their totals into a slot the other client
then read for the next five minutes.
This is deterministic rather than a race, and it is what Ocean Rentals reported.
Fixed by scoping the key to
revenue:{tenant_id}:{property_id}.3. Month boundaries ignored the property timezone
properties.timezoneexists and is populated (Europe/Paris, America/New_York) butnothing read it.
calculate_monthly_revenuebuilt naive datetimes for the monthboundaries, which compare as UTC against a
TIMESTAMP WITH TIME ZONEcolumn.Reservation
res-tz-1checks in at2024-02-29 23:30+00. Beach House Alpha is inParis, so locally that is
2024-03-01 00:30, a March booking worth 1250.00. UTCboundaries filed it under February and dropped it, which is the March discrepancy
Sunset Properties reported.
Fixed by constructing the boundaries in the property's own timezone.
4. Money was cast to a float
Amounts are stored as
NUMERIC(10, 3). The dashboard endpoint ranfloat(revenue_data['total'])before serialising, which rounds to the nearestbinary double. The seed data exposes this directly:
333.333 + 333.333 + 333.334is exactly
1000.000as a Decimal and999.9999999999999as a float.Fixed by quantizing to two decimal places and returning an exact decimal string.
The frontend type and display were updated to match.
Verification
Checked both client logins against the values in Postgres.
Redis now holds
revenue:tenant-a:prop-001andrevenue:tenant-b:prop-001asseparate entries. Backend logs show no pool initialization failures after restart.
Notes
The mock fallback in
calculate_total_revenueis still in place. It is harmlesswhile the database connects, but it is the reason a total outage presented as
healthy data. I would remove it so that a database failure surfaces as an error
rather than as convincing fiction, but that changes error handling behaviour beyond
the scope of a debugging fix, so I left it for review.