task by Augustwise · Pull Request #518 · mate-academy/node_accounting-app-with-db

Augustwise · 2026-04-15T16:47:33Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because a core task requirement and some test-related behavior are not yet satisfied.

Critical issues that must be fixed

The task explicitly requires a CRUD page to manage categories, but there is no Category model, no categories router, and no /categories endpoints. In src/createServer.js, you only import and mount the users and expenses routers; you need to add something like const categoriesRouter = require('./routes/categories'); and app.use('/categories', categoriesRouter); so the categories CRUD API is actually exposed. In src/models/Expense.model.js, Expense stores category as a simple string; to support proper category management and relationships, introduce a Category model (e.g., with name), add a categoryId foreign key to Expense, and define the Sequelize associations (e.g., Category.hasMany(Expense) and Expense.belongsTo(Category)). In src/routes/expenses.js, update the request handlers to work with this new relation: accept/validate categoryId (or map a category name to an existing Category), use categoryId instead of raw category strings when creating/updating expenses, and update any filters that currently use where.category to use where.categoryId or a join on Category.
The Jest tests for users are failing because User.destroy({ truncate: true }) cannot run while expenses.userId is protected by a strict database-level foreign key. To align with the provided tests, you should adjust your schema or migration so that truncating users works cleanly (for example, remove or relax the DB-level FK constraint and keep the association at the Sequelize level, or otherwise ensure truncation can succeed without errors).

Once you add the Category model plus full CRUD routes, wire them in the server, update Expense to reference categories correctly, and resolve the test failure caused by the FK constraint, your solution will align much better with the project requirements. You already have solid CRUD implementations for users and expenses; extending the same structure to categories and refining the schema will complete the task nicely.

✨ 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-15T16:50:35Z

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


The task requires a CRUD page to manage categories. There is no categories router mounted here. Add and mount a categories router (for example app.use('/categories', categoriesRouter)) so the app exposes endpoints to create/read/update/delete categories.

mateacademy-ai-mentor · 2026-04-15T16:50:35Z

@@ -2,8 +2,40 @@

 const { sequelize } = require('../db.js');



In createServer.js you import users and expenses routers but there is no import for a categories router. The task requires implementing a CRUD page to manage categories — add a categories router (e.g. const categoriesRouter = require('./routes/categories')).

mateacademy-ai-mentor · 2026-04-15T16:50:35Z

