Add server-side seller filtering for review list endpoints.#176
Merged
Conversation
Support sellerId query params for user and transaction review routes, load seller relations needed by clients, and add coverage for filtered review retrieval. Made-with: Cursor
📝 WalkthroughWalkthroughTwo review endpoints now support optional seller filtering by accepting a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Database
Client->>Controller: GET /reviews?sellerId=uuid
Controller->>Controller: Check if sellerId provided
Controller->>Service: getUserReviewsBySellerId(sellerId)
Service->>Service: Create read-only transaction
Service->>Repository: getUserReviewsBySellerId(sellerId)
Repository->>Database: Query: JOIN review.seller WHERE seller.firebaseUid = sellerId
Database-->>Repository: UserReviewModel[] with eager-loaded seller
Repository-->>Service: Return filtered reviews
Service-->>Controller: Return filtered reviews
Controller-->>Client: Response with seller-filtered reviews
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/repositories/TransactionReviewRepository.ts (1)
19-29: Parameter typeUuidis misleading for a firebaseUid value.
sellerIdis bound toseller.firebaseUid, which is a Firebase UID string and not a UUID. Typing the parameter asUuidwill silently mislead callers and future maintainers (and any type-driven validation downstream). Recommendstringhere, matching the controller signature.♻️ Proposed change
- public async getTransactionReviewsBySellerId( - sellerId: Uuid, - ): Promise<TransactionReviewModel[]> { + public async getTransactionReviewsBySellerId( + sellerId: string, + ): Promise<TransactionReviewModel[]> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repositories/TransactionReviewRepository.ts` around lines 19 - 29, The parameter type for getTransactionReviewsBySellerId is incorrect: sellerId is bound to seller.firebaseUid (a Firebase UID string), so change the method signature in TransactionReviewRepository from sellerId: Uuid to sellerId: string and update any usages/call sites that pass a Firebase UID to match the new type; ensure the controller signature and any interface typings that reference getTransactionReviewsBySellerId are updated to string as well to keep types consistent.src/api/controllers/TransactionReviewController.ts (1)
24-34: Consider pagination and clarify thesellerIdsemantics.Two non-blocking observations:
- The seller-filtered branch is unbounded — for a seller with many reviews, response size and DB load will grow linearly. Consider adding
take/skip(or cursor) parameters when this endpoint starts seeing real load.- The
sellerIdquery param value is matched againsttransaction.seller.firebaseUidin the repository (not the seller entity's primary-key UUID). The naming follows existing codebase convention for user identifiers, but it may be worth documenting in an OpenAPI annotation or a brief comment so frontend integrators don't pass a UUID by mistake.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/controllers/TransactionReviewController.ts` around lines 24 - 34, The getTransactionReviews method should accept pagination parameters and document sellerId semantics: add optional QueryParams like take (limit) and skip (offset) to the async getTransactionReviews method and pass them into the service calls (e.g., call transactionReviewService.getTransactionReviewsBySellerId(sellerId, take, skip) or transactionReviewService.getAllTransactionReviews(take, skip)) to avoid unbounded responses; also add an inline comment or OpenAPI annotation above getTransactionReviews clarifying that sellerId refers to transaction.seller.firebaseUid (not the seller PK UUID) so frontend integrators send the correct identifier.src/repositories/UserReviewRepository.ts (1)
17-26: SameUuidtyping concern asTransactionReviewRepository.getTransactionReviewsBySellerId.
sellerIdis matched againstfirebaseUid(a Firebase UID string), not a UUID. Recommend changing the parameter type tostringto match what callers actually pass.♻️ Proposed change
- public async getUserReviewsBySellerId( - sellerId: Uuid, - ): Promise<UserReviewModel[]> { + public async getUserReviewsBySellerId( + sellerId: string, + ): Promise<UserReviewModel[]> {Minor adjacent nit: the new method uses the alias
"seller"for the seller relation, while the existinggetUserReviewById(line 34) still uses"user2". Worth aligning on"seller"whenever that file is next touched, for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repositories/UserReviewRepository.ts` around lines 17 - 26, The method getUserReviewsBySellerId currently types sellerId as Uuid but compares it to review.seller.firebaseUid (a Firebase UID string); change the sellerId parameter type from Uuid to string in getUserReviewsBySellerId and update any callers accordingly so the types match firebaseUid, and while here consider aligning relation aliases (e.g., use "seller" consistently instead of "user2") with getUserReviewById for readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/controllers/TransactionReviewController.ts`:
- Around line 24-34: The getTransactionReviews method should accept pagination
parameters and document sellerId semantics: add optional QueryParams like take
(limit) and skip (offset) to the async getTransactionReviews method and pass
them into the service calls (e.g., call
transactionReviewService.getTransactionReviewsBySellerId(sellerId, take, skip)
or transactionReviewService.getAllTransactionReviews(take, skip)) to avoid
unbounded responses; also add an inline comment or OpenAPI annotation above
getTransactionReviews clarifying that sellerId refers to
transaction.seller.firebaseUid (not the seller PK UUID) so frontend integrators
send the correct identifier.
In `@src/repositories/TransactionReviewRepository.ts`:
- Around line 19-29: The parameter type for getTransactionReviewsBySellerId is
incorrect: sellerId is bound to seller.firebaseUid (a Firebase UID string), so
change the method signature in TransactionReviewRepository from sellerId: Uuid
to sellerId: string and update any usages/call sites that pass a Firebase UID to
match the new type; ensure the controller signature and any interface typings
that reference getTransactionReviewsBySellerId are updated to string as well to
keep types consistent.
In `@src/repositories/UserReviewRepository.ts`:
- Around line 17-26: The method getUserReviewsBySellerId currently types
sellerId as Uuid but compares it to review.seller.firebaseUid (a Firebase UID
string); change the sellerId parameter type from Uuid to string in
getUserReviewsBySellerId and update any callers accordingly so the types match
firebaseUid, and while here consider aligning relation aliases (e.g., use
"seller" consistently instead of "user2") with getUserReviewById for
readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d23a841-7636-4f6d-927f-24df1a26ea47
📒 Files selected for processing (8)
src/api/controllers/TransactionReviewController.tssrc/api/controllers/UserReviewController.tssrc/repositories/TransactionReviewRepository.tssrc/repositories/UserReviewRepository.tssrc/services/TransactionReviewService.tssrc/services/UserReviewService.tssrc/tests/TransactionReviewTest.test.tssrc/tests/UserReviewTest.test.ts
Contributor
Author
|
Wait this is partially because we're not requiring sellers to be in the response... |
Match the frontend contract by marking the buyer and seller relations as nullable: false and adding a migration that purges orphaned reviews and applies NOT NULL constraints to buyerId and sellerId. Made-with: Cursor
Contributor
Author
|
OK now we're good pls review |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Currently when running the seller endpoint from the frontend, the backend doesn't seem to be filtering reviews by sellers, rather it returns all user reviews. This means filtering is left to the client (frontend), which seems like wasted work and bad practice.
Made-with: Cursor
Overview / Changes
Changed review service to only search for reviews via seller id. Enforcing that a seller id is required when looking for reviews.
Test Coverage
Added relevant unit tests
Related PRs or Issues
(cuappdev/resell-ios#96)
Summary by CodeRabbit
New Features
Tests