-
Notifications
You must be signed in to change notification settings - Fork 2
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 behaviour endpoints #13
Conversation
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.
can you adding some ss to show your testing on postman
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.
awesome work! seems right to me but let's add some ss so everyone reviewing can see it works. also did you make sure to test edge cases, i.e trying to delete something that doesn't exist, adding a behaviour name that's not valid
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.
Looks really solid, just a couple small comments for design choices
} | ||
|
||
export interface BehaviourResponseDTO { | ||
id: string; |
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.
should this be a number? I know that for the UserDTO the id is a string, but I think that might be because it's a firebase user ID. Not 100% sure about that though.
But I think if there's no use case difference of the DTO, between having it as a string versus a number, then we should keep it consistent with the DB and have it as a number.
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.
agreed
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.
yeah me as well
try { | ||
behaviour = await PgBehaviour.findByPk(id, { raw: true }); | ||
if (!behaviour) { | ||
throw new Error(`Behaviour id ${id} not found`); |
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.
Do we want this to be a 404 NOT FOUND response? We could do this by making custom error types and handling them seperately in the behaviourRoutes.ts file. (Something like NotFoundError would work)
LOL sorry ik I'm nitpicking, but you're the first one that made endpoints so I wanna make sure some things are clear for others as well.
I also added the NotFoundError to the errorUtils file I found, but let me know if there's a better way to handle it. |
if (e instanceof NotFoundError) { | ||
res.status(404).send(getErrorMessage(e)); | ||
} else { | ||
res.status(500).send(getErrorMessage(e)); |
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.
for 500 i dont think we want to send the error message back. 500 is internal error so ideally we want to log the error internally, confirm @jerry-cheng5
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.
Yea I agree, I think that's the proper way to handle 500 errors.
(Normally its for security and shi and we don't reaaally have to worry about that, but its always good to practice the proper response conventions)
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.
So maybe just send some generic message back like:
An unexpected internal server error occurred. Please try again later.
or
An unexpected error occurred on our end. Please try again later.
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.
lgtm, wait for @laks0407 @jerry-cheng5 to take a final look
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.
Great work! Really impressed by how fast you did all this :)
Everything looks good to me!
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.
Looks great to me as well! Nice work :)
Created endpoints to modify/access the behaviour table.
Enabled getting all, getting one, creating, updating, and deleting entries.
Manually tested operations using Postman.