-
Notifications
You must be signed in to change notification settings - Fork 146
feat: detailed line schema #3770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis PR introduces detailed invoice line tracking to the billing system. It adds three new database tables (detailed lines, their amount discounts, and a schema version tracker), updates the BillingInvoice entity with a schema_level field, and establishes relationships between invoices and detailed line items. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Adding the new tables for detailed lines. A new table billing_invoice_write_schema_levels is added that can be used to toggle which schema version the adapter writes. Each invoice will have one schema version to signify which tables the invoice is using. This will allow us to do non-distruptive upgrades where needed.
071ea70 to
803bfc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openmeter/ent/schema/billing.go (1)
1346-1368: Redundant unique index on primary key.The
idfield will be the primary key, which already enforces uniqueness. The explicit unique index inIndexes()is redundant and creates unnecessary overhead.♻️ Suggested fix
func (BillingInvoiceWriteSchemaLevel) Indexes() []ent.Index { - return []ent.Index{ - index.Fields("id").Unique(), - } + return nil }tools/migrate/migrations/20260112160815_detailed-lines.up.sql (1)
1-10: Redundant unique index on primary key column.Line 10 creates a unique index on
id, butidis already the primary key (line 5). PostgreSQL automatically creates a unique index for primary keys, so this explicit index is redundant.If this migration is auto-generated by ent, consider removing the redundant index definition from the schema (as noted in the billing.go review).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (46)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/billinginvoice.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoice/billinginvoice.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoice/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoice_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoice_query.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoice_update.gois excluded by!**/ent/db/**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/billinginvoicewriteschemalevel.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel/billinginvoicewriteschemalevel.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel_delete.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel_query.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicewriteschemalevel_update.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline/billingstandardinvoicedetailedline.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline/where.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline_delete.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline_query.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedline_update.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount/billingstandardinvoicedetailedlineamountdiscount.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount/where.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount_create.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount_delete.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount_query.gois excluded by!**/ent/db/**openmeter/ent/db/billingstandardinvoicedetailedlineamountdiscount_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/ent.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 (3)
openmeter/ent/schema/billing.gotools/migrate/migrations/20260112160815_detailed-lines.down.sqltools/migrate/migrations/20260112160815_detailed-lines.up.sql
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
openmeter/ent/schema/billing.go
🧬 Code graph analysis (1)
openmeter/ent/schema/billing.go (4)
pkg/framework/entutils/mixins.go (3)
AnnotationsMixin(157-159)AnnotationsMixin(162-170)AnnotationsMixin(172-181)openmeter/billing/invoicedetailedline.go (1)
FlatFeeCategoryRegular(21-21)openmeter/productcatalog/price.go (1)
PaymentTermType(24-24)openmeter/billing/discount.go (1)
DiscountReason(147-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
openmeter/ent/schema/billing.go (8)
477-478: LGTM!Clean edge addition with cascade delete annotation. The naming
detailed_lines_v2is clear about versioning while the existingdetailed_linesedge remains for backward compatibility.
701-704: TODO comment noted.Good practice to document the planned rename. The comment correctly indicates this should be renamed alongside the DB table for consistency.
873-878: Comment doesn't match field definition.The comment says "Quantity is optional as for usage-based billing..." but the field is defined as required (no
Optional()orNillable()). This differs fromBillingInvoiceLinewhere quantity is optional.If detailed lines are only created after calculation (when quantity is known), then the field should be required and the comment should be updated. Otherwise, if the field should actually be optional, the definition needs to change.
910-920: Good use of StorageKey for the partial index.The explicit storage key
billingstdinvdetailedline_ns_parent_child_idkeeps the index name manageable. The partial index with thedeleted_at IS NULLfilter is a nice pattern for soft-delete scenarios.
923-937: LGTM!The edges are well-structured: both
billing_invoiceandbilling_invoice_lineedges are required (makes sense for detailed child lines), and theamount_discountsedge properly cascades deletes.
940-1000: LGTM!Nicely mirrors the existing
BillingInvoiceLineDiscountpattern. Good reuse ofBillingInvoiceLineDiscountBasemixin and consistent index structure with the explicit storage key for the partial unique index.
1166-1168: LGTM!Simple and clean addition. The default of
1aligns with the seeded value in the migration.
1205-1206: LGTM!The edge to
BillingStandardInvoiceDetailedLinewith cascade delete is properly defined, allowing direct traversal from invoice to detailed lines.tools/migrate/migrations/20260112160815_detailed-lines.up.sql (2)
14-61: Well-structured table with appropriate constraints and indexes.The
billing_standard_invoice_detailed_linestable looks solid:
- FKs properly cascade on delete
- GIN index on annotations for JSON queries
- Partial unique index for
child_unique_reference_idwith soft-delete awarenessOne observation: the
idcolumn has both a PRIMARY KEY constraint (line 44) and a unique index (lines 50-51). The unique index is redundant here too, but since this is likely auto-generated by ent, it's a minor nit.
62-87: LGTM!The discounts table follows the established pattern with appropriate FK to the parent detailed lines table and consistent indexing strategy.
tools/migrate/migrations/20260112160815_detailed-lines.down.sql (1)
1-32: LGTM!The down migration correctly reverses all changes in the proper order:
- Child table indexes and table dropped first
- Parent table indexes and table dropped second
- Column modification reverted
- Config table dropped last
The explicit
DROP INDEXstatements beforeDROP TABLEare technically redundant (dropping a table auto-drops its indexes), but they don't cause issues and the migration will work correctly.
Overview
Adding the new tables for detailed lines.
A new table billing_invoice_write_schema_levels is added that can be used to toggle which schema version the adapter writes.
Each invoice will have one schema version to signify which tables the invoice is using.
This will allow us to do non-distruptive upgrades where needed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.