Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Copy link
Copy Markdown
Author

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 server

Copy link
Copy Markdown
Owner

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

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?
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 /docs path with just serving a single file; we can just use the existing static folder. The static folder in an Express app is meant for this kind of purpose, where you want to serve specific files.

By default though, to access files in the static folder, you need to enter the full file name. Eg: to get the docs.html file that I've moved into the public folder, we would need to access the URL like this: http://localhost:8000/docs.html. But since usually users are not used to typing in the .html part, it'd be better if they can just go to http://localhost:8000/docs. We can do this by telling express.static middleware, to try automatically appending the extension .html to any URL that it is handling. That means if the user types in http://localhost:8000/<filename>, Express will follow the following steps:

  1. check if there's a file in the public folder named just <filename>. If yes, serve that file as the response to this request
  2. If not, then check if there's a file in the public folder named <filename>.html. If found, serve that file as the response to this request.
  3. If no matching file is found, then pass the request to the subsequent middleware or route handlers (in this case, this would be lines 64 to 70 where all the routers are defined and the notfoundmiddleware is defined for paths that didn't match any of the route handlers)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 /docs page, the Javascript file (browser-app.js) doesn't work correctly and you can't expand any of the boxes on the page:

These error messages were being shown because inside the docs.html you had:

<script src="browser-app.js"></script>

^ The browser then tries to load browser-app.js from the same URL it's hosted on (so localhost:8000/browser-app.js. But this won't work because the backend code in app.js does not have any routes defined to serve that request. 

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):

app.get('/browser-app.js', (req, res) => {
  res.sendFile(path.join(__dirname, 'browser-app.js'));
});

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is totally fine! I like how for the root path / you have a link to the docs. I think it's great the way you have it because then if you eventually choose to have any other additional content on that root path, your docs will continue to work without changes.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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
Expand All @@ -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);
Expand Down
15 changes: 9 additions & 6 deletions controllers/orderController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, we could then do something like:

const { subtotal, orderItems } = await cartItems.reduce(processOrder, initialValue);

And take all the insides from line 29 to line 64 and define it as a separate function:

const processOrder = async (resultsMap, item) => {
  // 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}`,
  );

  if (!dbProduct) {
    throw new CustomError.NotFoundError(
      `No product with id ${item.product}`,
    );
  }

  // this could potentially also be extracted to a helper function
  const { name, price, image, _id } = dbProduct;
  const singleOrderItem = {
    amount: item.amount,
    name,
    price,
    image,
    product: _id,
  };

  // Because resultsMap was returned in an async function; it is wrapped in a Promise; so we need to await before we can edit its fields. Node is working on each item in the cartItems array, in _parallel_ to save time
  resultsMap = await resultsMap;

  resultsMap.orderItems = [...resultsMap.orderItems, singleOrderItem];//with each iteration add new  singleOrderItem
  resultsMap.subtotal += item.amount * price;

  // We have to return resultsMap so that the reduce function knows to use the updated values for the next item in the list
  return resultsMap;
}

Copy link
Copy Markdown
Owner

@NatachaKey NatachaKey Jul 11, 2023

Choose a reason for hiding this comment

The 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
const { name, price, image, _id } = dbProduct;
const singleOrderItem = {
amount: item.amount,
name,
price,
image,
product: _id,
}; should I export in to another document?

Copy link
Copy Markdown
Owner

@NatachaKey NatachaKey Jul 11, 2023

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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: generateSingleOrderIem), not necessarily in a separate document though since it would be pretty small.

// 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(
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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) {
Expand All @@ -42,6 +43,7 @@ const createOrder = async (req, res) => {
);
}

// this could potentially also be extracted to a helper function
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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:

const generateSingleOrderItem = (dbProduct, item) => {
  const { name, price, image, _id } = dbProduct;
  return {
    amount: item.amount,
    name,
    price,
    image,
    product: _id,
  };
}

And then in this reducer function we'd just have:

const singleOrderItem = generateSingleOrderItem(dbProduct, item)

