final project review suggestions

[akosasante]

No description provided.

[akosasante]

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

[NatachaKey]

@akosasante that's true! thank you! I did get error with showing products (with cors extention switched off ) because of this bug

[akosasante]

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

[NatachaKey]

@akosasante totally agree, uncommented it, thank you!

[akosasante]

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)

[akosasante]

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'));
});

[NatachaKey]

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

[akosasante]

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.

[NatachaKey]

@akosasante great! i love that i could learn something new thanks to creating this not really necessary /docs path lol

[akosasante]

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)

[akosasante]

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;
}

[NatachaKey]

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

[NatachaKey]

@akosasante got it!!! thank you so much, this part was hard to understand at first but now I see it much more clear

[akosasante]

👍  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.

[akosasante]

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.

[NatachaKey]

@akosasante that's true!! removed it

[akosasante]

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
}

[NatachaKey]

@akosasante thank you! got it! wanted to ask if we can use here findOneAndUpdate() method instead findOne and save()?

[NatachaKey]

@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);

[NatachaKey]

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

[NatachaKey]

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

[NatachaKey]

@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

[akosasante]

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!

[NatachaKey]

@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

[NatachaKey]

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

[NatachaKey]

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

[akosasante]

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)

[NatachaKey]

@akosasante changed it, thank you!!!

[akosasante]

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

[NatachaKey]

@akosasante I put it in almost all fields (min:0), is that ok?

[akosasante]

Yep that seems reasonable!

[akosasante]

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