init commit #18
init commit #18
Conversation
|
Awesome! 🎉 I don't see any new cypress tests for this work here https://dashboard.cypress.io/projects/34dwxm/runs/6/specs. I'd like to make sure that we are testing the code as we are developing it to reduce possible future regressions. Let us know if you need any help with writing tests! |
There was a problem hiding this comment.
Looks good overall!
The next step is to update the response objects to follow the descriptions in the respective issues (which I linked to specific endpoints).
I did the delete one as an example :)
Also added a few nits (small change suggestions)
Let me know if you have any questions, great job so far 👍 👍 👍
server/server.js
Outdated
| res.sendStatus(200); | ||
| app.delete('/organization_id', function (req, res) { | ||
| var id = req.query.id; | ||
| let org = organizations[id] |
There was a problem hiding this comment.
Out of curiosity, why do a let org here, did you mean to send it in the response?
server/server.js
Outdated
|
|
||
| app.listen(port, () => console.log(`Example app listening on port ${ port }!`)) |
There was a problem hiding this comment.
Nit: Missing enter at the end of the file
server/server.js
Outdated
| 0: { 'Name': 'Nandu Business', 'Net Worth': '6 Billion', 'Business Type': 'Real Estate Dealer' }, | ||
| 1: { 'Name': 'Sailesh Business', 'Net Worth': '20 Billion', 'Business Type': 'Food Dealer' } | ||
| } | ||
| const event = { |
There was a problem hiding this comment.
Nit: To follow the format of organizations being plural, event here should also be plural
server/server.js
Outdated
|
|
||
| app.get('/event_details', function(req, res){ | ||
| var id = req.query.id; | ||
| let lit = event[id] |
There was a problem hiding this comment.
Nit: lit is probably not the best name 😂
If you rename event to events, then lit here can become event.
| let lit = event[id] | |
| let event = events[id] |
server/server.js
Outdated
| var id = req.query.id; | ||
| let org = organizations[id] | ||
| delete organizations[id] | ||
| res.sendStatus(200) |
There was a problem hiding this comment.
Issue: #11
Here's what you'd need to change to make it compliant to the issue:
| res.sendStatus(200) | |
| if (org) { | |
| res.status(200) | |
| res.send({ | |
| "success": 1, | |
| "timestamp": new Date().getTime() | |
| }) | |
| } else { | |
| res.status(500) | |
| res.send({ | |
| "success": 0, | |
| "timestamp": new Date().getTime() | |
| "error": "Organization with id " + id + " does not exist!" | |
| }) | |
| } |
I just wrote this in a comment and haven't tested this out, so there might be some bugs here :P
server/server.js
Outdated
| app.post('/organization_id', function(req, res) { | ||
| var id = req.query.id; | ||
| organizations[id] = req.query; | ||
| res.sendStatus(200); |
server/server.js
Outdated
| } | ||
| organizations[id] = org; | ||
|
|
||
| res.sendStatus(200); |
server/server.js
Outdated
| app.get('/organizations_id', function (req, res) { | ||
| var id = req.query.id; | ||
| let org = organizations[id] | ||
| res.send(org) |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
Great job adding tests, let's merge this in now and we can continue iterating in the future |
No description provided.