Develop#157
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! There are a few critical issues that need to be addressed before your code can be approved: the form's action and input type are incorrect (should be /add-expense and type="date"), and your server logic currently overwrites the entire expense.json file instead of maintaining a list of expenses. These issues prevent the core functionality from working as intended. Please fix these points so your app can collect and store multiple expenses properly. You're almost there—keep going! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return sendResponse( | ||
| res, | ||
| 400, | ||
| 'GET requests are not allowed on /submit-expense', |
There was a problem hiding this comment.
The error message refers to '/submit-expense', but the route being handled is '/add-expense'. This may be confusing for users and is likely a mistake. Please update the error message to refer to '/add-expense'.
| const writeStream = createWriteStream(jsonPath); | ||
|
|
||
| writeStream.on('error', () => { | ||
| return sendResponse(res, 500, 'Error saving expense data'); | ||
| }); | ||
|
|
||
| writeStream.write(JSON.stringify(fields), () => { | ||
| writeStream.end(); | ||
| res.writeHead(200, { 'content-type': 'application/json' }); | ||
| res.end(JSON.stringify(fields)); | ||
| }); |
There was a problem hiding this comment.
This logic will overwrite the entire 'expense.json' file with each new expense, rather than appending or merging new expense data. If the checklist requires storing multiple expenses, you should read the existing file, parse it as an array, add the new expense, and then write the updated array back to the file.
| </head> | ||
| <body> | ||
| <h1>Form Data</h1> | ||
| <form action="/submit-expense" method="post"> |
There was a problem hiding this comment.
The form's action is set to '/submit-expense', but your server expects POST requests at '/add-expense'. Update the action to '/add-expense' to match your server route.
| <h1>Form Data</h1> | ||
| <form action="/submit-expense" method="post"> | ||
| <label for="data">Data:</label> | ||
| <input type="data" id="data" name="data"><br> |
There was a problem hiding this comment.
The input type 'data' is not valid HTML. You likely meant 'date' for a date picker. Please change type="data" to type="date".
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! Before this can be approved, there are still some critical issues that need to be addressed. Most importantly, your server logic currently overwrites the entire expense.json file with each new expense, instead of maintaining a list of expenses as required. Please update your code so that each new expense is appended to an array in the JSON file. Additionally, make sure the error message for GET requests references the correct endpoint (/add-expense), and ensure the date input field uses name="date" to match what the server expects. Once these issues are fixed, your solution will fully meet the requirements. Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return sendResponse( | ||
| res, | ||
| 400, | ||
| 'GET requests are not allowed on /submit-expense', |
There was a problem hiding this comment.
The error message here says 'GET requests are not allowed on /submit-expense', but the correct endpoint is '/add-expense'. This could confuse users and should be updated to match the actual route.
| const writeStream = createWriteStream(jsonPath); | ||
|
|
||
| writeStream.on('error', () => { | ||
| return sendResponse(res, 500, 'Error saving expense data'); | ||
| }); | ||
|
|
||
| writeStream.write(JSON.stringify(fields), () => { | ||
| writeStream.end(); | ||
| res.writeHead(200, { 'content-type': 'application/json' }); | ||
| res.end(JSON.stringify(fields)); | ||
| }); |
There was a problem hiding this comment.
Critical issue: This logic overwrites the entire 'expense.json' file with only the most recent expense. According to the requirements, you should maintain a list of expenses (i.e., append the new expense to an array in the JSON file, not replace it). Please update this section to read the existing expenses, add the new one, and then write the updated array back to the file.
| <h1>Form Data</h1> | ||
| <form action="/add-expense" method="post"> | ||
| <label for="data">Data:</label> | ||
| <input type="date" id="data" name="data"><br> |
There was a problem hiding this comment.
The 'name' attribute for the date input is set to 'data', but your server expects a field named 'date'. Change 'name="data"' to 'name="date"' to ensure the server receives the correct field.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job! 🎉 Your solution now fully meets all the core requirements and constraints for the task. The HTML form is correctly structured, the server processes and appends new expenses to an array in db/expense.json as required, and the response is a well-formatted HTML page displaying the saved data as JSON. All previous critical issues—such as overwriting the JSON file instead of appending and referencing the wrong endpoint in error messages—have been addressed.
Please remember, if you encounter similar issues in the future (like overwriting files instead of appending or incorrect endpoint references), it's important to carefully review the requirements and test your endpoints thoroughly. These are common pitfalls, and learning to spot them will help you become a stronger developer.
You are encouraged to review the comments and feedback in detail to further improve your code quality and understanding. Keep up the great work, and continue learning from each review! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return sendResponse( | ||
| res, | ||
| 400, | ||
| 'GET requests are not allowed on /submit-expense', |
There was a problem hiding this comment.
The error message references /submit-expense, but the correct endpoint is /add-expense. Please update the message to reference /add-expense as required by the task.
| const jsonPath = path.resolve(__dirname, '..', 'db', 'expense.json'); | ||
|
|
||
| try { | ||
| fs.writeFileSync(jsonPath, JSON.stringify(fields, null, 2)); |
There was a problem hiding this comment.
This line overwrites the entire expense.json file with the new expense object. According to the requirements, you should append each new expense to an array in the JSON file, not overwrite it. Please update this logic to read the existing array, add the new expense, and then write the updated array back to the file.
2 participants
No description provided.