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

[12.0] l10n_br_nfe: Adição do wizard de importação de nf-e por xml #1985

Merged
merged 6 commits into from
Jul 15, 2022

Conversation

hirokibastos
Copy link
Contributor

Divisão de #1701

Este PR contém a parte principal da importação de nfes. Aqui estão inclusos o wizard de importação, mudanças nas visões e testes.

Além disso foi necessário um pequeno ajuste no campo nfe40_InfRespTec para que a importação funcione. Existe um problema na importação de campos computados e relacionais, vou criar um issue para discuti-lo. Este problema, porém, não atrapalha em nada o funcionamento do resto da importação.

Para realizar a importação basta usar este menu no módulo fiscal

Screenshot from 2022-06-15 12-21-12

@hirokibastos hirokibastos reopened this Jun 15, 2022
@hirokibastos hirokibastos marked this pull request as draft June 15, 2022 15:34
@hirokibastos hirokibastos force-pushed the feature/import_xml_wizard branch from 3f85c59 to 7b4b7e7 Compare June 15, 2022 15:39
@rvalyi
Copy link
Member

rvalyi commented Jun 15, 2022

@hirokibastos obigado pela quebra do PR, ficou bem mais digesto assim. Vc pode ver que o que foi trivial a gente ja conseguiu integrar facilemente (bem mais do que um pedaço grande).

Agora por favor se liga com esses 3 PRs para mudar o framework de binding do generateDS pro xsdata como eu tinha explicado durante os OCA Days o ano passado https://github.com/OCA/l10n-brazil/pulls?q=is%3Apr+is%3Aopen+xsdata

Do que eu vi dos seus PRs de NFe deve ficar bem compativel, mas é interessante vcs dar uma estudada nessas PRs mesmo que bem de rascunho ainda, porque:

  1. para ter revisões é bom fazer tb
  2. temos que testar muito essas mudanças pro xsdata para não ter regressão ao emitir NFe
  3. o ganho da mudança pro xsdata pode ser muito grande em especial para coisas que a KMEE tinha interesse como DFs, CTe, MDFe etc.. Eh importante tb acertar essa mudança antes de criar um bicho de 7 cabeças com a importação de Nfe em cima do generateDS (ate onde eu vi ta OK do que vcs fizeram).
  4. a ideia inicial é fazer na 14.0 onde de qualquer jeito nao tem bem muita gente ja usando a NFe por meses que poderia quebrar a cara com isso. Mas logo que tiver bem 100% na 14.0 a ideia seria fazer um backport para a v12 para poder baixar o custo da manutençao da NFe nas duas branches e porque do nosso lado os nossos clientes que estão bancando isso estão na 12 por alguns meses ainda... Mas ate por isso pode deixar não vamos fazer nenhuma loucura na 12.0.

@hirokibastos
Copy link
Contributor Author

@hirokibastos obigado pela quebra do PR, ficou bem mais digesto assim. Vc pode ver que o que foi trivial a gente ja conseguiu integrar facilemente (bem mais do que um pedaço grande).

Agora por favor se liga com esses 3 PRs para mudar o framework de binding do generateDS pro xsdata como eu tinha explicado durante os OCA Days o ano passado https://github.com/OCA/l10n-brazil/pulls?q=is%3Apr+is%3Aopen+xsdata

Do que eu vi dos seus PRs de NFe deve ficar bem compativel, mas é interessante vcs dar uma estudada nessas PRs mesmo que bem de rascunho ainda, porque:

1. para ter revisões é bom fazer tb

2. temos que testar muito essas mudanças pro xsdata para não ter regressão ao emitir NFe

3. o ganho da mudança pro xsdata pode ser muito grande em especial para coisas que a KMEE tinha interesse como DFs, CTe, MDFe etc.. Eh importante tb acertar essa mudança antes de criar um bicho de 7 cabeças com a importação de Nfe em cima do generateDS (ate onde eu vi ta OK do que vcs fizeram).

4. a ideia inicial é fazer na 14.0 onde de qualquer jeito nao tem bem muita gente ja usando a NFe por meses que poderia quebrar a cara com isso. Mas logo que tiver bem 100% na 14.0 a ideia seria fazer um backport para a v12 para poder baixar o custo da manutençao da NFe nas duas branches e porque do nosso lado os nossos clientes que estão bancando isso estão na 12 por alguns meses ainda... Mas ate por isso pode deixar não vamos fazer nenhuma loucura na 12.0.

Estou dando uma olhada nesses 3 PRs quando tenho tempo, mas meu conhecimento sobre qual seria o impacto dessas mudanças é bastante limitado. De qualquer forma, assim que terminar de passar por tudo deixo minha revisão lá.

@rvalyi
Copy link
Member

rvalyi commented Jun 15, 2022

