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

[14.0][NEW] Importação de documentos fiscais #2552

Merged
merged 25 commits into from
Oct 24, 2023

Conversation

felipezago
Copy link
Contributor

@felipezago felipezago commented Jun 23, 2023

Implementação de wizard base para importação de documentos fiscais, e também o wizard para importação de NFe.

@OCA-git-bot
Copy link
Contributor

Hi @rvalyi, @renatonlima,
some modules you are maintaining are being modified, check this out!

@rvalyi
Copy link
Member

rvalyi commented Jun 29, 2023

Ola @felipezago tem coisas legal nesse PR, porem é importante a gente estancar a sangria com o generateDS antes de aceitar mais funcionalidade com XML, ou seja, tem que fazer o merge desse PR #2371 (comment) antes...

Depois deve ser relativamente facil adaptar seus PR's olhando o as mudanças que eu tive que fazer nesse PR. Tem tb algumas outras coisas que eu vou sugerir mudar, mas isso vou deixar para depois da gente se livrar do generateDS.

@rvalyi
Copy link
Member

rvalyi commented Jul 18, 2023

Ola @felipezago eu prometo olhar mais daqui pouco essa PR. No geral não parece mal. Mas seria bom se vc puder fazer squash de alguns commits usando git rebase -i para não ficar fazendo e desfazendo coisas no mesmo PR. Se eu tivesse feito assim, o meu PR com a mudança de generateDS para xsdata teria tido 100 commits, mas eu fiz o esforço de re-organizar os commits para não poluir or repo, todos devemos fazer senão depois na hora das migrações ou entender porque algo foi feito assim (git blame) fica inviável.

Uma outra coisinha que eu vejo que não ta legal eh esse novo arquivo l10n_br_fiscal/tools/formatter.py. Não vale a pena adicionar um arquivo para ter uma so função que chama outra função apenas. Isso é quase um "leaky abstraction". Outra coisa, já que agora tem apenas um arquivo nessa pasta tools depois de extração do l10n_br_fiscal_certificate a gente queria na verdade tirar a pasta tools e botar o arquivo tools.py direitamente no models ou na raiz do modulo (so faz sentido ter pastas quando ha varios arquivos dentro) então por favor de um refatorzinho nessa parte.

O restante eu vou olhar melhor logo. Valeu pelo trabalho.

@felipezago felipezago force-pushed the feature/nfe_import branch 9 times, most recently from 7973f5b to 81e8e73 Compare July 18, 2023 19:15
@hirokibastos
Copy link
Contributor

@hirokibastos pode fazer um rebase?

Rebase feito

@mileo mileo requested a review from rvalyi October 24, 2023 19:23
Copy link
Member

@mileo mileo left a comment

Choose a reason for hiding this comment

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

LGTM: Apesar de entender que algumas coisas devem estar no módulo fiscal, pois servem de base para a importação de outros documentos como CT-E, SAT e etc.

Mas isso da pra ser melhorado quando chegarmos lá.

@mileo
Copy link
Member

mileo commented Oct 24, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-2552-by-mileo-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 11e9c36 into OCA:14.0 Oct 24, 2023
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at d2039c5. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants