Updated all the pr review changes #4
Conversation
laxman-aqw
commented
Oct 9, 2025
laxman-aqw
commented
- changed message to Username already taken
- changed variable name from existingUsernameUser -> userByUsername and existingEmailUser -> userByEmail
- seperated password hasing logic
- increased expiresIn time to 7d
- used replaced scrypt with bcrypt for password hashing
- added additional validations for username in user.dto.ts
- moved throwException code above next db call code
- added users as prefix in userController
- removed unnecessary routes
- removed length property from password
- updated timestamp with zone in user entity
- marked repository as readonly
- updated function name from findOne -> findOneByUsername
- removed appService from app module
- configured TypeOrmModule in app module and exported as TypeOrmModuleOptions
- added console log after app listen
- updated preetierrc to accept semi colons
There was a problem hiding this comment.
you can link the ERD image here somewhere as well
src/auth/auth.service.ts
Outdated
| private async getHashedPassword(password: string): Promise<string> { | ||
| const salt = await bcrypt.genSalt(); | ||
| const hashedPassword = await bcrypt.hash(password, salt); | ||
| return hashedPassword; | ||
| } |
There was a problem hiding this comment.
change the function name to hashPassword. also it is better to create a separate file for this method so that it can be reuse in other places as well.
| async signUp( | ||
| signUpUserDto: SignUpUserDto, | ||
| ): Promise<{ access_token: string }> { | ||
| const userByUsername = await this.userService.findOneByUsername( | ||
| signUpUserDto.username, | ||
| ); | ||
| if (userByUsername) { | ||
| throw new BadRequestException('Username already taken'); | ||
| } | ||
| const userByEmail = await this.userService.findOneByEmail( | ||
| signUpUserDto.email, | ||
| ); | ||
| if (userByEmail) { | ||
| throw new BadRequestException('Email already taken'); | ||
| } | ||
| const hashedPassword = await this.getHashedPassword(signUpUserDto.password); | ||
| const { email, fullName, username } = signUpUserDto; | ||
| const user = await this.userService.create({ | ||
| password: hashedPassword, | ||
| email, | ||
| fullName, | ||
| username, | ||
| }); | ||
| const payload = { sub: user.id, username: user.username }; | ||
| return { access_token: await this.jwtService.signAsync(payload) }; | ||
| } |
There was a problem hiding this comment.
add line spacing after code blocks for easier readability
src/user/user.controller.ts
Outdated
| import { UserService } from './user.service'; | ||
| import { SignUpUserDto } from 'src/auth/dto/signup-user-dto'; | ||
|
|
||
| @Controller('user') |
There was a problem hiding this comment.
| @Controller('user') | |
| @Controller('users') |
src/user/user.controller.ts
Outdated
| @Get('users') | ||
| async getAllUsers() { | ||
| return this.UserService.findAll(); | ||
| } | ||
|
|
||
| @Get('user/:id') | ||
| async getUser(@Param('id', ParseUUIDPipe) id: string) { | ||
| return this.UserService.findOneById(id); | ||
| } | ||
|
|
||
| @Post('user') | ||
| async createUser(@Body() body: SignUpUserDto) { | ||
| return this.UserService.create(body); | ||
| } |
There was a problem hiding this comment.
since we have already provided the route prefix in controller decorator, we don't need to again add them in each routes.
src/main.ts
Outdated
| whitelist: true, | ||
| transform: true, |
There was a problem hiding this comment.
i don't think we need these. Are you clear what these properties do?
There was a problem hiding this comment.
yes, learned about it in a video so thougt i could use it. will remove it.
| { | ||
| "singleQuote": true, | ||
| "trailingComma": "all", | ||
| "semi": true |
eslint.config.mjs
Outdated
| }, | ||
| { | ||
| rules: { | ||
| "prettier/prettier": ["error", { "semi": true }], |
There was a problem hiding this comment.
no need to add this rule. prettier will automatically take care of semicolons on save now
package.json
Outdated
| "typeorm": "typeorm-ts-node-commonjs -d src/data-source.ts", | ||
| "migration:generate": "pnpm run typeorm migration:generate", | ||
| "migration:run": "pnpm run typeorm migration:run", | ||
| "migration:revert": "pnpm run typeorm migration:revert", |
There was a problem hiding this comment.
we might need to update these scripts for migration. I will guide you in person for this one
There was a problem hiding this comment.
since you are in initial phase of the project, I suggest you to explore absolute imports. you'd need to modify this tsconfig.json file to do so. you can research on relative imports vs absolute imports separately to understand about them further
src/auth/auth.controller.ts
Outdated
| constructor(private authService: AuthService) {} | ||
|
|
||
| @HttpCode(HttpStatus.OK) | ||
| @Post('signUp') |
There was a problem hiding this comment.
Its better to use hyphen (kebab-case), instead of camelCase for endpoints
src/user/user.service.ts
Outdated
| return this.userRepository.find(); | ||
| } | ||
|
|
||
| findOneById(id: string): Promise<User | null> { |
There was a problem hiding this comment.
findOneById , findOneByUsername and findOneByEmail, can probably be generalised to one function.
The current User service seems to be more structured like a repository instead of a service.
src/auth/dto/signup-user-dto.ts
Outdated
| Matches, | ||
| } from 'class-validator'; | ||
|
|
||
| export class SignUpUserDto { |
There was a problem hiding this comment.
From wikipedia,
In the field of programming a data transfer object(DTO) is an object that carries data between processes.
Its not really an appropriate name here. Maybe a SignupRequestData or something similar
3 participants
