-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added posts crud #14
added posts crud #14
Conversation
id, | ||
}, | ||
}); | ||
if (currentPost.authorId !== user.authSchId && user.role !== 'BODY_ADMIN') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a good idea to only allow the original author to edit/delete posts?
id, | ||
}, | ||
}); | ||
if (currentPost.authorId !== user.authSchId && user.role !== 'BODY_ADMIN') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
async update(id: number, updatePostDto: UpdatePostDto) { | ||
return this.prisma.post.update({ | ||
where: { | ||
id, | ||
}, | ||
data: updatePostDto, | ||
}); | ||
} | ||
|
||
async remove(id: number) { | ||
return this.prisma.post.delete({ | ||
where: { | ||
id, | ||
}, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prisma update and delete methods throw an exception if no record is found, we should catch so no 500 error is thrown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for reviewing it in stages, I haven't started the app until now 🫠
but these bugs could've easily been found with some basic manual testing xd
try { | ||
return this.prisma.post.findUniqueOrThrow({ | ||
where: { | ||
id, | ||
}, | ||
include: { | ||
author: true, | ||
}, | ||
}); | ||
} catch (e) { | ||
if (e instanceof Prisma.PrismaClientKnownRequestError) { | ||
if (e.code === 'P2025') { | ||
throw new NotFoundException('This post does not exist.'); | ||
} | ||
throw new InternalServerErrorException('An error occurred.'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classic mistake: trying to catch an error in an async call without awaiting it
try { | ||
return this.prisma.post.delete({ | ||
where: { | ||
id, | ||
}, | ||
}); | ||
} catch (e) { | ||
if (e instanceof Prisma.PrismaClientKnownRequestError) { | ||
switch (e.code) { | ||
case 'P2000': | ||
throw new BadRequestException('The content is too long.'); | ||
case 'P2025': | ||
throw new NotFoundException('This post does not exist.'); | ||
} | ||
} | ||
throw new InternalServerErrorException('An error occurred.'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry az előző review elég passzív agresszív volt xd
Closes #2