add invitation system - #475
Conversation
|
Test Results98 tests 98 ✅ 8s ⏱️ Results for commit b62f0b9. ♻️ This comment has been updated with latest results. |
|
|
sol-wizard
left a comment
sol-wizard
left a comment
There was a problem hiding this comment.
🤖 AI-generated PR review. Treat the inline comments as suggestions, not verdicts — use your own judgement and verify each before changing anything.
Core flow looks sound. The three Major inline comments are what I'd resolve before merge.
One PR-level issue (no line to anchor to): the description doesn't match the code.
It describes an InviteRedemptions table, an InviteCode column on AppUsers, /api/invite/* routes, and a redeem input in Settings. Actual: ReferralCodes + Referrals, api/referral/*, and no redeem input in Settings — the migration drops InviteRedemptions and InviteCode. Looks like the description describes the first iteration. Worth updating since pr-release-note-check.yml gates on it.
Nits — feel free to ignore
nit:naming split across the stack — mobilefeature/invite/+InviteCodeDTOvs backendReferrals/ReferralCode/api/referral.nit:whitespace-only edits inAppUserConfiguration.cs,AppUser.cs,BlotzTaskDbContext.cs.speculative — feel free to ignore:a concurrent first-timeGET /my-codecould double-insert and surface a 409 from the unique index; retry fixes it, and the double-redeem race is correctly absorbed byDbUpdateException→ 409. I'd leave both.
Test worth adding: one handler test over RedeemReferralCode covering self-referral / already-redeemed / onboarding-gate. User-isolation logic, cheap with an in-memory context, and the comment on line 36 shows those branches are easy to conflate.
Assumptions: diff-only review, nothing built or run. I assumed InviteRedemptions / AppUsers.InviteCode were merged to main earlier, since this migration drops them without creating them — please confirm they were never populated in staging/prod, as DropTable is destructive with no data-migration step. I did not check badge seed config for the new enum values.
2cc9e13 to
6f5afc9
Compare
LiChenyang-GZ
left a comment
LiChenyang-GZ
left a comment
There was a problem hiding this comment.
AI-generated review. These are suggestions from an automated senior-level pass, not verdicts. Please verify each point yourself before acting — some may be wrong or missing context I don't have.
Solid feature overall. Things that are clearly right: the unique index on Referrals.RefereeUserId plus the Restrict FKs means a concurrent double-redeem gets caught by the DB even if it slips past the AnyAsync check (and DbUpdateException maps to 409, which happens to be the correct status). DeleteUser clearing Referrals manually while ReferralCodes cascades is the right split. ToUpperInvariant() normalisation lines up with the uppercase-only generator alphabet, and dropping I/O/0/1 from it is a nice touch.
dotnet build passes locally (0 errors).
Inline comments cover the concrete code issues. Three things that don't fit inline:
1. The PR description doesn't match the diff
| Description says | Actual |
|---|---|
InviteRedemptions table + AppUsers.InviteCode column |
ReferralCodes + Referrals tables |
GET/POST /api/invite/* |
/api/referral/my-code, /api/referral/redeem |
| Settings has an input to redeem another user's code | Settings only shows/copies your own code; redeem lives in onboarding |
| — | No mention of the 403 "onboarding only" restriction, which is the single biggest behavioural constraint in this PR |
| — | No mention that onboarding goes from 3 steps to 4 |
This affects the release note too: it says "find it in Settings", and users can indeed see their code there, but there is no entry point anywhere to use someone else's code after onboarding. As written the checklist is misleading.
2. Question: is the onboarding-only window intended?
handleFinish in onboarding-screen.tsx is shared by both the Skip button and Get Started, so either path sets IsOnboarded = true permanently. Two consequences: a user who taps Skip can never redeem, and every existing user is already onboarded, so none of them can participate at all.
If that's a deliberate anti-abuse decision it's fine — just worth stating in the description. If not, either add a redeem entry point in Settings, or relax the backend rule to something like "within N days of signup and not yet redeemed".
3. Minor / nits — feel free to ignore
EnsureReferralCode's two-phase write (insert withCode = null, then update) exists only because the code is derived from the identityId. The cost is thatCodestays nullable forever and every new user pays twoSaveChanges. A random 8-char code from the same alphabet plus a uniqueness retry would letCodebeIsRequired(). Both designs are defensible; the current one works.EventValueKey.InviteCount/TriggerAction.InviteRedeemedare unused//To doplaceholders — probably cleaner to add them in the badge PR that actually consumes them.- Naming is split three ways: backend
Referral, route/api/referral, mobilefeature/inviteand "invite" copy. Settling on one word would save real grep time later. - Whitespace-only edits in
AppUser.cs,AppUserConfiguration.csandBlotzTaskDbContext.csadd diff noise.
One test worth writing
RedeemReferralCodeCommandHandler's three guards (self-referral, double redeem, onboarding gate) are user-isolation logic that breaks silently when the onboarding rule changes, and they're short to cover with an in-memory/SQLite context. I wouldn't add anything beyond that here.
Both flagged issues are fixed: the migration no longer drops InviteRedemptions/AppUsers.InviteCode (confirmed no references remain), and the mobile hook now uses meta.errorMap (read by queryClient) instead of the unused errorNs.
| { | ||
| CompleteOffsetMins | ||
| CompleteOffsetMins, | ||
| //To do |
There was a problem hiding this comment.
what is this todo ?
example of TODO: //TODO: Need to complete this section and use in reward once product manager confirm what to do with the invitation badge
4 participants
Summary
Adds a referral code system:
Backend:
New ReferralCodes and Referrals tables (replaces the old InviteRedemptions table and InviteCode column on AppUsers)
GET /api/referral/my-code — lazily ensures and returns the user's unique referral code (self-healing: generates one on first access if it's missing, regardless of whether the Auth0 login webhook already created it)
POST /api/referral/redeem — validates and records a redemption (only allowed before onboarding is complete; prevents self-use and double redemption)
Codes are derived from each row's own auto-increment Id via Sqids (guaranteed unique by construction, no random-collision retries needed), using an uppercase-only alphabet that excludes visually ambiguous characters
Error handling: 404 code/user not found, 400 self-referral, 403 already onboarded, 409 already redeemed
Frontend:
Invite code redemption added to the onboarding flow, plus an "Invite Friends" entry in Settings showing the user's own code
One-tap copy for the user's own code
Input field (up to 12 characters) to redeem another user's code
Localised error messages (EN/ZH) per error type
Design note: redemption is intentionally limited to the onboarding window — this is a deliberate anti-abuse decision, not an oversight. Consequences: existing (already-onboarded) users cannot participate, and tapping Skip on any onboarding step permanently forfeits the ability to redeem, since it marks the user as onboarded. There is no redeem entry point outside onboarding by design.