refactor: make subscription sync a service#3700
Conversation
📝 WalkthroughWalkthroughLarge-scale refactor replacing a handler-based architecture with a service-based abstraction for billing subscription synchronization. Involves package restructuring from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
turip
added
the
release-note/ignore
turip
force-pushed
the
refactor/make-subscription-sync-a-service
branch
from
3b2d044 to
8eb6a42
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
openmeter/billing/worker/subscriptionsync/service/feehelper.go (1)
1-71: Helpers look good after the move; just notesetIfDoesNotEqual’s pointer expectationsThe move into the
servicepackage keeps the flat-fee helpers and the smalltypeWithEqual/setIfDoesNotEqualutility intact, so behaviour should be unchanged.One small thing to keep in mind:
setIfDoesNotEqualassumes bothexistingandwasChangeare non‑nil, and will panic if either is nil. That’s totally fine if callers always pass real pointers, but it might be worth either:
- adding a short comment about that precondition, or
- adding a defensive
if existing == nil || wasChange == nil { return }if you expect more generic reuse later.openmeter/billing/worker/subscriptionsync/service/suitebase_test.go (1)
35-59: Test suite migration toServicelooks good; be mindful of shared feature flagsThe suite’s move from holding a
*Handlerto a*Servicebuilt viaNew(Config{...})is consistent with the new implementation, and the rest of the helpers still compose fine around it.One minor thing to keep in mind:
AfterTestandenableProratingmutates.Service.featureFlagson a sharedServiceinstance. That’s totally reasonable for this style of integration suite, but if you ever start running these tests (or subtests) in parallel, you’ll want either a per‑testServiceinstance or some light synchronization around toggling those flags to avoid data races.Also applies to: 118-118, 161-164
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (1)
104-127: Consider usingclock.Now()instead oftime.Now()for reconciliationHere you pass
time.Now()intoSynchronizeSubscription, whileListSubscriptionsabove already relies onclock.Now()for its time window. For consistency and better testability (respecting frozen/adjusted time), it’d be cleaner to useclock.Now()here too:- return r.subscriptionSync.SynchronizeSubscription(ctx, subsView, time.Now()) + return r.subscriptionSync.SynchronizeSubscription(ctx, subsView, clock.Now())Totally fine to keep as-is if there’s a specific reason to use wall-clock time, but aligning with
clockwould match the rest of this component.openmeter/billing/validators/customer/customer.go (1)
17-30: Validator wiring + sync usage looks good (tiny optional tweak)The move to a
subscriptionsync.Servicedependency and the extra nil-check inNewValidatorare solid and keep the dependency graph explicit. The sync call inValidateDeleteCustomeris also wired correctly toSynchronizeSubscription.If you ever want to make time-dependent tests a bit more deterministic, you could capture
asOf := time.Now()once before theforloop and reuse it for eachSynchronizeSubscriptioncall, instead of callingtime.Now()per item—but that’s purely cosmetic.Also applies to: 36-37, 58-67
openmeter/billing/worker/worker.go (1)
67-69: Small inconsistency in error message.The error message still says "billing subscription sync handler is required" but the field is now
BillingSubscriptionSync(a service, not a handler). Minor nit, but worth fixing for consistency.if w.BillingSubscriptionSync == nil { - return fmt.Errorf("billing subscription sync handler is required") + return fmt.Errorf("billing subscription sync service is required") }openmeter/billing/worker/subscriptionsync/service/handlers.go (1)
16-18: Truncated comment.The comment on lines 16-17 appears incomplete: "it will make sure that we synchronize the" - it seems like something is missing at the end.
Consider completing the comment to explain what exactly is being synchronized, for example:
// HandleCancelledEvent is a handler for the subscription cancel event, it will make sure that -// we synchronize the +// we synchronize the subscription's billing state up to the cancellation date. func (s *Service) HandleCancelledEvent(ctx context.Context, event *subscription.CancelledEvent) error {openmeter/billing/worker/subscriptionsync/service/sync.go (3)
619-635: discountsToBillingDiscounts doesn't use receiver.This method also doesn't use the
sreceiver - it's a pure transformation function. Same reasoning applies as above: keeping it as a method maintains API consistency.If you want to be pedantic about it, this could be a package-level function since it doesn't use any Service state. But it's fine as-is for consistency.
714-786: getPatchesForExistingLine is well-documented.The inline comment on line 724 still says "resyncronization" - might want to fix that typo too while you're at it since you're fixing "Syncronize" → "Synchronize" elsewhere.
// Manual edits prevent resyncronization so that we preserve the user intent + // Manual edits prevent resynchronization so that we preserve the user intent
755-756: Comment typo: "syncronizing".Line 755 has a comment with "syncronizing" - same typo being fixed elsewhere in this PR.
- // Let's handle the flat fee prorating (e.g. syncronizing the amount maybe in retrospect) + // Let's handle the flat fee prorating (e.g. synchronizing the amount maybe in retrospect)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
app/common/billing.go(3 hunks)app/common/openmeter_billingworker.go(3 hunks)cmd/billing-worker/wire_gen.go(2 hunks)cmd/jobs/billing/subscriptionsync/sync.go(3 hunks)cmd/jobs/internal/wire.go(3 hunks)cmd/jobs/internal/wire_gen.go(7 hunks)cmd/jobs/quickstart/cronjobs.go(2 hunks)openmeter/billing/validators/customer/customer.go(3 hunks)openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go(4 hunks)openmeter/billing/worker/subscriptionsync/service.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/feehelper.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/handlers.go(6 hunks)openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/patch.go(2 hunks)openmeter/billing/worker/subscriptionsync/service/phaseiterator.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/phaseiterator_test.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/service.go(1 hunks)openmeter/billing/worker/subscriptionsync/service/suitebase_test.go(6 hunks)openmeter/billing/worker/subscriptionsync/service/sync.go(31 hunks)openmeter/billing/worker/subscriptionsync/service/sync_test.go(60 hunks)openmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.go(3 hunks)openmeter/billing/worker/worker.go(7 hunks)test/billing/subscription_test.go(4 hunks)test/subscription/framework_test.go(4 hunks)test/subscription/scenario_firstofmonth_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/billing/worker/subscriptionsync/service/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/phaseiterator_test.goopenmeter/billing/worker/subscriptionsync/service/patch.gotest/subscription/framework_test.goopenmeter/billing/worker/subscriptionsync/service.goopenmeter/billing/worker/subscriptionsync/service/phaseiterator.goopenmeter/billing/validators/customer/customer.gocmd/jobs/quickstart/cronjobs.gotest/billing/subscription_test.goopenmeter/billing/worker/subscriptionsync/service/feehelper.goopenmeter/billing/worker/subscriptionsync/service/suitebase_test.goopenmeter/billing/worker/subscriptionsync/service/service.gocmd/jobs/internal/wire.goopenmeter/billing/worker/subscriptionsync/service/sync_test.gotest/subscription/scenario_firstofmonth_test.gocmd/billing-worker/wire_gen.goopenmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.gocmd/jobs/billing/subscriptionsync/sync.goapp/common/billing.goapp/common/openmeter_billingworker.goopenmeter/billing/worker/subscriptionsync/reconciler/reconciler.goopenmeter/billing/worker/worker.gocmd/jobs/internal/wire_gen.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/handlers.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
openmeter/billing/worker/subscriptionsync/service/phaseiterator_test.gotest/subscription/framework_test.gotest/billing/subscription_test.goopenmeter/billing/worker/subscriptionsync/service/suitebase_test.goopenmeter/billing/worker/subscriptionsync/service/sync_test.gotest/subscription/scenario_firstofmonth_test.goopenmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.go
🧠 Learnings (3)
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/billing/worker/subscriptionsync/service/phaseiterator_test.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/sync.go
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
test/subscription/framework_test.goopenmeter/billing/worker/subscriptionsync/service/sync.go
📚 Learning: 2025-08-29T12:31:52.802Z
Learnt from: chrisgacsal
Repo: openmeterio/openmeter PR: 3291
File: app/common/customer.go:88-89
Timestamp: 2025-08-29T12:31:52.802Z
Learning: In Go projects using Google's wire dependency injection framework, named types (without =) should be used instead of type aliases (with =) to work around wire limitations. For example, use `type CustomerSubjectValidatorHook customerservicehooks.SubjectValidatorHook` instead of `type CustomerSubjectValidatorHook = customerservicehooks.SubjectValidatorHook` when wire is involved.
Applied to files:
openmeter/billing/validators/customer/customer.go
🧬 Code graph analysis (19)
openmeter/billing/worker/subscriptionsync/service/patch.go (3)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/billing/invoicelinesplitgroup.go (1)
LineOrHierarchy(281-285)
test/subscription/framework_test.go (1)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)
openmeter/billing/worker/subscriptionsync/service.go (3)
openmeter/subscription/subscriptionview.go (1)
SubscriptionView(19-24)openmeter/subscription/events.go (2)
CancelledEvent(104-104)SubscriptionSyncEvent(199-201)openmeter/billing/events.go (1)
InvoiceCreatedEvent(93-95)
openmeter/billing/validators/customer/customer.go (4)
openmeter/billing/validators/subscription/validator.go (2)
Validator(15-18)NewValidator(20-28)openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/subscription/service.go (1)
Service(37-40)
cmd/jobs/quickstart/cronjobs.go (1)
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (1)
ReconcilerAllInput(130-130)
test/billing/subscription_test.go (2)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (3)
Service(55-62)New(64-76)Config(20-27)
openmeter/billing/worker/subscriptionsync/service/suitebase_test.go (1)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)
cmd/jobs/internal/wire.go (2)
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (1)
Reconciler(23-28)app/common/billing.go (1)
NewBillingSubscriptionSyncService(124-132)
openmeter/billing/worker/subscriptionsync/service/sync_test.go (3)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)pkg/clock/clock.go (1)
Now(14-21)
cmd/billing-worker/wire_gen.go (2)
app/common/billing.go (1)
NewBillingSubscriptionSyncService(124-132)app/common/openmeter_billingworker.go (1)
NewBillingWorkerOptions(73-98)
openmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.go (3)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/testutils/time.go (1)
GetRFC3339Time(8-15)
cmd/jobs/billing/subscriptionsync/sync.go (1)
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (2)
ReconcilerListSubscriptionsInput(70-74)ReconcilerAllInput(130-130)
app/common/billing.go (3)
openmeter/billing/validators/customer/customer.go (1)
NewValidator(17-31)openmeter/billing/worker/subscriptionsync/service/service.go (3)
Service(55-62)New(64-76)Config(20-27)openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (3)
Reconciler(23-28)NewReconciler(57-68)ReconcilerConfig(30-35)
app/common/openmeter_billingworker.go (4)
app/common/billing.go (3)
NewBillingSubscriptionSyncService(124-132)BillingService(45-98)BillingAdapter(35-43)openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/billing/worker/worker.go (1)
WorkerOptions(21-36)
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (2)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)
openmeter/billing/worker/worker.go (5)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/billing/worker/asyncadvance/asyncadvance.go (1)
Handler(11-14)openmeter/subscription/subscriptionview.go (1)
SubscriptionView(19-24)openmeter/subscription/events.go (4)
CancelledEvent(104-104)ContinuedEvent(136-136)UpdatedEvent(168-172)SubscriptionSyncEvent(199-201)
cmd/jobs/internal/wire_gen.go (3)
app/common/billing.go (2)
NewBillingSubscriptionSyncService(124-132)NewBillingSubscriptionReconciler(115-122)app/common/notification.go (3)
NewNotificationWebhookHandler(126-151)NewNotificationEventHandler(64-97)NewNotificationService(99-118)openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (1)
Reconciler(23-28)
openmeter/billing/worker/subscriptionsync/service/sync.go (3)
openmeter/billing/worker/subscriptionsync/service.go (1)
Service(11-14)openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go (1)
NewInvoiceUpdater(24-29)openmeter/billing/worker/subscriptionsync/service/phaseiterator.go (1)
NewPhaseIterator(98-122)
openmeter/billing/worker/subscriptionsync/service/handlers.go (6)
openmeter/billing/worker/subscriptionsync/service/service.go (1)
Service(55-62)openmeter/subscription/events.go (1)
CancelledEvent(104-104)pkg/clock/clock.go (1)
Now(14-21)openmeter/subscription/subscriptionview.go (1)
SubscriptionView(19-24)openmeter/billing/events.go (1)
InvoiceCreatedEvent(93-95)pkg/models/id.go (1)
NamespacedID(7-10)
⏰ 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). (6)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (39)
openmeter/billing/worker/subscriptionsync/service/patch.go (1)
96-185:getDeletePatchesForLinehanging offServicelooks consistent with prior behaviourThe receiver switch to
*Serviceis clean, and the logic for bothLineOrHierarchyTypeLineandLineOrHierarchyTypeHierarchyis unchanged:
- Single lines: still skip
AnnotationSubscriptionSyncIgnoreand emit oneline_deletepatch.- Hierarchies: still optionally emit a
split_line_group_deletewhen the group itself isn’t deleted, then emitline_deletepatches for each non‑deleted child line.The default branch returning an error for unknown
LineOrHierarchytypes is also preserved. No issues from the refactor here.app/common/openmeter_billingworker.go (1)
16-94: Billing worker wiring tosubscriptionsync.Servicelooks correctThe switch from a concrete handler to
subscriptionsync.Serviceis wired cleanly:
- Importing
billing/worker/subscriptionsyncand addingNewBillingSubscriptionSyncServiceto theBillingWorkerwire set keeps DI centralized.NewBillingWorkerOptionsnow takessubscriptionSyncService subscriptionsync.Serviceand threads it through tobillingworker.WorkerOptions.BillingSubscriptionSync, alongside the existing router, event bus, billing, adapter, and subscription service fields.Assuming the wire-generated files have been regenerated, this should keep the worker behaviour identical while giving you the new service abstraction.
openmeter/billing/worker/subscriptionsync/service/phaseiterator.go (1)
1-24: Phase iterator now correctly lives in theservicepackageOnly the package declaration moved; the iterator, period math, and tracing logic are unchanged and are still tested via
phaseiterator_test.go. The refactor keeps behaviour intact while aligning namespaces with the new subscriptionsync service layout.openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go (1)
1-398: Invoice updater package move aligns with the new service layoutThe
InvoiceUpdaterremains functionally the same: it still
- parses
linePatchvalues into creates/updates/deletes and split-group changes,- updates mutable invoices via
UpdateInvoiceand optionally deletes now-empty invoices, and- records validation issues instead of mutating immutable invoices, using the shared flat-fee helpers.
Moving this into the
servicepackage just tightens cohesion around subscriptionsync without changing behaviour.openmeter/billing/worker/subscriptionsync/service/phaseiterator_test.go (1)
1-1277: Phase iterator tests still give strong coverage; package rename is fineRenaming the test package to
servicelines it up with the refactored implementation, and the table‑driven tests still cover a wide range of cadence, phase end, cancellation, flat‑fee vs usage‑based, and billing‑anchor scenarios. That should give good confidence that the iterator continues to behave the same under the new package structure.test/subscription/framework_test.go (1)
20-21: Subscription framework tests now cleanly depend onsubscriptionsync.ServiceThe switch from a concrete worker handler to the
subscriptionsync.Serviceinterface intestDepslooks solid:
- You construct the concrete service via
subscriptionsyncservice.Newwith the same core dependencies (billing service + adapter, subscription service, logger, tracer) that the handler previously used.testDepsnow exposes this assubscriptionSyncService subscriptionsync.Service, which keeps the tests aligned with the new abstraction without over‑coupling them to the implementation type.This should preserve the existing behaviour of the subscription tests while matching the new service-based design.
Also applies to: 33-42, 101-107, 145-154
openmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.go (1)
141-141: Service-based sync usage looks goodSwitching the tests to
s.Service.SynchronizeSubscriptionkeeps the behavior the same and matches the new service abstraction. No extra plumbing concerns from the test side.Also applies to: 338-338
cmd/billing-worker/wire_gen.go (1)
284-295: Wiring insubscriptionsyncServiceis consistentThe new
subscriptionsyncServiceconstruction and error handling fits the existing Wire-generated pattern, and passing it intoNewBillingWorkerOptionslines up with the updated options struct. All good from a wiring/cleanup perspective.openmeter/billing/worker/subscriptionsync/service/sync_test.go (1)
58-4632: Nice, call sites now consistently go through the sync serviceAll the updated tests that previously called a handler now use
s.Service.SynchronizeSubscription(...)/SynchronizeSubscriptionAndInvoiceCustomer(...), which lines up with the new service abstraction and keeps the test intent clear. Nothing worrying from a correctness or readability angle here.cmd/jobs/billing/subscriptionsync/sync.go (1)
10-10: CLI now targets the new reconciler API correctlyThe job now imports the reconciler package and passes
reconciler.ReconcilerListSubscriptionsInput/ReconcilerAllInputwith the expected fields. That lines up cleanly with the refactored subscriptionsync reconciler.Also applies to: 38-42, 77-81
test/subscription/scenario_firstofmonth_test.go (1)
216-216: Tests are wired to the new subscription sync service nicelyBoth sync calls now go through
tDeps.subscriptionSyncService, matching the new service abstraction while keeping the original test semantics intact. Looks good.Also applies to: 425-425
openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go (1)
24-35: Good move to depend on thesubscriptionsync.ServiceinterfaceThe reconciler now takes a
subscriptionsync.Servicevia config, with validation ensuring it’s non-nil, and stores it insubscriptionSync. That’s a nice, decoupled dependency and fits well with the new service-based design.Also applies to: 57-68
cmd/jobs/internal/wire.go (1)
21-21: Wire setup correctly updated for the new sync service + reconcilerThe Application struct now depends on
*billingworkersubscriptionreconciler.Reconciler, and the provider set includescommon.NewBillingSubscriptionSyncServicepluscommon.NewBillingSubscriptionReconciler. That lines up with the new service interface and keeps the DI graph tidy.Also applies to: 48-48, 68-86
openmeter/billing/worker/subscriptionsync/service/service.go (1)
15-76: Service config + constructor look solidNice setup here:
Config.Validateenforces all the important deps (billing, subscription, tx, logger, tracer),FeatureFlagsis explicit, and the interface assertion guaranteesServiceactually satisfiessubscriptionsync.Service. TheNewconstructor is minimal and clear, so this should be easy to wire and reason about.openmeter/billing/worker/subscriptionsync/service.go (1)
11-24: Nice, clean service abstractionEmbedding
EventHandlerandSyncServiceintoServicekeeps the API tidy, and the method signatures line up well with the domain events andSubscriptionView. This should make future consumers straightforward to wire.cmd/jobs/quickstart/cronjobs.go (1)
12-13: Reconciler wiring update looks consistentThe updated import and the
BillingSubscriptionReconciler.Allcall withreconciler.ReconcilerAllInputmatch the reconciler’s API and keep the cron job behavior unchanged.Also applies to: 37-40
test/billing/subscription_test.go (1)
17-19: Tests now match the new service abstraction nicelySwitching the suite to depend on
subscriptionsync.Serviceand constructing it viasubscriptionsyncservice.Newmirrors the production wiring well, and theSynchronizeSubscriptioncall increateCustomerWithSubscriptionkeeps behavior the same with the new API.Also applies to: 34-35, 45-56, 333-334
app/common/billing.go (2)
18-21: Good move centralizing subscription sync wiring hereCreating
subscriptionSyncServiceviaNewBillingSubscriptionSyncServiceand feeding it intobillingcustomer.NewValidatorkeeps all the billing-related wiring in one place and cleanly exposessubscriptionsync.Serviceas the dependency for validators. This should make it much easier to evolve the sync layer (e.g., add DB-backed avoidance) without touching callers.Also applies to: 76-82
115-121: Reconciler + sync service providers are well-shaped for Wire
NewBillingSubscriptionReconcilerandNewBillingSubscriptionSyncServiceline up with the corresponding config structs and return types, and they’re friendly to Wire-style DI. As long as they’re included in the relevant wire sets, this should keep worker wiring straightforward.Also applies to: 124-131
cmd/jobs/internal/wire_gen.go (4)
1-37: LGTM on the import and package structure changes.The import path update to
subscriptionsync/reconcileraligns with the new package structure. Since this is Wire-generated code, the changes correctly reflect the underlying wire.go configuration.
343-362: Service wiring looks good.The two-step initialization pattern is clean:
- Create the sync service with
NewBillingSubscriptionSyncService- Pass it to
NewBillingSubscriptionReconcilerThis properly separates the sync service from the reconciler, enabling the future database layer for sync avoidance mentioned in the PR objectives.
406-436: Variable rename is fine.Renaming
webhookHandlertohandlerworks and keeps things simpler. The usage inNewNotificationEventHandlerandNewNotificationServiceis consistent.
511-511: Type update aligns with the refactor.The
BillingSubscriptionReconcilerfield now correctly uses*reconciler.Reconcilerfrom the new package path.openmeter/billing/worker/worker.go (5)
14-14: Import update looks good.Clean switch to the new
subscriptionsyncpackage.
29-35: Nice simplification of the options struct.The field rename from
BillingSubscriptionSyncHandlertoBillingSubscriptionSyncis cleaner, and using thesubscriptionsync.Serviceinterface is a good abstraction.
74-83: Struct fields updated consistently.The Worker struct now correctly holds
subscriptionSync subscriptionsync.Serviceand the field ordering is logical.
99-104: Worker initialization is clean.Fields are assigned correctly from the options.
142-190: Event handlers updated correctly with typo fix.The method name correction from
SyncronizeSubscriptionAndInvoiceCustomertoSynchronizeSubscriptionAndInvoiceCustomer(fixing the missing "h" in "Synchronize") is applied consistently across all call sites. The handler methods (HandleCancelledEvent,HandleSubscriptionSyncEvent,HandleInvoiceCreation) follow a nice naming convention.openmeter/billing/worker/subscriptionsync/service/handlers.go (4)
1-1: Package rename looks good.The new
servicepackage name is appropriate for the service-based architecture.
24-41: Handler logic is solid.The cancellation handling logic is correct:
- First syncs to current time if
ActiveTois nil (returning an error after sync to ensure at least partial state is captured)- Otherwise syncs up to the subscription end date
The method name correction to
SynchronizeSubscriptionAndInvoiceCustomeris applied correctly.
45-48: Good defensive nil check.Adding the nil check for
eventat the start ofHandleInvoiceCreationis a nice safety improvement. This prevents potential nil pointer dereferences downstream.
54-78: Invoice creation handler looks correct.The logic properly:
- Skips gathering invoices (line 50-52)
- Collects unique affected subscription IDs
- Fetches views and synchronizes each subscription
Using
clock.Now()as the reference point (lines 73-75) is appropriate since delayed processing might need to provision additional lines.openmeter/billing/worker/subscriptionsync/service/sync.go (7)
1-1: Package structure looks good.The
servicepackage is well-organized for the new service-based architecture.
43-63: Receiver updated consistently.The
invoicePendingLinesmethod now uses the*Servicereceiver and trace span naming is updated accordingly.
65-76: HandleSubscriptionSyncEvent is clean.Proper nil check, fetches the subscription view, and delegates to
SynchronizeSubscriptionAndInvoiceCustomer.
78-100: SynchronizeSubscriptionAndInvoiceCustomer looks good.The trace span name is updated to reflect the corrected method name. The logic flow is clear: sync the subscription, then invoice pending lines.
102-186: SynchronizeSubscription is well-structured.The main sync logic is clean:
- Early return for subscriptions without billables
- Gets billing profile for validation
- Acquires lock on customer
- Lists invoices and builds lookup map
- Compares subscription with existing lines
- Generates and applies patches
The receiver changes are consistent throughout.
377-420: Annotation checking methods.These methods (
lineOrHierarchyHasAnnotation,lineHasAnnotation,hierarchyHasAnnotation) don't actually uses(the receiver). They could be standalone functions, but keeping them as methods maintains consistency with the rest of the service pattern and allows for potential future enhancements that might need service dependencies.
788-885: getPatchesForExistingHierarchy is comprehensive.The hierarchy patch logic handles both expansion (e.g., continue subscription) and shrinking (e.g., cancellation) scenarios. The sorting by
Period.Endensures the last child is correctly identified. Good handling of edge cases.
2 participants
Overview
This refactor just moves subscriptionsync into a service, as we will have to have DB layer for sync avoidance.
It also corrects the typo of missing
hfrom Synchornize.Notes for reviewer
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.