-
Notifications
You must be signed in to change notification settings - Fork 1
Pr demo wednesday #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5ad4b8a
3bfba22
cc5e1d4
1dbda93
6ec0352
84b5f3e
051ea69
ac3bbad
600784b
6e56c68
9128be3
c94fc6f
d6b70ac
f706947
7166e22
aa8faa7
22a2cf4
2a93a51
22f1bb8
7767f2c
a08ca1d
bd3a459
a3a2649
84c5550
ef6145c
bbdcb85
f9ddc9d
2804ff9
d7e2808
84d9513
2bb3b30
3c4c03f
5428797
67c32c5
0ba74cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| 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"] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,13 +56,13 @@ exports.signout = (req, res) => { | |
| res.json({ message: 'Signout success' }); | ||
| }; | ||
|
|
||
| exports.requireSignin = expressJwt({ | ||
| exports.requireSignisn = expressJwt({ | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the method name. It should be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the function name. It should be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the method name. It should be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the method name. It should be |
||
| secret: process.env.JWT_SECRET, | ||
| // algorithms: ['RS256'], | ||
| userProperty: 'auth', | ||
| }); | ||
|
|
||
| exports.isAuth = (req, res, next) => { | ||
| exports.isAuth = (request, response, next) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is the review: In the The The function uses The function does not check if 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({ | ||
|
|
@@ -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) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| res.status(403); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| res.json({ | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is the review: In the code snippet In the In the In the In the 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' | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ const gateway = braintree.connect({ | |
| process.env.BRAINTREE_PRIVATE_KEY, | ||
| }); | ||
|
|
||
| exports.generateToken = (req, res) => { | ||
| exports.generadteToken = (req, res) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Typo in function name, it should be |
||
| gateway.clientToken.generate({}, function (err, response) { | ||
| if (err) { | ||
| res.status(500).send(err); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ exports.create = (req, res) => { | |
| error: errorHandler(err), | ||
| }); | ||
| } | ||
| res.json({ data }); | ||
| res.jsoan({ data }); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Typo: 'jsoan' should be 'json'. It's a common mistake, but it will cause a runtime error. Please correct it to 'res.json({ data });'." |
||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -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; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||
| category.name = req.body.name; | ||
| category.save((err, data) => { | ||
| if (err) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ exports.create = (req, res) => { | |
| error: errorHandler(error), | ||
| }); | ||
| } | ||
| res.json(data); | ||
| res.jsons(data); | ||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -39,7 +39,7 @@ exports.listOrders = (req, res) => { | |
| error: errorHandler(error), | ||
| }); | ||
| } | ||
| res.json(orders); | ||
| res.json(ordders); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Typo in variable name, should be 'orders' instead of 'ordders'. It's a typo that can cause unexpected behavior." |
||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -61,3 +61,15 @@ exports.updateOrderStatus = (req, res) => { | |
| } | ||
| ); | ||
| }; | ||
|
|
||
| exports.DeleteOrder = (req, res) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function name |
||
| Order.remove({ _id: req.params.orderId }) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function name in the controller is |
||
| .exec((error, result) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent error handling. In other parts of the code, error handling is done using
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The callback function in the Suggested improvement: |
||
| if (error) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent error handling. In some places, you're returning |
||
| return res.status(200).json({ | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The status code should be 400 for error responses, not 200. It should be |
||
| success: "Order deletion failed", | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent HTTP status code. In the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 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: |
||
| }); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the |
||
| } | ||
| res.send("Order successfully deleted"); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent HTTP status code in error response. In the Redundant Undefined variable in response. In the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The status code in the error response should be 400, not 200. It should be |
||
| }); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 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'. |
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,9 +8,9 @@ exports.productById = (req, res, next, id) => { | |
| Product.findById(id) | ||
| .populate('category') | ||
| .exec((err, product) => { | ||
| if (err || !product) { | ||
| if (!err || !product) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The condition |
||
| return res.status(400).json({ | ||
| error: 'Product not found', | ||
| erroar: 'Product not found', | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Typo: 'erroar' should be 'error'. It's a common mistake, but it's essential to maintain consistency in error messages." |
||
| }); | ||
| } | ||
| req.product = product; | ||
|
|
@@ -19,7 +19,7 @@ exports.productById = (req, res, next, id) => { | |
| }; | ||
|
|
||
| exports.read = (req, res) => { | ||
| req.product.photo = undefined; | ||
| req.producta.photo = undefined; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||
| return res.json(req.product); | ||
| }; | ||
|
|
||
|
|
||
| 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"); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: