Conversation
WalkthroughThis pull request implements cascading delete behavior for polls in the database. It updates SQL migration scripts and the Prisma schema to redefine foreign key constraints on the "UserAction" and "Vote" tables with cascading delete and update options. Additionally, new controller and service methods are introduced to handle poll deletions, which include database transactions and updates to the user's poll count. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PC as PollController
participant PS as PollService
participant DB as Database
Client->>PC: DELETE /poll/:id
PC->>PS: deletePoll(id)
PS->>DB: Check if poll exists
DB-->>PS: {Poll data} / {Not found}
PS->>DB: Begin transaction to delete poll
DB-->>PS: Poll deleted
PS->>DB: Update user's pollsCreatedCount
DB-->>PS: User data updated
PS-->>PC: Return deleted poll data
PC->>Client: 200 JSON response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (4)
src/poll/poll.controller.ts (1)
50-61: Implementation looks good with one minor suggestionThe new
deletePollendpoint correctly uses the service layer to delete the poll and handles errors appropriately. However, consider adding a check if the poll exists before attempting to delete it.Consider adding a 404 response for when the poll isn't found:
@Delete(':id') async deletePoll(@Param('id') id: number, @Res() res: Response) { try { const poll = await this.pollService.deletePoll(Number(id)); + if (!poll) { + return res.status(404).json({ message: 'Poll not found' }); + } return res.status(200).json({ message: 'Poll deleted', poll: poll }); } catch (error) { + if (error.message === 'Poll not found') { + return res.status(404).json({ message: error.message }); + } return res .status(500) .json({ message: 'Internal server error', error: error.message }); } }src/poll/poll.service.ts (1)
138-165: Implementation looks solid with minor suggestion for error handlingThe
deletePollmethod correctly:
- Checks if the poll exists before attempting to delete it
- Uses a transaction to ensure data consistency
- Updates the user's
pollsCreatedCountOne improvement would be to use a more specific error type.
Use
BadRequestExceptionfor better error handling consistency:async deletePoll(pollId: number) { const poll = await this.databaseService.poll.findUnique({ where: { pollId }, }); if (!poll) { - throw new Error('Poll not found'); + throw new BadRequestException('Poll not found'); } return this.databaseService.$transaction(async (tx) => { const deleted = await tx.poll.delete({ where: { pollId, }, }); // Update user's pollsCreatedCount await tx.user.update({ where: { id: deleted.authorUserId }, data: { pollsCreatedCount: { decrement: 1, }, }, }); return deleted; }); }prisma/schema.prisma (2)
35-35: Add space after comma for consistencyThe Prisma schema change correctly adds cascading delete behavior, but there is a minor formatting issue.
- poll Poll @relation(fields: [pollId], references: [pollId],onDelete: Cascade) + poll Poll @relation(fields: [pollId], references: [pollId], onDelete: Cascade)
65-65: Add space after comma for consistencySimilar to the previous comment, there's a minor formatting issue in the relation definition.
- poll Poll @relation(fields: [pollId], references: [pollId],onDelete: Cascade) + poll Poll @relation(fields: [pollId], references: [pollId], onDelete: Cascade)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
prisma/migrations/20250402170145_add_cascade_delete/migration.sql(1 hunks)prisma/schema.prisma(2 hunks)src/poll/poll.controller.ts(1 hunks)src/poll/poll.service.ts(1 hunks)
🔇 Additional comments (1)
prisma/migrations/20250402170145_add_cascade_delete/migration.sql (1)
1-11: SQL migration implements cascading deletion correctlyThe migration properly:
- Drops existing foreign key constraints
- Adds new constraints with CASCADE behavior for both DELETE and UPDATE operations
This ensures that when a poll is deleted, all related user actions and votes will be automatically deleted as well.
|
@lovelgeorge99 I think we should delete the poll only when user is the author of the poll |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/poll/poll.service.ts (3)
138-169: Consider adding error handling for database operations.The method lacks a try-catch block for handling potential database errors, which could make debugging more difficult and lead to unhandled exceptions.
async deletePoll(userId: number, pollId: number) { + try { const poll = await this.databaseService.poll.findUnique({ where: { pollId }, }); if (!poll) { throw new BadRequestException('Poll not found'); } if (poll.authorUserId !== userId) { throw new BadRequestException('User Not Authorized'); } return this.databaseService.$transaction(async (tx) => { const deleted = await tx.poll.delete({ where: { pollId, }, }); // Update user's pollsCreatedCount await tx.user.update({ where: { id: deleted.authorUserId }, data: { pollsCreatedCount: { decrement: 1, }, }, }); return deleted; }); + } catch (error) { + // Log the error for debugging + console.error('Error deleting poll:', error); + throw new BadRequestException('Failed to delete poll'); + } }
138-138: Add JSDoc comments to document the method.Adding documentation would help other developers understand the purpose and behavior of this method.
+ /** + * Deletes a poll if the requesting user is the author. + * This operation will cascade delete related votes and user actions. + * + * @param userId - The ID of the user requesting the deletion + * @param pollId - The ID of the poll to delete + * @returns The deleted poll object + * @throws BadRequestException if poll doesn't exist or user is not authorized + */ async deletePoll(userId: number, pollId: number) {
146-148: Consider using more specific exception classes for authorization failures.NestJS provides more specific exception classes like
ForbiddenExceptionorUnauthorizedExceptionthat would be more appropriate for authorization failures.if (poll.authorUserId !== userId) { - throw new Error('User Not Authorized'); + throw new ForbiddenException('User is not authorized to delete this poll'); }Note: You would need to add the import for
ForbiddenExceptionfrom@nestjs/common.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/poll/poll.controller.ts(1 hunks)src/poll/poll.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/poll/poll.controller.ts
🔇 Additional comments (2)
src/poll/poll.service.ts (2)
150-168: The transaction implementation is correct and maintains data consistency.The method correctly uses a transaction to ensure that both the poll deletion and the user's poll count update are performed atomically, which maintains data consistency.
138-169:Details
✅ Verification successful
Verify that related records are properly cascaded.
According to the PR summary, cascading delete was implemented in the database schema. This method relies on that behavior to automatically delete related votes and user actions when a poll is deleted. Ensure that the Prisma schema and migrations properly set up these cascading relationships.
🏁 Script executed:
#!/bin/bash # Check for cascading delete configuration in Prisma schema echo "Checking Prisma schema for cascading delete configuration..." fd --type f "schema.prisma" --exec cat {} | grep -A 10 "model Vote" | grep "onDelete" fd --type f "schema.prisma" --exec cat {} | grep -A 10 "model UserAction" | grep "onDelete" # Check migration files for cascading delete echo "Checking migration files for cascading delete..." fd --extension sql --exec grep -l "CASCADE" {}Length of output: 783
Cascading Delete Behavior Validated
I verified that the Prisma schema correctly configures cascading deletes for related records. The schema shows that for both Vote and UserAction models the relationships include
onDelete: Cascade, and the migration files confirm CASCADE is applied. No further changes are required for proper cascading behavior in this deletion method.
|
|
||
| async deletePoll(userId: number, pollId: number) { | ||
| const poll = await this.databaseService.poll.findUnique({ | ||
| where: { pollId }, | ||
| }); | ||
|
|
||
| if (!poll) { | ||
| throw new Error('Poll not found'); | ||
| } | ||
| if (poll.authorUserId !== userId) { | ||
| throw new Error('User Not Authorized'); | ||
| } | ||
|
|
||
| return this.databaseService.$transaction(async (tx) => { | ||
| const deleted = await tx.poll.delete({ | ||
| where: { | ||
| pollId, | ||
| }, | ||
| }); | ||
|
|
||
| // Update user's pollsCreatedCount | ||
| await tx.user.update({ | ||
| where: { id: deleted.authorUserId }, | ||
| data: { | ||
| pollsCreatedCount: { | ||
| decrement: 1, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| return deleted; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling by using NestJS exception classes.
The method uses generic Error class for exceptions instead of NestJS's built-in exception classes. This is inconsistent with the rest of the service which uses BadRequestException.
async deletePoll(userId: number, pollId: number) {
const poll = await this.databaseService.poll.findUnique({
where: { pollId },
});
if (!poll) {
- throw new Error('Poll not found');
+ throw new BadRequestException('Poll not found');
}
if (poll.authorUserId !== userId) {
- throw new Error('User Not Authorized');
+ throw new BadRequestException('User Not Authorized');
}📝 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 deletePoll(userId: number, pollId: number) { | |
| const poll = await this.databaseService.poll.findUnique({ | |
| where: { pollId }, | |
| }); | |
| if (!poll) { | |
| throw new Error('Poll not found'); | |
| } | |
| if (poll.authorUserId !== userId) { | |
| throw new Error('User Not Authorized'); | |
| } | |
| return this.databaseService.$transaction(async (tx) => { | |
| const deleted = await tx.poll.delete({ | |
| where: { | |
| pollId, | |
| }, | |
| }); | |
| // Update user's pollsCreatedCount | |
| await tx.user.update({ | |
| where: { id: deleted.authorUserId }, | |
| data: { | |
| pollsCreatedCount: { | |
| decrement: 1, | |
| }, | |
| }, | |
| }); | |
| return deleted; | |
| }); | |
| } | |
| async deletePoll(userId: number, pollId: number) { | |
| const poll = await this.databaseService.poll.findUnique({ | |
| where: { pollId }, | |
| }); | |
| if (!poll) { | |
| throw new BadRequestException('Poll not found'); | |
| } | |
| if (poll.authorUserId !== userId) { | |
| throw new BadRequestException('User Not Authorized'); | |
| } | |
| return this.databaseService.$transaction(async (tx) => { | |
| const deleted = await tx.poll.delete({ | |
| where: { | |
| pollId, | |
| }, | |
| }); | |
| // Update user's pollsCreatedCount | |
| await tx.user.update({ | |
| where: { id: deleted.authorUserId }, | |
| data: { | |
| pollsCreatedCount: { | |
| decrement: 1, | |
| }, | |
| }, | |
| }); | |
| return deleted; | |
| }); | |
| } |
2 participants
Issue : #35
Summary by CodeRabbit