feat: app stripe credits support#4315
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds credit-type Stripe invoice lines, persists CreditRealizationID in line metadata, reconciles applied credits during invoice create/update, updates description formatting, skips credit lines in external ID mapping, and adds ledger-backed integration tests verifying credit-then-invoice progressive billing. ChangesStripe Invoice Credit Line Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
c3446c9 to
c6eead3
Compare
33b72f9 to
47295fb
Compare
c6eead3 to
3ddb256
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/app/stripe/invoice_credits_test.go (1)
167-171: ⚡ Quick winAdd
AssertExpectationsafter eachUpsertStandardInvoicecall to verify the mocks were actually hit.Both
expectStripeInvoiceCreateandexpectStripeInvoiceAddLinesuse.Once(), but withoutAssertExpectations, theOnce()constraint is never enforced. If the mock matcher inexpectStripeInvoiceAddLinesdoesn't fire (e.g., descriptions or amounts are subtly wrong), the call silently falls back to zero values,UpsertStandardInvoicereturns an error caught bys.NoError, but the "expected call wasn't invoked" would go undetected.✅ Proposed fix
upsertResult, err := stripeInvoicingApp.UpsertStandardInvoice(ctx, stripePartialInvoice) s.NoError(err) externalID, ok := upsertResult.GetExternalID() s.True(ok) s.Equal("stripe-partial-invoice-id", externalID) +s.StripeAppClient.AssertExpectations(s.T())And similarly after the final invoice upsert:
upsertResult, err := stripeInvoicingApp.UpsertStandardInvoice(ctx, stripeFinalInvoice) s.NoError(err) externalID, ok := upsertResult.GetExternalID() s.True(ok) s.Equal("stripe-final-invoice-id", externalID) +s.StripeAppClient.AssertExpectations(s.T())Also applies to: 229-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/stripe/invoice_credits_test.go` around lines 167 - 171, The test currently calls stripeInvoicingApp.UpsertStandardInvoice(...) twice but doesn't verify the mock expectations; after each UpsertStandardInvoice invocation add a call to the suite/mocks' AssertExpectations (e.g., s.T().Helper(); s.AssertExpectations(s.T()) or s.mockStripe.AssertExpectations(s.T())) so the .Once() expectations in expectStripeInvoiceCreate and expectStripeInvoiceAddLines are enforced; apply this right after the block that checks upsertResult/GetExternalID and again after the later UpsertStandardInvoice call around lines referenced (229-233) so any missed mock call fails the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/app/stripe/appinvoice.go`:
- Around line 231-233: createInvoice is currently appending credits from
line.CreditsApplied without checking credit.CreditRealizationID, which allows
empty IDs into Stripe metadata and causes the add/remove thrash; updateInvoice
already guards against this. Modify the loop in createInvoice that builds
stripeLineAdd (the one calling getCreditStripeAddInvoiceItemParams) to skip any
credit where credit.CreditRealizationID == "" (same guard used in
updateInvoice), so only credits with a non-empty CreditRealizationID are
converted to Stripe params.
In `@test/app/stripe/invoice_credits_test.go`:
- Around line 124-128: Add a defer call to reset the streaming connector after
adding test events to avoid leaking events across tests: after calling
s.MockStreamingConnector.AddSimpleEvent (and the other AddSimpleEvent calls
around lines corresponding to this block and the block at 186-190), add defer
s.MockStreamingConnector.Reset() so the events added by the test are cleared;
this ensures the suite-level TearDownTest (which only restores the Stripe mock)
won't leave stale events that can affect TestComplexInvoice or other tests.
---
Nitpick comments:
In `@test/app/stripe/invoice_credits_test.go`:
- Around line 167-171: The test currently calls
stripeInvoicingApp.UpsertStandardInvoice(...) twice but doesn't verify the mock
expectations; after each UpsertStandardInvoice invocation add a call to the
suite/mocks' AssertExpectations (e.g., s.T().Helper();
s.AssertExpectations(s.T()) or s.mockStripe.AssertExpectations(s.T())) so the
.Once() expectations in expectStripeInvoiceCreate and
expectStripeInvoiceAddLines are enforced; apply this right after the block that
checks upsertResult/GetExternalID and again after the later
UpsertStandardInvoice call around lines referenced (229-233) so any missed mock
call fails the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 644f56bd-9f40-45a2-947f-94d7d9fa0926
📒 Files selected for processing (3)
openmeter/app/stripe/appinvoice.gotest/app/stripe/invoice_credits_test.gotest/app/stripe/invoice_test.go
Overview
Add support for credit lines on stripe invoices.
Notes for reviewer
Summary by CodeRabbit
New Features
Tests