refactor: add helpers for flat fee line creation #2883
Conversation
📝 WalkthroughWalkthroughThis change adds a new input struct and constructor function for creating flat fee invoice lines in a centralized manner. It refactors all existing manual constructions of flat fee invoice lines in production and test code to use this new constructor, improving consistency and reducing explicit field assignments across the codebase. Changes
Possibly related PRs
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ 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/flat-fee-line-creation
branch
from
0f501a8 to
e83bd74
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: 0
🔭 Outside diff range comments (1)
test/billing/invoice_test.go (1)
645-667: 🛠️ Refactor suggestionComplete the refactoring for consistency.
This section still uses manual struct construction while other parts of the file have been refactored to use the new
billing.NewLine(billing.NewFlatFeeLineInput{...})pattern. For consistency with the PR objectives, consider refactoring this to use the new constructor as well.Apply this diff to maintain consistency:
- { - LineBase: billing.LineBase{ - Namespace: namespace, - Period: billing.Period{Start: periodStart, End: periodEnd}, - - InvoiceAt: line1IssueAt, - - Type: billing.InvoiceLineTypeFee, - ManagedBy: billing.ManuallyManagedLine, - - Name: "Test item1", - - Metadata: map[string]string{ - "key": "value", - }, - }, - FlatFee: &billing.FlatFeeLine{ - PerUnitAmount: alpacadecimal.NewFromFloat(100), - Quantity: alpacadecimal.NewFromFloat(1), - Category: billing.FlatFeeCategoryRegular, - PaymentTerm: productcatalog.InAdvancePaymentTerm, - }, - }, + billing.NewLine(billing.NewFlatFeeLineInput{ + Namespace: namespace, + Period: billing.Period{Start: periodStart, End: periodEnd}, + InvoiceAt: line1IssueAt, + ManagedBy: billing.ManuallyManagedLine, + Name: "Test item1", + Metadata: map[string]string{ + "key": "value", + }, + PerUnitAmount: alpacadecimal.NewFromFloat(100), + Quantity: alpacadecimal.NewFromFloat(1), + PaymentTerm: productcatalog.InAdvancePaymentTerm, + }),
🧹 Nitpick comments (2)
test/billing/collection_test.go (1)
359-372: Good usage with minor consistency suggestion.This usage properly includes context-specific fields like
Namespace,Currency, andInvoiceIDwhich are appropriate when adding a line to an existing invoice. The new constructor pattern is being used correctly.Consider adding the
ManagedByfield for consistency with other usages, unless the constructor handles this default appropriately.openmeter/billing/invoiceline.go (1)
591-599: Consider improving the generic type switching approach.While the generic function works, the current implementation using
any()type conversions and runtime type switching feels awkward and loses some type safety benefits of generics.Consider these alternative approaches:
Option 1: Interface-based approach
+type LineInput interface { + NewFlatFeeLineInput +} + -func NewLine[T NewFlatFeeLineInput](input T) *Line { - switch any(input).(type) { - case NewFlatFeeLineInput: - v, _ := any(input).(NewFlatFeeLineInput) - return newFlatFeeLine(v) - } - - return nil -} +func NewLine[T LineInput](input T) *Line { + switch v := any(input).(type) { + case NewFlatFeeLineInput: + return newFlatFeeLine(v) + default: + return nil + } +}Option 2: Direct function approach (if only one type is expected)
-func NewLine[T NewFlatFeeLineInput](input T) *Line { - switch any(input).(type) { - case NewFlatFeeLineInput: - v, _ := any(input).(NewFlatFeeLineInput) - return newFlatFeeLine(v) - } - - return nil -} +func NewFlatFeeLine(input NewFlatFeeLineInput) *Line { + return newFlatFeeLine(input) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openmeter/billing/invoiceline.go(1 hunks)openmeter/notification/internal/rule.go(1 hunks)test/app/custominvoicing/invocing_test.go(2 hunks)test/billing/collection_test.go(2 hunks)test/billing/invoice_test.go(3 hunks)test/billing/tax_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/billing/tax_test.go (2)
openmeter/billing/invoiceline.go (4)
NewLine(591-599)NewFlatFeeLineInput(563-589)Period(84-87)ManuallyManagedLine(70-70)openmeter/productcatalog/price.go (1)
InAdvancePaymentTerm(20-20)
openmeter/notification/internal/rule.go (2)
openmeter/billing/invoiceline.go (4)
NewLine(591-599)NewFlatFeeLineInput(563-589)ManuallyManagedLine(70-70)Period(84-87)openmeter/productcatalog/price.go (1)
InAdvancePaymentTerm(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
test/billing/tax_test.go (1)
289-303: LGTM! Clean refactoring to use centralized constructor pattern.The refactoring successfully replaces manual
billing.Linestruct construction with the newbilling.NewLineconstructor usingNewFlatFeeLineInput. This change:
- Encapsulates flat fee line creation logic in a single place
- Reduces boilerplate code and improves maintainability
- Maintains all the same field values, preserving test behavior
- Follows a consistent pattern across the codebase
All necessary fields are properly set through the input struct, and the semantic content of the test remains unchanged.
test/app/custominvoicing/invocing_test.go (1)
185-196: LGTM! Consistent application of the new constructor pattern.Both test methods have been properly refactored to use the new
billing.NewLine(billing.NewFlatFeeLineInput{...})constructor instead of manual struct construction. The changes:
- Apply the same beneficial refactoring pattern consistently across test methods
- Maintain identical field values, preserving all test logic and assertions
- Improve code readability and reduce boilerplate
- Align with the broader codebase refactoring mentioned in the PR summary
The semantic content of both
TestInvoicingFlowHooksEnabledandTestInvoicingFlowPaymentStatusOnlyremains unchanged while benefiting from the improved code organization.Also applies to: 354-365
test/billing/invoice_test.go (3)
141-157: LGTM! Proper usage of the new constructor pattern.This is a good example of the refactored flat fee line creation using
billing.NewLinewithbilling.NewFlatFeeLineInput. All necessary fields are properly provided and the change improves consistency.
172-183: Verify namespace field consistency.This usage of the new constructor is missing the
Namespacefield that was included in the previous usage (lines 141-157). Please verify:
- Does the constructor handle namespace defaults appropriately in this context?
- Should the namespace field be explicitly provided for consistency?
This inconsistency could lead to unexpected behavior if the namespace isn't properly set.
467-498: LGTM! Consistent usage of the new constructor.Both instances in
TestCreateInvoiceproperly use the new constructor pattern with all necessary fields including the namespace. The implementation is consistent and follows the refactoring approach correctly.test/billing/collection_test.go (1)
253-261: Verify field completeness and constructor defaults.This usage of the new constructor appears to be missing some fields (like
NamespaceandManagedBy) that were explicitly provided in other test files. Please verify:
- Does the constructor properly handle defaults for these fields in this test context?
- Are the defaults appropriate for this test scenario?
- Should these fields be explicitly provided for consistency?
openmeter/notification/internal/rule.go (1)
188-206: LGTM! Clean refactoring to use the new factory constructor.The refactoring successfully replaces manual struct construction with the new
billing.NewLinefactory function. This approach:
- Consolidates flat fee line creation logic in one place
- Automatically handles the
TypeandStatusfield defaults- Improves code readability and maintainability
- Reduces boilerplate code
All required fields are properly mapped from the previous manual construction to the new
NewFlatFeeLineInputstruct.openmeter/billing/invoiceline.go (2)
562-589: Well-designed input struct for flat fee line creation.The
NewFlatFeeLineInputstruct appropriately captures all necessary fields for creating flat fee lines. The field organization is logical and covers all required aspects (identification, timing, billing details, etc.).Note: The commented TODO about the
Categoryfield at line 585 suggests this might need future consideration, but the current hardcoded approach in the constructor is reasonable for now.
601-634: Solid constructor implementation with appropriate defaults.The
newFlatFeeLinefunction properly initializes all required fields with sensible defaults:
- ✅ Correctly sets
StatustoInvoiceLineStatusValid- ✅ Properly sets
TypetoInvoiceLineTypeFee- ✅ Uses
lo.CoalesceOrEmptyforManagedBydefaulting toSystemManagedLine- ✅ Hardcodes
CategorytoFlatFeeCategoryRegular(consistent with the TODO comment)The field mapping from input to output is comprehensive and correct.
turip
added
release-note/ignore
turip
force-pushed
the
refactor/flat-fee-line-creation
branch
from
e83bd74 to
089b21f
Compare
2 participants
Overview
Minor refactor on flat fee line creation.
Summary by CodeRabbit
New Features
Refactor
No changes to existing functionality or business logic.