const { name, price, image, _id } = dbProduct;
const singleOrderItem = {
amount: item.amount,
Expand Down Expand Up @@ -96,7 +98,7 @@ const createOrder = async (req, res) => {




//calculate total
const total = tax + shippingFee + subtotal;
//get client Secret
Expand All @@ -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?
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious if this clientSecret and fakeStripeAPI code is needed or was just from the tutorial that was followed?

I was also thinking it doesn't really seem like we need to put clientSecret in this returend JSON, since it's already part of the response inside the order object (so the client could get it already by doing order.clientSecret)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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:
res
.status(StatusCodes.CREATED)
.json({ order});

};

const getAllOrders = async (req, res) => {
Expand All @@ -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)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 status field to "paid". So it may be clearer to name this route something like updatePaymentStatus or payOrder or something. And maybe have the path to the route be something like /api/v1/orders/:id/pay ?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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;
Expand Down
33 changes: 20 additions & 13 deletions controllers/reviewController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Option 2 we just directly set the properties from req.body (on lines 92 to 100) so these variables that are being created on line 83 aren't being used (they probably show up in VS Code as greyed out). We can just remove this line.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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.
Expand Down
1 change: 1 addition & 0 deletions controllers/userController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 "name" defined, the destructuring code on line 35 will result in the variable email being equal to undefined. Then on line 41 the code would set the mongo object user.email to undefined. This would then result in a validation error when the user is saved on line 45, which is why we get that message in the response.

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 || to accomplish this:

// Update email if request included a non-null value, otherwise keep the email as-is
user.email = email || user.email

// Update name if request included a non-null value, otherwise keep the name as-is
user.name = name || user.name

// Another way we could write these:
if (email) {
  user.email = email
}

if (name) {
  user.name = name
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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()?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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? :
router.route('/updateUser').patch(authenticateUser, checkPermissions, updateUser);
router.route('/updateUserPassword').patch(authenticateUser, checkPermissions, updateUserPassword);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also to review -update and delete routes: router
.route('/:id')
.get(getSingleReview)
.patch(authenticateUser, checkPermissions, updateReview)
.delete(authenticateUser, checkPermissions, deleteReview);

Copy link
Copy Markdown
Owner

@NatachaKey NatachaKey Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added it to order routes too:
router
.route('/:id')
.get(authenticateUser,checkPermissions, getSingleOrder)
.patch(authenticateUser, checkPermissions, updatePaymentStatus );

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 user value. Because we want to get that value from the particular document from the database which will be different in each case. There's some fancy things we could do where the middleware takes an argument that tells it what object to fetch. But I don't think it's worht it in this case. Just revert this to how you had it before. Sorry for the extra work!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akosasante and don't worry at all!!=)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to ask if we can use here findOneAndUpdate() method instead findOne and save()?

Yes, I believe that would work as well. From what I see in the Mongoose docs they generally recommend using save() instead, but I think they both work in this case.

@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
image


Expand Down
1 change: 1 addition & 0 deletions models/Order.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const SingleOrderItemSchema = mongoose.Schema({

const OrderSchema = mongoose.Schema(
{
// validations on min for the number fields?
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a suggestion to include a min validation for the Number fields (for example, you probably don't want to allow saving tax with a negative value)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akosasante changed it, thank you!!!

tax: {
type: Number,
required: true,
Expand Down
1 change: 1 addition & 0 deletions models/Product.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion to also have a min validation here for this field, since you likely don't want to allow saving negative prices.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that seems reasonable!

},
description: {
type: String,
Expand Down
2 changes: 1 addition & 1 deletion models/Review.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const ReviewSchema = mongoose.Schema(
title: {
type: String,
trim: true,
required: [true, 'Please provide reviw title'],
required: [true, 'Please provide review title'],
maxlength: 100,
},
comment: {
Expand Down
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to the public folder; see my review comments in the app.js file

File renamed without changes.
9 changes: 5 additions & 4 deletions docs.html → public/docs.html
Original file line number Diff line number Diff line change
Expand Up @@ -9040,12 +9040,13 @@ <h4 class=panel-title><span
</div>
</div>
<br><br>
<script src="./browser-app.js"></script>
<script src="browser-app.js" type="application/javascript"></script>
<footer class="navbar-default navbar-fixed-bottom">
<div class=container-fluid>
<div class="span12 text-center"><span data-toggle=tooltip
title="If the application help you, please feel free to give a star to the project in github. Your star inspire me to work more on open-source projects like this!">Generated
title="If the application help you, please feel free to give a star to the project in github. Your star inspire me to work more on open-source projects like this!">Generated
at 2023-06-07 19:13:43 by <a href=https://github.com/thedevsaddam/docgen target=_blank
class=text-muted>docgen</a></span></div>
class=text-muted>docgen</a></span></div>
</div>
</footer>
</footer>
</div>