#163415664 validate user input #36
Conversation
Veraclins
left a comment
Veraclins
left a comment
There was a problem hiding this comment.
Great job @dbytecoderc, I particularly love how to try to abstract some repetitive operations into methods, that is cool. I left a few comments for you. Go through them and make necessary adjustments. Cheers!
| // app.use('api/v1/comments', commentRoutes); | ||
| app.use('/api/v1', routes); | ||
|
|
||
| app.use((req, res, next) => { |
There was a problem hiding this comment.
Make this the catch-all route. You can use a wild-card. This does not map to any route at the moment.
There was a problem hiding this comment.
I fixed this already you would see it in the next PR, i created an index folder in the route file and routed all the endpoints to that file and exported to the app file from there
| // }); | ||
| async createComment(req, res) { | ||
| const comment = req.body; | ||
| const checkquestion = await QuestionModel.getcommentById(comment.commentId); |
There was a problem hiding this comment.
If you are just creating a comment, (from the method name) how can the comment already exist in the DB? Also while will you call it checkquestion when you are getting a comment?
There was a problem hiding this comment.
Actually i was just brain storming when i did that, that piece of code is not yet functional, however at the time of writing this reply i have already fixed that,
| // id, | ||
| // } = req.params; | ||
| // const meetup = await MeetupModels.retrieveSingleMeetup(id); | ||
| // }, |
There was a problem hiding this comment.
If this code is no longer being used, remove it.
| topic VARCHAR(255) NOT NULL, | ||
| location TEXT NOT NULL, | ||
| date VARCHAR(50) NOT NULL, | ||
| happeningon VARCHAR(50) NOT NULL, |
There was a problem hiding this comment.
Either use camelCase or snake_case for variable name. Essentially snake_case only for db fields and camelCase everywhere else
| import Comments from '../controller/comments'; | ||
|
|
||
| // router.post('/auth/signup', Admin.createnewAdmin); | ||
| router.post('/auth/signup', Comments.createComment); |
There was a problem hiding this comment.
Do you think it is appropriate to map signup route to createComment?
There was a problem hiding this comment.
I have actually cleaned this up at the moment of posting this comment
| router.use('/auth', userRoutes); | ||
| router.use('/meetups', meetupRoutes); | ||
| router.use('/questions', questionsRoute); | ||
|
|
There was a problem hiding this comment.
How is the comment route used when it is not mapped here?
There was a problem hiding this comment.
The comment route is not actually functional here i was just doing some code scribbling i was supposed to remove the code for the comments
| router.post('/', Auth.verifyToken, Auth.adminAuth, meetupValidation.validCreateMeetup, meetupValidation.checkDate, meetupController.createMeetup); | ||
| router.get('/upcoming/', Auth.verifyToken, meetupController.getUpcomingMeetups); | ||
| router.get('/:meetupId', Auth.verifyToken, validate(validations.getMeetup), meetupController.getUpcomingMeetups); | ||
| router.get('/:id', Auth.verifyToken, meetupController.getSingleMeetup); |
There was a problem hiding this comment.
Since Auth.verifyToken is applied to all the routes here, why not move it to where this route file is mapped in the index.js file to avoid repetition?
| /* eslint-disable eol-last */ | ||
| const convertName = name => (name.charAt(0).toUpperCase() + name.slice(1)).trim(); | ||
|
|
||
| export default convertName; No newline at end of file |
There was a problem hiding this comment.
This file name and method name should correlate. Make up on your mind what this does and name them accordingly
What does this PR do?
Description of Task to be completed?
How should this be manually tested?
What are the relevant pivotal tracker stories?