Expliquei um pouco nos PR: o impacto vai ser muito mais na manutenção e capacidade dos bindings como nfelib do que no código nesse repo OCA/l10n-brazil onde a alteraçao é ate bastante transparente. Mas deveria ajudar tb a tirar 50% ou ate eliminar erpbrasil.edoc que é tb um ponto fraco da arquitetura atual, sendo um ponto fraco único onde a modularidade é quebrada porque qualquer mudanças na transmissão de qualquer documento fiscal impacta todos outros documentos fiscais, basta o cara fazer uma melhoria digamos de NFSe de familia X para impactar todos outros documentos fiscais e tb essa lib tem uma manutençao muito aleatória hoje.

@hirokibastos hirokibastos marked this pull request as ready for review June 15, 2022 16:24
@@ -47,6 +47,10 @@ class NFeLine(spec_models.StackedModel):
related="product_id.barcode",
)

nfe40_xProd = fields.Char(
related="product_id.name",
)
Copy link
Member

Choose a reason for hiding this comment

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

Essa parte não ta legal porque no Odoo a gente pode vender algo sem ter um produto. Ou o usuario pode alterar o label da linha. tem que procurar um pouco mas ja tinhamos conversado sobre. cc @renatonlima

Copy link
Contributor

Choose a reason for hiding this comment

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

concordo com o @rvalyi , principalmente nos lançamentos de notas de entrada, acho um pouco ruim criar cadastro de produto pra qualquer coisa, tem muita coisa que não tem necessidade, principalmente itens de uso e consumo que não tem controle de estoque.

Copy link
Contributor Author

@hirokibastos hirokibastos Jun 17, 2022

Choose a reason for hiding this comment

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

Como a resolução deste problema foge do escopo deste PR, apenas removi essas linhas daqui e criei #1986 com a mudança necessária.

Copy link
Member

Choose a reason for hiding this comment

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

@hirokibastos perfeito! Vamos analisar quanto antes.

@marcos-mendez
Copy link

marcos-mendez commented Jun 15, 2022 via email

@rvalyi
Copy link
Member

rvalyi commented Jul 5, 2022

@hirokibastos foi feito o merge do #1986 vc poderia fazer um rebase dessa PR por favor?

@hirokibastos hirokibastos force-pushed the feature/import_xml_wizard branch from cd62e70 to 874ad3e Compare July 5, 2022 13:43
@rvalyi rvalyi requested a review from renatonlima July 6, 2022 15:39
@rvalyi
Copy link
Member

rvalyi commented Jul 6, 2022

@hirokibastos valeu pelo rebase. Eh legal que parece que não vai ter impacto, mas vale a pena vc dar uma olhada tb nesse refator que ta sendo proposto (work in progress) para organizar melhor os mapeamento das linhas de NFe para deixar o código mais fácil de entender e manter #2000
De qualquer forma a gente vai dar mais retorno sobre essa PR em breve, mas vendo por cima eu acho legal, se a gente pede mudança não deveria ser grande coisas.

@hirokibastos
Copy link
Contributor Author

@hirokibastos valeu pelo rebase. Eh legal que parece que não vai ter impacto, mas vale a pena vc dar uma olhada tb nesse refator que ta sendo proposto (work in progress) para organizar melhor os mapeamento das linhas de NFe para deixar o código mais fácil de entender e manter #2000 De qualquer forma a gente vai dar mais retorno sobre essa PR em breve, mas vendo por cima eu acho legal, se a gente pede mudança não deveria ser grande coisas.

Estou acompanhando esse PR de perto, também acredito que não vai haver nenhum conflito. Obrigado pelo feedback, fico no aguarda das revisões.

@marcelsavegnago
Copy link
Member

marcelsavegnago commented Jul 14, 2022

@hirokibastos como ficaria no caso de um pedido de compra que demanda da geracao de uma fatura de compra ou que seja um outro processo que tbm vai contar com um botao gerar fatura de compra.. eh possivel linkar oq foi importado com uma fatura destas ?

@rvalyi
Copy link
Member

rvalyi commented Jul 14, 2022

@marcelsavegnago eu tinha prototipado a importação do invoice 3 anos atras no PR #753 que introduzia o framework.
Eu pretendo resgatar esse código e testar um pouco com as melhorias dessa PR. Mas TB é importante o @renatonlima poder avaliar melhor essa PR pois ele ficou trabalhando nesse assunto TB esses dias.

Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

Eu testei e me parece bastante razoável. Poderiam ser feitas melhorias mas nada que justificaria de atrasar o merge na minha opinião. Então parabens pelo trabalho @hirokibastos !

Testando eu detetei e corrigi esse pequenos problema no erpbrasil.edoc.base erpbrasil/erpbrasil.base#31

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@rvalyi
Copy link
Member

rvalyi commented Jul 15, 2022

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-1985-by-rvalyi-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 14135be into OCA:12.0 Jul 15, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5b0ad93. 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.

8 participants