v1 by wzfxc · Pull Request #510 · mate-academy/node_accounting-app-with-db

wzfxc · 2026-04-02T16:11:05Z

No description provided.

mateacademy-ai-mentor

Review Summary

This submission requires changes as it's missing the main feature of the task: implementing CRUD functionality for categories. The current code appears to be the solution from the previous lesson, focusing only on users and expenses.

To meet the requirements, you'll need to create the full stack for categories:

A new Category model.
A categories.service.js to handle database interactions.
A categories.controller.js for the request/response logic.
A categories.route.js to define the API endpoints.

Once those are in place, remember to register the new category routes in src/createServer.js and sync the new model in src/setup.js. The existing users.service.js is a great example to follow for structuring your new category service.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-02T16:13:41Z

+
+  app.use('/users', userRouter(users));
+
+  app.use('/expenses', expensesRouter(users, expenses));


The server is configured with routes for users and expenses, but the required route for categories is missing. You'll need to create and register a new router to handle category-related requests to fulfill the task requirements.

mateacademy-ai-mentor · 2026-04-02T16:13:41Z

@@ -0,0 +1,81 @@
+const usersService = require('../services/users.service');
+
+const get = () => async (req, res) => {


This controller method is defined as a higher-order function () => async (req, res) => {...}. However, in src/routes/users.route.js, it's called with a users argument that is unused since you've switched to a database service. This pattern is likely a remnant of an older in-memory storage implementation. Consider refactoring this to a simple async function async (req, res) => {...} and updating the route accordingly. This suggestion applies to all controller methods in this file for better code clarity.

mateacademy-ai-mentor · 2026-04-02T16:13:41Z

+const {
+  models: { Expense },
+} = require('../models/models');
+const { User } = require('../models/User.model');


For consistency with other files like users.service.js, it's better to import all models from the central ../models/models file.

mateacademy-ai-mentor · 2026-04-02T16:13:41Z

+
+const expensesService = require('../services/expenses.service');
+
+const get = () => async (req, res) => {


This higher-order function pattern seems to be a remnant from a previous implementation. Since services are now directly imported, you can simplify this and other controller methods to be simple async functions, e.g., const get = async (req, res) => { ... }.

mateacademy-ai-mentor · 2026-04-02T16:13:41Z

+    spentAtValue = new Date().toISOString();
+  }
+
+  const expense = await Expense.create({


The logic for creating an entity should be in the service layer. This separates the controller's responsibility (handling HTTP requests) from the service's responsibility (business logic and data access). Consider creating an expensesService.create() method.

mateacademy-ai-mentor · 2026-04-02T16:13:42Z

+    category: {
+      type: DataTypes.STRING,
+      allowNull: false,


For a full CRUD page to manage categories, consider creating a separate Category model and establishing a relationship (e.g., a foreign key) with the Expense model. This approach makes it easier to manage categories as independent entities, for instance, when you need to rename or delete one.

mateacademy-ai-mentor · 2026-04-02T16:13:42Z

+  getAll,
+  getById,
+  remove,


This service is missing the create and update methods. All database interactions and business logic for creating and updating expenses should be encapsulated within this service, rather than being handled directly in the controller. This is crucial for maintaining a clean and scalable architecture with a proper separation of concerns.

mateacademy-ai-mentor · 2026-04-02T16:13:42Z

+
+const express = require('express');
+
+module.exports = function (users, expenses) {


This factory function pattern, which accepts users and expenses arrays, is a remnant of the previous in-memory storage implementation. Since the application now uses a database and services to manage data, these parameters are no longer needed. You can refactor this module to export the router directly.

mateacademy-ai-mentor · 2026-04-02T16:13:42Z

+
+const express = require('express');
+
+module.exports = function (users) {


This router is defined as a factory function that takes a users array as an argument. However, since the application now uses a database service, this users argument is unused and is a leftover from a previous in-memory storage implementation. It's recommended to refactor this to export the router object directly and simplify the corresponding controller methods to no longer be higher-order functions. This will make the code cleaner and easier to understand.

mateacademy-ai-mentor · 2026-04-02T16:13:42Z

+  models: { User },
+} = require('../models/models');
+
+User.sync({ force: true });


To support the new category feature, you'll also need to sync the Category model here. This will ensure the categories table is created in the database when the setup script is run.

mateacademy-ai-mentor

Review Summary

Great job implementing the full CRUD functionality for categories! The new model, service, controller, and routes are well-structured and correctly integrated.

Before I can approve your work, there are two critical issues that need to be addressed:

Broken Setup Script: In src/setup.js, the import path for the User model is incorrect ('../models/models'). This prevents the database models from syncing. Please correct it to './models/models'.
Failing User Tests: The tests in tests/user.test.js are failing because they send a name field, but the User model expects firstName. Please update the test payloads to use firstName to match the model.

Once these two blockers are resolved, your project will be ready for approval. Keep up the great work!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+  const user = await categoriesService.create(name);
+
+  return res.status(201).json(user);


The variable user seems to be a leftover from copying code. Since you're creating a category here, a more descriptive name like newCategory would improve readability.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+  const userIdToUpdate = await categoriesService.getById(id);
+
+  if (!userIdToUpdate) {
+    return res.status(404).send('Not Found');
+  }
+
+  const updatedUser = await categoriesService.update({ id, name });
+
+  res.status(200).json(updatedUser);


The variable names userIdToUpdate and updatedUser seem to be from the users controller. To make the code clearer, consider renaming them to reflect that they are dealing with categories, for example, categoryToUpdate and updatedCategory.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+
+  if (!userExists) {
+    return res.status(400).send('Bad Request');
+  }


Now that you have a Category model, a great improvement here would be to validate that the provided category exists in the database. You can query the Category table to see if a category with the given name exists before creating the expense. This ensures better data integrity.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+  if (category !== undefined) {
+    if (typeof category !== 'string' || category.trim() === '') {
+      return res.status(400).send('Bad Request');
+    }


Similar to the create function, when updating an expense's category, it's a good practice to verify that the new category name exists in your categories table. This prevents assigning an invalid category to an expense.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+    category: {
+      type: DataTypes.STRING,
+      allowNull: false,


Storing the category as a string works, but it can lead to data integrity issues. For example, if you rename a category, you'd have to find and update every single expense that uses that category name.

A better approach is to create a relationship between the Expense and Category models. You could change this field to categoryId and make it an integer that references the id of a category. This is known as a foreign key relationship and is a fundamental concept in database design.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+
+const express = require('express');
+
+module.exports = function (users, expenses) {


It looks like the users and expenses parameters are leftovers from a previous version of the app that might have used in-memory data. Now that you're using a service layer to interact with the database, the controllers don't need these arguments passed to them. You could refactor this to match the cleaner approach you used in categories.route.js by removing the parameters from the function and the calls to the controller methods.

mateacademy-ai-mentor · 2026-04-04T11:41:38Z

+const {
+  models: { User },
+} = require('../models/models');


The import path for the User model seems incorrect. Since this file is located in the src directory, the path '../models/models' attempts to look for a models directory outside of src. To correctly locate the models' entry file, this path should likely be './models/models'.

mateacademy-ai-mentor

Review Summary

Excellent work implementing the full CRUD functionality for categories! The new model, service, controller, and routes are well-structured and follow best practices. I am approving this submission as an educational override to help you progress, but there are a few critical issues you should review before moving on.

The main problem is that several files are attempting to import models from a non-existent models.js file, which causes the application to crash. Please correct these paths to import directly from the specific model file, for example: const { User } = require('../models/User.model');. Additionally, the tests in tests/expense.test.js are failing because they create users with a name field instead of the required firstName. Fixing these two issues will get your application and tests running correctly. Keep up the fantastic effort!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-04T11:51:47Z

+  const user = await categoriesService.create(name);
+
+  return res.status(201).json(user);


Good work on the create function. For better readability, consider renaming the user variable to category since it holds the newly created category object. This will make the code's intent clearer.

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+  const userIdToUpdate = await categoriesService.getById(id);
+
+  if (!userIdToUpdate) {
+    return res.status(404).send('Not Found');
+  }
+
+  const updatedUser = await categoriesService.update({ id, name });
+
+  res.status(200).json(updatedUser);


The update logic is correct. To make the code even more self-explanatory, I suggest renaming userIdToUpdate to categoryToUpdate and updatedUser to updatedCategory. This aligns the variable names with the data they are handling in the categories controller.

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+const {
+  models: { Expense },
+} = require('../models/models');


This import will cause the application to fail because the file ../models/models.js doesn't seem to exist. Models should be imported directly from their definition files. Notice how the User model is imported correctly in this file—you should do the same for the Expense model.

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+const {
+  models: { Expense },
+} = require('../models/models');


This import will cause the application to fail because the file ../models/models.js does not exist. Models should be imported directly from their definition files. The correct way to import the Expense model here would be:

const { Expense } = require('../models/Expense.model');

Please apply this correction here and in all other files where models are imported this way.

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+module.exports = function (users, expenses) {
+  const router = express.Router();
+
+  router.get('/', expensesController.get(expenses));
+
+  router.get('/:id', expensesController.getOne(expenses));
+
+  router.post('/', expensesController.create(users, expenses));
+
+  router.patch('/:id', expensesController.update(users, expenses));
+
+  router.delete('/:id', expensesController.remove(expenses));


This function signature and the controller calls below still include the users and expenses parameters. Since the application now uses a service layer to handle data, these parameters are no longer needed here. You can simplify this by removing them from both the function definition and the controller calls (e.g., expensesController.get()).

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+module.exports = function (users) {
+  const router = express.Router();
+
+  router.get('/', usersController.get(users));
+
+  router.get('/:id', usersController.getOne(users));
+
+  router.post('/', usersController.create(users));
+
+  router.patch('/:id', usersController.update(users));
+
+  router.delete('/:id', usersController.remove(users));
+
+  return router;


This router module exports a function that accepts a users parameter, which is then passed to the controller methods. However, in createServer.js, this router is initialized without any arguments, making users undefined.

Furthermore, the controller functions (e.g., get, getOne) are now higher-order functions that don't expect any arguments. You should update this module to remove the users parameter from both the exported function and the calls to the controller methods (e.g., use usersController.get() instead of usersController.get(users)).

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+const {
+  models: { User },
+} = require('../models/models');


This import will cause the application to fail because the file ../models/models.js does not exist. Models should be imported directly from their definition files. The correct way to import the User model here would be:

const { User } = require('../models/User.model');

Please apply this correction here and in all other files where models are imported this way.

mateacademy-ai-mentor · 2026-04-04T11:51:48Z

+const {
+  models: { User },
+} = require('./models/models');


This import path for the User model is still incorrect and will cause the setup script to fail. My apologies for the confusion in the previous review. The User model should be imported directly from its own file, just like the Category model is. Please change this to import from './models/User.model.js'.

vadiimvooo

Good job, but you have to pass all tests. Feel free to ask for help in fe_chat.
Consider why the test "should create a new expense if optional fields is not provided" fails with a 400 error, even though the test name suggests category is optional—could the model or controller be requiring it as mandatory?
Look at the test "should return all expenses by category": what query parameter does it use for filtering, and does it match what the service expects?
In the update method of the controller: what is returned in the response—the old object before the update or the new one after? Why might this affect the test?

brespect

Well done! 👍

v1

1784244

mateacademy-ai-mentor suggested changes Apr 2, 2026

View reviewed changes

cleanup + category model

c2d6a21

wzfxc requested a review from mateacademy-ai-mentor April 4, 2026 11:37

mateacademy-ai-mentor suggested changes Apr 4, 2026

View reviewed changes

payload fix

2f68b15

wzfxc requested a review from mateacademy-ai-mentor April 4, 2026 11:49

mateacademy-ai-mentor suggested changes Apr 4, 2026

View reviewed changes

payload and imports

7f90669

wzfxc requested a review from mateacademy-ai-mentor April 4, 2026 12:02

vadiimvooo suggested changes Apr 4, 2026

View reviewed changes

tests passed

f02eb48

wzfxc requested a review from vadiimvooo April 5, 2026 12:45

brespect approved these changes Apr 6, 2026

View reviewed changes


		app.use('/users', userRouter(users));

		app.use('/expenses', expensesRouter(users, expenses));

		@@ -0,0 +1,81 @@
		const usersService = require('../services/users.service');

		const get = () => async (req, res) => {


		const expensesService = require('../services/expenses.service');

		const get = () => async (req, res) => {


		const express = require('express');

		module.exports = function (users, expenses) {


		const express = require('express');

		module.exports = function (users) {

		const user = await categoriesService.create(name);

		return res.status(201).json(user);

Conversation

wzfxc commented Apr 2, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadiimvooo left a comment

Choose a reason for hiding this comment

Uh oh!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development