Skip to content
Open

v1 #510

Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions .github/workflows/test.yml-template
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
name: Test

on:
pull_request:
branches: [master]

jobs:
build:
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [20.x]
services:
postgres:
image: postgres:latest
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: password
POSTGRES_DB: students
POSTGRES_PORT: 5432
POSTGRES_HOST: localhost
ports:
- 5432:5432
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
steps:
- uses: actions/checkout@v2
- name: Set up Node.js
uses: actions/setup-node@v1
with:
node-version: '20'
- name: Install dependencies
run: npm install
- name: Run tests
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: password
POSTGRES_DB: students
POSTGRES_HOST: localhost
POSTGRES_PORT: 5432
run: npm test
4 changes: 0 additions & 4 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,3 @@ node_modules

# MacOS
.DS_Store

# env files
*.env
.env*
9 changes: 5 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
},
"devDependencies": {
"@mate-academy/eslint-config": "latest",
"@mate-academy/scripts": "^1.8.6",
"@mate-academy/scripts": "^2.1.3",
"axios": "^1.7.2",
"eslint": "^8.57.0",
"eslint-plugin-jest": "^28.6.0",
Expand Down
81 changes: 81 additions & 0 deletions src/controllers/categories.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
const categoriesService = require('../services/categories.service');

const get = () => async (req, res) => {
res.json(await categoriesService.getAll());
};

const getOne = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const reqId = await categoriesService.getById(id);

if (!reqId) {
return res.status(404).send('Not Found');
}

res.json(reqId);
};

const create = () => async (req, res) => {
const { name } = req.body;

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

const user = await categoriesService.create(name);

return res.status(201).json(user);
Comment on lines +30 to +32
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +30 to +32
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

};

const remove = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const deleted = await categoriesService.remove(id);

if (!deleted) {
return res.status(404).send('Not Found');
}

res.status(204).send();
};

const update = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const { name } = req.body;

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

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);
Comment on lines +64 to +72
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +64 to +72
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

};

module.exports = {
get,
getOne,
create,
remove,
update,
};
188 changes: 188 additions & 0 deletions src/controllers/expenses.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
const { Expense } = require('../models/Expense.model');
const { User } = require('../models/User.model');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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


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

const get = () => async (req, res) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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) => { ... }.

res.json(await expensesService.getAll(req.query));
};

const getOne = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const reqId = await expensesService.getById(id);

if (!reqId) {
return res.status(404).send('Not Found');
}

res.json(reqId);
};

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

const parsedUserId = Number(userId);
const parsedAmount = Number(amount);

if (Number.isNaN(parsedUserId)) {
return res.status(400).send('Bad Request');
}

if (Number.isNaN(parsedAmount)) {
return res.status(400).send('Bad Request');
}

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

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

if (typeof note !== 'string') {
return res.status(400).send('Bad Request');
}

const userExists = await User.findByPk(parsedUserId);

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

Choose a reason for hiding this comment

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

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.


let spentAtValue;

if (spentAt !== undefined) {
const date = new Date(spentAt);

if (Number.isNaN(date.getTime())) {
return res.status(400).send('Bad Request');
}

spentAtValue = date.toISOString();
} else {
spentAtValue = new Date().toISOString();
}

const expense = await Expense.create({
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

userId: parsedUserId,
spentAt: spentAtValue,
title,
amount: parsedAmount,
category,
note,
});

return res.status(201).json(expense);
};

const update = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const { userId, title, spentAt, amount, category, note } = req.body;

const updates = {};

if (userId !== undefined) {
const parsedUserId = Number(userId);

if (Number.isNaN(parsedUserId)) {
return res.status(400).send('Bad Request');
}

const userExists = await User.findByPk(parsedUserId);

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

updates.userId = parsedUserId;
}

if (amount !== undefined) {
const parsedAmount = Number(amount);

if (Number.isNaN(parsedAmount)) {
return res.status(400).send('Bad Request');
}

updates.amount = amount;
}

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

updates.title = title;
}

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

Choose a reason for hiding this comment

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

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.


updates.category = category;
}

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

updates.note = note;
}

if (spentAt !== undefined) {
const date = new Date(spentAt);

if (Number.isNaN(date.getTime())) {
return res.status(400).send('Bad Request');
}

updates.spentAt = date.toISOString();
}

const expense = await expensesService.getById(id);

if (!expense) {
return res.status(404).send('Not Found');
}

await Expense.update(updates, { where: { id } });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similar to the create operation, this database update logic should be moved to the expenses.service.js to maintain a consistent architecture and keep your controllers thin.


return res.status(200).json(expense);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The expense object being returned here was fetched before the update on line 163. This means the client will receive the old data. To return the updated object, you should re-fetch it from the database after the update operation.

};

const remove = () => async (req, res) => {
const id = Number(req.params.id);

if (Number.isNaN(id)) {
return res.status(400).send('Bad Request');
}

const deleted = await expensesService.remove(id);

if (!deleted) {
return res.status(404).send('Not Found');
}

res.status(204).send();
};

module.exports = {
get,
getOne,
create,
remove,
update,
};
Loading
Loading