feat(backend): consolidate subscription creation by persisting custom plans to the database #2889
Conversation
📝 WalkthroughWalkthroughThis change updates plan handling to include a custom plan metadata key, filters out custom plans from listings, modifies HTTP handlers and services to require plan references, and refactors plan input validation. Tests are adjusted to create and publish plans directly and to verify filtering behavior. 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 ✨ 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 (
|
tothandras
force-pushed
the
feat/custom-subscription
branch
from
cf337b6 to
13383a4
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
🧹 Nitpick comments (4)
openmeter/productcatalog/subscription/service/change.go (1)
51-51: Consider more descriptive variable naming.The variable name
planis quite generic. Consider using a more descriptive name likesubscriptionPlanorconvertedPlanto better indicate its purpose in the subscription change workflow.- plan := PlanFromPlan(*p) + subscriptionPlan := PlanFromPlan(*p)And update the usage:
- curr, new, err := s.WorkflowService.ChangeToPlan(ctx, request.ID, request.WorkflowInput, plan) + curr, new, err := s.WorkflowService.ChangeToPlan(ctx, request.ID, request.WorkflowInput, subscriptionPlan)openmeter/productcatalog/subscription/http/mapping.go (1)
448-449: Address TODO: Consider using annotations instead of metadata.The TODO comment indicates that annotations should be used instead of metadata for marking custom plans. This should be addressed to ensure the implementation aligns with the intended design.
Would you like me to help implement the annotation-based approach or create an issue to track this task?
openmeter/productcatalog/subscription/http/change.go (1)
65-74: Consider extracting effective period logic to a helper function.This logic for determining the effective period appears to be duplicated in the
CreateSubscriptionhandler. Consider extracting it to a shared helper function to maintain DRY principles.+// getEffectivePeriod returns the effective period for a plan, defaulting to now if not specified +func getEffectivePeriod(createPlanInput plan.CreatePlanInput) productcatalog.EffectivePeriod { + effectiveFrom := createPlanInput.EffectiveFrom + if effectiveFrom == nil { + effectiveFrom = lo.ToPtr(clock.Now()) + } + return productcatalog.EffectivePeriod{ + EffectiveFrom: effectiveFrom, + EffectiveTo: createPlanInput.EffectiveTo, + } +}Then use it:
-// Publish the custom plan to make it active -effectiveFrom := createPlanInput.EffectiveFrom -if effectiveFrom == nil { - effectiveFrom = lo.ToPtr(clock.Now()) -} -customPlan, err = h.PlanService.PublishPlan(ctx, plan.PublishPlanInput{ - NamespacedID: customPlan.NamespacedID, - EffectivePeriod: productcatalog.EffectivePeriod{ - EffectiveFrom: effectiveFrom, - EffectiveTo: createPlanInput.EffectiveTo, - }, -}) +// Publish the custom plan to make it active +customPlan, err = h.PlanService.PublishPlan(ctx, plan.PublishPlanInput{ + NamespacedID: customPlan.NamespacedID, + EffectivePeriod: getEffectivePeriod(createPlanInput), +})openmeter/productcatalog/subscription/http/create.go (1)
55-71: Consider improving the custom plan detection mechanism.The current approach of marshaling/unmarshaling JSON to detect the presence of
customPlanfield is inefficient. While the comment explains the constraint with the API transformation functions, consider these alternatives for future improvements:
- Add a discriminator field to the API request body
- Use separate endpoints for custom and plan subscriptions
- Use reflection to check field presence without JSON serialization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
e2e/productcatalog_test.go(4 hunks)openmeter/productcatalog/plan/adapter/plan.go(2 hunks)openmeter/productcatalog/plan/plan.go(1 hunks)openmeter/productcatalog/subscription/http/change.go(2 hunks)openmeter/productcatalog/subscription/http/create.go(3 hunks)openmeter/productcatalog/subscription/http/handler.go(2 hunks)openmeter/productcatalog/subscription/http/mapping.go(3 hunks)openmeter/productcatalog/subscription/plan.go(1 hunks)openmeter/productcatalog/subscription/service/change.go(1 hunks)openmeter/productcatalog/subscription/service/create.go(1 hunks)openmeter/productcatalog/subscription/service/plan.go(0 hunks)openmeter/productcatalog/subscription/testutils/adapter.go(0 hunks)openmeter/server/router/router.go(1 hunks)openmeter/subscription/aligment_test.go(3 hunks)pkg/framework/commonhttp/decoder.go(1 hunks)test/customer/customer.go(3 hunks)test/customer/testenv.go(6 hunks)
💤 Files with no reviewable changes (2)
- openmeter/productcatalog/subscription/testutils/adapter.go
- openmeter/productcatalog/subscription/service/plan.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/framework/commonhttp/decoder.go (1)
pkg/models/error.go (1)
NewGenericValidationError(140-142)
openmeter/productcatalog/subscription/service/create.go (5)
openmeter/productcatalog/subscription/plan.go (1)
PlanInput(15-17)pkg/clock/clock.go (1)
Now(14-21)pkg/models/error.go (1)
NewGenericValidationError(140-142)openmeter/productcatalog/plan.go (1)
PlanStatusActive(18-18)openmeter/productcatalog/subscription/service/plan.go (1)
PlanFromPlan(39-44)
openmeter/productcatalog/subscription/http/mapping.go (9)
pkg/models/error.go (1)
NewGenericValidationError(140-142)api/api.gen.go (5)
CustomPlanInput(2083-2102)Metadata(4613-4613)Plan(5458-5509)Alignment(1021-1025)Currency(1868-1880)openmeter/productcatalog/plan/service.go (1)
CreatePlanInput(98-101)pkg/models/metadata.go (1)
Metadata(9-9)openmeter/productcatalog/plan/plan.go (2)
MetadataKeyCustomPlan(60-60)Plan(18-30)pkg/models/model.go (1)
NamespacedModel(180-182)openmeter/productcatalog/plan.go (2)
Plan(40-45)PlanMeta(152-173)openmeter/productcatalog/alignment.go (1)
Alignment(3-6)openmeter/productcatalog/plan/httpdriver/mapping.go (1)
AsPlanPhase(136-159)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: Developer environment
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
openmeter/productcatalog/subscription/http/handler.go (1)
11-11: LGTM! Clean dependency injection implementation.The addition of the plan service import and field to the HandlerConfig follows the established dependency injection pattern in the codebase. This enables subscription handlers to interact with plan-related functionality as mentioned in the PR objectives.
Also applies to: 37-37
pkg/framework/commonhttp/decoder.go (1)
8-8: LGTM! Improved error handling consistency.The change from direct HTTP 400 errors to generic validation errors improves consistency across the codebase. The error message is clearer and the wrapping with
%wpreserves the original error context.Also applies to: 13-13
openmeter/productcatalog/plan/adapter/plan.go (1)
9-9:✅ Verification successful
LGTM! Proper filtering implementation for custom plans.
The filtering logic correctly excludes custom plans from public listings using SQL JSON operations. The implementation:
- Uses proper SQL selector function pattern
- Positioned correctly in the query building sequence
- Follows the established query building conventions in the method
Verify that the referenced constant
plan.MetadataKeyCustomPlanexists and is properly defined:Also applies to: 63-66
🏁 Script executed:
#!/bin/bash # Description: Verify the MetadataKeyCustomPlan constant exists in the plan package # Expected: Should find the constant definition rg -A 3 "MetadataKeyCustomPlan.*=" --type goLength of output: 871
LGTM – Custom plan filtering implemented correctly
- The SQL JSON filter using
sqljson.HasKeywithplan.MetadataKeyCustomPlanproperly excludes custom plans from public listings.- Verified that
plan.MetadataKeyCustomPlan = "openmeter.custom_plan"is defined inopenmeter/productcatalog/plan/plan.go.No further changes required.
openmeter/server/router/router.go (1)
389-389: LGTM! Proper dependency wiring.The addition of
PlanService: config.Plancorrectly wires up the plan service dependency to the subscription handler configuration, following the established pattern used for other services in the router.openmeter/productcatalog/plan/plan.go (1)
59-64: LGTM! Clean metadata constant definitions.The metadata key constants are well-named and follow good naming conventions with the "openmeter." prefix. This provides a clean foundation for identifying and filtering custom plans throughout the system.
The TODO comments for base plan tracking are noted - these will be useful for maintaining relationships between custom plans and their source plans.
openmeter/productcatalog/subscription/service/create.go (3)
21-25: Good architectural improvement - plan reference validation.The refactoring to require plan references exclusively is a clean separation of concerns. The validation ensures that a plan reference is always provided, which is essential for the new architecture.
34-38: Proper validation for deleted plans.The deletion check with timestamp comparison is correctly implemented. The error message provides clear context including namespace, key, version, and deletion timestamp, which will be helpful for debugging.
40-44: Correct active status validation.The active status check at the current time ensures that only currently active plans can be used for subscription creation. This prevents issues with expired or not-yet-active plans.
test/customer/customer.go (2)
219-219: Good refactoring - using centralized plan creation helper.Replacing inline plan creation with the helper method reduces code duplication and makes tests more maintainable.
569-622: Well-structured test helper method.The
createTestPlanhelper method is well-implemented with:
- Proper use of
t.Helper()for test tooling- Complete plan structure with phases and rate cards
- Plan creation and publication with error assertions
- Conversion to subscription plan format
- One-year effective period for testing
This centralizes plan setup logic and makes tests more maintainable.
test/customer/testenv.go (4)
16-18: Proper imports for plan service integration.The new imports are correctly added to support the plan service functionality in the test environment.
35-35: Clean interface extension.Adding the
Plan()method to theTestEnvinterface follows the established pattern and maintains consistency with other service accessors.
97-114: Proper plan service initialization.The plan service setup follows the established pattern with:
- Repository adapter creation with proper configuration
- Service creation with required dependencies (feature, logger, adapter, publisher)
- Appropriate error handling and wrapping
- Mock event publisher for testing
The configuration matches the pattern used for other services in the test environment.
158-159: Correct service integration.The plan service is properly included in the returned
testEnvinstance, maintaining field order consistency.openmeter/productcatalog/subscription/plan.go (2)
15-17: LGTM! Struct simplification aligns with consolidation goals.The removal of the
planfield fromPlanInputand keeping only thereffield is consistent with the PR's objective to consolidate subscription creation by requiring plan references rather than full plan inputs.
19-25: Validation logic correctly updated for simplified struct.The validation now appropriately checks only for the presence of a plan reference, which is the expected behavior after removing the
planfield. The error message is clear and informative.openmeter/subscription/aligment_test.go (1)
22-80: Test refactoring improves clarity and aligns with API changes.The direct instantiation of
plan.Planstruct makes the test more straightforward and aligns with the removal of thePlanFromPlanInputfunction. The test logic remains focused on billing period calculation while being simpler to understand.openmeter/productcatalog/subscription/service/change.go (2)
20-29: Good validation and error handling for plan reference.The validation correctly ensures a plan reference is provided and uses appropriate error handling. The error message is clear and helpful for debugging.
31-42: Excellent addition of plan status and deletion validation.The validation logic for plan status and deletion timestamp is comprehensive and follows best practices:
- Checks if the plan is active at the current time
- Validates that the plan hasn't been deleted
- Provides detailed error messages with context
e2e/productcatalog_test.go (3)
14-14: Good timing consistency improvement.The switch from standard
time.Now()toclock.Now()provides better control over timing in tests and aligns with timing handling improvements in the subscription services.Also applies to: 400-400
429-431: Assertion correctly updated for new custom plan behavior.The updated assertion appropriately expects a non-nil plan reference with the "custom_plan_" prefix, which aligns with the new approach where custom plans are created and published before subscription creation.
782-851: Comprehensive test for custom plan filtering functionality.The new test thoroughly validates the custom plan filtering behavior:
- Creates a subscription with a custom plan
- Verifies custom plans are excluded from the general plan listing
- Confirms non-custom plans are still included
- Uses appropriate assertions and logging for debugging
The test design is well-structured and provides good coverage of the filtering functionality.
openmeter/productcatalog/subscription/http/mapping.go (2)
266-266: Good error handling improvement!Wrapping the timing conversion error in
models.NewGenericValidationErrorprovides better error categorization and helps distinguish validation errors from other types of errors.
457-458: Verify the hardcoded version approach for custom plans.The version is hardcoded to
1for all custom plans. This might cause issues if custom plans need versioning support in the future.Please confirm that custom plans should always start with version 1 and won't require version management.
openmeter/productcatalog/subscription/http/change.go (1)
31-151: Well-structured refactoring!The handler refactoring improves code clarity by:
- Clearly separating custom and plan subscription handling
- Providing descriptive error messages
- Ensuring custom plans are created and published atomically
openmeter/productcatalog/subscription/http/create.go (2)
72-191: Well-implemented subscription creation flow!The refactored handler properly handles both custom and plan subscriptions with:
- Clear separation of concerns
- Proper error handling and validation
- Atomic custom plan creation and publishing
- Good use of helper functions for customer resolution
210-248: Clean implementation of customer resolution!The
getCustomerhelper function is well-structured with proper parameter validation and error handling.
… plans to the database
tothandras
force-pushed
the
feat/custom-subscription
branch
from
13383a4 to
d8d8562
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
openmeter/productcatalog/subscription/service/change.go (1)
20-24: Consider providing a more descriptive error message.The error message could be more specific to help API consumers understand what's missing.
- return def, fmt.Errorf("plan reference must be provided") + return def, fmt.Errorf("plan reference is required in PlanInput")test/customer/customer.go (1)
614-616: Consider making the effective period configurable for test flexibility.The hardcoded 1-year effective period might limit test scenarios that need different time ranges.
// createTestPlan creates and publishes a plan that can be used in tests -func (s *CustomerHandlerTestSuite) createTestPlan(ctx context.Context, t *testing.T) subscription.Plan { +func (s *CustomerHandlerTestSuite) createTestPlan(ctx context.Context, t *testing.T, effectiveDuration ...time.Duration) subscription.Plan { t.Helper() + + duration := 365 * 24 * time.Hour // default 1 year + if len(effectiveDuration) > 0 { + duration = effectiveDuration[0] + }And update line 615:
- EffectiveTo: lo.ToPtr(clock.Now().Add(365 * 24 * time.Hour)), // 1 year from now + EffectiveTo: lo.ToPtr(clock.Now().Add(duration)),openmeter/productcatalog/subscription/http/change.go (1)
45-129: Excellent refactoring of the change subscription logic.The refactored code properly handles both custom plans and plan references:
- Custom plans: Creates and publishes the plan before using its reference
- Plan references: Directly uses the provided reference
- Error handling: Clear, descriptive error messages at each step
- Timing: Properly defaults to current time if not specified
The separation of concerns and control flow is much clearer now.
Consider extracting the custom plan creation logic into a separate method to improve readability:
+func (h *handler) createAndPublishCustomPlan(ctx context.Context, customPlan api.CustomPlanInput, namespace string) (*plan.Plan, error) { + createPlanInput, err := AsCustomPlanCreateInput(customPlan, namespace) + if err != nil { + return nil, fmt.Errorf("failed to convert custom plan: %w", err) + } + + createdPlan, err := h.PlanService.CreatePlan(ctx, createPlanInput) + if err != nil { + return nil, fmt.Errorf("failed to create custom plan: %w", err) + } + + effectiveFrom := createPlanInput.EffectiveFrom + if effectiveFrom == nil { + effectiveFrom = lo.ToPtr(clock.Now()) + } + + publishedPlan, err := h.PlanService.PublishPlan(ctx, plan.PublishPlanInput{ + NamespacedID: createdPlan.NamespacedID, + EffectivePeriod: productcatalog.EffectivePeriod{ + EffectiveFrom: effectiveFrom, + EffectiveTo: createPlanInput.EffectiveTo, + }, + }) + if err != nil { + return nil, fmt.Errorf("failed to publish custom plan: %w", err) + } + + return &publishedPlan, nil +}This would make the main handler method more concise and easier to follow.
openmeter/productcatalog/subscription/http/create.go (2)
55-71: Consider a more direct approach for type detection.The marshal/unmarshal approach to detect custom plans works but is inefficient. Consider using type assertions or a more direct field check.
- // Any transformation function generated by the API will succeed if the body is serializable, so we have to check for the presence of - // fields to determine what body type we're dealing with - type testForCustomPlan struct { - CustomPlan any `json:"customPlan"` - } - - var t testForCustomPlan - - bodyBytes, err := json.Marshal(body) - if err != nil { - return CreateSubscriptionRequest{}, fmt.Errorf("failed to marshal request body: %w", err) - } - - if err := json.Unmarshal(bodyBytes, &t); err != nil { - return CreateSubscriptionRequest{}, fmt.Errorf("failed to unmarshal request body: %w", err) - } - - if t.CustomPlan != nil { + // Check if the request contains a custom plan by attempting to access the field directly + if body.CustomPlan != nil {
125-132: Consider extracting common workflow input setup.The workflow input setup logic is duplicated between custom plan and plan reference branches. Consider extracting this into a helper function.
func setupWorkflowInput(timing subscription.Timing, name, description string, metadata map[string]string) subscriptionworkflow.ChangeSubscriptionWorkflowInput { return subscriptionworkflow.ChangeSubscriptionWorkflowInput{ Timing: timing, Name: name, Description: description, MetadataModel: models.MetadataModel{ Metadata: metadata, }, } }Also applies to: 170-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
e2e/productcatalog_test.go(4 hunks)openmeter/productcatalog/plan/adapter/plan.go(2 hunks)openmeter/productcatalog/plan/plan.go(1 hunks)openmeter/productcatalog/subscription/http/change.go(2 hunks)openmeter/productcatalog/subscription/http/create.go(3 hunks)openmeter/productcatalog/subscription/http/handler.go(2 hunks)openmeter/productcatalog/subscription/http/mapping.go(3 hunks)openmeter/productcatalog/subscription/plan.go(1 hunks)openmeter/productcatalog/subscription/service/change.go(1 hunks)openmeter/productcatalog/subscription/service/create.go(1 hunks)openmeter/productcatalog/subscription/service/plan.go(0 hunks)openmeter/productcatalog/subscription/testutils/adapter.go(0 hunks)openmeter/server/router/router.go(1 hunks)openmeter/subscription/aligment_test.go(3 hunks)pkg/framework/commonhttp/decoder.go(1 hunks)test/customer/customer.go(3 hunks)test/customer/testenv.go(6 hunks)
💤 Files with no reviewable changes (2)
- openmeter/productcatalog/subscription/testutils/adapter.go
- openmeter/productcatalog/subscription/service/plan.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/framework/commonhttp/decoder.go
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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 (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
openmeter/productcatalog/subscription/http/handler.go (2)
11-11: LGTM!The import is properly added to support the new
PlanServicefield in theHandlerConfigstruct.
37-37: LGTM!The
PlanServicefield addition follows the established pattern and enables subscription handlers to interact with plan operations, which is essential for the custom plan persistence feature.openmeter/productcatalog/plan/adapter/plan.go (2)
9-9: LGTM!The
sqljsonimport is correctly added to support JSON field operations in the SQL query.
63-67:✅ Verification successful
Verify that the
MetadataKeyCustomPlanconstant is accessible.The filter implementation correctly excludes custom plans from listings. However, ensure that the
plan.MetadataKeyCustomPlanconstant is properly exported and accessible from this package.Run the following script to verify the constant is properly defined and exported:
🏁 Script executed:
#!/bin/bash # Description: Verify that MetadataKeyCustomPlan is exported and accessible # Check if the constant is defined and exported (starts with uppercase) ast-grep --pattern 'const ( $$$ MetadataKeyCustomPlan = $_ $$$ )' # Also check for direct const declaration ast-grep --pattern 'const MetadataKeyCustomPlan = $_'Length of output: 672
Constant Export Verified
The
MetadataKeyCustomPlanconstant is defined and exported inopenmeter/productcatalog/plan/plan.go(line 60). No further changes are needed.openmeter/productcatalog/plan/plan.go (1)
59-64: Well-designed metadata key structure!The
MetadataKeyCustomPlanconstant is properly defined with a namespaced key that prevents conflicts. The TODO comments show good foresight for tracking custom plan lineage in the future.openmeter/server/router/router.go (1)
384-395: LGTM! Necessary dependency injection for custom plan handling.The addition of
PlanServiceto the subscription handler configuration aligns with the PR objective to consolidate subscription creation by persisting custom plans to the database.test/customer/testenv.go (2)
16-18: Well-structured addition of plan service to test environment.The plan service integration follows the existing patterns and properly extends the test environment interface.
Also applies to: 35-35, 45-45, 62-64
97-115: Proper initialization of plan service with appropriate error handling.The plan repository and service are correctly initialized with the required dependencies, including database client, logger, feature registry, and mock event publisher for testing.
openmeter/productcatalog/subscription/service/create.go (2)
21-30: Good refactoring to enforce plan persistence before subscription creation.The change to require a plan reference ensures that all plans (including custom ones) are persisted to the database before being used in subscriptions, aligning with the PR objective.
34-44: Comprehensive plan validation with clear error messages.The validation logic properly checks both deletion status and active status of the plan, providing descriptive error messages with relevant context.
openmeter/productcatalog/subscription/service/change.go (1)
16-63: Clean refactoring to centralize plan reference handling!The refactoring successfully consolidates plan handling by requiring plan references, which aligns with the PR objective of persisting custom plans to the database. The validation logic is comprehensive and the error messages provide good context.
test/customer/customer.go (1)
219-219: Excellent refactoring to centralize test plan creation!The introduction of
createTestPlanhelper method effectively reduces code duplication and provides a consistent way to create test plans across multiple test cases. The helper properly handles both plan creation and publication.Also applies to: 518-518, 570-622
openmeter/subscription/aligment_test.go (2)
22-81: Good simplification of test plan creation!The direct struct instantiation makes the test more readable and eliminates unnecessary complexity from using CreatePlanInput. The plan structure is well-defined with appropriate phases and rate cards for testing aligned billing period calculations.
80-80:❌ Incorrect review comment
Consider maintaining error handling for plan conversion.
The removal of error handling from the plan conversion could mask potential issues during test execution. Even in tests, it's valuable to catch unexpected errors early.
🏁 Script executed:
#!/bin/bash # Description: Check if PlanFromPlan can return errors by examining its implementation # Search for the PlanFromPlan function definition to understand if it can return errors ast-grep --pattern 'func PlanFromPlan($_) $_'Length of output: 536
No error to handle—PlanFromPlan never returns an error
The signature of PlanFromPlan is:func PlanFromPlan(p plan.Plan) subscription.PlanSince it doesn’t return an error, there’s nothing to catch. You can safely use the simpler call without error handling.
Likely an incorrect or invalid review comment.
openmeter/productcatalog/subscription/plan.go (2)
15-17: LGTM! Good simplification of the PlanInput structure.Removing the inline plan field aligns well with the PR objective of persisting custom plans to the database. This ensures all plans are properly stored before being referenced in subscriptions.
19-25: LGTM! Validation logic correctly updated.The validation now correctly enforces that only plan references are accepted, which is consistent with the structural changes.
e2e/productcatalog_test.go (4)
14-14: Good improvement using the clock package.Using
clock.Now()instead oftime.Now()enables better testability with mockable time sources.
400-400: Consistent use of clock package.Good to see consistent usage of
clock.Now()throughout the test file.
429-432: Correct assertion update for custom subscription plans.The updated assertion correctly verifies that custom plans are now persisted and have a plan reference with a key containing "custom_plan_".
782-851: Excellent test coverage for custom plan filtering.This test thoroughly validates that:
- Custom plans are properly created with the metadata marker
- Custom plans are filtered out from the list API response
- Non-custom plans remain visible in the list
The test creates a separate customer to avoid conflicts and verifies the filtering logic works as expected.
openmeter/productcatalog/subscription/http/mapping.go (3)
9-9: LGTM! Appropriate imports added.The new imports support ULID generation for unique keys, plan creation input types, and default value handling.
Also applies to: 17-17, 23-23
266-266: Good error handling improvement.Using
models.NewGenericValidationErrorprovides better error categorization for validation failures.
445-494: Well-structured custom plan conversion function.The
AsCustomPlanCreateInputfunction properly:
- Generates unique keys using ULID
- Marks plans as custom via metadata
- Provides sensible defaults
- Validates currency codes
- Handles alignment settings
- Converts phases with proper error wrapping
The implementation is clean and follows good practices.
openmeter/productcatalog/subscription/http/change.go (2)
11-12: LGTM! Required imports for plan management.The new imports support the enhanced functionality for creating and publishing custom plans.
Also applies to: 15-15
42-42: Good error message improvement.More descriptive error messages help with debugging.
openmeter/productcatalog/subscription/http/create.go (7)
13-14: LGTM! Import additions support the new functionality.The new imports are appropriate for the enhanced subscription creation logic that handles custom plan creation and publication.
Also applies to: 18-19
46-53: Good practice: Early variable initialization improves readability.Declaring the shared variables at the beginning of the function makes the code flow clearer and easier to follow.
92-95: Good fallback handling for effective time.The logic to default to the current time when
effectiveFromis nil is correct and handles edge cases well.
140-149: Verify plan existence before subscription creation.The plan retrieval logic is correct and properly validates the plan exists before proceeding with subscription creation.
180-185: Good separation of customer retrieval logic.Moving customer retrieval to the end and making it common for both branches improves code organization and reduces duplication.
187-191: Clean final request construction.The final request assembly is clear and includes all necessary components for subscription creation.
86-105:✅ Verification successful
Verify custom plan creation and publication error handling.
The custom plan creation and publication flow looks correct, but ensure that failed plan creation doesn't leave orphaned resources.
🏁 Script executed:
#!/bin/bash # Description: Check if there are cleanup mechanisms for failed plan creation/publication # Expected: Find cleanup handlers or transaction management # Search for cleanup or rollback mechanisms in plan service rg -A 10 -B 5 "CreatePlan|PublishPlan" --type go | rg -i "cleanup|rollback|transaction|defer" # Check for error handling patterns in plan service ast-grep --pattern 'func $_(ctx context.Context, $_) ($_) { $$$ defer $$$ $$$ }'Length of output: 998
Plan creation & publication are transactional with automatic rollback
PlanService.CreatePlan and PublishPlan in openmeter/productcatalog/plan/service/plan.go are both executed via
transaction.Run, which wraps all database operations in a transaction and rolls back on error. This guarantees no orphaned resources remain if either step fails—no additional cleanup is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openmeter/productcatalog/plan/plan.go (1)
59-63: Track the TODO for custom plan metadata.These commented constants for tracking base plan information in custom plans would be valuable for maintaining the relationship between custom and base plans.
Would you like me to create a GitHub issue to track implementing these metadata keys for custom plan lineage tracking?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
e2e/productcatalog_test.go(4 hunks)openmeter/ent/db/migrate/schema.go(1 hunks)openmeter/ent/db/mutation.go(8 hunks)openmeter/ent/db/plan.go(4 hunks)openmeter/ent/db/plan/plan.go(4 hunks)openmeter/ent/db/plan/where.go(2 hunks)openmeter/ent/db/plan_create.go(7 hunks)openmeter/ent/db/plan_update.go(4 hunks)openmeter/ent/db/runtime.go(1 hunks)openmeter/ent/schema/productcatalog.go(1 hunks)openmeter/productcatalog/plan/adapter/plan.go(2 hunks)openmeter/productcatalog/plan/plan.go(1 hunks)openmeter/productcatalog/plan/service.go(1 hunks)openmeter/productcatalog/subscription/http/mapping.go(3 hunks)tools/migrate/migrations/20250526111543_plan-is-custom.down.sql(1 hunks)tools/migrate/migrations/20250526111543_plan-is-custom.up.sql(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tools/migrate/migrations/20250526111543_plan-is-custom.up.sql
- openmeter/productcatalog/plan/service.go
- tools/migrate/migrations/20250526111543_plan-is-custom.down.sql
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- 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 (30)
openmeter/ent/schema/productcatalog.go (1)
41-43: LGTM! Theis_customfield is well-implemented.The boolean field addition follows the existing schema patterns with an appropriate default value and clear documentation. This properly supports the PR's objective of persisting custom plans to the database.
openmeter/ent/db/migrate/schema.go (1)
1732-1732: Auto-generated migration is consistent with schema changes.The
is_customcolumn addition in the migration file correctly reflects the schema changes inproductcatalog.go.openmeter/productcatalog/plan/adapter/plan.go (2)
62-64: LGTM! Appropriate placement of custom plan filter.The filter correctly excludes custom plans from the listing, which aligns with the PR objective of consolidating subscription creation.
188-188: LGTM! Correctly sets the custom flag during plan creation.The implementation properly persists the custom plan flag to the database, enabling the consolidation of subscription creation as intended.
openmeter/ent/db/plan/where.go (2)
128-131: Generated code looks correct.The
IsCustompredicate function follows the established pattern for boolean field predicates in this generated file.
758-767: Generated predicate functions are properly implemented.The
IsCustomEQandIsCustomNEQfunctions follow the standard pattern for field predicates and are correctly placed in the file.openmeter/ent/db/runtime.go (1)
1172-1175: LGTM! Generated code follows the expected pattern.The initialization of the
is_customfield descriptor and default value follows the same pattern as other boolean fields in this generated file.openmeter/ent/db/plan.go (4)
47-48: LGTM! Field addition is properly documented.The
IsCustomfield is correctly added with appropriate documentation and JSON serialization tag.
102-103: LGTM! Scan handling correctly implemented.The
IsCustomfield is properly added to the boolean fields case for SQL scanning.
215-220: LGTM! Value assignment follows the established pattern.The
is_customfield handling inassignValuescorrectly implements type checking and value assignment, consistent with other boolean fields in the entity.
318-320: LGTM! String representation updated correctly.The
is_customfield is properly included in theString()method output.e2e/productcatalog_test.go (3)
14-14: Good use of clock package for consistent time handling.Using
clock.Now()instead oftime.Now()improves testability by allowing time to be mocked in tests.Also applies to: 400-400
429-431: Assertion correctly updated to reflect custom plan persistence.The change from checking
nilto verifying the plan exists and has acustom_plan_prefix correctly reflects the new behavior where custom plans are persisted to the database.
782-843: Comprehensive test for custom plan filtering.The test properly verifies that custom plans are filtered out from the plan listing API. Good practices observed:
- Creates a separate customer to avoid conflicts
- Verifies that the custom plan is created successfully
- Confirms non-custom plans remain in the list
- Uses appropriate assertions
openmeter/productcatalog/subscription/http/mapping.go (2)
266-266: Improved error handling with validation context.Wrapping the error with
models.NewGenericValidationErrorprovides better context for API consumers.
445-493: Well-structured custom plan creation function.The new
AsCustomPlanCreateInputfunction is a significant improvement:
- Generates unique keys using ULID to avoid conflicts
- Properly sets
IsCustom: trueto mark plans as custom- Provides sensible defaults (e.g., "Custom Plan" for name)
- Handles metadata, alignment, and currency validation appropriately
- Clear error messages with proper context
openmeter/ent/db/plan_update.go (1)
1-1: Generated code correctly implements is_custom field handling.The generated methods for handling the
is_customfield follow the established patterns in the codebase:
- Setter methods with proper chaining
- Nullable variant for optional updates
- Correct integration in
sqlSavemethodsAlso applies to: 181-194, 411-413, 716-729, 976-978
openmeter/ent/db/plan/plan.go (4)
43-44: LGTM! New field constant follows established pattern.The
FieldIsCustomconstant is correctly defined following the same pattern as other field constants in the file. This supports the PR objective of distinguishing custom plans from regular plans.
92-92: LGTM! Column properly added to the slice.The
FieldIsCustomis correctly included in theColumnsslice, which is necessary for the field to be recognized as a valid column in database operations.
124-125: LGTM! Default value variable properly declared.The
DefaultIsCustomvariable follows the same pattern as other default value variables. The zero value (false) is appropriate as most plans would be non-custom by default.
198-201: LGTM! Ordering function correctly implemented.The
ByIsCustomfunction is properly implemented following the exact same pattern as other ordering functions in the file. This enables query result ordering by theis_customfield.openmeter/ent/db/mutation.go (8)
38655-38655: LGTM!The field addition follows the established pattern for mutation fields, using a pointer type to distinguish between unset and set states.
39328-39362: LGTM!The method implementations for the
is_customfield are correct and follow the established patterns for mutation methods, including proper error handling and validation.
39560-39560: LGTM!The fields slice capacity is correctly updated from 13 to 14 to accommodate the new field.
39600-39602: LGTM!The field is correctly added to the Fields() method output when set.
39637-39638: LGTM!The Field() method correctly handles the new field case.
39674-39675: LGTM!The OldField() method correctly handles retrieving the old value for the new field.
39776-39782: LGTM!The SetField() method correctly handles type validation and setting the new field value.
39919-39921: LGTM!The ResetField() method correctly handles resetting the new field.
openmeter/ent/db/plan_create.go (1)
1-1375: Generated code changes look consistent.This is an auto-generated file by the Ent ORM framework. The changes properly add support for the new
is_customboolean field following the established patterns for other fields. All necessary methods for creation, updating, and upserting (both single and bulk operations) have been consistently added.Since this is generated code, any modifications should be made to the schema definition files rather than editing this file directly.
Existing subscriptions are not considered.
Summary by CodeRabbit
New Features
is_customattribute to plans, enabling filtering out custom plans from listings.is_customflag.PlanServicedependency in router configuration for seamless plan management.Bug Fixes
Refactor
Tests