Reverter ganhos de TabCoins em conteúdos pegos pelo firewall interno e outros ajustes

[Rafatcb]

Mudanças realizadas

Reversão de ganhos de TabCoins

A função creditOrDebitTabCoins foi chamada para reverter possíveis ganhos de TabCoins que o usuário teve com os conteúdos publicados que foram pegos pelo firewall interno.

Endpoint

• POST /api/v1/moderations/review_firewall/[id]: confirmar ou desfazer um evento específico de firewall. A confirmação (confirm) adiciona a feature nuked ao usuário ou o status deleted aos conteúdos envolvidos. A reversão (undo) retorna os TabCoins removidos, restaura o status dos conteúdos e features dos usuários, a depender do tipo de firewall. Este endpoint retornará os dados atualizados contendo o evento original do firewall, o novo evento criado ao desfazer o firewall, os dados dos conteúdos e usuários afetados.

Resolve #1463

Tipo de mudança

• Melhorias no firewall interno
• 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.
• Tanto os novos testes quanto os antigos estão passando localmente.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
tabnews	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 30, 2024 0:39am

[aprendendofelipe]

Importante contribuição, @Rafatcb! 💪💪💪

Com a adição de undoContentsTabcoins, talvez seja melhor executar tudo dentro de uma transação, mas é preciso pensar melhor se isso faz sentido, pois, se der algum erro, não seria interessante reverter toda a transação.

Fiz comentários no código

[Rafatcb]

Com a adição de undoContentsTabcoins, talvez seja melhor executar tudo dentro de uma transação, mas é preciso pensar melhor se isso faz sentido, pois, se der algum erro, não seria interessante reverter toda a transação.

Acho que criar uma transação para cada iteração do for pode fazer sentido, assim garantimos que toda a atualização do saldo referente ao conteúdo X foi feita corretamente, sem impactar a atualização do saldo dos outros conteúdos ou a atualização do status. O que acha?

Edit: Vendo melhor, a condição em que entrará na função creditOrDebitTabCoins só tem uma query que modifica o banco, então a transação não teria impacto, apenas um try / catch que poderia prevenir algo, mas não sei se tem necessidade ali. Vou subir as alterações sem try / catch e sem transaction.

[aprendendofelipe]

@Rafatcb, aquele owner_id: context.user.id deixou visível que falta um teste que garanta que os conteúdos serão despublicados mesmo sendo de diferentes usuários. Já faltava um teste desse antes do PR.

---

E a questão de diferentes usuários me fez pensar na possível situação de usuários legítimos que publicarem algo pela mesma rede pública, ao mesmo tempo, e somente o terceiro saberá que alguma coisa errada aconteceu. Os dois primeiros não saberão o que aconteceu com suas publicações. Será que é interessante verificar se alguma das publicações é de um usuário diferente do terceiro e enviar uma notificação por email?

---

Um efeito colateral da implementação é que ficou mais complicado de reverter o efeito do firewall em casos de falso positivo. Antes, apesar de ter que fazer diretamente no banco, era só voltar as publicações de draft para published, mas agora seria necessário também desfazer o efeito de creditOrDebitTabCoins. Só que falta uma definição de como seria isso de forma a não causar inconsistências.

[Rafatcb]

aquele owner_id: context.user.id deixou visível que falta um teste que garanta que os conteúdos serão despublicados mesmo sendo de diferentes usuários. Já faltava um teste desse antes do PR.

Criarei o teste 👍

E a questão de diferentes usuários me fez pensar na possível situação de usuários legítimos que publicarem algo pela mesma rede pública, ao mesmo tempo, e somente o terceiro saberá que alguma coisa errada aconteceu. Os dois primeiros não saberão o que aconteceu com suas publicações. Será que é interessante verificar se alguma das publicações é de um usuário diferente do terceiro e enviar uma notificação por email?

Acho que isso pode ser interessante sim.

Um efeito colateral da implementação é que ficou mais complicado de reverter o efeito do firewall em casos de falso positivo. Antes, apesar de ter que fazer diretamente no banco, era só voltar as publicações de draft para published, mas agora seria necessário também desfazer o efeito de creditOrDebitTabCoins. Só que falta uma definição de como seria isso de forma a não causar inconsistências.

Ainda não pensei na melhor forma em fazer isso. Você já tem alguma ideia? Se seria criando uma função SQL ou uma feature e um endpoint para desfazer os efeitos de um evento do firewall específico?

[aprendendofelipe]

Ainda não pensei na melhor forma em fazer isso. Você já tem alguma ideia? Se seria criando uma função SQL ou uma feature e um endpoint para desfazer os efeitos de um evento do firewall específico?

