Subscription IsCustom #2414
Subscription IsCustom #2414
Conversation
📝 WalkthroughWalkthroughA comprehensive update adds a new Changes
✨ Finishing Touches
🪧 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/subs-edited-status
branch
from
a6d0ca1 to
c95aa7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
openmeter/subscription/subscription.go (1)
32-46: 💡 Verification agent🧩 Analysis chain
The AsEntityInput method doesn't propagate the new LastEditedAt field.
The
AsEntityInput()method doesn't include the newly addedLastEditedAtfield when creating aCreateSubscriptionEntityInput. This might lead to the edit timestamp being lost when converting between representations.
🏁 Script executed:
#!/bin/bash # Check if CreateSubscriptionEntityInput has a LastEditedAt field # and how this field is used in other parts of the codebase echo "Checking if CreateSubscriptionEntityInput struct has LastEditedAt field:" rg "type CreateSubscriptionEntityInput struct" -A 20 echo -e "\nChecking where AsEntityInput is used:" rg "AsEntityInput\(\)" -B 2 -A 2Length of output: 2667
Action Required: Propagate the New
LastEditedAtField in the Entity ConversionThe current implementation of the
AsEntityInputmethod inopenmeter/subscription/subscription.godoes not include the newLastEditedAtfield. This omission means that when converting aSubscriptioninto aCreateSubscriptionEntityInput, any updated timestamp indicating the last edit is not propagated, potentially resulting in stale edit metadata. To resolve this, please:
- Update the
CreateSubscriptionEntityInputstruct (located inopenmeter/subscription/repository.go) to include aLastEditedAtfield if it’s meant to track edit timestamps.- Modify the
AsEntityInputmethod to assigns.LastEditedAtto the corresponding field of the input struct.- Verify the changes wherever
AsEntityInputis used (e.g., inopenmeter/subscription/service/service.go) to ensure that the edit timestamp is correctly passed through the conversion.
🧹 Nitpick comments (10)
api/openapi.yaml (2)
15830-15841: Clear documentation for theisCustomproperty.
The added schema forisCustomis well documented, explaining that a subscription is custom if it either lacks a plan reference or has been edited. One minor nitpick: the description uses "it's original form" where the correct possessive is "its original form." Consider fixing this for grammatical accuracy.
16005-16016: Replication of theisCustomfield in the expanded schema.
The changes correctly integrate theisCustomfield into this part of the schema, mirroring the previous definition. The description is clear; however, as noted earlier, please update "it's original form" to "its original form" for grammatical correctness.tools/migrate/migrations/20250311123806_subscription-editedat.down.sql (1)
1-2: SQL Down Migration: Revert last_edited_at Column AdditionThe migration correctly drops the
last_edited_atcolumn from the "subscriptions" table as part of the rollback process. Consider verifying that there are no dependent queries or application logic that might break when the column is removed. Optionally, you might add anIF EXISTSclause to avoid errors if the column does not exist in some environments.api/client/javascript/src/client/schemas.ts (2)
7110-7113: Code looks good with a minor grammatical issueThe implementation of the
isCustomfield in the Subscription schema is well-documented and aligns with the PR objectives. The field correctly identifies custom subscriptions based on the specified criteria.There is a small grammatical error in the documentation:
- /** @description Whether the subscription is custom. A subscription is custom if - * - it was customized at creation (not based on a plan, has no PlanReference) - * - or if it has been edited (even if eventually edited back to it's original form) */ + /** @description Whether the subscription is custom. A subscription is custom if + * - it was customized at creation (not based on a plan, has no PlanReference) + * - or if it has been edited (even if eventually edited back to its original form) */
7232-7235: Same grammatical issue in SubscriptionExpanded schemaThe implementation for the SubscriptionExpanded schema looks good but contains the same grammatical error.
- /** @description Whether the subscription is custom. A subscription is custom if - * - it was customized at creation (not based on a plan, has no PlanReference) - * - or if it has been edited (even if eventually edited back to it's original form) */ + /** @description Whether the subscription is custom. A subscription is custom if + * - it was customized at creation (not based on a plan, has no PlanReference) + * - or if it has been edited (even if eventually edited back to its original form) */tools/migrate/migrations/20250311123806_subscription-editedat.up.sql (1)
1-3: New SQL Migration: Adding "last_edited_at" ColumnThe migration script correctly adds the new nullable "last_edited_at" column of type
timestamptzto the "subscriptions" table. This aligns with the PR objectives to store the last edit timestamp, which will serve as an indicator for custom subscriptions.Suggestions:
- Index Consideration: If the
last_edited_atcolumn is expected to be frequently queried (e.g., sorting or filtering by edit timestamps), consider adding an index on this column in a future migration.- Default Value: Confirm that having this column nullable (with no default value) fits the requirements for both new and existing subscriptions.
api/openapi.cloud.yaml (2)
15550-15561: Property Definition forisCustomThe new
isCustomproperty is clearly defined with typebooleanand a detailed description explaining its criteria. One minor suggestion: consider correcting the possessive typo in the description—change "it's original form" to "its original form" to improve clarity.
15725-15736: Schema Extension withisCustomfor Subscription MappingThe inclusion of the
isCustomfield in the schema—with its associated type and descriptive text—provides a clear and self-documenting mechanism for identifying custom subscriptions. Again, please consider updating the phrase "even if eventually edited back to it's original form" to "its original form" for grammatical correctness.api/api.gen.go (2)
4654-4657: Documentation comment has a minor typoThe documentation for the
IsCustomfield has a small grammatical error in the third bullet point. It should be "its" (possessive) rather than "it's" (contraction for "it is").// IsCustom Whether the subscription is custom. A subscription is custom if // - it was customized at creation (not based on a plan, has no PlanReference) -// - or if it has been edited (even if eventually edited back to it's original form) +// - or if it has been edited (even if eventually edited back to its original form) IsCustom bool `json:"isCustom"`
4749-4752: Same typo in the second documentation blockThis documentation comment has the same grammatical error as the previous one.
// IsCustom Whether the subscription is custom. A subscription is custom if // - it was customized at creation (not based on a plan, has no PlanReference) -// - or if it has been edited (even if eventually edited back to it's original form) +// - or if it has been edited (even if eventually edited back to its original form) IsCustom bool `json:"isCustom"`
📜 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 (25)
api/api.gen.go(5 hunks)api/client/go/client.gen.go(3 hunks)api/client/javascript/src/client/schemas.ts(2 hunks)api/openapi.cloud.yaml(4 hunks)api/openapi.yaml(4 hunks)api/spec/src/productcatalog/subscription.tsp(1 hunks)openmeter/ent/db/migrate/schema.go(3 hunks)openmeter/ent/db/mutation.go(10 hunks)openmeter/ent/db/setorclear.go(1 hunks)openmeter/ent/db/subscription.go(4 hunks)openmeter/ent/db/subscription/subscription.go(3 hunks)openmeter/ent/db/subscription/where.go(2 hunks)openmeter/ent/db/subscription_create.go(5 hunks)openmeter/ent/db/subscription_update.go(4 hunks)openmeter/ent/schema/subscription.go(1 hunks)openmeter/productcatalog/subscription/http/mapping.go(2 hunks)openmeter/subscription/repo/mapping.go(1 hunks)openmeter/subscription/repo/subscriptionrepo.go(1 hunks)openmeter/subscription/repository.go(1 hunks)openmeter/subscription/service/service.go(1 hunks)openmeter/subscription/service/workflowservice.go(1 hunks)openmeter/subscription/service/workflowservice_test.go(1 hunks)openmeter/subscription/subscription.go(2 hunks)tools/migrate/migrations/20250311123806_subscription-editedat.down.sql(1 hunks)tools/migrate/migrations/20250311123806_subscription-editedat.up.sql(1 hunks)
🔇 Additional comments (56)
api/openapi.yaml (2)
15753-15759: Insertion of the newisCustomfield in field listings.
The new fieldisCustomis correctly added in the field list alongside existing fields such asactiveFrom,status, andcustomerId. This ensures that clients are aware of the field as part of the model.
15931-15937: Consistent inclusion ofisCustomin the properties list.
The update in this section consistently includes theisCustomfield in the list of properties along withactiveFrom,status,customerId,currency, andphases. This maintains uniformity across the model definitions.api/openapi.cloud.yaml (2)
15473-15479: New Required Field Addition:isCustomin Object ListThe addition of the
- isCustomelement to the list ensures that this new field is explicitly required in the object. This aligns with the subscription logic whereby a subscription is considered custom if it either lacks a plan reference or has been edited. Verify that this change is consistent with your overall API design and that downstream consumers are aware of the new required property.
15651-15657: Consistent Inclusion ofisCustomin Required ListsThis hunk introduces
- isCustomconsistently within another related object’s required list. Ensure that markingisCustomas required for both objects is intentional and that any necessary documentation or client library adjustments are made to reflect this requirement.openmeter/ent/schema/subscription.go (1)
38-38: LGTM: Added last_edited_at field to track subscription editsThe new
last_edited_atfield is well-defined with appropriate modifiers:
Optional()allows the field to be omittedNillable()allows the field to be explicitly set to NULLThis implementation aligns with the PR objective of tracking when a subscription was last edited.
api/spec/src/productcatalog/subscription.tsp (1)
99-104: LGTM: Well-documented isCustom propertyThe new
isCustomboolean property is added with comprehensive documentation that clearly defines when a subscription is considered custom:
- When created without a plan reference
- When it has been edited at any point
This implementation matches the PR objectives and provides clear guidance for API consumers.
openmeter/subscription/service/workflowservice.go (1)
89-91: Good addition: Empty customizations validationThis validation check prevents processing empty customization lists, failing early with a clear error message. This is a good defensive programming practice that improves error handling in the workflow service.
openmeter/subscription/repository.go (1)
51-52: LGTM: New MarkEdited method for tracking subscription customizationsThe new
MarkEditedmethod provides a clean interface for marking subscriptions as custom when they're edited. This method:
- Takes a timestamp parameter to record when the edit occurred
- Returns the updated subscription
- Has a clear, concise description
This implementation supports the PR objective of tracking subscription edits.
openmeter/ent/db/subscription.go (5)
50-51: Well-structured field addition for tracking subscription edits.The
LastEditedAtfield addition is properly documented and follows the same pattern as other nullable time fields in the struct. This aligns with the PR objective to store the timestamp of the last edit made to a subscription.
124-125: Correctly updated scan values method to handle the new field.The
scanValuesmethod is appropriately updated to include the newLastEditedAtfield, ensuring it's handled as asql.NullTimetype during database operations.
231-237: Properly implemented value assignment for LastEditedAt.The assignment logic is consistent with other nullable time field implementations, correctly handling type checking, validity verification, and value assignment.
340-344: String method correctly updated to include LastEditedAt.The
Stringmethod is properly updated to include the new field in the string representation, with appropriate null checking and time formatting.
19-56:❌ Incorrect review comment
Missing IsCustom method in the Subscription struct.
The PR objectives specify that an
IsCustomfield/method should be added to determine whether a subscription is custom based on either lacking a plan reference or having an edit timestamp. This functionality appears to be missing from the implementation.Please verify this with the following script:
🏁 Script executed:
#!/bin/bash # Check if IsCustom method exists in other files in the codebase rg -A 5 "func \(\w+ (Subscription|subscription)\) IsCustom\(\)" --type goLength of output: 284
Clarification: The IsCustom method is not missing.
The necessary IsCustom functionality is implemented in the repository within the fileopenmeter/subscription/subscription.go:
- The method signature is:
func (s Subscription) IsCustom() bool { return s.LastEditedAt != nil || s.PlanRef == nil }- This implementation meets the PR objectives to determine if a subscription is custom (i.e., lacking a plan reference or having a non-nil edit timestamp).
Please update the documentation or references if needed to point to this implementation.
Likely an incorrect or invalid review comment.
openmeter/ent/db/subscription/subscription.go (3)
43-44: Appropriate field constant declaration.The constant for the new field follows the existing naming pattern and clearly documents its purpose.
101-101: Correctly added LastEditedAt to Columns slice.The field is properly added to the Columns slice, ensuring it's included in database operations.
205-208: Well-implemented ordering function for LastEditedAt.The
ByLastEditedAtfunction follows the same pattern as other ordering functions, allowing proper ordering by the new field in queries.openmeter/subscription/repo/mapping.go (1)
48-53: Correctly mapped LastEditedAt in the domain model.The field mapping is properly implemented, ensuring that the
LastEditedAttimestamp is converted to UTC and included in the mapped subscription object. This maintains consistency with other time fields in the mapping function.openmeter/subscription/service/service.go (1)
203-207: Properly implemented edit timestamp tracking.The edit timestamp is now correctly updated during subscription modifications by calling the
MarkEditedmethod with the current time. This implementation aligns with the PR objective to track the last edit time for subscriptions.openmeter/subscription/repo/subscriptionrepo.go (1)
32-47: Looks good - Implementation follows proper repository patternThe implementation of
MarkEditedfollows the established repository pattern in the codebase, properly using transactions, error handling, and UTC time conversion. This aligns with the PR's objective to track modifications to subscriptions by storing the edit timestamp.openmeter/ent/db/setorclear.go (1)
2088-2100: Implementation correctly follows the established patternThe new
SetOrClearLastEditedAtmethods for bothSubscriptionUpdateandSubscriptionUpdateOnetypes follow the consistent pattern seen throughout the file. They handle the nullableLastEditedAtfield appropriately, ensuring it can be either set or cleared based on whether a value is provided.api/api.gen.go (3)
13964-14008: Auto-generated Swagger spec changes look goodThese changes to the embedded Swagger specification correctly reflect the addition of the
IsCustomfield to the API models. This is auto-generated content that properly documents the API changes.
14022-14025: Auto-generated Swagger spec changes (continued)Continuing the auto-generated changes to the Swagger specification. No issues found.
14040-14364: Remaining auto-generated Swagger spec changes look goodThe rest of the changes to the embedded Swagger specification continue to document the API changes. This is expected for auto-generated content.
openmeter/subscription/subscription.go (2)
27-29: Well-structured field addition for tracking subscription edits.The
LastEditedAtfield is properly implemented as a pointer totime.Timewith an appropriate JSON tag that includesomitempty. This aligns well with the PR objective of storing the timestamp of the last edit to a subscription.
73-75: Clear and concise implementation of the IsCustom logic.The method correctly implements the custom subscription logic as defined in the PR objectives - a subscription is custom if it either lacks a plan reference or has been edited (indicated by LastEditedAt not being nil).
api/client/go/client.gen.go (3)
4553-4556: Implementation ofIsCustomfield looks good.The new
IsCustomboolean field has been properly added to the Subscription struct with clear documentation explaining when a subscription is considered custom. The field follows Go naming conventions and is correctly tagged for JSON serialization.
4648-4651: Implementation ofIsCustomfield is consistent.The
IsCustomfield has been implemented consistently in the SubscriptionExpanded struct with identical documentation and JSON tagging, maintaining consistency across related structs.
30428-30742: Swagger spec updates align with model changes.The swagger spec has been appropriately updated to include the new
isCustomfield. These changes appear to be auto-generated and align with the model definition changes made above.openmeter/productcatalog/subscription/http/mapping.go (2)
194-194: Implementation of the IsCustom flag.The new
IsCustomfield is correctly added to the API subscription model, using theIsCustom()method from the subscription domain model. This fulfills the requirement to expose this information via the API.
523-523: Correctly propagates IsCustom to the expanded subscription view.The
IsCustomfield is properly forwarded from the basic subscription API model to the expanded subscription API model, ensuring consistency across different API representations.openmeter/subscription/service/workflowservice_test.go (1)
123-129: Updated test expectation to match new validation requirement.The test case has been updated to expect an error when no patches are provided, rather than expecting no action to occur. This change correctly aligns with the new validation in the workflow service that requires at least one customization when editing a subscription.
This change in behavior makes sense for tracking custom subscriptions as described in the PR objectives - requiring explicit customizations helps identify which subscriptions have been edited.
openmeter/ent/db/subscription_update.go (4)
160-178: Added LastEditedAt field methods to SubscriptionUpdate.These new methods properly implement the database operations for the
last_edited_atfield, which is essential for tracking when a subscription was last edited. This field is a key part of the PR's objectives to identify custom subscriptions.The implementation follows the consistent pattern used for other time fields in this generated code.
362-367: Added LastEditedAt field handling in sqlSave method.The
sqlSavemethod is correctly updated to handle the newlast_edited_atfield, ensuring proper database persistence when updating subscriptions.
633-651: Added LastEditedAt field methods to SubscriptionUpdateOne.These methods implement the necessary operations for the single-entity update variant, maintaining consistency with the batch update implementation.
865-870: Added LastEditedAt field handling in SubscriptionUpdateOne.sqlSave.The single-entity
sqlSavemethod is properly updated to handle thelast_edited_atfield, ensuring consistency with the batch update implementation.openmeter/ent/db/migrate/schema.go (4)
1688-1688: Well-designed column addition for tracking subscription edits.The addition of the
last_edited_atcolumn with nullable Time type is well-implemented and aligns perfectly with the PR objective of tracking when a subscription was last edited. This field will be essential for determining if a subscription is custom.
1700-1703: Properly updated foreign key reference for customer ID.The column index has been correctly adjusted from 12 to 13 to account for the newly added
last_edited_atfield, maintaining proper foreign key relationships.
1705-1709: Properly updated foreign key reference for plan ID.The column index has been correctly adjusted from 13 to 14 to account for the newly added
last_edited_atfield, maintaining proper foreign key relationships.
1728-1731: Properly updated index for namespace and customer ID.The column index has been correctly adjusted to reference the new column positions after adding the
last_edited_atfield, ensuring database indexes remain valid.openmeter/ent/db/mutation.go (10)
36869-36869: Field addition looks good.The addition of
last_edited_atfield to theSubscriptionMutationstruct aligns with the PR objective of tracking when a subscription was last edited.
37523-37570: Well-implemented field management methods.The implementation includes all necessary methods for managing the
last_edited_atfield (set, get, get old value, clear, check if cleared, reset). The error handling inOldLastEditedAtcorrectly validates the operation type and ID presence before fetching the old value.
37768-37768: Capacity adjustment is correct.Increasing the capacity of the fields slice from 13 to 14 accommodates the new
last_edited_atfield.
37808-37810: Field inclusion logic is properly implemented.The code correctly appends the
last_edited_atfield to the fields list when it's set.
37845-37846: Field accessor method properly updated.The
Fieldmethod is correctly extended to return the value of thelast_edited_atfield.
37882-37883: OldField accessor method properly updated.The
OldFieldmethod is correctly extended to return the old value of thelast_edited_atfield.
37984-37990: SetField method properly updated.The
SetFieldmethod is correctly extended to handle setting thelast_edited_atfield with appropriate type checking.
38036-38038: ClearedFields method properly updated.The method now correctly includes the
last_edited_atfield when it has been cleared.
38068-38070: ClearField method properly updated.The
ClearFieldmethod is correctly extended to handle clearing thelast_edited_atfield.
38118-38120: ResetField method properly updated.The
ResetFieldmethod is correctly extended to handle resetting thelast_edited_atfield.openmeter/ent/db/subscription/where.go (2)
130-133: Methods for the newLastEditedAtfield look well-implementedThis addition introduces the main equality predicate for the new
last_edited_atfield that will be used to track subscription edits, aligning with the PR objective of determining custom subscriptions.
804-852: Complete set of predicate functions for thelast_edited_atfieldThese predicate functions follow the standard pattern used for other fields in the Subscription entity, providing a full range of comparison operators (EQ, NEQ, GT, LT, etc.) and nullability checks. The implementation is thorough and consistent with the codebase's patterns.
openmeter/ent/db/subscription_create.go (5)
173-185: Well-implemented creation methods for the new fieldThese methods correctly handle setting the
last_edited_atfield during subscription creation, with proper null-handling logic that follows the established pattern of the codebase.
427-430: Properly integrated into the creation specThe
last_edited_atfield is correctly integrated into thecreateSpecmethod, ensuring that the value will be properly set in the database when a new subscription is created.
675-691: Proper upsert operations for the new fieldThese methods provide the necessary functionality for handling the
last_edited_atfield during upsert operations, allowing for setting, updating, and clearing the field value.
903-922: Single item upsert operations implemented correctlyThese methods correctly implement the operations needed for the
last_edited_atfield in the context of upserting a single item, maintaining consistency with the codebase patterns.
1301-1320: Bulk upsert operations implemented correctlyThese methods provide the necessary bulk upsert functionality for the
last_edited_atfield, completing the full set of CRUD operations required for the new field.
| @@ -35,6 +35,7 @@ func (Subscription) Fields() []ent.Field { | |||
| field.String("plan_id").Optional().Nillable(), | |||
| field.String("customer_id").NotEmpty().Immutable(), | |||
| field.String("currency").GoType(currencyx.Code("")).MinLen(3).MaxLen(3).NotEmpty().Immutable(), | |||
| field.Time("last_edited_at").Optional().Nillable(), | |||
There was a problem hiding this comment.
updated_at is not the same, simply edited_at? but I'm fine with last_edited_at
There was a problem hiding this comment.
no, that's the resource's updated at. last_edited_at is for the whole subscription
| @@ -29,6 +29,23 @@ func NewSubscriptionRepo(db *db.Client) *subscriptionRepo { | |||
| } | |||
| } | |||
|
|
|||
| func (r *subscriptionRepo) MarkEdited(ctx context.Context, id models.NamespacedID, at time.Time) (subscription.Subscription, error) { | |||
| return entutils.TransactingRepo(ctx, r, func(ctx context.Context, repo *subscriptionRepo) (subscription.Subscription, error) { | |||
| ent, err := repo.db.Subscription.UpdateOneID(id.ID).SetLastEditedAt(at.UTC()).Where(dbsubscription.Namespace(id.Namespace)).Save(ctx) | |||
There was a problem hiding this comment.
move where close to the beginning
| ent, err := repo.db.Subscription.UpdateOneID(id.ID).SetLastEditedAt(at.UTC()).Where(dbsubscription.Namespace(id.Namespace)).Save(ctx) | |
| ent, err := repo.db.Subscription.UpdateOneID(id.ID).Where(dbsubscription.Namespace(id.Namespace)).SetLastEditedAt(at.UTC()).Save(ctx) |
There was a problem hiding this comment.
the current order follow sql syntax order
| @@ -519,6 +520,7 @@ func MapSubscriptionViewToAPI(view subscription.SubscriptionView) (api.Subscript | |||
| UpdatedAt: apiSub.UpdatedAt, | |||
| Status: apiSub.Status, | |||
| Alignment: &alg, | |||
| IsCustom: apiSub.IsCustom, | |||
There was a problem hiding this comment.
Should we expose EditedAt as well on the API?
There was a problem hiding this comment.
we can, but then we commit to it in this form
Overview
IsCustomfield: if a subscription doesn't have a plan ref (created as a custom sub) or is edited (has edit timestamp) it will be a custom subscription.Summary by CodeRabbit