fix(billing): cumulative review — refund correlation + cleanup#3543
fix(billing): cumulative review — refund correlation + cleanup#3543PierreBrisorgueil merged 2 commits intomasterfrom
Conversation
CRITICAL: backfill PaymentIntent metadata with real cs_* session ID after creditPack succeeds in handleCheckoutPaymentCompleted, so charge.refunded events can correlate the charge back to the correct ledger entry (the __pending__ placeholder was breaking refundPartial lookups). Add 3 unit tests covering PI update, absent PI, and non-fatal failure path. HIGH: replace stale TODO PR-N3 in refundPartial with accurate comment — heuristic is still intentionally needed since refundPartial does not receive packId from the webhook call-site. HIGH: add Mixed type caveat JSDoc to billing.usage.model (meterBreakdown and counters fields) matching the existing note in billing.extraBalance.model. MEDIUM: document BillingExtraBalance.organization vs BillingUsage.organizationId asymmetry in billing.controller.js. MEDIUM: add startup priceUsd validation warning in billing.init.js so misconfigured packs are caught before a refund event exposes the gap.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a necessary architectural improvement by backfilling Stripe PaymentIntent metadata, which enables reliable refund correlation. Most acceptance criteria regarding documentation and webhook updates have been met.
However, a high-severity issue was identified in the refund logic where missing price metadata causes a full unit reversal instead of a proportional adjustment. Additionally, while the PR introduces startup validation for billing configurations, this logic is currently untested. Codacy analysis is up to standards, although it highlights systemic code duplication within the test suite that should be addressed to reduce maintenance overhead.
About this PR
- The new startup validation logic in 'billing.init.js' lacks unit or integration test coverage. Since this logic generates warnings for invalid configurations, it is important to verify that the validation correctly identifies and logs missing or non-numeric 'priceUsd' values.
- There is a systemic pattern of duplicated repository and service mock setups across multiple billing test files. This increases maintenance friction; consider extracting common Jest mocks for SubscriptionRepository, OrganizationRepository, and Stripe into a shared test utility.
1 comment outside of the diff
modules/billing/services/billing.extra.service.js
line 113🔴 HIGH RISK
The fallback logic for missing 'priceUsd' incorrectly refunds the entire pack amount regardless of the actual refund value. For a partial refund, this leads to an over-correction where the user loses more units than they were refunded for. If the price is unknown, the system should either skip the partial refund or return 'applied: false' to avoid data corruption.refundUnits = 0;
Test suggestions
- Verify PaymentIntent metadata is updated with the real session ID after a successful checkout
- Verify the webhook skip PaymentIntent update if the 'payment_intent' field is missing from the session object
- Verify Stripe API errors during metadata backfill are caught and do not fail the credit replenishment flow
- Verify that config validation in 'billing.init.js' correctly warns when 'priceUsd' is missing or non-numeric
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that config validation in 'billing.init.js' correctly warns when 'priceUsd' is missing or non-numeric
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const stripe = getStripe(); | ||
| if (stripe) { | ||
| try { | ||
| await stripe.paymentIntents.update(paymentIntentId, { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: This metadata backfill is a robust solution for the 'correlation' problem. By propagating the 'stripeSessionId' and 'packId' to the PaymentIntent, you ensure future events (like refunds) can be mapped back to internal ledger entries. Suggestion: Consider if additional metadata like 'userId' or 'environment' should also be propagated to improve long-term auditability.
| }, | ||
| }; | ||
|
|
||
| jest.unstable_mockModule('../repositories/billing.subscription.repository.js', () => ({ |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Duplicated mock setups identified. Extracting these into a shared utility would improve maintainability.
| import BillingExtraService from '../services/billing.extra.service.js'; | ||
| import BillingExtraBalanceRepository from '../repositories/billing.extraBalance.repository.js'; | ||
|
|
||
| // NOTE: BillingExtraBalance uses 'organization' (ObjectId ref); BillingUsage uses 'organizationId' (string field). |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The comment describes 'organizationId' as a 'string field', but the corresponding Mongoose schema actually defines it as a 'Schema.ObjectId'. It would be more accurate to describe it as an ObjectId reference to prevent confusion during query construction.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 57 minutes and 53 seconds.Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (72.72%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3543 +/- ##
==========================================
- Coverage 87.83% 87.78% -0.05%
==========================================
Files 128 128
Lines 3577 3587 +10
Branches 1048 1052 +4
==========================================
+ Hits 3142 3149 +7
- Misses 345 347 +2
- Partials 90 91 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Improves billing refund correlation for extras pack purchases by attempting to ensure the real Stripe Checkout session ID (cs_*) is available for later refund processing, plus adds documentation/warnings to reduce future billing misconfig and confusion.
Changes:
- After successful extras
creditPack, attempts to backfill Stripe PaymentIntent metadata with the real Checkout session ID for refund correlation. - Updates refund heuristic commentary and adds Mixed-type mutation caveat notes on usage model fields.
- Adds a startup warning for misconfigured packs missing
priceUsd, and documents model field asymmetry in the billing controller.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/billing/tests/billing.webhook.checkout.unit.tests.js | Adds unit coverage for PaymentIntent metadata update behavior and non-fatal failure handling. |
| modules/billing/services/billing.webhook.service.js | Adds Stripe PaymentIntent metadata update after extras credit to support later refund correlation. |
| modules/billing/services/billing.extra.service.js | Replaces stale TODO with an accurate explanation of the refund pack lookup heuristic. |
| modules/billing/models/billing.usage.model.mongoose.js | Documents Mixed-field validator caveats for counters/meterBreakdown. |
| modules/billing/controllers/billing.controller.js | Notes historical asymmetry between organization fields across billing collections. |
| modules/billing/billing.init.js | Warns at startup when configured packs have missing/invalid priceUsd. |
| // Backfill PaymentIntent metadata with the real session ID so that charge.refunded | ||
| // events can correlate the charge back to this ledger entry. | ||
| // At session.create time stripeSessionId was set to '__pending__' (Stripe forbids | ||
| // self-reference). Propagating the real cs_* ID here ensures charge.metadata carries | ||
| // it when a refund is issued later. | ||
| if (paymentIntentId) { | ||
| const stripe = getStripe(); | ||
| if (stripe) { | ||
| try { | ||
| await stripe.paymentIntents.update(paymentIntentId, { | ||
| metadata: { | ||
| organizationId, | ||
| packId, | ||
| kind: 'extras', | ||
| stripeSessionId, // real cs_* ID | ||
| }, |
| }); | ||
| } catch (err) { | ||
| // Log but don't fail — refund correlation may need fallback path | ||
| console.warn('[billing.webhook] PaymentIntent metadata update failed:', err.message); |
Summary
handleCheckoutPaymentCompletednow callsstripe.paymentIntents.updateaftercreditPacksucceeds, backfilling the realcs_*session ID onto PaymentIntent metadata. PreviouslystripeSessionId: '__pending__'was left on the PaymentIntent, socharge.metadata.stripeSessionIdwas always'__pending__'at refund time — causingrefundPartialto find no matching topup entry and silently skip. Non-fatal: PI update failure is caught/warned but never blocks the credit flow.refundPartialheuristic (meterUnits-based pack lookup) is still intentionally needed —handleChargeRefundeddoes not passpackIdtorefundPartial. Replaced misleading TODO with accurate comment explaining why the heuristic is retained.billing.extraBalance.modelonmeterBreakdownandcountersfields (Schema.Types.Mixedvalidators don't fire on in-place mutations).billing.controller.jsnotingBillingExtraBalance.organization(ObjectId ref) vsBillingUsage.organizationId(string field).billing.init.jsnow warns at boot if any configured pack is missing a validpriceUsd, surfacing misconfiguration before a refund exposes the gap.H2 (resetAllDue 7d race) skipped per scope — track as separate GitHub issue.
Test plan
billing.webhook.checkout.unit.tests.jscovering: PI update called with real session ID, PI update skipped whenpayment_intentabsent, PI update failure is non-fatal (resolves, creditPack still called)