Também não pensei. Acho que o ponto mais importante é como ficaria no banco de dados, já que não podemos excluir os balanços criados pelo evento de firewall. Provavelmente teria que criar novos balanços revertendo os do evento de reversão 😅

Talvez uma undoAllFirewallSideEffect parecida com a undoAllRatingOperations.

[Rafatcb]

Firewall

Percebi que a alteração que fiz removia o conteúdo, mas não atualizava o deleted_at, então aproveitei para corrigir isso. Também passei a atualizar o updated_at nas três procedures do firewall.

Email

A parte do e-mail acabou ficando bem "personalizada" de acordo com singular/plural e publicação/comentário. Para as publicações, consegui enviar o título, mas para os comentários não sei o que seria melhor para quem receber o e-mail ter uma noção, então passei apenas os IDs. Se você ter alguma sugestão de melhoria, pode falar.

Não sei se o texto "Identificamos que você está tentando..." é adequado, se deveria mencionar "Identificamos muitas novas publicações no seu IP" ou algo parecido.

Desfazer o efeito do firewall

Modifiquei o user.addFeatures e o balance.undo, e criei o content.undoDeleted para conseguir atualizar os dados de uma forma mais adequada para essa situação específica.

Algumas dúvidas:

1. Essa é uma ação sobre o "firewall", então criei o endpoint DELETE /api/v1/firewall/[event_id], mas na verdade a ação é orientada pela tabela events, tanto que recebe o event.id como parâmetro. O endpoint deveria ser DELETE /api/v1/events/[id]?
2. É mais adequado realizar o event.findOne e a validação no controller e passar o objeto foundEvent para o model (e nesse caso o model precisaria confiar que o objeto foundEvent está correto) ou deixar o findOne e as validações no model mesmo?
3. Acha que faz mais sentido o endpoint retornar algo específico, retornar {} ou null?

[aprendendofelipe]

@Rafatcb, está muito bom, mas como ainda não consegui ver tudo, já vou deixar alguns comentários.

Sobre a refatoração em models/transacional, aprovo todas as mudanças. Até por isso acho que talvez compense separar em um PR prévio a esse e já matar essa etapa.

Sobre a parte de email dentro de models/firewall, será que não são coisas que deveriam estar no models/notification?

Tem mais comentários no código 👍

[Rafatcb]

Sobre a refatoração em models/transacional, aprovo todas as mudanças. Até por isso acho que talvez compense separar em um PR prévio a esse e já matar essa etapa.

Ok, vou fazer isso. Eu fiz aqui porque estava precisando copiar os estilos e percebi a repetição.

Sobre a parte de email dentro de models/firewall, será que não são coisas que deveriam estar no models/notification?

Eu havia começado a fazer no models/notification, mas depois de inspecionar o código tinha ficado em dúvida se esse era o "padrão do projeto". Vou organizar e passar para o models/notification.

[Rafatcb]

Enquanto estou criando o endpoint GET, percebi algo que acho que está errado (filterOutput no model), então decidi aproveitar para comentar também outros pontos que poderiam ser diferentes.

[Rafatcb]

Fiz o GET. Abaixo estão alguns pontos que refleti e fiquei indeciso sobre como proceder. O PR está funcional mas acredito que esses pontos (além dos já comentados) mereçam atenção antes de realizar o merge:

1. Não sei se a interface de retorno está ideal:

   {
     affected: {
       contents?,
       users?
     },
     event,
     status: 'active' | 'reversed'
   }

   Eu escolhi adicionar um atributo affected (que pode ter outro nome) para não correr o risco de eventualmente retornarmos mais coisas e ficar incompatível com a interface atual (imagine que vamos retornar um event afetado, por exemplo). E a interface do affected está compatível com o retorno do endpoint do undo.

2. Não consegui entender como modificar o schemas no validator para conseguir usar no filteredOutputValues do GET. Comecei experimentando validar apenas o retorno de event, mas o schema está esperando que o objeto seja o event, e não que tenha uma tributo event, então já não soube como lidar com esses casos de "atributos com o mesmo nome". Outro exemplo é o status, que não deveria ter a mesma validação do status do content.

   Além disso, precisei modificar o schemas.id para aceitar um Array para conseguir passar um array no where.id do content.findAll. Não me parece ser a solução ideal.

[Rafatcb]

O comportamento ao desfazer os TabCoins na função creditOrDebitTabCoins ignora a operação caso o conteúdo esteja com o total de TabCoins negativo:

