-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat criação de post #8
base: dev
Are you sure you want to change the base?
The head ref may contain hidden characters: "feat-cria\u00E7\u00E3o-de-post"
Conversation
89d1e6a
to
598ce64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acredito que realizando as seguintes ações, esse PR pode ser aceito sem problemas:
- Gerar a data de criação automaticamente ao invés de recebê-la pela requisição do usuário;
- Realizar testes mais extensivos, tentando englobar mais casos, principalmente no que se refere à validação de parâmetros;
- Tornar as rotas mais legíveis, seguindo o que é estabelecido na indústria para APIs RESTful. (Um bom lugar para ter uma ideia sobre isso é aqui)
src/routes/routes.js
Outdated
|
||
const router = Router() | ||
|
||
router.post('/createpost', postController.createPost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não há necessidade de colocar o nome do método HTTP utilizado na rota. Uma melhor forma de escrever esse endpoint seria simplesmente /posts
, dado que o método HTTP POST
já implica na criação de um objeto novo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
createPost:async(req,res)=>{ | ||
// @ts-ignore | ||
let {user_id,title,body,date} = req.body | ||
if(!user_id||!title||!body){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essa condição é insuficiente para validar os dados, dado que não há uma verificação para user_id
negativos ou datas inválidas.
IDs inválidos podem ser filtrados usando um código como o abaixo:
if (user_id <= 0) {
// Devolver 400 para o usuário
}
Já no que se trata de datas inválidas, é possível verificar se ela é válida ao criar um objeto do tipo Date
com ela:
if (isNaN(new Date(date))) {
// Devolver 400 para o usuário
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boa noite, esse verificação da data invalida seria a data de criação do usuário?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A data de criação do post. Mas, como eu tava vendo aqui que o #2 especifica que a data é gerada automaticamente, é só não aceitar o campo date
pelo body da requisição e gerar ele automaticamente usando new Date()
.
src/test/post.test.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aqui nós temos que o describe
nos fala da rota /post
e as funções internas dele utilizam a rota /createpost
. Talvez modificar ambas para usar o /posts
recomendado no outro comentário seja o melhor para manter a consistência.
No mais, os testes de validação poderiam ter englobado alguns outros casos, como:
- Falta de todos os dados;
- Dados presentes, porém inválidos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/controllers/post.controller.js
Outdated
res.status(201).json({message:"201 : post criado com sucesso"}) | ||
} catch (error) { | ||
console.error(error) | ||
res.status(500).json({message:"500" + error}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nunca é interessante devolver a mensagem de erro diretamente para o usuário, pois é possível que ela contenha dados sensíveis do banco de dados. A mensagem aqui deve ser alguma coisa genérica, como Houve um erro ao inserir um novo post
, pois não resulta em nenhuma possível falha de segurança.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/controllers/post.controller.js
Outdated
// @ts-ignore | ||
createPost:async(req,res)=>{ | ||
// @ts-ignore | ||
let {user_id,title,body,date} = req.body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retirar a data de criação do corpo da requisição não atende o descrito em #2. Lembre-se que a data deve ser gerada automaticamente ao inserir um novo post, pelo uso da função new Date()
, por exemplo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eu reviso que tudo que foi revisado antes continua valendo kkk
eu pedi review de novo sem querer kkkkkk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
No futuro, só lembra de utilizar um arquivo .env
para puxar os dados de conexão com o banco de dados, acabou passando batido na primeira review que fiz. Isso é importante porque não dá para confiar em repositórios privados no GitHub para esconder essas informações, ainda mais com a recente descoberta da vulnerabilidade CFOR.
No description provided.