Skip to content

Conversation

carolsprak
Copy link
Contributor

Olá Filipe!!

Adorei o vídeo tutorial do Tabnews! Sua ideia de fazer a gente colocar a mão na massa, funcionou pra mim kkkkk
Eu fazia parte daquele grupo que tinha receio de mexer no código daqui. :D
Aí vai meu primeiro PR! \o/

@vercel
Copy link

vercel bot commented Feb 19, 2022

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

To accomplish this, @carolsprak needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@rodrigoKulb
Copy link
Contributor

@carolsprak dei uma olhada em sua PR e achei a mais completa até o momento, você acrescentou o teste de perder a feature, e renomeou de forma correta firstUser, secondUser e thirdUser.

Meu voto até o momento seria para aprovar essa PR.

@huogerac
Copy link

LGTM

@filipedeschamps
Copy link
Owner

@carolsprak sensacional sua contribuição! Falei sobre ela no diário de desenvolvimento

Em breve vou dar continuidade nesse PR, consertando o linter e movendo algumas coisas de lá pra cá 🤝

@vercel
Copy link

vercel bot commented Feb 22, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tabnews/tabnews/E7Bk9sbd8riww6mr24HNzP5WkNwS
✅ Preview: https://tabnews-git-migrationprotection-tabnews.vercel.app

@carolsprak
Copy link
Contributor Author

Ah que legal gente!! Fico feliz pelo PR aceito e de contribuir para o projeto. 😁🥳

@filipedeschamps
Copy link
Owner

Fiz dois commits:

Linting

O primeiro roda o npm run lint:fix para consertar os problemas de linting.

Padrão nos testes

Isso é um WIP (Work In Progress) pra ver como tudo ficava e ver se consigo aproveitar isso para os outros testes. Então usei esse aqui como cobaia e o que eu fiz foi o seguinte:

Alterei os testes para seguir o seguinte padrão e ordem (quando fizer sentido):

  1. Primeiro teste com usuário anônimo
  2. Depois teste com usuário padrão
  3. Por último teste com usuário privilegiado.

Nos testes em que envolvem somente 1 usuário, por exemplo no teste de um usuário padrão tentando ler as migrations, eu não dei o nome ele como firstUser porque nesse escopo não vai ter um secondUser. Eu dei o nome dele de defaultUser. Isso também facilitou o teste, porque como só tem 1 teste nesse escopo, pude remover o beforeEach. O padrão de firstUser e secondUser deveria ser usado quando há o contexto de um usuário interagindo com o outro, como no caso de um usuário tentanto modificar as informações de outro usuário.

No teste User with "migration:read" feature eu juntei nele a ideia do User losing migration:read feature (que achei ótima ideia), pra sempre usar o mesmo usuário, inclusive usar a exata mesma sessão e se certificar que podemos tirar a qualquer momento um poder mesmo quando a sessão está ativa e nem foi renovada. Então ao invés de firstUser ou thirdUser eu coloquei como privilegedUser e fui sempre usando ele. Na versão POST desses testes, eu deletei o teste que testava a remoção da feature, dado que o mecanismo é testado no GET. O que me faz pensar em extrair isso para um use case que testa unicamente esse fluxo (pra não precisar replicar isso em outros testes) e também ficaria responsável por testar a adição e remoção de features através da API.

No texto que vai dentro do test() eu mudei da expectativa para a ação. Isso porque uma ação pode resultar em várias expectativas e você eventualmente vai ler elas pelos assertions encontrados dentro do teste. Isso também facilita levemente quando você precisa mudar a expectativa para uma ação e não precisar mudar nada nos textos.

Deletei também o teste antigo lá de dentro da pages.

Então ficou assim:

image

image

@filipedeschamps filipedeschamps merged commit f81905f into main Feb 23, 2022
@filipedeschamps filipedeschamps deleted the migrationProtection branch February 23, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants