Conversation
Contributor
|
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 (1)
📝 WalkthroughWalkthroughRefactors credit-purchase: splits Charge into ChargeBase + Realizations, introduces creditpurchase.Status and status_detailed persistence, extracts credit-grant into its own DB table/edge, composes the Adapter into role-specific interfaces (including CreateCreditGrant), and updates services/tests to use Realizations and the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as CreditPurchaseService
participant Adapter as Adapter (Charge / CreditGrant / Payment)
participant DB as Database
participant Ledger as Ledger
Client->>Service: Initiate external credit purchase
Service->>Adapter: CreateCreditGrant(chargeID, TransactionGroupID, GrantedAt)
Adapter->>DB: Insert charge_credit_purchase_credit_grants row
DB-->>Adapter: saved grant (TransactionGroupID, GrantedAt)
Adapter-->>Service: TimedGroupReference
Service->>Adapter: UpdateCharge(ChargeBase)
Adapter->>DB: Update charge_credit_purchases.status_detailed / base fields
alt external payment auth/settle
Service->>Adapter: CreateExternalPayment(...) / CreateInvoicedPayment(...)
Adapter->>Ledger: Record external payment
Adapter->>DB: Persist payment realization (external/invoiced)
Adapter-->>Service: Payment realization reference
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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: 6
🧹 Nitpick comments (3)
test/credits/sanity_lifecycle_test.go (1)
531-543: Guard the new realization pointer before reading.Status.If the transition stops returning
ExternalPaymentSettlement, this shared helper panics instead of failing with a clean assertion. As.Require().NotNil(updatedCharge.Realizations.ExternalPaymentSettlement)before each status check would make these lifecycle failures much easier to read.💡 Small test-helper hardening
s.NoError(err) + s.Require().NotNil(updatedCharge.Realizations.ExternalPaymentSettlement) s.Equal(payment.StatusAuthorized, updatedCharge.Realizations.ExternalPaymentSettlement.Status) updatedCharge, err = s.Charges.HandleCreditPurchaseExternalPaymentStateTransition(ctx, charges.HandleCreditPurchaseExternalPaymentStateTransitionInput{ ChargeID: chargeID, TargetPaymentState: payment.StatusSettled, }) s.NoError(err) + s.Require().NotNil(updatedCharge.Realizations.ExternalPaymentSettlement) s.Equal(payment.StatusSettled, updatedCharge.Realizations.ExternalPaymentSettlement.Status)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_lifecycle_test.go` around lines 531 - 543, The test reads updatedCharge.Realizations.ExternalPaymentSettlement.Status without guarding the pointer, which can panic; before each s.Equal that inspects .Status (after calls to Charges.HandleCreditPurchaseExternalPaymentStateTransition), add s.Require().NotNil(updatedCharge.Realizations.ExternalPaymentSettlement) to assert the realization exists and fail the test cleanly if it does not.openmeter/ent/schema/chargescreditpurchase.go (1)
117-123: Make credit-grant rows append-only in the schema.
transaction_group_idandgranted_atlook like write-once realization data. Marking themImmutable()would let Ent enforce that invariant instead of relying on callers not to use the generated update builders.Suggested schema hardening
field.String("transaction_group_id"). SchemaType(map[string]string{ dialect.Postgres: "char(26)", }). - NotEmpty(), + NotEmpty(). + Immutable(), - field.Time("granted_at"), + field.Time("granted_at"). + Immutable(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/chargescreditpurchase.go` around lines 117 - 123, The schema currently allows updates to write-once fields: add Ent's Immutable() to the field definitions for "transaction_group_id" (the field.String("transaction_group_id") chain) and "granted_at" (the field.Time("granted_at") chain) so Ent will enforce append-only semantics; keep existing SchemaType and NotEmpty() on transaction_group_id and preserve any other chained modifiers when adding Immutable() to both field builders in chargescreditpurchase.go.openmeter/billing/charges/creditpurchase/charge.go (1)
69-70: Double-check the expand contract forrealizations.
Realizationsis a value field, so a direct JSON marshal ofChargewill still emit"realizations": {}even when nothing was expanded. For an expand-only block, that blurs “not loaded” vs “loaded but empty” and can quietly change the external API shape. If this type is returned directly, I'd make the field nullable or map it through a response DTO that can omit unloaded realizations.Also applies to: 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/charge.go` around lines 69 - 70, The Charge struct's Realizations field is a value type (Realizations) so JSON marshaling emits "realizations": {} even when not expanded; change the field to be a pointer (*Realizations) or make Charge-to-response DTO mapping that uses a nullable/omitted field to distinguish "not loaded" vs "loaded empty". Update the Charge struct declaration (Realizations) and any code that constructs or serializes Charge (also check the similar occurrence around the other instance noted at lines 146-147) to either initialize the pointer only when expanded or map to a response DTO that omits the realizations key when nil/unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/charges/SKILL.md:
- Around line 504-507: The documentation still refers to a non-existent
creditpurchase.State field; update the checklist to reflect the actual Go model:
creditpurchase.Charge is composed of creditpurchase.ChargeBase plus
creditpurchase.Realizations (which contains CreditGrantRealization,
ExternalPaymentSettlement, and InvoiceSettlement) and remove any mention of
State. Specifically, replace the line describing `creditpurchase.State` being
empty with a statement that lifecycle outcomes live in `Realizations`, and
ensure the earlier bullets reference `creditpurchase.ChargeBase`,
`creditpurchase.Charge`, and `creditpurchase.Realizations` exactly as named in
the code.
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 103-104: convert.go reads charge.Realizations.InvoiceSettlement
and ExternalPaymentSettlement which are expansion-only and may be nil because
the service's Get and Create paths don't request expansions; update the service
methods so charges returned to convert.go always include realizations by passing
Expands: meta.Expands{meta.ExpandRealizations} when fetching
charges—specifically, modify the Get method (which calls GetByID) to call
GetByID with Expands including meta.ExpandRealizations, and update the Create
path to either fetch the newly created charge via GetByID with the same Expands
or ensure Create returns the charge with realizations populated; reference the
Get, Create, and GetByID methods and use meta.Expands/meta.ExpandRealizations to
implement the fix.
In `@openmeter/billing/charges/creditpurchase/adapter/mapper.go`:
- Around line 18-31: MapChargeBaseFromDB currently maps dbEntity.EffectiveAt
only into Intent.EffectiveAt and thus drops the top-level
creditpurchase.ChargeBase.EffectiveAt; update the returned
creditpurchase.ChargeBase literal in MapChargeBaseFromDB to set EffectiveAt:
convert.SafeToUTC(dbEntity.EffectiveAt) (in addition to leaving
Intent.EffectiveAt as-is) so the top-level EffectiveAt is preserved across
create/get/list/update round-trips.
In `@openmeter/billing/charges/creditpurchase/service/promotional.go`:
- Around line 27-30: Replace the fresh clock.Now() used as GrantedAt when
creating the credit grant with the timestamp supplied by the handler’s ledger
transaction group reference: pass ledgerTransactionGroupReference.Timestamp (or
ledgerTransactionGroupReference.GrantedAt if that’s the exact field name) into
creditpurchase.CreateCreditGrantInput. Update the call in CreateCreditGrant (the
grantRealization creation) to use that field instead of clock.Now(), and remove
the now-unused pkg/clock import from the file; this preserves the original
ledger timestamp provided by OnPromotionalCreditPurchase.
In `@tools/migrate/migrations/20260410144542_credit_purchase_refactor.down.sql`:
- Around line 9-12: The rollback currently drops
charge_credit_purchase_credit_grants before restoring legacy columns; instead,
alter charge_credit_purchases to ADD the columns credit_granted_at and
credit_grant_transaction_group_id first, then populate them from
charge_credit_purchase_credit_grants (join on charge_credit_purchase_id ->
charge_credit_purchases.id and copy granted_at -> credit_granted_at and
transaction_group_id -> credit_grant_transaction_group_id), and only after the
UPDATE drop the charge_credit_purchase_credit_grants table; ensure the ALTER
TABLE and UPDATE statements reference charge_credit_purchases and
charge_credit_purchase_credit_grants exactly as named.
In `@tools/migrate/migrations/20260410144542_credit_purchase_refactor.up.sql`:
- Around line 3-27: The migration drops credit_grant_transaction_group_id and
credit_granted_at before populating the new charge_credit_purchase_credit_grants
table, which will lose historical grant data; modify the migration to CREATE the
charge_credit_purchase_credit_grants table first (using the schema shown), then
INSERT rows into charge_credit_purchase_credit_grants selecting id AS
id_for_grant (generate new id if you use a different PK strategy), namespace,
created_at, updated_at, deleted_at, credit_grant_transaction_group_id ->
transaction_group_id, credit_granted_at -> granted_at, and charge_id =
charge_credit_purchases.id for every row where credit_grant_transaction_group_id
IS NOT NULL (or credit_granted_at IS NOT NULL) to backfill existing grants,
ensure uniqueness constraints (namespace+charge_id, charge_id) are satisfied or
deduplicate before inserting, and only after successful backfill DROP the legacy
columns credit_grant_transaction_group_id and credit_granted_at and then proceed
with setting status_detailed NOT NULL.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/charge.go`:
- Around line 69-70: The Charge struct's Realizations field is a value type
(Realizations) so JSON marshaling emits "realizations": {} even when not
expanded; change the field to be a pointer (*Realizations) or make
Charge-to-response DTO mapping that uses a nullable/omitted field to distinguish
"not loaded" vs "loaded empty". Update the Charge struct declaration
(Realizations) and any code that constructs or serializes Charge (also check the
similar occurrence around the other instance noted at lines 146-147) to either
initialize the pointer only when expanded or map to a response DTO that omits
the realizations key when nil/unset.
In `@openmeter/ent/schema/chargescreditpurchase.go`:
- Around line 117-123: The schema currently allows updates to write-once fields:
add Ent's Immutable() to the field definitions for "transaction_group_id" (the
field.String("transaction_group_id") chain) and "granted_at" (the
field.Time("granted_at") chain) so Ent will enforce append-only semantics; keep
existing SchemaType and NotEmpty() on transaction_group_id and preserve any
other chained modifiers when adding Immutable() to both field builders in
chargescreditpurchase.go.
In `@test/credits/sanity_lifecycle_test.go`:
- Around line 531-543: The test reads
updatedCharge.Realizations.ExternalPaymentSettlement.Status without guarding the
pointer, which can panic; before each s.Equal that inspects .Status (after calls
to Charges.HandleCreditPurchaseExternalPaymentStateTransition), add
s.Require().NotNil(updatedCharge.Realizations.ExternalPaymentSettlement) to
assert the realization exists and fail the test cleanly if it does not.
🪄 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: ed88dbb0-26c0-4ddc-9f5a-9da64fa85853
⛔ Files ignored due to path filters (27)
openmeter/ent/db/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant/chargecreditpurchasecreditgrant.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchasecreditgrant_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/cursor.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (18)
.agents/skills/charges/SKILL.mdapi/v3/handlers/customers/credits/convert.goopenmeter/billing/charges/creditpurchase/adapter.goopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/creditgrant.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/creditpurchase/service/promotional.goopenmeter/billing/charges/creditpurchase/statemachine.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.gotest/credits/sanity_lifecycle_test.gotest/credits/sanity_test.gotools/migrate/migrations/20260410144542_credit_purchase_refactor.down.sqltools/migrate/migrations/20260410144542_credit_purchase_refactor.up.sql
turip
force-pushed
the
refactor/creditpurchase-shape
branch
from
6f32375 to
0b2f64c
Compare
tothandras
requested changes
Apr 11, 2026
Signed-off-by: András Tóth <4157749+tothandras@users.noreply.github.com>
tothandras
approved these changes
Apr 11, 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
Make sure that the external API surface of creditpurchase matches the usagebased and flatfee ones.
Major changes
Refactored the
creditpurchasecharge package to follow the same structural pattern asflatfeeandusagebased.All three charge types now share a consistent shape:
ChargeBase(base-row data: identity, intent, status,scheduling state) embedded in
ChargealongsideRealizations(expand-only edge data). Each type defines its ownStatustype with aToMetaChargeStatus()bridge, persisted in astatus_detailedDB column.Moved
CreditGrantRealizationout of thecreditpurchasebase row into its owncharge_credit_purchase_credit_grantsedge table. This makescreditpurchase.Stateempty — all lifecycle outcomes(
CreditGrantRealization,ExternalPaymentSettlement,InvoiceSettlement) now live uniformly inRealizations,loaded via edge queries when
ExpandRealizationsis requested.Restructured the
creditpurchaseadapter into composite sub-interfaces:ChargeAdapter,CreditGrantAdapter,ExternalPaymentAdapter, andInvoicedPaymentAdapter.UpdateChargenow acceptsChargeBaseinstead of the fullCharge, ensuring realization edges are only written through their dedicated adapter methods(
CreateCreditGrant,CreateExternalPayment, etc.). Service methods that only change edge data updatecharge.Realizationsin memory without callingUpdateCharge.Fixed all callers across the codebase — API handlers, ledger charge adapters, service code, and tests — to use
type-specific
Statuscomparisons (flatfee.StatusFinal,creditpurchase.StatusActive) instead ofmeta.ChargeStatus, and to access realization data through.Realizationsinstead of.State.Notes for reviewer
Summary by CodeRabbit
Documentation
Refactor
Database
Tests