Skip to content

app-with-db#522

Open
Vlshedevr wants to merge 6 commits intomate-academy:masterfrom
Vlshedevr:develop
Open

app-with-db#522
Vlshedevr wants to merge 6 commits intomate-academy:masterfrom
Vlshedevr:develop

Conversation

@Vlshedevr
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@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

Your implementation has critical blockers that prevent task completion. The core requirement — CRUD for categories — is completely missing; there is no Category model, service, controller, or routes anywhere in the codebase. Additionally, both services import from '../models/models' which doesn't exist, causing import failures at runtime. In db.js, dotenv.config() is called after destructuring environment variables, so .env values will be undefined. There are also typos: expenses.rotes.js should be expenses.routes.js and userRoter should be userRouter in createServer.js. You must create the Category CRUD infrastructure (model, service, controller, routes) and fix the import/dotenv issues before this can be approved.


✨ 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

Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js Outdated
Comment thread src/createServer.js Outdated
Comment thread src/controlers/user.controler.js
Comment thread src/controlers/user.controler.js
Comment thread src/models/User.model.js
Comment thread src/routes/user.routes.js
Comment thread src/routes/user.routes.js
Comment thread src/routes/user.routes.js
Comment thread src/routes/user.routes.js
Comment thread src/routes/user.routes.js
Comment thread src/models/Expense.model.js
Comment thread src/models/Expense.model.js
Comment on lines +14 to +17
userId: {
type: DataTypes.INTEGER,
allowNull: false,
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CRITICAL - Missing models index: Both expenses.services.js and user.services.js import from '../models/models', but this file doesn't exist. Create src/models/models.js that exports all models (User, Expense, and Category).

Comment thread src/models/Expense.model.js
Comment thread src/models/Expense.model.js
Comment thread src/services/user.services.js
Comment thread src/services/expenses.services.js
Comment on lines +1 to +2
const { Op } = require('sequelize');
const { models } = require('../models/models');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both services import from '../models/models' but this index file doesn't exist. Create src/models/models.js that exports all models: const { Expense } = require('./Expense.model'); const { User } = require('./User.model'); const { Category } = require('./Category.model'); module.exports = { models: { Expense, User, Category } };

Copy link
Copy Markdown

@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

Your Category CRUD infrastructure is well-structured, but there are critical runtime bugs that must be fixed. In categories.controller.js, line 42 calls .status() twice (res.status(400).status('Bad request')) which will hang requests - the second call should be .send() instead. In user.controller.js, you're passing id (string) to getOne() on line 47 instead of normalaizedId (number), which will cause the database lookup to fail. In createServer.js, there's a typo: userRoter should be userRouter - this will cause an import failure when the server starts. These are functional blockers that will prevent your API from working correctly. Minor issues to address separately: the filename expenses.rotes.js should be expenses.routes.js, and the route path /category should be /categories for consistency with your other REST endpoints. The test infrastructure has a separate foreign key constraint issue in teardown that isn't related to your code.


✨ 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

Comment thread src/controlers/categories.controler.js
Comment thread src/controlers/categories.controler.js
Comment thread src/controlers/categories.controler.js
Comment thread src/controlers/categories.controler.js
Comment thread src/controlers/categories.controler.js
Comment thread src/controlers/user.controler.js
Comment thread src/controlers/user.controler.js
Comment thread src/controlers/user.controler.js
Comment thread src/controlers/user.controler.js
Comment thread src/services/categories.services.js
Copy link
Copy Markdown

@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
Copy Markdown

@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

Your solution successfully implements all core Category CRUD functionality with PostgreSQL storage, and all previously identified HIGH-priority issues have been resolved. The codebase demonstrates solid understanding of Express routing, Sequelize models, and proper service/controller architecture.

Minor fixes needed:

  • expenses.controller.js line 71: Change 'Not fond' to 'Not found'
  • expenses.controller.js remove() function: Rename narmalaizedId to normalizedId for consistency with the rest of your codebase
  • expenses.services.js line 14: Rename newTodo to newUser for clarity

Note on test failures: The 17 test failures are due to a test setup issue in tests/user.test.js where the afterEach cleanup tries to truncate the users table without also truncating the related expenses table (foreign key constraint). Update the cleanup to truncate both tables or use TRUNCATE ... CASCADE.

Great work on getting the core functionality working correctly! These are minor polish issues that don't affect the application's behavior.


✨ 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

Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/controlers/expenses.controler.js
Comment thread src/services/expenses.services.js
Copy link
Copy Markdown

@Denys-Kravchuk9988 Denys-Kravchuk9988 left a comment

Choose a reason for hiding this comment

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

Good job!

Added 1 comment and please try to resolve comments from AI

Comment thread src/createServer.js Outdated

app.use(cors());

app.use('/users', express.json(), userRouter);
Copy link
Copy Markdown

@Denys-Kravchuk9988 Denys-Kravchuk9988 Apr 24, 2026

Choose a reason for hiding this comment

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

I would recommend to use app.use(express.json()); instead of duplicating in every endpoint. Also there is a sense to add additional improvement (not required but it's used in real projects)
app.use(express.json({ limit: '1mb' }));

Copy link
Copy Markdown

@brespect brespect left a comment

Choose a reason for hiding this comment

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

Good progress, but you need to pass all tests before requesting the review

@Vlshedevr Vlshedevr requested a review from brespect May 6, 2026 13:00
Copy link
Copy Markdown

@brespect brespect left a comment

Choose a reason for hiding this comment

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

Well done!

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.

4 participants