fix: fix late review comments for costbasis change - #4883
Conversation
📝 WalkthroughWalkthroughCredit purchase cost bases now support JSON round trips and shared fiat conversion. Legacy settlements use centralized validation and mapping. Database relationships restrict referenced cost-basis deletion. Effective-time and expiration validation now handle missing explicit effective times. ChangesCredit purchase cost-basis flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CreditPurchase
participant ResolvedCostBasis
participant InvoiceAndPaymentServices
participant CustomerCreditsAPI
CreditPurchase->>ResolvedCostBasis: resolve fiat amount
ResolvedCostBasis-->>InvoiceAndPaymentServices: rounded FiatAmount
InvoiceAndPaymentServices-->>CustomerCreditsAPI: invoice or external purchase amount
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openmeter/billing/charges/creditpurchase/costbasis_test.go (1)
62-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice round-trip table. Consider a few negative decode cases too.
The happy paths are covered well.
UnmarshalJSONalso adds several rejection branches that no test exercises yet: a fiat payload that carries custom-currency fields, a dynamic payload that carriesrateorcurrencyCostBasisID, a pinned payload without an ID, and an unknownmode. A small table of raw JSON strings mapped to expected error substrings would lock that behavior in.Not a blocker, just a cheap way to keep the strict validation honest during future refactors.
🤖 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 `@openmeter/billing/charges/creditpurchase/costbasis_test.go` around lines 62 - 99, Extend TestCostBasisJSONRoundTrip with table-driven negative JSON cases covering fiat payloads with custom-currency fields, dynamic payloads with rate or currencyCostBasisID, pinned payloads without an ID, and an unknown mode. Unmarshal each raw payload into CostBasis and assert the error contains the expected validation substring, preserving the existing happy-path round-trip assertions.tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql (1)
1-11: 🩺 Stability & Availability | 🔵 TrivialUse non-blocking migration DDL for the index/foreign-key changes.
Both directions touch live charge tables with DDL that acquires locks while holding the migration transaction. The up migration only marks its schema migrations as locked (
atlas:nolint MF101), so add an online rollout plan or a maintenance window for bothup.sqlanddown.sql, using patterns such as non-blocking indexes/constraint validation where the chosen runner supports them.🤖 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 `@tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql` around lines 1 - 11, The charge-table index and foreign-key changes use blocking DDL in both migration directions. Update tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql lines 1-11 and tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.down.sql lines 1-8 to use the runner’s supported online index creation and constraint-validation patterns, or explicitly schedule these operations for a maintenance window; ensure both up and down migrations have a non-blocking rollout plan rather than relying only on atlas:nolint MF101.Sources: Path instructions, Linters/SAST tools
🤖 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/billing/charges/service/creditpurchase_test.go`:
- Around line 319-337: The “referenced cost basis cannot be deleted” subtest
currently accepts any deletion error and does not verify the referenced row
survives. Update the assertions after DeleteOneID to require the specific
charge_credit_purchase_cost_basis_charge_fk foreign-key violation, then fetch
and verify the cost-basis row remains readable, while preserving the existing
persisted-charge checks.
In
`@tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql`:
- Around line 1-11: Add a preflight before the unique index statements in the
migration that detects duplicate non-NULL cost_basis_id values in
charge_flat_fees, charge_usage_based, and charge_credit_purchases. Make the
migration fail before creating any indexes when duplicates exist, or explicitly
clean up duplicates first while preserving valid references.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/costbasis_test.go`:
- Around line 62-99: Extend TestCostBasisJSONRoundTrip with table-driven
negative JSON cases covering fiat payloads with custom-currency fields, dynamic
payloads with rate or currencyCostBasisID, pinned payloads without an ID, and an
unknown mode. Unmarshal each raw payload into CostBasis and assert the error
contains the expected validation substring, preserving the existing happy-path
round-trip assertions.
In
`@tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql`:
- Around line 1-11: The charge-table index and foreign-key changes use blocking
DDL in both migration directions. Update
tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql
lines 1-11 and
tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.down.sql
lines 1-8 to use the runner’s supported online index creation and
constraint-validation patterns, or explicitly schedule these operations for a
maintenance window; ensure both up and down migrations have a non-blocking
rollout plan rather than relying only on atlas:nolint MF101.
🪄 Autofix
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 Plus
Run ID: d695625c-9ce6-4fb1-8d2d-e1420881ebf2
⛔ Files ignored due to path filters (2)
openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (17)
api/v3/handlers/customers/credits/convert.goopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/costbasis.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/costbasis.goopenmeter/billing/charges/creditpurchase/costbasis_test.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/lineengine.goopenmeter/billing/charges/creditpurchase/service/realizations/service.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.down.sqltools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sql
e4975ae to
344a252
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/migrate/credit_purchase_cost_basis_backfill_test.go (2)
270-298: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider mirroring the
cost_basis_idunique index in the test DDL.The follow-up migration
tools/migrate/migrations/20260807145338_enforce_charge_cost_basis_relationships.up.sqladdsCREATE UNIQUE INDEX "chargecreditpurchases_cost_basis_id" ON "charge_credit_purchases" ("cost_basis_id"). The test schema here omits it, so a backfill regression that assigned onecost_basis_idto two charges would still pass. The temp table in the migration already declarescost_basis_idasUNIQUE, so this is belt-and-braces, but it is a one-liner and it pins the invariant the two migrations agree on.Note that a plain unique index tolerates multiple
NULLvalues in PostgreSQL, so the promotional and fiat rows stay valid.🧪 Proposed test DDL addition
initial_payment_settlement_status text NULL, cost_basis_id text NULL ); + + CREATE UNIQUE INDEX chargecreditpurchases_cost_basis_id + ON charge_credit_purchases (cost_basis_id); CREATE TABLE charge_credit_purchase_cost_bases (🤖 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 `@tools/migrate/credit_purchase_cost_basis_backfill_test.go` around lines 270 - 298, Update the test DDL for charge_credit_purchases to add the unique index chargecreditpurchases_cost_basis_id on cost_basis_id, matching the follow-up migration and preserving PostgreSQL’s handling of multiple NULL values.
236-247: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEach subtest starts a fresh PostgreSQL database.
withCreditPurchaseCostBasisBackfillTablescallstestutils.InitPostgresDBper case, so this table of 12 cases pays 12 database initializations. Every case expects the migration to fail, and the migration wraps everything inBEGIN; ... COMMIT;, so a failed run leaves no rows behind. You could share one database across the cases and delete the inserted row between them. Totally optional, just a CI-time win.🤖 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 `@tools/migrate/credit_purchase_cost_basis_backfill_test.go` around lines 236 - 247, Optimize the table-driven test around withCreditPurchaseCostBasisBackfillTables by initializing the PostgreSQL database once for all cases instead of once per subtest. Reuse that database while ensuring each case removes its inserted row before the next case, preserving the existing migration failure assertions and isolation.
🤖 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/billing/charges/creditpurchase/charge_test.go`:
- Around line 88-124: Add a test case alongside
TestCreateInputValidateRejectsHistoricalExpiryWithoutEffectiveAt where ExpiresAt
equals the frozen now value, and assert validation rejects it with the same
effective-time error. Keep the existing earlier-expiry coverage and reuse the
current CreateInput setup.
---
Nitpick comments:
In `@tools/migrate/credit_purchase_cost_basis_backfill_test.go`:
- Around line 270-298: Update the test DDL for charge_credit_purchases to add
the unique index chargecreditpurchases_cost_basis_id on cost_basis_id, matching
the follow-up migration and preserving PostgreSQL’s handling of multiple NULL
values.
- Around line 236-247: Optimize the table-driven test around
withCreditPurchaseCostBasisBackfillTables by initializing the PostgreSQL
database once for all cases instead of once per subtest. Reuse that database
while ensuring each case removes its inserted row before the next case,
preserving the existing migration failure assertions and isolation.
🪄 Autofix
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 Plus
Run ID: 63fbd000-e57c-46e8-8a8e-78f7ca6a9e97
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
openmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/service.gotools/migrate/credit_purchase_cost_basis_backfill_test.gotools/migrate/migrations/20260807061955_migrate_credit_purchase_cost_basis_schema_level_2.down.sqltools/migrate/migrations/20260807061955_migrate_credit_purchase_cost_basis_schema_level_2.up.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/creditpurchase/charge.go
344a252 to
10fb5c7
Compare
|
Regarding CodeRabbit’s blocking-DDL note: thanks for flagging the availability tradeoff. We’re intentionally keeping this migration transactional and accepting the write-lock behavior for this rollout, which will be coordinated accordingly. Splitting it into non-transactional concurrent-index and staged constraint migrations would introduce partial-application and recovery complexity that we do not want to take on in this PR. No migration change is planned for this item. |
2 participants
Summary
Root cause
Late review identified that the credit-purchase cost-basis foreign key cascaded in the wrong direction once the relationship became active. The original cost-basis change also unintentionally made service-period start affect credit-purchase effective time and left several representation and persistence invariants enforced only by application conventions.
Impact
Cost-basis records can no longer cascade-delete financial charge records, every charge type enforces the intended one-to-one cost-basis relationship in PostgreSQL, and the follow-up cleanup removes duplicated conversion and loading paths without changing purchase amount behavior.
Validation
go test ./openmeter/billing/charges/creditpurchase ./openmeter/billing/charges/creditpurchase/adapter ./openmeter/billing/charges/creditpurchase/service/... ./api/v3/handlers/customers/creditsPOSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/charges/servicemake migrate-check-lintmake migrate-check-validatemake lint-go-fastTicket: OM-436
Summary by CodeRabbit
New Features
Bug Fixes
Data Integrity
Greptile Summary
The PR corrects credit-purchase cost-basis persistence and effective-time behavior while consolidating conversion logic.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Input[Credit-purchase input] --> Validate[Validate intent and expiry] Validate --> Normalize[Normalize purchase intent] Normalize --> Persist[Persist charge] Persist --> CostBasis{Cost-basis schema} CostBasis -->|Legacy| Upgrade[Materialize dedicated cost basis] CostBasis -->|Current| Reuse[Reuse loaded cost-basis edge] Upgrade --> Reuse Reuse --> Resolve[Resolve fiat rate] Resolve --> Amount[Calculate rounded fiat amount] Amount --> Invoice[Invoice or external settlement]Reviews (3): Last reviewed commit: "fix: validate credit purchase expiry at ..." | Re-trigger Greptile
Context used: