Notificações: Atualizações do Backend #1701

mthmcalixto · 2024-05-16T16:14:19Z

Mudanças realizadas

Adiciona novas páginas/api e lógicas relacionadas ao backend para o funcionamento do feat:notifications.

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.

vercel · 2024-05-16T16:14:22Z

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

A member of the Team first needs to authorize it.

aprendendofelipe

@mthmcalixto, obrigado pelo PR! 💪

Não cheguei a revisar tudo porque acredito que o que já comentei no código já vai lhe dar bastante coisa para adequar, e vai exigir mudanças inclusive nas partes que eu ainda não revisei.

Algo que não comentei no código, mas que acho importante repetir o que já tinha comentado em #1692, é que não precisamos de tantos endpoints. Provavelmente um /api/v1/notifications com os métodos GET, PATCH e DELETE deve ser suficiente.

infra/migrations/1714326942609_user-notifications.js

aprendendofelipe · 2024-05-16T21:21:32Z

infra/migrations/1714326942609_user-notifications.js

+      type: 'varchar(254)',
+      notNull: true,
+    },
+    body_reply_line: {


Isso não precisa ser armazenado

@aprendendofelipe se não armazenar o body, precisa ter algo para armazenar o "title" do contéudo respondido.

Já tem o "type" então, mas precisa ter o username também e o title, o username de quem respondeu.

@mthmcalixto acho que deve ser possível montar a mensagem da notificação a partir dos dados salvos na tabela.

Então, por exemplo, vamos supor que foi criado um comentário. Se armazenarmos o id do evento, como mencionado em aqui, conseguimos acesso ao comentário através de events.metadata.id. A partir disso, é possível conseguir o título da publicação e o nome de quem comentou.

WITH content_originator AS ( SELECT path[1] as root_id, owner_id FROM events INNER JOIN contents ON contents.id::text = events.metadata->>'id' WHERE events.id = 'e4dfddb9-d16b-424d-b5b6-d512747b1ee3' ) SELECT users.username, contents.title FROM contents INNER JOIN content_originator ON true INNER JOIN users ON users.id = content_originator.owner_id WHERE contents.id = content_originator.root_id

E a mensagem poderia ser montada: ${username} comentou na sua publicação ${title}.

Como comentei em um PR meu (#1638 (comment)), não sei se teremos problemas de desempenho com essas queries buscando pelo metadata numa tabela com tantos registros. Eu precisaria ler mais sobre isso e talvez seja necessário criar um índice. Se alguém tiver mais conhecimento sobre isso, pode me corrigir..

@Rafatcb é justamente isso que queria evitar, passando apenas o ID iria precisa buscar em outro lugar o nome e o titulo, mas ao fazer igual o email, já retorna os dados prontos e salva na coluna username e o titulo da publicação.

aprendendofelipe · 2024-05-16T21:23:55Z

infra/migrations/1714326942609_user-notifications.js

+      type: 'varchar',
+      notNull: true,
+    },
+    content_link: {


Melhor armazenar o originador da notificação de uma maneira mais flexível, que suporte outros tipos de notificações. Talvez armazenar o id do evento, mas é preciso pensar melhor sobre isso

@aprendendofelipe não consigo imaginar algo sem o "sender" de onde partiu aquela notificação.

podemos mudar "from_id" para "sender_id"

Podemos criar algo assim também:

Tipo de Notificação: Alerta Evento: Novo Comentário Mensagem: "Você recebeu um novo comentário em sua postagem. Verifique agora!"

Tipo de Notificação: Lembrete Evento: Aniversário do Usuário Mensagem: "Lembrete: Hoje é seu aniversário! Celebre seu dia especial conosco."

Aqui já iria ter o "sender_id", ele pode se opcional nesse caso.

Tipo de Notificação: Social Evento: Mencionado em um Comentário Mensagem: "@NomeDoUsuario mencionou você em um comentário. Confira agora!"

Podemos fazer com que sender_id seja um array também, assim podemos colocar multiplos usuarios de uma vez só, algo com o @Rafatcb falou.

x usuários comentou em sua publicação...

Tenho a impressão de que, para realizar o agrupamento de notificações, o ideal é agrupar na hora da consulta (precisaria pensar se isso é possível) ou no backend, e não salvar agrupado no banco de dados.

@Rafatcb

Estava pensando em algo assim;

aprendendofelipe · 2024-05-16T21:29:26Z

models/notification.js

@@ -2,10 +2,11 @@ import email from 'infra/email.js';
 import webserver from 'infra/webserver.js';
 import authorization from 'models/authorization.js';
 import content from 'models/content.js';
+import { NotificationWeb } from 'models/notifications';


Não é interessante ter um models/notifications/index.js ao mesmo tempo que um models/notifications.js.

Note que aqui está importando models/notifications de dentro de models/notifications.js, algo bem estranho.

Não sei se faz sentido essa pasta /web e um arquivo manage.js. Seria bom uma organização mais clara de models/notifications.

@aprendendofelipe não existe um models/notifications.js, existe um models/notification.js que é responsavel pelo envio do email.

Minha ideia em notifications era separar notifications/web, notifications/email.

o from 'models/notifications' é justamente a pasta de notifications.

@aprendendofelipe já que estamos mexendo aqui, podemos mexer no email, mover ele pra dentro de models/notifications

models/notification.js

aprendendofelipe · 2024-05-16T21:42:16Z

models/authorization.js

@@ -91,6 +91,7 @@ function filterInput(user, feature, input, target) {
      password: input.password,
      description: input.description,
      notifications: input.notifications,
+      notifications_id: input.notifications_id,


Não entendi a necessidade de notifications_id aqui e mais abaixo

@aprendendofelipe era para permissão de saida, mais ainda não entendo muito o filterOutput.

Esse é o filterInput e você está falando do filterOutput, então fiquei confuso. Sobre a saída, em um endpoint exclusivo para obter notificações, acredito que você possa fazer algo parecido com read:user:self:

tabnews.com.br/models/authorization.js

Lines 194 to 209 in 74ca378

if (feature === 'read:user:self') {

if (user.id && output.id && user.id === output.id) {

filteredOutputValues = {

id: output.id,

username: output.username,

email: output.email,

description: output.description,

notifications: output.notifications,

features: output.features,

tabcoins: output.tabcoins,

tabcash: output.tabcash,

created_at: output.created_at,

updated_at: output.updated_at,

};

}

}

Ou seja, você compara o id do usuário notificado com o id do usuário que fez a requisição.

Na prática, isso é só uma segunda barreira de segurança, já que você obtém o id do usuário pelo context.

mthmcalixto · 2024-05-17T22:43:16Z

@mthmcalixto, obrigado pelo PR! 💪

Não cheguei a revisar tudo porque acredito que o que já comentei no código já vai lhe dar bastante coisa para adequar, e vai exigir mudanças inclusive nas partes que eu ainda não revisei.

Algo que não comentei no código, mas que acho importante repetir o que já tinha comentado em #1692, é que não precisamos de tantos endpoints. Provavelmente um /api/v1/notifications com os métodos GET, PATCH e DELETE deve ser suficiente.

Precisa pensar em um estrutura para /api/v1/notifications.

/api/v1/notifications/web - GET - Retorna todas as notificações.
/api/v1/notifications/web/{id} - GET - Retorna uma notificação específica com o ID fornecido.
/api/v1/notifications/web - DELETE - Arquiva todas as notificações.
/api/v1/notifications/web/{id} - DELETE - Arquiva uma notificação específica com o ID fornecido.
/api/v1/notifications/web - PATCH - Atualiza todas as notificações.
/api/v1/notifications/web/{id} - PATCH - Atualiza uma notificação específica com o ID fornecido.

Talvez não deletar tudo, apenas colocar um status "delete" e um "delete_at", caso o usuário queira recuperar essas notificações.

Rafatcb

Obrigado por mais um PR!

Deixei apenas alguns comentários, já que ainda estamos discutindo sobre a estrutura da tabela, o que consequentemente mudará a implementação.

Houve uma discussão em algum lugar sobre o motivo de usar DELETE (hard delete) na tabela de notificações? Me parece um ponto positivo mantermos o histórico, como mencionei em #738 (comment).

infra/migrations/1714326942609_user-notifications.js

Rafatcb · 2024-05-19T00:44:24Z

models/authorization.js

@@ -91,6 +91,7 @@ function filterInput(user, feature, input, target) {
      password: input.password,
      description: input.description,
      notifications: input.notifications,
+      notifications_id: input.notifications_id,


Esse é o filterInput e você está falando do filterOutput, então fiquei confuso. Sobre a saída, em um endpoint exclusivo para obter notificações, acredito que você possa fazer algo parecido com read:user:self:

tabnews.com.br/models/authorization.js

Lines 194 to 209 in 74ca378

if (feature === 'read:user:self') {

if (user.id && output.id && user.id === output.id) {

filteredOutputValues = {

id: output.id,

username: output.username,

email: output.email,

description: output.description,

notifications: output.notifications,

features: output.features,

tabcoins: output.tabcoins,

tabcash: output.tabcash,

created_at: output.created_at,

updated_at: output.updated_at,

};

}

}

Ou seja, você compara o id do usuário notificado com o id do usuário que fez a requisição.

Na prática, isso é só uma segunda barreira de segurança, já que você obtém o id do usuário pelo context.

models/validator.js

models/notification.js

models/notifications/web/index.js

models/notification.js

mthmcalixto · 2024-05-20T21:48:16Z

@Rafatcb verifique os novos códigos, refiz algumas coisas.

Os endpoints:

/api/v1/notifications/web - GET - Retorna todas as notificações.
/api/v1/notifications/web/{id} - GET - Retorna uma notificação específica com o ID fornecido.
/api/v1/notifications/web - DELETE - Arquiva todas as notificações.
/api/v1/notifications/web/{id} - DELETE - Arquiva uma notificação específica com o ID fornecido.
/api/v1/notifications/web - PATCH - Atualiza todas as notificações.
/api/v1/notifications/web/{id} - PATCH - Atualiza uma notificação específica com o ID fornecido.

Delete não deleta, apenas coloca status draft

O que mais arrumei foi em models\notification.js

Deixei sender_id caso mais pra frente a gente precise pegar o username do usuário que invocou aquela notificação, mas ele não é salvo no banco de dados.

- new endpoints - refactor models/notification.js - refactor models/notifications/web

mthmcalixto · 2024-05-23T17:54:06Z

O que falta para terminar as atualizações desse backend?

Rafatcb

Fiz mais comentários no código. Sugiro criar testes também para facilitar o entendimento de cada rota.

Nos comentários, questionei a utilidade de termos GET /api/v1/notifications/web/[id]. Acredito que ela possa ser removida.

Rafatcb · 2024-05-23T22:59:19Z

infra/migrations/1714326942609_user-notifications.js

+    status: {
+      type: 'varchar',
+      default: 'unread',
+      notNull: true,
+      check: "status IN ('unread', 'read', 'draft')",
+    },


Considerando que as notificações poderão ser apagadas do banco de dados, eu tiraria o status de "soft delete". Como sobraria apenas o status unread e read, o que acham de substituir a coluna status por uma coluna read do tipo boolean?

Rafatcb · 2024-05-23T23:26:36Z

models/notification.js

  sendReplyEmailToParentUser,
+  sendReplyWebToParentUser,


Tenho a impressão que o que fará mais sentido para quem chama o modelo notification será notificar sem saber a forma da notificação. Por exemplo, expotar uma função onContentReply (ou algo parecido), e essa função envia a notificação por e-mail e web, ou nenhuma (a depender da configuração).

Acredito que esse é o melhor caminho porque assim centralizamos a responsabilidade de como determinada ação é notificada, inclusive facilitando quando futuramente termos configurações separadas para habilitar/desabilitar notificações por e-mail e web, ou quando termos mais tipos de notificação (o PR #1638 já cria alguns). O que vocês acham?

models/notifications/web/index.js

models/notifications/web/queries.js

Rafatcb · 2024-05-23T23:55:39Z

pages/api/v1/notifications/web/[id]/index.public.js

+    });
+  }
+
+  if (notificationFound.status === 'read') {


Acredito que não precise retornar erro caso já esteja lida, apenas ignorar, retornando sucesso para o cliente.

Imagine uma conexão lenta onde o usuário marca a notificação como lida duas vezes, da forma como sugeri, o código do cliente não precisará tratar o erro nesse cenário, já que a ação desejada foi realizada (marcar a notificação como lida).

@Rafatcb pensei em algo assim;

Dê uma olhada na minha sugestão aqui. Se aceitarem, não precisará dessa verificação notificationFound.status === 'read'.

pages/api/v1/notifications/web/[id]/index.public.js

Rafatcb · 2024-05-23T23:59:55Z

pages/api/v1/notifications/web/[id]/index.public.js

+async function deleteHandler(request, response) {
+  const userTryingToDelete = request.context.user;
+
+  const notificationsFound = await webNotifyQueries.findAllCountByUserId(userTryingToDelete.id);


Tem algum motivo para realizar essa busca antes de buscar diretamente a notificação especificada pelo query.id?

@Rafatcb é apenas para verificar se exister, não ir direto excluindo, pode acontecer que ele nao tenha, mas ele acesse o endpoint e force que alguma notificação seja excluida no banco de dados, antes disso acontecer, eu estou verificando se ela existe.

pode acontecer que ele nao tenha, mas ele acesse o endpoint e force que alguma notificação seja excluida no banco de dados

Acho que isso não é um problema. Se não ter nada para ser atualizado ou removido, o update ou delete no banco de dados apenas não terá efeito.

Rafatcb · 2024-05-24T00:03:40Z

pages/api/v1/notifications/web/index.public.js

+  .use(authentication.injectAnonymousOrUser)
+  .use(controller.logRequest)
+  .use(cacheControl.noCache)
+  .get(authorization.canRequest('update:user'), getHandler)


Tenho a impressão que aqui deveria ser algo parecido como em GET /api/v1/user:

tabnews.com.br/pages/api/v1/user/index.public.js

Line 19 in f7fbb88

.get(authorization.canRequest('read:session'), renewSessionIfNecessary, getHandler);

Rafatcb · 2024-05-24T00:05:32Z

pages/api/v1/notifications/web/index.public.js

+  const notificationsFound = await webNotifyQueries.findAllByUserId(userTryingToPatch.id);
+
+  if (!notificationsFound || notificationsFound.length === 0) {
+    throw new NotFoundError({


Não sei se vão concordar comigo, mas acho que uma experiência melhor nesse endpoint de PATCH é atualizar tudo, sem retornar erro caso não existam notificações ou mesmo caso já estejam marcadas como lidas.

feat(notifications): backend updates

8311bd8

mthmcalixto mentioned this pull request May 16, 2024

Adicionar UI para notificações e API #1692

Closed

3 tasks

aprendendofelipe reviewed May 16, 2024

View reviewed changes

mthmcalixto requested a review from aprendendofelipe May 17, 2024 21:39

Rafatcb added back Envolve modificações no backend novo recurso Nova funcionalidade/recurso labels May 18, 2024

Rafatcb reviewed May 19, 2024

View reviewed changes

mthmcalixto force-pushed the notifications-backend branch from 7399dd8 to c6fad81 Compare May 20, 2024 21:45

mthmcalixto requested a review from Rafatcb May 20, 2024 21:46

mthmcalixto force-pushed the notifications-backend branch from c6fad81 to 3d49fd3 Compare May 20, 2024 21:49

refactor(notifications): queries and pages api

efcb6b0

- new endpoints - refactor models/notification.js - refactor models/notifications/web

mthmcalixto force-pushed the notifications-backend branch from 3d49fd3 to efcb6b0 Compare May 20, 2024 22:14

mthmcalixto mentioned this pull request May 22, 2024

Retornar resposta da criação de conteúdo independente do resultado da notificação #1630

Closed

aprendendofelipe marked this pull request as draft May 23, 2024 00:20

Rafatcb reviewed May 24, 2024

View reviewed changes

Rafatcb added the pendente Aguardando ação do autor do Pull Request label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notificações: Atualizações do Backend #1701

Notificações: Atualizações do Backend #1701

mthmcalixto commented May 16, 2024 •

edited

Loading

vercel bot commented May 16, 2024

aprendendofelipe left a comment

aprendendofelipe May 16, 2024

mthmcalixto May 17, 2024 •

edited

Loading

Rafatcb May 19, 2024

mthmcalixto May 19, 2024

aprendendofelipe May 16, 2024

mthmcalixto May 17, 2024 •

edited

Loading

mthmcalixto May 17, 2024

Rafatcb May 19, 2024

mthmcalixto May 20, 2024

aprendendofelipe May 16, 2024

mthmcalixto May 17, 2024 •

edited

Loading

mthmcalixto May 22, 2024

aprendendofelipe May 16, 2024

mthmcalixto May 17, 2024

Rafatcb May 19, 2024

mthmcalixto commented May 17, 2024 •

edited

Loading

Rafatcb left a comment

Rafatcb May 19, 2024

mthmcalixto commented May 20, 2024 •

edited

Loading

mthmcalixto commented May 23, 2024 •

edited

Loading

Rafatcb left a comment

Rafatcb May 23, 2024

Rafatcb May 23, 2024

Rafatcb May 23, 2024

mthmcalixto May 25, 2024

Rafatcb May 26, 2024

Rafatcb May 23, 2024

mthmcalixto May 25, 2024

Rafatcb May 26, 2024

Rafatcb May 24, 2024

Rafatcb May 24, 2024

	if (feature === 'read:user:self') {
	if (user.id && output.id && user.id === output.id) {
	filteredOutputValues = {
	id: output.id,
	username: output.username,
	email: output.email,
	description: output.description,
	notifications: output.notifications,
	features: output.features,
	tabcoins: output.tabcoins,
	tabcash: output.tabcash,
	created_at: output.created_at,
	updated_at: output.updated_at,
	};
	}
	}

Notificações: Atualizações do Backend #1701

Are you sure you want to change the base?

Notificações: Atualizações do Backend #1701

Conversation

mthmcalixto commented May 16, 2024 • edited Loading

Mudanças realizadas

Tipo de mudança

Checklist:

vercel bot commented May 16, 2024

aprendendofelipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto commented May 17, 2024 • edited Loading

Rafatcb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto commented May 20, 2024 • edited Loading

mthmcalixto commented May 23, 2024 • edited Loading

Rafatcb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthmcalixto commented May 16, 2024 •

edited

Loading

mthmcalixto May 17, 2024 •

edited

Loading

mthmcalixto May 17, 2024 •

edited

Loading

mthmcalixto May 17, 2024 •

edited

Loading

mthmcalixto commented May 17, 2024 •

edited

Loading

mthmcalixto commented May 20, 2024 •

edited

Loading

mthmcalixto commented May 23, 2024 •

edited

Loading