Subscription IsCustom #2414
Subscription IsCustom #2414
Conversation
📝 WalkthroughWalkthroughThis pull request introduces two major changes to the subscription models. First, a new boolean field ( Changes
Possibly related PRs
✨ 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
GAlexIHU
force-pushed
the
galexi/subs-edited-status
branch
from
c95aa7e to
b620294
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
openmeter/ent/db/subscription.go (1)
1-347:⚠️ Potential issueMissing IsCustom method according to PR objectives.
According to the PR objectives, subscriptions should be classified as custom if they either lack a plan reference (no PlanID) or have an associated edit timestamp (LastEditedAt is set). However, the implementation of the
IsCustommethod that would determine this status is missing from the Subscription struct.Suggest adding the following method to meet the requirements:
// IsCustom returns true if the subscription is classified as custom. // A subscription is custom if it either lacks a plan reference or has been edited. func (s Subscription) IsCustom() bool { return s.PlanID == nil || s.LastEditedAt != nil }
🧹 Nitpick comments (6)
api/openapi.yaml (2)
15818-15829: Define 'isCustom' with Clear Documentation
TheisCustomproperty is added with an appropriate boolean type and a detailed description outlining when a subscription is considered custom. One nitpick: in the description, please change "edited back to it's original form" to "edited back to its original form" for grammatical correctness.
15993-16004: Uniform Schema Definition for 'isCustom'
The specification for theisCustomproperty here mirrors the changes in the previous model with a clear boolean type and extended description. As with the previous hunk, consider correcting "edited back to it's original form" to "edited back to its original form" for clarity.api/client/javascript/src/client/schemas.ts (2)
7110-7113: Fix typo in JSDoc comment.There's a typographical error in the JSDoc comment on line 7112. It currently says "...edited back to it's original form" when it should say "...edited back to its original form". The word "its" is the possessive form, while "it's" is a contraction of "it is".
/** @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) */ + * - or if it has been edited (even if eventually edited back to its original form) */ isCustom: boolean
7232-7235: Fix the same typo in the second JSDoc comment.The same typographical error exists in this JSDoc comment as well. Please change "it's" to "its" for grammatical correctness.
/** @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) */ + * - or if it has been edited (even if eventually edited back to its original form) */ isCustom: booleanapi/api.gen.go (2)
4654-4657: Fix typographical error in commentThere's a typographical error in the documentation comment. The comment uses "it's" (contraction of "it is") instead of "its" (possessive).
// 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: Fix typographical error in commentThe same typographical error appears in this comment as well. The comment uses "it's" (contraction of "it is") instead of "its" (possessive).
// 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/20250312103404_subscription-editedat.down.sql(1 hunks)tools/migrate/migrations/20250312103404_subscription-editedat.up.sql(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tools/migrate/migrations/20250312103404_subscription-editedat.down.sql
- tools/migrate/migrations/20250312103404_subscription-editedat.up.sql
🚧 Files skipped from review as they are similar to previous changes (13)
- openmeter/subscription/service/workflowservice.go
- openmeter/ent/schema/subscription.go
- openmeter/subscription/repository.go
- api/spec/src/productcatalog/subscription.tsp
- openmeter/subscription/service/service.go
- openmeter/subscription/subscription.go
- openmeter/subscription/repo/mapping.go
- openmeter/productcatalog/subscription/http/mapping.go
- openmeter/ent/db/migrate/schema.go
- openmeter/subscription/repo/subscriptionrepo.go
- openmeter/ent/db/subscription/where.go
- openmeter/ent/db/mutation.go
- api/openapi.cloud.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
api/openapi.yaml (2)
15741-15747: Ensure Consistent Field Inclusion
The new- isCustomfield is correctly added to the list of schema fields. Please verify that the ordering is intentional and consistent with your field grouping conventions across models.
15919-15925: Consistent Update to Required Fields
The required fields update now includes- isCustom, alongside other properties such asactiveFrom,status,customerId, etc. Ensure that this inclusion is aligned with your intended validation rules and that downstream consumers are updated accordingly.api/client/javascript/src/client/schemas.ts (1)
7110-7113: Implementation aligns well with PR objectives.The added
isCustomfield correctly implements the requirements stated in the PR objectives. The JSDoc comments clearly explain when a subscription is considered custom (either created without a plan reference or edited at some point). This implementation is consistent with similar changes in other files noted in the AI summary.Also applies to: 7232-7235
openmeter/ent/db/subscription/subscription.go (3)
43-44: LGTM! New field definition follows established pattern.The addition of the
FieldLastEditedAtconstant correctly follows the established pattern in the codebase for field definitions.
101-101: LGTM! Field properly added to Columns array.The
FieldLastEditedAtis correctly added to theColumnsarray, ensuring it will be included in SQL column aggregation.
205-208: LGTM! Ordering function implementation is consistent with existing pattern.The
ByLastEditedAtfunction follows the same pattern as other ordering functions in this file, using thesql.OrderByFieldmethod to enable ordering of results based on the new field.openmeter/ent/db/setorclear.go (1)
2088-2100: LGTM! SetOrClear methods follow consistent pattern.The
SetOrClearLastEditedAtmethods for bothSubscriptionUpdateandSubscriptionUpdateOneare implemented correctly, following the established pattern for nullable fields. These methods will enable clients to set or clear the last_edited_at field in a consistent way.openmeter/ent/db/subscription.go (4)
50-51: LGTM! Field addition to Subscription struct follows established pattern.The
LastEditedAtfield is correctly added as a pointer totime.Timeand marked as optional in JSON serialization, matching the pattern used for other nullable time fields in the struct.
124-125: LGTM! Field included in scanValues method.The
FieldLastEditedAtis correctly included in the scanValues method case statement for time fields, allowing it to be properly scanned from SQL rows.
231-237: LGTM! Field handling added to assignValues method.The new field is properly handled in the assignValues method, with correct type checking and value assignment.
340-344: LGTM! Field included in String method.The
LastEditedAtfield is correctly included in the string representation generated by the String method, following the same pattern as other time fields.api/api.gen.go (2)
4654-4657: The implementation properly aligns with PR objectivesThe addition of the
IsCustomboolean field to bothSubscriptionandSubscriptionExpandedstructs correctly implements the custom subscription detection functionality as described in the PR objectives. The field is properly typed and tagged for JSON serialization.Also applies to: 4749-4752
13949-13992: Changes to swagger specification align with API changesThe changes to the
swaggerSpecvariable are auto-generated from the OpenAPI/Swagger specification and reflect the addition of theIsCustomfield to the subscription models.Also applies to: 14007-14007, 14025-14346
api/client/go/client.gen.go (3)
4553-4556: NewIsCustomfield added to the Subscription structThe implementation correctly adds a boolean field to track custom subscriptions. The documentation clearly explains the two conditions under which a subscription is considered custom.
4648-4651: NewIsCustomfield added to the SubscriptionExpanded structSimilar to the previous change, this adds the same field with identical documentation to the SubscriptionExpanded struct. This ensures consistency between the different subscription representations.
30405-30721: Updated swagger specification with the new fieldThe swagger specification has been updated to include the new
IsCustomfield. This is an essential part of the change to ensure the API documentation and client code generation remain in sync.openmeter/ent/db/subscription_update.go (4)
160-178: Well-implemented methods forlast_edited_atfield managementThe implementation correctly adds the necessary methods to manage the
last_edited_atfield in theSubscriptionUpdatestruct, following the same pattern as other fields in the codebase. This provides a complete API for setting, conditionally setting, and clearing the timestamp.
362-367: Properly integrated field handling in SQL save operationThe
sqlSavemethod has been correctly updated to include thelast_edited_atfield in database operations, maintaining consistency with how other fields are handled.
633-651: Consistent implementation forSubscriptionUpdateOnemethodsThe implementation provides the same set of methods for the
SubscriptionUpdateOnestruct, ensuring consistency across the API. This follows the established pattern throughout the codebase.
865-870: SQL save operation properly handles the new fieldThe
sqlSavemethod forSubscriptionUpdateOnecorrectly includes logic for thelast_edited_atfield, ensuring database operations will work as expected.openmeter/ent/db/subscription_create.go (6)
173-185: Properly implemented creation methods for the timestamp fieldThe implementation adds the necessary methods to set the
last_edited_atfield during subscription creation, following the same patterns as other fields in the codebase.
427-430: Correctly updated create specification for the new fieldThe
createSpecmethod has been properly extended to handle thelast_edited_atfield, ensuring it's correctly included when creating new subscription records.
675-691: Complete implementation of upsert operations for the timestamp fieldThe
SubscriptionUpsertstruct has been extended with all necessary methods for managing thelast_edited_atfield during upsert operations, providing a comprehensive API.
903-922: Consistent implementation forSubscriptionUpsertOneThe implementation provides the same set of methods for the
SubscriptionUpsertOnestruct, maintaining consistency across different operation types.
1301-1320: Consistent implementation for bulk operationsThe implementation extends to bulk operations as well, ensuring the
last_edited_atfield can be properly managed in all contexts where subscriptions are created or updated.
1-1344: The implementation aligns perfectly with the PR objectivesThe changes to add and manage the
last_edited_atfield directly support the PR's goal of "storing the timestamp of the last edit made to a subscription." This timestamp will be used to determine if a subscription is custom, as stated in the PR objectives: "A subscription will be marked as custom if it either lacks a plan reference or has an associated edit timestamp."The implementation is thorough, covering all aspects of the entity lifecycle (create, update, upsert) and follows the established patterns of the codebase.
|
Please rebase |
GAlexIHU
force-pushed
the
galexi/subs-edited-status
branch
from
b620294 to
a208fb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/openapi.yaml (2)
16048-16059: IntroduceisCustomwith Detailed DocumentationThe
isCustomfield is properly introduced with its type (boolean) and a descriptive multi-line explanation. However, in the description text, the phrase “edited back to it's original form” contains a minor grammatical error; it should be “edited back to its original form”.Consider applying this small fix:
- - 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)
16223-16234: RepeatisCustomDefinition with Minor Grammar NoteHere again the
isCustomfield is introduced with the same description as in the previous hunk. The description is clear but repeats the grammatical error in “edited back to it's original form”, which should be corrected to “its original form”. A diff suggestion is provided below for consistency:- - 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)
📜 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(3 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/20250312103404_subscription-editedat.down.sql(1 hunks)tools/migrate/migrations/20250312103404_subscription-editedat.up.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- tools/migrate/migrations/20250312103404_subscription-editedat.down.sql
- tools/migrate/migrations/20250312103404_subscription-editedat.up.sql
- openmeter/ent/schema/subscription.go
- api/spec/src/productcatalog/subscription.tsp
- openmeter/subscription/service/workflowservice.go
- openmeter/subscription/repo/subscriptionrepo.go
- openmeter/subscription/service/service.go
- openmeter/ent/db/subscription.go
- openmeter/ent/db/subscription/subscription.go
- openmeter/subscription/repo/mapping.go
- openmeter/subscription/subscription.go
- openmeter/productcatalog/subscription/http/mapping.go
- openmeter/ent/db/subscription_update.go
- openmeter/ent/db/migrate/schema.go
- api/client/go/client.gen.go
- openmeter/ent/db/mutation.go
- openmeter/subscription/repository.go
- api/openapi.cloud.yaml
🔇 Additional comments (32)
openmeter/subscription/service/workflowservice_test.go (1)
123-130: Behavior change test update aligns with implementationThe test case name and expectations have been updated to reflect the behavior change in
EditRunning, now requiring at least one patch to be provided. This test case now properly validates that an empty customization set returns an error with the appropriate message.openmeter/ent/db/setorclear.go (1)
2088-2100: Well-implemented utility methods for LastEditedAt fieldThese functions follow the established pattern in the codebase for setting or clearing optional fields. They correctly handle nil pointers by clearing the field or dereferencing and setting the value when non-nil, maintaining consistency with other similar methods in the file.
api/openapi.yaml (2)
15971-15977: AddisCustomin Field ListThe new
isCustomfield has been added to the list of subscription fields. This is in line with the new functionality for flagging custom subscriptions. Please verify that clients using this schema are aware of the change and can handle an additional field in the list.
16149-16155: ConsistentisCustomInclusion for Expanded SchemaThe
isCustomfield is consistently added in the subscription listing (likely representing an expanded view). Its placement among other fields such asactiveFrom,status, andcurrencyappears appropriate. Ensure that downstream processing or consumers that expect a fixed order (if any) are updated accordingly.api/client/javascript/src/client/schemas.ts (2)
7166-7169: Well-documented implementation of the isCustom field in Subscription schema.The newly added boolean field is properly documented with clear criteria for when a subscription is considered custom. The documentation aligns well with the PR objectives, explaining that a subscription is custom if it either lacks a plan reference or has been edited.
7288-7291: Consistent implementation of isCustom field in SubscriptionExpanded schema.The same isCustom field and documentation are properly added to the SubscriptionExpanded schema, maintaining consistency across related schema definitions. This ensures the custom subscription status is available in both regular and expanded subscription representations.
api/api.gen.go (3)
4661-4667: LGTM! The new IsCustom field is well-documented.The addition of this field with clear documentation accurately implements the described PR requirements for identifying custom subscriptions. The comment clearly explains the two conditions that make a subscription custom.
4756-4763: LGTM! Consistent implementation across subscription types.Good job maintaining consistency by adding the same field with identical documentation to the
SubscriptionExpandedstruct. This ensures theIsCustomproperty is available in all relevant subscription representations.
14202-14517: LGTM! Updated swagger spec to reflect the schema changes.The embedded swagger specification has been properly updated to include the new
IsCustomfield. This ensures that the API documentation accurately reflects the modified data models.openmeter/ent/db/subscription/where.go (11)
130-133: Implementation is consistent
This addition follows the existing pattern of equality-based predicates for Ent fields.
804-807: Looks correct
The equality check forlast_edited_atis implemented in line with Ent’s conventions.
809-812: No issues
The not-equal predicate is aligned with standard Ent usage.
814-817: No concerns
TheInpredicate logic is appropriate and mirrors typical patterns.
819-822: All good
TheNotInimplementation cleanly complements theInpredicate.
824-827: Standard approach
Greater-than predicate is correctly declared for time comparison.
829-832: Looks fine
Greater-than-or-equal predicate properly matches Ent’s syntax.
834-837: No issues
The less-than time comparison is standard.
839-842: Checks out
Less-than-or-equal predicate forlast_edited_atis consistent with Ent.
844-847: All set
IsNil predicate provides a clear filter for null values.
849-852: No concerns
NotNil predicate completes the optional field handling thoroughly.openmeter/ent/db/subscription_create.go (12)
173-177: Looks good
SetLastEditedAtadds a straightforward way to initialize the field.
179-185: No issues
SetNillableLastEditedAtis a clean approach to optional field handling.
427-430: Well-integrated
Includinglast_edited_atin the create spec ensures correct persistence.
675-679: Looks correct
SetLastEditedAtfor upsert logic aligns with other fields’ patterns.
681-685: Good approach
UpdateLastEditedAtreflects the typical “set excluded” usage for upserts.
687-691: All good
ClearLastEditedAtproperly nullifies the optional timestamp field.
903-908: Consistent
SetLastEditedAtinSubscriptionUpsertOneis implemented as expected.
910-915: No concerns
UpdateLastEditedAtfor single upsert usage is in line with best practices.
917-922: Looks fine
ClearLastEditedAtensures optional logic remains flexible for single upserts.
1301-1306: Checks out
SetLastEditedAtin bulk upsert handles the timestamp correctly.
1308-1313: No issues
UpdateLastEditedAtfor bulk upsert parallels single upsert logic neatly.
1315-1320: All set
ClearLastEditedAtin bulk upsert properly nullifies the field.
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