-
Notifications
You must be signed in to change notification settings - Fork 384
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
feat: adicionar retorno de resultados para comentários #1601
Conversation
@mthmcalixto is attempting to deploy a commit to the TabNews Team on Vercel. A member of the Team first needs to authorize it. |
@mthmcalixto Obrigado pela PR 🤝
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ótima contribuição, @mthmcalixto! Eu nem tinha percebido que a API não havia sido atualizada com isso.
O comentário do @kaique-soares é importante, podemos manter uma padronização nos endpoints sobre isso usando o mesmo nome de parâmetro. O PR onde foi implementado para publicações do usuário é o #1188. Acho que você pode usar uma lógica bem parecida no seu PR. Serão três cenários possíveis:
- Busca apenas por comentários.
- Busca apenas por publicações.
- Busca por ambos.
Eu não testei, mas vendo o outro arquivo, como tem a opção "ambos", acho que vai continuar precisando do loop com o delete content.body;
, mas ter o exclude: ['body']
é interessante para o caso de busca apenas por título, assim já obtemos menos dados do banco. Se isso se provar correto, podemos até atualizar o outro arquivo também.
E, por favor, crie testes para as novas funcionalidades. Pode ver no outro PR como foi feito e adaptar para o contents
. Qualquer dúvida, pode comentar.
PS: Depois faz um squash
dos seus commits e coloca a mensagem em inglês, por favor. Se não souber fazer squash, dá uma olhada nessa resposta do Stack Overflow.
@kaique-soares e @Rafatcb, obrigado por avaliar o PR, eu dei uma olhada no #1188. E basicamente ele fez tudo que precisa ser feito na rota /contents que não usa o username do usuário. Há apenas um detalhe que testei nas rotas criadas, como no endpoint:
Esse endpoint retorna todos os conteúdos do usuário, incluindo comentários e publicações, sem a necessidade de passar a query "with_children". No entanto, o endpoint:
Retorna apenas publicações, o que significa que, teoricamente, não deveria incluir comentários em um novo PR, a menos que a query "with_children" esteja presente. Portanto, acredito que as alterações feitas em #1188 podem ser aplicadas a esse novo endpoint /contents. Além disso, sobre o exclude: ['body'], no endpoint que busca o conteúdo do usuário, ele consegue retornar sem o corpo se for uma publicação. Vou tentar implementar e usar a mesma lógica para o endpoint /contents. |
Isso mesmo @mthmcalixto, tem uma diferença nos valores default dos parâmetros. Para o endpoint |
@aprendendofelipe Dei uma progredida, mas acabei percebendo sobre o que o @Rafatcb disse, o body acaba ficando entre ambos, tanto nas publicações e comentários, só é possível remover se a chamada principal for sem os querys, então é só verificar se está solicitando que tenha comentários, o with_children e usar o attributes para excluir o body. Com isso, o body iria ficar apenas em ambos quando solicitado, a não ser que faça uma interceptação em results.rows e remova os corpos dos parent_id nulos, mas ele iria acabar puxando esse body do banco de dados, talvez se limitar o tamanho do body direto na query do banco de dados, iria retornar menos dados do body e então alterar o results.rows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show @mthmcalixto!
Fiz comentários no código, e também gostaria de sugerir encurtar os nomes dos testes.
Eu não revisei o conteúdo dos testes, só dei uma passada pelos nomes deles. Pelas descrições, acho que está testando tudo que é necessário, mas acho que as descrições poderiam se concentrar na parte importante de cada teste.
Caso ache válido, você pode usar describe
para agrupar alguns testes.
E aproveitando sobre o assunto testes... Eu acho que não estamos adotando uma boa estratégia ao limpar o banco de dados a cada teste. Falo especificamente sobre os testes que só usam o método GET.
Testes que podem ser executados com um mesmo conjunto de dados (no banco de dados) poderiam ser agrupados e executados de forma mais rápida.
Em alguns casos também daria para usar test.each
ou describe.each
para testes idênticos em que só mudam os dados de entrada e saída.
Não precisamos fazer isso nesse PR, mas gostaria de saber opiniões sobre isso.
@aprendendofelipe corrigido eecd852, realmente os testes estão ficando maiores, seria interessante achar uma forma de diminuir. No vscode tem uma extensão do Jest que é possível iniciar um teste individualmente, então não precisa passar por todos e isso agiliza o teste. |
Não revisei o PR ainda, só vi os comentários aqui. @mthmcalixto, não esquece de deixar a mensagem do commit em inglês, por favor.
Você pode usar
👍 @aprendendofelipe Se estiver muito lento, dá pra tentar evitar o |
@Rafatcb alterei a mensagem do commit, esperando apenas a revisão do PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mthmcalixto, fiz comentários sobre os nomes dos testes. Veja se faz sentido pra você.
E sobre o commit:
- Não entendi porque fala em
breaking change
na mensagem, acho que pode retirar; - O tipo ficou como
revert
ao invés defeat
; - É bom inserir um contexto, algo como
feat(content endpoint):
; - A mensagem pode ficar mais clara se citar que está adicionando os parâmetros
with_root
ewith_children
; - O título do PR não precisa ser igual ao do commit, e pode ser em português.
@aprendendofelipe Sobre o |
Um breaking change, no contexto da API, seria mudar o comportamento de forma que quem está usando a API agora, precisará mudar alguma coisa depois da implementação. O ideal, inclusive, é que algum teste existente quebre quando tem breaking change. Como sua mudança não mudou o comportamento padrão, ela não é uma breaking change, então não precisa se preocupar com isso. |
04b4f75
to
6d9676c
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Chegamos em um ponto interessante. Como lidar quando for enviado em conjunto a Da forma como está, enviar Uma opção é deixar como está, ou seja, os parâmetros são ignorados, mas não acho uma boa opção. Outra opção é retornar um erro de validação dos parâmetros, mas acho que não vale a pena a complexidade para validar isso. A terceira é fazer igual ao que ocorre no Para essa última alternativa, deve ser suficiente alterar este condicional adicionando tabnews.com.br/models/content.js Lines 239 to 241 in d03462c
Ficando assim: if (!values?.where?.owner_username && values?.where?.parent_id === null) {
options.strategy = 'relevant_global';
} E é bom adicionar testes para isso. O que acham? |
@aprendendofelipe não estou ciente da Tem alguma coisa errada em tabnews-git-fork-mthmcalixto-main-tabnews.vercel.app/api/v1/contents, ele não está retornando o conteúdo, mas localmente está, existe alguma limitação? Entendi, se não passar a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mthmcalixto, sobre a implementação da funcionalidade, está ótima. Até sugiro que na próxima vez que mexer no código, deixar o arquivo dos testes em um commit separado, pois é só nele que ainda precisa mexer, e se alguém sugerir algo mais interessante, fica fácil aproveitar o seu commit da funcionalidade.
Sobre os testes, tem muito condicional dentro deles. Isso é bem ruim.
Até mantive os comentários que eu já tinha feito mais cedo no código, dizendo para remover o withDraft
, mas a verdade é que deveria remover todos, ou praticamente todos, os condicionais, pois não fica nada claro o que os testes estão fazendo.
Um exemplo é que existia o teste chamado get new root, including draft...
que, contrariando o título, corretamente não recuperava o rascunho, mas tem que analisar a lógica do teste para descobrir isso, quando o correto era o expect
deixar isso claro.
Falando em expect
, preferencialmente ele deveria estar dentro do array passado para test.each
.
Outro detalhe sobre a organização dos testes, é que não está mais testando exclusivamente os parâmetros de tipo de conteúdo, então o describe
não faz mais sentido como está.
Eu não queria submeter esse feedback sem sugerir uma solução, mas como já demorei demais, preferi dar esse retorno parcial, pois alguém pode dar alguma ideia com base nele.
const firstRootContent = await orchestrator.createContent({ | ||
owner_id: firstUser.id, | ||
title: 'Primeiro conteúdo criado', | ||
status: withDraft ? 'draft' : 'published', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status: withDraft ? 'draft' : 'published', | |
status: 'draft', |
const url = `${orchestrator.webserverUrl}/api/v1/contents?${params.join('&')}`; | ||
const withChildren = params.includes('with_children=true'); | ||
const withRoot = !params.includes('with_root=false'); | ||
const withDraft = content.includes('draft'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const withDraft = content.includes('draft'); |
{ content: 'relevant root', params: [] }, | ||
{ content: 'relevant root, including draft,', params: ['with_children=false'] }, | ||
{ content: 'relevant children', params: ['with_children=true'] }, | ||
{ content: 'new root, including draft,', params: ['with_children=false', 'with_root=true', 'strategy=new'] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ content: 'new root, including draft,', params: ['with_children=false', 'with_root=true', 'strategy=new'] }, |
|
||
if (!withDraft) { | ||
rootReturn.push({ | ||
id: firstRootContent.id, | ||
owner_id: firstUser.id, | ||
parent_id: null, | ||
slug: 'primeiro-conteudo-criado', | ||
title: 'Primeiro conteúdo criado', | ||
status: 'published', | ||
source_url: null, | ||
created_at: firstRootContent.created_at.toISOString(), | ||
updated_at: firstRootContent.updated_at.toISOString(), | ||
published_at: firstRootContent.published_at.toISOString(), | ||
deleted_at: null, | ||
owner_username: firstUser.username, | ||
tabcoins: 1, | ||
children_deep_count: 0, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!withDraft) { | |
rootReturn.push({ | |
id: firstRootContent.id, | |
owner_id: firstUser.id, | |
parent_id: null, | |
slug: 'primeiro-conteudo-criado', | |
title: 'Primeiro conteúdo criado', | |
status: 'published', | |
source_url: null, | |
created_at: firstRootContent.created_at.toISOString(), | |
updated_at: firstRootContent.updated_at.toISOString(), | |
published_at: firstRootContent.published_at.toISOString(), | |
deleted_at: null, | |
owner_username: firstUser.username, | |
tabcoins: 1, | |
children_deep_count: 0, | |
}); | |
} |
34a5b98
to
92b681e
Compare
@aprendendofelipe pode verificar, acredito que os testes vão fazer mais sentindo agora. Apenas para clarear, o teste está reutilizando o retorno sem a necessidade de criar outros testes com mesmo retorno, o teste verifica com base nos No caso se contém Também tem a lógica dos comentários, com a estratégia Consigo criar vários testes utilizando apenas os Eu não consigo imaginar outra solução se não tiver condicionais para isso. |
@aprendendofelipe a única coisa que passou pela minha cabeça agora foi:
Eu fiz as modificações para ver se realmente pensei algo factível, e realizei um commit na branch do @mthmcalixto para facilitar de compartilhar o que eu estava pensando: |
3fa8e8a
to
db775fc
Compare
Isso @Rafatcb, é praticamente como você fez. Fiz um commit aproximando um pouco mais do que pensei, ou seja, com o conjunto de testes usando os mesmos dados do banco, o que faz eles serem executados em 1/4 do tempo. Tentei deixar o diff o menor possível, então a parte do describe talvez não tenha ficado da melhor forma. Também corrigi um erro de digitação e removi um teste duplicado. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fiz um commit aproximando um pouco mais do que pensei, ou seja, com o conjunto de testes usando os mesmos dados do banco, o que faz eles serem executados em 1/4 do tempo.
Eu tinha tentado usar o beforeAll
a princípio, mas não tinha pensado em fazer a divisão como você fez para funcionar com o beforeAll
.
Parece tudo certo para mim.
Podemos fazer uma refatoração do arquivo de teste em outro PR com as descobertas que tivemos nesse PR, como mencionado nesse comentário #1601 (review).
add with_children and with_root query params to filter content between posts and comments.
db775fc
to
aa77f45
Compare
aa77f45
to
6a822f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Podemos fazer uma refatoração do arquivo de teste em outro PR com as descobertas que tivemos nesse PR, como mencionado nesse comentário #1601 (review).
Combinado, então bora pro merge! 🤝
@mthmcalixto, seu código já está rodando em produção! 🚀🚀🚀 Bem vindo à Turma de Contribuidores do TabNews! 👏👏👏 |
Vocês dois são bons em criar testes hahah, ficou muito bom 🚀🚀🚀 |
Adicionar uma query com nome type na API para retornar os comentários de uma postagem, listar todos os comentários por paginação e estratégia.
Mudanças realizadas
Adicionar retorno de resultados para comentários, como recentemente foi adicionado na aba recentes tabs com "todos", "comentários", além das "publicações"., não encontrei alguma alteração na API em relação ao retorno dos comentários.
Endpoints afetados:
Novo endpoint adicionado:
O "type" deve possuir um dos seguintes valores: "comments"
Tipo de mudança
Checklist: