fix: credit-then-invoice discount sync#4376
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughAdds UpdateSubscriptionItemID across adapters and services, makes subscription_item_id mutable in schemas, introduces DetailedLines credit helpers, integrates credit application into flat-fee and usage-based flows, implements subscription-sync repair for subscription-item reconciliation, and expands related tests. ChangesCharge subscription item update and credit handling flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
turip
added
kind/bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openmeter/billing/invoicedetailedline_test.go (1)
207-211: 💤 Low valueConsider using
InexactFloat64()for decimal assertions per coding guidelines.The helper works fine and provides a good error message, but the coding guidelines suggest using
require.Equal(t, expectedFloat64, actual.InexactFloat64())for decimal comparisons. Since you're dealing with simple integer-based test values, this would work well:require.Equal(t, expected.InexactFloat64(), actual.InexactFloat64())That said, your current approach is readable and the error message is clear, so this is really just a style alignment thing.
🤖 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/invoicedetailedline_test.go` around lines 207 - 211, Replace the custom equality check in requireDecimalEqual with an assertion that compares float64s using the Decimal.InexactFloat64() method; specifically, inside requireDecimalEqual (and any callers), call require.Equal(t, expected.InexactFloat64(), actual.InexactFloat64()) instead of using expected.Equal(actual) so the test follows the coding guideline for decimal assertions.openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go (1)
333-367: 💤 Low valueConsider adding a brief comment explaining the two-clone pattern.
The logic here is sound - you need
ratingLinewith nulled split-line fields for rating, andratedLineto preserve the original structure. But it took me a moment to understand why there are two clones. A one-liner above line 341 could help future readers:// ratingLine is a separate clone with split-line metadata cleared for rating; // generated detailed lines are merged back into ratedLine which keeps original structure.Just a thought - the existing comment on lines 347-350 does explain the why for nulling the fields, but doesn't explain why a second clone is needed.
🤖 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/flatfee/service/realizations/credittheninvoice.go` around lines 333 - 367, Add a one-line comment in rateFlatFeeLine explaining the two-clone pattern: that we create ratedLine as the preserved copy keeping original split-line metadata and create a separate ratingLine with SplitLineGroupID/SplitLineHierarchy nulled so rating (GenerateDetailedLines) runs on a sanitized version and its generated detailed lines are then merged back into ratedLine; place this comment near where ratedLine and ratingLine are cloned/initialized to clarify why both clones are needed.openmeter/billing/charges/flatfee/adapter/charge.go (1)
82-85: ⚡ Quick winAdd the same managed-model validation guard used by other mutating adapter methods.
Small consistency win:
UpdateChargeandDeleteChargevalidatecharge.ManagedModelbefore DB writes, but this new mutator currently skips that fast-fail check.Suggested patch
func (a *adapter) UpdateSubscriptionItemID(ctx context.Context, charge flatfee.Charge, newSubscriptionItemID string) (flatfee.Charge, error) { + if err := charge.ManagedModel.Validate(); err != nil { + return flatfee.Charge{}, err + } + if err := charge.Validate(); err != nil { return flatfee.Charge{}, err }🤖 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/flatfee/adapter/charge.go` around lines 82 - 85, Add the same managed-model validation guard to UpdateSubscriptionItemID: before performing DB writes, check charge.ManagedModel (like in UpdateCharge and DeleteCharge) and return an error if it’s invalid/empty to fast-fail; update the adapter.UpdateSubscriptionItemID function to perform this validation immediately after charge.Validate() and before any DB operations so it matches the other mutators' behavior.
🤖 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/worker/subscriptionsync/service/repair.go`:
- Around line 38-45: Before calling lo.SliceToMap on target.Items, detect
duplicate UniqueID values and fail/handle explicitly: compute the list of
billable UniqueIDs (filtering via item.IsBillable()), use lo.Map + lo.Uniq (or
build a count map) to compare counts and if counts differ return an error/log
indicating duplicate UniqueID(s) so you don't silently overwrite; then only call
lo.SliceToMap to build targetByUniqueID once uniqueness is confirmed. Reference:
target.Items, targetstate.StateItem, UniqueID, lo.SliceToMap, and
targetByUniqueID.
---
Nitpick comments:
In `@openmeter/billing/charges/flatfee/adapter/charge.go`:
- Around line 82-85: Add the same managed-model validation guard to
UpdateSubscriptionItemID: before performing DB writes, check charge.ManagedModel
(like in UpdateCharge and DeleteCharge) and return an error if it’s
invalid/empty to fast-fail; update the adapter.UpdateSubscriptionItemID function
to perform this validation immediately after charge.Validate() and before any DB
operations so it matches the other mutators' behavior.
In `@openmeter/billing/charges/flatfee/service/realizations/credittheninvoice.go`:
- Around line 333-367: Add a one-line comment in rateFlatFeeLine explaining the
two-clone pattern: that we create ratedLine as the preserved copy keeping
original split-line metadata and create a separate ratingLine with
SplitLineGroupID/SplitLineHierarchy nulled so rating (GenerateDetailedLines)
runs on a sanitized version and its generated detailed lines are then merged
back into ratedLine; place this comment near where ratedLine and ratingLine are
cloned/initialized to clarify why both clones are needed.
In `@openmeter/billing/invoicedetailedline_test.go`:
- Around line 207-211: Replace the custom equality check in requireDecimalEqual
with an assertion that compares float64s using the Decimal.InexactFloat64()
method; specifically, inside requireDecimalEqual (and any callers), call
require.Equal(t, expected.InexactFloat64(), actual.InexactFloat64()) instead of
using expected.Equal(actual) so the test follows the coding guideline for
decimal assertions.
🪄 Autofix (Beta)
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
Run ID: f33236c6-e217-40b2-a228-ba99b429db22
⛔ Files ignored due to path filters (7)
openmeter/ent/db/chargecreditpurchase_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_update.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**
📒 Files selected for processing (24)
openmeter/billing/charges/flatfee/adapter.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/service.goopenmeter/billing/charges/flatfee/service/linemapper.goopenmeter/billing/charges/flatfee/service/realizations/credittheninvoice.goopenmeter/billing/charges/flatfee/service/subscription.goopenmeter/billing/charges/models/chargemeta/mixin.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/subscription.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/linemapper.goopenmeter/billing/charges/usagebased/service/subscription.goopenmeter/billing/invoicedetailedline.goopenmeter/billing/invoicedetailedline_test.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/repair.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ent/schema/chargesusagebased.goopenmeter/server/server_test.go
💤 Files with no reviewable changes (2)
- openmeter/ent/schema/chargescreditpurchase.go
- openmeter/ent/schema/chargesflatfee.go
turip
changed the title
turip
force-pushed
the
feat/fix-discount-sync
branch
from
8175b66 to
eb10643
Compare
tothandras
approved these changes
May 18, 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
This PR tightens credit-then-invoice reconciliation for subscription-backed charges. From the billing domain perspective it covers three related consistency issues:
The PR also extends credit-then-invoice subscription sync coverage for usage-based draft and issued invoice edit flows, including ledger balance expectations, charge state expectations, immutable invoice warning behavior, and promotional credit settlement.
Validation
Summary by CodeRabbit
New Features
Improvements
Reliability
Tests