cambiatus / backend

Cambiatus GraphQL API
GNU Affero General Public License v3.0
21 stars 18 forks source link

Improvement on kyc error message when document is not valid #240

Closed MatheusBuss closed 2 years ago

MatheusBuss commented 2 years ago

What issue does this PR close

Closes #211

Changes Proposed ( a list of new changes introduced by this PR)

henriquecbuss commented 2 years ago

I don't know if you're already doing this, but this seems like the perfect instance to start looking at TDD. The basic idea is that you write a failing test first, and then make the code work. This usually helps you write tests that test the behavior, instead of testing your code.

For example, imagine a scenario where the user entered a document with 9 digits, but the document needs to have 10 digits. In that case, you would first write a test that says the document needs 10 digits, and if it doesn't, a specific error message comes up. That test will clearly fail, because we haven't treated that case yet. Then you write the code needed to fix that test, and iterate on it until the test passes. After that, you can come back and refactor the code, and move on with other scenarios.

You don't need to do this, but I think it can be pretty nice in these kinds of changes

MatheusBuss commented 2 years ago

but this seems like the perfect instance to start looking at TDD.

Yeah, I agree, but I haven't quite gotten my mind around to TDD yet. At this point the code is almost complete (but probably could use some refactoring) so I'll have to start on TDD on another date :( . Thanks for the heads up though.

MatheusBuss commented 2 years ago

bro olhando esse monte de código, fiquei aqui pensando, pq não fazemos a higiene da string (tirar os eventuais -.)e só validamos com a regex normal?

+1, já simplifica bastante coisa. Inlcusive, pelo que testei, o frontend só aceita números nos campos de documentos, @NeoVier pode me corrigir se estiver errado. Daí só higieniza as entradas e adapta as regexes para trabalharem com dígitos.

Para a validação de check_null_first_digit, nós precisamos fazer isso? A regex consegue olhar isso também.

Essa flag seria só pra diferenciar os documentos que requerem um dígito não nulo e os documentos que não requerem, pra ele acusar de maneira certa. A regex pega sim, mas em alguns documentos não tem problema esse primeiro dígito ser nulo.

henriquecbuss commented 2 years ago

o frontend só aceita números nos campos de documentos

Parece que sim. De qualquer maneira, os - são dados de apresentação. Talvez seja melhor guardar no backend sem eles, e se quisermos mostrá-los, adicionamos no frontend

MatheusBuss commented 2 years ago

Parece que sim. De qualquer maneira, os - são dados de apresentação. Talvez seja melhor guardar no backend sem eles, e se quisermos mostrá-los, adicionamos no frontend

Higienizei os dados só pra fazer a verificação. Imagino que pra higienizar todos os dados que vão entrar seria bom também higienizar os dados que já estão guardados. Se for o caso podemos abrir uma issue pra isso @lucca65?

lucca65 commented 2 years ago

Acho que não precisa bro, esses dados foram feitos só para ter um acompanhamento que era necessário na época

  1. Tirar os - só para validar
  2. Guardar como vier