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

Adicionar UI para notificações e API #1692

Closed
wants to merge 1 commit into from

Conversation

mthmcalixto
Copy link
Contributor

@mthmcalixto mthmcalixto commented May 12, 2024

Mudanças realizadas

Todas as notificações do usuário, exclua, leia tudo, leia apenas uma notificação. A página /notificacoes exibe todas as notificações do usuário.

Também foi adicionado /api/notifications, por ela será atualizado os status e deletar notificações do usuário.

Precisou ser feito uma customPagination por causa do idioma que não era possível alterar, basicamente é o mesmo código da Table.Pagination, apenas com idioma alterado.

Gravando.2024-05-12.030953.mp4

Resolve #738

Tipo de mudança

[x] Nova funcionalidade

Checklist:

  • As modificações não geram novos logs de erro ou aviso (warning).
  • Eu adicionei testes que provam que a correção ou novo recurso funciona conforme esperado.
  • Os antigos estão passando localmente.

Copy link

vercel bot commented May 12, 2024

@mthmcalixto is attempting to deploy a commit to the TabNews Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

@mthmcalixto, muito obrigado pelo PR! Imagino quanto trabalho teve com ele. 💪💪💪

Vou deixar algumas sugestões:

  • Tente quebrar o PR em diferentes problemas menores que você possa resolver separadamente dos demais, e de forma desacoplada. No mínimo, dá para abrir um PR só cuidando da API, outro para o front, e também pode criar um PR só de refatorações, com as melhorias que você já fez aqui, mas que não tem relação direta com o PR.
  • Para adiantar a revisão, cite as decisões importantes que tomou, explicando porque escolheu determinado caminho e porque descartou outras possibilidades. Chegou a ver o comentário do @Rafatcb na Criar o sistema de notificação via API e interface web (em complemento ao email) #738?
  • Para facilitar refatorações futuras, evite colocar a extensão dos arquivos (.js) nas importações (mesmo que muito código antigo ainda esteja importando assim no TabNews).
  • Reveja a modelagem da nova tabela. Normalize os dados e pense em diferentes tipos de notificações.
  • Não acho que seja interessante usar o endpoint /user. Deve ser suficiente o /notifications com os métodos GET, PATCH e DELETE.
  • Não entendi a separação em infra/notification-web, models/notifications e models/manage-notifications. Se for o caso, pode transformar models/notifications em uma pasta com diferentes arquivos, mas exportando tudo de models/notifications/index.js.
  • Não vejo razão para o retry (nem gerar logs) na query em infra/notification-web. O infra/database que deve lidar com isso. Parece que você está lidando com essa gravação como se fosse um acesso a um serviço externo.
  • No front, não vejo necessidade de uma página de notificações, mas apenas um painel que esteja acessível em qualquer página.
  • Revise seu próprio código antes de submeter o novo PR (veja pages/interface/index.js).

@mthmcalixto
Copy link
Contributor Author

@aprendendofelipe Obrigado pelo retorno!!

Para adiantar a revisão, cite as decisões importantes que tomou, explicando porque escolheu determinado caminho e porque descartou outras possibilidades. Chegou a ver #738 (comment) do @Rafatcb na #738

É como se fosse um sistema do próprio tabnews, diretamente no banco de dados e sem custos por fora e foi tomado a decisão de fazer dessa forma por que as notificações não precisam ser em tempo real, basta ser algo como TabCoins, sempre que tiver uma atualiza, basta retornar os valores.

Sei que alguns outros frameworks por fora pode parecer melhor e mais fácil a manuntenção, mas no final vai gerar um custo adicional ao projeto.

Reveja a modelagem da nova tabela. Normalize os dados e pense em diferentes tipos de notificações.

Eu apenas dei o start mas pode pensar bastante sobre isso no futuro, adiciona novos tipos de mensagens etc... como mencionar, ganho de TabCoins.

No front, não vejo necessidade de uma página de notificações, mas apenas um painel que esteja acessível em qualquer página.

Apenas segue a lógica da maioria dos sites, o GitHub utiliza dessa forma em vez de um modal no header.

Não entendi a separação em infra/notification-web, models/notifications e models/manage-notifications. Se for o caso, pode transformar models/notifications em uma pasta com diferentes arquivos, mas exportando tudo de models/notifications/index.js.

Eu reaproveitei o sistema de notificação do email e fiz por cima dele, notifications é do email que já envia para os usuários, notification-web são funções que está sendo utilizada dentro de notifications e manage-notifications é a parte da API.

Não acho que seja interessante usar o endpoint /user. Deve ser suficiente o /notifications com os métodos GET, PATCH e DELETE.

Eu não quis fazer algo que vão acessar por fora, apenas interna e queria associar as notificações no login do usuário.

@mthmcalixto
Copy link
Contributor Author

mthmcalixto commented May 13, 2024

@aprendendofelipe

Fiz algumas mudanças.

Novas rotas, notificações estão separadas de user agora, tudo é feito pela API.

Rotas;

/api/v1/notifications/web/read -> PUT
/api/v1/notifications/web/read-all -> POST
/api/v1/notifications/web/delete-all -> DELETE
/api/v1/notifications/web/all -> GET

Foi criado o hook chamado useNotifications, com as seguintes funções e retornos;

notifications -> Retorna as notificações após chamar getAllNotifications()
isLoading -> Verificar se está fazendo o request
error -> erros
getAllNotifications -> Faz o fetch igual fetchUser
readNotification -> Faz o Post passando o id da notificação
readAllNotifications -> Ler todas as notificações
deleteAllNotifications -> Deleta todas as notificações

Alterei models anterior para models/notifications, ele contém;

notifications/web -> index.js - Adiciona os dados no banco de dados, relacionado a notificação; tipo, quem enviou, quem vai receber etc...
notifications/web -> manage.js - Mexe com todas as buscas, atualizações etc...

Foi removido o retry; sobre a página de notificações, eu repensei e precisa ter por causa dos usuários mobile, eles precisa ver algum tela de notificação.

@Rafatcb mencionou sobre enviar para duas pessoas, com essas modificações, o sistema de notificação web consegue enviar para multiplos usuários, basta passar o from_id e o to_id.

A página eu alterei também, assim mostrando mais informações;

image

All user notifications, delete, read all, read one.
/notifications page and /api/notifications api.
@mthmcalixto
Copy link
Contributor Author

@aprendendofelipe, abri dois;

Frontend: #1700
Backend: #1701

@aprendendofelipe
Copy link
Collaborator

Fechando pra gente focar no #1701

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pendente Aguardando ação do autor do Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Criar o sistema de notificação via API e interface web (em complemento ao email)
2 participants