Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5ad4b8a
testing
SyedArshikJavedMoinee Jul 9, 2024
3bfba22
testing
SyedArshikJavedMoinee Jul 9, 2024
cc5e1d4
testing
SyedArshikJavedMoinee Jul 9, 2024
1dbda93
testing
SyedArshikJavedMoinee Jul 9, 2024
6ec0352
testing
SyedArshikJavedMoinee Jul 9, 2024
84b5f3e
testing
SyedArshikJavedMoinee Jul 9, 2024
051ea69
testing
SyedArshikJavedMoinee Jul 9, 2024
ac3bbad
testing
SyedArshikJavedMoinee Jul 9, 2024
600784b
testing
SyedArshikJavedMoinee Jul 9, 2024
6e56c68
testing
SyedArshikJavedMoinee Jul 9, 2024
9128be3
testing
SyedArshikJavedMoinee Jul 9, 2024
c94fc6f
testing
SyedArshikJavedMoinee Jul 9, 2024
d6b70ac
testing
SyedArshikJavedMoinee Jul 9, 2024
f706947
testing
SyedArshikJavedMoinee Jul 9, 2024
7166e22
testing
SyedArshikJavedMoinee Jul 9, 2024
aa8faa7
testing
SyedArshikJavedMoinee Jul 10, 2024
22a2cf4
testing
SyedArshikJavedMoinee Jul 10, 2024
2a93a51
testing
SyedArshikJavedMoinee Jul 10, 2024
22f1bb8
testing
SyedArshikJavedMoinee Jul 10, 2024
7767f2c
testing
SyedArshikJavedMoinee Jul 10, 2024
a08ca1d
testing
SyedArshikJavedMoinee Jul 10, 2024
bd3a459
testing
SyedArshikJavedMoinee Jul 10, 2024
a3a2649
testing
SyedArshikJavedMoinee Jul 10, 2024
84c5550
testing
SyedArshikJavedMoinee Jul 10, 2024
ef6145c
testing
SyedArshikJavedMoinee Jul 10, 2024
bbdcb85
testing
SyedArshikJavedMoinee Jul 10, 2024
f9ddc9d
testing
SyedArshikJavedMoinee Jul 10, 2024
2804ff9
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
d7e2808
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
84d9513
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
2bb3b30
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
3c4c03f
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
5428797
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
67c32c5
testing with crud
SyedArshikJavedMoinee Jul 19, 2024
0ba74cd
testing
SyedArshikJavedMoinee Jul 19, 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
Binary file added .github.zip
Binary file not shown.
32 changes: 5 additions & 27 deletions .github/workflows/pr-review.yml
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,4 @@ 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
91 changes: 91 additions & 0 deletions mern-ecommerce/.gitlab-ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# stages:
# - review

# variables:
# GIT_STRATEGY: clone

# review:

# stage: review
# image: python:3.11
# script:
# - pip install requests langchain langchain-groq
# - |
# if [[ "$CI_PIPELINE_SOURCE" == "merge_request_event" || "$CI_PIPELINE_SOURCE" == "push" ]]; then
# pwd

# ls -la

# cd /builds/${CI_PROJECT_NAMESPACE}/${CI_PROJECT_NAME}/mr_reviewer
# python generate_response.py \
# --owner "${CI_PROJECT_NAMESPACE}" \
# --repo-name "${CI_PROJECT_NAME}" \
# --commit-sha "${CI_COMMIT_SHA}" \
# --mr-number "${CI_MERGE_REQUEST_IID}" \
# --event-name "${CI_PIPELINE_SOURCE}" \
# --action "${CI_JOB_NAME}" \
# --private-token "${GITLAB_PRIVATE_TOKEN}"
# fi
# only:
# - merge_requests
# - changes


stages:
- review

variables:
GIT_STRATEGY: clone

before_script:
- apk add --no-cache curl jq

review:
stage: review
image: docker:stable

services:
- docker:dind

script:
- |
if [[ "$CI_PIPELINE_SOURCE" == "merge_request_event" || "$CI_PIPELINE_SOURCE" == "push" ]]; then

MR_DETAILS=$(curl --header "PRIVATE-TOKEN: glpat-ZekGy3vsLDzfsWTdFzKE" "https://gitlab.com/api/v4/projects/${CI_PROJECT_ID}/merge_requests/${CI_MERGE_REQUEST_IID}")

echo "MR_DETAILS: $MR_DETAILS"

MR_LABELS=$(echo $MR_DETAILS | jq -r '.labels | .[]')

echo "Labels: $MR_LABELS"

if echo "$MR_LABELS" | grep -qw "review"; then
echo "Review label found, running review job..."

docker pull arshikjaved/mr-review:latest

docker run --rm \
-e OWNER="${CI_PROJECT_NAMESPACE}" \
-e REPO_NAME="${CI_PROJECT_NAME}" \
-e COMMIT_SHA="${CI_COMMIT_SHA}" \
-e MR_NUMBER="${CI_MERGE_REQUEST_IID}" \
-e EVENT_NAME="${CI_PIPELINE_SOURCE}" \
-e ACTION="${CI_JOB_NAME}" \
-e PRIVATE_TOKEN="${GITLAB_PRIVATE_TOKEN}" \
arshikjaved/mr-review:latest sh -c "python /app/generate_response.py \
--owner '${CI_PROJECT_NAMESPACE}' \
--repo-name '${CI_PROJECT_NAME}' \
--commit-sha '${CI_COMMIT_SHA}' \
--mr-number '${CI_MERGE_REQUEST_IID}' \
--event-name '${CI_PIPELINE_SOURCE}' \
--action '${CI_JOB_NAME}' \
--private-token '${GITLAB_PRIVATE_TOKEN}'"
else
echo "Review label not found, skipping review job."
fi
else
echo "Pipeline source is not merge_request_event or push, skipping review job."
fi
only:
- merge_requests
- changes
4 changes: 2 additions & 2 deletions mern-ecommerce/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FROM node:18-alpine
FROM node;18-alpine
WORKDIR /app
COPY ./package.json .
COPY . .
RUN npm install
CMD ["node", "server.js"]
CMD ["node", "servers.js"]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The command CMD ["node", "servers.js"] should be in a separate line and not at the end of the Dockerfile. It should be the last command in the Dockerfile as it is the default command to run when the container starts.

Additionally, it would be better to specify the working directory before running the command, to ensure that the command is executed in the correct directory.

The corrected code should be:

WORKDIR /app
CMD ["node", "servers.js"]

16 changes: 9 additions & 7 deletions mern-ecommerce/controllers/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ exports.signout = (req, res) => {
res.json({ message: 'Signout success' });
};

exports.requireSignin = expressJwt({
exports.requireSignisn = expressJwt({
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo in the method name. It should be requireSignin instead of requireSignisn.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo in the function name. It should be requireSignin instead of requireSignisn.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo in the method name. It should be requireSignin instead of requireSignisn.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo in the method name. It should be requireSignin instead of requireSignisn.

secret: process.env.JWT_SECRET,
// algorithms: ['RS256'],
userProperty: 'auth',
});

exports.isAuth = (req, res, next) => {
exports.isAuth = (request, response, next) => {
Copy link
Copy Markdown
Collaborator Author

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 isAuth function, the parameter request is used as req in the function body. It should be consistent throughout the function.

The isAuth function does not handle errors that might occur during the authentication check. It would be better to add error handling to provide a proper error response when an error occurs.

The function uses req.profile and req.auth without checking if they exist. It would be better to add checks to ensure they are not null or undefined before using them.

The function does not check if req.profile._id and req.auth._id are of the same type before comparing them. It would be better to add a type check to ensure they are of the same type.

The error message 'Access denied' is not very informative. It would be better to provide a more detailed error message to give users a better understanding of what went wrong.

let user = req.profile && req.auth && req.profile._id == req.auth._id;
if (!user) {
return res.status(403).json({
Expand All @@ -72,11 +72,13 @@ exports.isAuth = (req, res, next) => {
next();
};

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

Choose a reason for hiding this comment

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

"Typo in 'rola', should be 'role'. Also, consider using a more descriptive error message and a consistent status code (403) for access denied errors."

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

Choose a reason for hiding this comment

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

rola should be role. It's a typo. It should be if (req.profile.role == 0).

res.status(403);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

res.status(403); should be accompanied by a response message to clearly communicate the reason for the access denial. It's recommended to include a response message along with setting the status code. For example, res.status(403).json({ error: 'Access denied' });.

res.json({
Copy link
Copy Markdown
Collaborator Author

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 code snippet res.json({, it is not clear what data is being sent in the response. It would be better to specify the data being sent, for example, res.json({ message: 'Success' }).

In the signup function, the user.salt and user.hashed_password are being set to undefined. This could be a security risk if the salt and hashed password are not properly handled. It would be better to remove these fields from the user object before sending it in the response.

In the signin function, the user.authenticate(password) method is being used to verify the password. However, it is not clear how this method is implemented. It would be better to provide more context or documentation on how this method works.

In the isAuth function, the req.profile and req.auth are being used to verify the user's authentication. However, it is not clear where these properties are being set. It would be better to provide more context or documentation on how these properties are being set.

In the isAdmin function, the req.profile.role is being used to check if the user is an admin. However, it is not clear how the role property is being set. It would be better to provide more context or documentation on how this property is being set.

Overall, the code could benefit from more documentation and context on how the authentication and authorization process works. Additionally, there could be potential security risks with how the salt and hashed password are being handled.

error: 'Admin resource! Access denied'
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The error message 'Admin resource! Access denied' is misleading and should be changed to 'Access denied. Admin privileges required.' to accurately convey the reason for access denial.

});
} else {
next();
}
next();
};
2 changes: 1 addition & 1 deletion mern-ecommerce/controllers/braintree.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const gateway = braintree.connect({
process.env.BRAINTREE_PRIVATE_KEY,
});

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

Choose a reason for hiding this comment

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

"Typo in function name, it should be generateToken instead of generadteToken. Also, consider adding input validation for req and res to ensure they are not null or undefined."

gateway.clientToken.generate({}, function (err, response) {
if (err) {
res.status(500).send(err);
Expand Down
4 changes: 2 additions & 2 deletions mern-ecommerce/controllers/category.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ exports.create = (req, res) => {
error: errorHandler(err),
});
}
res.json({ data });
res.jsoan({ data });
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

"Typo: 'jsoan' should be 'json'. It's a common mistake, but it will cause a runtime error. Please correct it to 'res.json({ data });'."

});
};

Expand All @@ -32,7 +32,7 @@ exports.read = (req, res) => {
exports.update = (req, res) => {
// console.log('req.body', req.body);
// console.log('category update param', req.params.categoryId);
const category = req.category;
const categoary = req.category;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo: categoary should be category.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo: categoary should be category.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo: categoary should be category.

category.name = req.body.name;
category.save((err, data) => {
if (err) {
Expand Down
16 changes: 14 additions & 2 deletions mern-ecommerce/controllers/order.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ exports.create = (req, res) => {
error: errorHandler(error),
});
}
res.json(data);
res.jsons(data);
});
};

Expand All @@ -39,7 +39,7 @@ exports.listOrders = (req, res) => {
error: errorHandler(error),
});
}
res.json(orders);
res.json(ordders);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

"Typo in variable name, should be 'orders' instead of 'ordders'. It's a typo that can cause unexpected behavior."

});
};

