Conversation
Contributor
📝 WalkthroughWalkthroughAdds invoiced-payment support for credit-purchase charges: new invoiced payment ent/schema, DB migration adding Changes
Sequence DiagramsequenceDiagram
participant Test as Test Harness
participant CPS as CreditPurchase Service
participant BIS as Billing Invoice Service
participant CIS as CustomInvoicing Service
participant DB as Database
Test->>CPS: Create credit purchase with InvoiceSettlement
CPS->>DB: Persist credit purchase (state: initiated)
Test->>CPS: Post payment authorized callback
CPS->>DB: Update state -> authorized, attach InvoiceSettlement (InvoiceID)
Test->>BIS: Approve standard invoice
BIS->>DB: Mark invoice approved
Test->>CIS: Trigger payment processing (TriggerPaid)
CIS->>DB: Read invoiced payment by invoice/line -> update authorized/settled timestamps/status
CIS->>CPS: Notify/trigger charge settlement callbacks
CPS->>DB: Read final state
DB-->>Test: Confirm InvoiceSettlement present and charge settled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
📝 Coding Plan
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
commented
Mar 19, 2026
mark-vass-konghq
force-pushed
the
feat/credit-purchase-invoice
branch
from
bc84b50 to
68a9222
Compare
Co-authored-by: Peter Turi <peter.turi@openmeter.io>
turip
force-pushed
the
feat/credit-purchase-invoice
branch
from
5cafbf2 to
7a684c9
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/creditpurchase/chargecreditpurchase.go (1)
90-112:⚠️ Potential issue | 🟡 MinorMissing validation for
InvoiceSettlementinState.Validate().The new
InvoiceSettlementfield isn't validated, butExternalPaymentSettlementis. For consistency and to catch invalid data early, you'll want to add the same validation pattern:🛡️ Proposed fix to add validation
func (s State) Validate() error { var errs []error if s.CreditGrantRealization != nil { if err := s.CreditGrantRealization.Validate(); err != nil { errs = append(errs, fmt.Errorf("credit grant realization: %w", err)) } } if s.ExternalPaymentSettlement != nil { if err := s.ExternalPaymentSettlement.Validate(); err != nil { errs = append(errs, fmt.Errorf("external payment settlement: %w", err)) } } + if s.InvoiceSettlement != nil { + if err := s.InvoiceSettlement.Validate(); err != nil { + errs = append(errs, fmt.Errorf("invoice settlement: %w", err)) + } + } + return errors.Join(errs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/chargecreditpurchase.go` around lines 90 - 112, State.Validate is missing validation for the InvoiceSettlement field; add a nil-check like the one for ExternalPaymentSettlement that calls s.InvoiceSettlement.Validate(), and if it returns an error append it to errs with fmt.Errorf("invoice settlement: %w", err) so it gets included in the final errors.Join(errs...); this mirrors the pattern used for CreditGrantRealization.Validate() and ExternalPaymentSettlement.Validate().
🧹 Nitpick comments (1)
tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql (1)
34-35: Minor: Unique index onidis redundant with PRIMARY KEY.Line 35 creates
chargecreditpurchaseinvoicedpayment_idas a unique index onid, butidis already the primary key (line 24). This is likely auto-generated by Ent and harmless, just a small overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql` around lines 34 - 35, The migration adds a redundant unique index "chargecreditpurchaseinvoicedpayment_id" on column "id" of table "charge_credit_purchase_invoiced_payments" which duplicates the PRIMARY KEY; remove that CREATE UNIQUE INDEX statement (or skip generating it) so only the primary key enforces uniqueness. Locate the statement creating "chargecreditpurchaseinvoicedpayment_id" in the migration and delete it (or adjust the Ent schema/migration generator to avoid emitting a unique index for "id") so only the existing primary key remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Line 517: The test is asserting a charge-status constant against res.Status,
but HandlePaymentTrigger returns an invoice status; update the assertion to
compare res.Status to the invoice status constant (e.g.,
billing.StandardInvoiceStatusPaid) instead of meta.ChargeStatusFinal, and ensure
the billing package symbol is referenced/imported where the test asserts the
result from HandlePaymentTrigger.
- Around line 408-411: The assertion in the test callback for
CreditPurchaseTestHandler.onCreditPurchaseInitiated is checking the wrong
settlement type; update the assert.Equal that compares
charge.Intent.Settlement.Type() to use creditpurchase.SettlementTypeInvoice
(instead of SettlementTypeExternal) so it matches the InvoiceSettlement intent
set earlier; locate the initatedCallback.Handler invocation in
creditpurchase_test.go where charge.Intent.Settlement.Type() is asserted and
change the expected value to SettlementTypeInvoice.
- Around line 492-500: The assertions in the onCreditPurchasePaymentSettled
handler are checking for external settlement types/fields but this test path
uses an invoice settlement; update the checks in the callback
(onCreditPurchasePaymentSettled) to assert SettlementTypeInvoice instead of
SettlementTypeExternal and to reference charge.State.InvoicePaymentSettlement
(and its Authorized.TransactionGroupID) rather than ExternalPaymentSettlement,
keeping the rest of the expectations (payment.StatusAuthorized and
meta.ChargeStatusActive) unchanged.
---
Outside diff comments:
In `@openmeter/billing/charges/creditpurchase/chargecreditpurchase.go`:
- Around line 90-112: State.Validate is missing validation for the
InvoiceSettlement field; add a nil-check like the one for
ExternalPaymentSettlement that calls s.InvoiceSettlement.Validate(), and if it
returns an error append it to errs with fmt.Errorf("invoice settlement: %w",
err) so it gets included in the final errors.Join(errs...); this mirrors the
pattern used for CreditGrantRealization.Validate() and
ExternalPaymentSettlement.Validate().
---
Nitpick comments:
In
`@tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql`:
- Around line 34-35: The migration adds a redundant unique index
"chargecreditpurchaseinvoicedpayment_id" on column "id" of table
"charge_credit_purchase_invoiced_payments" which duplicates the PRIMARY KEY;
remove that CREATE UNIQUE INDEX statement (or skip generating it) so only the
primary key enforces uniqueness. Locate the statement creating
"chargecreditpurchaseinvoicedpayment_id" in the migration and delete it (or
adjust the Ent schema/migration generator to avoid emitting a unique index for
"id") so only the existing primary key remains.
🪄 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: f3492d99-e6c1-4f00-98c2-a6a05bf0eda5
⛔ Files ignored due to path filters (41)
openmeter/ent/db/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_query.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**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/chargecreditpurchaseinvoicedpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment/chargecreditpurchaseinvoicedpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchaseinvoicedpayment_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeepayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeepayment/chargeflatfeepayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeepayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeepayment_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_create.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 (8)
openmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/billing/charges/flatfee/service/payment.goopenmeter/billing/charges/models/payment/invoiced.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/ent/schema/billing.goopenmeter/ent/schema/chargescreditpurchase.gotools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.down.sqltools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql (2)
36-37: Redundant unique index on primary key columnThe
idcolumn is already defined asPRIMARY KEY ("id")on line 26, which automatically creates a unique index. This additional unique index onidis redundant and just adds storage overhead and write cost without any benefit.🧹 Suggested removal
--- create index "chargecreditpurchaseinvoicedpayment_id" to table: "charge_credit_purchase_invoiced_payments" -CREATE UNIQUE INDEX "chargecreditpurchaseinvoicedpayment_id" ON "charge_credit_purchase_invoiced_payments" ("id");If this migration is generated from an Ent schema, the fix would be removing the redundant index annotation there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql` around lines 36 - 37, Remove the redundant unique index creation for the primary key: delete the CREATE UNIQUE INDEX statement that creates "chargecreditpurchaseinvoicedpayment_id" on table "charge_credit_purchase_invoiced_payments" (the "id" column is already declared PRIMARY KEY and has an implicit unique index), and if this migration was generated from an Ent schema, remove the redundant index annotation from the schema so future migrations do not reintroduce "chargecreditpurchaseinvoicedpayment_id".
30-31: Redundant unique constraints oncharge_idThere are two unique indexes involving
charge_id:
- Line 31:
UNIQUE INDEX ... ("charge_id")— globally unique- Line 41:
UNIQUE INDEX ... ("namespace", "charge_id")— unique per namespaceIf
charge_idis globally unique (line 31), then(namespace, charge_id)is automatically unique too, making line 41 redundant.If the intent is to have
charge_idunique only within a namespace, then line 31 is overly restrictive and should be a non-unique index (for lookup performance) while line 41 provides the actual uniqueness constraint.Could you clarify the intended constraint? My guess is you want per-namespace uniqueness, which would mean changing line 31 to a regular index.
Also applies to: 40-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql` around lines 30 - 31, The migration currently creates two conflicting uniqueness constraints: a globally unique index charge_credit_purchase_invoiced_payments_charge_id_key on charge_id and a per-namespace unique index charge_credit_purchase_invoiced_payments_namespace_charge_id_key on (namespace, charge_id); if the intended semantics are per-namespace uniqueness, change the first statement in the diff to create a non-unique index (CREATE INDEX charge_credit_purchase_invoiced_payments_charge_id_key ON charge_credit_purchase_invoiced_payments (charge_id)) instead of CREATE UNIQUE INDEX, so the per-namespace UNIQUE on (namespace, charge_id) remains the actual constraint and the single-column index still provides lookup performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql`:
- Around line 36-37: Remove the redundant unique index creation for the primary
key: delete the CREATE UNIQUE INDEX statement that creates
"chargecreditpurchaseinvoicedpayment_id" on table
"charge_credit_purchase_invoiced_payments" (the "id" column is already declared
PRIMARY KEY and has an implicit unique index), and if this migration was
generated from an Ent schema, remove the redundant index annotation from the
schema so future migrations do not reintroduce
"chargecreditpurchaseinvoicedpayment_id".
- Around line 30-31: The migration currently creates two conflicting uniqueness
constraints: a globally unique index
charge_credit_purchase_invoiced_payments_charge_id_key on charge_id and a
per-namespace unique index
charge_credit_purchase_invoiced_payments_namespace_charge_id_key on (namespace,
charge_id); if the intended semantics are per-namespace uniqueness, change the
first statement in the diff to create a non-unique index (CREATE INDEX
charge_credit_purchase_invoiced_payments_charge_id_key ON
charge_credit_purchase_invoiced_payments (charge_id)) instead of CREATE UNIQUE
INDEX, so the per-namespace UNIQUE on (namespace, charge_id) remains the actual
constraint and the single-column index still provides lookup performance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46da1a89-fd18-4c6f-9d7e-09050b6dcf37
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
tools/migrate/migrations/20260319114832_charge-creditpurchase-invoiced-payment.up.sql
mark-vass-konghq
approved these changes
Mar 19, 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
Add schema and basic testcase for creditpurchase via standard invoice codepath.
Notes for reviewer
Summary by CodeRabbit
New Features
Tests
Chores