Skip to content
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

Pull request colossal #32

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Pull request colossal #32

wants to merge 18 commits into from

Conversation

jppaulo06
Copy link
Collaborator

@jppaulo06 jppaulo06 commented Feb 2, 2023

Algumas das mudanças (todas no back):

  • Altera a versão node da imagem do docker de 14 para 18, possibilitando alguns módulos
  • Adiciona access token para o sistema de autenticação a partir de jwt's. O secret deve estar guardado na .env
  • Adiciona sistema de autenticação
  • O sistema de armazenar senhas passa a utilizar o bcrypt
  • Adiciona várias rotas. Vale a pena dar uma brincada no postman para descobrir todas elas.
  • Para cada uma das rotas que insere informação na db, foi feita uma série de validadores.
  • Todo erro (ou quase todo) pode ser traduzido para o português facilmente identificando a linguagem do browser do usuário. Isso porque estão sempre linkados a um json que pode ser substituído facilmente (dar uma olhada em @/errors/errors_messages/index.js)
  • Para interagir com a db de modo mais ágio, foram criados vários helpers, sendo os quais de insert, remove, update e select (dar uma olhada em @/utils/queryHelper.js)

jppaulo06 and others added 18 commits January 24, 2023 20:06
de mensagem de erros em @/utils/error-messages, adiciona controller e
service de warning-classes, corrige alguns validators, refatora o codigo
em geral
…tantos erros assim (n teste tanto, mas fe). Ainda nao adiciona os selects com join
…difica Avisos.vue para passar a utiliza-lo. No back, modifica a rota de usuario para que suporte mudancas de linguagem. Isso eh algo em desenvolvimento, entao vale ressaltar que ainda nao funciona.
…orme a propriedade Accept-Language da header do request. Entretanto, ainda nao define Content-Language na response.
Signed-off-by: José Lucas Silva Mayer <[email protected]>
@josemayer
Copy link
Contributor

Pull-request espetacular, bixo...

A escolha de arquitetura que você usou para definir as classes e serviços ficou muito boa. É realmente muito fácil escalar o backend com esse formato.

Gostei demais do tratamento de erros também. Todos são bastante esclarecedores e os códigos estão bem definidos.

O uso de PATCH, PUT, DELETE, POST e GET estão bem adequados a uma REST API, com autorizações corretas e seguindo o sistema de autenticação.

No geral, tá muito bom mesmo! Parabénssss 😁

@josemayer
Copy link
Contributor

josemayer commented Aug 4, 2023

Alguns pontos que vale ressaltar, mas nada muito alarmante:

Coisas que eu corrigi até agora

  • No serviço de autenticação, você esqueceu de adicionar o .auth no import das mensagens de erro para se referir aos erros específicos de autenticação
  • No serviço de escala de horários de administradores, você adicionou .schedulea. Corrigi para .schedule, conforme o arquivo /backend/errors/error_messages/en-US.json

Coisas que não corrigi

  • Acredito que ter uma rota específica para os botões dos slides seja algo pouco eficiente para o frontend recuperar. Seria interessante se, no backend, adicionássemos os botões como um objeto-filho dos objetos de slide, da rota /slide (o frontend pode fazer uma única requisição, nesse caso)
  • Semanticamente falando, um usuário não deveria ter a permissão de se autoadicionar como administrador no registro. Acredito que isso tenha um propósito de debug no desenvolvimento (para facilitar e evitar de ficar atualizando as roles), mas, em produção, o backend não deve ser permissivo a isso.

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.

2 participants