fix: grants service should not bypass charges service for now#4166
Merged
fix: grants service should not bypass charges service for now#4166
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds explicit support for promotional credit grants in the billing system. It includes API conversion logic that maps promotional settlements to Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handler as API Handler
participant Service as CreditGrant Service
participant ChargesService as Charges Service
participant Ledger as Billing/Ledger
Client->>Handler: Create promotional credit grant
Handler->>Service: Create(grant intent)
Service->>ChargesService: Create(intents list)
ChargesService->>Ledger: Create charge & realizations
Ledger-->>ChargesService: Charge ID
ChargesService-->>Service: Created charges array
Service->>Service: Validate exactly one charge
Service->>ChargesService: GetByID(chargeID, expand=realizations)
ChargesService->>Ledger: Fetch charge with realizations
Ledger-->>ChargesService: Charge with realizations
ChargesService-->>Service: Charge object
Service->>Service: Convert to CreditPurchaseCharge
Service-->>Handler: Created credit grant
Handler->>Handler: toAPICreditGrantPurchase (promotional settlement)
Handler->>Handler: funding_method=none, purchase=nil
Handler-->>Client: API response (no purchase block)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
turip
force-pushed
the
fix/credit-grant-creation
branch
from
b2357da to
39d2a24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/billing/creditgrant/service/service.go (1)
87-117: Rewiring throughchargesService.Createlooks solid.The fix itself is clean—going through the standard service path instead of bypassing it is the right move. The
len(result) != 1guard plus the refetch-with-ExpandRealizationsmake the contract explicit.One tiny note: you're doing a create + round-trip
GetByIDon every call. ThechargesService.Createpath doesn't return charges with realizations already expanded, so this extra fetch is necessary if you need that data. If realizations turn out to be optional for some callers down the road, you could gate the refetch on "only if needed"—but for now, it's fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/creditgrant/service/service.go` around lines 87 - 117, The current flow always creates a charge via s.chargesService.Create and then refetches it with s.chargesService.GetByID to expand realizations; to avoid an unnecessary round-trip when realizations aren't required, add a toggle (e.g., a boolean flag on the service method input or a field on the credit grant input like includeRealizations) and branch: if includeRealizations is false, convert result[0] directly to a credit purchase via result[0].AsCreditPurchaseCharge() (or equivalent) and return it; otherwise perform the existing GetByID using createdChargeID and charge.AsCreditPurchaseCharge(); update the method signature and callers accordingly so callers can opt-in to the expensive refetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/credits/creditgrant_test.go`:
- Around line 86-281: Replace uses of context.Background() with the test-scoped
context from s.T() in each test and helper to tie cancellation to the test
harness: change the ctx initializations in
TestCreateInvoiceFundedCreatesInvoiceArtifacts, TestCreatePromotionalGrant,
TestCreateExternalGrantAndSettle, TestListCreditGrants (currently using ctx :=
context.Background()) to use ctx := s.T().Context(), and update any callers of
mustCreatePromotionalCreditGrant (the helper already accepts ctx) to pass that
test context; remove the now-unnecessary context import if it becomes unused.
---
Nitpick comments:
In `@openmeter/billing/creditgrant/service/service.go`:
- Around line 87-117: The current flow always creates a charge via
s.chargesService.Create and then refetches it with s.chargesService.GetByID to
expand realizations; to avoid an unnecessary round-trip when realizations aren't
required, add a toggle (e.g., a boolean flag on the service method input or a
field on the credit grant input like includeRealizations) and branch: if
includeRealizations is false, convert result[0] directly to a credit purchase
via result[0].AsCreditPurchaseCharge() (or equivalent) and return it; otherwise
perform the existing GetByID using createdChargeID and
charge.AsCreditPurchaseCharge(); update the method signature and callers
accordingly so callers can opt-in to the expensive refetch.
🪄 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: e1030374-6298-403d-9fc5-d406839a2460
📒 Files selected for processing (5)
.agents/skills/charges/SKILL.mdapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/convert_test.goopenmeter/billing/creditgrant/service/service.gotest/credits/creditgrant_test.go
Comment on lines
+86
to
+281
| func (s *CreditGrantTestSuite) TestCreateInvoiceFundedCreatesInvoiceArtifacts() { | ||
| ctx := context.Background() | ||
| ns := s.GetUniqueNamespace("creditgrant-service-invoice-funded") | ||
|
|
||
| customInvoicing := s.SetupCustomInvoicing(ns) | ||
| cust := s.createLedgerBackedCustomer(ns, "test-subject") | ||
|
|
||
| _ = s.ProvisionBillingProfile(ctx, ns, customInvoicing.App.GetID(), | ||
| billingtest.WithProgressiveBilling(), | ||
| billingtest.WithCollectionInterval(datetime.MustParseDuration(s.T(), "PT1H")), | ||
| billingtest.WithManualApproval(), | ||
| ) | ||
|
|
||
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:53Z", time.UTC).AsTime() | ||
| clock.SetTime(now) | ||
|
|
||
| grant, err := s.CreditGrantService.Create(ctx, creditgrant.CreateInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| Name: "$10.00 grant for $10.00 charge", | ||
| Description: lo.ToPtr("A $10.00 grant for $10.00 charge available immediately on grant."), | ||
| Currency: USD, | ||
| Amount: alpacadecimal.NewFromInt(10), | ||
| Priority: lo.ToPtr(int16(10)), | ||
| FundingMethod: creditgrant.FundingMethodInvoice, | ||
| Purchase: &creditgrant.PurchaseTerms{ | ||
| Currency: USD, | ||
| PerUnitCostBasis: lo.ToPtr(alpacadecimal.NewFromInt(1)), | ||
| }, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Equal(creditpurchase.SettlementTypeInvoice, grant.Intent.Settlement.Type()) | ||
| s.Equal(creditpurchase.StatusActive, grant.Status) | ||
| s.NotNil(grant.Realizations.CreditGrantRealization) | ||
|
|
||
| standardInvoices, err := s.BillingService.ListStandardInvoices(ctx, billing.ListStandardInvoicesInput{ | ||
| Namespaces: []string{ns}, | ||
| Expand: billing.StandardInvoiceExpandAll, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Len(standardInvoices.Items, 1) | ||
|
|
||
| invoice := standardInvoices.Items[0] | ||
| s.Equal(cust.ID, invoice.Customer.CustomerID) | ||
| s.Len(invoice.Lines.OrEmpty(), 1) | ||
| s.Equal(grant.ID, *invoice.Lines.OrEmpty()[0].ChargeID) | ||
|
|
||
| gatheringInvoices, err := s.BillingService.ListGatheringInvoices(ctx, billing.ListGatheringInvoicesInput{ | ||
| Namespaces: []string{ns}, | ||
| Customers: []string{cust.ID}, | ||
| Expand: []billing.GatheringInvoiceExpand{billing.GatheringInvoiceExpandLines}, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Len(gatheringInvoices.Items, 0) | ||
| } | ||
|
|
||
| func (s *CreditGrantTestSuite) TestCreatePromotionalGrant() { | ||
| ctx := context.Background() | ||
| ns := s.GetUniqueNamespace("creditgrant-service-promotional") | ||
|
|
||
| cust := s.createLedgerBackedCustomer(ns, "test-subject") | ||
| sandboxApp := s.InstallSandboxApp(s.T(), ns) | ||
| _ = s.ProvisionBillingProfile(ctx, ns, sandboxApp.GetID()) | ||
|
|
||
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:53Z", time.UTC).AsTime() | ||
| clock.SetTime(now) | ||
|
|
||
| grant, err := s.CreditGrantService.Create(ctx, creditgrant.CreateInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| Name: "Promotional grant", | ||
| Description: lo.ToPtr("Promotional credit grant"), | ||
| Currency: USD, | ||
| Amount: alpacadecimal.NewFromInt(25), | ||
| Priority: lo.ToPtr(int16(15)), | ||
| FundingMethod: creditgrant.FundingMethodNone, | ||
| }) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.Equal(creditpurchase.SettlementTypePromotional, grant.Intent.Settlement.Type()) | ||
| s.Equal(creditpurchase.StatusFinal, grant.Status) | ||
| s.NotNil(grant.Realizations.CreditGrantRealization) | ||
| s.Nil(grant.Realizations.ExternalPaymentSettlement) | ||
| s.Nil(grant.Realizations.InvoiceSettlement) | ||
|
|
||
| gatheringInvoices, err := s.BillingService.ListGatheringInvoices(ctx, billing.ListGatheringInvoicesInput{ | ||
| Namespaces: []string{ns}, | ||
| Customers: []string{cust.ID}, | ||
| Expand: []billing.GatheringInvoiceExpand{billing.GatheringInvoiceExpandLines}, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Len(gatheringInvoices.Items, 0) | ||
| } | ||
|
|
||
| func (s *CreditGrantTestSuite) TestCreateExternalGrantAndSettle() { | ||
| ctx := context.Background() | ||
| ns := s.GetUniqueNamespace("creditgrant-service-external") | ||
|
|
||
| cust := s.createLedgerBackedCustomer(ns, "test-subject") | ||
| sandboxApp := s.InstallSandboxApp(s.T(), ns) | ||
| _ = s.ProvisionBillingProfile(ctx, ns, sandboxApp.GetID()) | ||
|
|
||
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:53Z", time.UTC).AsTime() | ||
| clock.SetTime(now) | ||
|
|
||
| grant, err := s.CreditGrantService.Create(ctx, creditgrant.CreateInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| Name: "External grant", | ||
| Description: lo.ToPtr("External credit grant"), | ||
| Currency: USD, | ||
| Amount: alpacadecimal.NewFromInt(30), | ||
| Priority: lo.ToPtr(int16(20)), | ||
| FundingMethod: creditgrant.FundingMethodExternal, | ||
| Purchase: &creditgrant.PurchaseTerms{ | ||
| Currency: USD, | ||
| PerUnitCostBasis: lo.ToPtr(alpacadecimal.NewFromFloat(0.5)), | ||
| AvailabilityPolicy: lo.ToPtr(creditpurchase.CreatedInitialPaymentSettlementStatus), | ||
| }, | ||
| }) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.Equal(creditpurchase.SettlementTypeExternal, grant.Intent.Settlement.Type()) | ||
| s.Equal(creditpurchase.StatusActive, grant.Status) | ||
| s.NotNil(grant.Realizations.CreditGrantRealization) | ||
| s.Nil(grant.Realizations.ExternalPaymentSettlement) | ||
|
|
||
| grant, err = s.CreditGrantService.UpdateExternalSettlement(ctx, creditgrant.UpdateExternalSettlementInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| ChargeID: grant.ID, | ||
| TargetStatus: payment.StatusAuthorized, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Equal(creditpurchase.StatusActive, grant.Status) | ||
| s.NotNil(grant.Realizations.ExternalPaymentSettlement) | ||
| s.Equal(payment.StatusAuthorized, grant.Realizations.ExternalPaymentSettlement.Status) | ||
|
|
||
| grant, err = s.CreditGrantService.UpdateExternalSettlement(ctx, creditgrant.UpdateExternalSettlementInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| ChargeID: grant.ID, | ||
| TargetStatus: payment.StatusSettled, | ||
| }) | ||
| s.Require().NoError(err) | ||
|
|
||
| s.Equal(creditpurchase.StatusFinal, grant.Status) | ||
| s.NotNil(grant.Realizations.ExternalPaymentSettlement) | ||
| s.Equal(payment.StatusSettled, grant.Realizations.ExternalPaymentSettlement.Status) | ||
| } | ||
|
|
||
| func (s *CreditGrantTestSuite) TestListCreditGrants() { | ||
| ctx := context.Background() | ||
| ns := s.GetUniqueNamespace("creditgrant-service-list") | ||
|
|
||
| cust := s.createLedgerBackedCustomer(ns, "test-subject") | ||
| sandboxApp := s.InstallSandboxApp(s.T(), ns) | ||
| _ = s.ProvisionBillingProfile(ctx, ns, sandboxApp.GetID()) | ||
|
|
||
| now := datetime.MustParseTimeInLocation(s.T(), "2026-04-17T11:23:53Z", time.UTC).AsTime() | ||
| clock.SetTime(now) | ||
|
|
||
| firstGrant := s.mustCreatePromotionalCreditGrant(ctx, ns, cust.GetID(), "list-grant-1", alpacadecimal.NewFromInt(10)) | ||
| secondGrant := s.mustCreatePromotionalCreditGrant(ctx, ns, cust.GetID(), "list-grant-2", alpacadecimal.NewFromInt(20)) | ||
|
|
||
| result, err := s.CreditGrantService.List(ctx, creditgrant.ListInput{ | ||
| Namespace: ns, | ||
| CustomerID: cust.ID, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Len(result.Items, 2) | ||
|
|
||
| ids := lo.Map(result.Items, func(item creditpurchase.Charge, _ int) string { | ||
| return item.ID | ||
| }) | ||
| s.Contains(ids, firstGrant.ID) | ||
| s.Contains(ids, secondGrant.ID) | ||
| } | ||
|
|
||
| func (s *CreditGrantTestSuite) mustCreatePromotionalCreditGrant(ctx context.Context, namespace string, customerID customer.CustomerID, name string, amount alpacadecimal.Decimal) creditpurchase.Charge { | ||
| s.T().Helper() | ||
|
|
||
| grant, err := s.CreditGrantService.Create(ctx, creditgrant.CreateInput{ | ||
| Namespace: namespace, | ||
| CustomerID: customerID.ID, | ||
| Name: name, | ||
| Description: lo.ToPtr(name), | ||
| Currency: USD, | ||
| Amount: amount, | ||
| Priority: lo.ToPtr(int16(5)), | ||
| FundingMethod: creditgrant.FundingMethodNone, | ||
| }) | ||
| s.Require().NoError(err) | ||
|
|
||
| return grant | ||
| } |
Contributor
There was a problem hiding this comment.
Prefer s.T().Context() over context.Background() in tests.
Each test method and the mustCreatePromotionalCreditGrant helper grab a fresh context.Background() even though testing.T is accessible via s.T(). Switching to s.T().Context() ties cancellation and lifecycle to the test harness — also means you can drop the context import if that becomes the only usage (helper signature already takes ctx, so it's fine there).
♻️ Proposed tweak
func (s *CreditGrantTestSuite) TestCreateInvoiceFundedCreatesInvoiceArtifacts() {
- ctx := context.Background()
+ ctx := s.T().Context()
ns := s.GetUniqueNamespace("creditgrant-service-invoice-funded")Apply the same change at lines 143, 181, and 238.
As per coding guidelines: "Use t.Context() when a testing.T or testing.TB is available instead of introducing context.Background() to keep cancellation and test-scoped lifecycle tied to the test harness".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/credits/creditgrant_test.go` around lines 86 - 281, Replace uses of
context.Background() with the test-scoped context from s.T() in each test and
helper to tie cancellation to the test harness: change the ctx initializations
in TestCreateInvoiceFundedCreatesInvoiceArtifacts, TestCreatePromotionalGrant,
TestCreateExternalGrantAndSettle, TestListCreditGrants (currently using ctx :=
context.Background()) to use ctx := s.T().Context(), and update any callers of
mustCreatePromotionalCreditGrant (the helper already accepts ctx) to pass that
test context; remove the now-unnecessary context import if it becomes unused.
GAlexIHU
approved these changes
Apr 17, 2026
This was referenced Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fix issues around grant creation:
Notes for reviewer
Summary by CodeRabbit
New Features
funding_method=nonein API responses without purchase details.Bug Fixes
Tests