feat(billing): add subscription end proration mode - #4553
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new ChangesSubscription end proration mode feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler
participant ProfileService
participant Adapter
participant Database
participant SyncService
participant TargetStateBuilder
participant ProrationLogic
Client->>HTTPHandler: Create/update workflow with subscription_end_proration_mode
HTTPHandler->>ProfileService: Normalize and validate profile
ProfileService->>Adapter: Persist workflow config
Adapter->>Database: Save subscription_end_proration_mode
SyncService->>SyncService: Fetch customer profile with mode
SyncService->>TargetStateBuilder: buildSyncPlanInput with mode
TargetStateBuilder->>TargetStateBuilder: Assign mode to StateItems
ProrationLogic->>ProrationLogic: shouldProrate() checks mode for conditional behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
GAlexIHU
previously approved these changes
Jun 22, 2026
turip
marked this pull request as ready for review
June 22, 2026 16:48
Contributor
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the migration correctly backfills existing rows with the legacy mode and all three API paths (create, update with field, update without field) produce the correct InvoicingConfig value before validation. The migration two-step (ADD COLUMN with bill_full_period default, then ALTER DEFAULT to bill_actual_period) is correct and non-destructive. CREATE defaults are applied at the HTTP conversion layer; UPDATE backfills from the stored profile in the service before validation. The shouldProrate() switch is straightforward, tested at the unit level, and the integration test updates correctly reflect the new prorated-by-default behavior for new profiles. No files require special attention. The migration, Ent schema, service backfill, and HTTP conversion layers are all consistent with each other. Important Files Changed
|
GAlexIHU
changed the title
GAlexIHU
added
the
release-note/ignore
GAlexIHU
previously approved these changes
Jun 23, 2026
GAlexIHU
force-pushed
the
feat/cancel-proration-service-period
branch
from
June 23, 2026 12:57
e8da2a2 to
fccf8e2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/profile.go (1)
111-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregate validation issues in the touched
Validate()methods.Nice addition overall, but these methods still short-circuit on first error. Please collect all failures into
var errs []errorand returnmodels.NewNillableGenericValidationError(errors.Join(errs...))so callers get the full validation set in one response.Suggested pattern
func (i UpdateProfileInput) Validate() error { - if i.ID == "" { - return errors.New("id is required") - } - if i.Namespace == "" { - return errors.New("namespace is required") - } - if i.AppReferences != nil { - return errors.New("apps cannot be updated") - } - return nil + var errs []error + if i.ID == "" { + errs = append(errs, errors.New("id is required")) + } + if i.Namespace == "" { + errs = append(errs, errors.New("namespace is required")) + } + if i.AppReferences != nil { + errs = append(errs, errors.New("apps cannot be updated")) + } + return models.NewNillableGenericValidationError(errors.Join(errs...)) }As per coding guidelines, "For Go
Validate() errormethods, prefer collecting all validation issues intovar errs []errorand returningmodels.NewNillableGenericValidationError(errors.Join(errs...))instead of returning on the first invalid field".Also applies to: 132-138, 525-533
🤖 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/profile.go` around lines 111 - 116, The Validate() method currently returns immediately on the first validation error, preventing callers from seeing all validation issues at once. Refactor the method to collect all validation failures into a var errs []error slice, appending errors from each validation check (such as SubscriptionEndProrationMode.Validate()) instead of returning early, then return models.NewNillableGenericValidationError(errors.Join(errs...)) at the end to provide the complete set of validation issues. Apply this same pattern to all other Validate() methods mentioned in the comment.Source: Coding guidelines
🤖 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/profile.go`:
- Around line 111-116: The Validate() method currently returns immediately on
the first validation error, preventing callers from seeing all validation issues
at once. Refactor the method to collect all validation failures into a var errs
[]error slice, appending errors from each validation check (such as
SubscriptionEndProrationMode.Validate()) instead of returning early, then return
models.NewNillableGenericValidationError(errors.Join(errs...)) at the end to
provide the complete set of validation issues. Apply this same pattern to all
other Validate() methods mentioned in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 367acfa0-64c3-4691-9444-b39cf0787953
⛔ Files ignored due to path filters (18)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/__init__.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_enums.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yamlopenmeter/ent/db/billingworkflowconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billingworkflowconfig/billingworkflowconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billingworkflowconfig/where.gois excluded by!**/ent/db/**openmeter/ent/db/billingworkflowconfig_create.gois excluded by!**/ent/db/**openmeter/ent/db/billingworkflowconfig_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/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (23)
api/api.gen.goapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/billing/profile.tspapi/spec/packages/legacy/src/billing/profile.tspapi/v3/api.gen.goapi/v3/handlers/billingprofiles/convert.goopenmeter/billing/adapter/profile.goopenmeter/billing/defaults.goopenmeter/billing/httpdriver/profile.goopenmeter/billing/profile.goopenmeter/billing/service/profile.goopenmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem_test.goopenmeter/ent/schema/billing.goopenmeter/subscription/billing.gotools/migrate/migrations/20260623101013_add_billing_profile_subscription_end_proration_mode.down.sqltools/migrate/migrations/20260623101013_add_billing_profile_subscription_end_proration_mode.up.sql
✅ Files skipped from review due to trivial changes (5)
- tools/migrate/migrations/20260623101013_add_billing_profile_subscription_end_proration_mode.down.sql
- openmeter/subscription/billing.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem_test.go
- api/v3/api.gen.go
- api/spec/packages/aip-client-javascript/src/index.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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/profile.go (1)
360-365: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t overwrite explicit proration-mode updates during merge.
This merge behavior keeps the mode from
pand can prevent an update payload from ever changingSubscriptionEndProrationMode, which breaks the new configurability.💡 Proposed fix
- merged.WorkflowConfig.Invoicing.SubscriptionEndProrationMode = p.WorkflowConfig.Invoicing.SubscriptionEndProrationMode + if o.WorkflowConfig.Invoicing.SubscriptionEndProrationMode == "" { + merged.WorkflowConfig.Invoicing.SubscriptionEndProrationMode = p.WorkflowConfig.Invoicing.SubscriptionEndProrationMode + }🤖 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/profile.go` around lines 360 - 365, The SubscriptionEndProrationMode field assignment does not follow the same override pattern as the other Invoicing fields (AutoAdvance, DraftPeriod, DueAfter, ProgressiveBilling). Currently it directly uses the value from p.WorkflowConfig.Invoicing.SubscriptionEndProrationMode without checking for an explicit override in o.Invoicing.SubscriptionEndProrationMode first. Change the SubscriptionEndProrationMode assignment to use the same lo.FromPtrOr pattern used for the other fields to properly respect explicit updates while falling back to the default WorkflowConfig value when no override is provided.
🧹 Nitpick comments (2)
openmeter/billing/profile.go (1)
96-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregate validation errors in
Validate()methods instead of failing fast.Both updated
Validate()paths still return on first invalid field. Please align with the repo pattern so callers receive the full validation set in one response.💡 Pattern to apply
func (c *InvoicingConfig) Validate() error { + var errs []error + - if c.DraftPeriod.IsNegative() && c.AutoAdvance { - return fmt.Errorf("draft period must be greater or equal to 0") - } + if c.DraftPeriod.IsNegative() && c.AutoAdvance { + errs = append(errs, fmt.Errorf("draft period must be greater or equal to 0")) + } - if c.DueAfter.IsNegative() { - return fmt.Errorf("due after must be greater or equal to 0") - } + if c.DueAfter.IsNegative() { + errs = append(errs, fmt.Errorf("due after must be greater or equal to 0")) + } if c.DefaultTaxConfig != nil { if err := c.DefaultTaxConfig.Validate(); err != nil { - return fmt.Errorf("invalid tax behavior: %w", err) + errs = append(errs, fmt.Errorf("invalid tax behavior: %w", err)) } } if c.SubscriptionEndProrationMode == "" { - return errors.New("subscription end proration mode is required") + errs = append(errs, errors.New("subscription end proration mode is required")) + } else if err := c.SubscriptionEndProrationMode.Validate(); err != nil { + errs = append(errs, fmt.Errorf("invalid subscription end proration mode: %w", err)) } - - if err := c.SubscriptionEndProrationMode.Validate(); err != nil { - return fmt.Errorf("invalid subscription end proration mode: %w", err) - } - - return nil + return models.NewNillableGenericValidationError(errors.Join(errs...)) }As per coding guidelines, “For Go
Validate() errormethods, prefer collecting all validation issues intovar errs []errorand returningmodels.NewNillableGenericValidationError(errors.Join(errs...))instead of returning on the first invalid field`.Also applies to: 516-530
🤖 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/profile.go` around lines 96 - 120, The Validate() method on the InvoicingConfig type currently returns immediately upon encountering the first validation error instead of collecting all validation issues. Refactor the method by declaring a var errs []error slice at the start, then replace all return statements (for DraftPeriod, AutoAdvance, DueAfter, DefaultTaxConfig, and SubscriptionEndProrationMode validations) with errs = append(errs, ...) to collect the errors, and finally return models.NewNillableGenericValidationError(errors.Join(errs...)) at the end of the method to provide callers with the complete set of validation issues in a single response.Source: Coding guidelines
openmeter/billing/profile_test.go (1)
13-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice enum coverage—please add one focused merge/update behavior test too.
This table verifies mode validation well, but it doesn’t assert the changed merge/update semantics for this setting. A small case for “explicit update value overrides existing mode” (and empty update preserves existing) would guard the new plumbing end-to-end.
As per path instructions, “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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/profile_test.go` around lines 13 - 59, The TestInvoicingConfigValidateSubscriptionEndProrationMode test only validates enum values but doesn't cover the merge/update behavior for SubscriptionEndProrationMode. Add new test cases to verify that when updating a configuration with an explicit mode value, it properly overrides an existing mode, and when the update contains an empty mode value, the existing mode is preserved. These tests should demonstrate the end-to-end update plumbing works correctly for this setting by creating a base configuration with a mode, applying an update with either a new explicit mode or an empty mode, and asserting the resulting configuration reflects the expected merge behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@openmeter/billing/profile.go`:
- Around line 360-365: The SubscriptionEndProrationMode field assignment does
not follow the same override pattern as the other Invoicing fields (AutoAdvance,
DraftPeriod, DueAfter, ProgressiveBilling). Currently it directly uses the value
from p.WorkflowConfig.Invoicing.SubscriptionEndProrationMode without checking
for an explicit override in o.Invoicing.SubscriptionEndProrationMode first.
Change the SubscriptionEndProrationMode assignment to use the same lo.FromPtrOr
pattern used for the other fields to properly respect explicit updates while
falling back to the default WorkflowConfig value when no override is provided.
---
Nitpick comments:
In `@openmeter/billing/profile_test.go`:
- Around line 13-59: The TestInvoicingConfigValidateSubscriptionEndProrationMode
test only validates enum values but doesn't cover the merge/update behavior for
SubscriptionEndProrationMode. Add new test cases to verify that when updating a
configuration with an explicit mode value, it properly overrides an existing
mode, and when the update contains an empty mode value, the existing mode is
preserved. These tests should demonstrate the end-to-end update plumbing works
correctly for this setting by creating a base configuration with a mode,
applying an update with either a new explicit mode or an empty mode, and
asserting the resulting configuration reflects the expected merge behavior.
In `@openmeter/billing/profile.go`:
- Around line 96-120: The Validate() method on the InvoicingConfig type
currently returns immediately upon encountering the first validation error
instead of collecting all validation issues. Refactor the method by declaring a
var errs []error slice at the start, then replace all return statements (for
DraftPeriod, AutoAdvance, DueAfter, DefaultTaxConfig, and
SubscriptionEndProrationMode validations) with errs = append(errs, ...) to
collect the errors, and finally return
models.NewNillableGenericValidationError(errors.Join(errs...)) at the end of the
method to provide callers with the complete set of validation issues in a single
response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a17e306-e4df-4579-9028-46cccbf6f216
📒 Files selected for processing (16)
api/v3/handlers/billingprofiles/convert.goopenmeter/billing/adapter/profile.goopenmeter/billing/httpdriver/profile.goopenmeter/billing/profile.goopenmeter/billing/profile_test.goopenmeter/billing/service/profile.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem_test.gotest/billing/invoice_test.gotest/billing/profile.gotest/billing/profile_test.gotest/subscription/framework_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/service/profile.go
- openmeter/billing/adapter/profile.go
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go
- api/v3/handlers/billingprofiles/convert.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem_test.go
- openmeter/billing/worker/subscriptionsync/service/sync.go
- openmeter/billing/httpdriver/profile.go
- openmeter/billing/worker/subscriptionsync/service/sync_test.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go
turip
enabled auto-merge (squash)
June 23, 2026 13:31
Contributor
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
GAlexIHU
self-requested a review
June 23, 2026 14:41
GAlexIHU
approved these changes
Jun 23, 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
Adds a billing-profile setting for subscription-end proration behavior:
subscriptionEndProrationMode/subscription_end_proration_modebill_full_periodpreserves the legacy behavior for existing profilesbill_actual_periodis the default for new profilesThe setting controls how subscription-ending shortened service periods are billed for flat-fee subscription sync lines when product-catalog prorating is enabled.
Why
We had a bug/behavior where
ServicePeriod < FullServicePeriodlines did not go through proration when they terminated with the subscription. That made terminal shortened periods bill the full period amount in some cancel/change flows.This PR keeps the old behavior available for existing customers through a persisted billing-profile mode, while letting new profiles use the corrected behavior by default.
What changed
SubscriptionEndProrationModeto billing workflow invoicing config.subscription_end_proration_modeonbilling_workflow_configs.bill_full_period, then switches the column default tobill_actual_period.Validation
make gen-apimake generateatlas migrate --env local hashgo vet -tags=dynamic ./openmeter/billing ./openmeter/billing/httpdriver ./api/v3/handlers/billingprofiles ./openmeter/billing/worker/subscriptionsync/service ./openmeter/billing/worker/subscriptionsync/service/targetstatego test -tags=dynamic ./openmeter/billing ./openmeter/billing/worker/subscriptionsync/service/targetstate ./api/v3/handlers/billingprofiles ./openmeter/billing/httpdrivergo test -tags=dynamic ./openmeter/billing/worker/subscriptionsync/service -run 'TestSubscriptionHandlerScenarios/TestInAdvanceFlatFeeCancelAtFirstBillingBoundary'go test -tags=dynamic ./openmeter/billing/worker/subscriptionsync/service -run 'TestSubscriptionHandlerScenarios/(TestInAdvanceFlatFeeCancelAtFirstBillingBoundaryKeepsInitialProration|TestInArrearsProratingGathering)'git diff --checkSummary by CodeRabbit