Skip to content

Develop#164

Open
OlegMysko wants to merge 8 commits intomate-academy:masterfrom
OlegMysko:develop
Open

Develop#164
OlegMysko wants to merge 8 commits intomate-academy:masterfrom
OlegMysko:develop

Conversation

@OlegMysko
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Great job setting up the Node.js server and serving the initial HTML form! However, the core functionality for submitting and saving expense data has a few critical issues that prevent the application from working as required.

Here are the key areas to fix:

  1. Incomplete Form: The form in src/index.html is missing the required input field for amount.
  2. Mismatched URLs: The form's action attribute in src/index.html is set to /expense, but the server in src/createServer.js expects POST requests at /add-expense. These must match.
  3. Data Overwritten: Your server logic overwrites db/expense.json on each submission. You need to read the existing file content, add the new expense to the array, and then write the entire updated array back to the file.
  4. Incorrect Response: The server currently responds with raw JSON. The requirement is to return an HTML page that displays the well-formatted content of db/expense.json, for example by using <pre> tags.

Please address these points to meet the project's core requirements. You're on the right track!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

return;
}

if (req.method === 'POST' && req.url === '/add-expense') {

Choose a reason for hiding this comment

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

This route handler is set up for the path /add-expense, but the form in src/index.html is configured to submit data to a different URL (/expense). These paths need to match for the server to process the form submission.

return;
}

fs.writeFileSync('db/expense.json', JSON.stringify(obj, null, 2));

Choose a reason for hiding this comment

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

fs.writeFileSync overwrites the file every time a new expense is submitted. This means you are only ever storing the most recent expense, not a list of all expenses.

To meet the requirement of saving the data, you should:

  1. Read the contents of db/expense.json first.
  2. Parse the JSON content into an array (and handle cases where the file is empty or doesn't exist by starting with an empty array).
  3. Add the new expense object (obj) to this array.
  4. Write the entire updated array back to the file.

Comment on lines +44 to +45
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(obj));

Choose a reason for hiding this comment

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

The task requires returning an HTML page that displays the well-formatted JSON from the expense.json file.

Currently, your code returns a raw JSON response (application/json). To fix this, you should:

  1. Set the Content-Type header to text/html.
  2. After saving the data, read the full content of db/expense.json.
  3. Send an HTML string as the response body. A good way to display formatted JSON is to wrap it in <pre> tags, like this: res.end(
    ${fileContent}
    );

src/index.html Outdated
</head>
<body>
<h1>Add Expense</h1>
<form action="http://localhost:5701/expense" method="post">

Choose a reason for hiding this comment

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

The action attribute of the form is set to /expense, but the server endpoint in createServer.js is expecting POST requests at /add-expense. This needs to be updated for the form to submit to the correct endpoint.

src/index.html Outdated
Comment on lines 11 to 20
<label>
Title:
<input type="text" name="title" required />
</label>
<br />

<label>
Date:
<input type="date" name="date" required />
</label>

Choose a reason for hiding this comment

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

The task requires the form to collect date, title, and amount. You have inputs for title and date, but the input field for amount is missing.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Great progress on setting up the server and the HTML form! However, there are a few critical issues that prevent the application from working as required.

Here are the key things to fix:

  1. Data Overwriting: In src/createServer.js, the code is saving only the new expense to db/expense.json, which overwrites all previous entries. Make sure to write the entire updated array of expenses back to the file.
  2. Incorrect Response: The server is currently responding with raw JSON. The requirement is to return an HTML page (Content-Type: 'text/html') that displays the full, updated list of expenses, perhaps using a <pre> tag for nice formatting.
  3. Form Submission Error: There's a mismatch between how the form sends data and how the server expects to receive it. The form sends standard form data, but the server expects JSON. You'll need to add a script to src/index.html to intercept the form submission, format the data as JSON, and send it using the fetch() API.

Once these issues are addressed, the core functionality should be complete. Keep up the great work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example


arr.push(obj);

fs.writeFileSync('db/expense.json', JSON.stringify(obj, null, 2));

Choose a reason for hiding this comment

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

You are correctly reading the existing expenses and adding the new one to the arr variable. However, when writing back to the file, you're only saving the new obj. This overwrites all the previous data. According to the requirements, the entire updated array of expenses should be saved.

Consider which variable holds the complete list of expenses that should be written to the file.

Comment on lines +60 to +61
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(obj));

Choose a reason for hiding this comment

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

