Practical Test - The Flex - Rahul Trivedi - #137
Open
slidracoon72 wants to merge 2 commits into
Open
Conversation
The dashboard was never reaching the database: DatabasePool built its DSN
from settings fields that do not exist, passed a sync QueuePool to
create_async_engine, and exposed get_session() as a coroutine that callers
used with `async with`. Every failure was caught and answered with a
hardcoded "mock data for testing" dictionary, so clients were shown
invented revenue. Point the pool at DATABASE_URL, make the session usable,
reuse the singleton, and delete the fallback - a revenue endpoint that
cannot reach its data must fail, not improvise.
Tenant isolation: the revenue cache key was `revenue:{property_id}`, but
property IDs are only unique within a tenant (both clients own a prop-001),
so whichever tenant missed the cache first served its revenue to the other
for the next five minutes. Key on tenant + property + period, verify the
cached payload belongs to the caller, and fall through to the database when
Redis is unavailable. The endpoint also defaulted an unresolved tenant to a
shared "default_tenant" bucket and never checked property ownership; there
is now no default tenant, and another tenant's property returns 404.
Timezone: month boundaries were naive UTC datetimes compared against
timestamptz check-ins, shifting every month by the property's UTC offset.
A 2024-02-29 23:30 UTC check-in is 00:30 on 1 March in Paris, so Sunset's
March was short 1250.00 and one booking. Build the bounds in the property's
own timezone and convert to UTC; report the timezone and period used.
Precision: money went Decimal -> float -> JSON number -> JS double ->
Math.round, so halfway amounts rounded the wrong way. Keep the sum in
Decimal, round once with ROUND_HALF_UP at the API edge, and transport it as
a string alongside the exact value.
Also: property list is now fetched per tenant instead of hardcoding every
client's portfolio, the client-supplied X-Simulated-Tenant header is gone,
getTenantId reads the app_metadata claim and no longer deletes a working
session over an ID format, a stale-response race in RevenueSummary is
fixed, and the hardcoded "+12%" trend badge is removed.
Verified by scripts/verify_fixes.sh (19 end-to-end checks against both
client logins) and backend/tests/test_revenue_fixes.py (17 unit tests).
I could not cover all the bugs in the video as I had only 5 minutes on my Loom subscription. So all the changes are explained in detail in FINDINGS.MD
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.
I, Rahul Trivedi, have completed this assignment as part of the Flex interview process.
I could not cover all the bugs and fixes in the Loom video as I had only 5 minutes on my Loom subscription. So all the changes are explained in detail in FINDINGS.MD file.
Here is the Loom video link:
https://www.loom.com/share/f89cef0020ba4c82ad7dc7b724935283