Project 3 - Ecommerce App (Marina and Ian)#55
Project 3 - Ecommerce App (Marina and Ian)#55ianthehamster wants to merge 71 commits intorocketacademy:mainfrom
Conversation
finish up backend setup
Create and seed users and addresses tables
… cart in frontend
add Users router and controller + migration to edit addresses table
add logic to fetch all orders of the current user
add fetching order address in getOne req
Fix product qty in orders, update stock upon submitting an order
implement transaction in post order request
remove logs
add deleting a user logic
clean up BE a bit
delete unwanted files
| @@ -1 +1,3 @@ | |||
| node_modules/ No newline at end of file | |||
| ).routes(); | ||
| const ordersRouter = new OrdersRouter(ordersController).routes(); | ||
| const categoriesRouter = new CategoriesRouter(categoriesController).routes(); | ||
| const usersRouter = new UsersRouter(usersController).routes(); |
There was a problem hiding this comment.
so you use checkJwt for the Products router, but not for the users or orders router? That seems a bit of an odd choice to me from a security perspective!
| }); | ||
| } | ||
| } | ||
| Order.init( |
There was a problem hiding this comment.
what is your primary key? Try to always ensure that the model is a exact representation of your table in the DB
| }); | ||
|
|
||
| // Define association with categories here | ||
| this.belongsTo(models.category); |
There was a problem hiding this comment.
products belong to categories? that seems wrong to me
|
|
||
| async getAllAddresses(req, res) { | ||
| try { | ||
| const address = await this.model.sequelize.query( |
| const productToBeBought = await this.model.findByPk(productId); | ||
|
|
||
| const { title, price, description } = productToBeBought.dataValues; | ||
|
|
||
| const purchasedProduct = { | ||
| title: title, | ||
| price: price, | ||
| description: description, | ||
| stock_purchased: stock_purchased, | ||
| }; | ||
|
|
||
| const stock_left = productToBeBought.dataValues.stock_left; | ||
|
|
||
| await productToBeBought.update({ | ||
| stock_left: stock_left - stock_purchased, | ||
| }); |
There was a problem hiding this comment.
What if you couldn't find a product? Then there is a lot of room for bugs here. Validating your requests params and values is key, as much as validating that your DB queries yield a result
| const { title, price, description, stock_left, img, categoryId } = req.body; | ||
|
|
||
| try { | ||
| const newProduct = await this.model.create({ | ||
| title: title, | ||
| price: price, | ||
| description: description, | ||
| stock_left: stock_left, | ||
| img: img, | ||
| categoryId: categoryId, | ||
| }); |
There was a problem hiding this comment.
Here you blindly trust the client. Any request values that come in you just insert into the DB. What if certain values are undefined, what if some values are JS scripts or SQL? All this opens up your app to vulnerabilities
| Messages: [ | ||
| { | ||
| From: { | ||
| Email: 'ianchow9898@gmail.com', | ||
| Name: 'Techie E-Store', | ||
| }, | ||
| To: [ | ||
| { | ||
| Email: userEmail, | ||
| Name: userFirstName, |
There was a problem hiding this comment.
Whenever your controllers get too big, and we possibly use external providers to facilitate some functionality, it might make sense to change your architecture and folder structure. Here we could use a stripe service for example and an email service. This would abstract away all this code from the controller. You could google for three-tier architecture, hexagonal architecture, domain-driven design and other topics. If we update our controller to accept and respond to requests, and use services to handle business logic, our code gets easier to read and write as well.
| } | ||
|
|
||
| async getUserBasedOnEmail(req, res) { | ||
| const { email } = req.body; |
There was a problem hiding this comment.
now this is really bad. I can just use any hacked email list, and retrieve any user registered on your website. That means I get everyone's first name, last name and phone number. Scams incoming $$$$$$$
There was a problem hiding this comment.
What you should do:
- Protect the route behind your authentication service
- Make sure via the token, that only the user that the token belongs to has access to retrieve the user as well
- Do not return sensitive information on the endpoint if possible
| } | ||
| } | ||
|
|
||
| // Not working |
| '/stripe', | ||
| this.controller.createStripeProduct.bind(this.controller), | ||
| ); | ||
| router.post( | ||
| '/stripeCreate', | ||
| this.controller.seedStripeProducts.bind(this.controller), | ||
| ); | ||
|
|
||
| router.post( | ||
| '/remainingStripeProducts', | ||
| this.controller.seedRemainingStripeProducts.bind(this.controller), | ||
| ); | ||
|
|
||
| router.post( | ||
| '/create-checkout-session-external', | ||
| this.controller.makePaymentMultiple.bind(this.controller), | ||
| ); | ||
|
|
||
| router.post( | ||
| '/get-stripe-prices', | ||
| this.controller.getAllPricesFromStripe.bind(this.controller), | ||
| ); | ||
|
|
||
| router.post( | ||
| '/send-email-success', | ||
| this.controller.sendMailToCustomer.bind(this.controller), |
There was a problem hiding this comment.
I don't agree with these routes. Those could fit better towards the REST pattern.
| router.get('/', this.controller.getAllAddresses.bind(this.controller)); | ||
| router.post('/', this.controller.postNewAddress.bind(this.controller)); | ||
| router.get( | ||
| '/get-address-id', |
There was a problem hiding this comment.
it is a get route already, we don't need to define that in the route string itself
|
|
||
| try { | ||
| const categoryToBeUpdated = await this.model.findByPk(categoryId); | ||
|
|
There was a problem hiding this comment.
basically can check here if category exists. If no, respond to the client
3 participants
No description provided.