Feat/remove disabled files #189
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
WalkthroughMakes wallet optional on User entity and IUser interface, adds optional id to UpdateUserDto, replaces disabled/stub controllers with implemented UserController and UserVolunteerController using Prisma/use-cases, and updates userRoutes to wire new controllers and add user–volunteer endpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router
participant A as AuthMiddleware
participant UC as UserController
participant Ux as Use Case(s)
participant Repo as PrismaUserRepository
C->>R: HTTP request (create/get/update/delete user)
R->>A: Authenticate
A-->>R: OK or 401
alt Auth OK
R->>UC: Invoke handler
UC->>Ux: Execute use-case
Ux->>Repo: Persist/query
Repo-->>Ux: Result/NotFound/Error
alt Success
Ux-->>UC: User/ack
UC-->>C: 200/201 with payload
else Not Found
Ux-->>UC: null
UC-->>C: 404
else Error
Ux-->>UC: throw
UC-->>C: 400 with message
end
else Auth Fail
R-->>C: 401 Unauthorized
end
sequenceDiagram
autonumber
participant C as Client
participant R as Router
participant A as AuthMiddleware
participant UVC as UserVolunteerController
participant P as PrismaClient
C->>R: POST/GET/DELETE user–volunteer
R->>A: Authenticate
A-->>R: OK or 401
alt Auth OK
R->>UVC: Handler (add/list/remove)
UVC->>P: find/create/findMany/deleteMany
alt Success / created
P-->>UVC: Data / count
UVC-->>C: 200/201 (or 404 if none deleted)
else Error
P-->>UVC: Error
UVC-->>C: 500 with message
end
else
R-->>C: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/userRoutes.ts (1)
33-40: Route conflict:/users/:idshadows/users/:emailBoth are param routes; the first will catch all, making the email route unreachable. Also, the controller reads
req.query.email, notreq.params.Fix by using a non-conflicting path and param:
-router.get( - "/users/:email", - wrapHandler(userController.getUserByEmail.bind(userController)) -); +router.get( + "/users/by-email/:email", + wrapHandler(userController.getUserByEmail.bind(userController)) +);And update the controller to read
req.params.email:// in UserController.getUserByEmail const { email } = req.params; // instead of req.query
🧹 Nitpick comments (11)
src/modules/user/dto/UpdateUserDto.ts (1)
11-13: Validateidas UUID instead of string
User.idis a UUID. Validate accordingly to catch malformed IDs at the edge.Apply:
@@ -import { +import { IsString, IsEmail, MinLength, MaxLength, IsOptional, Matches, + IsUUID, } from "class-validator"; @@ - @IsOptional() - @IsString({ message: "ID must be a string" }) - id?: string; + @IsOptional() + @IsUUID("4", { message: "ID must be a valid UUID" }) + id?: string;src/modules/user/domain/entities/User.entity.ts (1)
26-30: Make nullable columns optional at type level
verificationTokenandverificationTokenExpiresare nullable in the DB but non-optional in TypeScript. Make them optional to avoid unsafe access.@Column({ nullable: true }) - verificationToken: string; + verificationToken?: string; @Column({ type: "timestamp", nullable: true }) - verificationTokenExpires: Date; + verificationTokenExpires?: Date;src/modules/user/presentation/controllers/userVolunteer.controller.ts (5)
4-4: Share a single PrismaClient instanceInstantiating a client per module can exhaust connections (especially under hot-reload). Centralize a singleton (e.g., src/prismaClient.ts) and import it here.
Example:
// src/prismaClient.ts import { PrismaClient } from "@prisma/client"; export const prisma = globalThis.prisma ?? new PrismaClient(); if (process.env.NODE_ENV !== "production") (globalThis as any).prisma = prisma;Then:
-import { PrismaClient } from "@prisma/client"; -const prisma = new PrismaClient(); +import { prisma } from "../../../prismaClient";
41-58: Consider returning the related entities onlyCurrent response includes join rows; many consumers expect an array of volunteers. Optional: map to
userVolunteers.map(x => x.volunteer).
60-77: Same as above for users listingReturn just the users if that’s the expected API.
97-99: Prefer 204 No Content on successful deletionSemantically cleaner for delete operations; body not needed.
- res.status(200).json({ - message: "User removed from volunteer successfully", - }); + res.status(204).send();
7-9: Authorization check (beyond authentication)Endpoints are auth-protected but lack authorization (e.g., only admins/owners can link/unlink). If roles exist, enforce them here or in middleware.
Also applies to: 79-105
src/routes/userRoutes.ts (1)
49-82: User–Volunteer routes wiring looks good; minor DX nits
- Good: auth on all new routes; handlers wrapped with centralized error propagation.
- Nit: If you switch delete to 204, ensure clients don’t expect JSON.
src/modules/user/presentation/controllers/UserController.ts (3)
1-10: Import class-validator; consolidate use-case imports.Enables DTO validation and reduces import noise.
-import { Request, Response } from "express"; -import { CreateUserUseCase } from "../../use-cases/userUseCase"; -import { GetUserByIdUseCase } from "../../use-cases/userUseCase"; -import { GetUserByEmailUseCase } from "../../use-cases/userUseCase"; -import { UpdateUserUseCase } from "../../use-cases/userUseCase"; -import { DeleteUserUseCase } from "../../use-cases/userUseCase"; +import { Request, Response } from "express"; +import { validate } from "class-validator"; +import { + CreateUserUseCase, + GetUserByIdUseCase, + GetUserByEmailUseCase, + UpdateUserUseCase, + DeleteUserUseCase, +} from "../../use-cases/userUseCase";
12-26: Loosen coupling by injecting the repository (keeps default for compat/testing).Makes the controller easier to test and swap implementations; no behavior change.
-export default class UserController { - private userRepository: PrismaUserRepository; +export default class UserController { + private userRepository: PrismaUserRepository; @@ - constructor() { - this.userRepository = new PrismaUserRepository(); + constructor(repo?: PrismaUserRepository) { + this.userRepository = repo ?? new PrismaUserRepository(); this.createUserUseCase = new CreateUserUseCase(this.userRepository); this.getUserByIdUseCase = new GetUserByIdUseCase(this.userRepository); this.getUserByEmailUseCase = new GetUserByEmailUseCase(this.userRepository); this.updateUserUseCase = new UpdateUserUseCase(this.userRepository); this.deleteUserUseCase = new DeleteUserUseCase(this.userRepository); }
28-35: Optional: Set Location header for Created responses.Improves REST semantics without changing body shape.
- const user = await this.createUserUseCase.execute(userDto); - res.status(201).json(this.toSafeUser(user)); + const user = await this.createUserUseCase.execute(userDto); + if (user?.id) res.setHeader("Location", `/users/${user.id}`); + res.status(201).json(this.toSafeUser(user));
📜 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 (10)
src/modules/user/domain/entities/User.entity.ts(1 hunks)src/modules/user/domain/interfaces/IUser.ts(1 hunks)src/modules/user/dto/UpdateUserDto.ts(1 hunks)src/modules/user/presentation/controllers/UserController.disabled(0 hunks)src/modules/user/presentation/controllers/UserController.stub.ts(0 hunks)src/modules/user/presentation/controllers/UserController.ts(1 hunks)src/modules/user/presentation/controllers/userVolunteer.controller.disabled(0 hunks)src/modules/user/presentation/controllers/userVolunteer.controller.stub.ts(0 hunks)src/modules/user/presentation/controllers/userVolunteer.controller.ts(1 hunks)src/routes/userRoutes.ts(2 hunks)
💤 Files with no reviewable changes (4)
- src/modules/user/presentation/controllers/UserController.stub.ts
- src/modules/user/presentation/controllers/userVolunteer.controller.stub.ts
- src/modules/user/presentation/controllers/userVolunteer.controller.disabled
- src/modules/user/presentation/controllers/UserController.disabled
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/user/presentation/controllers/userVolunteer.controller.ts (1)
tests/__mocks__/prisma.ts (1)
PrismaClient(1-3)
src/modules/user/presentation/controllers/UserController.ts (4)
src/modules/user/repositories/PrismaUserRepository.ts (1)
PrismaUserRepository(7-80)src/modules/user/use-cases/userUseCase.ts (5)
CreateUserUseCase(7-22)GetUserByIdUseCase(24-39)GetUserByEmailUseCase(41-56)UpdateUserUseCase(78-84)DeleteUserUseCase(70-76)src/modules/user/dto/CreateUserDto.ts (1)
CreateUserDto(10-41)src/modules/user/dto/UpdateUserDto.ts (1)
UpdateUserDto(10-49)
src/routes/userRoutes.ts (3)
src/modules/user/presentation/controllers/UserController.ts (1)
UserController(11-105)src/modules/user/presentation/controllers/userVolunteer.controller.ts (1)
UserVolunteerController(6-106)src/middleware/authMiddleware.ts (1)
authMiddleware(13-56)
🔇 Additional comments (1)
src/modules/user/domain/entities/User.entity.ts (1)
20-21: Wallet optional + nullable: OKMatches the interface change. Keep an eye on uniqueness semantics with NULLs (Postgres/MySQL allow multiple NULLs), which is usually desired here.
Ensure a migration alters
wallettoNULLand retains the unique index. If not present, generate one.
| email: string; | ||
| password: string; | ||
| wallet: string; | ||
| wallet?: string; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wallet made optional: contract change looks good; confirm downstream assumptions
Aligns with the entity change. Please verify no service/mapper still treats wallet as required and that a DB migration exists to make users.wallet nullable in all environments.
Run:
🏁 Script executed:
#!/bin/bash
# Find places that may still require a wallet or assume non-null
rg -nP --type=ts -C2 '\bwallet\s*:\s*string\b|\.wallet\b(?!\?)'Length of output: 5604
🏁 Script executed:
#!/bin/bash
# List files with 'migration' in name or content
rg -i migration -l || echo "No files with 'migration' in name/content"
# Check for altering users.wallet nullability in those files
rg -nP 'wallet.*nullable|nullable.*wallet|alter.*wallet' -g '*migration*' -C2 || echo "No wallet nullability changes found in migration files"Length of output: 1013
Add missing DB migration and update code to handle optional wallet
- No migration found modifying
users.walletto nullable; add a migration (e.g. via Prisma) to alter the column in all environments. - Update downstream code still assuming non-null wallets:
- src/modules/user/use-cases/userUseCase.ts:
user.wallet = data.wallet - src/modules/auth/dto/emailVerification.dto.ts:
wallet: stringand default"" - src/repository/IUserRepository.ts & src/repository/user.repository.ts:
walletparameter still required - Integration and entity tests in
__tests__still expectwalletto always be present
- src/modules/user/use-cases/userUseCase.ts:
🤖 Prompt for AI Agents
In src/modules/user/domain/interfaces/IUser.ts around line 7, users.wallet was
made optional but there is no DB migration and multiple callers still assume a
non-null wallet; create and apply a DB migration (e.g. Prisma migration) to
alter users.wallet to nullable in all environments, then update code to accept
and propagate an optional wallet: change assignments like user.wallet =
data.wallet to handle undefined/null (e.g. only assign when present or allow
null), change DTOs (src/modules/auth/dto/emailVerification.dto.ts) to make
wallet optional (remove default "" and type string -> string | undefined),
update repository interface and implementation
(src/repository/IUserRepository.ts & src/repository/user.repository.ts) to
accept an optional wallet parameter, and adjust integration/entity tests in
__tests__ to account for nullable wallets (add scenarios for missing wallet and
update expectations); run migrations, update seeds and regenerate client if
applicable.
Comment on lines
+28
to
+40
| async createUser(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const userDto = new CreateUserDto(); | ||
| Object.assign(userDto, req.body); | ||
|
|
||
| const user = await this.createUserUseCase.execute(userDto); | ||
| res.status(201).json(user); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent leaking sensitive fields in create response; add DTO validation and consistent error mapping.
Currently, createUser returns the raw user object, which likely includes password (hashed) and possibly verification token fields. Also, DTO decorators aren’t enforced without explicit validation, and error mapping is inconsistent with the intended 404/409 semantics.
Apply this diff in createUser:
async createUser(req: Request, res: Response): Promise<void> {
try {
- const userDto = new CreateUserDto();
- Object.assign(userDto, req.body);
-
- const user = await this.createUserUseCase.execute(userDto);
- res.status(201).json(user);
+ const userDto = Object.assign(new CreateUserDto(), req.body);
+ if (!(await this.validateOr400(userDto, res))) return;
+ const user = await this.createUserUseCase.execute(userDto);
+ res.status(201).json(this.toSafeUser(user));
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}Add the helper methods shown in a later comment to support toSafeUser, validateOr400, and handleError.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/user/presentation/controllers/UserController.ts around lines 28
to 40, the createUser method currently returns raw user objects (potentially
leaking sensitive fields like password and verification tokens), lacks DTO
validation enforcement, and maps errors inconsistently; fix it by 1) validating
the incoming CreateUserDto before use (e.g., run class-validator validation and
return 400 with validation errors when invalid) via a validateOr400 helper that
inspects validation errors and responds with res.status(400).json({ errors: ...
}); 2) after the use case returns a user, convert it to a safe representation
using a toSafeUser helper that strips sensitive fields (password,
verificationToken, resetToken, etc.) and only returns public fields (id, email,
name, createdAt, etc.); 3) centralize error mapping with a handleError helper
that maps domain errors to appropriate HTTP statuses (e.g., 409 for conflict,
404 for not found, 400 for bad request, 500 for unexpected) and sends consistent
JSON error responses; and 4) update createUser to call validateOr400(req,
userDto, res) before executing the use case, wrap the use case call in try/catch
that delegates to handleError on failure, and respond with
res.status(201).json(toSafeUser(user)) on success.
Comment on lines
+42
to
+56
| async getUserById(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const { id } = req.params; | ||
| const user = await this.getUserByIdUseCase.execute(id); | ||
| if (!user) { | ||
| res.status(404).json({ error: "User not found" }); | ||
| return; | ||
| } | ||
| res.status(200).json(user); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unreachable 404 branch; validate id param; return safe payload; unify error handling.
GetUserByIdUseCase throws on missing/not-found, so the if (!user) 404 block never runs. Also, guard for missing id and sanitize response.
async getUserById(req: Request, res: Response): Promise<void> {
try {
- const { id } = req.params;
- const user = await this.getUserByIdUseCase.execute(id);
- if (!user) {
- res.status(404).json({ error: "User not found" });
- return;
- }
- res.status(200).json(user);
+ const { id } = req.params;
+ if (!id) {
+ res.status(400).json({ error: "User ID is required" });
+ return;
+ }
+ const user = await this.getUserByIdUseCase.execute(id);
+ res.status(200).json(this.toSafeUser(user));
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+58
to
+76
| async getUserByEmail(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const { email } = req.query; | ||
| if (!email) { | ||
| res.status(400).json({ error: "Email is required" }); | ||
| return; | ||
| } | ||
| const user = await this.getUserByEmailUseCase.execute(email as string); | ||
| if (!user) { | ||
| res.status(404).json({ error: "User not found" }); | ||
| return; | ||
| } | ||
| res.status(200).json(user); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden query param parsing; remove unreachable 404; return safe payload; unify error handling.
req.query.email can be a string or string[]; the current as string cast is unsafe. The use-case throws on not found, so the if (!user) branch is redundant.
async getUserByEmail(req: Request, res: Response): Promise<void> {
try {
- const { email } = req.query;
- if (!email) {
- res.status(400).json({ error: "Email is required" });
- return;
- }
- const user = await this.getUserByEmailUseCase.execute(email as string);
- if (!user) {
- res.status(404).json({ error: "User not found" });
- return;
- }
- res.status(200).json(user);
+ const raw = req.query.email;
+ const email =
+ typeof raw === "string" ? raw : Array.isArray(raw) ? raw[0] : "";
+ if (!email) {
+ res.status(400).json({ error: "Email is required" });
+ return;
+ }
+ const user = await this.getUserByEmailUseCase.execute(email);
+ res.status(200).json(this.toSafeUser(user));
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/user/presentation/controllers/UserController.ts around lines 58
to 76, tighten query param parsing and simplify response/error flows: validate
req.query.email safely (handle string | string[] | undefined) by extracting a
single string (e.g., if array take the first element) and check it's a non-empty
string before proceeding; remove the unreachable if (!user) 404 branch since the
use-case throws when not found; before res.json(user) sanitize the user object
to exclude sensitive fields (password, tokens, etc.) and only return a safe DTO;
unify error handling in the catch to map known error types/names from the
use-case to appropriate status codes (e.g., NotFound -> 404, Validation -> 400)
and default to 500 for unexpected errors, including returning a generic message
while logging the real error for diagnostics.
Comment on lines
+78
to
+92
| async updateUser(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const { id } = req.params; | ||
| const userDto = new UpdateUserDto(); | ||
| Object.assign(userDto, req.body); | ||
| userDto.id = id; | ||
|
|
||
| await this.updateUserUseCase.execute(userDto); | ||
| res.status(200).json({ message: "User updated successfully" }); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate update DTO; guard missing id; unify error handling.
Without explicit validation, DTO decorators don’t run. Also, Prisma will throw on missing/invalid id; add a param guard.
async updateUser(req: Request, res: Response): Promise<void> {
try {
- const { id } = req.params;
- const userDto = new UpdateUserDto();
- Object.assign(userDto, req.body);
- userDto.id = id;
-
- await this.updateUserUseCase.execute(userDto);
+ const { id } = req.params;
+ if (!id) {
+ res.status(400).json({ error: "User ID is required" });
+ return;
+ }
+ const userDto = Object.assign(new UpdateUserDto(), req.body);
+ userDto.id = id;
+ if (!(await this.validateOr400(userDto, res))) return;
+ await this.updateUserUseCase.execute(userDto);
res.status(200).json({ message: "User updated successfully" });
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/user/presentation/controllers/UserController.ts around lines 78
to 92, the updateUser method fails to run DTO validation and doesn't guard the
id param: ensure the UpdateUserDto is validated (e.g., call class-validator's
validate or validateOrReject on userDto after Object.assign and before executing
the use case), add an explicit check that req.params.id exists and is in the
expected format (return 400 with a clear message if missing/invalid) instead of
relying on Prisma to throw, and unify error handling by mapping
validation/client errors to 400 and unexpected errors to 500 (include
error.message in logs but return a generic message for 500 responses).
Comment on lines
+94
to
+105
| async deleteUser(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const { id } = req.params; | ||
| await this.deleteUserUseCase.execute(id); | ||
| res.status(200).json({ message: "User deleted successfully" }); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add centralized helpers for safe responses, validation, and error mapping (P2002 → 409, P2025/not found → 404).
Reduces duplication and enforces consistent status codes aligned with the intended API.
async deleteUser(req: Request, res: Response): Promise<void> {
try {
const { id } = req.params;
if (!id) {
res.status(400).json({ error: "User ID is required" });
return;
}
await this.deleteUserUseCase.execute(id);
res.status(200).json({ message: "User deleted successfully" });
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}
+
+ // Centralized helpers
+ private toSafeUser(user: any) {
+ if (!user) return user;
+ const { password, verificationToken, verificationTokenExpires, ...safe } = user;
+ return safe;
+ }
+
+ private async validateOr400(dto: object, res: Response): Promise<boolean> {
+ const errors = await validate(dto as any);
+ if (errors.length > 0) {
+ res.status(400).json({
+ errors: errors.map((e: any) => ({
+ field: e.property,
+ constraints: e.constraints ?? {},
+ })),
+ });
+ return false;
+ }
+ return true;
+ }
+
+ private handleError(res: Response, error: unknown): void {
+ if (error && typeof error === "object" && "code" in (error as any)) {
+ const code = (error as any).code;
+ if (code === "P2002") {
+ res.status(409).json({ error: "Email already in use" });
+ return;
+ }
+ if (code === "P2025") {
+ res.status(404).json({ error: "User not found" });
+ return;
+ }
+ }
+ if (error instanceof Error && /not found/i.test(error.message)) {
+ res.status(404).json({ error: error.message });
+ return;
+ }
+ res.status(400).json({
+ error: error instanceof Error ? error.message : "Unknown error",
+ });
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/user/presentation/controllers/UserController.ts around lines 94
to 105, replace the inline try/catch and direct res.json usage with centralized
helpers: validate the id param with the existing validation helper (return a 400
via the safe response helper on invalid input), call deleteUserUseCase, then
return success via the safeResponse helper; in the catch, pass the thrown error
into the shared error-mapping utility so DB/ORM errors map P2002 → 409 and
P2025/not-found → 404 (fallback to 500 or use error.message for unknown errors)
and send the mapped status and payload via the safe response helper to ensure
consistent responses across controllers.
Comment on lines
+94
to
+104
| async deleteUser(req: Request, res: Response): Promise<void> { | ||
| try { | ||
| const { id } = req.params; | ||
| await this.deleteUserUseCase.execute(id); | ||
| res.status(200).json({ message: "User deleted successfully" }); | ||
| } catch (error: unknown) { | ||
| res.status(400).json({ | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard missing id; map known errors (404/409) via centralized handler.
Keep 200 for compatibility, but ensure param validation and consistent error mapping.
async deleteUser(req: Request, res: Response): Promise<void> {
try {
- const { id } = req.params;
- await this.deleteUserUseCase.execute(id);
+ const { id } = req.params;
+ if (!id) {
+ res.status(400).json({ error: "User ID is required" });
+ return;
+ }
+ await this.deleteUserUseCase.execute(id);
res.status(200).json({ message: "User deleted successfully" });
} catch (error: unknown) {
- res.status(400).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
+ this.handleError(res, error);
}
}📝 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.
Suggested change
| async deleteUser(req: Request, res: Response): Promise<void> { | |
| try { | |
| const { id } = req.params; | |
| await this.deleteUserUseCase.execute(id); | |
| res.status(200).json({ message: "User deleted successfully" }); | |
| } catch (error: unknown) { | |
| res.status(400).json({ | |
| error: error instanceof Error ? error.message : "Unknown error", | |
| }); | |
| } | |
| } | |
| async deleteUser(req: Request, res: Response): Promise<void> { | |
| try { | |
| const { id } = req.params; | |
| if (!id) { | |
| res.status(400).json({ error: "User ID is required" }); | |
| return; | |
| } | |
| await this.deleteUserUseCase.execute(id); | |
| res.status(200).json({ message: "User deleted successfully" }); | |
| } catch (error: unknown) { | |
| this.handleError(res, error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/modules/user/presentation/controllers/UserController.ts around lines 94
to 104, the deleteUser handler lacks validation of the id param and bypasses the
centralized error mapper; first validate that req.params.id exists and return a
400 response immediately if missing, then change the signature to accept next
(req, res, next) and, inside the catch, call next(error) instead of responding
directly so the centralized error handler can map domain errors to 404/409; keep
the 200 JSON response on success.
Comment on lines
+12
to
+25
| const existing = await prisma.userVolunteer.findFirst({ | ||
| where: { | ||
| userId, | ||
| volunteerId, | ||
| }, | ||
| }); | ||
|
|
||
| if (existing) { | ||
| res.status(400).json({ | ||
| error: "User is already assigned to this volunteer", | ||
| }); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate links under race; rely on DB uniqueness and return 409
The pre-check + create pattern is racy. Enforce a unique composite on (userId, volunteerId) and handle Prisma P2002 for idempotency.
Apply:
-import { PrismaClient } from "@prisma/client";
+import { PrismaClient, Prisma } from "@prisma/client";
@@
- // Check if the relationship already exists
- const existing = await prisma.userVolunteer.findFirst({
- where: {
- userId,
- volunteerId,
- },
- });
-
- if (existing) {
- res.status(400).json({
- error: "User is already assigned to this volunteer",
- });
- return;
- }
-
const userVolunteer = await prisma.userVolunteer.create({
data: {
userId,
volunteerId,
},
});
@@
- } catch (error) {
- res.status(500).json({
- error: error instanceof Error ? error.message : "Unknown error",
- });
- }
+ } catch (error) {
+ if (
+ error instanceof Prisma.PrismaClientKnownRequestError &&
+ error.code === "P2002"
+ ) {
+ res.status(409).json({ error: "User is already assigned to this volunteer" });
+ return;
+ }
+ res.status(500).json({
+ error: error instanceof Error ? error.message : "Unknown error",
+ });
+ }And in Prisma schema (model name may vary):
model UserVolunteer {
id String @id @default(uuid())
userId String
volunteerId String
// ...
@@unique([userId, volunteerId])
}Also applies to: 26-38
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Issue: User module contained legacy or stub files marked as disabled that needed to be removed to avoid confusion and reduce tech debt.
Changes Made:
Removed Disabled Files:
UserController.disabled(legacy implementation)userVolunteer.controller.disabled(legacy implementation)UserController.stub.ts(temporary stub)userVolunteer.controller.stub.ts(temporary stub)Created New Controllers:
UserController.ts: New implementation using existing use cases and repository patternuserVolunteer.controller.ts: New implementation with direct Prisma operations for user-volunteer relationshipsUpdated DTOs and Entities:
idproperty toUpdateUserDtoto support controller operationswalletproperty optional inIUserinterface andUser.entity.tsnullable: truefor wallet fieldFixed Routes and Imports:
userRoutes.tsto import from new controllers instead of stubsPOST /users/:userId/volunteers/:volunteerId- Add user to volunteerGET /users/:userId/volunteers- Get volunteers by user IDGET /volunteers/:volunteerId/users- Get users by volunteer IDDELETE /users/:userId/volunteers/:volunteerId- Remove user from volunteerMaintained API Compatibility: All existing endpoints continue to work with the same request/response structure
📸 Evidence (A photo is required as evidence)
Before: User module contained 4 disabled/stub files that were causing confusion and tech debt.
After: User module now contains only 2 properly implemented controllers with full functionality.
Files Removed:
src/modules/user/presentation/controllers/UserController.disabledsrc/modules/user/presentation/controllers/userVolunteer.controller.disabledsrc/modules/user/presentation/controllers/UserController.stub.tssrc/modules/user/presentation/controllers/userVolunteer.controller.stub.tsFiles Created/Updated:
src/modules/user/presentation/controllers/UserController.ts(new)src/modules/user/presentation/controllers/userVolunteer.controller.ts(new)src/modules/user/dto/UpdateUserDto.ts(updated)src/modules/user/domain/interfaces/IUser.ts(updated)src/modules/user/domain/entities/User.entity.ts(updated)src/routes/userRoutes.ts(updated)⏰ Time spent breakdown
Analysis and Planning: 15 minutes
File Removal: 5 minutes
Controller Implementation: 25 minutes
DTO and Entity Updates: 15 minutes
Route Updates: 10 minutes
Testing and Verification: 10 minutes
Total Time: ~80 minutes
💬 Comments
This refactoring successfully addresses the tech debt issue by removing all disabled/stub files from the user module and replacing them with proper, working implementations. The changes maintain backward compatibility while providing a cleaner, more maintainable codebase.
Key Benefits:
Technical Notes:
Future Considerations:
Thank you for contributing to Volunchain, 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