• Se a quantidade de TabCoins do conteúdo for positiva, o usuário perde-as. Por exemplo: +10 -4, o usuário perderá 6 TabCoins (ficando com 0). Teste Spamming valid "child" contents with TabCoins earnings em tests/integration/api/v1/contents/firewall.post.test.js.
• Se a quantidade de TabCoins do conteúdo for negativa, a quantidade de TabCoins do usuário não muda. Por exemplo: +1 -5, o usuário continuará com o saldo de -4 TabCoins. Teste Spamming valid "child" contents with one with negative TabCoins em tests/integration/api/v1/contents/firewall.post.test.js.

Para os conteúdos pegos pelo firewall, manteremos esse comportamento, certo?

Os testes para esse cenário em tests/integration/api/v1/moderations/review_firewall/[id]/post.test.js são os testes de nome With a "firewall:block_contents:text_root" event with a content with negative TabCoins (um para undo e outro para confirm).

[aprendendofelipe]

A implementação está ótima!

Fiz alguns comentários, mas a única coisa que me preocupa são os testes, pois podem ter ficado de difícil manutenção.

Gostaria de ouvir opiniões sobre como podemos deixar mais claros quais são os requisitos que cada teste está garantindo.

[aprendendofelipe]

Não estou sugerindo alterar isso nesse PR, apenas abrindo um debate para melhorias futuras...

Será que não era melhor notificar todos os usuários afetados? Penso em um caso de alguém distraído que não se atentou à mensagem de erro recebida, ou alguém usando um sistema desenvolvido pela comunidade, que use a API, mas não apresente os erros corretamente.

Uma outra alteração no mesmo trecho de código pode envolver não notificar mais de uma vez pelo mesmo conteúdo. Podemos olhar o status_before_update para não duplicar as notificações em caso de eventos consecutivos.

[aprendendofelipe]

Precisa testar o GET aqui? Ou realmente está testando algum requisito do POST? Será que a intenção não era testar o comportamento ao realizar POST no outro evento?

A mesma dúvida se aplica ao próximo teste.

[Rafatcb]

Estava testando para garantir que os dois eventos retornam os mesmos dados (review de um, e GET de outro). Isso confirma tanto que estão retornando os mesmos campos quanto que o segundo passou a ser considerado como "avaliado" também (visto que o GET retorna o evento de review).

Inicialmente os endpoints não estavam retornando as mesmas coisas, foi então que percebi a necessidade de modificar o content.confirmFirewallStatus e content.undoFirewallStatus para retornar o nome de usuário, TabCoins e children_deep_count. O validator não apontou esse problema porque o firewall_event apenas valida o schemas.content e schemas.user, sem especificar quais campos são required.

[aprendendofelipe]

A parte que verifica o GET /firewall para esse caso deveria ser adicionada no arquivo de testes do endpoint específico, mesmo que a etapa de preparação seja idêntica, os expects vão mudar, pois lá não precisamos nos preocupar com o que foi retornado no POST /review, e aqui não precisamos verificar o GET /firewall.

Isso deixa os testes mais objetivos, e ajuda a ir direto no ponto do problema em caso de erros.

[Rafatcb]

Gostaria de ouvir opiniões sobre como podemos deixar mais claros quais são os requisitos que cada teste está garantindo.

Acredita que o problema seja o nome das funções? Eu acho que o comportamento esperado fica mais claro quando é usado it('should ....

[aprendendofelipe]

Gostaria de ouvir opiniões sobre como podemos deixar mais claros quais são os requisitos que cada teste está garantindo.

Acredita que o problema seja o nome das funções? Eu acho que o comportamento esperado fica mais claro quando é usado it('should ....

Em alguns casos pode ser melhor adequar o nome, mas em outros é preciso ser mais objetivo nas asserções. Em muitos casos, diminuir a quantidade de expect já deixaria mais explícita a razão de existir de cada teste.

Algumas asserções extras ajudam no momento da criação do teste, pois são verificações se a etapa de preparação está correta antes de ir para o que realmente queremos testar. Essas asserções que ajudam apenas durante a criação dos testes podem ser removidas assim que tiver certeza de que o teste realmente testa o que se espera. Algumas faz sentido manter apenas por documentação, por facilitarem a leitura, por exemplo uma verificação do status (ou o valor de algum campo específico) retornado em uma requisição durante a etapa de preparação, mas isso serve apenas como uma alternativa aos comentários, então não é interessante afastar essas asserções do trecho que definiu cada resultado.

Casos onde seja obrigatório manter as asserções na etapa de preparação do teste para garantir que ele esteja funcionando podem indicar a falta de outros testes para essas etapas específicas.

Mas esses problemas estão presentes em muitos dos nossos testes, não é algo exclusivo do PR atual, então não vai impedir a mesclagem. Minha preocupação é por ser uma dívida técnica que está crescendo muito.

Bora pro merge, pois é muito importante reverter esses os ganhos 🚀