-
Notifications
You must be signed in to change notification settings - Fork 7
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
#167750066 Users should be able to comment on Travel requests #27
base: develop
Are you sure you want to change the base?
Conversation
1fe9620
to
280db99
Compare
280db99
to
3d8ad23
Compare
3d8ad23
to
54942f5
Compare
54942f5
to
b3e00d9
Compare
b3e00d9
to
2980d9a
Compare
static async getCommentById(req, res) { | ||
const { commentId } = req.params; | ||
try { | ||
const getComment = await Comments.findByPk(commentId); |
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.
Hi @oyewoas. Nice work so far. But I'd like to suggest, can we take advantage of the model relationships to fetch more details? For example, here, can we also get the user details and also the request details related to the comment?
Please check using include
in Sequelize
queries.
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.
why do you think we need extra details apart from the comment @darasimiolaifa
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.
Say for instance, currently, what you're going to send to the frontend is the userId
of the person who made the comment. But the frontend needs more than that. It needs the firstName
, and lastname
of the person, because that is what will be displayed. So, rather than send the userId
, and then discover later that you still need to make another round trip to the database to fetch the user details, you can do it in one full query sweep by using include
.
src/test/index.test.js
Outdated
import './utils/jwt.test'; | ||
import './utils/emailTemplatesFunction.test'; | ||
import './services/autoMailer.test'; | ||
// import './controllers/users.test'; |
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.
You forgot to 'un-comment' these lines before pushing. Merge conflicts lurking.
2980d9a
to
a5f86fe
Compare
a5f86fe
to
e702c80
Compare
e702c80
to
d57ea72
Compare
d57ea72
to
3a2f327
Compare
3a2f327
to
02b397b
Compare
02b397b
to
6fccc41
Compare
- setup controller for user to comment on requests - setup route for user to comment on requests - setup unit test for user to comment on requests route [Delivers #167750066]
6fccc41
to
022760d
Compare
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.
Very good job @oyewoas
- if i update, I should be able to get a response of the update, i only got a message.
- I tried to do some security checks and I realized that anybody could get all comments/single comment, update or delete. It would be better it if its only the user that created the comment to be able to update/delete/get
What does this PR do?
This PR implements Travel request comments features
Description of tasks to be completed?
How should this be manually tested/checked?
git clone https://github.com/andela/wolfsbane-backend.git
cd wolfsbane-backend
git fetch origin ft-create-accommodation-facilities-167750066
git checkout origin/ft-create-accommodation-facilities-167750066
npm install
npm run pre-test
to run migrations and seed the databasenpm test
npm run migrate
andnpm run seed
to migrate and seed the databasenpm run start:dev
to start the development serverPOST
request tohttp://localhost:3000/api/v1/users/signin
using the following details as a JSON object:src/database/seeders/create-6-request.js
then copy any of theid
valuesPOST
request tohttp://localhost:3000/api/v1/requests/:requestId/comments
, replace:requestId
with theid
copied in 13 above:Sample request:
Sample response:
GET
request tohttp://localhost:3000/api/v1/requests/:requestId/comments
, replace:requestId
with theid
copied in 13 above:Sample response:
GET
request tohttp://localhost:3000/api/v1/comments/:commentId
, replace:commentId
with theid
of one of the returned comment in 15 above:Sample response:
PUT
request tohttp://localhost:3000/api/v1/comments/:commentId
, replace:commentId
with theid
of one of the returned comment in 15 above:Sample response:
DELETE
request tohttp://localhost:3000/api/v1/comments/:commentId
, replace:commentId
with theid
of one of the returned comment in 15 above:Sample response:
Any background context you want to provide?
Make sure you drop your database tables, then run migrations and seeds again, both for test and development environments
What are the relevant pivotal tracker stories?
#167750066