Enables Open Comments on BuyerRequests#142
Conversation
- Introduced CommentsModule to handle comment-related operations. - Implemented CommentsController for managing comments via RESTful endpoints. - Created CommentsService for business logic related to comments. - Added DTOs for creating and updating comments. - Included basic entity structure for comments. - Added unit tests for CommentsController and CommentsService to ensure functionality.
WalkthroughAdds a new Comments feature: registers CommentsModule and Comment entity, defines relations with BuyerRequest and User, implements controller endpoints for creating, listing (paginated), and deleting comments with guards and ownership checks, and introduces DTOs, service logic, and tests (unit, e2e, and factories). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as CommentsController
participant Auth as JwtAuthGuard
participant Service as CommentsService
participant DB as CommentRepository
Client->>API: POST /buyer-request/:id/comments {text}
API->>Auth: Validate JWT
Auth-->>API: OK
API->>Service: create(buyerRequestId, userId, dto)
Service->>DB: save({buyerRequestId, userId, text})
DB-->>Service: Comment
Service-->>API: Comment
API-->>Client: 201 Comment
sequenceDiagram
participant Client
participant API as CommentsController
participant Auth as Jwt+RolesGuard
participant Service as CommentsService
participant DB as CommentRepository
Client->>API: DELETE /comments/:id
API->>Auth: Validate JWT + roles
Auth-->>API: OK
API->>Service: remove(commentId, userId)
Service->>DB: findOne(commentId)
DB-->>Service: Comment|null
Service->>Service: check ownership
Service->>DB: delete(commentId)
DB-->>Service: OK
Service-->>API: void
API-->>Client: 200/204
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- Added a one-to-many relationship between BuyerRequest and Comment entities. - Updated User entity to include a one-to-many relationship with Comment. - Defined Comment entity with necessary fields and relationships to BuyerRequest and User.
- Introduced CreateCommentDto for comment creation with validation using class-validator. - Added fields for requestId, userId, and text with appropriate validation decorators to ensure data integrity.
|
Noticed the Comment entity requires a UUID as id, but on User & BuyerRequest they both make use of integer as id. |
- Removed UUID fields from CreateCommentDto and replaced them with string validation for text input. - Changed the primary key of Comment entity from UUID to auto-incrementing integer. - Updated buyerRequestId and userId fields in Comment entity to use integer types instead of UUIDs. - Adjusted relationships in Comment entity to reflect the new field names and types.
… authorization - Added TypeOrmModule integration for Comment entity in CommentsModule. - Updated CommentsController to handle comment creation and retrieval for specific buyer requests, including user authentication and authorization. - Enhanced CommentsService to support creating comments linked to buyer requests and user-specific deletion with proper error handling for unauthorized actions.
- Implemented e2e tests for the Comments module, covering user registration, comment posting, and retrieval for buyer requests. - Included tests for authentication and authorization scenarios, ensuring only authorized users can post and delete comments. - Verified response structures and status codes for various comment-related operations.
- Updated end-to-end tests for the Comments module to utilize factory functions for user and buyer request creation, improving test readability and maintainability. - Changed authentication method from token-based to cookie-based for user actions. - Adjusted API endpoints to include versioning in requests, ensuring consistency across the application.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (26)
src/modules/comments/dto/create-comment.dto.ts (2)
4-7: Reject whitespace-only strings and cap payload sizeAs written, strings containing only spaces will pass validation, and there’s no upper bound to prevent excessively large payloads. Trim input before validation and add a reasonable MaxLength. MinLength(1) becomes redundant once trimming is applied.
Apply this diff within the selected lines:
- @IsString() - @IsNotEmpty() - @MinLength(1) + @Transform(({ value }) => (typeof value === 'string' ? value.trim() : value)) + @IsString() + @IsNotEmpty({ message: 'text cannot be empty' }) + @MaxLength(2000) text: string;Add the supporting imports outside the selected lines:
- import { IsNotEmpty, IsString, MinLength } from 'class-validator'; + import { IsNotEmpty, IsString, MaxLength } from 'class-validator'; + import { Transform } from 'class-transformer';
1-1: Tidy up redundant MinLength importMinLength(1) is redundant once we trim and use IsNotEmpty. Replace with MaxLength and add Transform import.
- import { IsNotEmpty, IsString, MinLength } from 'class-validator'; + import { IsNotEmpty, IsString, MaxLength } from 'class-validator'; + import { Transform } from 'class-transformer';src/modules/comments/tests/comments.controller.spec.ts (1)
17-19: Expand coverage: exercise controller endpoints and guard behavior“Defined” is a good smoke test, but add specs to cover:
- POST /buyer-requests/:id/comments happy path and validation error on empty text
- GET pagination (default limit 20, page query)
- DELETE authorization: only owner/admin can delete
I can scaffold these tests using a mocked service and parameterized cases. Want me to push a spec outline?
src/modules/comments/comments.module.ts (1)
7-11: Consider exporting CommentsService for reuse by other modulesIf other modules (e.g., notifications, buyer-requests) need to trigger comment-related actions, export the service. If not needed today, you can skip.
@Module({ imports: [TypeOrmModule.forFeature([Comment])], controllers: [CommentsController], - providers: [CommentsService], + providers: [CommentsService], + exports: [CommentsService], })src/modules/users/entities/user.entity.ts (1)
41-42: Relation setup looks good; confirm foreign key type alignment and delete semantics
- Ensure Comment entity’s ManyToOne to User infers the FK type from User.id (number). Don’t manually declare userId: 'uuid' there; mismatch will break migrations (noted in PR discussion).
- Consider onDelete: 'CASCADE' on the ManyToOne side in Comment so a user deletion cleans up comments, if that matches your domain rules.
If you want, I can propose a concrete Comment entity relation snippet to guarantee correct FK typing and cascade behavior.
src/modules/comments/tests/comments.service.spec.ts (3)
1-2: Add imports for repository token and entities used in mocksRequired by the DI fixes suggested.
import { Test, TestingModule } from '@nestjs/testing'; import { CommentsService } from '../services/comments.service'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { Comment } from '../entities/comment.entity'; +import { BuyerRequest } from '@/modules/buyer-requests/entities/buyer-request.entity'; +import { User } from '@/modules/users/entities/user.entity';
15-17: Expand coverage: validate business rules and paginationAdd unit tests for:
- create: associates userId and buyerRequestId; rejects empty/whitespace text
- findAllForBuyerRequest: honors default limit 20 and page parameter
- remove: only author or admin can delete (non-owner forbidden)
I can draft these tests with repository mocks exercising success and failure paths. Want me to proceed?
7-13: Optionally share a single repo mock instance across providersHelps reduce boilerplate and keeps call tracking centralized when asserting interactions.
Example (outside the selected lines):
const repoMock = { create: jest.fn(), save: jest.fn(), findAndCount: jest.fn(), findOne: jest.fn(), delete: jest.fn(), };Then reuse repoMock in the provider useValues.
src/modules/comments/dto/update-comment.dto.ts (1)
1-4: Prefer@nestjs/mapped-typesover@nestjs/swaggerfor DTO utilitiesUsing Swagger’s PartialType couples DTOs to Swagger and can complicate tests or non-HTTP usage.
@nestjs/mapped-typesis the recommended source forPartialType.Apply this diff:
-import { PartialType } from '@nestjs/swagger'; +import { PartialType } from '@nestjs/mapped-types';test/comments.e2e-spec.ts (2)
38-49: Strengthen creation test assertions (optional)Consider asserting the returned ID format and that the comment text echoes back, to tighten guarantees. Example additions:
expect(res.status).toBe(201); - expect(res.body.data).toHaveProperty('id'); + expect(res.body.data).toHaveProperty('id'); + expect(typeof res.body.data.id).toBe('string'); // UUID string expect(res.body.data.text).toBe('Is this still available?');
67-75: Expand GET assertions to validate contents and requirements (optional)Issue requires returning user data and supports pagination. Consider also asserting that:
- The array contains the previously created comment.
- Comment objects include user info (e.g.,
useroruserId), per the “Include user data in responses” note.Example additions:
expect(res.status).toBe(200); expect(Array.isArray(res.body.data)).toBe(true); + expect(res.body.data.length).toBeGreaterThanOrEqual(1); + expect(res.body.data.some((c: any) => c.text === 'Is this still available?')).toBe(true); expect(res.body.meta).toHaveProperty('total'); expect(res.body.meta).toHaveProperty('page'); + // If the endpoint includes user info: + // expect(res.body.data[0]).toHaveProperty('user');src/modules/buyer-requests/entities/buyer-request.entity.ts (2)
14-14: Avoid path alias inconsistency; prefer relative import or verify alias setupOnly this line uses the
@/alias while the rest of the file uses relative paths. Unless tsconfig path aliases are fully wired for all runtimes (dev, tests, migrations), this can break builds/tests.Apply this diff for consistency:
-import { Comment } from '@/modules/comments/entities/comment.entity'; +import { Comment } from '../../comments/entities/comment.entity';
70-72: Ensure cascading behavior for comments on BuyerRequest deletionFrom a data-integrity standpoint, deleting a buyer request should likely remove its comments. This is configured on the Comment entity’s
@ManyToOne(() => BuyerRequest, ...)side usingonDelete: 'CASCADE'(or an explicit FK in migrations), not on this@OneToMany.Please verify the Comment entity applies
onDelete: 'CASCADE'forbuyerRequest, and that the migration includes the same.test/factories/buyer-request.factory.ts (1)
9-23: Fail fast with clearer error when buyer request creation failsCurrently, the helper returns the response body even if the request failed, which can cascade into unclear test failures later. Check the status and throw with context.
Apply this diff:
const res = await request(app.getHttpServer()) .post('/api/v1/buyer-requests') .set('Cookie', cookie) .send({ title: `Test Request ${requestCounter}`, description: 'Test description', budgetMin: 100, budgetMax: 200, categoryId: 1, expiresAt: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000).toISOString(), ...overrides, }); + if (res.status !== 201) { + throw new Error(`Failed to create buyer request. Status: ${res.status}. Body: ${JSON.stringify(res.body)}`); + } + return res.body.data || res.body;src/app.module.ts (1)
46-72: Optional: reduce entity registration friction withautoLoadEntitiesTo avoid manually editing the entities array whenever a new entity is added, consider enabling
autoLoadEntities: trueinTypeOrmModule.forRoot()and removing the explicitentitiesarray. This can also help prevent migration/test issues due to missing entities.Example:
TypeOrmModule.forRoot({ type: 'postgres', url: process.env.DATABASE_URL, ssl: process.env.DB_SSL === 'true' ? { rejectUnauthorized: false } : undefined, autoLoadEntities: true, synchronize: process.env.NODE_ENV !== 'production', logging: process.env.NODE_ENV === 'development', })test/factories/user.factory.ts (3)
12-15: Avoid hardcoding the auth challenge; prefer server-issued challenges to prevent brittlenessHardcoding the message format and timestamp can drift from the server’s expected challenge format and validation rules. If your auth flow supports a challenge endpoint, fetch the challenge for the wallet and sign that instead.
Would you like me to add a helper that calls a hypothetical GET /api/v1/auth/challenge?walletAddress=... and falls back to the current behavior if unavailable?
16-25: Assert expected status and fail fast when registration failsRight now, a failed registration could return HTML/error JSON and you’ll proceed to parse it as a success. Add a status assertion to make failures obvious in tests.
Apply this diff:
- const registerRes = await request(app.getHttpServer()) - .post('/api/v1/auth/register') - .send({ + const registerRes = await request(app.getHttpServer()) + .post('/api/v1/auth/register') + .send({ walletAddress, signature, message, name: `User ${userCounter}`, email: `user${userCounter}@test.com`, ...overrides, - }); + }) + .expect(201);
27-35: Harden cookie extraction and surface a descriptive error if missingIf the Set-Cookie header is absent or the cookie name differs (e.g., Authentication vs token), downstream tests will fail non-descriptively. Guard and error early with context.
Apply this diff:
const cookies = registerRes.headers['set-cookie']; - const tokenCookie = Array.isArray(cookies) ? cookies.find((c) => c.startsWith('token=')) : undefined; + const tokenCookie = Array.isArray(cookies) + ? cookies.find((c) => /^token=/.test(c)) + : undefined; + + if (!tokenCookie) { + throw new Error( + `Auth cookie not found in Set-Cookie header. Got: ${JSON.stringify(cookies)}` + ); + }If your app uses a different cookie name, update the regex accordingly.
src/modules/comments/entities/comment.entity.ts (3)
19-20: Align createdAt column naming with existing convention (created_at)BuyerRequest uses snake_case DB column names for timestamps. For consistency across tables, consider naming the column created_at here as well.
Apply this diff:
- @CreateDateColumn({ type: 'timestamp' }) - createdAt: Date; + @CreateDateColumn({ name: 'created_at', type: 'timestamp' }) + createdAt: Date;
5-28: Add an index to optimize list queries by buyerRequestId and createdAtGET /buyer-requests/:id/comments orders by createdAt and filters by buyerRequestId. A composite index will significantly improve pagination performance at scale.
Apply these diffs:
-import { Column, Entity, PrimaryGeneratedColumn, CreateDateColumn, ManyToOne, JoinColumn } from 'typeorm'; +import { Column, Entity, PrimaryGeneratedColumn, CreateDateColumn, ManyToOne, JoinColumn, Index } from 'typeorm';-@Entity('comments') +@Entity('comments') +@Index('idx_comments_request_created_at', ['buyerRequestId', 'createdAt']) export class Comment {If you prefer simpler indexing, at least index buyerRequestId.
26-28: Confirm eager loading of user is intentional for list responsesEager: true ensures user data is included automatically, matching the requirement. Be aware of the query shape on large lists; if you see performance issues, consider switching to explicit joins in service queries.
src/modules/comments/controllers/comments.controller.ts (1)
14-21: Minor: prefer a typed AuthRequest over manual assertionsTo avoid repeated casting, define an interface that augments Express.Request with your user shape and reuse it across controllers.
If desired, I can add an AuthRequest type to src/modules/auth/types and update controllers accordingly.
src/modules/comments/services/comments.service.ts (4)
24-31: Sanitize pagination inputs and cap limit to prevent heavy queriesProtect against page<=0 and abuse of limit.
Apply this diff:
- async findAllForBuyerRequest(buyerRequestId: number, page: number = 1, limit: number = 20) { - const [comments, total] = await this.commentRepository.findAndCount({ + async findAllForBuyerRequest(buyerRequestId: number, page = 1, limit = 20) { + const safePage = Math.max(1, Number.isFinite(page) ? page : 1); + const safeLimit = Math.min(100, Math.max(1, Number.isFinite(limit) ? limit : 20)); + const [comments, total] = await this.commentRepository.findAndCount({ where: { buyerRequestId }, order: { createdAt: 'DESC' }, - skip: (page - 1) * limit, - take: limit, + skip: (safePage - 1) * safeLimit, + take: safeLimit, }); return { results: comments, meta: { total, - page, - limit, - lastPage: Math.ceil(total / limit), + page: safePage, + limit: safeLimit, + lastPage: Math.ceil(total / safeLimit), }, }; }
15-22: Optional: normalize text before saveTrim whitespace to avoid “empty-looking” comments passing validation with spaces.
Apply this diff:
async create(buyerRequestId: number, userId: number, createCommentDto: CreateCommentDto) { + const text = createCommentDto.text?.trim() ?? ''; const comment = this.commentRepository.create({ buyerRequestId, userId, - ...createCommentDto, + text, }); return this.commentRepository.save(comment); }Validation already enforces non-empty text, but this avoids leading/trailing padding.
43-49: Remove unused placeholder methods or implement themfindOne/update return placeholder strings and are not used by the controller. Remove to reduce surface area until needed.
Apply this diff:
- findOne(id: number) { - return `This action returns a #${id} comment`; - } - - update(id: number, updateCommentDto: UpdateCommentDto) { - return `This action updates a #${id} comment`; - }
15-22: Consider validating BuyerRequest existence on createCurrently, a comment can be created for a non-existent buyerRequestId. If the DB doesn’t enforce FK constraints or if the request is soft-deleted, you’ll end up with orphans.
If desired, I can add a quick check via a BuyerRequestRepository or a foreign key constraint + test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/app.module.ts(4 hunks)src/modules/buyer-requests/entities/buyer-request.entity.ts(2 hunks)src/modules/comments/comments.module.ts(1 hunks)src/modules/comments/controllers/comments.controller.ts(1 hunks)src/modules/comments/dto/create-comment.dto.ts(1 hunks)src/modules/comments/dto/update-comment.dto.ts(1 hunks)src/modules/comments/entities/comment.entity.ts(1 hunks)src/modules/comments/services/comments.service.ts(1 hunks)src/modules/comments/tests/comments.controller.spec.ts(1 hunks)src/modules/comments/tests/comments.service.spec.ts(1 hunks)src/modules/users/entities/user.entity.ts(2 hunks)test/comments.e2e-spec.ts(1 hunks)test/factories/buyer-request.factory.ts(1 hunks)test/factories/user.factory.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/modules/comments/dto/update-comment.dto.ts (1)
src/modules/comments/dto/create-comment.dto.ts (1)
CreateCommentDto(3-8)
src/modules/comments/comments.module.ts (1)
src/app.module.ts (1)
Module(42-90)
src/modules/comments/services/comments.service.ts (2)
src/modules/comments/dto/create-comment.dto.ts (1)
CreateCommentDto(3-8)src/modules/comments/dto/update-comment.dto.ts (1)
UpdateCommentDto(4-4)
test/comments.e2e-spec.ts (2)
test/factories/user.factory.ts (1)
createTestUser(7-37)test/factories/buyer-request.factory.ts (1)
createBuyerRequest(6-23)
src/modules/comments/controllers/comments.controller.ts (2)
src/modules/wishlist/controller/wishlist.controller.ts (1)
UseGuards(16-47)src/modules/comments/dto/create-comment.dto.ts (1)
CreateCommentDto(3-8)
src/modules/comments/entities/comment.entity.ts (2)
src/modules/buyer-requests/entities/buyer-request.entity.ts (1)
Entity(22-81)src/modules/users/entities/user.entity.ts (1)
Entity(15-49)
🔇 Additional comments (4)
src/modules/comments/comments.module.ts (1)
8-10: All repository injections are covered—no changes needed.The
CommentsServiceonly injects@InjectRepository(Comment)andCommentsModule’sTypeOrmModule.forFeature([Comment])matches exactly.test/comments.e2e-spec.ts (1)
51-57: LGTM: unauthenticated creation correctly returns 401This aligns with the access-control requirement.
src/app.module.ts (1)
20-20: LGTM: Comments module and entity are correctly wired into the app
- CommentsModule is registered.
- Comment entity is included in TypeORM entities.
- Imports list includes CommentsModule.
This should enable controller/routes and persistence for comments when
synchronizeis on in non-prod.Also applies to: 40-40, 68-69, 87-88
src/modules/comments/entities/comment.entity.ts (1)
7-15: ID and FK types align with existing User/BuyerRequest numeric keysUsing numeric primary and foreign keys here matches User.id and BuyerRequest.id in the provided snippets. This resolves the mismatch noted in the PR discussion.
| @@ -0,0 +1,38 @@ | |||
| import { Controller, Get, Post, Body, Patch, Param, Delete, UseGuards, ParseIntPipe, Req, Query } from '@nestjs/common'; | |||
There was a problem hiding this comment.
Defaulting page with ParseIntPipe causes 400 when omitted; use DefaultValuePipe(1) + ParseIntPipe
Nest won’t apply the “= 1” default before the pipe, so undefined trips ParseIntPipe.
Apply this diff:
-import { Controller, Get, Post, Body, Patch, Param, Delete, UseGuards, ParseIntPipe, Req, Query } from '@nestjs/common';
+import { Controller, Get, Post, Body, Patch, Param, Delete, UseGuards, ParseIntPipe, Req, Query, DefaultValuePipe } from '@nestjs/common';
@@
- @Query('page', ParseIntPipe) page: number = 1
+ @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: numberAlso applies to: 26-27
🤖 Prompt for AI Agents
In src/modules/comments/controllers/comments.controller.ts around lines 1 and
26-27, the controller currently applies ParseIntPipe directly to the "page"
query param which causes a 400 when the param is omitted because the default
(=1) isn't applied before pipes; update the @Query decorator(s) to use
DefaultValuePipe(1) before ParseIntPipe (e.g. @Query('page', new
DefaultValuePipe(1), ParseIntPipe) page: number) for both occurrences so a
missing page defaults to 1 and then is parsed as an int.
| @Controller('comments') | ||
| export class CommentsController { | ||
| constructor(private readonly commentsService: CommentsService) {} | ||
|
|
||
| @Post('/buyer-request/:id/comments') | ||
| @UseGuards(JwtAuthGuard) |
There was a problem hiding this comment.
Route paths are mis-scoped; they produce /comments/buyer-request/:id/comments and /comments/comments/:id
The @controller('comments') prefix combined with child paths doubles segments and deviates from the spec (plural “buyer-requests”). Endpoints won’t match clients/tests.
Apply this diff to align with the spec:
-@Controller('comments')
+@Controller()
export class CommentsController {
@@
- @Post('/buyer-request/:id/comments')
+ @Post('buyer-requests/:id/comments')
@@
- @Get('/buyer-request/:id/comments')
+ @Get('buyer-requests/:id/comments')
@@
- @Delete('/comments/:id')
+ @Delete('comments/:id')Also applies to: 23-27, 31-34
🤖 Prompt for AI Agents
In src/modules/comments/controllers/comments.controller.ts around lines 8-13
(and also apply same pattern to lines 23-27 and 31-34): the controller prefix is
wrong — @Controller('comments') causes doubled segments; change the controller
decorator to @Controller('buyer-requests') and make the child route paths
relative (remove the repeated resource name). Specifically, replace
"/buyer-request/:id/comments" with ":id/comments" (so the final route becomes
/buyer-requests/:id/comments) and remove any "/comments" prefix in child routes
so they are relative (e.g. change "/comments/:id" handlers to ":id" under the
correct controller) ensuring the plural "buyer-requests" is used per the spec.
| @UseGuards(JwtAuthGuard, RolesGuard) | ||
| @Roles('admin', 'buyer') | ||
| remove(@Param('id', ParseIntPipe) id: number, @Req() req: Request) { | ||
| const user = req.user as { id: number }; | ||
| return this.commentsService.remove(id, user.id); |
There was a problem hiding this comment.
Roles constraint blocks legitimate owners from deleting (e.g., sellers); delegate ownership/admin logic to the service
Spec: only the comment author or admin may delete. Restricting to roles('admin','buyer') prevents other roles from deleting their own comments.
Apply this diff (paired with a service change) to remove the role restriction and pass an isAdmin flag derived from req.user:
- @UseGuards(JwtAuthGuard, RolesGuard)
- @Roles('admin', 'buyer')
- remove(@Param('id', ParseIntPipe) id: number, @Req() req: Request) {
- const user = req.user as { id: number };
- return this.commentsService.remove(id, user.id);
- }
+ @UseGuards(JwtAuthGuard)
+ remove(@Param('id', ParseIntPipe) id: number, @Req() req: Request) {
+ const user = req.user as { id: number; role?: string; roles?: string[] };
+ const roles = Array.isArray(user?.roles) ? user.roles : (user?.role ? [user.role] : []);
+ const isAdmin = roles.includes('admin');
+ return this.commentsService.remove(id, user.id, isAdmin);
+ }Optionally, consider introducing a typed AuthRequest interface to avoid the type assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @UseGuards(JwtAuthGuard, RolesGuard) | |
| @Roles('admin', 'buyer') | |
| remove(@Param('id', ParseIntPipe) id: number, @Req() req: Request) { | |
| const user = req.user as { id: number }; | |
| return this.commentsService.remove(id, user.id); | |
| @UseGuards(JwtAuthGuard) | |
| remove(@Param('id', ParseIntPipe) id: number, @Req() req: Request) { | |
| const user = req.user as { id: number; role?: string; roles?: string[] }; | |
| const roles = Array.isArray(user?.roles) | |
| ? user.roles | |
| : user?.role | |
| ? [user.role] | |
| : []; | |
| const isAdmin = roles.includes('admin'); | |
| return this.commentsService.remove(id, user.id, isAdmin); | |
| } |
🤖 Prompt for AI Agents
In src/modules/comments/controllers/comments.controller.ts around lines 32 to
36, the controller currently restricts deletion to Roles('admin','buyer') which
prevents legitimate owners with other roles (e.g., sellers) from deleting their
own comments; remove the Roles decorator (and the RolesGuard requirement) so
ownership checks happen in the service, extract user and compute an isAdmin
boolean from req.user (e.g., req.user.role === 'admin' or a boolean flag) and
pass both user.id and isAdmin to commentsService.remove(id, user.id, isAdmin);
optionally replace the inline cast with a small typed AuthRequest interface for
req to avoid type assertions.
| async remove(commentId: number, userId: number): Promise<void> { | ||
| const comment = await this.commentRepository.findOne({ | ||
| where: { id: commentId }, | ||
| }); | ||
|
|
||
| if (!comment) { | ||
| throw new NotFoundException('Comment not found'); | ||
| } | ||
|
|
||
| if (comment.userId !== userId) { | ||
| throw new ForbiddenException('You can only delete your own comment'); | ||
| } | ||
|
|
||
| await this.commentRepository.remove(comment); | ||
| } |
There was a problem hiding this comment.
Deletion does not allow admin override
Spec requires owner or admin can delete. Currently only the owner can. Extend the method to accept an isAdmin flag and allow deletion when true.
Apply this diff (and keep controller change in sync):
- async remove(commentId: number, userId: number): Promise<void> {
+ async remove(commentId: number, userId: number, isAdmin = false): Promise<void> {
const comment = await this.commentRepository.findOne({
where: { id: commentId },
});
if (!comment) {
throw new NotFoundException('Comment not found');
}
- if (comment.userId !== userId) {
+ if (!isAdmin && comment.userId !== userId) {
throw new ForbiddenException('You can only delete your own comment');
}
- await this.commentRepository.remove(comment);
+ await this.commentRepository.remove(comment);
}If you prefer not to change the signature, inject a CurrentUser provider or pull roles from a request-scoped context. The above is the smallest change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async remove(commentId: number, userId: number): Promise<void> { | |
| const comment = await this.commentRepository.findOne({ | |
| where: { id: commentId }, | |
| }); | |
| if (!comment) { | |
| throw new NotFoundException('Comment not found'); | |
| } | |
| if (comment.userId !== userId) { | |
| throw new ForbiddenException('You can only delete your own comment'); | |
| } | |
| await this.commentRepository.remove(comment); | |
| } | |
| async remove(commentId: number, userId: number, isAdmin = false): Promise<void> { | |
| const comment = await this.commentRepository.findOne({ | |
| where: { id: commentId }, | |
| }); | |
| if (!comment) { | |
| throw new NotFoundException('Comment not found'); | |
| } | |
| if (!isAdmin && comment.userId !== userId) { | |
| throw new ForbiddenException('You can only delete your own comment'); | |
| } | |
| await this.commentRepository.remove(comment); | |
| } |
🤖 Prompt for AI Agents
In src/modules/comments/services/comments.service.ts around lines 51 to 65, the
remove method only allows the comment owner to delete; update the signature to
async remove(commentId: number, userId: number, isAdmin = false): Promise<void>
and change the authorization check to allow deletion when comment.userId ===
userId OR isAdmin is true; keep the controller (or any callers) in sync by
passing the new isAdmin boolean (or if you prefer not to change signature,
instead read current user roles from a request-scoped provider and allow
deletion when the current user is admin).
| const module: TestingModule = await Test.createTestingModule({ | ||
| controllers: [CommentsController], | ||
| providers: [CommentsService], | ||
| }).compile(); |
There was a problem hiding this comment.
Fix DI: mock CommentsService to avoid unresolved repository dependencies
Providing the real CommentsService here will likely fail module compilation if the service injects repositories via TypeORM (common in Nest). Mock the service in the controller unit test.
- const module: TestingModule = await Test.createTestingModule({
- controllers: [CommentsController],
- providers: [CommentsService],
- }).compile();
+ const module: TestingModule = await Test.createTestingModule({
+ controllers: [CommentsController],
+ providers: [
+ {
+ provide: CommentsService,
+ useValue: {
+ create: jest.fn(),
+ findAllForBuyerRequest: jest.fn(),
+ findOne: jest.fn(),
+ remove: jest.fn(),
+ },
+ },
+ ],
+ }).compile();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [CommentsController], | |
| providers: [CommentsService], | |
| }).compile(); | |
| const module: TestingModule = await Test.createTestingModule({ | |
| controllers: [CommentsController], | |
| providers: [ | |
| { | |
| provide: CommentsService, | |
| useValue: { | |
| create: jest.fn(), | |
| findAllForBuyerRequest: jest.fn(), | |
| findOne: jest.fn(), | |
| remove: jest.fn(), | |
| }, | |
| }, | |
| ], | |
| }).compile(); |
🤖 Prompt for AI Agents
In src/modules/comments/tests/comments.controller.spec.ts around lines 9 to 12,
the test currently provides the real CommentsService which will pull in
unresolved TypeORM repository dependencies; replace that provider with a mocked
provider (e.g., provide: CommentsService, useValue: { /* stub methods used by
controller */ } or useClass: MockCommentsService) and implement stubs for the
specific service methods the controller calls in these tests (returning simple
resolved values or jest.fn() mocks). Ensure the TestingModule providers array
uses the mock so the module compiles without needing actual repositories.
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [CommentsService], | ||
| }).compile(); | ||
|
|
||
| service = module.get<CommentsService>(CommentsService); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm which repositories are injected before finalizing mocks
To avoid over/under-provisioning, verify the exact @InjectRepository(...) tokens in CommentsService.
🏁 Script executed:
#!/bin/bash
# List repositories injected into CommentsService
rg -n -C2 -g 'src/**/comments/**' -P '@InjectRepository\(([^)]+)\)' src/modules/comments/services/comments.service.ts || trueLength of output: 278
Add a mock provider for the Comment repository
In src/modules/comments/tests/comments.service.spec.ts, the CommentsService constructor only injects one repository:
@InjectRepository(Comment)
private readonly commentRepository: Repository<Comment>;You must mock this in your test module to satisfy Nest’s DI:
const mockCommentRepository = {
find: jest.fn(),
save: jest.fn(),
// add other methods used by CommentsService
};
await Test.createTestingModule({
providers: [
CommentsService,
+ {
+ provide: getRepositoryToken(Comment),
+ useValue: mockCommentRepository,
+ },
],
}).compile();• Import getRepositoryToken from @nestjs/typeorm.
• Ensure mockCommentRepository implements every repository method CommentsService calls.
• This will prevent “Nest can’t resolve dependencies” errors when running the spec.
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [CommentsService], | ||
| }).compile(); |
There was a problem hiding this comment.
Fix DI: provide repository mocks for CommentsService dependencies
Instantiating the real service without its repository providers will fail. Provide mocks for repositories it injects (at least Comment; include BuyerRequest/User if used for existence/ownership checks).
- const module: TestingModule = await Test.createTestingModule({
- providers: [CommentsService],
- }).compile();
+ const module: TestingModule = await Test.createTestingModule({
+ providers: [
+ CommentsService,
+ { provide: getRepositoryToken(Comment), useValue: {
+ create: jest.fn(), save: jest.fn(), findAndCount: jest.fn(),
+ findOne: jest.fn(), delete: jest.fn()
+ } },
+ // Include these only if injected by the service:
+ { provide: getRepositoryToken(BuyerRequest), useValue: {} },
+ { provide: getRepositoryToken(User), useValue: {} },
+ ],
+ }).compile();Supporting imports needed outside the selected lines:
+ import { getRepositoryToken } from '@nestjs/typeorm';
+ import { Comment } from '../entities/comment.entity';
+ import { BuyerRequest } from '@/modules/buyer-requests/entities/buyer-request.entity';
+ import { User } from '@/modules/users/entities/user.entity';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [CommentsService], | |
| }).compile(); | |
| // Add these imports alongside your existing ones: | |
| import { getRepositoryToken } from '@nestjs/typeorm'; | |
| import { Comment } from '../entities/comment.entity'; | |
| import { BuyerRequest } from '@/modules/buyer-requests/entities/buyer-request.entity'; | |
| import { User } from '@/modules/users/entities/user.entity'; | |
| const module: TestingModule = await Test.createTestingModule({ | |
| providers: [ | |
| CommentsService, | |
| { | |
| provide: getRepositoryToken(Comment), | |
| useValue: { | |
| create: jest.fn(), | |
| save: jest.fn(), | |
| findAndCount: jest.fn(), | |
| findOne: jest.fn(), | |
| delete: jest.fn(), | |
| } | |
| }, | |
| // Include these only if the service actually injects them: | |
| { provide: getRepositoryToken(BuyerRequest), useValue: {} }, | |
| { provide: getRepositoryToken(User), useValue: {} }, | |
| ], | |
| }).compile(); |
| import { UserRole } from '../../auth/entities/user-role.entity'; | ||
| import { Notification } from '../../notifications/entities/notification.entity'; | ||
| import { Wishlist } from '../../wishlist/entities/wishlist.entity'; | ||
| import { Comment } from '@/modules/comments/entities/comment.entity'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm tsconfig path alias for '@/...' is configured in tests and runtime
The alias '@/modules/...' requires tsconfig paths and Jest/ts-jest config. Verify it exists to avoid import resolution errors.
🏁 Script executed:
#!/bin/bash
# Check for path alias configuration
echo "tsconfig path mappings (if any):"
fd -a 'tsconfig*.json' | xargs -I{} sh -c 'echo "--- {} ---"; rg -n -C2 "\"paths\"\\s*:" "{}" || true; echo'
echo
echo "References to '@/modules' imports:"
rg -n -C1 -P "@/modules/" src | sed -n '1,200p'Length of output: 2979
🏁 Script executed:
#!/bin/bash
echo "1. Listing Jest config files:"
fd -a "jest.config.*" || true
echo
echo "2. Checking for moduleNameMapper in those files:"
fd -a "jest.config.*" | xargs -I{} sh -c 'echo "--- {} ---"; rg -n -C2 "moduleNameMapper" "{}" || echo "no mapping found"; echo'
echo
echo "3. Searching package.json for Jest settings (moduleNameMapper/modulePaths/moduleDirectories):"
rg -n -C2 '"jest"\s*:' package.json || true
rg -n -C2 "moduleNameMapper" package.json || true
rg -n -C2 "modulePaths" package.json || true
rg -n -C2 "moduleDirectories" package.json || trueLength of output: 1182
🏁 Script executed:
#!/bin/bash
echo "1. Checking for tsconfig-paths or module-alias in dependencies:"
rg -n '"tsconfig-paths"' package.json || echo "➤ tsconfig-paths not found"
rg -n '"module-alias"' package.json || echo "➤ module-alias not found"
echo
echo "2. Searching for runtime registration of path aliases:"
rg -n "module-alias/register" -C1 src || echo "➤ no module-alias/register usage"
rg -n "tsconfig-paths/register" -C1 src || echo "➤ no tsconfig-paths/register usage"Length of output: 668
Add path-alias support for '@/...' in Jest and at runtime
The @/* alias is defined in tsconfig, but:
- Jest’s
moduleNameMapperonly handles^src/(.*)$:—it needs a mapping for^@/(.*)$. - At runtime,
tsconfig-pathsis installed but never registered, so Node won’t resolve@/...imports.
Please update:
- jest.config.js (around line 11):
moduleNameMapper: { - '^src/(.*)$': '/src/$1'
- '^src/(.*)$': '/src/$1',
- '^@/(.*)$': '/src/$1'
},
- **Runtime entrypoint** (e.g. at the very top of `src/main.ts` or `index.ts`):
```ts
import 'tsconfig-paths/register';
or add
require('tsconfig-paths/register');This ensures both tests and the running app correctly resolve @/... imports.
🤖 Prompt for AI Agents
In src/modules/users/entities/user.entity.ts around line 13, imports using the
'@/...' alias won't resolve in tests or at runtime; update jest.config.js
(around line 11) to add a moduleNameMapper entry mapping '^@/(.*)$' to
'<rootDir>/src/$1' so Jest recognizes '@/...' imports, and register
tsconfig-paths at app startup by adding an import or require for
'tsconfig-paths/register' at the very top of your runtime entrypoint (e.g.,
src/main.ts or index.ts) so Node resolves the path aliases at runtime.
| import { AppModule } from '../src/app.module'; | ||
| import { createTestUser } from './factories/user.factory'; | ||
| import { createBuyerRequest } from './factories/buyer-request.factory'; | ||
| import { getConnection } from 'typeorm'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TypeORM connection cleanup: verify v0.2 vs v0.3 API usage
getConnection() is removed in TypeORM v0.3+. If the repo uses v0.3+, use DataSource instead (e.g., app.get(DataSource).dropDatabase()), or getDataSourceToken() from @nestjs/typeorm.
Verification script to confirm the TypeORM version and API usage across the repo:
If on v0.3+, prefer:
// replace: import { getConnection } from 'typeorm';
import { DataSource } from 'typeorm';
// replace in afterAll:
await app.get(DataSource).dropDatabase();Also applies to: 33-36
🏁 Script executed:
#!/bin/bash
# Check installed TypeORM version (from package.json)
rg -nP '"typeorm"\s*:\s*".*"' -C2 package.json
# See if getConnection() is used elsewhere
rg -nP '\bgetConnection\s*\(' -C2
# See if DataSource is used in the codebase
rg -nP '\bDataSource\b' -C3Length of output: 4873
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' test/comments.e2e-spec.ts || trueLength of output: 2561
Replace getConnection() with DataSource in e2e cleanup
TypeORM v0.3+ removes getConnection(). Update the import and teardown in test/comments.e2e-spec.ts:
• File: test/comments.e2e-spec.ts
– Line 3: replace getConnection import
– Lines 31–36: switch from getConnection().dropDatabase() to using app.get(DataSource)
-import { getConnection } from 'typeorm';
+import { DataSource } from 'typeorm';
afterAll(async () => {
- await getConnection().dropDatabase();
+ const dataSource = app.get(DataSource);
+ await dataSource.dropDatabase();
await app.close();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { getConnection } from 'typeorm'; | |
| // test/comments.e2e-spec.ts | |
| import { DataSource } from 'typeorm'; | |
| // …other imports and test setup… | |
| afterAll(async () => { | |
| const dataSource = app.get(DataSource); | |
| await dataSource.dropDatabase(); | |
| await app.close(); | |
| }); |
🤖 Prompt for AI Agents
In test/comments.e2e-spec.ts around lines 7 and 31–36, replace the TypeORM v0.2
getConnection usage with the new DataSource API: change the import of
getConnection to import { DataSource } from 'typeorm', and in the test teardown
obtain the DataSource instance from the Nest app (const ds =
app.get(DataSource)) then call ds.dropDatabase() (and optionally ds.destroy())
instead of getConnection().dropDatabase(); update any type references
accordingly.
| let userA: any; | ||
| let userB: any; | ||
| let buyerRequestId: number; | ||
| let commentId: number; |
There was a problem hiding this comment.
Fix commentId type: Comment IDs are UUID strings, not numbers
Per the PR discussion, Comment.id is a UUID. This test declares commentId as a number, which will cause type mismatches and potential runtime issues.
Apply this diff:
- let commentId: number;
+ let commentId: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let commentId: number; | |
| let commentId: string; |
🤖 Prompt for AI Agents
In test/comments.e2e-spec.ts around line 14, commentId is declared as a number
but Comment.id is a UUID string; change the declaration to a string (or a
UUID-typed string) and update any assignments and assertions that expect a
numeric id to treat it as a string (e.g., assign response.body.id or
response.body.data.id to commentId, and use string comparisons or UUID
validators where needed).
|
@Villarley Please i noticed u merged and then reverted this |
|
I will close the issue, accidentally merged. I'll reopen it, pls apply again. |
Okay |
|
Next time merge into develop and mark it as pr in progress |
|
@Villarley Still haven't seen the new issue |
2 participants
🚀 StarShop Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
GET /buyer-requests/:id/commentsendpoint, with a default limit of 20 comments per page and support for an optional page query parameter.📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
npmcommand🌌 Comments
npm run typeorm migration:runThank you for contributing to StarShop, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
Tests