Open tanerochris opened 4 years ago
Hey @tanerochris Please could you attach screenshot for test on different endpoints?
Hello you can clone the branch and run the test. yarn test or npm test
On Fri, May 15, 2020, 19:56 Teneng Clifford notifications@github.com wrote:
Hey @tanerochris https://github.com/tanerochris Please could you attach screenshot for test on different endpoints?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BuildForSDG/voxnostra/pull/20#issuecomment-629424222, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACC3JXKVWUTHWCWLQDN34ELRRWF7LANCNFSM4NBFPPPQ .
@KaiserPhemi I will love to mention that we are using NextJS for the project ,it is an opinionated framework so there are some aspects of the file structure and naming convention that is dictated by the framework. Equally handling API requests.
On Thu, May 21, 2020, 03:46 Oluwafemi Akinwa notifications@github.com wrote:
@KaiserPhemi requested changes on this pull request. Critical
- File Naming: Going forward let's adopt camelCase for naming files as opposed to snake-case that's been used.
- It appears responses aren't sent for error messages thrown.
- I noticed we're using res.json() & having to set the header. res.send() automatically sets the header so it's preferable.
- I might need to schedule a sync with all today(21st May) on this .
In pages/api/user/sign-out.js https://github.com/BuildForSDG/voxnostra/pull/20#discussion_r428397670:
@@ -0,0 +1,19 @@ +import Middleware from '../../../middlewares'; +/**
- Sets the session value to null.
- *
- DELETE endpoint
- @param req
- @param res
- @returns {Promise
} - */ +async function SignOutHandler({ req, res }) {
- if (req.method === 'DELETE') {
Nitpick
If this route is meant to specifically handle sign-out (cos the frontend should specifically send delete request in the first place) why check the http method in the first place? Besides I believe all checking/validation should be handle by a middleware.
In pages/api/user/sign-up.js https://github.com/BuildForSDG/voxnostra/pull/20#discussion_r428398945:
+import { setUserSession } from '../../../helpers/auth-helpers'; +import Middleware from '../../../middlewares'; +import User from '../../../models/user/user.model'; + +/**
- POST only
- Sign up a user, body must contain these two fields
- *
- @param req
- @param req.body.email
- @param req.body.password
- @param res
- @returns {Promise
} - */ +async function SignUpHandler({ req, res }) {
- if (req.method === 'POST') {
Nitpick
Same thing as the comment I left above.
In pages/api/projects/index.js https://github.com/BuildForSDG/voxnostra/pull/20#discussion_r428408301:
@@ -0,0 +1,40 @@ +import mongoose from 'mongoose'; +import Middleware from '../../../middlewares'; + +const Project = mongoose.model('Project'); +/**
- @async
- @desc Api handles project list and search request
- @param {{ req: NextApiRequest, res: NextApiResponse }} prop
- */ +const IndexProjectHandler = async ({ req, res }) => {
- if (req.method === 'GET') {
- const limit = req.query.limit || 0;
Nitpick
A better approach would be to abstract these into a helper file or a middleware
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BuildForSDG/voxnostra/pull/20#pullrequestreview-415822168, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACC3JXNQTJJ5TXQGOWR4VLLRSSIY5ANCNFSM4NBFPPPQ .
Description
Developing the API backend for creating, updating and viewing of projects on voxnostra, ### How Has This Been Tested? Tests have been written for the different endpoints
Any background context you want to provide?
### Relevant Task link (from the project board) Card 37652856
Questions