feat: make sure lines are generated in a continous manner #3087
Conversation
📝 WalkthroughWalkthroughThe changes introduce new methods and fields to support more precise handling and correction of service periods for subscription invoice lines. Logic is added to adjust upcoming invoice line periods based on previous lines when period calculation algorithms change. Related data structures and tests are updated to reflect and verify this behavior. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ 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
added
release-note/feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openmeter/billing/worker/subscription/sync_test.go (1)
4465-4465: Fix typo in test method name.The test method name has a typo - "Syncronize" should be "Synchronize" to be consistent with the rest of the codebase.
-func (s *SubscriptionHandlerTestSuite) TestSyncronizeSubscriptionPeriodAlgorithmChange() { +func (s *SubscriptionHandlerTestSuite) TestSynchronizeSubscriptionPeriodAlgorithmChange() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openmeter/billing/invoicelinesplitgroup.go(1 hunks)openmeter/billing/worker/subscription/phaseiterator.go(3 hunks)openmeter/billing/worker/subscription/sync.go(3 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
openmeter/billing/invoicelinesplitgroup.go (1)
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
openmeter/billing/worker/subscription/sync_test.go (2)
undefined
<retrieved_learning>
Learnt from: chrisgacsal
PR: #2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In productcatalog.UsageBasedRateCard, the BillingCadence field is a non-pointer isodate.Period, while in productcatalog.FlatFeeRateCard, BillingCadence is a pointer type (*isodate.Period). This means MonthPeriod should be used directly for UsageBasedRateCard (not &MonthPeriod).
</retrieved_learning>
<retrieved_learning>
Learnt from: GAlexIHU
PR: #2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like openmeter/entitlement/metered/lateevents_test.go may use variables like meterSlug and namespace without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
</retrieved_learning>
openmeter/billing/worker/subscription/phaseiterator.go (1)
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
openmeter/billing/worker/subscription/sync.go (1)
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
🧬 Code Graph Analysis (2)
openmeter/billing/invoicelinesplitgroup.go (1)
openmeter/billing/invoiceline.go (1)
Period(81-84)
openmeter/billing/worker/subscription/sync.go (4)
pkg/slicesx/groupby.go (1)
UniqueGroupBy(5-18)openmeter/billing/invoicelinesplitgroup.go (4)
LineOrHierarchy(281-285)LineOrHierarchyTypeLine(277-277)LineOrHierarchyTypeHierarchy(278-278)SplitLineHierarchy(193-196)openmeter/billing/invoiceline.go (3)
Line(304-318)SubscriptionManagedLine(63-63)Period(81-84)openmeter/billing/annotations.go (1)
AnnotationSubscriptionSyncIgnore(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Developer environment
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
openmeter/billing/worker/subscription/phaseiterator.go (3)
48-54: LGTM! Well-structured metadata enhancement.The addition of
PhaseKey,PeriodIndex, andItemVersionfields provides better structured access to subscription item metadata that was previously only available in theUniqueIDstring. This supports the period correction logic mentioned in the PR objectives.
382-385: LGTM! Consistent field population.The new fields are properly populated with the appropriate values from the function parameters and iterator context. The implementation maintains consistency with how these values are used in the
UniqueIDconstruction.
476-479: LGTM! Appropriate handling for one-time items.The field population is consistent with the recurring item generation method. Setting
PeriodIndexto 0 for one-time items is appropriate since they don't have multiple billing periods.openmeter/billing/invoicelinesplitgroup.go (1)
337-346: LGTM! Clean abstraction for service period access.The new
ServicePeriod()method provides a unified interface to access service periods from both Line and SplitLineHierarchy types. The implementation correctly handles both types and follows the same pattern as the existingChildUniqueReferenceID()method. This abstraction supports the synchronization logic mentioned in the PR objectives.openmeter/billing/worker/subscription/sync_test.go (2)
4465-4575: Excellent test implementation for period correction feature.This test provides comprehensive coverage for the new
correctPeriodStartForUpcomingLinesfunctionality. It properly:
- Sets up a realistic scenario with a monthly subscription
- Simulates period algorithm changes through manual invoice updates
- Uses the correct annotation (
billing.AnnotationSubscriptionSyncIgnore) to mark manually adjusted lines- Verifies that subsequent periods are correctly aligned to the previous period's end
- Follows the existing test patterns and conventions
The test directly validates the core requirement from the PR objectives: "adjusts the period start for upcoming billing lines to ensure continuity in their generation."
4490-4490: No action needed:isodate.MustParseis correct forUsageBasedRateCard’s BillingCadenceIn the
sync_test.gopatch, thatBillingCadence: isodate.MustParse(s.T(), "P1M")lives inside a&productcatalog.UsageBasedRateCard{…}block. SinceUsageBasedRateCard.BillingCadenceis a non-pointerisodate.Period, usingisodate.MustParsehere matches the field’s type. All otherBillingCadencefields in this file usedatetime.MustParse(with or withoutlo.ToPtr) to satisfy pointer or different period types.openmeter/billing/worker/subscription/sync.go (4)
9-9: LGTM: Import addition is appropriate.The
stringspackage import is correctly added to support string operations in the new period correction logic.
286-297: LGTM: Period correction integration is well-structured.The integration correctly applies period correction before the uniqueness check, which is the right sequence since corrections might affect line uniqueness. The error handling and flow control are appropriate.
342-409: LGTM: Period correction logic is sound with proper validation.The function correctly:
- Skips the first period (index 0) as it doesn't need correction
- Constructs unique IDs for previous periods using a consistent format
- Handles both line and hierarchy types appropriately
- Validates that period starts are aligned before applying corrections
- Maintains consistency across service, billing, and full service periods
The logic aligns well with the PR objective of ensuring continuity in billing periods.
432-443: LGTM: Hierarchy scope checking logic is correct.The function correctly identifies the last line in the hierarchy by matching the service period end and delegates the actual scope checking to
isLineInScopeForPeriodCorrection. The implementation is clean and follows the expected pattern.
turip
force-pushed
the
feat/make-sure-lines-are-continous
branch
from
aaccef9 to
7c6480d
Compare
2 participants
Overview
Add correctPeriodStartForUpcomingLines into billing subscription sync, that corrects the period start for the upcoming lines, it will adjust the period start for the lines.
The adjustment only happens if the line is subscription managed and has billing.subscription.sync.ignore annotation. This esentially allows for reanchoring if the period calculation changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests