Remover ou Editar conteúdos de outros usuários

[filipedeschamps]

O plano inicial era ir parcialmente colocando a issue #348 em Produção, mas estou um pouco inseguro com a migration dado os motivos conversados a partir desse comentário. Então mais do que nunca vou deixar ela pendurada aqui e ir fazendo os commits contra essa branch.

71e005c

• Habilitei novamente os logs do migrator.
• Fiz isso, pois agora temos um sistema de logs melhor para pesquisar/investigar.

db0b0d4

• Notei que na migration passada eu escolhi um nome de arquivo ruim, então nessa procurei melhorar.
• O padrão que pensei para esse caso foi [create|alter]-table-[tablename]-[description] o que resultou em alter-table-contents-add-deleted-at-checks
• Essa migration adiciona a coluna `deleted_at, altera a constraint dos valores que aceita e substitui a constraint de valores únicos por um índice único.

[vercel]

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

Name	Status	Preview	Updated
tabnews	✅ Ready (Inspect)	Visit Preview	Jun 4, 2022 at 0:19AM (UTC)

[filipedeschamps]

b0c84ff

• Pode ser impressão minha, mas parece que ultimamente os testes estão mais devagares (até considerando outros lugares que não envolvem o unstable_revalidate() e depois quero ver se tem a ver com a atualização do Node.js para v16.
• Por conta disso, coloquei o timeout do Jest globalmente para 60 segundos, pois o CI estava quebrando para testes que não tinha nada a ver com a implementação.

6fbda7d

• Começa a trabalhar com deleted_at primariamente no que envolve POST (mas ainda falta muita coisa para fazer).
• Houve um reflexo nos testes do PATCH por conta da mensagem sobre o status que agora também explica que um dos valores válidos dessa propriedade é deleted.
• Mas o foco principal desse commit foi uma verificação no model content para que não seja possível criar conteúdos diretamente com valor deleted. Não vejo fazer sentido.

[filipedeschamps]

362c568

• Antes para criar um conteúdo, o método content.create() executava uma busca adicional no banco de dados para saber se um conteúdo com o mesmo owner_id e slug existia e isso era feito pela função checkForContentUniqueness(). Essa função foi removida em favor de deixar o banco de dados estourar um erro com o check nativo do índice único. Assim vamos economizar uma ida ao banco de dados e centralizar a verificação de duplicidade em um único lugar, com uma única regra.
• Por conta disso tive que refatorar um pouco como o database.js lida com os erros e agora nós levamos em conta o code retornado por ele.
• O que me deixou mais surpreso é que não tinha nenhum teste para conteúdo duplicado no POST (e não tem em PATCH ainda). Inclusive vou adicionar mais alguns testes testando combinações diferentes de status, mas para isso preciso fazer a parte do deleted final.

[filipedeschamps]

9ba5d07

• Remove mais uma consulta adicional que estava sendo feita no banco por dentro da função populatePublishedAtValue() e que era usada tanto na criação de novos conteúdos, quanto atualização. Agora ela faz os cálculos apenas com os contents que já estavam na memória.
• O método content.update() agora popula o campo deleted_at pela função populateDeletedAtValue().
• Adicionado teste que se certifica receber um 201 no caso de existir um conteúdo do mesmo user, com o mesmo slug, mas diferentes nos status de published e deleted (por conta de popular o deleted_at).
• Orchestrator agora possui o método orchestrator.updateContent() para forçar atualizações de conteúdo direto pelo model content.

[tembra]

O padrão que pensei para esse caso foi [create|alter]-table-[tablename]-[description] o que resultou em alter-table-contents-add-deleted-at-checks

O que sempre tenho visto como padrão de nomenclatura para migrations é primeiro ser informado a ação que será executada de fato e somente depois informações que também são relevantes. Assim a leitura é mais imperativa e direta.

Exemplos:

• create table contents
• add deleted_at column on contents table
• add unique index with owner_id slug and deleted_at columns on contents table

Se quisermos agregar várias funcionalidades em uma única migration, como por exemplo reunir as duas últimas acima listadas (criação de coluna e índice único) em uma só, vejo usarem bastante algo como:

• add delete feature on contents table

[filipedeschamps]

57e16b4

• Agora o where dos métodos de busca do content suportam $or e a interface foi inspirada no Sequelize, por exemplo:

    const contentToBeUpdated = await content.findOne({
      where: {
        username: request.query.username,
        slug: request.query.slug,
        $or: [{ status: 'draft' }, { status: 'published' }],
      },
    });

• Hoje ninguém pode mandar pela API esses parâmetros (não há interface pública para definir isso), mas de qualquer forma toda validação já está sendo feita com o validator, inclusive da string que vai dentro de cada objeto do array $or, então você não consegue pedir por um status que não existe.

• Agora o método findOne() adiciona um LIMIT 1 na query. Antes estava 30 que era o padrão da paginação.

• E num geral, as regras são o seguinte:

  • Você não consegue dar GET num conteúdo com status deleted (tanto root quanto child).
  • Você pode atualizar o status de um conteúdo para deleted uma única vez e isto irá preencher o campo deleted_at. Depois disso, não há mais como atualizar o status, nem nada do conteúdo.
  • O mesmo usuário não pode criar dois conteúdos com o mesmo slug se eles estiverem com status draft ou published.
  • Mas pode existir no banco de dados o mesmo slug se o conteúdo estiver deleted (por conta do uniqueness causado pela combinação com deleted_at).
  • Todas essas regras valem tanto para POST (criação de um novo conteúdo) quanto para PATCH (atualização).
  • Fora isso e como já foi apontado lá em cima, o POST não pode criar um conteúdo diretamente com status deleted.

[filipedeschamps]

Fiz merge do commit 86d29e3 do @gabrieldev525 (que inclusive ficou lá no topo na lista dos commits porque foi feito antes de todo backend 👍 ) e apliquei alguns ajustes. Me sinto pronto para rodar a migration no ambiente de Homologação 😍

Em paralelo, infelizmente o CI está reclamando de alguma mensagem do commit lá do passado (quando fiz um squash pela interface do Github).

[filipedeschamps]

Migrations rodadas e feature pode ser integralmente testada em Homologação: https://tabnews-git-delete-tabnews.vercel.app/ 👍

E segue vídeo mostrando o comportamento:

Screen.Recording.2022-06-03.at.10.42.57.AM.mov

Nota que na Home não sumiu automaticamente porque por enquanto não estamos forçando o revalidate.

Em paralelo, criei a issue #435 sobre o que fazer com o histórico de commits.

[aprendendofelipe]

Tudo está funcionando como o esperado!

Apaguei uma resposta que tinha uma resposta dentro dela e agora só tenho acesso pelo link, ou seja, fica uma resposta isolada da árvore.

Testei também começar a escrever uma resposta para um artigo, mas apaguei ele antes de publicar a resposta. Ao publicar, a condição da resposta ficou a mesma, publicada, mas só acessa pelo link direto.
https://tabnews-git-delete-tabnews.vercel.app/fe1ipe/ac6e7f04-0862-47f5-a554-2046a2200c43

Apagar uma resposta que tem respostas dentro faz aparecer a mensagem "Esse conteúdo não está mais disponível.", mas ainda é possível acessar as respostas enquanto não sair da página.

Será que não seria interessante aparecerem essas respostas isoladas pelo menos em page/[username]?

[aprendendofelipe]

Tudo está funcionando como o esperado!

Ou quase...
Apagar uma mensagem root e dar F5 gera o seguinte erro enquanto a página não é revalidada para gerar o 404:
"Application error: a client-side exception has occurred (see the browser console for more information)."

Se não me engano, essa mensagem aparece quando ocorre uma exceção não tratada na busca de dados para geração da página estática. Se for isso mesmo, precisaria ver qual erro está gerando e, se fizer sentido, retornar { notFound: true }

[aprendendofelipe]

No menu, e também na mensagem de confirmação de remoção da publicação, está sendo usada a palavra "deletar"... Acho bem estranho usar esse brasileirismo, sendo que temos alternativas como "apagar" ou "remover"... O que vocês acham?

[ermesonsampaio]

@aprendendofelipe também acho que fica estranho...

Testando essa feature achei que seria interessante criar uma página de 404, acho que isso seria algo interessante de adicionar ao TabNews após a conclusão da Milestone atual.

[filipedeschamps]

Muito obrigado pelos testes 😍 😍 😍

Apagar uma resposta que tem respostas dentro faz aparecer a mensagem "Esse conteúdo não está mais disponível.", mas ainda é possível acessar as respostas enquanto não sair da página.

Correto! Mas esse é um state que vai ser existente apenas na instância de quem deletou a resposta inicial, da mesma forma que quando deletar um root content, tudo fica ali em baixo, mas é só na memória daquela página. Não sei se agora vale a pena mexer nesse comportamento.

Será que não seria interessante aparecerem essas respostas isoladas pelo menos em page/[username]?

Não entendi essa sugestão. Você diz para não perder as respostas em baixo? Daí sugiro fazer como o Twitter faz e como o @gabrieldev525 tinha preparado a mensagem do componente (e da página) que era receber da API um conteúdo que se estivesse com status deleted ela seria mostrada, daí toda árvore continua sendo mostrada. Podemos fazer na próxima Milestone.

Apagar uma mensagem root e dar F5 gera o seguinte erro enquanto a página não é revalidada para gerar o 404: "Application error: a client-side exception has occurred (see the browser console for more information)."

Se não me engano, essa mensagem aparece quando ocorre uma exceção não tratada na busca de dados para geração da página estática. Se for isso mesmo, precisaria ver qual erro está gerando e, se fizer sentido, retornar { notFound: true }

Boa!!! Tem duas partes esse problema:

Server

tabnews.com.br/pages/[username]/[slug]/index.public.js

Lines 168 to 194 in a8f9b7f

	try {
	contentFound = await content.findOne({
	where: {
	username: context.params.username,
	slug: context.params.slug,
	status: 'published',
	},
	});
	if (!contentFound) {
	throw new NotFoundError({
	message: `O conteúdo informado não foi encontrado no sistema.`,
	action: 'Verifique se o "slug" está digitado corretamente.',
	stack: new Error().stack,
	errorUniqueCode: 'PAGES:USERNAME:SLUG:GET_STATIC_PROPS:SLUG_NOT_FOUND',
	key: 'slug',
	});
	}
	} catch (error) {
	if (error instanceof NotFoundError) {
	return {
	notFound: true,
	};
	}
	throw error;
	}

Isso está coberto por essa lógica, mas a condição if (!contentFound) { nunca vai ser executada, pois cai no catch ali de baixo. Então vou refatorar 🤝

Client

O erro que você presenciou é do client, veja essa parte da mensagem:

Application error: a client-side exception has occurred

Isso está acontecendo porque o fluxo da abertura de uma página é o seguinte:

1. Você abre uma página e ela pode ser uma página estática não revalidada.
2. Isso dispara a revalidação no background da Vercel.
3. Em paralelo- para compensar o fato de você poder estar vendo uma página estática desatualizada - o client usa o swr para bater na API e pegar a última versão do conteúdo dessa página (tanto o conteúdo raiz, quanto a árvore de children).
4. E se o conteúdo não existe mais, a API vai retornar um 404 e no objeto do conteúdo, ao invés de ter um content, tem um objeto de erro, e daí o client estoura, pois os componentes não estão esperando isso.

Sugestão para esse caso

Não fazer nada nessa Milestone e juntar com o que o @ermesonsampaio sugeriu e fazer o seguinte:

1. Na próxima Milestone, fazer um método que seja possível descobrir qual o root content (independente de onde você esteja na árvore) e rodar um unstable_revalidate(). Assim podemos tirar a necessidade do swr fazer um request adicional para API na busca do conteúdo mais atualizado.
2. Criar uma página 404, porque a que tem hoje não dá nem para ler o que está escrito se você usa o tema Dark no sistema operacional.

No menu, e também na mensagem de confirmação de remoção da publicação, está sendo usada a palavra "deletar"... Acho bem estranho usar esse brasileirismo, sendo que temos alternativas como "apagar" ou "remover"... O que vocês acham?

Perfeito, vou alterar 🤝

[filipedeschamps]

@aprendendofelipe analisando aqui a parte do Server, o que eu expliquei está errado. Ele cai na condição if (!contentFound) {, que dá um throw num NotFoundError e que daí cai no catch e retorna o notFound: true. Ta estranho esse código, mas lembro que fiz isso para comportar se nesse ponto alguém digitar na URL um [username] que não existe.

Por exemplo:

[OK] https://www.tabnews.com.br/filipedeschamps/angular-v14-acabou-de-ser-lancado
[404] https://www.tabnews.com.br/filipedeschampssss/angular-v14-acabou-de-ser-lancado

Quem vai fazer estourar o 404 ali em cima é a falta de encontrar o [username] naquele path path.

[filipedeschamps]

Trocado tudo para "apagar" 🤝

[aprendendofelipe]

@filipedeschamps 🚀

---

esse é um state que vai ser existente apenas na instância de quem deletou a resposta inicial

Isso, isso, isso... O que falo nessa mensagem é está dentro de:

Tudo está funcionando como o esperado!

---

Não entendi essa sugestão.

A sugestão é, na página dos usuários, aparecerem também as postagens "em resposta de", e não só os conteúdos root. Assim a resposta que perdeu o root ainda estaria acessível facilmente caso o autor queira excluí-la ou, talvez, torná-la root.

---

Com relação ao erro momentâneo no lugar do 404...
Não é nada grave, pois só ocorre em uma condição bem específica, então pode ser sanado com calma depois. Eu posso investigar melhor quando tiver tempo, pois tenho curiosidade nessa mensagem que vejo em muitos sites em produção (por exemplo ao tentar editar uma Function Hook do banco de dados do Supabase).
E realmente não deve ser nada no lado do servidor.

---

Na próxima Milestone, fazer um método que seja possível descobrir qual o root content (independente de onde você esteja na árvore) e rodar um unstable_revalidate(). Assim podemos tirar a necessidade do swr fazer um request adicional para API na busca do conteúdo mais atualizado.

Acredito que, para eliminar o swr, a revalidação precise ser feita em cada conteúdo que esteja no mesmo ramo, não só no root.

---

Criar uma página 404

Minha 404 favorita... hahaha

// pages/404.js
import { useEffect } from "react"
import { useRouter } from "next/router"

export default function Custom404() {
    const router = useRouter()

    useEffect(() => {
        router.replace("/")
    })

    return null
}

[filipedeschamps]

A sugestão é, na página dos usuários, aparecerem também as postagens "em resposta de", e não só os conteúdos root. Assim a resposta que perdeu o root ainda estaria acessível facilmente caso o autor queira excluí-la ou, talvez, torná-la root.

Ahh perfeita sugestão 🤝 não de executar agora, mas sim a página de perfil poderia total listar todas as respostas, até porque, todas podem ter valor concreto e quanto mais isso ficar exposto, melhor... muito bem bolado!! 👍

Acredito que, para eliminar o swr, a revalidação precise ser feita em cada conteúdo que esteja no mesmo ramo, não só no root.

Puts é verdade. Fora todos os nós intermediários, um que vai dar problema é o que a pessoa recebe o link por email. Então a gente tem que melhorar em como o swr retorna o dado, e talvez abstrair isso num custom hook.

Minha 404 favorita... hahaha

😂 200 forever 😂

[filipedeschamps]

Merged! Let's goooooo!!!

Rodei as migrations em produção, fiz um post de teste, deletei, tudo ok 🤝

E isso levou a Milestone 4 para 74% finalizada, estamos cada vez mais próximos das TabCoins 👍

[filipedeschamps]

@aprendendofelipe e @tembra vocês foram citado nesse post de comemoração: https://www.tabnews.com.br/filipedeschamps/novas-melhorias-habilidade-de-apagar-suas-publicacoes-e-edicao-pela-moderacao 🎉