feat(billing): add mutable line deletion hook#4317
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAdds a LineEngine lifecycle hook OnMutableStandardLinesDeleted; invoice edit flow snapshots pre-edit lines and detects newly deleted standard lines, the service groups deleted lines by engine type and dispatches per-engine callbacks; engines and test helpers are updated (mostly no-op implementations) and unit tests added. ChangesMutable Standard Lines Deletion Lifecycle Hook
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/billing/charges/usagebased/service/lineengine.go (1)
231-233: Heads-up: this no-op may need revisiting for usage-based lines.Flat fee and credit purchase engines are stateless w.r.t. line deletion, but usage-based lines can have active realization runs started during
OnStandardInvoiceCreated. If a mutable usage-based standard line is deleted while a run is active, the run currently has no invoice line to land its results on — which could leave orphaned state in the charge state machine.This is clearly out of scope for this PR (the hook foundation is the right first step!), but it's worth tracking as a follow-up so the usage-based engine can fire an appropriate trigger (e.g., cancel/void the active run) when its lines are deleted.
🤖 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/usagebased/service/lineengine.go` around lines 231 - 233, The no-op OnMutableStandardLinesDeleted in LineEngine currently ignores deletions which can orphan active usage-based realization runs started in OnStandardInvoiceCreated; update OnMutableStandardLinesDeleted to detect if any deleted standard lines belong to usage-based charges, look up active realization runs in the charge state machine for those line IDs, and trigger appropriate cleanup (e.g., cancel or void the run and persist state) so runs do not remain orphaned — use LineEngine's existing helpers for charge lookup and state transitions (same services used by OnStandardInvoiceCreated) to locate runs and invoke the cancel/void transition.openmeter/billing/service/invoice_test.go (1)
18-37: 💤 Low valueGood coverage of the key scenarios!
The four cases — newly deleted, already deleted, unchanged, and appeared-already-deleted — map cleanly to the expected behavior. One gap worth considering: there's no test case for a non-mutable (system-managed) line that transitions from non-deleted to deleted. Since the hook is named
OnMutableStandardLinesDeleted, the filtering should exclude such lines, but nothing currently verifies that.🧪 Suggested additional case
before := billing.NewStandardInvoiceLines([]*billing.StandardLine{ newStandardLineForLineEngineTest("newly-deleted", billing.LineEngineTypeInvoice, false), newStandardLineForLineEngineTest("already-deleted", billing.LineEngineTypeInvoice, true), newStandardLineForLineEngineTest("unchanged", billing.LineEngineTypeInvoice, false), + newSystemManagedLineForLineEngineTest("system-deleted", billing.LineEngineTypeInvoice, false), }) after := billing.NewStandardInvoiceLines([]*billing.StandardLine{ newStandardLineForLineEngineTest("newly-deleted", billing.LineEngineTypeInvoice, true), newStandardLineForLineEngineTest("already-deleted", billing.LineEngineTypeInvoice, true), newStandardLineForLineEngineTest("unchanged", billing.LineEngineTypeInvoice, false), newStandardLineForLineEngineTest("new-deleted-line", billing.LineEngineTypeInvoice, true), + newSystemManagedLineForLineEngineTest("system-deleted", billing.LineEngineTypeInvoice, true), })(If filtering happens in the caller rather than in
collectNewlyDeletedStandardLines, this might be better placed in a test forexecuteTriggerOnInvoiceinstead.)🤖 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/service/invoice_test.go` around lines 18 - 37, Add a test case to verify that system-managed (non-mutable) lines are excluded when they transition from non-deleted to deleted: in TestCollectNewlyDeletedStandardLines extend the "after" and "before" fixtures to include a line created by newStandardLineForLineEngineTest with an identifier like "system-deleted" where the before has DeletedAt=nil and the after has DeletedAt set, but with the line marked as non-mutable/system-managed; call collectNewlyDeletedStandardLines and assert that this "system-deleted" line is NOT present in deletedLines (ensuring filtering by mutability), and if you determine filtering happens at a different layer, add the equivalent assertion to the executeTriggerOnInvoice test instead while referencing the OnMutableStandardLinesDeleted hook to validate behavior.openmeter/billing/service/lineengine_test.go (1)
13-57: ⚡ Quick winGreat happy-path coverage; add one failure-path case too.
Consider adding a case where an engine hook returns an error (or engine type is unregistered), and assert the service returns the expected wrapped error. It’ll harden the new dispatch path with minimal effort.
As per coding guidelines "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 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/service/lineengine_test.go` around lines 13 - 57, Add a failure-path test alongside TestOnMutableStandardLinesDeletedGroupsLinesByEngine that verifies OnMutableStandardLinesDeleted returns a wrapped error when an engine fails or is missing: create a recordingLineEngine (or a stub) whose OnMutableStandardLinesDeleted method returns a sentinel error, register only the other engine (or omit registration for the failing engine), call svc.OnMutableStandardLinesDeleted with lines that include the failing engine's LineEngineType, and assert the call returns a non-nil error that wraps the sentinel (use errors.Is or string containment); reference TestOnMutableStandardLinesDeletedGroupsLinesByEngine, Service.RegisterLineEngine, Service.OnMutableStandardLinesDeleted, and recordingLineEngine to locate where to add the new test.
🤖 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.
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 231-233: The no-op OnMutableStandardLinesDeleted in LineEngine
currently ignores deletions which can orphan active usage-based realization runs
started in OnStandardInvoiceCreated; update OnMutableStandardLinesDeleted to
detect if any deleted standard lines belong to usage-based charges, look up
active realization runs in the charge state machine for those line IDs, and
trigger appropriate cleanup (e.g., cancel or void the run and persist state) so
runs do not remain orphaned — use LineEngine's existing helpers for charge
lookup and state transitions (same services used by OnStandardInvoiceCreated) to
locate runs and invoke the cancel/void transition.
In `@openmeter/billing/service/invoice_test.go`:
- Around line 18-37: Add a test case to verify that system-managed (non-mutable)
lines are excluded when they transition from non-deleted to deleted: in
TestCollectNewlyDeletedStandardLines extend the "after" and "before" fixtures to
include a line created by newStandardLineForLineEngineTest with an identifier
like "system-deleted" where the before has DeletedAt=nil and the after has
DeletedAt set, but with the line marked as non-mutable/system-managed; call
collectNewlyDeletedStandardLines and assert that this "system-deleted" line is
NOT present in deletedLines (ensuring filtering by mutability), and if you
determine filtering happens at a different layer, add the equivalent assertion
to the executeTriggerOnInvoice test instead while referencing the
OnMutableStandardLinesDeleted hook to validate behavior.
In `@openmeter/billing/service/lineengine_test.go`:
- Around line 13-57: Add a failure-path test alongside
TestOnMutableStandardLinesDeletedGroupsLinesByEngine that verifies
OnMutableStandardLinesDeleted returns a wrapped error when an engine fails or is
missing: create a recordingLineEngine (or a stub) whose
OnMutableStandardLinesDeleted method returns a sentinel error, register only the
other engine (or omit registration for the failing engine), call
svc.OnMutableStandardLinesDeleted with lines that include the failing engine's
LineEngineType, and assert the call returns a non-nil error that wraps the
sentinel (use errors.Is or string containment); reference
TestOnMutableStandardLinesDeletedGroupsLinesByEngine,
Service.RegisterLineEngine, Service.OnMutableStandardLinesDeleted, and
recordingLineEngine to locate where to add the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7b9eab0-b5fa-4814-aad6-ff44ef2e78a0
📒 Files selected for processing (11)
openmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/service.goopenmeter/billing/service/invoice.goopenmeter/billing/service/invoice_test.goopenmeter/billing/service/lineengine.goopenmeter/billing/service/lineengine_test.goopenmeter/billing/testutils/lineengine.go
turip
force-pushed
the
chore/lineengine-mutable-line-delete-hook
branch
from
66ba7d0 to
36daa87
Compare
tothandras
approved these changes
May 8, 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.
Summary
Summary by CodeRabbit