-
Notifications
You must be signed in to change notification settings - Fork 0
feat(cart): add cart service #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Parent:
Develop
omkar273
wants to merge
11
commits into
develop
Choose a base branch
from
feat/cart
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…, Supabase, Cloudinary, caching, event publishing, webhooks, and Razorpay. Removed deprecated entries and added placeholders for user-specific values.
…eeky to Caygnus/codegeeky-lms-backend
…mmand list in main.go
…d database seeding command with sample data generation
…erations and Swagger documentation updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 9a6f8c1 in 4 minutes and 57 seconds. Click for details.
- Reviewed
8626lines of code in37files - Skipped
1files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. ent/schema/cart.go:75
- Draft comment:
The 'expires_at' field is marked as Immutable, so it cannot be updated after creation. However, the update service appears to allow changing its value. Confirm if this is intentional or adjust the schema/update logic. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. internal/service/cart.go:161
- Draft comment:
The UpdateCart service assigns a new 'expires_at' from the update request, yet the underlying repository update does not update this field (likely due to its immutability in the schema). Ensure the design is consistent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is speculative - it assumes the repository doesn't update ExpiresAt without showing evidence. The service code looks correct - it updates the field and passes it to the repo. Without seeing the repository code, we can't verify if there's actually an issue. The comment violates the rule about speculative comments ("If X, then Y is an issue"). I could be wrong about dismissing this - if the reviewer has seen the repository code and knows for certain that ExpiresAt is immutable, this would be a real issue worth fixing. Even if the reviewer has seen the repository code, the comment should be more specific and provide evidence rather than speculation. As written, it's not actionable enough. The comment should be removed because it's speculative and lacks concrete evidence of an actual problem. If there is a real schema/code mismatch, it should be reported with specific details.
3. docs/swagger/docs.go:1468
- Draft comment:
Typo/Consistency issue: The description says "... filter internships by internship id" whereas the parameter is an array (named 'internship_ids'). Consider updating the description to use plural (e.g., "internship ids") for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While technically correct that there's a slight mismatch between singular/plural, this is an extremely minor documentation issue that doesn't affect functionality. The meaning is still clear to API users. This feels like nitpicking that doesn't warrant a code change. The comment is technically accurate - there is an inconsistency. And consistency in API documentation is generally good practice. However, this is an auto-generated Swagger file and the inconsistency is so minor that it doesn't impact API usability or understanding. The effort of making the change outweighs the benefit. Delete the comment. While technically correct, this is too minor of a documentation issue to warrant a change, especially in an auto-generated file.
4. docs/swagger/swagger-3-0.json:1
- Draft comment:
There's a potential duplicated value in the 'types.UserRole' enum. The enum contains 'ADMIN' twice. Please review if the duplicate is intentional or a typographical error. - Reason this comment was not posted:
Marked as duplicate.
5. docs/swagger/swagger.json:168
- Draft comment:
Typographical suggestion: The description for the 'user_id' parameter reads "These fields are used to filter carts by user id", but only a single field is involved. Consider using singular form, e.g., "This field is used to filter carts by user id". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct, this is an extremely minor grammatical issue that doesn't affect functionality or understanding. The meaning is perfectly clear either way. The comment is purely about wording and doesn't suggest any actual code changes that would improve the API. The comment is technically accurate - there is a mismatch between singular/plural. Could this inconsistency cause confusion for API users? No, this minor grammatical issue would not cause any real confusion. The parameter name user_id makes it obvious that it's a single field. This is the kind of nitpicky comment that adds noise without value. Delete the comment. The grammatical suggestion is too minor to be worth a PR comment and doesn't affect API functionality or clarity.
6. docs/swagger/swagger.json:1462
- Draft comment:
Typographical note: The description reads "These fields are used to filter internships by internship id" while the parameter name is 'internship_ids'. It may be clearer to use plural consistently, e.g. "internship IDs", especially since it's an array. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment points out a minor inconsistency in documentation, it's not a critical issue that affects functionality. The meaning is still clear either way. The rules state to only keep comments that require clear code changes, and to remove comments that are purely informative or unimportant. The inconsistency could potentially confuse API users since the parameter accepts multiple IDs but the description implies a single ID. Documentation clarity does have value. However, the inconsistency is extremely minor - both versions would be understood by API users. This falls under "obvious or unimportant" per the rules. Delete the comment as it suggests a trivial documentation change that doesn't meaningfully impact API usability.
7. docs/swagger/swagger.json:1655
- Draft comment:
Typographical consistency: The summary value "Update an internship batch" uses a lowercase 'i' in 'internship' while related descriptions (e.g., 'Internship Batch ID') use title-case. Consider aligning the capitalization for consistency (e.g., "Update an Internship Batch"). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While consistency in documentation is good, this is an extremely minor stylistic issue that doesn't affect functionality or clarity. The current lowercase version is perfectly readable and follows common API documentation conventions. Many APIs use sentence case for summaries. This seems like the kind of nitpicky comment that adds noise without real value. Could inconsistent capitalization cause confusion for API consumers? Some style guides do recommend consistent capitalization in technical documentation. The lowercase version is clear and follows common API conventions. This level of nitpicking about capitalization doesn't meaningfully improve the API documentation or user experience. Delete the comment. While consistency is good, this capitalization difference is too minor to warrant a comment and doesn't impact usability.
8. docs/swagger/swagger.json:1665
- Draft comment:
Typographical consistency: The parameter description here is "Internship batch details" with a lowercase 'b' in 'batch', whereas elsewhere "Internship Batch" is capitalized (e.g., in 'Internship Batch ID'). Consider using consistent capitalization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While consistency in documentation is good, this is an extremely minor capitalization issue that doesn't affect functionality or understanding. The comment is about style rather than substance. The rules state to remove comments that are obvious or unimportant. Capitalization consistency at this level seems too minor to warrant a comment. Could inconsistent capitalization cause confusion for API users? Could it reflect poorly on the project's professionalism? While professional documentation is important, this level of capitalization detail is too minor to meaningfully impact API usability or project perception. The meaning is clear either way. Delete the comment as it falls under the category of obvious/unimportant issues that don't warrant a PR comment.
9. docs/swagger/swagger.json:3335
- Draft comment:
It looks like 'ADMIN' appears twice in the enum array. Please verify if this duplication is intended or if one of them should be replaced with another value. - Reason this comment was not posted:
Marked as duplicate.
10. docs/swagger/swagger.yaml:889
- Draft comment:
Typographical note: In the description at line 889, the parameter 'user_id' is singular, so consider changing "These fields are used to filter carts by user id" to "This field is used to filter carts by user ID" for clarity and consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct that "fields" should be "field" since it refers to a single parameter, and "ID" is typically capitalized, this is an extremely minor wording issue that doesn't affect functionality or clarity. The current wording is still perfectly understandable. This kind of nitpicky comment about documentation wording doesn't meet the bar for being an important enough issue to flag. The comment is technically accurate - it would make the documentation slightly more precise. And consistent documentation can be important for API usability. While technically correct, this level of minor grammar/style fixing in documentation doesn't warrant a PR comment. The meaning is clear either way. We should focus PR comments on more substantive issues. Delete this comment. The suggested change is too minor to be worth a PR comment. The current wording is clear enough.
11. docs/swagger/swagger.yaml:1248
- Draft comment:
It looks like the parameterinternship_idsin the/categoriesGET endpoint might be a typo. Please confirm if this should be named differently (for example,category_idsor another intended term) to align with the endpoint’s purpose. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The parameter allows filtering categories by which internships they contain. Since categories can have multiple internships (shown by the internships array field in the models), and you may want to find categories containing specific internships, this is a valid and useful filter parameter. The name internship_ids accurately describes what it filters by. I could be wrong about the relationship between categories and internships - maybe the internship_ids parameter is meant for something else that I'm not seeing in the API spec. No, the models clearly show a many-to-many relationship between categories and internships through the internships array field. The parameter name and functionality align with this data model. The internship_ids parameter is correctly named and serves a valid filtering purpose - finding categories that contain specific internships. The comment suggesting it might be a typo is incorrect.
Workflow ID: wflow_GzS4rFs4Ecc1z4LA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces a comprehensive cart service with repository, service, middleware, and test utilities, along with a script for seeding the database with sample data.
CartServiceinterface and implementation inservice/cart.gofor managing carts and line items.GetUserDefaultCart()to retrieve the default cart for a user.Repositoryinterface ininternal/domain/cart/repository.gofor cart operations.internal/repository/ent/cart.gofor database interactions.InMemoryCartStoreininternal/testutil/inmemory_cart_store.gofor testing.AuthenticateMiddlewareininternal/rest/middleware/auth.goto support development API key.CartType,CartLineItemEntityType, andCartFilterininternal/types/cart.go.internal/api/dto/cart.go.seed-datacommand inscripts/main.goto seed database with sample data.SeedData()inscripts/internal/seed_data.gofor seeding categories, internships, and discounts.This description was created by
for 9a6f8c1. You can customize this summary. It will automatically update as commits are pushed.