Fix charge-backed discount invoice edits#4603
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR switches discount handling to ChangesBilling Discounts Migration
Sequence Diagram(s)sequenceDiagram
participant CustomerChargesHandler
participant billing
participant api
CustomerChargesHandler->>billing: DiscountsFromProductCatalog / UpsertCorrelationIDs
billing-->>CustomerChargesHandler: billing.Discounts
CustomerChargesHandler->>api: map flat-fee or usage-based intent
api-->>CustomerChargesHandler: charge intent with billing discounts
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)
✨ 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 |
Contributor
Greptile SummaryThis PR updates charge-backed invoice discount editing to use the billing discount envelope. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "test(e2e): accept any standard invoice f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 6
🤖 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/charges/flatfee/adapter/intentoverride_test.go`:
- Around line 129-134: The intent override test in requireOverrideMatches
currently verifies only Percentage, so a regression that changes
billing.PercentageDiscount.CorrelationID would still pass. Update the assertions
in intentoverride_test.go to also check the CorrelationID on the seeded
billing.PercentageDiscount, using the existing requireOverrideMatches path and
the test fixture that sets PercentageDiscounts, so both fields are covered.
In `@openmeter/billing/discount.go`:
- Around line 135-152: Discounts.Validate is wrapping the joined validation
errors with models.NewGenericValidationError and an extra “discounts:” prefix
instead of using the repo-standard nillable validation error. Update
Discounts.Validate to keep aggregating Percentage and Usage errors into errs,
then return models.NewNillableGenericValidationError(errors.Join(errs...))
directly so callers like usagebased.IntentMutableFields.Validate don’t
double-prefix the field name and the validation shape stays consistent.
In `@openmeter/billing/httpdriver/invoice.go`:
- Around line 533-538: The rejection log in invoice edit handling is leaking
sensitive tenant data by writing request.Input verbatim. Update the billing
invoice edit warning in the invoice edit path to omit the raw payload and
instead log only identifiers from request.InvoiceID plus invoiceType and a
redacted or summarized payload shape; keep the existing rejection error detail.
Use the logging call in the invoice edit rejection branch as the target for this
change.
In
`@openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go`:
- Around line 8922-8928: The assertTotals helper is using suite-based equality
for alpacadecimal.Decimal values instead of the repo’s preferred decimal
assertion style. Update the checks in CreditThenInvoiceTestSuite.assertTotals to
use require.Equal with s.T() and the InexactFloat64() values, matching the
existing test pattern for decimal comparisons and keeping the same expected
fields (Amount, DiscountsTotal, CreditsTotal, Total).
- Around line 4307-4308: The current test only checks the percentage value and
misses correlation ID propagation. Update the assertions around
editedInvoiceLine.RateCardDiscounts.Percentage to also verify a non-empty
CorrelationID, and extend assertFlatFeeIntent so it validates the propagated
charge intent discount CorrelationID as well. Use the existing edited invoice
line and discount helper paths to ensure the edit flow still preserves/backfills
the discount ID.
In
`@tools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.up.sql`:
- Line 2: The migration for charge_flat_fee_overrides only adds the new
discounts column and leaves existing percentage_discounts data behind, so older
overrides will read as empty through billing.Discounts. Update the migration to
backfill existing rows by transforming percentage_discounts into the new
discounts JSON shape as part of the same change, using the
charge_flat_fee_overrides table and its legacy percentage_discounts column so
previously saved overrides remain visible after the schema switch.
🪄 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: b8f425b1-1938-465a-b24b-169b5e64a1fa
⛔ Files ignored due to path filters (25)
openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicesplitlinegroup_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee/chargeflatfee.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/chargeflatfeeoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride/chargeflatfeeoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/chargeusagebased.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/chargeusagebasedoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride/chargeusagebasedoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride_update.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/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (28)
api/v3/handlers/customers/charges/convert.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/intentoverride.goopenmeter/billing/charges/flatfee/adapter/intentoverride_test.goopenmeter/billing/charges/flatfee/adapter/mapper.goopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/manualedit.goopenmeter/billing/charges/service/pendinglines.goopenmeter/billing/charges/usagebased/adapter/intentoverride_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/discount.goopenmeter/billing/httpdriver/deprecations.goopenmeter/billing/httpdriver/handler.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/service/invoicecalc/discounts.goopenmeter/billing/stdinvoiceedit.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeflatfee.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.down.sqltools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.up.sql
turip
force-pushed
the
feat/billing-discount-charge-edits
branch
from
f312cd5 to
2d347a2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/billinginvoice_discount_edit_test.go`:
- Around line 238-285: The current test in billinginvoice_discount_edit_test
only exercises the update endpoint and still hard-codes the discount
CorrelationId, so it won’t catch regressions in the new discount normalization
flow. Update the test around the invoice edit path to send the percentage
discount without CorrelationId, then verify the returned invoice or a subsequent
GetInvoice shows the edited line persisted with a generated correlation ID and
the expected 50% discount. Keep the assertions focused on the update flow in
UpdateInvoiceWithResponse and the line discount data in the invoice response.
In `@e2e/creditsdisabled/billinginvoice_discount_edit_test.go`:
- Around line 238-285: The credits-disabled invoice edit test still sets a
CorrelationId and only checks for a 200 response, so it can miss regressions in
discount ID backfilling or persistence. Update the test around the invoice line
update in billinginvoice_discount_edit_test.go to omit the CorrelationId when
building the BillingDiscounts/ BillingDiscountPercentage for the edited line,
then assert the response from UpdateInvoiceWithResponse returns the line with a
generated ID and the expected 50% percentage discount. Use the existing invoice
update flow and response assertions near replacementLines and updateResp to
locate the change.
🪄 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: 8a6333f7-8ef4-4f5e-a7d9-6791a80b4b3d
⛔ Files ignored due to path filters (25)
openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoicesplitlinegroup_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee/chargeflatfee.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/chargeflatfeeoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride/chargeflatfeeoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeeoverride_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/chargeusagebased.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/chargeusagebasedoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride/chargeusagebasedoverride.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedoverride_update.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/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (32)
.agents/skills/e2e/SKILL.mdAGENTS.mdapi/v3/handlers/customers/charges/convert.goe2e/billinginvoice_discount_edit_test.goe2e/creditsdisabled/billinginvoice_discount_edit_test.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/intentoverride.goopenmeter/billing/charges/flatfee/adapter/intentoverride_test.goopenmeter/billing/charges/flatfee/adapter/mapper.goopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/manualedit.goopenmeter/billing/charges/service/pendinglines.goopenmeter/billing/charges/usagebased/adapter/intentoverride_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/rating.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/discount.goopenmeter/billing/httpdriver/deprecations.goopenmeter/billing/httpdriver/handler.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/service/invoicecalc/discounts.goopenmeter/billing/stdinvoiceedit.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeflatfee.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.down.sqltools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.up.sql
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/e2e/SKILL.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (28)
- tools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.down.sql
- openmeter/billing/service/invoicecalc/discounts.go
- openmeter/billing/httpdriver/deprecations.go
- openmeter/billing/charges/flatfee/service/create.go
- openmeter/billing/stdinvoiceedit.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeflatfee.go
- openmeter/billing/charges/usagebased/rating.go
- openmeter/billing/charges/usagebased/service/rating/testutils/testutils.go
- tools/migrate/migrations/20260629144435_add_charge_flat_fee_override_discounts.up.sql
- openmeter/ent/schema/chargesusagebased.go
- openmeter/billing/charges/flatfee/adapter/mapper.go
- openmeter/billing/charges/usagebased/adapter/intentoverride_test.go
- openmeter/billing/charges/usagebased/service/create.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go
- openmeter/billing/charges/flatfee/adapter/charge.go
- openmeter/billing/charges/flatfee/service/manualedit.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.go
- openmeter/billing/charges/flatfee/adapter/intentoverride.go
- openmeter/billing/httpdriver/handler.go
- openmeter/ent/schema/chargesflatfee.go
- openmeter/billing/charges/usagebased/charge.go
- openmeter/billing/charges/flatfee/adapter/intentoverride_test.go
- api/v3/handlers/customers/charges/convert.go
- openmeter/billing/discount.go
- openmeter/billing/charges/service/pendinglines.go
- openmeter/billing/httpdriver/invoice.go
- openmeter/billing/charges/flatfee/charge.go
- openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/spec/packages/legacy/src/billing/invoices/invoice.tsp`:
- Around line 659-661: The shared discounts property on the invoice model is
being deprecated even though it is still valid for create and update inputs,
which will incorrectly mark writable SDK fields as deprecated. Remove the
`#deprecated` annotation from the discounts member in the invoice.tsp definition
and keep the V1 read-only limitation in the description, or move the deprecation
to a response-only projection if the intent is read-only. Use the discounts
property and the invoice shape definition to locate the change.
🪄 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: 62104875-aa67-44c1-85a9-3e82b1e01498
⛔ Files ignored due to path filters (4)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (4)
api/api.gen.goapi/spec/packages/legacy/src/billing/invoices/invoice.tspe2e/billinginvoice_discount_edit_test.goe2e/creditsdisabled/billinginvoice_discount_edit_test.go
💤 Files with no reviewable changes (2)
- e2e/billinginvoice_discount_edit_test.go
- e2e/creditsdisabled/billinginvoice_discount_edit_test.go
tothandras
previously approved these changes
Jun 29, 2026
Contributor
|
approved |
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 fixes discount edits for charge-backed invoice lines by making charge discount persistence use the billing discount envelope consistently.
billing.Discountswrappers that preserve discount correlation IDs for percentage and usage discounts.discountsfield while leaving the previous percentage-only field deprecated.Root Cause
Charge-backed invoice-line edits could introduce rate-card discounts without the billing correlation ID shape expected by detailed-line calculation. For flat-fee charge-managed lines, the edit path also needed the charge override storage to match the invoice-line discount envelope so recalculation could happen through the charge engine instead of trying to update managed invoice lines directly.
Validation
go vet -tags=dynamic ./...env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic -count=1 ./openmeter/billing/worker/subscriptionsync/service ./openmeter/billing/charges/flatfee/... ./openmeter/billing/charges/usagebased/... ./api/v3/handlers/customers/chargesenv POSTGRES_HOST=127.0.0.1 go test -tags=dynamic -count=1 ./openmeter/billing/httpdrivermake test-nocacheSummary by CodeRabbit