-
Notifications
You must be signed in to change notification settings - Fork 0
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
added business requests #17
Conversation
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.
Overall, really solid PR, this was a large task so great job handling that. Just some minor changes to be made, good work fellas, Christmas bonus incoming!!!
.pnp.cjs
Outdated
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.
We should not be adding these files to the commits
.pnp.loader.mjs
Outdated
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.
Same with this file, we don't want to add this
.yarn/install-state.gz
Outdated
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.
Same with this file
routes/businessRouter.js
Outdated
@@ -34,4 +34,239 @@ businessRouter.get('/:id', async (req, res) => { | |||
} | |||
}); | |||
|
|||
// POST add a new business | |||
businessRouter.post('/', async (req, res) => { | |||
console.log(req.body); |
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.
Nit: We shouldn't have console logs in our final commits
routes/businessRouter.js
Outdated
createdDate, | ||
city, | ||
} = req.body; | ||
console.log(req.body); |
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.
Console log same as before
routes/businessRouter.js
Outdated
); | ||
res.status(200).send(newBusiness); | ||
} catch (err) { | ||
res.status(400).send(err.message); |
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.
This should send a 500 error not a 400 error
routes/businessRouter.js
Outdated
// Sending a response | ||
response.status(200).send('Update successful'); | ||
} catch (err) { | ||
response.status(400).send(err.message); |
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.
This should be a 500 error
routes/businessRouter.js
Outdated
|
||
const newBusiness = await db.query( | ||
`INSERT INTO business ( | ||
id, type, name, street, zipCode, state, qb_vendor_name, qb_city_state_zip, |
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.
Remove the id, we figured out how to auto increment it.
routes/businessRouter.js
Outdated
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, | ||
$17, $18, $19, $20, $21, $22, $23, $24, $25, $26, $27, $28, $29, $30, | ||
$31, $32, $33, $34, $35, $36, $37, $38, $39, $40, $41, $42, $43, $44, | ||
$45, $46, $47, $48, $49, $50, $51, $52, $53, $54, $55) |
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.
Just remember to remove $55 when you are removing the id
No description provided.