-
Notifications
You must be signed in to change notification settings - Fork 1
final project review suggestions #2
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,18 +33,21 @@ app.use( | |
| rateLimiter({ | ||
| windowsMs: 15 * 60 * 1000, | ||
| max: 60, | ||
| }) | ||
| }), | ||
| ); | ||
|
|
||
| app.use(helmet()); | ||
| app.use(cors({ origin: ["http://localhost:3000", "https://deployed_to_netlify_url.com"], credentials: true })); //CHAGE TO REAL URL ON NETLIFY | ||
| app.use(cors({ | ||
| origin: ['http://localhost:3000', 'https://deployed_to_netlify_url.com'], | ||
| credentials: true, | ||
| })); //CHAGE TO REAL URL ON NETLIFY <--have you done this? | ||
| app.use(xss()); | ||
| app.use(mongoSanitize()); | ||
|
|
||
| //app.use(morgan('tiny')); //log in terminal every request methode + statuscode | ||
| app.use(morgan('tiny')); //log in terminal every request methode + statuscode // Why not using morgan? | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that you have this commented out; I think it's helpful to have logging turned on even in your deployed app so that you can check the logs in Render.com if your backend has any issues that you need to debug
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante totally agree, uncommented it, thank you! |
||
| app.use(express.json()); | ||
| app.use(cookieParser(process.env.JWT_SECRET)); //access cookies coming back from the browser- with each request the browser will send cookie + we're signing cookies with jwt secret | ||
| app.use(express.static('./public')); | ||
| app.use(express.static('./public', { extensions: ['html'] })); // a way for /docs to work being served from public folder instead of needing its own dedicated route definition (also fix issue with js not working) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commented out lines 60-62. Instead of creating a separate route to serve the By default though, to access files in the static folder, you need to enter the full file name. Eg: to get the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will also help solve the issue where currently when you try to go to the These error messages were being shown because inside the ^ The browser then tries to load You could have also had a route handler for that (but I think it's simpler to just put both the html and the JS file into the public folder and serve them using the express.static middleware as explained in the previous comment):
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante so glad to learn how to solve these bugs. Is it a good practive to have /docs route for documetation or is it better just to leave in on '/' path?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way is totally fine! I like how for the root path
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante great! i love that i could learn something new thanks to creating this not really necessary /docs path lol |
||
| app.use(fileUpload()); | ||
|
|
||
| // //testing in postman if the token is stored in a cookie | ||
|
|
@@ -54,9 +57,9 @@ app.use(fileUpload()); | |
| // res.send('ecommerce api'); | ||
| // }); | ||
|
|
||
| app.get('/docs', (req, res) => { | ||
| res.sendFile(path.join(__dirname, 'docs.html')); | ||
| }); | ||
| // app.get('/docs', (req, res) => { | ||
| // res.sendFile(path.join(__dirname, 'docs.html')); | ||
| // }); // Could get this with just the public folder | ||
|
|
||
| app.use('/api/v1/auth', authRouter); | ||
| app.use('/api/v1/users', userRouter); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,17 +23,18 @@ const createOrder = async (req, res) => { | |
| //initial state | ||
| const initialValue = { subtotal: 0, orderItems: [] }; | ||
|
|
||
| // this reduce function is probably worth refactoring/extracting into its own function since its somewhat long. | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a suggestion, we could then do something like: And take all the insides from line 29 to line 64 and define it as a separate function:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante UPD already resolved below. Akos, what do you mean exactly when say // this could potentially also be extracted to a helper function
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante got it!!! thank you so much, this part was hard to understand at first but now I see it much more clear
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Yeah I just meant it could be made its own function, that returns the singleOrderItem object (maybe called: |
||
| // Loop through each item in `cartItems` array (if the array is empty, this will just be skipped) | ||
| const { subtotal: subtotal, orderItems: orderItems } = await cartItems.reduce( | ||
| const { subtotal, orderItems } = await cartItems.reduce( | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When doing object destructuring, we can simplify/shorten the code a bit by just using the variable name (only if the variable name that you want to store the value in, and the name of the object property are exactly the same, as in this case) |
||
| async (resultsMap, item) => { | ||
| // Each iteration, item will be the next item in the array. | ||
| // Each iteration, item will be the next item in the array. | ||
| //resultsMap will be whatever we return at the end of the reduce function, and the first time it will be equal to `initialValue` (because we pass that to reduce as the second argument on line 63) | ||
|
|
||
| const dbProduct = await Product.findOne({ _id: item.product }); | ||
| console.log( | ||
| `looping through: resultsMap=${JSON.stringify( | ||
| await resultsMap | ||
| )} | item=${JSON.stringify(item)} | dbProduct=${dbProduct}` | ||
| await resultsMap, | ||
| )} | item=${JSON.stringify(item)} | dbProduct=${dbProduct}`, | ||
| ); | ||
|
|
||
| if (!dbProduct) { | ||
|
|
@@ -42,6 +43,7 @@ const createOrder = async (req, res) => { | |
| ); | ||
| } | ||
|
|
||
| // this could potentially also be extracted to a helper function | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depends on how long you're finding this function and if you think this functionality is something that may end up getting reused somewhere else in the code; but this might be another place to extract code into a helper function: And then in this reducer function we'd just have: |
||
| const { name, price, image, _id } = dbProduct; | ||
| const singleOrderItem = { | ||
| amount: item.amount, | ||
|
|
@@ -96,7 +98,7 @@ const createOrder = async (req, res) => { | |
|
|
||
|
|
||
|
|
||
|
|
||
| //calculate total | ||
| const total = tax + shippingFee + subtotal; | ||
| //get client Secret | ||
|
|
@@ -116,7 +118,7 @@ const createOrder = async (req, res) => { | |
| }); | ||
| res | ||
| .status(StatusCodes.CREATED) | ||
| .json({ order, clientSecret: order.clientSecret }); | ||
| .json({ order, clientSecret: order.clientSecret }); // any particular reason to return clientSecret at top-level when it's already part of the order object? | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just curious if this I was also thinking it doesn't really seem like we need to put
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante no, there was no particular reason, i just followed the tutorial, i left: |
||
| }; | ||
|
|
||
| const getAllOrders = async (req, res) => { | ||
|
|
@@ -139,6 +141,7 @@ const getCurrentUserOrders = async (req, res) => { | |
| res.status(StatusCodes.OK).json({ orders, count: orders.length }); | ||
| }; | ||
|
|
||
| // it's a bit misleading to name this route updateOrder, since it seems like the only thing you can do is set an order to "paid" (by passing in an intent id) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This endpoint doesn't really let you update the order itself, apart from setting a paymentIntentId and making a request to this endpoint always sets the
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante updated the code, to be honest i didn't get this part with the fake payment, we are takingpaymentIntentId from req.body which is i guess "someRandomString" but instead of this "someRandomString" is should be smth else i will try to integrate stripe and create new PR if you don't mind. |
||
| const updateOrder = async (req, res) => { | ||
| const { id: orderId } = req.params; | ||
| const { paymentIntentId } = req.body; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,21 +79,28 @@ const updateReview = async (req, res) => { | |
| // res.status(StatusCodes.OK).json({ review }); | ||
|
|
||
| //OPTION 2 | ||
| const { id: reviewId } = req.params; //TAKE ID FROM REQ.PARAMS AND ASSIGN IT TO REVIEWiD | ||
| const { rating, title, comment } = req.body; | ||
| const review = await Review.findOne({ _id: reviewId }); | ||
| const { id: reviewId } = req.params; //TAKE ID FROM REQ.PARAMS AND ASSIGN IT TO REVIEWiD | ||
| const { rating, title, comment } = req.body; // don't need this | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Option 2 we just directly set the properties from
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante that's true!! removed it |
||
| const review = await Review.findOne({ _id: reviewId }); | ||
|
|
||
| if (!review) { | ||
| throw new CustomError.NotFoundError(`No review with id ${reviewId}`); | ||
| } | ||
|
|
||
| checkPermissions(req.user, review.user); | ||
| review.rating = req.body.hasOwnProperty('rating') ? req.body.rating : review.rating | ||
| review.title = req.body.hasOwnProperty('title') ? req.body.title : review.title | ||
| review.comment = req.body.hasOwnProperty('comment') ? req.body.comment : review.comment | ||
| if (!review) { | ||
| throw new CustomError.NotFoundError(`No review with id ${reviewId}`); | ||
| } | ||
|
|
||
| await review.save(); | ||
| res.status(StatusCodes.OK).json({ review }); | ||
| checkPermissions(req.user, review.user); | ||
| // can simplify with: review.rating = req.body?.rating || review.rating (but hasOwnProperty is better because then that allows us to send/unset values like this: {rating: null}) | ||
| review.rating = req.body.hasOwnProperty('rating') | ||
| ? req.body.rating | ||
| : review.rating; | ||
| review.title = req.body.hasOwnProperty('title') | ||
| ? req.body.title | ||
| : review.title; | ||
| review.comment = req.body.hasOwnProperty('comment') | ||
| ? req.body.comment | ||
| : review.comment; | ||
|
|
||
| await review.save(); | ||
| res.status(StatusCodes.OK).json({ review }); | ||
|
|
||
| //option 3 | ||
| //When we do the object destructuring (const { rating, title, comment } = req.body;), then any fields that didn't exist in req.body will just be saved to the variables on the left-hand-side as undefined. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ const updateUser = async (req, res) => { | |
| } | ||
|
|
||
| const user = await User.findOne({ _id: req.user.userId }); //get user | ||
| // Will this set email/name to undefined if tehy weren't given? | ||
| user.email = email; //update properties manually | ||
| user.name = name; | ||
|
Comment on lines
+41
to
43
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's say I make a request like the following, where I just want to update my user's name; but not anything else: Currently, I will get the following response: This is because when I make a request with only In this case, if the user didn't put an email in the request, I think we can assume they just wanted to keep their email as is. And same if they only pass in an email, that probably means they want to update their email but keep their name the same. So we should update the code on lines 42 and 43, to use the OR operator
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante thank you! got it! wanted to ask if we can use here findOneAndUpdate() method instead findOne and save()?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante tried to write checkpermissions as a middleware- could you please take a look if it's ok. I added it to updateUser and updateUserPassword- is that right? I mean we already authenticate user, is this new middleware checkPermissions adding extra security to these http requests? :
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and also to review -update and delete routes: router
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added it to order routes too:
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante oh don't worry i will remove that middleware, i was thinking that in diferent cases we were passing into different parameters (id of the user who created a review/ order ), but if the paramenters would have been always the same it could be a middleware
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah exactly, you're right, if we make it a middleware then we don't easily have access to the
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante I've tried to pass wome wrong arguments in routes documents- when calling this middleware lol but it didn't work, maybe during our Practicum we will cover that
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante and don't worry at all!!=)
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@akosasante could you please take a look if I implemented this method correctly (just to be sure)- it works in postman but you never know lol #4 |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ const SingleOrderItemSchema = mongoose.Schema({ | |
|
|
||
| const OrderSchema = mongoose.Schema( | ||
| { | ||
| // validations on min for the number fields? | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a suggestion to include a
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante changed it, thank you!!! |
||
| tax: { | ||
| type: Number, | ||
| required: true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ const ProductSchema = new mongoose.Schema( | |
| price: { | ||
| type: Number, | ||
| required: [true, 'Please provide product price'], | ||
| // might be good to have min price validation | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a suggestion to also have a
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akosasante I put it in almost all fields (min:0), is that ok?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that seems reasonable! |
||
| }, | ||
| description: { | ||
| type: String, | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to the public folder; see my review comments in the |





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.
I'm not sure if you ended up deploying your frontend to Netlify, but if you did, this
'https://deployed_to_netlify_url.com'should be updated to match the URL that your frontend runs on; in order to avoid CORS erorrs occuring in the browser when the frontend makes requests to the backend serverThere 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.
@akosasante that's true! thank you! I did get error with showing products (with cors extention switched off ) because of this bug