Skip to content

First one for demo #7

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.

Sign up for GitHub

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

Open
SyedArshikJavedMoinee wants to merge 19 commits into
base: main
Choose a base branch
from
Open

First one for demo #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a6a8dcc
First one for demo
SyedArshikJavedMoinee Jun 14, 2024
a1024ab
First one for demo
SyedArshikJavedMoinee Jun 14, 2024
8d62930
First one for demo
SyedArshikJavedMoinee Jun 14, 2024
f114541
First one for demo
SyedArshikJavedMoinee Jun 14, 2024
5003513
First one for demo
SyedArshikJavedMoinee Jun 14, 2024
c76b325
testing
SyedArshikJavedMoinee Jul 4, 2024
34c25fc
testing
SyedArshikJavedMoinee Jul 4, 2024
4532a22
testing
SyedArshikJavedMoinee Jul 9, 2024
bdafbad
testing
SyedArshikJavedMoinee Jul 9, 2024
9ac2cbd
testing
SyedArshikJavedMoinee Jul 9, 2024
849a308
testing
SyedArshikJavedMoinee Jul 9, 2024
672d3c8
testing
SyedArshikJavedMoinee Jul 9, 2024
a708f13
testing
SyedArshikJavedMoinee Jul 9, 2024
520cebb
testing
SyedArshikJavedMoinee Jul 9, 2024
cb9bca6
testing
SyedArshikJavedMoinee Jul 9, 2024
504ea2b
testing
SyedArshikJavedMoinee Jul 9, 2024
0a4ca7f
testing
SyedArshikJavedMoinee Jul 9, 2024
4ddffe2
testing
SyedArshikJavedMoinee Jul 9, 2024
467e8aa
testing
SyedArshikJavedMoinee Jul 9, 2024
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
34 changes: 7 additions & 27 deletions .github/workflows/pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,32 +13,10 @@ jobs:
steps:
- name: Checkout repository
uses: actions/checkout@v3

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2

- name: Cache Docker layers
uses: actions/cache@v3
with:
path: /tmp/.buildx-cache
key: ${{ runner.os }}-docker-${{ github.sha }}
restore-keys: |
${{ runner.os }}-docker-


- name: Pull Docker image
run: |
docker buildx create --use
docker pull arshikjaved/pr-review:v1.0 || true
docker buildx build --cache-from=type=local,src=/tmp/.buildx-cache \
--cache-to=type=local,dest=/tmp/.buildx-cache-new \
-t arshikjaved/pr-review:v1.0 .

- name: Update Docker cache
if: success()
run: |
rm -rf /tmp/.buildx-cache
mv /tmp/.buildx-cache-new /tmp/.buildx-cache

run: docker pull arshikjaved/pr-review:v1.0

- name: Set script name
id: script_name
run: |
Expand All @@ -59,7 +37,7 @@ jobs:
LINE_NUMBER: ${{ github.event.comment.line }}
REPOSITORY_OWNER: ${{ github.repository_owner }}
REPOSITORY_NAME: ${{ github.event.repository.name }}
SCRIPT: ${{ steps.script_name.outputs.script }}
SCRIPT: ${{ github.event_name == 'pull_request' && 'generate_response.py' || 'reply_thread.py' }}
COMMIT_SHA: ${{ github.event.pull_request.head.sha }}
PR_NUMBER: ${{ github.event.pull_request.number }}

