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 /isUsernameAvailable/:username route in users main route #140
Added /isUsernameAvailable/:username route in users main route #140
Conversation
controllers/usersController.js
Outdated
if (!result.userExists) { | ||
return res.json({ | ||
message: 'User name Available', | ||
userExists: result.userExists |
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 need to return the data of the user with that username too? 🤔
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.
we are not returning data of user.it will return boolean false. Shall I remove 'userExists' Property and return only message in case of success?
utils/swaggerDefinition.js
Outdated
@@ -248,6 +248,14 @@ const swaggerOptions = { | |||
} | |||
} | |||
}, | |||
userAvailable: { | |||
type: 'object', | |||
properties: { |
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.
Conflicts with the actual response sent by the route.
Is there any contract written for this API?
What is the expected response?
controllers/usersController.js
Outdated
userExists: result.userExists | ||
}) | ||
} | ||
return res.boom.notFound('User name not Available') |
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.
Any update on this? As per discussed in the meeting, we were to send a 200 with userExists as false.
controllers/usersController.js
Outdated
userExists: result.userExists | ||
}) | ||
} | ||
return res.boom.notFound('User name not Available') |
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.
Any update on this? As per discussed in the meeting, we were to send a 200 with userExists as false.
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.
Requesting some changes. Please check
routes/users.js
Outdated
* schema: | ||
* $ref: '#/components/schemas/errors/badImplementation' | ||
*/ | ||
router.get('/isUsernameAvailable/:username', authenticate, usersController.checkUser) |
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.
Needs to be before /:username
, right? 🤔
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.
done
controllers/usersController.js
Outdated
try { | ||
const result = await userQuery.fetchUser({ username: req.params.username }) | ||
return res.json({ | ||
userAvailable: !result.userExists |
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 be isUsernameAvailable
as per API contracts
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.
done
const result = await userQuery.fetchUser({ username: req.params.username }) | ||
return res.json({ | ||
userAvailable: !result.userExists | ||
}) |
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 case when the user is not logged in seems to be missing 🤔
controllers/usersController.js
Outdated
@@ -48,6 +48,25 @@ const getUser = async (req, res) => { | |||
} | |||
} | |||
|
|||
/** | |||
* Checks whether a user exists or not |
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 be something like checks whether a given username is available
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.
done
controllers/usersController.js
Outdated
* @param res {Object} - Express response object | ||
*/ | ||
|
||
const checkUser = async (req, res) => { |
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 function name should also be changed accordingly
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.
done
docs/swaggerDefinition.js
Outdated
userAvailable: { | ||
type: 'object', | ||
properties: { | ||
userAvailable: { |
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.
re: isUsernameAvailable
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.
done. Changed the property name only to isUsernameAvailable.
}) | ||
}) | ||
|
||
it('Should return userAvailable as false as we are passing existing user', function (done) { |
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: username available
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 to me! 👌
Thank you for your patience with me 😂
Approving from my side
Thanks for all the help swaraj |
* schema: | ||
* $ref: '#/components/schemas/errors/badImplementation' | ||
*/ | ||
router.get('/isUsernameAvailable/:username', authenticate, usersController.getUsernameAvailabilty) |
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.
API schema yet to be added to the schema file for the new route.
https://github.com/Real-Dev-Squad/website-backend/blob/develop/README.md#api-documentation
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.
Updated the schema file using npm run generate-api-schema. Please let me know if any further changes needs to be done
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 route to check user availability. Added swagger definition and integration test for it.