feat(billing): notify user when an automatic top-up succeeds - #3577
feat(billing): notify user when an automatic top-up succeeds#3577baktun14 wants to merge 2 commits into
Conversation
Automatic wallet-balance top-ups were credited silently. Users can be surprised by a charge they don't remember triggering and have no in-product record of it. Tag PaymentIntents created by the reload job with an auto_topup marker so the webhook settlement path can tell automatic top-ups from manual "Add Funds" charges. When such a charge actually credits the wallet (the row transitions created -> succeeded on this delivery), the settlement returns an AutoTopUpSucceeded descriptor that the webhook dispatcher publishes after commit, mirroring the first-purchase-bonus flow. A new handler emails the user the amount charged, their resulting available balance, and a link to billing. Exactly-once is enforced at three layers: the Stripe idempotency key per job, the settle guard that no-ops an already-succeeded row, and a deterministic per-transaction notificationId used as the broker singletonKey. Manual top-ups and skipped checks never notify. Ref CON-793
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR marks automatic top-ups in Stripe metadata, propagates structured settlement outcomes, publishes ChangesAutomatic top-up settlement and notification flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/api/src/app/providers/jobs.provider.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/api/src/app/services/auto-top-up-succeeded/auto-top-up-succeeded.handler.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/api/src/app/services/auto-top-up-succeeded/auto-top-up-succeeded.handler.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3577 +/- ##
==========================================
- Coverage 76.33% 75.55% -0.78%
==========================================
Files 1134 1047 -87
Lines 29598 27288 -2310
Branches 7352 6894 -458
==========================================
- Hits 22593 20617 -1976
+ Misses 6174 5868 -306
+ Partials 831 803 -28
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the reported inline findings, I also checked two other candidate issues and ruled them out:
- Stale balance in the email:
topUpWalletawaits the on-chainauthorizeSpendingcall and commits beforesettleSucceededTransactionreturns, and theAutoTopUpSucceededevent is only published after that commit — so by the time the notification handler readsgetDeploymentBalanceInFiat, the credited balance is already reflected, not stale. - Bonus-publish failure dropping the auto-top-up event:
DomainEventsService.publish()catches its own enqueue errors internally and returnsnullrather than throwing, so a failedFirstPurchaseBonusGrantedpublish does not prevent the subsequentAutoTopUpSucceededpublish instripe-webhook.service.ts.
Extended reasoning...
Verified two candidate issues raised by finder agents against the actual code paths (RefillService.topUpWallet ordering relative to commit/publish, and DomainEventsService.publish's internal try/catch) and confirmed neither is a real bug. Recording this so it isn't re-explored in a future pass.
- Link the success email to plain billing instead of CONSOLE_WEB_PAYMENT_LINK, whose openPayment=true query auto-opens the manual Add-Funds modal — wrong for an informational "you were already charged" email. - Reuse centsToUsd for the amount conversion instead of an ad hoc /100. - Move the notificationId dedup note to a JSDoc block (no inline comments).
There was a problem hiding this comment.
Reviewed and no bugs found this run. Since my last pass on this PR, the auto-top-up email link now strips the ?openPayment=true param before use and the two style nits (inline comment, cents→USD reuse) have both been addressed.
What was reviewed: the auto_topup metadata marker threading through both reload paths, the webhook settlement's SettlementOutcome (bonusGrant/autoTopUp) publish-after-commit flow, and the new notification handler/template. Checked exactly-once guarantees (idempotency key, settle-guard no-op, deterministic notificationId singletonKey) — logic holds. Two candidate issues (fragile billing-URL string split; a bonus-publish failure dropping the auto-top-up notification on the same delivery) were flagged by the bug-hunting system and ruled out.
Given this touches Stripe webhook settlement and billing-notification logic — a financial critical path — a human look is still worthwhile even with no findings.
Extended reasoning...
This PR adds an "auto top-up succeeded" email notification: it tags reload-job PaymentIntents with an auto_topup metadata marker, has the webhook settlement path return that as part of a new SettlementOutcome (alongside the existing first-purchase-bonus grant), and publishes an AutoTopUpSucceeded domain event after commit that a new job handler turns into an email via Novu.
I reviewed this PR previously and flagged one real bug (the email linked to CONSOLE_WEB_PAYMENT_LINK, which carries ?openPayment=true and auto-opens the manual Add-Funds modal) plus two style nits (an inline // comment that should be JSDoc, and re-deriving cents→USD instead of reusing centsToUsd). All three have since been addressed in the current diff: #billingUrl() now strips the query string, the singletonKey rationale is now a JSDoc block, and the template now calls centsToUsd.
This run's bug-hunting system found no new bugs. It raised and ruled out two candidates: the fragility of the .split("?")[0] fix for the billing link, and whether a bonus-publish failure could permanently drop the auto-top-up notification on retry (since a retried webhook delivery finds the transaction already succeeded and short-circuits before recomputing the outcome). Neither was confirmed as a real, actionable bug.
Security risk is limited: no new user input is parsed, the Stripe webhook signature verification path is unchanged, and the notification only echoes server-computed amounts/balances. The main risk surface is correctness of the exactly-once notification guarantees, which the PR addresses with three layers (Stripe idempotency key, settle-guard no-op, deterministic notificationId singletonKey) and covers with dedicated tests (98% patch coverage, new unit/integration specs for the settle-path autoTopUp assembly and webhook dispatch).
Given the change sits in Stripe webhook settlement and billing-notification code — a financial-critical path, even though this specific change is additive and well-tested — I think a human review is still the right call rather than an automated shadow-approval, despite the review turning up nothing further this run.
|
I don't like that this PR mixes 2 different terms "auto top up" which deployment escrow balance autopup and auto reload/auto charge which is charge of user credit card. |
2 participants
Why
Fixes CON-793
Automatic wallet-balance top-ups are credited silently today. Users can be surprised by a charge they don't remember triggering and have no in-product record that it happened. Part of the CON-717 fixed-threshold GA rollout; the failure case is tracked separately (CON-764).
What
auto_topupmetadata marker so the webhook settlement path can distinguish automatic top-ups from manual "Add Funds" charges. Applies to both the fixed-threshold and legacy predicted-spend paths — all automatic top-ups notify.created → succeededon this delivery), the settlement returns anAutoTopUpSucceededdescriptor that the webhook dispatcher publishes after commit, mirroring the existing first-purchase-bonus flow (never fires on a rolled-back credit).notificationIdused as the brokersingletonKey. Manual top-ups and skipped checks never notify.Tests: new handler + template specs, webhook publish coverage (auto vs manual vs replayed/no-op settle), and integration coverage of the settle-path
autoTopUpassembly.Summary by CodeRabbit
New Features
Bug Fixes