-
Notifications
You must be signed in to change notification settings - Fork 8
Feature/152 accept hacker route #611
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
Conversation
67ef3bf to
e863783
Compare
middlewares/hacker.middleware.js
Outdated
| */ | ||
| async function acceptHacker(req, res, next) { | ||
| req.body.status = Constants.General.HACKER_STATUS_ACCEPTED; | ||
| updateHacker(req, res, next); |
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.
Since this is a middleware, just put it under the route, remove this line and call next()
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.
To add onto Harsh's comment:
function parseAccept(req, res, next) {
req.body.status = Constants.General.HACKER_STATUS_ACCEPTED;
next();
}and then in routes/hacker.js:
...
Middleware.Hacker.parseAccept,
Middleware.Hacker.updateHacker,
...
middlewares/hacker.middleware.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Sets Hacker Status to Accepted and runs it through updateHacker's functionality. |
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.
Nit: Rename function to parseAccept, similar to parseConfirmation and make comment just say it sets the req.body.status to accepted
printharsh
left a comment
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 good! Just very minor nits, could you also add some tests (under hacker.test.js PATCH update one hacker tests should be very similar to the ones you'd write).
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, added to Harsh's comment.
middlewares/hacker.middleware.js
Outdated
| */ | ||
| async function acceptHacker(req, res, next) { | ||
| req.body.status = Constants.General.HACKER_STATUS_ACCEPTED; | ||
| updateHacker(req, res, next); |
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.
To add onto Harsh's comment:
function parseAccept(req, res, next) {
req.body.status = Constants.General.HACKER_STATUS_ACCEPTED;
next();
}and then in routes/hacker.js:
...
Middleware.Hacker.parseAccept,
Middleware.Hacker.updateHacker,
...
middlewares/hacker.middleware.js
Outdated
| * @param {*} res | ||
| * @param {*} next | ||
| */ | ||
| async function acceptHacker(req, res, next) { |
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.
This function should not be async.
pierreTklein
left a comment
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.
Please include some additional tests for this route:
- Failure on someone who is not authenticated
- Failure on someone who is not authorized
- Failure on invalid input
- Success for accepting a hacker.
421c6d3 to
21b999a
Compare
loreina
left a comment
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 we revert changes that happened in docs folder aside from your changes
788c7b7 to
e36f3a9
Compare
e36f3a9 to
d4264c5
Compare
loreina
left a comment
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.
one versioning change, everything else looks great !!!
Co-Authored-By: Loreina Chew <loreina.chew@gmail.com>
Tickets:
List of changes:
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Failure on someone who is not authenticated
Failure on someone who is not authorized
Failure on invalid input
Success for accepting a hacker.
On hacker.test.js
POSTMAN Patch request to hacker/accept/:id
Test Configuration:
Firmware version:
Hardware:
Toolchain:
SDK:
Questions for code reviewers?
Checklist: