chore: get rid of billing.Period#4184
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR migrates the billing system from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/worker/subscriptionsync/service/sync_test.go (1)
4613-4629:⚠️ Potential issue | 🟡 MinorPoint this line-period assertion at the created invoice.
This block says it’s checking the standard invoice, but
expectLinesstill receivesgatheringInvoice, so a regression in the created invoice’s period mapping could slip through.Suggested fix
- s.expectLines(gatheringInvoice, subsView.Subscription.ID, []expectedLine{ + s.expectLines(invoice, subsView.Subscription.ID, []expectedLine{As per coding guidelines, "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/sync_test.go` around lines 4613 - 4629, The test is asserting line periods against gatheringInvoice but should target the created invoice; update the s.expectLines call to pass the created invoice variable (e.g., createdInvoice) instead of gatheringInvoice so the period mapping for the actually-created invoice is validated; keep the same expectedLine block and subscription ID (subsView.Subscription.ID) and run the test to confirm it now fails/errs where the regression exists.
🧹 Nitpick comments (2)
openmeter/billing/charges/service/creditpurchase_test.go (1)
587-590: Tiny nit: you could reuseservicePeriodhere.The literal on lines 587-590 is identical to the
servicePeriodvariable defined at lines 439-442, so you could just asserts.Equal(servicePeriod, line.Period)and dodge the duplicateMustParseTimeInLocationcalls. Totally optional — feel free to ignore if you prefer the assertion to spell out the expected values explicitly.♻️ Proposed tweak
- s.Equal(timeutil.ClosedPeriod{ - From: datetime.MustParseTimeInLocation(s.T(), "2026-01-01T00:00:00Z", time.UTC).AsTime(), - To: datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:00:00Z", time.UTC).AsTime(), - }, line.Period) + s.Equal(servicePeriod, line.Period)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/creditpurchase_test.go` around lines 587 - 590, Replace the duplicated literal ClosedPeriod construction with the existing servicePeriod variable to avoid redundant MustParseTimeInLocation calls: update the assertion from s.Equal(timeutil.ClosedPeriod{...}, line.Period) to s.Equal(servicePeriod, line.Period), referencing the servicePeriod defined earlier and the line.Period being asserted; this uses the already-parsed times and removes the duplicate parsing logic.openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go (1)
70-73: Tiny nit: you can just passr.BillingPeriodthrough. 🪄Since
r.BillingPeriodis now already atimeutil.ClosedPeriod, rebuilding it field-by-field is equivalent to the field itself. Not a correctness issue at all — feel free to ignore if you prefer the explicit form.✨ Optional simplification
- BillingPeriod: timeutil.ClosedPeriod{ - From: r.BillingPeriod.From, - To: r.BillingPeriod.To, - }, + BillingPeriod: r.BillingPeriod,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go` around lines 70 - 73, The BillingPeriod construction is redundant: instead of rebuilding a timeutil.ClosedPeriod from r.BillingPeriod fields, just assign BillingPeriod: r.BillingPeriod; update the code in targetstateitem.go where the struct is created (refer to the BillingPeriod field and the variable r) to pass r.BillingPeriod directly rather than reconstructing a new timeutil.ClosedPeriod with From/To.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/billing/worker/subscriptionsync/service/sync_test.go`:
- Around line 4613-4629: The test is asserting line periods against
gatheringInvoice but should target the created invoice; update the s.expectLines
call to pass the created invoice variable (e.g., createdInvoice) instead of
gatheringInvoice so the period mapping for the actually-created invoice is
validated; keep the same expectedLine block and subscription ID
(subsView.Subscription.ID) and run the test to confirm it now fails/errs where
the regression exists.
---
Nitpick comments:
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 587-590: Replace the duplicated literal ClosedPeriod construction
with the existing servicePeriod variable to avoid redundant
MustParseTimeInLocation calls: update the assertion from
s.Equal(timeutil.ClosedPeriod{...}, line.Period) to s.Equal(servicePeriod,
line.Period), referencing the servicePeriod defined earlier and the line.Period
being asserted; this uses the already-parsed times and removes the duplicate
parsing logic.
In
`@openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go`:
- Around line 70-73: The BillingPeriod construction is redundant: instead of
rebuilding a timeutil.ClosedPeriod from r.BillingPeriod fields, just assign
BillingPeriod: r.BillingPeriod; update the code in targetstateitem.go where the
struct is created (refer to the BillingPeriod field and the variable r) to pass
r.BillingPeriod directly rather than reconstructing a new timeutil.ClosedPeriod
with From/To.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 191d8b56-f616-4938-a727-c820c67f5033
📒 Files selected for processing (55)
openmeter/app/stripe/appinvoice.goopenmeter/billing/adapter/invoice.goopenmeter/billing/adapter/invoicelinesplitgroup.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/usagebased/service/run/invoice.goopenmeter/billing/charges/usagebased/service/run/payment.goopenmeter/billing/charges/usagebased/service/run/payment_test.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/httpdriver/invoice_test.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/invoicedetailedline.goopenmeter/billing/invoiceline.goopenmeter/billing/invoicelinesplitgroup.goopenmeter/billing/lineengine/splitlinegroup.goopenmeter/billing/rating/service/mutator/credits_test.goopenmeter/billing/rating/service/rate/dynamic_test.goopenmeter/billing/rating/service/rate/package_test.goopenmeter/billing/rating/service/rate/tieredgraduated_test.goopenmeter/billing/rating/service/rate/tieredvolume_test.goopenmeter/billing/rating/service/rate/unit_test.goopenmeter/billing/rating/service/testutil/ubptest.goopenmeter/billing/service/invoice.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/invoicecalc/period.goopenmeter/billing/service/quantitysnapshot.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoice_test.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/item.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcharge.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/phaseiterator.goopenmeter/billing/worker/subscriptionsync/service/targetstate/phaseiterator_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/notification/internal/rule.gotest/app/custominvoicing/invocing_test.gotest/app/stripe/invoice_test.gotest/billing/adapter_test.gotest/billing/collection_test.gotest/billing/invoice_test.gotest/billing/suite.gotest/billing/tax_test.gotest/billing/taxcode_dual_write_test.gotest/billing/ubpflatfee_test.gotest/customer/subject.go
💤 Files with no reviewable changes (1)
- openmeter/billing/invoiceline.go
tothandras
approved these changes
Apr 20, 2026
This was referenced Apr 20, 2026
Merged
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
next patches will require it.
Notes for reviewer
Summary by CodeRabbit
From/Tofield naming instead ofStart/Endacross invoicing, rating, and subscription synchronization systems.