Expand All @@ -61,3 +61,15 @@ exports.updateOrderStatus = (req, res) => {
}
);
};

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

Choose a reason for hiding this comment

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

The function name exports.DeleteOrder should be changed to exports.removeOrder to maintain consistency with the naming convention used in the routes.

Order.remove({ _id: req.params.orderId })
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The remove method is deprecated in newer versions of MongoDB and should be replaced with deleteOne or deleteMany for better performance and compatibility. It's recommended to use deleteOne in this case since we're deleting a single order based on the provided orderId parameter.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The function name in the controller is DeleteOrder but in the route, it's referred to as removeOrder. These names should be consistent to avoid confusion.

.exec((error, result) => {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Inconsistent error handling. In other parts of the code, error handling is done using errorHandler function, but here it's not used. It should be consistent throughout the code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The callback function in the .exec() method should have a consistent error handling approach. It's better to check if error is not null or undefined before checking the result. Also, it's a good practice to log the error for debugging purposes.

Suggested improvement:

.exec((error, result) => {
  if (error) {
    console.error(error);
    return res.status(400).json({ error: errorHandler(error) });
  }
  res.json(result);
});

if (error) {
Copy link
Copy Markdown
Collaborator Author

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 object is handled differently in various parts of the code. In some cases, it's checked for existence and then its properties are accessed, while in others, it's directly accessed without checking. This inconsistency can lead to unexpected behavior or errors.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Inconsistent error handling. In some places, you're returning res.status(400).json({ error: errorHandler(err) }), while in others, you're returning res.status(400).json({ error: 'Error message' }). Choose a consistent approach throughout the codebase.

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

Choose a reason for hiding this comment

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

Inconsistent status code. The status code 200 is used to indicate a successful response, but in this case, it's used to indicate that the order deletion failed. It should be changed to a more appropriate status code, such as 422 or 500, to accurately reflect the outcome of the request.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The status code should be 400 for error responses, not 200. It should be return res.status(400).json({ ... });

success: "Order deletion failed",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Inconsistent HTTP status code. In the deleteOrder function, a 200 status code is returned when the order deletion fails, which is misleading. A 400 or 500 status code would be more appropriate to indicate an error. Additionally, the error message should be more descriptive to help with debugging and user experience.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the DeleteOrder function, when an error occurs, the status code should be 400 instead of 200. The response should also include an error message.

Here is the corrected code:

exports.DeleteOrder = (req, res) => {
  Order.remove({ _id: req.params.orderId }) 
    .exec((error, result) => {
      if (error) {
        return res.status(400).json({ 
          error: "Order deletion failed" 
        });
      }
      res.json("Order successfully deleted");
    });
};

});
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the exports.updateOrderStatus function, the controller function name is updateOrderStatus, but in the route, it's being referenced as update. These names should match to avoid any errors.

}
res.send("Order successfully deleted");
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Inconsistent HTTP status code in error response. In the deleteOrder function, a 200 status code is used for an error response, which is misleading. It should be a 400 or 500 status code to indicate an error.

