-
Notifications
You must be signed in to change notification settings - Fork 390
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
Adiciona paginação ao endpoint GET /users #1676
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 @Rafatcb! 💪
Pensando em simplificar a implementação/manutenção, estou em dúvida se precisamos de todas essas diferentes estratégias. A que você chamou de recently_updated
já não seria suficiente para a necessidade da moderação?
Fiz comentários no código.
models/user.js
Outdated
import validator from 'models/validator.js'; | ||
|
||
async function findAll() { | ||
async function findAll(values = {}, options) { |
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.
E se apenas substituir a findAll
pela nova findWithStrategy
já apropriada para /api/v1/users
?
A tentativa de deixar a findAll
flexível acaba tornando ela mais complexa do que o necessário, e dificultando seu entendimento, como acontece em content.findAll()
. Em models/content
isso até se justifica, mas aqui não estou vendo necessidade.
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.
Você diz para retornar a contagem de usuários, os resultados de forma ordenada e com paginação sempre? Se for isso, o findAll
é utilizado no PR #1638, então teria alguns pontos negativos pela contagem não ser necessária naquela situação e pela paginação poder, eventualmente, causar um bug difícil de identificar (se futuramente termos um evento que envolva mais contas do que o tamanho padrão da paginação).
Hoje o findAll
está ordenado por created_at ASC
, o que já prejudicaria o desempenho no outro PR, mas não precisaria dessa ordenação por padrão.
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.
Eu deixei as funções findAll
e findAllWithPagination
separadas para deixá-las menos flexíveis. Se você ter uma sugestão melhor de usar uma única função tendo em vista os pontos que levantei no meu comentário anterior, diga que eu modifico.
let firstUser; | ||
let secondUser; | ||
let privilegedUser; | ||
let unprivilegedUser; |
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.
Precisa mesmo de firstUser
e secondUser
? Deve ser suficiente criar aqui apenas as variáveis privilegedUser
e defaultUser
, já que são duplicadas com as outras duas.
unprivilegedUser
me parece menos adequada do que defaultUser
, que já é o padrão adotado em todos os testes.
E deve compensar também declarar aqui a privilegedUserSession
, que pode ser atribuída em beforeAll()
. O que acha?
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.
Precisa mesmo de firstUser e secondUser?
Não é necessário, mas facilita o entendimento dos testes nas comparações expect(responseBody).toStrictEqual
, onde pelo nome da variável o leitor consegue saber que um foi criado antes do outro.
unprivilegedUser me parece menos adequada do que defaultUser, que já é o padrão adotado em todos os testes.
Ok.
E deve compensar também declarar aqui a privilegedUserSession, que pode ser atribuída em beforeAll(). O que acha?
Isso pode ser feito sim 👍
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.
Pensando em simplificar a implementação/manutenção, estou em dúvida se precisamos de todas essas diferentes estratégias. A que você chamou de
recently_updated
já não seria suficiente para a necessidade da moderação?
Seria sim. Pensei nas outras apenas para ficar num padrão parecido com o /contents
, que possui as estratégias old
e new
, por exemplo, mas posso deixar apenas com uma ordenação.
models/user.js
Outdated
import validator from 'models/validator.js'; | ||
|
||
async function findAll() { | ||
async function findAll(values = {}, options) { |
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.
Você diz para retornar a contagem de usuários, os resultados de forma ordenada e com paginação sempre? Se for isso, o findAll
é utilizado no PR #1638, então teria alguns pontos negativos pela contagem não ser necessária naquela situação e pela paginação poder, eventualmente, causar um bug difícil de identificar (se futuramente termos um evento que envolva mais contas do que o tamanho padrão da paginação).
Hoje o findAll
está ordenado por created_at ASC
, o que já prejudicaria o desempenho no outro PR, mas não precisaria dessa ordenação por padrão.
let firstUser; | ||
let secondUser; | ||
let privilegedUser; | ||
let unprivilegedUser; |
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.
Precisa mesmo de firstUser e secondUser?
Não é necessário, mas facilita o entendimento dos testes nas comparações expect(responseBody).toStrictEqual
, onde pelo nome da variável o leitor consegue saber que um foi criado antes do outro.
unprivilegedUser me parece menos adequada do que defaultUser, que já é o padrão adotado em todos os testes.
Ok.
E deve compensar também declarar aqui a privilegedUserSession, que pode ser atribuída em beforeAll(). O que acha?
Isso pode ser feito sim 👍
cac9131
to
2430335
Compare
2430335
to
0951ab4
Compare
0951ab4
to
d514068
Compare
d514068
to
965721b
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.
Já aprovei, mas vejam, por favor, se faz sentido mexer nos pontos que comentei no código 🤝
Em produção! 🚀🚀🚀 Já adicionei a feature |
Mudanças realizadas
Resolve a parte do backend do issue #1675.
Passei o
getPagination
para um outro arquivo para poder ser reutilizado e também aproveitei para refatorar algumas coisas simples, como substituirhttp://localhost:3000
pororchestrator.webserverUrl
em alguns testes.GET /api/v1/users
comper_page
epage
, ordenado pela data de atualização (updated_at
, mais recentes primeiro).Tipo de mudança
Checklist: