Merge feature-endpoints into feature-roles - #56
Conversation
stephan418
left a comment
stephan418
left a comment
There was a problem hiding this comment.
Please review the comments and update the code accordingly
| minTeamSize: z.number(), | ||
| briefDescription: z.string(), | ||
| fullDescription: z.string(), | ||
| eventPid: z.string(), |
There was a problem hiding this comment.
Should not be needed (as the eventPid is in the route, like: /events/:eventPid/disciplines)
There was a problem hiding this comment.
Also in general the code from feature-roles should be used (as this code is more secure). Just leave the parts in that are new here
| maxTeamSize: DataType.NUMBER, | ||
| briefDescription: DataType.STRING, | ||
| ["fullDescription?"]: DataType.STRING, | ||
| }) |
There was a problem hiding this comment.
Please pass the zod error (result.error) as the second parameter to generateInvalidBodyError
| }, | ||
| }); | ||
|
|
||
| if (!discipline) { |
There was a problem hiding this comment.
This has no effect (discipline will always be defined) as prisma.[].update only throws an error if the item to update was not found (PrismaClientKnownRequestError with code P2025)
| }, | ||
| }); | ||
| } | ||
| if (e instanceof Prisma.PrismaClientUnknownRequestError) { |
There was a problem hiding this comment.
Is already handled by the server by default, so this is useless code
| }); | ||
| } catch (e) { | ||
| if (e instanceof Prisma.PrismaClientKnownRequestError) { | ||
| return res.status(500).json({ |
There was a problem hiding this comment.
As said, the 404 error should be dispatched here (when the code is P2025)
| }, | ||
| }); | ||
|
|
||
| if (!updatedRec) { |
There was a problem hiding this comment.
Same as above here: Prisma always returns a model here and throws if the pid could not be found
|
|
||
| return res.status(200).json({ | ||
| type: "success", | ||
| payload: {}, |
There was a problem hiding this comment.
Should include some type of payload, was originally only removed because it contained sensitive data
| return res.status(204).end(); | ||
| } catch (e) { | ||
| if (e instanceof PrismaClientKnownRequestError && e.code === "P2025") { | ||
| console.log("not found error"); |
There was a problem hiding this comment.
Remove console.log()s and use logger.debug or logger.info for messages that should appear in debug or production respecively
| console.log("not found error"); | ||
| throw new NotFoundError(tableToUpdate[2], pid); | ||
| } | ||
| if (e instanceof Prisma.PrismaClientUnknownRequestError) { |
There was a problem hiding this comment.
Already handled by the default error handler, as above
| switch(tableToUpdate) { | ||
| case "events": return prisma.event.update; | ||
| case "disciplines": return prisma.discipline.update; | ||
| default: return prisma.roleSchema.update; |
There was a problem hiding this comment.
The default should probably throw some sort of exception and not just assume roleSchema. What if the user passes a string like test as the tableToUpdate?
There was a problem hiding this comment.
When the user passes garbage the router won't be called so a 404 will be returned.
There was a problem hiding this comment.
How would that work?
3 participants
Adds multiple features to prepare for release