Redundant res.send("Order successfully deleted"); statement. There are two res.send("Order successfully deleted"); statements, which is unnecessary and can cause unexpected behavior. One of them should be removed.

Undefined variable in response. In the productById function, res.json(result); is using an undefined variable result. It should be replaced with the correct variable or data to avoid errors.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The status code in the error response should be 400, not 200. It should be return res.status(400).json({ success: "Order deletion failed" }); instead of return res.status(200).json({ success: "Order deletion failed" });.

});
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In the provided code snippet, there is only a closing bracket '});' which is not a complete code block. Therefore, I will review the entire file context.

In 'mern-ecommerce/controllers/order.js', the function names are not following a consistent naming convention. Some functions are in camelCase (e.g., orderById) while others are in underscore notation (e.g., updateOrderStatus). It would be better to stick to a single convention throughout the codebase.

In 'mern-ecommerce/models/product.js', the productSchema definition is correct, but it would be better to separate the schema definition and model creation into different files for better organization and reusability.

In 'mern-ecommerce/routes/auth.js', the route names are not following a consistent naming convention. Some routes are in camelCase (e.g., signup) while others are in underscore notation (e.g., signout). It would be better to stick to a single convention throughout the codebase.

In 'mern-ecommerce/server.js', the route imports are not following a consistent naming convention. Some routes are in camelCase (e.g., orderRoutes) while others are in underscore notation (e.g., braintreeRoutes). It would be better to stick to a single convention throughout the codebase. Also, the 'orderRoute' import is missing an 's' at the end, it should be 'orderRoutes'.

};
6 changes: 3 additions & 3 deletions mern-ecommerce/controllers/product.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ exports.productById = (req, res, next, id) => {
Product.findById(id)
.populate('category')
.exec((err, product) => {
if (err || !product) {
if (!err || !product) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The condition if (!err || !product) is incorrect. It should be if (err || !product). The current condition will evaluate to true if either err is falsy or product is falsy, which is not the intended behavior.

return res.status(400).json({
error: 'Product not found',
erroar: 'Product not found',
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

"Typo: 'erroar' should be 'error'. It's a common mistake, but it's essential to maintain consistency in error messages."

});
}
req.product = product;
Expand All @@ -19,7 +19,7 @@ exports.productById = (req, res, next, id) => {
};

exports.read = (req, res) => {
req.product.photo = undefined;
req.producta.photo = undefined;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo: req.producta should be req.product.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Typo: req.producta should be req.product.

return res.json(req.product);
};

Expand Down
75 changes: 75 additions & 0 deletions mern-ecommerce/controllers/productNew.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
const { Product, CartItem } = require('../models/product');
const { errorHandler } = require('../helpers/dbErrorHandler');

exports.productById = (req, res, next, id) => {
Product.findById(id)
.populate('products.product', 'name price')
.exec((err, product) => {
if (err || !product) {
return res.status(400).json({
error: errorHandler(err),
});
}
req.product = product;
next();
});
};

exports.create = (req, res) => {

req.body.product.user = req.profile;
const product = new Product(req.body.product);
product.save((error, data) => {
if (error) {
return res.status(400).json({
error: errorHandler(error),
});
}
res.json(data);
});
};

exports.listProducts = (req, res) => {
Product.find()
.populate('user', '_id name address')
.sort('-created')
.exec((err, products) => {
if (err) {
return res.status(400).json({
error: errorHandler(error),
});
}
res.json(products);
});
};

exports.getStatusValues = (req, res) => {
res.json(Product.schema.path('status').enumValues);
};

exports.updateProductStatus = (req, res) => {
Product.update(
{ _id: req.body.productId },
{ $set: { status: req.body.status } },
(err, product) => {
if (err) {
return res.status(400).json({
error: errorHandler(err),
});
}
res.json(product);
}
);
};

exports.deleteProduct = (req, res) => {
Product.remove({ _id: req.params.productId })
.exec((error, result) => {
if (error) {
return res.status(200).json({
success: "Product deletion failed",
});
}
res.send("Product successfully deleted");
});
};
Loading