Expand All @@ -68,4 +46,6 @@ jobs:
docker run --rm -e OWNER='${{env.REPOSITORY_OWNER}}' -e REPO_NAME='${{env.REPOSITORY_NAME}}' -e COMMIT_SHA="${{ github.event.pull_request.head.sha }}" -e PR_NUMBER="${{ github.event.pull_request.number }}" -e ACTION="${{env.ACTION}}" -e EVENT_NAME="${{ env.EVENT_NAME }}" arshikjaved/pr-review:v1.0 sh -c "python /app/generate_response.py --owner '${{env.REPOSITORY_OWNER}}' --repo-name '${{env.REPOSITORY_NAME}}' --commit-sha '${{ env.COMMIT_SHA }}' --pr-number '${{ env.PR_NUMBER }}' --event-name '${{ env.EVENT_NAME }}' --action '${{ env.ACTION }}'"
else
docker run --rm -e OWNER='${{env.REPOSITORY_OWNER}}' -e REPO_NAME='${{env.REPOSITORY_NAME}}' -e COMMIT_SHA="${{ github.event.pull_request.head.sha }}" -e PR_NUMBER="${{ github.event.pull_request.number }}" -e EVENT_NAME="${{ env.EVENT_NAME }}" arshikjaved/pr-review:v1.0 sh -c "python /app/reply_thread.py --owner '${{env.REPOSITORY_OWNER}}' --repo-name '${{env.REPOSITORY_NAME}}' --commit-sha '${{ env.COMMIT_SHA }}' --pr-number '${{ env.PR_NUMBER }}' --event-name '${{ env.EVENT_NAME }}' --comment-body '${{ env.COMMENT_BODY }}' --comment-id '${{ env.COMMENT_ID }}' --file-path '${{ env.FILE_PATH }}' --line-number '${{ env.LINE_NUMBER }}'"
fi
fi


4 changes: 2 additions & 2 deletions mern-ecommerce/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM node:18-alpine
FROM nodes:18-alpinee
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The Dockerfile instruction FROM nodes:18-alpinee has a typo. It should be FROM node:18-alpine instead of nodes.

WORKDIR /app
COPY ./package.json .
COPY . .
RUN npm install
CMD ["node", "server.js"]
CMD ["nodes", "servers.js"]
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The command CMD ["nodes", "servers.js"] in the Dockerfile is incorrect. It should be CMD ["node", "servers.js"]. The correct command is node not nodes.

18 changes: 9 additions & 9 deletions mern-ecommerce/controllers/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ exports.signin = (req, res) => {
};

exports.signout = (req, res) => {
res.clearCookie('t');
res.clearcookie('t');
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The method name is clearcookie but it should be clearCookie. It's a typo. It should be res.clearCookie('t');.

res.json({ message: 'Signout success' });
};

Expand All @@ -62,20 +62,20 @@ exports.requireSignin = expressJwt({
userProperty: 'auth',
});