+  {
+    userId: {
+      type: DataTypes.INTEGER,
+      allowNull: false,


You register /users and /expenses routes but you don't mount a /categories route. Mount the categories router (e.g. app.use('/categories', categoriesRouter)) so the CRUD endpoints are available as required by the task.

mateacademy-ai-mentor · 2026-04-15T16:50:35Z

+      type: DataTypes.FLOAT,
+      allowNull: false,
+    },
+    category: {


The Expense model stores category as a plain string. To properly implement a categories CRUD page you should add a dedicated Category model and relate Expense to it (e.g. categoryId FK and associations) or explicitly document that categories are managed as strings and implement persistence accordingly.

mateacademy-ai-mentor · 2026-04-15T16:50:35Z

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


There is no Category model or any association between Expense and a Category. The task requires CRUD management for categories — add a Category model/table and define associations (e.g. Category.hasMany(Expense) and Expense.belongsTo(Category) or add a categoryId FK on Expense).

mateacademy-ai-mentor · 2026-04-15T16:50:35Z

+
+const router = express.Router();
+
+function parseCategories(categories) {


parseCategories treats categories as strings which is fine for filtering free-form categories, but to meet the requirement you should persist categories and link expenses to them (so consider parsing incoming category IDs or names depending on chosen design).

mateacademy-ai-mentor · 2026-04-15T16:50:36Z

+  if (categoryList.length > 0) {
+    where.category = { [Op.in]: categoryList };


Filtering uses where.category = { [Op.in]: categoryList } which works for string categories; if you introduce a Category model change filters to use categoryId (and include necessary joins) or query by Category.name via joins.

mateacademy-ai-mentor · 2026-04-15T16:50:36Z

+      spentAt,
+      title,
+      amount,
+      category,


When creating an expense you accept category as a free string (Expense.create includes category). To support a categories CRUD, accept and validate a categoryId (or map provided category name to an existing Category) and persist a foreign key instead of a free string.

mateacademy-ai-mentor · 2026-04-15T16:50:36Z

+  if (category !== undefined) {
+    expense.category = category;


The update handler also assigns expense.category = category from request body. If you switch to a Category model, update this logic to validate and set categoryId (and handle removal/changes accordingly).

mateacademy-ai-mentor

Review Summary

I am requesting changes because the core requirement to link expenses to categories via a categoryId foreign key is not implemented, which means the Category model is not actually integrated into expense management. The Expense model (e.g., in src/models/Expense.model.js and the DB setup in src/db.js) still defines category as a STRING instead of an integer categoryId with allowNull: true, violating the requirement to reference Category rather than store raw names [CHECKLIST ITEM #5]. In src/models/models.js you import Category but never define the Sequelize associations Category.hasMany(Expense, { foreignKey: 'categoryId', constraints: false }) and Expense.belongsTo(Category, { foreignKey: 'categoryId', constraints: false }), so the ORM relation between expenses and categories does not exist. The expenses routes (src/routes/expenses.js and related helpers in src/routes/categories.js) still accept and filter by a category string, instead of accepting/validating categoryId (or mapping a provided name to an existing Category) and querying by where.categoryId or via an include: Category join [CHECKLIST ITEM #6]. You should also import Category where needed (e.g., in routes and user-related helpers) so you can validate that any provided categoryId actually exists before creating or updating an expense. When adding these associations, keep constraints: false (and avoid DB-level FKs) for userId and categoryId so test flows that truncate tables (like User.destroy({ truncate: true })) continue to work [CHECKLIST ITEM #7]. Once you replace the category string with categoryId, add the associations, and update all expense create/update/filter logic to use the relation, your solution will align well with the task requirements; the existing Category CRUD and router wiring are a solid foundation to build on.

✨ 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-15T17:20:12Z

+  host: POSTGRES_HOST || '20.160.160.166',
  dialect: 'postgres',
  port: POSTGRES_PORT || 5432,
-  password: POSTGRES_PASSWORD || '123',
+  password: POSTGRES_PASSWORD || 'strongpassword',


The model currently defines category as a STRING. The task requires using a dedicated Category model and storing a foreign key on Expense instead. Replace this category string field with a categoryId INTEGER (allowNull: true) and rely on associations to link Expense <> Category (see checklist item #5).

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+    category: {
+      type: DataTypes.STRING,
+      allowNull: true,
+    },


The Expense model currently stores category as a plain string. The task requires introducing a Category model and linking expenses to categories via a foreign key. Replace this category string field with a categoryId integer field (allowNull: true if categories are optional) and add Sequelize associations (Category.hasMany(Expense) and Expense.belongsTo(Category)) elsewhere (e.g., in your models index). Also update routes to accept/validate categoryId (or map a category name to an existing Category) when creating/updating expenses (checklist items #5 and #6).

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

@@ -1,7 +1,19 @@
 'use strict';

+const express = require('express');


Category model exists, but you must ensure associations between Category and Expense are defined (e.g., Category.hasMany(Expense, { foreignKey: 'categoryId', constraints: false }) and Expense.belongsTo(Category, { foreignKey: 'categoryId', constraints: false })) in your central models file. Also ensure DB-level FK constraints won't prevent test truncation (use constraints: false or avoid DB-level FK if tests truncate tables) — this ties to checklist items #4, #5 and #7.

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

 const { sequelize } = require('../db.js');

+const { DataTypes } = require('sequelize');
+


You are not importing the Category model here. Per the task you should import Category so route handlers can validate/resolve category references and/or include Category in queries (checklist items #5 and #6).

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+      type: DataTypes.STRING,
+      allowNull: false,
+    },
+  },
+  {
+    tableName: 'users',
+    timestamps: false,
+  },
 );

 module.exports = {


parseCategories treats categories as text values. If you switch to categoryId this parser should convert values to numbers (and validate them). If you keep name-based filtering, ensure you resolve names against the Category table before querying expenses.

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+  if (categoryList.length > 0) {
+    where.category = { [Op.in]: categoryList };


Filtering currently uses where.category = { [Op.in]: categoryList }. After switching to the relation you must filter by categoryId (e.g., where.categoryId = { [Op.in]: ... }) or perform a join on Category to filter by name. Update this logic to use the new relation (checklist item #6).

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+});
+
+router.post('/', async (req, res) => {
+  const { userId, spentAt, title, amount, category, note } = req.body;


Request handlers destructure category from the body. Update handlers to accept/validate categoryId (or accept a category name and map it to an existing Category record) before creating/updating expenses, per checklist item #6.

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+    const expense = await Expense.create({
+      userId,
+      spentAt,
+      title,
+      amount,
+      category,
+      note,


When creating an expense you pass the category string into Expense.create. Change this to persist categoryId (or resolve the category name to an id) so expenses reference Category rows instead of storing raw strings.

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+
+const updateHandler = async (req, res) => {
+  const { id } = req.params;
+  const { userId, spentAt, title, amount, category, note } = req.body;


Update handler still accepts category and assigns expense.category = category. Change this to accept/assign categoryId (or resolve a category name to an id) and save the FK on the expense.

mateacademy-ai-mentor · 2026-04-15T17:20:12Z

+  if (category !== undefined) {
+    expense.category = category;


This assignment updates the old category string. After schema change you must assign to expense.categoryId (or perform the mapping to an existing Category id) instead of expense.category.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can continue progressing while you address a few remaining issues. You’ve done a solid job moving to a numeric categoryId in the Expense model and wiring up CRUD routes for users, expenses, and categories using Sequelize and Postgres, which covers the core relational requirements well. The main blocking problem right now is that some expense tests fail because the update endpoint is not returning exactly what the tests expect (see tests/expense.test.js around line 304), so compare the expected object to your actual response and adjust the update handler to persist and return the fully updated expense record in the correct shape and status. Another important issue is that your create/update expense logic treats any provided categoryId (including null) as something to validate against Category.findByPk, which prevents clearing an expense’s category even though categoryId is allowed to be null; adjust the condition so null is treated as an intentional "no category" rather than an invalid id. In your GET filtering, both in src/routes/expenses.js and src/routes/categories.js where you map the categories query parameter to numbers before building where.categoryId, non-numeric category names are silently ignored, so either document that only numeric ids are supported or enhance the logic to resolve provided names to Category ids before filtering. As a smaller improvement, normalizing userId from the query string to a number before using it in where.userId will make filtering more robust and consistent with your integer schema. Overall, your structure and use of associations are on the right track; refining these edge cases and test expectations will help you reach a production-quality implementation.

✨ 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-15T17:41:38Z

+router.get('/', async (req, res) => {
+  const categories = await Category.findAll();
+


The categories filter is mapped to numbers and NaNs are removed, so non-numeric category names (strings) will be ignored. If you intend to support filtering by category name (allowed by the task as an option), map provided names to Category ids before building the where.categoryId filter; otherwise document that only numeric IDs are accepted.

brespect

Well done!

task

2263363

mateacademy-ai-mentor suggested changes Apr 15, 2026

View reviewed changes

fix

f09397f

Augustwise requested a review from mateacademy-ai-mentor April 15, 2026 17:17

mateacademy-ai-mentor suggested changes Apr 15, 2026

View reviewed changes

fix

b99e96c

Augustwise requested a review from mateacademy-ai-mentor April 15, 2026 17:39

mateacademy-ai-mentor suggested changes Apr 15, 2026

View reviewed changes

fix3

2128965

Augustwise requested a review from mateacademy-ai-mentor April 15, 2026 18:24

brespect approved these changes Apr 16, 2026

View reviewed changes

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


		const router = express.Router();

		function parseCategories(categories) {

		if (categoryList.length > 0) {
		where.category = { [Op.in]: categoryList };

		@@ -1,7 +1,19 @@
		'use strict';

		const express = require('express');

		const { sequelize } = require('../db.js');

		const { DataTypes } = require('sequelize');

		router.get('/', async (req, res) => {
		const categories = await Category.findAll();

Conversation

Augustwise commented Apr 15, 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!

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!

brespect left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants