feat: use UBP flat fee lines #2898
Conversation
📝 Walkthrough""" WalkthroughThis change introduces a feature flag to enable a new usage-based representation for flat fee billing lines in the subscription billing workflow. Supporting helpers are added to manage both legacy and new line types. The merging and patching logic is refactored for type safety and error handling, and comprehensive tests ensure compatibility during the transition. Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
refactor/test-cleanup
branch
from
d295485 to
9d7ba24
Compare
turip
force-pushed
the
feat/use-ubp-lines-subs-sync
branch
2 times, most recently
from
04d1fef to
430e0b2
Compare
turip
added
release-note/feature
turip
force-pushed
the
feat/use-ubp-lines-subs-sync
branch
from
430e0b2 to
459091f
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openmeter/billing/worker/subscription/feehelper.go(1 hunks)openmeter/billing/worker/subscription/sync.go(5 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
openmeter/billing/worker/subscription/sync_test.go (6)
pkg/clock/clock.go (2)
FreezeTime(33-36)Now(14-21)openmeter/productcatalog/phase.go (1)
PhaseMeta(18-33)openmeter/productcatalog/ratecard.go (2)
UsageBasedRateCard(354-360)RateCardMeta(52-83)openmeter/billing/invoiceline.go (5)
InvoiceLineTypeFee(31-31)Line(282-296)InvoiceLineType(27-27)InvoiceLineType(36-41)InvoiceLineTypeUsageBased(33-33)pkg/isodate/date.go (1)
MustParse(151-158)openmeter/billing/worker/subscription/sync.go (1)
Handler(67-74)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/billing/worker/subscription/feehelper.go (2)
1-11: LGTM! Clean package structure and imports.The package declaration and imports are well-structured and include all necessary dependencies for decimal handling and billing operations.
12-13: Good documentation of temporary nature.The comment clearly explains that these are temporary helpers for the flat fee transition and should be removed after migration. This helps future developers understand the lifecycle of this code.
openmeter/billing/worker/subscription/sync_test.go (1)
3456-3539: Excellent backward compatibility test implementation!This test thoroughly validates the
UseUsageBasedFlatFeeLinesfeature flag behavior and ensures smooth transition from legacy flat fee lines to usage-based flat fee lines. The test design is particularly well-thought-out:
- Proper flag lifecycle management: The test correctly toggles the feature flag and uses a defer function to ensure cleanup
- Backward compatibility verification: It confirms that existing flat fee lines remain unchanged when the flag is enabled
- Forward compatibility testing: New lines created after flag enablement correctly use the usage-based approach
- Mixed state validation: The final assertion verifies that both line types can coexist, which is crucial for the migration strategy described in the PR
The test directly supports the PR objective of enabling usage-based flat fees as an interim step without disrupting existing data, and it properly validates that "existing flat fee lines created prior to this update remain unchanged."
openmeter/billing/worker/subscription/sync.go (4)
35-35: LGTM!The feature flag addition follows the existing pattern and is appropriately named.
625-636: Proper error handling implementation!The error handling correctly propagates errors from
mergeChangesFromLinewith appropriate context.
756-802:Details
✅ Verification successful
Good refactoring with improved error handling!
The refactoring improves type safety by returning a structured result and adds proper validation for flat fee line merging. The error messages are descriptive and will help with debugging.
🏁 Script executed:
#!/bin/bash # Description: Verify the helper functions exist and are properly implemented # Search for the helper function definitions ast-grep --pattern 'func isFlatFee($_) $_' ast-grep --pattern 'func getFlatFeePerUnitAmount($_) $_' ast-grep --pattern 'func setFlatFeePerUnitAmount($_, $_) $_' # Check the feehelper.go file mentioned in the summary fd -t f "feehelper.go" | xargs cat 2>/dev/null | head -50Length of output: 5183
Approve mergeChangesFromLine refactoring
Verified that all helper functions referenced in
sync.goare present and correctly implemented inopenmeter/billing/worker/subscription/feehelper.go:
isFlatFee(line *billing.Line) boolgetFlatFeePerUnitAmount(line *billing.Line) (alpacadecimal.Decimal, error)setFlatFeePerUnitAmount(line *billing.Line, perUnitAmount alpacadecimal.Decimal) errorThese cover both
InvoiceLineTypeFeeandInvoiceLineTypeUsageBasedcases with appropriate error handling. No further changes required.
519-533:Details
❓ Verification inconclusive
Verify nil FeatureKey handling for flat fee lines.
When creating usage-based lines for flat fees, the code uses
lo.FromPtr(item.SubscriptionItem.RateCard.AsMeta().FeatureKey). IfFeatureKeyis nil (which might be valid for flat fees), this will result in an empty string. Please verify that this is the intended behavior.Additionally, there's a typo in the method name: "lineFromSubscritionRateCard" should be "lineFromSubscriptionRateCard".
🏁 Script executed:
#!/bin/bash # Description: Check if FeatureKey can be nil for flat fee rate cards # Search for flat fee rate card definitions and their FeatureKey usage ast-grep --pattern 'type $_ struct { $$$ FeatureKey $_ $$$ }' # Search for flat fee creation patterns to understand FeatureKey requirements rg -A 5 -B 5 "FlatPrice.*FeatureKey"Length of output: 67916
Verify nil FeatureKey handling under
UseUsageBasedFlatFeeLines.
- Confirm that
item.SubscriptionItem.RateCard.AsMeta().FeatureKeyis always non-nil whenUseUsageBasedFlatFeeLinesis enabled. If it can benil, you’ll end up with an empty string in the invoice line’sFeatureKey. Consider falling back toRateCardMeta.Keyor explicitly erroring/handling the nil case.- Rename the helper from
lineFromSubscritionRateCard→lineFromSubscriptionRateCardto fix the typo.
turip
force-pushed
the
feat/use-ubp-lines-subs-sync
branch
from
459091f to
38c8878
Compare
2 participants
Overview
This patch allows for synchronizing the flat fees as "usage-based" flat fees. The feature is not exposed via configuration, as this is just an interim step towards using the "usage-based" flat fees everywhere.
Previously created flat fee lines are not changed: they will be migrated using an SQL migration to the new format in a separate PR, once we have fully validated this change.
Summary by CodeRabbit
New Features
Bug Fixes
Tests