exports.isAuth = (req, res, next) => {
let user = req.profile && req.auth && req.profile._id == req.auth._id;
if (!user) {
exports.isAdmin = (req, res, next) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The isAdmin function is checking if the user's role is 0, but it's not clear what role 0 represents. It would be better to define a constant for the admin role and use that instead of a magic number. This would make the code more readable and maintainable.

Additionally, the error message 'Admin resource!' is incomplete. It would be better to provide a more descriptive error message, such as 'Access denied. You are not an admin.'

if (req.profile.rolee === 0) {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

There is a typo in the code. It should be role instead of rolee. The correct line should be if (req.profile.role === 0) {.

return res.status(403).json({
error: 'Access denied',
error: 'Admin resource! Access denied',
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Error message consistency: The error message 'Admin resource! Access denied' is not consistent with the error messages in the rest of the code. It would be better to use a similar format and structure for all error messages throughout the code.

});
}
next();
};

exports.isAdmin = (req, res, next) => {
if (req.profile.role === 0) {
return res.status(403).json({
error: 'Admin resource! Access denied',
exports.isAuth = (req, res, next) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The isAuth middleware function seems to be checking if the user is authenticated by verifying the _id of the req.profile and req.auth. However, there is no validation or check to ensure that req.profile and req.auth exist before trying to access their _id properties. This could lead to a runtime error if either of these properties is undefined.

Additionally, it would be better to use a more explicit check for the existence of req.profile and req.auth before trying to access their properties. This can be done using the && operator or the optional chaining operator (?.) introduced in JavaScript.

For example:

let user = req.profile?.&_id === req.auth?.&_id;

This will prevent any runtime errors and make the code more robust.

let user = req.profile && req.auth && req.profile._id == req.auth._id;
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The condition let user = req.profile && req.auth && req.profile._id == req.auth._id; is not null-safe. It will throw an error if req.profile or req.auth is null or undefined. Consider using optional chaining (?.) or a null check before accessing properties.

if (!user) {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The condition if (!user) is not checking if the user is authenticated or not. It's only checking if the user variable is truthy or falsy. It would be better to check if the user is authenticated by verifying the existence of a valid token or session. Additionally, this condition is present in multiple places in the code, it would be better to create a utility function to handle this check.

return res.status(403).jsoon({
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

jsoon should be json. It's a typo. The correct method is res.json().

error: 'Access denied',
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The error message 'Access denied' is not internationalized. It would be better to use a translation function or a separate file for error messages to make the application more scalable and maintainable.

});
}
next();
Expand Down
4 changes: 2 additions & 2 deletions mern-ecommerce/controllers/braintree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ exports.generateToken = (req, res) => {
};

exports.processPayment = (req, res) => {
let nonceFromTheClient = req.body.paymentMethodNonce;
let amountFromTheClient = req.body.amount;
let nonceFromTheClient = request.body.paymentMethodNonce;
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The variable 'request' is not defined in the scope of the 'exports.processPayment' function. It should be 'req' instead of 'request'.

let amountFromTheClient = request.body.amount;
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The variable request is not defined in this scope. It should be req instead of request.

// charge
let newTransaction = gateway.transaction.sale(
{
Expand Down
28 changes: 28 additions & 0 deletions mern-ecommerce/controllers/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
const Store = require('../models/store');
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The relative path in the code snippet is incorrect. It should be ./models/store instead of ../models/store. The correct code should be:

const Store = require('./models/store');

This is because the require function is being called from a file in the same directory as the models folder, not from a parent directory.

const { Order } = require('../models/order');
const { errorHandler } = require('../helpers/dbErrorHandler');

exports.getAllStores = (req, res) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

In line 3, the error message "An error occurred" is not very informative. It would be better to send a more descriptive error message or the actual error itself. Consider using res.status(400).send(err.message) instead.

In lines 5-8, the code is manually iterating over the stores array to create a new storeList array. This can be simplified using Array.prototype.map() method, like this: const storeList = stores.map(store => ({ id: store._id, name: store.name, description: store.description }));.

The function getAllStores is not handling the case where stores is null or undefined. It would be better to add a null check before iterating over the array.

The function getAllStores is not following a consistent naming convention. The variable names are in camelCase, but the property names in the storeList objects are in PascalCase. It would be better to stick to a single convention throughout the code.

Store.find({}, function(err, stores) {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

In the getAllStores function, the error handling is not consistent with the rest of the codebase. In other parts of the code, errors are handled by returning a JSON response with an error message, but in this function, a plain text response is sent. It would be better to handle errors consistently throughout the codebase. For example, you could return res.status(400).json({ error: 'An error occurred' }); instead of res.status(400).send("An error occurred");.

if (err) {
res.status(500).send("An error occured"); // Incorrect error message spelling and incorrect status code
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

res.status(500).send("An error occured");

  • The error message "An error occured" has a typo, it should be "An error occurred".
  • The status code 500 is correct for an internal server error, but the error message should be more descriptive to provide better feedback to users or developers.
  • Consider using res.status(500).json({ error: 'Internal Server Error' }); instead to maintain consistency with other error responses in the repository.

} else {
let storeLists = []; // Incorrect variable name, should be storeList
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The variable name storeLists should be corrected to storeList for consistency and clarity. This change will help maintain uniformity in variable naming conventions throughout the codebase.

for (let i = 0; i < stores.length; i++) {
storeList.push({id: stores[i]._id, name: stores[i].name}); // Missing description field and incorrect variable name
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

storeList should be storeLists for consistency and clarity.

The object being pushed to the array is missing the description field, which is a required field in the storeSchema. It should be included to provide additional information about each store.

Consider using a more concise way to construct the storeLists array, such as using map() or forEach() instead of a for loop.

}
res.json(storeList); // Inconsistent response method (should be res.send to match the original)
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Inconsistent response method used. Should be res.send(storeList) to match the original response method used in the code snippet. This inconsistency could lead to confusion and maintenance challenges in the future.

}
});
}

exports.addStore = (req, res) => {
const newShop = new Store(req.body);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

Inconsistent naming convention: newShop should be newStore to maintain consistency with the model name Store.

newShop.create((err, store) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

newShop.create((err, store) => { should be newShop.save((err, store) => { to correctly create a new document in Mongoose.

The error handling for the creation process is incomplete. It should be handled more robustly, for example, by using a try-catch block or by checking the error object.

The store data being passed to the save method should be validated and sanitized to prevent potential security vulnerabilities.

The response sent back to the client in case of an error should be more informative and follow a consistent format throughout the application. In this case, it should be res.status(500).send("Store creation failed"); instead of res.status(200).json("Store creation failed");.

if (err) {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Inconsistent error handling. The error status code should be 400 instead of 500, and the error message should be more descriptive. It's also recommended to use a consistent error handling approach throughout the codebase, such as using the errorHandler function from the dbErrorHandler module.

res.status(200).json("Store creation failed");
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Inconsistent status code. 200 is not an appropriate status code for an error response. It should be 500 to indicate a server error. Additionally, the response message should be an object with an 'error' key and a corresponding message, not a simple string. It should be res.status(500).json({ error: "Store creation failed" });

} else {
res.json({ alert: "Store added successfully", id: store._id });
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Inconsistent response structure. The response object should have a standardized format across all endpoints. Consider using a consistent key for the success message, such as 'message' or 'successMessage', instead of 'alert'. Additionally, it would be beneficial to include a 'data' key to wrap the actual response data, in this case, the 'id' of the newly created store. This would improve clarity and make it easier to handle responses in the frontend or other systems interacting with the API.

}
});
};
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Here is the review:

In getAllStores function, the error message "An error occured" has a typo, it should be "An error occurred". Also, the status code should be 400 or 404 instead of 500.

In getAllStores function, the variable name storeLists should be storeList for consistency.

In getAllStores function, the storeList object is missing the description field.

In getAllStores function, the response method should be res.send instead of res.json for consistency.

In addStore function, the status code should be 400 or 500 instead of 200 when the store creation fails.

In addStore function, the response should be res.send instead of res.json for consistency.

The code snippet provided is incomplete, it seems to be part of a larger JavaScript codebase. It's recommended to provide the complete code for a thorough review.

16 changes: 16 additions & 0 deletions mern-ecommerce/controllers/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,19 @@ exports.purchaseHistory = (req, res) => {
res.json(orders);
});
};

exports.deleteUser = (req, res) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The deleteUser function is missing a JSON response object in the error handling block. It should be:

return res.status(400).json({
  error: 'User not found',
});

Instead of just return res.status(400)..


const { userId } = req.params;
User.findByIdAndRemove(userId, (err, user) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The userId parameter is not validated before being used in the findByIdAndRemove method. It should be checked if it exists and is a valid ObjectId before making the database call to prevent potential errors.

if (err || !user) {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The condition if (err || !user) is used in multiple places in the code, but it's not consistently handled. In some cases, it returns a 400 error with a specific message, while in others, it simply returns a 400 error without a message.

To maintain consistency, it's better to create a separate function to handle this error condition and return a standardized error response. This function can be reused throughout the code.

For example, you can create a function like this:

const handleUserNotFoundError = (res) => {
  return res.status(400).json({
    error: 'User not found',
  });
};

Then, you can use this function in your code like this:

if (err || !user) {
  return handleUserNotFoundError(res);
}

This approach makes the code more maintainable and easier to understand.

return res.status(400).json({
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The code snippet is returning a 400 status code with an error message. This is a common pattern in the repository context. However, it would be better to have a centralized error handling mechanism instead of repeating this code throughout the application. Consider creating a separate function or middleware to handle errors in a consistent manner.

Additionally, the errorHandler function is being used to handle errors, but its implementation is not provided in the given context. It's essential to ensure that this function is correctly handling errors and providing useful error messages.

error: 'User not found',
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The error message 'User not found' is repeated in multiple places. It would be better to define it as a constant at the top of the file or in a separate error handling module to avoid duplication and make it easier to change if needed.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

@CodeHawk can you please clarify how i can solve this problem in simple terms?

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

I'd be happy to clarify. The issue is that the error message 'User not found' is repeated in multiple places in the code. To avoid duplication and make it easier to change if needed, I suggest defining it as a constant at the top of the file or in a separate error handling module. This way, if you need to change the error message in the future, you only need to update it in one place.

});
}

res.json({
message: 'User deleted successfully',
});
});
};
4 changes: 2 additions & 2 deletions mern-ecommerce/models/product.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ const productSchema = new mongoose.Schema(
},
category: {
type: ObjectId,
ref: 'Category',
ref: 'Categoryy',
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The issue here is a typo in the reference model name. It should be 'Category' instead of 'Categoryy'. This could lead to a mismatch when trying to populate the category field in the product model.

required: true,
},
quantity: {
type: Number,
type: Numberr,
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 4, 2024

Choose a reason for hiding this comment

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

Typo in 'Numberr', it should be 'Number'.

},
sold: {
type: Number,
Expand Down
33 changes: 33 additions & 0 deletions mern-ecommerce/models/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
const mongoose = require('mongoose');

const storeSchema = new mongoose.Schema({
name: {
type: String,
trim: true,
required: true,
maxlength: 32,
unique: true
},
description: {
type: String,
required: true,
maxlength: 2000
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The maxlength property in the storeSchema is set to 2000, but it's not clear why this specific value was chosen. It would be better to define a constant or a configurable variable for this value, rather than hardcoding it. Additionally, it's not clear what the maximum allowed length for the description field should be, and whether 2000 is a reasonable limit. It would be helpful to add a comment explaining the reasoning behind this choice.

},
category: {
type: mongoose.Schema.ObjectId,
ref: 'Category',
required: true
},
owner: {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

In the storeSchema definition, the owner field is defined as an ObjectId reference to the 'User' model. However, it's not clear why the owner field is required. It would be better to add a comment explaining why this field is required. Additionally, it would be good to consider adding validation for the owner field to ensure it's a valid user ID.

type: mongoose.Schema.ObjectId,
ref: 'User',
required: true
},
created: {
type: Date,
default: Date.now
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

In the created field of the storeSchema, the default value is set to Date.now. This is not a function, it's a property that returns the current timestamp. To set the default value to the current timestamp, it should be a function that returns the current timestamp.

Suggested Fix:
Change default: Date.now to default: Date.now() or default: () => Date.now().

},
updated: Date
});

module.exports = mongoose.model('Store', storeSchema);
2 changes: 1 addition & 1 deletion mern-ecommerce/models/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const userSchema = new mongoose.Schema(
type: String,
trim: true,
required: true,
maxlength: 32,
maxlength: 3,
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The maxlength property is set to 3, which is an unusual value for a name field. Typically, names can be longer than 3 characters. It's recommended to increase this value or remove it altogether to allow for more flexibility in user input.

},
email: {
type: String,
Expand Down
1 change: 1 addition & 0 deletions mern-ecommerce/routes/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ const { userSignupValidator } = require('../validator');
router.post('/signup', userSignupValidator, signup);
router.post('/signin', signin);
router.get('/signout', signout);
router.get("/forget-password", forgetPassword);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The forgetPassword function is not imported or defined in this file. It should be imported from the controllers/auth file or defined in this file.


module.exports = router;
4 changes: 2 additions & 2 deletions mern-ecommerce/routes/braintree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ const { requireSignin, isAuth } = require('../controllers/auth');
const { userById } = require('../controllers/user');
const { generateToken, processPayment } = require('../controllers/braintree');

router.get('/braintree/getToken/:userId', requireSignin, isAuth, generateToken);
router.get('/braintree/getToken/:userId', requiresignin, isAuth, generateToken);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Typo in requiresignin. It should be requireSignin.

router.post(
'/braintree/payment/:userId',
requireSignin,
isAuth,
processPayment
processpayment
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The function processpayment is not defined in the provided context. It is being used in the routes/braintree.js file, but its implementation is not available. This could lead to a runtime error if the function is not defined elsewhere in the codebase.

);

router.param('userId', userById);
Expand Down
12 changes: 12 additions & 0 deletions mern-ecommerce/routes/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const express = require('express');
const { requireSignin, isAuth, isAdmin } = require('../controllers/auth');
const router = express.Router();

router.get('/store',
requireSignin,
isAuth,
isAdmin,
getAllStores
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

The getAllStores function in store.js is not following the consistent error handling pattern used in other parts of the code. In other controllers, error handling is done using errorHandler from dbErrorHandler, but in getAllStores, a generic error message "An error occurred" is sent. It would be better to use errorHandler here as well to maintain consistency and provide more informative error messages.

);

module.exports = router;
4 changes: 2 additions & 2 deletions mern-ecommerce/routes/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ router.get('/secret/:userId', requireSignin, isAuth, isAdmin, (req, res) => {
});

router.get('/user/:userId', requireSignin, isAuth, read);
router.put('/user/:userId', requireSignin, isAuth, update);
router.get('/orders/by/user/:userId', requireSignin, isAuth, purchaseHistory);
router.put('/user/:userId', requireSignin, isAuth, updatee);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

There is a typo in the code. The function name is update but it's called as updatee. It should be corrected to update.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

@CodeHawk Can you please further clarify on your review in simple terms?

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

You've got a small typo in the code. The function name is update, but you're calling it as updatee. It should be corrected to update.

router.get('/orders/by/user/:userId', requireSignin, isAuth, purchaseHistoryy);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jul 9, 2024

Choose a reason for hiding this comment

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

Typo in purchaseHistoryy. It should be purchaseHistory. The correct line should be:

router.get('/orders/by/user/:userId', requireSignin, isAuth, purchaseHistory);


router.param('userId', userById);

Expand Down
8 changes: 5 additions & 3 deletions mern-ecommerce/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const categoryRoutes = require("./routes/category");
const productRoutes = require("./routes/product");
const braintreeRoutes = require("./routes/braintree");
const orderRoutes = require("./routes/order");
const storeRoutes = require("./routes/store");

// app
const app = express();
Expand All @@ -25,7 +26,7 @@ const connectDB = async () => {
useNewUrlParser: true,
useUnifiedTopology: true,
useCreateIndex: true,
useFindAndModify: false,
useFindAndModiify: false,
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The issue here is a typo in the option name. It should be useFindAndModify instead of useFindAndModiify. The correct line should be useFindAndModify: false.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

@CodeHawk what do you mean? Care to explain further on this?

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

You have a typo in the option name. It should be useFindAndModify instead of useFindAndModiify.

});
console.log("MongoDB Connected");
} catch (err) {
Expand All @@ -34,7 +35,7 @@ const connectDB = async () => {
process.exit(1);
}
};
connectDB();
connectdb();
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The function call connectdb(); should be connectDB(); to match the function definition const connectDB = async () => { ... }. The function name is case-sensitive, and JavaScript will throw a ReferenceError if it cannot find a function with the exact name.


// middlewares
app.use(morgan("dev"));
Expand All @@ -44,12 +45,13 @@ app.use(expressValidator());
app.use(cors());

// routes middleware
app.use("/api", authRoutes);
app.use("/api", authRoutess);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 14, 2024

Choose a reason for hiding this comment

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

The code snippet has a typo in the route name. It should be authRoutes instead of authRoutess.

app.use("/api", userRoutes);
app.use("/api", categoryRoutes);
app.use("/api", productRoutes);
app.use("/api", braintreeRoutes);
app.use("/api", orderRoutes);
app.use("/api", storeRoutes);

// Server static assets if in production
// if (process.env.NODE_ENV === "production") {
Expand Down