Fix/revenue dashboard bugs - #170
Open
MuDavid wants to merge 3 commits into
Open
Conversation
Revenue cache keys and the DB-unavailable fallback data were keyed by property_id alone. Since property IDs are only unique per tenant (see the composite primary key on properties), two different tenants requesting the same property_id could receive each other's cached revenue data. Both the cache key and the fallback lookup are now scoped by (property_id, tenant_id).
calculate_monthly_revenue was a stub that always returned zero, and the revenue path actually in use summed reservations over all time with no date filtering at all, so a monthly report could never match totals a client computed from the raw booking data for a given month. Implemented the monthly aggregation: month boundaries are built in the property's own timezone (read from properties.timezone) and converted to UTC before comparing against check_in_date, so a reservation is bucketed by the date it falls on for that property, not the server's UTC date. Added month/year as optional query params on /dashboard/summary, and extended the cache key to include them so a monthly total can't collide with the all-time total in cache.
Revenue totals are tracked with sub-cent precision (NUMERIC(10,3)) and computed as Decimal throughout, but were cast straight to float with no rounding step, so the cent value that reached the client depended on incidental binary floating-point representation rather than a deliberate rounding rule. Quantize to 2 decimal places with standard half-up rounding before the float conversion.
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
Fixes the three issues reported in ASSIGNMENT.md:
Cross-tenant revenue leak (Client B / Ocean Rentals) — the Redis cache key and
the DB-fallback lookup were keyed by
property_idalone. Since property IDs areonly unique per tenant, two tenants sharing the same
property_idcould receiveeach other's cached revenue. Both are now scoped by
(property_id, tenant_id).Timezone-naive monthly revenue (Client A / Sunset Properties) — monthly
totals were computed with naive UTC month boundaries, so a reservation near a
month boundary could be bucketed into the wrong month relative to the property's
own local timezone. Month boundaries are now computed in the property's timezone
and converted to UTC before querying.
Imprecise cent rounding (finance team) — revenue totals were cast straight
from
Decimaltofloatwith no explicit rounding, so the cent value dependedon incidental binary floating-point representation. Totals are now quantized to
2 decimal places with standard half-up rounding before conversion.
See commit messages for full root-cause detail on each fix.
Video walkthrough
https://www.loom.com/share/1e8f68cb2b76465aa3769d6706ba2e52