feat: Association Model #55
Conversation
|
tested with dummy info: |
|
|
relations made: faculty and user |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { UpdateAssociationDto } from './dto/update-association.dto'; | ||
| import { ApiQuery, ApiTags } from '@nestjs/swagger'; | ||
|
|
||
| ApiTags('associations'); |
There was a problem hiding this comment.
| ) {} | ||
|
|
||
| create(createAssociationDto: CreateAssociationDto) { | ||
| return this.associationRepository.save(createAssociationDto); |
There was a problem hiding this comment.
The create method directly saves the DTO to the repository without creating an entity instance first. This approach bypasses any entity-level validations or transformations. Additionally, since CreateAssociationDto is empty, this will not properly handle the nested User entity and Faculty relationship that need to be established. Consider using the repository's create method first to instantiate the entity, then save it.
| return this.associationRepository.save(createAssociationDto); | |
| const association = this.associationRepository.create(createAssociationDto); | |
| return this.associationRepository.save(association); |
| update(id: number, updateAssociationDto: UpdateAssociationDto) { | ||
| return this.associationRepository.update(id, updateAssociationDto); |
There was a problem hiding this comment.
The update method is inconsistent with the project's established pattern in FacultiesService. It uses repository.update() which returns an UpdateResult instead of the updated entity. Following the project's pattern, this should: 1) find the entity first and throw NotFoundException if not found, 2) apply the update, 3) return the updated Association entity. Consider wrapping this in a transaction like FacultiesService does.
| remove(id: number) { | ||
| return this.associationRepository.delete(id); |
There was a problem hiding this comment.
The remove method is inconsistent with the project's established pattern in FacultiesService. It uses repository.delete() which returns a DeleteResult instead of the deleted entity. Following the project's pattern, this should: 1) find the entity first and throw NotFoundException if not found, 2) delete it, 3) return the deleted Association entity. Consider wrapping this in a transaction like FacultiesService does.
| create(@Body() createAssociationDto: CreateAssociationDto) { | ||
| return this.associationsService.create(createAssociationDto); | ||
| } | ||
|
|
||
| // Feature #50: GET /associations?facultyId=1 | ||
| @ApiQuery({ name: 'facultyId', required: false, type: Number }) | ||
| @Get() | ||
| findAll(@Query('facultyId') facultyId?: string) { | ||
| return this.associationsService.findAll(facultyId ? +facultyId : undefined); | ||
| } | ||
|
|
||
| @Get(':id') | ||
| findOne(@Param('id') id: string) { | ||
| return this.associationsService.findOne(+id); | ||
| } | ||
|
|
||
| @Patch(':id') | ||
| update( | ||
| @Param('id') id: string, | ||
| @Body() updateAssociationDto: UpdateAssociationDto, | ||
| ) { |
There was a problem hiding this comment.
The controller is missing ValidationPipe on the Body parameters, which is inconsistent with the project's established pattern in FacultiesController. Without ValidationPipe, DTO validation decorators won't be enforced. Consider adding ValidationPipe to the @Body() decorators for create and update methods.
| @Post() | ||
| create(@Body() createAssociationDto: CreateAssociationDto) { | ||
| return this.associationsService.create(createAssociationDto); | ||
| } | ||
|
|
||
| // Feature #50: GET /associations?facultyId=1 | ||
| @ApiQuery({ name: 'facultyId', required: false, type: Number }) | ||
| @Get() | ||
| findAll(@Query('facultyId') facultyId?: string) { | ||
| return this.associationsService.findAll(facultyId ? +facultyId : undefined); | ||
| } | ||
|
|
||
| @Get(':id') | ||
| findOne(@Param('id') id: string) { | ||
| return this.associationsService.findOne(+id); | ||
| } | ||
|
|
||
| @Patch(':id') | ||
| update( | ||
| @Param('id') id: string, | ||
| @Body() updateAssociationDto: UpdateAssociationDto, | ||
| ) { | ||
| return this.associationsService.update(+id, updateAssociationDto); | ||
| } | ||
|
|
||
| @Delete(':id') | ||
| remove(@Param('id') id: string) { | ||
| return this.associationsService.remove(+id); | ||
| } |
There was a problem hiding this comment.
The controller endpoints are missing API documentation decorators (@apioperation, @apiresponse) that are used consistently in FacultiesController. This makes the Swagger documentation incomplete for the associations endpoints. Consider adding these decorators to document the endpoints properly, including success and error response codes.
| @Post() | ||
| create(@Body() createAssociationDto: CreateAssociationDto) { | ||
| return this.associationsService.create(createAssociationDto); | ||
| } | ||
|
|
||
| // Feature #50: GET /associations?facultyId=1 | ||
| @ApiQuery({ name: 'facultyId', required: false, type: Number }) | ||
| @Get() | ||
| findAll(@Query('facultyId') facultyId?: string) { | ||
| return this.associationsService.findAll(facultyId ? +facultyId : undefined); | ||
| } | ||
|
|
||
| @Get(':id') | ||
| findOne(@Param('id') id: string) { | ||
| return this.associationsService.findOne(+id); | ||
| } | ||
|
|
||
| @Patch(':id') | ||
| update( | ||
| @Param('id') id: string, | ||
| @Body() updateAssociationDto: UpdateAssociationDto, | ||
| ) { | ||
| return this.associationsService.update(+id, updateAssociationDto); | ||
| } | ||
|
|
||
| @Delete(':id') | ||
| remove(@Param('id') id: string) { | ||
| return this.associationsService.remove(+id); | ||
| } |
There was a problem hiding this comment.
The controller is missing authentication guards on sensitive operations (create, update, delete), which is inconsistent with the project's security pattern in FacultiesController. Without @UseGuards(JwtAuthGuard), these endpoints are publicly accessible, allowing anyone to create, modify, or delete associations without authentication. This is a significant security concern. Consider adding @UseGuards(JwtAuthGuard) and @ApiBearerAuth decorators to these endpoints.
| findOne(id: number) { | ||
| return this.associationRepository.findOne({ | ||
| where: { id }, | ||
| relations: ['faculty', 'user'], | ||
| }); |
There was a problem hiding this comment.
The findOne method doesn't handle the case where no association is found, which is inconsistent with the project's established pattern in FacultiesService. Following the project's pattern, this method should throw a NotFoundException when the entity is not found instead of returning null/undefined.
| @Get(':id') | ||
| findOne(@Param('id') id: string) { | ||
| return this.associationsService.findOne(+id); | ||
| } | ||
|
|
||
| @Patch(':id') | ||
| update( | ||
| @Param('id') id: string, | ||
| @Body() updateAssociationDto: UpdateAssociationDto, | ||
| ) { | ||
| return this.associationsService.update(+id, updateAssociationDto); | ||
| } | ||
|
|
||
| @Delete(':id') | ||
| remove(@Param('id') id: string) { | ||
| return this.associationsService.remove(+id); |
There was a problem hiding this comment.
The controller is missing ParseIntPipe on the @param('id') parameters, which is inconsistent with the project's established pattern in FacultiesController. Without ParseIntPipe, invalid id values (like non-numeric strings) won't be validated and could cause runtime errors. Consider adding ParseIntPipe to the id parameters in findOne, update, and remove methods.
Closes #50
Closes #46