nsLittle's Node Eval#169
Conversation
…ers', and '/users/:name' plus 4 passing tests
| description: Brand names of sunglasses | ||
| get: | ||
| summary: GET brand names | ||
| description: GET brand names of sunglasses |
There was a problem hiding this comment.
missing a tag key to group apis together under a same category: brands, products, login/user, etc
| name: Mutsumi Hata | ||
| url: https://github.com/nsLittle | ||
| paths: | ||
| /: |
There was a problem hiding this comment.
this is not really an api, the home page is a FE concept and not a BE one.
| summary: GET brand names | ||
| description: GET brand names of sunglasses | ||
| operationId: get.brands | ||
| responses: |
There was a problem hiding this comment.
missing a response schema reference, the whole point of swagger is to see what API needs and returns + description, etc. The real value is in the schemas!
| - name: name | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: string |
| content: | ||
| application/json: | ||
| examples: | ||
| Oakley: |
There was a problem hiding this comment.
LOVE it! this should have been in every api, but why only in this one?
| }; | ||
|
|
||
| const newItem= { | ||
| product: req.body.product || '', |
| const userCart = user.cart; | ||
| res.status(200).json(userCart); | ||
| } else { | ||
| res.status(401).send('Unauthorized'); |
There was a problem hiding this comment.
this is not accurate, if you didnt find the user, means that the user was not found (400), not that the request is unauthorized
| const authenticate = (req, res, next) => { | ||
| const { username, password } = req.body; | ||
|
|
||
| if (username || password) { | ||
| return res.status(400).send(`Who are you?`); | ||
| }; | ||
| next(); | ||
| }; |
| const authenticate = (req, res, next) => { | ||
| const { username, password } = req.body; | ||
|
|
||
| if (username || password) { |
There was a problem hiding this comment.
if there is a username or a password, then the request is invalid?
| }); | ||
|
|
||
| // Authentication middleware | ||
| const authenticate = (req, res, next) => { |
There was a problem hiding this comment.
this is not really authenticating.
You need to basically accessToken that was returned in the /login api.
The login api needs to check the sent user & password and then generate that token that is checked on each api
|
|
||
| // BASIC MIDDLEWARE | ||
| app.use((req, res, next) => { | ||
| console.log('Basic Middleware Stuff...'); |
There was a problem hiding this comment.
need this console log? use comment instead.
| console.log('AUTHENTICATION'); | ||
| console.log('AuthHeader: ', authHeader); |
|
|
||
| if (authHeader) { | ||
| const token = authHeader.split(' ')[1]; | ||
| console.log('AuthHeader Deconstructed: ', token); |
| }; | ||
|
|
||
| // AUTHENTICATED ROUTES | ||
| app.get('/users', authenticateJWT, (req, res) => { |
| res.status(200).json({ users: userNames }); | ||
| }); | ||
|
|
||
| app.get('/:name', authenticateJWT, (req, res) => { |
There was a problem hiding this comment.
You defined well the cart routes in the swagger file but this is not matching your server.js file. Here the url is not containing the resource (cart). Make sure to always include it!
| const userName = req.params.name.toLowerCase(); | ||
| const user = users.find(user => user.name.first.toLowerCase() === userName); |
There was a problem hiding this comment.
you are repeating this validation in almost every api, maybe can be part of the authenticationToken?
| expect(res).to.have.status(200); | ||
| expect(res.body).to.be.an('object'); | ||
| expect(res.body).to.have.property('Burberry'); | ||
|
|
||
| const products = res.body['Burberry']; | ||
|
|
||
| expect(products).to.be.an('array'); | ||
|
|
||
| products.forEach(product => { | ||
| expect(product).to.have.property('id'); | ||
| expect(product).to.have.property('categoryId'); | ||
| expect(product).to.have.property('name'); | ||
| expect(product).to.have.property('description'); | ||
| expect(product).to.have.property('price'); | ||
| expect(product).to.have.property('imageUrls'); | ||
| }) |
There was a problem hiding this comment.
when this test fails, it will be VERY hard to find why or what happened
| .end((err, res) => { | ||
| if (err) return done(err); | ||
| expect(res).to.have.status(200); | ||
| expect(res.body).to.be.an('object'); |
There was a problem hiding this comment.
this is not really valuable for testing.
Becomes redundant for the next steps you do in the test
| expect(productDetails).to.be.an('object'); | ||
| expect(productDetails).to.have.property('name'); | ||
| expect(productDetails).to.have.property('description'); | ||
| expect(productDetails).to.have.property('price'); | ||
| expect(productDetails).to.have.property('imageUrls'); |
There was a problem hiding this comment.
instead of many expect, do one comparing productDetails to an mock object
| expect(userCart).to.have.property('items').that.is.an('array'); | ||
| expect(userCart.items[0]).to.have.property('product'); | ||
| expect(userCart.items[1]).to.have.property('quantity'); | ||
| expect(userCart).to.have.property('total').that.is.a('number'); |
There was a problem hiding this comment.
why not testing the actual number so you test the logic you did to update the total?
| expect(userCart).to.have.property('total').that.is.a('number'); | |
| expect(userCart).to.have.property('total').that.equals(sumNumber); |
2 participants
post /{name} is still wonky