Skip to content
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

Preço e prazo de fretes pelos correios #260

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pedrosancao
Copy link
Contributor

@pedrosancao pedrosancao commented Feb 22, 2021

Novo PR para dar continuidade ao #76 devido à falta de aceso para adicionar commits no repositório do colega que abriu o PR.

Estou dando continuidade no trabalho do @tupizz nesse PR e seria sensacional se concluirmos de forma colaborativa, tem várias tarefas para fazer e espaço para todo mundo colaborar, se você quer participar e ainda não tem acesso avisa aqui que eu adiciono a permissão no fork 😉

Proponho as seguintes tarefas:

  • corrigir conflitos com branch principal
  • separar responsabilidades dos serviços (e99f63e)
  • revisão de código
  • padronizar mensagens de erro desse recurso e outros endpoints BrasiAPI
  • criar documentação
    • no README
    • na página /docs
  • revisão
  • rebase

Resolve: #232


Documentação de integração https://www.correios.com.br/[...]-precos-e-prazos

@vercel
Copy link

vercel bot commented Feb 22, 2021

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/brasilapi/brasilapi/A3bfkoz7xgnY6Z79Q8z91yfPKysU
✅ Preview: https://brasilapi-git-fork-pedrosancao-feature-shipping-brasilapi.vercel.app

Copy link
Member

@lucianopf lucianopf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mestre @pedrosancao deixei uns comentários aí mas fica a vontade de dar dismiss! 😬

pages/api/shipping/v1/calculate-price.js Outdated Show resolved Hide resolved
pages/api/shipping/v1/calculate-price.js Outdated Show resolved Hide resolved
services/shipping/index.js Outdated Show resolved Hide resolved
services/soapRequestService/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@murilohns murilohns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dúvida: Se quisermos calcular frete em outro serviço, conseguimos usar esse mesmo endpoint?

Os parâmetros de entrada são específicos para buscas nos correios, acha que deixar específico dessa forma é a melhor estratégia?

pages/api/shipping/v1/calculate-price.js Outdated Show resolved Hide resolved
pages/api/shipping/v1/calculate-price.js Show resolved Hide resolved
services/shipping/index.js Outdated Show resolved Hide resolved
services/soapRequestService/index.js Outdated Show resolved Hide resolved
This reverts commit 50895bf.

Using HTTPS here broke the tests.
@sonarcloud
Copy link

sonarcloud bot commented Feb 23, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pedrosancao
Copy link
Contributor Author

pedrosancao commented Feb 23, 2021

Dúvida: Se quisermos calcular frete em outro serviço, conseguimos usar esse mesmo endpoint?

A princípio não, mas a proposta do PR é ser especifico do Correio.

Os parâmetros de entrada são específicos para buscas nos correios, acha que deixar específico dessa forma é a melhor estratégia?

Pretendo fazer, além de ocultar/tornar opcional alguns dos parâmetros.

@pedrosancao
Copy link
Contributor Author

Pessoal, esses foram os comentários que achei mais relevantes no PR relacionado que fomentam a discussão:

Sobre as questões:

Validação: body

Excelente ponto, acho que não foi discutido nada sobre isso +1 Eu sei que o Joy é muito comum, mas ele vai ser descontinuado. Não conhecia a solução que você usou, mas me pareceu bem simples. No passado eu tive uma experiência interessante com o is-my-json-valid. @lucianopf alguma opinião sobre isso?

Validação: mensagem de erro

Outro excelente ponto, acho que isso ainda não foi discutido também, mas de todos os serviços que vamos consultar, o BrasilAPI tem que criar uma "camada de anticorrupção" e normalizar todos os erros. Isso foi feito no cep-promise, pois lá também é feita a consulta de serviços diferentes.

Pelo que vi, no seu PR tem esses padrões para mensagens de erro:

{
      name: "",
      errors: [""],
      message: ""
}

E num outro momento:

{
      error: {
        message: "",
        more_info: ""
      }
    }

E no caso do endpoint de consulta de cep, tem esse padrão:

{
  "name": "",
  "message": "",
  "type": "",
  "errors": [
    {
      "name": "",
      "message": "",
      "service": ""
    }
  ]
}

A hora é agora de padronizarmos joy

Resposta de sucesso

Pelo que eu vi você usou tanto pra entrada quanto saída os parâmetros do serviço original. Sugiro sinceramente abstrair isso e implementar os próprios com um padrão mais atual e em iglês +1 tipo ao invés de sCepOrigem ser somente origin (dado que o contexto é cep).

Endpoint

Ao invés de shipping o que acha de talvez algo relacionado ao correios porque shipping é algo que pode se encaixar em qualquer serviço... por exemplo imagina implementar algo do fedex. Então talves até tava pensando aqui algo /shipping/correios o que acha? Ou definir o serviço no body.. não sei.

Consumir SOAP

O que acha de, ao invés de implementarmos a nossa solução, usar um módulo que já tenha resolvido isso?

Por exemplo:

* https://www.npmjs.com/package/soap

* https://www.npmjs.com/package/easy-soap-request

Originalmente publicado por @filipedeschamps em #76 (comment)


Seguindo a thread [...]

Validação: body

Eu confesso que não conhecia esse Yup, dei uma lidinha na doc e gostei bastante! Inclusive nesse momento estava ofão de uma lib de validação pq o projeto Hapi ta sendo discontinuado e não curto muito o AJV.

Validação: mensagem de erro

To total de acordo com o @filipedeschamps nessa, acho que estamos num bom ponto pra pensar em normalizar os formatos de resposta de errrors grimacing

Resposta de sucesso

Concordo bastante tbm que da pra abstrair o nome dos parâmetros de entrada e os de saída, acho que vai ficar ainda mais simples de usar essa rota grimacing

Endpoint

Boa mestre, super interessante a proposta de criar um "workspace" shipping, acho que libera várias novas implementações, tipo usando a Loggi grimacing

Consumir SOAP

Concordo e inclusive acredito que tem lógica exclusiva do correio no arquivo soapRequestService, e que talvez devesse ser renomeado pra CorreiosSoapService ou tornar o arquivo atual mais genérico

Originalmente publicado por @lucianopf em #76 (review)

@LorhanSohaky
Copy link
Member

@pedrosancao , consegue atualizar o PR pra gente voltar a revisar?

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.

None yet

5 participants