The task requires you to "return an HTML page with well formatted JSON". Currently, you're sending back raw JSON with a Content-Type of application/json and only the newly added expense.

To meet the requirements, you should:

  1. Set the Content-Type to 'text/html'.
  2. Send back an HTML string as the response.
  3. This HTML should display the entire updated array of expenses (arr), not just the single obj. A <pre> tag is a great way to format the JSON data inside the HTML.

src/index.html Outdated
</head>
<body>
<h1>Add Expense</h1>
<form action="http://localhost:5701/add-expense" method="post">

Choose a reason for hiding this comment

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

This form is configured for a standard submission, which sends data in a format like title=...&date=...&amount=... with a Content-Type of application/x-www-form-urlencoded.

Your server code in createServer.js, however, uses JSON.parse() on the incoming data, which means it's expecting a Content-Type of application/json and a body that is a JSON string.

To make this work, you should add a <script> tag to this file. In the script, you'll need to:

  1. Add an event listener to the form to prevent its default submission.
  2. Get the values from the input fields.
  3. Use the fetch() API to send a POST request to /add-expense with the data formatted as a JSON string in the request body.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I'm approving this submission to help you move forward, but there are a few important issues to address to get the application fully working.

Great job implementing the fetch API in src/index.html to handle form submissions! That's a major improvement.

Please focus on these key areas next:

  • Data Handling: In src/createServer.js, the server is overwriting expense.json on each submission. Remember to read the existing array from the file, add the new expense to it, and then write the entire array back.
  • Server Response: The POST request handler should respond with an HTML page (Content-Type: 'text/html') that displays all expenses, not raw JSON.
  • URL Mismatch: The fetch URLs in src/index.html for both getting and submitting expenses do not match the routes defined in src/createServer.js. Make sure these are identical for the requests to succeed.

Addressing these points will make your project fully functional. You're making great progress!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

}


if (req.method === 'POST' && req.url === '/add-expense') {

Choose a reason for hiding this comment

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

There's a mismatch between the endpoint you're listening to here (/add-expense) and the one the client-side code is sending its request to. In src/index.html, the fetch call is made to /submit-expense.

For the form submission to work, these two URLs must be identical.

}


fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2));

Choose a reason for hiding this comment

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

This is the same issue from the previous review. This line overwrites expense.json with only the single new expense object, deleting all previous entries. The goal is to maintain a list (an array) of all expenses.

To fix this, you need to:

  1. Read the existing content from DATA_PATH.
  2. If the file has content, parse the JSON into a JavaScript array. If the file is empty or doesn't exist, start with a new empty array ([]).
  3. Add the new expense object (obj) to this array (e.g., using .push()).
  4. Convert the entire updated array back into a JSON string.
  5. Write this new JSON string to the file, overwriting the old content with the complete list.

Comment on lines +49 to +50
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(JSON.stringify(obj));

Choose a reason for hiding this comment

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

This response does not meet the task requirements, which was also mentioned in the previous review. The task asks you to "return an HTML page with well formatted JSON".

Currently, you are returning raw JSON data (Content-Type: 'application/json') containing only the newly added expense.

Instead, you should:

  1. Set the Content-Type header to 'text/html'.
  2. Read the full, updated content of expense.json (which should be an array of all expenses).
  3. Create an HTML string that displays this data, perhaps inside a <pre> tag for nice formatting.
  4. Send this HTML string as the response body.

const data = fs.existsSync(DATA_PATH)
? fs.readFileSync(DATA_PATH, 'utf-8')
: '';
const obj = data ? JSON.parse(data) : {};

Choose a reason for hiding this comment

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

When the expense.json file is empty or doesn't exist, you are initializing obj as an empty object ({}). Since the data structure for expenses should be an array, it would be more consistent and less error-prone to default to an empty array ([]) here.


async function loadExpenses() {
try {
const res = await fetch('/db/expense.json');

Choose a reason for hiding this comment

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

This fetch call is attempting to load the JSON file directly. While this might work in some server setups, your Node.js server has a specific API endpoint for this.

Based on src/createServer.js, the correct endpoint to get all expenses is /expenses. You should update this URL to match the one on the server.

amount: form.amount.value
};

await fetch('/submit-expense', {

Choose a reason for hiding this comment

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

The URL for the POST request here is /submit-expense, but the server is listening for requests on /add-expense (as defined in src/createServer.js).

To fix this, make sure the URL in this fetch call exactly matches the route you've set up in your server for adding a new expense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants