Skip to content

fix(billing): cumulative review — refund correlation + cleanup#3543

Merged
PierreBrisorgueil merged 2 commits intomasterfrom
fix/billing-cumulative-review
Apr 30, 2026
Merged

fix(billing): cumulative review — refund correlation + cleanup#3543
PierreBrisorgueil merged 2 commits intomasterfrom
fix/billing-cumulative-review

Conversation

@PierreBrisorgueil
Copy link
Copy Markdown
Contributor

Summary

  • CRITICAL — refund correlation fix: handleCheckoutPaymentCompleted now calls stripe.paymentIntents.update after creditPack succeeds, backfilling the real cs_* session ID onto PaymentIntent metadata. Previously stripeSessionId: '__pending__' was left on the PaymentIntent, so charge.metadata.stripeSessionId was always '__pending__' at refund time — causing refundPartial to find no matching topup entry and silently skip. Non-fatal: PI update failure is caught/warned but never blocks the credit flow.
  • HIGH — stale TODO PR-N3 removed: refundPartial heuristic (meterUnits-based pack lookup) is still intentionally needed — handleChargeRefunded does not pass packId to refundPartial. Replaced misleading TODO with accurate comment explaining why the heuristic is retained.
  • HIGH — Mixed type caveat in billing.usage.model: Added same JSDoc note as billing.extraBalance.model on meterBreakdown and counters fields (Schema.Types.Mixed validators don't fire on in-place mutations).
  • MEDIUM — field asymmetry documented: Added comment in billing.controller.js noting BillingExtraBalance.organization (ObjectId ref) vs BillingUsage.organizationId (string field).
  • MEDIUM — pack priceUsd startup warning: billing.init.js now warns at boot if any configured pack is missing a valid priceUsd, surfacing misconfiguration before a refund exposes the gap.

H2 (resetAllDue 7d race) skipped per scope — track as separate GitHub issue.

Test plan

  • 3 new unit tests in billing.webhook.checkout.unit.tests.js covering: PI update called with real session ID, PI update skipped when payment_intent absent, PI update failure is non-fatal (resolves, creditPack still called)
  • Full suite: 920 tests pass, 0 failures
  • Lint: clean

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.
Copilot AI review requested due to automatic review settings April 30, 2026 07:51
@codacy-production
Copy link
Copy Markdown

codacy-production Bot commented Apr 30, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity · 2 duplication

Metric Results
Complexity 5
Duplication 2

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

Copy link
Copy Markdown

@codacy-production codacy-production Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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', () => ({
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ 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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

Warning

Rate limit exceeded

@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 57 minutes and 53 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 03cac026-3f21-4f9d-93ef-cd1511982e7d

📥 Commits

Reviewing files that changed from the base of the PR and between 0c3c90d and 7358df3.

📒 Files selected for processing (6)
  • modules/billing/billing.init.js
  • modules/billing/controllers/billing.controller.js
  • modules/billing/models/billing.usage.model.mongoose.js
  • modules/billing/services/billing.extra.service.js
  • modules/billing/services/billing.webhook.service.js
  • modules/billing/tests/billing.webhook.checkout.unit.tests.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/billing-cumulative-review

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 57 minutes and 53 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.78%. Comparing base (dd8ea07) to head (7358df3).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
modules/billing/billing.init.js 25.00% 2 Missing and 1 partial ⚠️

❌ 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +165 to +180
// 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);
@PierreBrisorgueil PierreBrisorgueil merged commit 9d39136 into master Apr 30, 2026
5 of 7 checks passed
@PierreBrisorgueil PierreBrisorgueil deleted the fix/billing-cumulative-review branch April 30, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants