-
Notifications
You must be signed in to change notification settings - Fork 30
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
Zhifan created registration api endpoints #1200
base: development
Are you sure you want to change the base?
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.
I have tested the API endpoints, and they work as expected. However, I recommend implementing trimming for the parameters received from the request body to prevent issues caused by unintended whitespace in the input.
PR.1200.mp4
const { eventId } = req.body; | ||
const userId = req.body.requestor.requestorId; |
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.
const { eventId } = req.body; | |
const userId = req.body.requestor.requestorId; | |
const { eventId } = req.body.eventId?.trim(); | |
const userId = req.body.requestor?.requestorId?.trim(); |
Recommend trimming eventId and requestorId using .trim() to ensure no unintended leading or trailing spaces.
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.
Thank you for the review and suggestion. However, in this case, trimming might not be necessary since both eventId/registrationId and userId are MongoDB ObjectIds that come from our system (database and authentication) rather than direct user input. The existing MongoDB ObjectId validation mongoose.Types.ObjectId.isValid()
will catch any malformed IDs. If we do need to handle string cleaning in the future, I think it would be better handled at the input validation layer or form processing level prior to passing into the controller.
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.
The API endpoints works as exptected. The code is very clear and easy to read.
There may be a minor issue with the error message, which I've mentioned in the code details.
Screen.Recording.2025-01-29.at.11.31.40.AM.mov
|
||
if (!mongoose.Types.ObjectId.isValid(registrationId)) { | ||
return res.status(400).send({ error: 'Invalid registration ID format' }); | ||
} |
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 registrationId
required here? If so, perhaps we should add a condition to check if it is empty first. Currently, if I don't provide a registrationId
when making the cancel request, the error message returned is 'Invalid registration ID format.'
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.
yes it should be required and theres a chain of methods in the router for validation. I looked into it and add a middleware to enforced the validation. Please review it again.
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.
Now I can get the right error message. Everything's good on my side. Great job!
The API endpoints works as exptected. The test went as it was explained and done in the video. 20250130-1706-20.3932015.mp4 |
Description
Related PRS (if any):
This backend PR is not related to frontend PR.
…
Main changes explained:
…
How to test:
npm run build
andnpm start
to run this PR locallyhttp://localhost:4500/api/register/create
andhttp://localhost:4500/api/register/cancel
with the following format to verifyScreenshots or videos of changes:
summary.mp4
Note:
Include the information the reviewers need to know.