SubscriptionAddons III #2677
SubscriptionAddons III #2677
Conversation
📝 WalkthroughWalkthroughThis change removes the entire "subscription addon rate card" feature from the codebase. It deletes all database schemas, entities, repositories, service logic, and API fields related to subscription addon rate cards and their item links. The migration scripts drop the associated tables. The code, API models, and tests are refactored to eliminate references to rate cards within subscription addons, including input fields, validation, and mapping logic. The remaining subscription addon functionality is preserved and refactored to work without rate card associations, and the workflow for adding addons is updated accordingly. Related documentation and diagrams are also updated to reflect the simplified model. 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 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
GAlexIHU
force-pushed
the
galexi/addons-2-diff
branch
from
ba81ffd to
a6414bf
Compare
GAlexIHU
force-pushed
the
galexi/addons-3
branch
from
977f97d to
fa9f036
Compare
GAlexIHU
force-pushed
the
galexi/addons-3
branch
from
489d26e to
b7182ed
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: 4
🔭 Outside diff range comments (1)
api/openapi.yaml (1)
19828-19850: 🛠️ Refactor suggestionObsolete
SubscriptionAddonRateCardschema remains—consider removal or deprecationThe
SubscriptionAddonRateCardschema is still present, even though the PR and AI summary state that rate card linkage for subscription addons has been removed. If this schema is no longer used in any API request/response, it should be removed or marked as deprecated to avoid confusion and reduce maintenance burden.If you intend to keep it for backward compatibility, add a
deprecated: trueflag and a comment explaining its status.
♻️ Duplicate comments (3)
api/spec/src/productcatalog/subscriptionaddon.tsp (1)
80-113: New SubscriptionAddonAddon model looks goodExtracting the addon properties into a separate model improves modularity and reusability. The model preserves all the original fields and metadata annotations.
openmeter/subscription/addon/diff/affected.go (1)
14-40: Consider simplifying the nested loops to improve code readability.The current implementation has 4 levels of nested loops, which can make the code difficult to understand and maintain. Consider extracting some of the logic into helper functions to reduce nesting depth.
For example, you could refactor the inner loops into a separate function:
func GetAffectedItemIDs(view subscription.SubscriptionView, addon subscriptionaddon.SubscriptionAddon) map[string][]string { affected := map[string][]string{} for _, inst := range addon.GetInstances() { instPer := inst.CadencedModel.AsPeriod() for _, rc := range inst.RateCards { rcKey := rc.AddonRateCard.RateCard.Key() - - for _, p := range view.Phases { - for _, items := range p.ItemsByKey { - for _, item := range items { - if item.Spec.RateCard.Key() != rcKey { - continue - } - - itemPer := item.SubscriptionItem.CadencedModel.AsPeriod() - - if instPer.Intersection(itemPer) != nil { - if _, ok := affected[rcKey]; !ok { - affected[rcKey] = []string{} - } - - affected[rcKey] = append(affected[rcKey], item.SubscriptionItem.ID) - } - } - } - } + findAffectedItems(view, rcKey, instPer, affected) } } // Let's dedupe return lo.MapEntries(affected, func(key string, value []string) (string, []string) { return key, lo.Uniq(value) }) } +func findAffectedItems(view subscription.SubscriptionView, rcKey string, instPer time.Period, affected map[string][]string) { + for _, p := range view.Phases { + for _, items := range p.ItemsByKey { + for _, item := range items { + if item.Spec.RateCard.Key() != rcKey { + continue + } + + itemPer := item.SubscriptionItem.CadencedModel.AsPeriod() + + if instPer.Intersection(itemPer) != nil { + if _, ok := affected[rcKey]; !ok { + affected[rcKey] = []string{} + } + + affected[rcKey] = append(affected[rcKey], item.SubscriptionItem.ID) + } + } + } + } +}openmeter/subscription/testutils/helpers.go (1)
18-36: Write out public function name in commentThe function
CreateSubscriptionFromPlanis public but lacks a proper documentation comment explaining its purpose, parameters, and return values. Consider adding a doc comment following Go conventions.+// CreateSubscriptionFromPlan creates a subscription from the given plan input with a specified start time. +// It returns the created plan and subscription view. func CreateSubscriptionFromPlan(t *testing.T, deps *SubscriptionDependencies, planInp plan.CreatePlanInput, startAt time.Time) (subscription.Plan, subscription.SubscriptionView) {
🧹 Nitpick comments (16)
openmeter/subscription/workflow/service/subscription.go (1)
78-87: Added validation to prevent editing subscriptions with addons.This new check prevents users from editing subscriptions that have addons attached, which aligns with the PR objective that states "the implementation includes a check that prevents editing if an addon is already applied to a subscription." The error message is clear and appropriate.
One suggestion would be to provide more details in the error message about why this limitation exists and what alternatives users might have.
Consider enhancing the error message to provide more context:
- return subscription.SubscriptionView{}, models.NewGenericForbiddenError(fmt.Errorf("subscription with addons cannot be edited")) + return subscription.SubscriptionView{}, models.NewGenericForbiddenError(fmt.Errorf("subscription with addons cannot be edited - please remove addons first before making changes"))openmeter/subscription/addon/compatible.go (1)
9-10: Consider documenting validation purposeThe
validateRateCardsfunction provides important compatibility checking between rate cards. Consider adding a function comment that explains the purpose and criteria for validation.-// TODO: this will be moved to different package -func validateRateCards(source, target productcatalog.RateCard) error { +// TODO: this will be moved to different package +// validateRateCards ensures that the source rate card (from an addon) is compatible +// with the target rate card by checking key equality, price types, payment terms, +// and entitlement template types. +func validateRateCards(source, target productcatalog.RateCard) error {openmeter/subscription/addon/addon.go (1)
23-28: Consider addressing the TODO comment.There's a TODO comment about potentially breaking up the Addon field into AddonID + AddonMeta. This might be worth considering as it would potentially improve the model design.
Consider addressing the TODO comment about breaking up the Addon field into separate components for cleaner design, either in this PR or tracking it for future work.
openmeter/subscription/workflow/service/addon.go (1)
28-28: Consider implementing subscription-level locking
The TODO comment highlights potential concurrency issues. Without a subscription lock, concurrent calls might create conflicts or duplicate addons.openmeter/productcatalog/ratecard.go (1)
372-377: Duplicate approach to ChangeMeta
The logic here is identical to the FlatFeeRateCard. Consider unifying them in a shared helper to reduce repetition.-func (r *UsageBasedRateCard) ChangeMeta(fn func(m RateCardMeta) (RateCardMeta, error)) error { - var err error - r.RateCardMeta, err = fn(r.RateCardMeta) - if err != nil { - return err - } - return r.Validate() -} +// Potential approach: unify in a shared function, e.g.: +func applyChangeMeta(r *RateCardMeta, fn func(m RateCardMeta) (RateCardMeta, error)) error { + newMeta, err := fn(*r) + if err != nil { + return err + } + *r = newMeta + return r.Validate() +}openmeter/subscription/addon/http/handler.go (3)
31-36: Consider validating configuration in the constructor.While the constructor correctly follows the factory pattern, it might be beneficial to validate that required dependencies are not nil before creating the handler. This would prevent potential nil pointer panics at runtime.
func NewHandler(config HandlerConfig, options ...httptransport.HandlerOption) Handler { + // Validate required dependencies + if config.SubscriptionAddonService == nil { + panic("SubscriptionAddonService is required") + } + if config.SubscriptionWorkflowService == nil { + panic("SubscriptionWorkflowService is required") + } + if config.NamespaceDecoder == nil { + panic("NamespaceDecoder is required") + } + if config.Logger == nil { + config.Logger = slog.Default() + } + return &handler{ HandlerConfig: config, Options: options, } }
43-50: Improve error message specificity in resolveNamespace.The current error message when namespace resolution fails is generic. A more specific error message would help with debugging and could provide more context about what went wrong.
func (h *handler) resolveNamespace(ctx context.Context) (string, error) { ns, ok := h.NamespaceDecoder.GetNamespace(ctx) if !ok { - return "", commonhttp.NewHTTPError(http.StatusInternalServerError, errors.New("internal server error")) + return "", commonhttp.NewHTTPError(http.StatusInternalServerError, errors.New("failed to resolve namespace from context")) } return ns, nil }
1-15: Add godoc comments to improve code documentation.Adding godoc comments for the package, interface, and important functions would improve the documentation and make the code more maintainable. This is especially important for public APIs.
+// Package httpdriver provides HTTP handlers for subscription addon operations. package httpdriver import ( "context" "errors" "log/slog" "net/http" "github.com/openmeterio/openmeter/openmeter/namespace/namespacedriver" "github.com/openmeterio/openmeter/openmeter/subscription" subscriptionaddon "github.com/openmeterio/openmeter/openmeter/subscription/addon" subscriptionworkflow "github.com/openmeterio/openmeter/openmeter/subscription/workflow" "github.com/openmeterio/openmeter/pkg/framework/commonhttp" "github.com/openmeterio/openmeter/pkg/framework/transport/httptransport" )Similarly, add comments for the Handler interface and key methods.
openmeter/subscription/addon/http/get.go (1)
40-52: Consider handling different error types specifically.The error handling doesn't distinguish between different types of errors (e.g., not found vs. internal server error). This limits the ability to provide appropriate HTTP status codes to clients.
Consider enhancing error handling:
res, err := h.SubscriptionAddonService.Get(ctx, req.SubscriptionAddonID) if err != nil { + // Handle specific error types with appropriate status codes + if errors.Is(err, subscription.ErrSubscriptionAddonNotFound) { + return GetSubscriptionAddonResponse{}, httptransport.NewError(http.StatusNotFound, err.Error()) + } return GetSubscriptionAddonResponse{}, err }openmeter/subscription/addon/extend.go (2)
34-55: Review price merging logic for potential fallback or rounding concerns.Merging flat prices by summing amounts and overwriting the payment term is logically sound. Consider whether:
- Additional rounding rules or currency precision checks are required.
- Different price types (beyond flat) must be supported soon.
Overall, this block is well-structured with a clear fallback error for unsupported types.
81-81: Consider implementing 'Restore' or remove the placeholder.Returning a not implemented error is an acceptable temporary placeholder, but it may cause confusion for callers if they mistakenly expect restore functionality. Either add explanatory docs or remove the method if it's not planned soon.
openmeter/subscription/addon/http/create.go (1)
27-48: Validate request payload for required fields.Here, the request body is simply decoded, and namespace resolution is enforced. Consider additional validation for the addon input, especially if certain fields are mandatory. This helps ensure robust error handling before calling downstream services.
openmeter/subscription/addon/http/mapping.go (2)
36-48: Consider enhancing error message for missing instancesThe error message "no instances found" could be more descriptive to aid debugging. Consider including the addon ID or other identifying information.
- return api.SubscriptionAddon{}, errors.New("no instances found") + return api.SubscriptionAddon{}, fmt.Errorf("no instances found for subscription addon ID: %s", addon.ID)
49-53: Consider adding validation for empty periods arrayThe code assumes pers[0] exists when creating the union of periods. While the preceding check ensures the array isn't empty, it would be more robust to handle this case explicitly in the Reduce function.
- union := lo.Reduce(pers, func(agg timeutil.OpenPeriod, item timeutil.OpenPeriod, _ int) timeutil.OpenPeriod { - return agg.Union(item) - }, pers[0]) + // Start with first period as initial value, or create empty result if no periods + var initialPeriod timeutil.OpenPeriod + if len(pers) > 0 { + initialPeriod = pers[0] + } + + union := lo.Reduce(pers, func(agg timeutil.OpenPeriod, item timeutil.OpenPeriod, _ int) timeutil.OpenPeriod { + return agg.Union(item) + }, initialPeriod)openmeter/subscription/testutils/helpers.go (2)
38-64: Add explanation for "use workflow service instead" commentThe comment "For most cases, use the workflow service instead!" doesn't explain why. Consider explaining the specific scenarios where this helper should or shouldn't be used.
-// For most cases, use the workflow service instead! +// CreateMultiInstanceAddonForSubscription creates an addon with multiple quantity changes. +// Note: This bypasses workflow validations and should only be used for testing edge cases. +// For standard use cases, use the workflow service's AddAddon method instead. func CreateMultiInstanceAddonForSubscription(t *testing.T, deps *SubscriptionDependencies, subID models.NamespacedID, addonInp addon.CreateAddonInput, quants []subscriptionaddon.CreateSubscriptionAddonQuantityInput) (addon.Addon, subscriptionaddon.SubscriptionAddon) {
66-86: Explain "hacky" approach and provide alternativesThe comment describes the approach as "a bit hacky" but doesn't explain why or suggest better alternatives. Consider clarifying the comment to help future developers understand the limitations.
-// this is a bit hacky, we reuse the addon's effective period as cadence for the subscriptionaddon -// For most cases, use the workflow service instead! +// CreateAddonForSubscription creates a subscription addon using the addon's effective period +// to determine its activation timeline. This approach simplifies testing but doesn't mirror +// how addons would typically be activated in production code. +// +// Note: This bypasses workflow validations. For standard use cases, use the +// workflow service's AddAddon method instead. func CreateAddonForSubscription(t *testing.T, deps *SubscriptionDependencies, subID models.NamespacedID, addonInp addon.CreateAddonInput) (addon.Addon, subscriptionaddon.SubscriptionAddon) {
📜 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 (102)
api/api.gen.go(5 hunks)api/client/go/client.gen.go(5 hunks)api/client/javascript/src/client/schemas.ts(5 hunks)api/openapi.cloud.yaml(4 hunks)api/openapi.yaml(4 hunks)api/spec/src/productcatalog/subscriptionaddon.tsp(2 hunks)app/common/subscription.go(5 hunks)cmd/billing-worker/wire_gen.go(1 hunks)cmd/jobs/internal/wire_gen.go(1 hunks)cmd/server/main.go(1 hunks)cmd/server/wire_gen.go(1 hunks)openmeter/ent/db/addonratecard.go(1 hunks)openmeter/ent/db/addonratecard/addonratecard.go(0 hunks)openmeter/ent/db/addonratecard/where.go(0 hunks)openmeter/ent/db/addonratecard_create.go(0 hunks)openmeter/ent/db/addonratecard_query.go(3 hunks)openmeter/ent/db/addonratecard_update.go(0 hunks)openmeter/ent/db/client.go(4 hunks)openmeter/ent/db/ent.go(0 hunks)openmeter/ent/db/expose.go(0 hunks)openmeter/ent/db/hook/hook.go(0 hunks)openmeter/ent/db/migrate/schema.go(0 hunks)openmeter/ent/db/paginate.go(0 hunks)openmeter/ent/db/predicate/predicate.go(0 hunks)openmeter/ent/db/runtime.go(0 hunks)openmeter/ent/db/setorclear.go(0 hunks)openmeter/ent/db/subscriptionaddon.go(3 hunks)openmeter/ent/db/subscriptionaddon/subscriptionaddon.go(0 hunks)openmeter/ent/db/subscriptionaddon/where.go(0 hunks)openmeter/ent/db/subscriptionaddon_create.go(0 hunks)openmeter/ent/db/subscriptionaddon_query.go(1 hunks)openmeter/ent/db/subscriptionaddon_update.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard/subscriptionaddonratecard.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard/where.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard_create.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard_delete.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard_query.go(0 hunks)openmeter/ent/db/subscriptionaddonratecard_update.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink/subscriptionaddonratecarditemlink.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink/where.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink_create.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink_delete.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink_query.go(0 hunks)openmeter/ent/db/subscriptionaddonratecarditemlink_update.go(0 hunks)openmeter/ent/db/subscriptionitem.go(1 hunks)openmeter/ent/db/subscriptionitem/subscriptionitem.go(0 hunks)openmeter/ent/db/subscriptionitem/where.go(0 hunks)openmeter/ent/db/subscriptionitem_create.go(0 hunks)openmeter/ent/db/subscriptionitem_query.go(3 hunks)openmeter/ent/db/subscriptionitem_update.go(0 hunks)openmeter/ent/db/tx.go(0 hunks)openmeter/ent/schema/addon.go(0 hunks)openmeter/ent/schema/subscription.go(0 hunks)openmeter/ent/schema/subscription_addon.go(0 hunks)openmeter/productcatalog/ratecard.go(3 hunks)openmeter/server/router/router.go(5 hunks)openmeter/server/router/subscriptionaddon.go(1 hunks)openmeter/server/server_test.go(4 hunks)openmeter/subscription/addon/README.md(2 hunks)openmeter/subscription/addon/addon.go(3 hunks)openmeter/subscription/addon/addonratecard.go(0 hunks)openmeter/subscription/addon/compatible.go(1 hunks)openmeter/subscription/addon/diff/addon.go(2 hunks)openmeter/subscription/addon/diff/affected.go(1 hunks)openmeter/subscription/addon/diff/apply.go(2 hunks)openmeter/subscription/addon/diff/apply_test.go(14 hunks)openmeter/subscription/addon/extend.go(1 hunks)openmeter/subscription/addon/extend_test.go(1 hunks)openmeter/subscription/addon/http/create.go(1 hunks)openmeter/subscription/addon/http/get.go(1 hunks)openmeter/subscription/addon/http/handler.go(1 hunks)openmeter/subscription/addon/http/list.go(1 hunks)openmeter/subscription/addon/http/mapping.go(1 hunks)openmeter/subscription/addon/ratecard.go(1 hunks)openmeter/subscription/addon/repo/mapping.go(2 hunks)openmeter/subscription/addon/repo/subscriptionaddon.go(1 hunks)openmeter/subscription/addon/repo/subscriptionaddonratecard.go(0 hunks)openmeter/subscription/addon/repo/transaction.go(0 hunks)openmeter/subscription/addon/repository.go(0 hunks)openmeter/subscription/addon/service/change_test.go(0 hunks)openmeter/subscription/addon/service/create_test.go(1 hunks)openmeter/subscription/addon/service/list_test.go(0 hunks)openmeter/subscription/addon/service/service.go(2 hunks)openmeter/subscription/patch/patch_test.go(2 hunks)openmeter/subscription/service/sync_test.go(1 hunks)openmeter/subscription/state.go(2 hunks)openmeter/subscription/testutils/compare.go(1 hunks)openmeter/subscription/testutils/helpers.go(1 hunks)openmeter/subscription/testutils/service.go(1 hunks)openmeter/subscription/timing.go(1 hunks)openmeter/subscription/workflow/service.go(3 hunks)openmeter/subscription/workflow/service/addon.go(1 hunks)openmeter/subscription/workflow/service/addon_test.go(1 hunks)openmeter/subscription/workflow/service/service.go(1 hunks)openmeter/subscription/workflow/service/subscription.go(2 hunks)openmeter/subscription/workflow/service/subscription_test.go(5 hunks)pkg/models/error.go(1 hunks)test/billing/subscription_suite.go(3 hunks)tools/migrate/migrations/20250416094534_delete-subsadd-ratecards.down.sql(1 hunks)tools/migrate/migrations/20250416094534_delete-subsadd-ratecards.up.sql(1 hunks)
💤 Files with no reviewable changes (44)
- openmeter/ent/db/ent.go
- openmeter/ent/schema/subscription.go
- openmeter/subscription/addon/addonratecard.go
- openmeter/ent/db/subscriptionaddon/where.go
- openmeter/ent/schema/addon.go
- openmeter/ent/db/subscriptionaddon_create.go
- openmeter/ent/db/tx.go
- openmeter/ent/db/predicate/predicate.go
- openmeter/subscription/addon/service/list_test.go
- openmeter/subscription/addon/repository.go
- openmeter/ent/db/expose.go
- openmeter/subscription/addon/repo/transaction.go
- openmeter/ent/db/subscriptionitem/subscriptionitem.go
- openmeter/ent/db/addonratecard_create.go
- openmeter/subscription/addon/service/change_test.go
- openmeter/ent/db/subscriptionaddonratecarditemlink_delete.go
- openmeter/ent/db/runtime.go
- openmeter/ent/db/subscriptionitem_create.go
- openmeter/ent/db/subscriptionitem/where.go
- openmeter/subscription/addon/repo/subscriptionaddonratecard.go
- openmeter/ent/db/addonratecard/addonratecard.go
- openmeter/ent/db/addonratecard/where.go
- openmeter/ent/db/subscriptionaddonratecard/subscriptionaddonratecard.go
- openmeter/ent/db/subscriptionaddon/subscriptionaddon.go
- openmeter/ent/db/subscriptionaddonratecarditemlink.go
- openmeter/ent/db/subscriptionaddonratecard_delete.go
- openmeter/ent/db/subscriptionaddon_update.go
- openmeter/ent/db/subscriptionaddonratecarditemlink/where.go
- openmeter/ent/db/subscriptionaddonratecarditemlink/subscriptionaddonratecarditemlink.go
- openmeter/ent/db/paginate.go
- openmeter/ent/db/subscriptionaddonratecard_create.go
- openmeter/ent/db/migrate/schema.go
- openmeter/ent/schema/subscription_addon.go
- openmeter/ent/db/subscriptionaddonratecard/where.go
- openmeter/ent/db/subscriptionaddonratecard.go
- openmeter/ent/db/subscriptionaddonratecarditemlink_create.go
- openmeter/ent/db/setorclear.go
- openmeter/ent/db/subscriptionaddonratecarditemlink_query.go
- openmeter/ent/db/subscriptionaddonratecard_update.go
- openmeter/ent/db/subscriptionaddonratecard_query.go
- openmeter/ent/db/hook/hook.go
- openmeter/ent/db/addonratecard_update.go
- openmeter/ent/db/subscriptionitem_update.go
- openmeter/ent/db/subscriptionaddonratecarditemlink_update.go
🧰 Additional context used
🧬 Code Graph Analysis (23)
openmeter/subscription/testutils/compare.go (1)
openmeter/subscription/subscriptionspec.go (2)
CreateSubscriptionItemPlanInput(553-557)CreateSubscriptionItemCustomerInput(598-602)
openmeter/subscription/addon/repo/subscriptionaddon.go (1)
openmeter/ent/db/addon_query.go (1)
AddonQuery(24-37)
openmeter/subscription/timing.go (1)
openmeter/subscription/state.go (1)
SubscriptionActionChangeAddons(42-42)
openmeter/server/router/subscriptionaddon.go (4)
openmeter/server/router/router.go (1)
Router(194-216)openmeter/subscription/addon/http/list.go (1)
ListSubscriptionAddonsParams(17-19)openmeter/subscription/addon/http/create.go (1)
CreateSubscriptionAddonParams(16-18)openmeter/subscription/addon/http/get.go (1)
GetSubscriptionAddonParams(15-18)
openmeter/subscription/addon/diff/addon.go (2)
openmeter/subscription/apply.go (2)
NewAppliesToSpec(35-39)ApplyContext(13-15)pkg/models/error.go (1)
NewGenericNotImplementedError(171-173)
openmeter/subscription/addon/ratecard.go (3)
openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/predicate/predicate.go (1)
AddonRateCard(24-24)openmeter/ent/schema/addon.go (5)
AddonRateCard(87-89)AddonRateCard(91-95)AddonRateCard(97-111)AddonRateCard(113-125)AddonRateCard(127-140)
openmeter/subscription/addon/compatible.go (2)
openmeter/productcatalog/ratecard.go (1)
RateCard(29-41)openmeter/productcatalog/entitlement.go (1)
EntitlementTemplate(34-39)
cmd/server/wire_gen.go (1)
app/common/subscription.go (1)
NewSubscriptionServices(41-112)
openmeter/subscription/patch/patch_test.go (2)
openmeter/subscription/subscriptionspec.go (1)
CreateSubscriptionItemPlanInput(553-557)openmeter/productcatalog/ratecard.go (2)
RateCard(29-41)RateCardMeta(52-83)
openmeter/subscription/addon/service/service.go (1)
pkg/models/error.go (1)
NewGenericValidationError(138-140)
cmd/jobs/internal/wire_gen.go (3)
app/common/productcatalog.go (1)
NewAddonService(74-94)cmd/jobs/internal/wire.go (1)
Application(35-62)app/common/subscription.go (1)
NewSubscriptionServices(41-112)
cmd/billing-worker/wire_gen.go (2)
app/common/productcatalog.go (1)
NewAddonService(74-94)app/common/subscription.go (1)
NewSubscriptionServices(41-112)
openmeter/server/router/router.go (5)
openmeter/subscription/workflow/service.go (1)
Service(12-19)openmeter/subscription/service.go (1)
Service(10-31)openmeter/subscription/addon/http/handler.go (3)
Handler(17-21)NewHandler(31-36)HandlerConfig(23-29)pkg/framework/transport/httptransport/options.go (1)
WithErrorHandler(29-33)pkg/framework/transport/httptransport/handler.go (1)
ErrorHandler(73-75)
openmeter/subscription/addon/http/handler.go (6)
openmeter/subscription/addon/http/create.go (1)
CreateSubscriptionAddonHandler(24-24)openmeter/subscription/addon/http/list.go (1)
ListSubscriptionAddonsHandler(24-24)openmeter/subscription/addon/http/get.go (1)
GetSubscriptionAddonHandler(24-24)openmeter/subscription/workflow/service.go (1)
Service(12-19)pkg/framework/transport/httptransport/options.go (1)
HandlerOption(19-21)pkg/framework/commonhttp/errors.go (1)
NewHTTPError(35-41)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (2)
SubscriptionAddonAddon(5730-5742)SubscriptionAddonAddonUpdate(5745-5745)api/client/go/client.gen.go (2)
SubscriptionAddonAddon(5278-5290)SubscriptionAddonAddonUpdate(5293-5293)
openmeter/subscription/addon/extend_test.go (5)
openmeter/productcatalog/ratecard.go (3)
RateCardMeta(52-83)FlatFeeRateCard(211-218)RateCard(29-41)openmeter/subscription/testutils/feature.go (1)
ExampleFeatureKey(13-13)openmeter/productcatalog/price.go (3)
NewPriceFrom(363-385)InAdvancePaymentTerm(20-20)InArrearsPaymentTerm(21-21)openmeter/productcatalog/entitlement.go (5)
EntitlementTemplate(34-39)NewEntitlementTemplateFrom(230-246)BooleanEntitlementTemplate(393-396)StaticEntitlementTemplate(352-360)MeteredEntitlementTemplate(253-273)openmeter/testutils/time.go (1)
GetISODuration(19-26)
openmeter/subscription/testutils/helpers.go (8)
openmeter/subscription/testutils/service.go (1)
SubscriptionDependencies(38-52)openmeter/productcatalog/plan/service.go (1)
CreatePlanInput(98-101)openmeter/subscription/workflow/service.go (2)
CreateSubscriptionWorkflowInput(21-25)ChangeSubscriptionWorkflowInput(27-32)openmeter/subscription/timing.go (1)
Timing(14-17)openmeter/productcatalog/addon/service.go (1)
CreateAddonInput(98-101)openmeter/subscription/addon/quantity.go (1)
CreateSubscriptionAddonQuantityInput(25-28)openmeter/subscription/addon/addon.go (1)
CreateSubscriptionAddonInput(105-112)openmeter/productcatalog/effectiveperiod.go (1)
EffectivePeriod(19-25)
openmeter/ent/db/addonratecard_query.go (6)
openmeter/ent/db/ent.go (2)
QueryContext(65-65)Interceptor(68-68)openmeter/ent/db/addonratecard/addonratecard.go (1)
OrderOption(143-143)openmeter/ent/db/addonratecard.go (2)
AddonRateCard(21-63)AddonRateCard(99-122)openmeter/ent/db/predicate/predicate.go (1)
AddonRateCard(24-24)openmeter/ent/db/addon_query.go (1)
AddonQuery(24-37)openmeter/ent/db/feature_query.go (1)
FeatureQuery(24-37)
openmeter/subscription/addon/diff/apply_test.go (5)
openmeter/subscription/testutils/helpers.go (3)
CreateSubscriptionFromPlan(18-36)CreateAddonForSubscription(68-86)CreateMultiInstanceAddonForSubscription(39-64)openmeter/subscription/testutils/plan.go (2)
GetExamplePlanInput(72-80)BuildTestPlan(51-70)openmeter/subscription/testutils/addon.go (2)
GetExampleAddonInput(89-91)BuildAddonForTesting(16-41)openmeter/productcatalog/effectiveperiod.go (1)
EffectivePeriod(19-25)openmeter/testutils/time.go (1)
GetISODuration(19-26)
openmeter/ent/db/subscriptionitem_query.go (7)
openmeter/ent/db/ent.go (2)
QueryContext(65-65)Interceptor(68-68)openmeter/ent/db/subscriptionitem/subscriptionitem.go (1)
OrderOption(159-159)openmeter/ent/db/predicate/predicate.go (1)
SubscriptionItem(239-239)openmeter/ent/db/subscriptionitem.go (2)
SubscriptionItem(21-73)SubscriptionItem(120-147)openmeter/ent/db/subscriptionphase_query.go (1)
SubscriptionPhaseQuery(24-37)openmeter/ent/db/entitlement_query.go (1)
EntitlementQuery(26-41)openmeter/ent/db/billinginvoiceline_query.go (1)
BillingInvoiceLineQuery(29-50)
app/common/subscription.go (4)
openmeter/subscription/addon/repo/subscriptionaddon.go (1)
NewSubscriptionAddonRepo(23-27)openmeter/subscription/addon/repo/subscriptionaddonquantity.go (1)
NewSubscriptionAddonQuantityRepo(18-22)openmeter/subscription/testutils/service.go (1)
NewService(54-178)openmeter/subscription/workflow/service/service.go (1)
WorkflowServiceConfig(11-18)
api/client/go/client.gen.go (8)
api/client/javascript/src/client/schemas.ts (4)
Addon(9423-9423)SubscriptionAddonAddon(9869-9870)AddonInstanceType(9425-9425)SubscriptionAddonAddonUpdate(9871-9872)api/api.gen.go (4)
Addon(776-828)SubscriptionAddonAddon(5730-5742)AddonInstanceType(856-856)SubscriptionAddonAddonUpdate(5745-5745)openmeter/ent/db/predicate/predicate.go (1)
Addon(10-10)openmeter/ent/db/subscriptionitem/where.go (1)
Key(105-107)openmeter/ent/db/plan/where.go (2)
Key(99-101)Version(109-111)openmeter/ent/db/planphase/where.go (1)
Key(100-102)openmeter/ent/db/customer/where.go (1)
Key(137-139)openmeter/ent/db/subscriptionphase/where.go (1)
Key(94-96)
api/api.gen.go (8)
api/client/javascript/src/client/schemas.ts (4)
Addon(9423-9423)SubscriptionAddonAddon(9869-9870)AddonInstanceType(9425-9425)SubscriptionAddonAddonUpdate(9871-9872)openmeter/ent/db/predicate/predicate.go (1)
Addon(10-10)api/client/go/client.gen.go (4)
Addon(696-748)SubscriptionAddonAddon(5278-5290)AddonInstanceType(776-776)SubscriptionAddonAddonUpdate(5293-5293)openmeter/ent/db/subscriptionitem/where.go (1)
Key(105-107)openmeter/ent/db/plan/where.go (2)
Key(99-101)Version(109-111)openmeter/ent/db/planphase/where.go (1)
Key(100-102)openmeter/ent/db/customer/where.go (1)
Key(137-139)openmeter/ent/db/subscriptionphase/where.go (1)
Key(94-96)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (161)
tools/migrate/migrations/20250416094534_delete-subsadd-ratecards.up.sql (1)
1-4: Schema cleanup looks good.The migration properly drops the tables related to subscription addon rate cards as part of the broader refactoring to remove this feature from the codebase. The order of operations is correct - first dropping the link table that depends on the main table, then dropping the main table itself.
tools/migrate/migrations/20250416094534_delete-subsadd-ratecards.down.sql (1)
1-34: Down migration correctly recreates tables.The down migration script properly recreates both tables with all necessary schema elements (primary keys, foreign keys, constraints, and indexes). The order is reversed from the up migration - first creating the main table, then the link table that depends on it. This ensures a clean rollback if needed.
pkg/models/error.go (1)
192-192: Documentation fix approved.The comment correction accurately describes the function's behavior, now correctly stating that it returns true if the error is a
GenericNotImplementedError.cmd/server/main.go (1)
176-176: Integration of SubscriptionAddonService looks good.Adding the subscription addon service to the router configuration properly integrates it into the application's HTTP handling pipeline, allowing subscription addon functionality to be exposed via the API.
openmeter/subscription/workflow/service/service.go (2)
5-7: LGTM: Import added for subscription addon package.The import for the subscription addon package is correctly added to support the new dependency.
11-14: LGTM: Added AddonService field to WorkflowServiceConfig.The
WorkflowServiceConfigstruct is appropriately extended with anAddonServicefield of typesubscriptionaddon.Service. This change enables the workflow service to interact with subscription addons, which aligns with the PR objective of implementing the subscription addon creation flow.cmd/server/wire_gen.go (1)
251-251: LGTM: Updated dependency injection for subscription services.The
addonServiceparameter is correctly added to thecommon.NewSubscriptionServicesfunction call, ensuring the addon service is properly wired into the subscription services. This change is consistent with the function signature inapp/common/subscription.goand ensures the addon service dependency is available throughout the subscription workflow.openmeter/subscription/testutils/compare.go (1)
178-179: Improved error messages for test assertions.The error messages for assertion failures now include the item key and phase key, providing better context for diagnosing test failures when comparing subscription item inputs.
openmeter/subscription/addon/ratecard.go (1)
1-9: LGTM: Clean implementation of the new SubscriptionAddonRateCard structThe struct is simple, well-organized, and correctly imports the required package. The JSON tag is properly defined. This aligns with the broader refactoring that replaces the previous entity-based implementation with a lightweight wrapper.
openmeter/subscription/state.go (2)
36-43: New subscription action added correctlyThe new
SubscriptionActionChangeAddonsconstant is properly added and follows the same naming and formatting conventions as existing action constants.
87-89: State machine correctly configured for addon changesThe state machine configuration is appropriately updated to allow the new
SubscriptionActionChangeAddonsaction as a reentry transition when a subscription is active. This ensures addon changes can be made without changing the subscription's active status.openmeter/subscription/timing.go (1)
174-181: Timing validation appropriately handles addon changesThe timing validation logic correctly validates the new
SubscriptionActionChangeAddonsaction by ensuring the subscription view isn't nil, while allowing any timing option for addon changes. This implementation aligns with the broader subscription addon workflow.openmeter/subscription/addon/repo/subscriptionaddon.go (1)
117-128: Improved eager loading with nested relationshipGood refactoring of the query to use nested loading of rate cards through the addon relation instead of loading them separately. This change:
- Improves query efficiency
- Aligns with the broader removal of direct
SubscriptionAddonRateCardentities- Maintains the same functionality while simplifying the data access pattern
This approach properly loads rate cards through the addon relationship hierarchy.
openmeter/subscription/service/sync_test.go (1)
317-320: Updated function callback signature to match interface changes.The return type of the callback function has been updated to return
(productcatalog.RateCardMeta, error)instead of justproductcatalog.RateCardMeta, which aligns with changes to theChangeMetamethod in theRateCardinterface. This ensures error propagation is handled properly.openmeter/subscription/patch/patch_test.go (2)
51-54: Updated function callback signature to match interface changes.The callback now correctly returns both the metadata and an error value, aligning with the updated interface for
ChangeMeta.
112-115: Updated function callback signature to match interface changes.The callback now correctly returns both the metadata and an error value, aligning with the updated interface for
ChangeMeta.cmd/billing-worker/wire_gen.go (1)
231-241: Added initialization of addon service for subscription workflow.The addon service is now properly initialized and passed to the subscription services initialization, which aligns with the PR's objective of implementing the subscription addon creation flow. The error handling pattern correctly follows the existing pattern in the file.
openmeter/subscription/addon/diff/addon.go (1)
73-76: Improved error handling by replacing panic with structured error.This change appropriately replaces a panic with a proper error return mechanism, making the code more robust and maintainable. Using
models.NewGenericNotImplementedErrorprovides a consistent error type that can be handled by callers.openmeter/subscription/addon/README.md (2)
11-11: Documentation update reflects architectural change.The relationship description has been updated to indicate that rate cards are now "calculated from Addon" rather than directly stored, which aligns with the removal of subscription addon rate card entities from the codebase.
37-40: Good addition of quirks documentation.The new "Quirks" section provides valuable information about potential edge cases in feature resolution. This helps developers understand expected behavior when addons create new subscription items.
cmd/jobs/internal/wire_gen.go (1)
237-247: Proper dependency initialization for subscription addon service.The code now correctly initializes the addon service before passing it to the subscription service. The error handling and cleanup logic is thorough and follows the same pattern as other service initializations in this file.
openmeter/subscription/testutils/service.go (1)
156-161: Test workflow service updated to include addon service dependency.The workflow service initialization has been properly updated to include the subscription addon service as a dependency, which aligns with the changes in the main codebase. This ensures tests will correctly reflect the production behavior.
openmeter/server/router/subscriptionaddon.go (4)
6-7: Import looks goodThe import for the subscription addon HTTP driver looks appropriate.
12-14: Implementation looks correctThe ListSubscriptionAddons method correctly delegates to the handler with the appropriate parameters. This implementation follows the router pattern consistently used throughout the codebase.
20-22: Implementation looks correctThe CreateSubscriptionAddon method correctly delegates to the handler with the appropriate parameters.
28-31: Implementation looks correctThe GetSubscriptionAddon method correctly delegates to the handler with the appropriate parameters, passing both the subscription ID and subscription addon ID.
openmeter/subscription/addon/compatible.go (3)
11-13: Key validation looks goodThe key validation ensures that rate cards with different keys can't be combined, which is important for maintaining data integrity.
18-31: Price type validation looks goodThe validation for price types and payment terms is thorough and properly handles the special case for flat price types.
33-35: Entitlement template validation looks goodThe validation for entitlement template types ensures only compatible templates can be combined.
api/spec/src/productcatalog/subscriptionaddon.tsp (1)
19-19: Updated to use the new modelThe addon property now references the new SubscriptionAddonAddon model instead of an inline definition, which improves code modularity.
openmeter/ent/db/addonratecard.go (1)
73-73: Removed subscription addon rate cards edgeThe loadedTypes array size has been reduced from 3 to 2 boolean values, reflecting the removal of the subscription_addon_rate_cards edge. This aligns with the broader refactoring to eliminate the subscription addon rate card feature from the codebase.
test/billing/subscription_suite.go (5)
24-25: Good addition of addon repository imports.The new imports for addon repository and service are appropriately added to support the subscription addon functionality.
33-35: Proper import organization for subscription addon components.These imports correctly add the necessary subscription addon modules (domain model, repository, and service) that will be needed for the test suite.
48-48: Well-structured addition of SubscriptionAddonService to the mixin.The SubscriptionAddonService field is appropriately added to the SubscriptionMixin struct to make the addon service accessible in tests.
135-159: Correctly implemented addon initialization in SetupSuite.The code properly initializes:
- The addon repository with necessary configuration
- The addon service with proper dependencies
- The subscription addon repositories
- The subscription addon service with all required dependencies
This initialization follows the same pattern as other services in the test suite and ensures proper dependency injection.
168-168: Proper dependency injection of addon service into workflow.The SubscriptionAddonService is correctly passed to the workflow service, ensuring that the workflow can orchestrate operations involving subscription addons.
app/common/subscription.go (6)
10-11: Well-organized imports for addon functionality.The imports are properly structured to include all necessary addon-related components: the product catalog addon domain model, subscription addon domain model, repository, and service.
Also applies to: 17-19
33-39: The TODO comment should be addressed.There's a TODO comment to "break up to multiple initializers" that should be addressed, as this initialization is becoming increasingly complex with the addition of addons.
Consider breaking this large initializer into smaller, more focused initializers for better maintainability. This could be tracked separately but should be addressed in a future PR.
48-49: Good addition of addon service parameter.The addonService parameter is properly added to the NewSubscriptionServices function, maintaining consistency with how other services are injected.
72-82: Well-structured initialization of subscription addon components.The code correctly:
- Initializes the subscription addon repositories
- Creates a new subscription addon service with proper configuration
- Injects all necessary dependencies (transaction manager, logger, addon service, subscription service, repositories)
This follows the established pattern for service initialization in the application.
84-89: Proper injection of addon service into workflow service.The addon service is correctly passed to the subscription workflow service, allowing the workflow to coordinate operations involving subscription addons.
106-111: Complete wiring of addon service in return value.The subscription addon service is correctly included in the returned SubscriptionServiceWithWorkflow struct, making it accessible to consumers of this initialization function.
openmeter/subscription/addon/addon.go (3)
6-7: Good addition of time import for new functionality.The time package is appropriately imported to support the new GetInstanceAt method that accepts a time parameter.
32-47: Well-implemented GetInstanceAt method.This new method provides a convenient way to retrieve the subscription addon instance active at a specific time, which is helpful for time-based operations and queries. The implementation is clean and follows the pattern of other time-based retrieval methods.
111-112: Simplification of the CreateSubscriptionAddonInput structure.Removing the RateCards field from the input aligns with the architectural change to rely on the addon's own rate cards rather than managing them separately. This simplifies the API and reduces duplication.
openmeter/ent/db/subscriptionaddon.go (4)
53-53: Correct adjustment of loadedTypes array size.The loadedTypes array size is properly reduced from [4]bool to [3]bool following the removal of the RateCards edge. This ensures the array size matches the actual number of edges.
70-74: Proper index update in QuantitiesOrErr method.The index used to check if the Quantities edge was loaded is correctly updated from [2] to [1] after the removal of the RateCards edge.
79-85: Correct index update in AddonOrErr method.The index used to check if the Addon edge was loaded is properly updated from [3] to [2] after the removal of the RateCards edge.
1-243:Details
✅ Verification successful
Verify consistent removal of rate cards across the entire entity code.
The rate cards edge has been removed from the SubscriptionAddonEdges struct and related methods have been adjusted. However, it would be good to verify that all related code (predicates, where clauses, etc.) has been consistently updated throughout the generated Ent code.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to rate cards in the subscription addon entity code grep -r "rate_?cards\|RateCards" --include="*.go" openmeter/ent/db/subscriptionaddon* || echo "No remaining references found"Length of output: 672
Rate Cards Removal Consistency Verified
The removal of the rate cards edge and its associated code from the
SubscriptionAddonentity has been fully validated. A thorough search across all related files (including predicates, where clauses, and query methods) returned no remaining references, which confirms that the changes have been consistently applied throughout the generated Ent code.openmeter/subscription/addon/diff/affected.go (1)
43-45: LGTM!Good use of the
lolibrary for deduplicating map values.openmeter/ent/db/subscriptionitem.go (1)
85-85: LGTM!The change to adjust the
loadedTypesarray size from 4 to 3 elements is consistent with the removal of the "subscription_addon_rate_card_items" edge from the database schema.openmeter/subscription/addon/service/service.go (2)
47-47: LGTM!Good improvement to use
models.NewGenericValidationErrorfor input validation errors, providing better error classification.
68-74: LGTM!This change simplifies the subscription retrieval to just get the view of a subscription, which aligns with removing the rate card functionality while still validating that the subscription exists.
openmeter/subscription/workflow/service.go (2)
18-18: LGTM!Good addition of the
AddAddonmethod to the workflow service interface to support the subscription addon creation flow.
34-54: LGTM!Well-structured input type with appropriate validation rules that ensure the addon ID is provided and the initial quantity is positive.
openmeter/subscription/workflow/service/addon.go (12)
20-23: No concerns with the new AddAddon method signature
The signature is well-defined to accept the needed subscription ID and workflow input.
24-26: Input validation usage is consistent
Ensuring early validation helps maintain clear error handling.
50-69: Restoring subscription spec approach
Reverting the spec to a clean state before applying new diffs ensures consistent data. This logic is sound and reduces the risk of compounding errors.
70-80: Timing validation logic
Validating the timing before applying the addon ensures that changes align with the subscription lifecycle.
81-97: Creation of subscription addon
The logic correctly handles the creation outcome and checks for a nil return to prevent unexpected nil pointer issues.
98-106: Reverting and syncing diffs
Reapplying the restored diffs helps maintain consistent subscription states. This step is crucial for correct billing calculations.
107-117: Applying diff from the newly created addon
Applying the new addon's diff ensures the spec reflects the latest state. Good use of domain logic for subscription changes.
118-127: Subscription update after diff reapplication
Calling Update on the subscription after finalizing diffs is a sensible approach to keep database records aligned with memory.
128-132: Storing final purchase result
Returning the updated subscription view and newly created addon together is convenient for downstream usage.
133-135: Error propagated from transaction
Gracefully returning the transactional error ensures clear failure states if anything goes awry.
137-138: Final return statement
Clean and straightforward return for the success path.
145-160:Details
✅ Verification successful
Ensure hasAddons function is used consistently
This helper function accurately detects non-zero quantity addons. Double-check it's used in all places where the logic is needed. Otherwise some calls might inadvertently skip this check.
🏁 Script executed:
#!/bin/bash # Searching for references to hasAddons function to confirm usage. rg -A 3 "hasAddons"Length of output: 800
Action: Consistently Apply hasAddons in Subscription Workflows
The
hasAddonshelper function correctly detects non-zero quantity addons. Our search confirms that it’s defined inopenmeter/subscription/workflow/service/addon.goand is consistently used inopenmeter/subscription/workflow/service/subscription.gowhere the check is required. Please continue to ensure that any new or related flows that handle addons integrate this function to maintain uniform logic across the codebase.openmeter/productcatalog/ratecard.go (2)
37-37: Improved error handling in ChangeMeta
Changing the signature to include an error return is beneficial for controlling validations within the meta update.
228-233: ChangeMeta implementation for FlatFeeRateCard
This is a straightforward pass-through that properly handles the error returned from the callback. Looks good!openmeter/subscription/addon/service/create_test.go (1)
203-203: Reference to RateCards appears to contradict the summary
The summary mentions removing references to rate cards, but this test still checks for them.Likely an incorrect or invalid review comment.
openmeter/subscription/addon/http/handler.go (3)
17-21: Clean interface design for subscription addon HTTP operations.The Handler interface is well-defined with clear method signatures that correspond to the specific operations (create, list, get). This follows good API design principles by exposing only the necessary operations and returning specific handler types that can be found in other files.
23-29: Proper dependency injection pattern in HandlerConfig.The configuration struct follows good dependency injection practices by explicitly declaring all required dependencies. This approach makes the handler testable and the dependencies clear. The inclusion of both the addon-specific service and the workflow service indicates proper separation of concerns in the architecture.
1-50: Overall implementation aligns well with the PR objectives.The implementation provides a well-structured foundation for managing subscription addons through HTTP endpoints, which aligns with the PR objectives. The design follows clean architecture principles with proper separation of concerns, making it maintainable and extensible for future enhancements mentioned in the PR description, such as plan-addon linking.
api/client/go/client.gen.go (5)
5235-5241: Refactored Addon field to use named struct typeThe Addon field has been changed from an embedded anonymous struct to the named struct type
SubscriptionAddonAddon. This change improves type safety and readability.
5274-5294: New type definitions for SubscriptionAddonAddon structuresThese new type definitions extract the subscription addon properties into reusable named types:
SubscriptionAddonAddon: A struct containing basic addon reference data (id, instanceType, key, version)SubscriptionAddonAddonUpdate: A type alias for map[string]interface{} for flexible updatesThis refactoring aligns with the removal of subscription addon rate cards by providing a cleaner interface for addon references without the rate card dependencies.
5301-5307: Updated Addon field in SubscriptionAddonCreateSimilar to the earlier change, the Addon field in the SubscriptionAddonCreate struct now uses the named SubscriptionAddonAddon type, providing consistency across related structures.
5349-5355: Updated Addon field in SubscriptionAddonUpdateThe Addon field now uses a pointer to SubscriptionAddonAddonUpdate, maintaining the optional nature of this field (with omitempty). This change provides a flexible update mechanism while maintaining type consistency.
36753-37287: Updated Swagger specificationThe embedded Swagger specification has been updated to reflect the structural changes made to the API types. This ensures that the client documentation and code generation remain in sync with the actual implementation.
openmeter/server/router/router.go (5)
54-55: Properly added imports for subscription addon functionality.These imports provide the necessary subscription addon types and HTTP driver packages required for the added functionality.
105-105: Good addition of SubscriptionAddonService to Config struct.This field allows the router to accept the subscription addon service as a dependency.
179-189: Properly validates subscription service dependencies.The validation checks ensure that all required subscription-related services (workflow, service, and addon) are provided before the router is initialized.
204-204: Added subscription addon handler field to Router struct.This field will hold the initialized subscription addon HTTP handler.
365-374: Well-structured subscription addon handler initialization.The handler is properly initialized with all required dependencies:
- SubscriptionAddonService for addon operations
- SubscriptionWorkflowService for workflow operations
- SubscriptionService for core subscription functionality
- NamespaceDecoder for multi-tenancy support
- Logger for structured logging
- Error handler for consistent error management
The implementation follows the same pattern as other handlers in the router.
openmeter/subscription/workflow/service/subscription_test.go (5)
63-89: Good test coverage for subscription item annotations.This test ensures that all created subscription items have the proper ownership annotations, which is essential for tracking item ownership and origin within the system.
277-277: Properly added AddonService dependency to workflow service.The workflow service now correctly includes the AddonService dependency, which is necessary for the subscription addons functionality.
977-977: Added SubsDeps field to test dependencies struct.This field provides access to subscription-related dependencies in test cases, allowing for better organization and reuse of setup code.
1079-1082: Updated ChangeMeta signature to include error return.The function signature now properly returns an error alongside the modified meta, which improves error handling.
1015-1049: Excellent test for addon-related edit restrictions.This test verifies a key constraint mentioned in the PR objectives: a subscription with an addon should not be editable. The test:
- Creates a subscription
- Adds an addon to the subscription
- Attempts to edit the subscription
- Verifies that the edit fails with a GenericForbiddenError
This test case ensures that the implementation correctly enforces the business rule that subscriptions with addons cannot be edited.
api/api.gen.go (6)
5687-5693: Type change improves structure clarity.The change from an inline anonymous struct to a dedicated
SubscriptionAddonAddontype for theAddonfield enhances code readability and type consistency.
5729-5742: Good extraction of the add-on properties into a dedicated type.Centralizing the add-on properties in a dedicated type improves code organization and reuse. The extracted fields (
Id,InstanceType,Key, andVersion) match the core properties needed from the completeAddontype (as seen in the relevant code snippets).
5744-5745: Type alias for addon updates provides flexibility.Defining
SubscriptionAddonAddonUpdateas a map type allows for flexible partial updates while maintaining type information in the API.
5753-5756: Consistent type usage in creation model.Good to see consistent use of the new
SubscriptionAddonAddontype across related structs, including here in theSubscriptionAddonCreatemodel.
5801-5804: Proper pointer usage for optional update fields.Using a pointer to
SubscriptionAddonAddonUpdatewith theomitemptytag is appropriate for this update field, as it allows clients to indicate whether they want to update the addon info or leave it unchanged.
16726-17285: Updated Swagger specification aligns with code changes.The API specification has been updated to reflect the new type structure. This ensures consistency between the implementation and API documentation.
openmeter/subscription/addon/http/get.go (2)
15-24: Well-structured type definitions for API consistency.The type definitions follow a clean pattern with proper separation of URL parameters, internal request structure with namespaced IDs, and response types. This maintains consistency with the rest of the codebase's HTTP handlers.
27-59: Verify that the subscription addon belongs to the specified subscription.The handler correctly retrieves both the subscription addon and subscription view, but doesn't explicitly verify that the addon belongs to the specified subscription. This could potentially allow retrieving an addon from one subscription while specifying a different subscription ID in the URL.
Consider modifying the service call to verify this relationship:
- res, err := h.SubscriptionAddonService.Get(ctx, req.SubscriptionAddonID) + res, err := h.SubscriptionAddonService.Get(ctx, req.SubscriptionID, req.SubscriptionAddonID)Or add a validation check after retrieving both resources:
view, err := h.SubscriptionService.GetView(ctx, req.SubscriptionID) if err != nil { return GetSubscriptionAddonResponse{}, err } + // Verify addon belongs to the subscription + if res.SubscriptionID != req.SubscriptionID { + return GetSubscriptionAddonResponse{}, fmt.Errorf("subscription addon does not belong to the specified subscription") + } + return MapSubscriptionAddonToResponse(view, *res)api/client/javascript/src/client/schemas.ts (6)
8361-8361: LGTM: Proper refactoring to use a dedicated type.The change replaces an inline addon definition with a reference to the new dedicated
SubscriptionAddonAddonschema. This improves maintainability and type consistency throughout the codebase.
8408-8432: LGTM: Well-defined schema with proper documentation.The newly introduced
SubscriptionAddonAddonschema properly encapsulates addon properties with appropriate JSDoc comments, making the schema more maintainable and reusable. This follows good TypeScript practices by creating a dedicated type rather than repeating the same structure in multiple places.
8433-8434: Verify empty record type is sufficient for updates.The
SubscriptionAddonAddonUpdatetype is defined as an empty record (Record<string, never>), which suggests no properties can be updated on an addon. Ensure this aligns with your intended update behavior.This differs from the Go implementation which uses a flexible
map[string]interface{}for updates. If addon properties should be updatable, consider using a partial type instead:-SubscriptionAddonAddonUpdate: Record<string, never> +SubscriptionAddonAddonUpdate: Partial<components['schemas']['SubscriptionAddonAddon']>
8468-8468: LGTM: Consistent use of the new schema type.Good implementation using the dedicated
SubscriptionAddonAddontype in the SubscriptionAddonCreate schema.
8548-8548: LGTM: Optional update field properly typed.The addon field is correctly marked as optional using the
?modifier and references the appropriate update type. This ensures type safety when submitting partial updates.
8869-8872: LGTM: Proper type exports for the new schema types.The new type aliases are correctly exported, making them available for consumers of this module.
openmeter/subscription/addon/repo/mapping.go (3)
55-56: Model change: Rate cards now accessed through Addon entityThe subscription addon model has been refactored to access rate cards through the Addon entity rather than directly from the subscription addon. This aligns with the removal of the subscription addon rate card feature from the codebase.
73-90: Function updated to use AddonRateCard instead of SubscriptionAddonRateCardThe signature and implementation of this function have been updated to work with AddonRateCard entities instead of the now-removed SubscriptionAddonRateCard entities. This change is consistent with the overall refactoring to simplify the data model.
The implementation correctly:
- Handles nil entity cases
- Maps to the domain model using addonrepo.FromAddonRateCardRow
- Performs additional nil checking on the mapped value
95-97: Updated function signature to match entity type changeThis function signature has been updated to match the change from SubscriptionAddonRateCard to AddonRateCard entities, maintaining consistency with the changes to the underlying data model.
openmeter/subscription/addon/http/list.go (2)
16-25: Well-structured types for subscription addon listingThe types are clearly defined with appropriate namespacing and separation of concerns between parameter extraction, request model, and response model.
27-65: New handler implements complete subscription addon listing flowThis handler correctly:
- Resolves the namespace from the context
- Constructs a properly namespaced request
- Calls the service layer to fetch subscription addons
- Retrieves the subscription view for context
- Maps domain models to API responses
- Handles errors appropriately at each step
- Sets the correct HTTP status code
The implementation follows the project's handler pattern and maintains proper separation of concerns.
openmeter/ent/db/subscriptionaddon_query.go (1)
447-452: Updated loadedTypes array size to reflect removed edgeThe array size has been reduced from 4 to 3 elements, corresponding to the removal of the "rate_cards" edge from the SubscriptionAddon entity. This change is consistent with the broader refactoring to simplify the subscription addon model by removing the subscription addon rate card feature.
api/openapi.cloud.yaml (5)
19303-19386: Refactoring to reusable schema foraddonproperty is correct and improves maintainability.The change from inline object definitions to referencing the new
SubscriptionAddonAddonschema for theaddonproperty inSubscriptionAddonis correct, improves schema reuse, and aligns with best practices for OpenAPI specifications. The new schema is well-defined and encapsulates the required fields and constraints.
19428-19443: Refactoring to reusable schema foraddonproperty in creation model is correct.The use of the new
SubscriptionAddonAddonschema for theaddonproperty inSubscriptionAddonCreateis correct and consistent with the changes in the main model. This improves maintainability and ensures consistency across API models.
19522-19537: Refactoring to reusable schema foraddonproperty in update model is correct.The use of the new
SubscriptionAddonAddonUpdateschema for theaddonproperty inSubscriptionAddonUpdateis correct and prepares the API for future extensibility. The placeholder schema is acceptable for now.
19352-19387: NewSubscriptionAddonAddonschema is well-defined and appropriate.The new
SubscriptionAddonAddonschema is well-defined, with all required fields and constraints for referencing an add-on. This centralizes the definition and improves consistency across the API.
19388-19390: NewSubscriptionAddonAddonUpdateschema is a valid placeholder.The new
SubscriptionAddonAddonUpdateschema is currently empty, which is acceptable as a placeholder for future extensibility in update operations.openmeter/subscription/addon/extend.go (6)
7-11: Ensure consistent import usage and dependency management.The introduction of
"github.com/samber/lo"for pointer manipulation (line 7) and direct references to models and entitlement (lines 9,11) appear valid. However, verify that the introduction of these imports aligns with project conventions and that the library versions (if pinned) are compatible with the rest of the codebase.Do you want me to run a script to confirm there are no conflicting or outdated references to
lo,models, orentitlementin other files?
17-20: Validate pointer-based reflection approach.Checking
reflect.TypeOf(target)fornilis a sensible way to ensure the caller supplies a valid pointer. This guards against runtime errors when performing reflection-based modifications.
22-22: Enforce pointer usage for rate card changes.The kind check for
Ptris essential to ensureChangeMetacan be safely applied. This is a correct approach to prevent accidentally callingApplyon non-pointer rate cards.
26-27: Skip early when no price or entitlement template to merge.Early return if both
PriceandEntitlementTemplateare nil is efficient and clear. This avoids unnecessary operations when there is effectively nothing to apply.
30-32: Confirm rate card compatibility validation coverage.The call to
validateRateCards(a.AddonRateCard.RateCard, target)is pivotal for ensuring consistent rate card structures. Confirm that unit tests thoroughly cover scenarios of incompatible rate cards to prevent runtime errors downstream.
56-75: Metered entitlement merging is comprehensive but consider additional templates.The logic for merging the
IssueAfterResetfields vialo.ToPtris straightforward. If new entitlement types are introduced, ensure the switch statement is extended consistently. The distinct handling for boolean vs. metered is a good approach, but keep in mind future expansion needs.openmeter/subscription/addon/http/create.go (3)
1-14: Check request decoding and error handling consistency.The new package (
httpdriver) and imports look consistent with the project structure. Ensure that error-handling patterns (likefmt.Errorf) align with how your codebase typically wraps or logs contextual details.
15-25: Structure for request, response, and handler types is concise.Using clear typed structs (
CreateSubscriptionAddonParams,CreateSubscriptionAddonRequest, etc.) for the addon creation workflow is helpful. The typed approach improves maintainability and helps avoid confusion between different creation flows.
49-65: Leverage structured response encoding with adequate error reporting.Returning
def, errprovides a clear way to bubble up issues fromh.SubscriptionWorkflowService.AddAddon. The final mapping (MapSubscriptionAddonToResponse(view, add)) is straightforward. Optionally, consider logging creation success/failure for audit trails.api/openapi.yaml (5)
19687-19690: Refactoredaddonproperty to reusable component—good improvementThe
addonproperty inSubscriptionAddonnow references the newSubscriptionAddonAddoncomponent, which centralizes and enforces the structure for add-on info. This improves schema reuse, maintainability, and type safety. The change is consistent with the PR objectives and AI summary.
19812-19815: Consistent use of reusable component inSubscriptionAddonCreateThe
addonproperty inSubscriptionAddonCreatenow references the sameSubscriptionAddonAddoncomponent, ensuring consistency between creation and retrieval models. This is a best practice for OpenAPI schema design.
19906-19909: Correct use of update-specific component inSubscriptionAddonUpdateThe
addonproperty inSubscriptionAddonUpdatenow referencesSubscriptionAddonAddonUpdate, which is a type alias for update operations. This maintains clarity and separation between create/read and update models, and is a good API design pattern.
19736-19771: NewSubscriptionAddonAddoncomponent is well-definedThe new
SubscriptionAddonAddoncomponent is clearly defined with required fields (id,key,version,instanceType) and appropriate descriptions. This centralizes the add-on info structure and improves maintainability and validation.
19772-19773:SubscriptionAddonAddonUpdateis defined as a type alias—OKThe
SubscriptionAddonAddonUpdatecomponent is defined as an object with a description, serving as a type alias for update operations. This is acceptable for OpenAPI and code generation purposes.openmeter/ent/db/subscriptionitem_query.go (3)
26-33: Clean removal of subscription addon rate card references from the query struct.The changes correctly remove all subscription addon rate card related fields from the
SubscriptionItemQuerystruct as part of the broader effort to remove this feature from the codebase.
322-334: Properly updated clone method to match the modified struct fields.The
Clonemethod has been correctly updated to reflect the changes to the struct fields, maintaining consistency in the codebase.
448-452: Correctly updated loadedTypes array size.The
loadedTypesarray size has been updated from 4 to 3 elements to reflect the removal of the subscription addon rate card items edge, ensuring proper array initialization.openmeter/subscription/addon/diff/apply_test.go (3)
49-54: Good refactoring to use shared test utilities.The test now uses
subscriptiontestutilspackage functions instead of local helper functions, improving code reuse and maintainability.
533-534: Enhanced test coverage with annotation preservation validation.New assertion verifies that annotations are preserved after applying diffs, which is an important requirement when merging rate cards.
544-544: Consistent validation of annotation preservation.Similar to the earlier check, this assertion ensures annotations remain unchanged across different segments of the timeline, providing thorough test coverage.
openmeter/subscription/workflow/service/addon_test.go (5)
40-69: Comprehensive test for input validation.Good test case that verifies invalid inputs are properly rejected with appropriate validation errors, ensuring the API behaves correctly when receiving improper requests.
71-101: Test aligns with the PR's key constraint.This test properly verifies that when a subscription already has an addon, the system returns a NotImplemented response, which is one of the key constraints mentioned in the PR objectives.
103-150: Complete end-to-end test for successful addon creation.This test thoroughly validates that the subscription is correctly synchronized with the new addon's contents, including a manual verification of the expected changes using the diff and apply mechanisms.
152-180: Proper conflict detection test.The test correctly verifies that attempting to add the same addon twice results in a conflict error, ensuring data integrity in the system.
184-195: Useful helper function for test normalization.The
stripFeatureIDsfunction elegantly solves the comparison problem when feature IDs aren't known in advance, making the tests more robust and less brittle.openmeter/server/server_test.go (5)
53-53: Added import for subscription addon package.Proper inclusion of the subscription addon package import to support the new service interfaces and implementations.
479-479: Added NoopSubscriptionAddonService to test server setup.This change properly integrates the new subscription addon service into the test server setup, ensuring tests can run with the new functionality.
520-520: Properly configured router with the subscription addon service.The subscription addon service is correctly assigned to the router configuration, ensuring API routes can use the service.
1212-1214: Implemented AddAddon method in NoopSubscriptionWorkflowService.The workflow service interface implementation is correctly extended with the new AddAddon method, maintaining interface compatibility.
1216-1236: Added NoopSubscriptionAddonService implementation.Comprehensive no-op implementation of the subscriptionaddon.Service interface with all the required methods, providing a good foundation for testing.
openmeter/ent/db/addonratecard_query.go (3)
24-30: Confirmed removal of subscription addon rate cards edgeI can see the withSubscriptionAddonRateCards field has been removed from the AddonRateCardQuery struct, which aligns with the PR objective of removing the old subscription addon rate card entities and their linking tables.
411-414: Adjustment of loadedTypes array sizeThe loadedTypes array size has been correctly adjusted from 3 to 2 elements to reflect the removal of the subscription addon rate cards edge query capability.
298-305: Proper cleanup in Clone methodThe Clone method has been appropriately updated to remove the withSubscriptionAddonRateCards field, ensuring that cloned query builders don't attempt to retain references to the removed edge.
openmeter/subscription/addon/extend_test.go (9)
19-29: Good test setup with reusable test dataThe test file properly initializes common test data that will be reused across multiple test cases, making the tests more maintainable and readable.
31-48: Thorough validation testing for edge casesGood tests for handling nil or non-pointer targets, which are important edge cases that could lead to runtime panics if not handled properly.
50-98: Comprehensive type compatibility validation testsThese tests ensure proper validation when merging rate cards with incompatible price types, entitlement types, or keys, which is critical for maintaining data integrity.
100-119: Test for no-op scenario when nothing to mergeThis test verifies that when an addon rate card has no entitlement or price, the target rate card remains unchanged, which is an important boundary case.
121-145: Validation of payment term compatibilityGood test to ensure that addon rate cards with different payment terms cannot be merged, preventing potential billing inconsistencies.
147-170: Comprehensive price merging test casesThese tests thoroughly verify the price merging behavior, including:
- Preserving target price when addon has no price
- Adding addon price to target price (flat fee addition)
- Adding addon price when target has no price
This ensures the extend functionality correctly handles all price merging scenarios.
Also applies to: 172-198, 200-223
225-243: Thorough entitlement merging testsThese tests verify the entitlement merging behavior for different scenarios:
- Adding addon entitlement to target
- Preserving target entitlement when addon has none
- Correctly handling metered entitlements and combining their values
The test in lines 290-322 specifically verifies that issueAfterReset values are properly accumulated across multiple applies, which is critical for ensuring correct entitlement calculations.
Also applies to: 245-263, 265-322
325-367: Well-implemented test helper typeThe nonPointerRateCard type provides a clean way to test the pointer validation in the Apply method. All interface methods are appropriately implemented.
369-375: Useful test factory functionThe getTestAddonRateCard function encapsulates the creation of test objects, making the tests more readable and maintainable.
openmeter/subscription/addon/http/mapping.go (3)
20-34: Clean API request to workflow input mappingThe function properly maps the API request fields to the internal workflow input structure, including handling of optional metadata. This follows a clear and consistent mapping pattern.
56-71: Good error handling for rate card conversionThe implementation properly handles errors during rate card conversion and provides clear error messages that include the underlying cause, which will be helpful for troubleshooting.
73-101: Well-structured API response mappingThe function creates a comprehensive API response with all necessary fields, including metadata, timing information, and quantity details. The use of lo.EmptyableToPtr ensures that empty metadata is handled correctly.
openmeter/ent/db/client.go (3)
422-423: Clean removal of SubscriptionAddonRateCard referencesThe code removes references to SubscriptionAddonRateCard entity in the Use and Intercept methods, which is consistent with the PR's removal of the subscription addon rate card feature.
Also applies to: 443-444
7271-7272: Consistent removal in hooks and interceptors structuresThe SubscriptionAddonRateCard entity has been properly removed from the hooks and interceptors structures. This aligns with the overall objective of removing the subscription addon rate card feature from the codebase.
Also applies to: 7284-7285
1-7312:Details
❓ Verification inconclusive
Verify downstream impact of entity removal
While the entity removal from the database client is clean, the removal of SubscriptionAddonRateCard and related entities may impact other components that previously relied on these entities.
Consider thoroughly testing all subscription addon related functionality to ensure there are no regressions or unexpected behaviors.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the removed entities in the codebase echo "Checking for remaining references to SubscriptionAddonRateCard..." rg -i "SubscriptionAddonRateCard" --type go echo "Checking for remaining references to SubscriptionAddonRateCardItemLink..." rg -i "SubscriptionAddonRateCardItemLink" --type go echo "Checking database schema migration scripts for related tables..." rg -i "subscription_addon_rate_card" --type sqlLength of output: 14912
Action Required: Verify Downstream Impact from Residual References
The script output shows that multiple components—such as subscription addon modules, mapping functions, API generators, and even migration scripts—still reference SubscriptionAddonRateCard (and SubscriptionAddonRateCardItemLink). Although the ent client appears to have removed these entities from the database client layer (as evidenced by the migration scripts dropping the related tables), the higher-level code still depends on them.
Please ensure that:
- Any remaining references are either updated or refactored so that they no longer assume the presence of these entities.
- All subscription addon functionality (including tests, API mappings, and related domain logic) is thoroughly exercised to catch regressions or unexpected behaviors.
- Coordination is maintained with both the migration changes and the upper-layer domain code to ensure consistency across the stack.
GAlexIHU
force-pushed
the
galexi/addons-3
branch
from
125a828 to
044748f
Compare
GAlexIHU
force-pushed
the
galexi/addons-3
branch
from
044748f to
92cf0bc
Compare
2 participants
Overview
Implement create flow for subscription addons with the following constraints:
Notable contents:
restoreis implemented)Summary by CodeRabbit
New Features
Improvements
Removals
Tests
Chores