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][l10n_br_fiscal][l10n_br_nfe][l10n_br_account][l10n_br_account_nfe] importação dos account.move com as NFe's - contra-proposta #2781

Open
wants to merge 13 commits into
base: 14.0
Choose a base branch
from

Conversation

rvalyi
Copy link
Member

@rvalyi rvalyi commented Nov 9, 2023

As announced, here is the Akretion alternative proposal for #2763, that is for importing account.move accounting and financial data along with NFe's or fiscal documents in general.

@mileo @hirokibastos as I warned you the code is totally different from what you proposed and as you can see I could not reuse even a single line of code. It took me 4 days and much more if considering I already spent lot's of time thinking about this ahead during these years when I designed the localization and specially the l10n_br_account module architecture and data-binding framework/xsdata/nfelib. So since you announced @hirokibastos would give it a try I was absolutely convinced it was out of reach from a beginner to come any close to such design. There was no way I could spend half an hour giving advice (unlike what I did for the dfe, cte, mdfe...) here and here and converging with something as complete and consistent as I'm proposing here:

key features:

  • import NFe's along with their financial account.move (until now only l10n_br_fiscal.document was imported)
  • import a set of several attachments right from the Vendor Bills native upload button (unlike [WIP] Criação de invoice ao importar nota fiscal #2763)
  • import nearly all Brazilian tax values (l10n_br_nfe/models/document_line,py refactor de0d912 ). The tax computation is disabled so these values (amount, base, rate...) are kept as imported. This is done by extracting the bare minimum from [12.0] Opção de chamar ou não o motor de impostos #1412 (it's better to leave the rest up for debate in another PR, specially considering also [14.0] l10n_br_account, l10n_br_fiscal: Possibilidade de informar os valores dos impostos manualmente. #2559 ) and complemented with 1f07ec0 This refactor to import all tax values is one of the reasons the diff is larger but IMHO we had to do it. The code is also a lot cleaner now and I could finally clean a useless/non orthogonal importation method introduced by Gabriel Cardoso on a hurry 3 years ago. We could merge that in a separate PR if that helps...
  • accounting entries for Brazilian taxes are properly generated (not at all the case with [WIP] Criação de invoice ao importar nota fiscal #2763). See next point.
  • account.move(.lines) are generated with odoo.test.common.Form so that all onchanges are called as expected just like if the account.move was filled manually. This avoids spaghetti code and glue modules that would otherwise be needed to play all the onchange of any combo of installed modules. This approach is already used in the OCA for years in modules such as OCA/rma. [WIP] Criação de invoice ao importar nota fiscal #2763 does not address this and open the door to lot's of logic duplication.
  • NFe dups (payment terms) are properly imported but in a much smoother way than [WIP] Criação de invoice ao importar nota fiscal #2763 (avoid useless computations and risk of inconsistent data whenever anything would be recomputed in the account.move.). Proper payable/receivable account_id unlike [WIP] Criação de invoice ao importar nota fiscal #2763
  • built atop of the composite.move refactor (that's a reason why the diff is a bit large but we could merge the composite.move PR first) so that a single account.move can now eventually have different l10n_br_fiscal.document from its invoice_line_ids. This can be useful to import several CTe's or several NFSe's under the same account.move as these fiscal document allow only 1 item line and as this is usually done in other ERP's. At least we are reusing the _onchange_fiscal_document_line_id from this feature to properly complete the account.move.line with their proper onchanges.
  • most of the work is in l10n_br_account so that it will work with any fiscal document (Cte, MDFe...). On the contrary in the [WIP] Criação de invoice ao importar nota fiscal #2763 proposal most of the work was in l10n_br_account_nfe so it was poorly designed to work with other fiscal documents.
  • rename some variables and methods from the previous importation wizard: xml can actually be a file of any format (XML and json are natively supported by xsdata and hence nfelib). Typically we "sniff" the uploaded file and only then determine the format so calling the file xml in 1st place was bad. Other bad names: xml while you were actually dealing with a "binding", calling a document what was actually a document_key... That the problem of letting a random sequence of beginners design a critic API... These renames are also one of the reasons the diff is larger but IMHO they are required.

cc @renatonlima @mbcosta @douglascstd

Now @mileo IMHO you could have anticipated better it would be a dead end to task @hirokibastos on this... Has he even 2 years of Odoo experience? He never got a PR merged yet in the OCA outside from the repo, right? Even KMEE got only one single module accepted outside from l10n-brazil in 10 years (to show the time in the POS...); meanwhile we got over 350... Even @marcelsavegnago got over 20 modules accepted in last 3 years only...

Instead, If you look the guys who design electronic invoicing in Europe, you will find Simone Orsi and Alexis de Lattre for OCA/edi, Alexis de Lattre for OCA/l10n-france, Pedro Baeza for OCA/l10n-spain... All guys with 20 years+ of professional experience and 15 years with Odoo, so there is no way would be any simpler in Brazil were we have the most complex electronic invoicing and tax engine in the world.... So again not @hirokibastos 's fault as I said. And as for "doing it my way", sorry you cannot tell something like that after KMEE did nearly no contribution since v12 during nearly 2 years waiting for us to do all the hard migration work. I also spent countless days helping you a ton for your CTe, MDFe and SPED requirements in the last 3 months... If you look at review or contribution statistics you can see I'm not really the one to blame: https://www.dixmit.com/en/blog/our-blog-1/ranking-of-top-contributors-to-the-odoo-community-association-oca-in-2022-8

So are you sure it's reasonable to point me as the culprit if KMEE beginner PRs are not always merged in OCA/l10n-brazil whenever you need some advanced feature?

Meanwhile, @marcelsavegnago @antoniospneto @felipemotter are living proofs that most serious contributions get very carefully reviews from me and make their way into the project.

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch 3 times, most recently from 6ac7899 to 79875cf Compare November 9, 2023 05:45
@OCA-git-bot
Copy link
Contributor

Hi @mbcosta, @antoniospneto, @felipemotter, @renatonlima,
some modules you are maintaining are being modified, check this out!

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from 79875cf to 8b76d1b Compare November 9, 2023 06:19
@rvalyi rvalyi marked this pull request as draft November 9, 2023 10:59
@rvalyi
Copy link
Member Author

rvalyi commented Nov 9, 2023

botei como "draft", para sinalizar que não estaria 100%. Falta uns detalhes muito pequeno e uns testes, Mas já funciona perfeitamente eu diria que esta 95%.

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from 8b76d1b to 45a614f Compare November 9, 2023 11:16
@rpsjr
Copy link
Contributor

rpsjr commented Nov 9, 2023

@rvalyi , seria possível tratar a importação de nfe com com a tag autXML (vide #2765) neste commit?

A tag autXML traz apenas CNPJs, sem nomes para os contantos.

Acredito que a solução passar por

  1. tratar a extração da utXML para que se possa identificar o nome da pessoa jurídica e formatar os dados para que o contato possas seridentificado/criado

ou

  1. desprezar a informação das pessoas autorizadas, removendo o campo nfe40_autXML

@rvalyi
Copy link
Member Author

rvalyi commented Nov 9, 2023

eu vi seu bug report, mas au ainda não resolvi. Vou olhar... Na pior vc acrescenta duas linhas para remover au autXML do binding antes de jogar no método import,_binding_nfe e aí contorna o problema por enquanto se vc tiver pressa. Mas com certeza vamos resolver.

@felipemotter
Copy link
Contributor

Legal @rvalyi, quero testar esta noite.

@felipemotter
Copy link
Contributor

Fala @rvalyi parabéns pelo trabalho absurdo. Está muito bom.

Fiz uma revisão básica aqui e alguns pontos que levantei:


Sobre a importação:

Como testei com os dados de um cliente modelo que temos, não posso compartilhar o xml que usei para testes, mas tentarei ser o mais claro possível:

Inconsistência 1: Ao tentar importar uma fatura com produtos que tenham unidades de medida(uom) de categorias diferentes (por exemplo: milheiro e quilo), acontece algum conflito bem grande, trocando produtos e até estourando erro em alguns casos. Eu não tive tempo suficiente para entender melhor.

Inconsistência 2: Importando uma nota apenas com uom de mesma categoria de uom, produtos que eram milheiro, por exemplo, acabaram ficando como unidade ao importar o move.

Vale ressaltar que no fiscal ta OK.


Sobre vários documentos fiscais para uma única fatura:

A abordagem parece legal, não testei a fundo, mas tenho uma dúvida: da forma que está colocado, só seria possível lançar uma fatura com vários documentos fiscais caso estes fossem importados, correto? Porque da forma que está hoje, só conseguimos criar um documento fiscal já vinculado a uma fatura.


Para deixar claro, realmente esta PR está muito mais madura e fluída do que a outra proposta.

@rvalyi
Copy link
Member Author

rvalyi commented Nov 10, 2023

A abordagem parece legal, não testei a fundo, mas tenho uma dúvida: da forma que está colocado, só seria possível lançar uma fatura com vários documentos fiscais caso estes fossem importados, correto? Porque da forma que está hoje, só conseguimos criar um documento fiscal já vinculado a uma fatura.

Então meu objetivo aqui era evitar sabotar o projeto com a outra proposta... Acabei não me dedicando tanto na questão do move composto. Mas a gente poderia imaginar várias soluções. Por examplo essa PR já importa uma serie de documentos fiscais. Poderia ter um checkbox no wizard para escolher de juntar no mesmo account move se for possível. Aí ele apenas passaria o account.move criado no primeiro documento para importar os outros documentos. O método import_fiscal_document tem um parâmetro para isso. Poderia tambem escolher um account.move no wizard. Ou então a gente poderia imaginar um botão lá no account.move para abrir o wizard. Poderia tambem ter um Wizard que deixaria escolher um l10n_br_fiscal.document sem ser para importar ele. Aí talvez valeria a pena trazer de volta o menu que permite listar e editar os l10n_br_fiscal.document sem ser pela intermediação do account.move, assim como temos quando vc instala apenas o módulo l10n_br_fiscal antes de instalar o módulo l10n_br_account.

Vou olhar os outros pontos que vc levantou. Vc pode mandar sua nota pelo Telegram ou anonimizar ela e mandar aqui (EDIT: eu saquei o problema da unidade, já já arrumo).

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from 45a614f to d606a21 Compare November 12, 2023 12:04
@rvalyi
Copy link
Member Author

rvalyi commented Nov 12, 2023

@felipemotter tinha algo zoado no wizard de depare da Kmee: logo que importava um produto uma vez zoava a unidade pelo jeito. Eu fiz um workaround no último commit. Agora a uom funciona com sua NFe por examplo.

Eu TB melhorei algumas outras coisas.

@rpsjr eu alterei para não importar mais o autXML, não deve mais criar problema, se puder confirmar...

Bem, ainda não tá perfeito, mas eu acho que não tá longe. Contribuições bem vindas.

@rpsjr
Copy link
Contributor

rpsjr commented Nov 13, 2023

Falha no runboat >>

2023-11-13 00:33:37,232 329 CRITICAL bde23c8e1-a38d-42b6-93c9-7b2fdf651cb5 odoo.service.server: Failed to initialize database bde23c8e1-a38d-42b6-93c9-7b2fdf651cb5.
Traceback (most recent call last):
File "/opt/odoo/odoo/service/server.py", line 1201, in preload_registries
registry = Registry.new(dbname, update_module=update_module)
File "/opt/odoo/odoo/modules/registry.py", line 89, in new
odoo.modules.load_modules(registry._db, force_demo, status, update_module)
File "/opt/odoo/odoo/modules/loading.py", line 459, in load_modules
processed_modules += load_marked_modules(cr, graph,
File "/opt/odoo/odoo/modules/loading.py", line 347, in load_marked_modules
loaded, processed = load_module_graph(
File "/opt/odoo/odoo/modules/loading.py", line 249, in load_module_graph
cr.commit()
File "", line 2, in commit
File "/opt/odoo/odoo/sql_db.py", line 101, in check
return f(self, *args, **kwargs)
File "/opt/odoo/odoo/sql_db.py", line 451, in commit
flush_env(self)
File "/opt/odoo/odoo/sql_db.py", line 78, in flush_env
env_to_flush['base'].flush()
File "/opt/odoo/odoo/models.py", line 5486, in flush
self.recompute()
File "/opt/odoo/odoo/models.py", line 5945, in recompute
process(field)
File "/opt/odoo/odoo/models.py", line 5929, in process
field.recompute(recs)
File "/opt/odoo/odoo/fields.py", line 1176, in recompute
self.compute_value(recs)
File "/opt/odoo/odoo/fields.py", line 1198, in compute_value
records._compute_field_value(self)
File "/opt/odoo/odoo/models.py", line 4082, in _compute_field_value
odoo.fields.determine(field.compute, self)
File "/opt/odoo/odoo/fields.py", line 82, in determine
return needle(*args)
File "/mnt/data/odoo-addons-dir/l10n_br_purchase/models/purchase_order_line.py", line 98, in _compute_amount
line._update_taxes()
File "/mnt/data/odoo-addons-dir/l10n_br_fiscal/models/document_fiscal_line_mixin_methods.py", line 291, in _update_taxes
if self.document_id.imported_document:
AttributeError: 'purchase.order.line' object has no attribute 'document_id'

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch 2 times, most recently from 98836ed to 755c47c Compare November 13, 2023 13:42
@rvalyi
Copy link
Member Author

rvalyi commented Nov 13, 2023

@rpsjr foi corrigido.

Pessoal, o problema principal que sobra é que tem algumas NFe's que o total não bate. Parece ser algo relacionado a soma de frete ou IPI no amount_total do account.move. Quando importa a NFe sem ter o modulo l10n_br_account instalado importa os totais certinho, mas com o l10n_br_account os totais não batem mais. Ai ele ate não importa as duplicatas nesse caso. Se alguém conseguir ajudar a resolver...

cc @renatonlima @marcelsavegnago @antoniospneto @felipemotter @mbcosta @hirokibastos

@rvalyi
Copy link
Member Author

rvalyi commented Nov 13, 2023

mais informações sobre a questão dos totais:

  1. numa importação de NFe apenas com o modulo l10n_br_nfe instalado o amount_total da NFe bate porque temos nfe40_vNFe = related("amount_total") ai ao importar escreve o valor certo no amount_total sem recalcular o total, Mas logo que recalcula o total fica zoado se a nota tiver IPI ou frete por examplo.

  2. o ponto é que o amount_total das linhas não é correto. Pois normalmente dentro do metodo _compute_amounts do l10n_br_fiscal/models/document_fiscal_line_mixin_methods.py temos:

    def _compute_amounts(self):
        [....]
            record.amount_tax = record.amount_tax_not_included

            add_to_amount = sum([record[a] for a in record._add_fields_to_amount()])
            rm_to_amount = sum([record[r] for r in record._rm_fields_to_amount()])

            # Valor do documento (NF)
            record.amount_total = (
                record.amount_untaxed + record.amount_tax + add_to_amount - rm_to_amount
            )

Mas este calculo do amount_total não esta considerando os valores amount_tax, add_to_amount e rm_to_amount. Parece que estes ultimos campos estão zerados apesar da gente importar IPI ou frete corretamente por examplo. Então eu acho que o caminho é corrigir o codigo para que esses valores sejam calculados ou importados corretamente para depois a soma do amount_total ficar OK. A gente poderia até pensar em forçar o valors do vNF no amount_total do account.move. Mas ai ele não iria bater com o valor das linhas/lançamerntos e ai ele nem iria salvar.

@felipemotter
Copy link
Contributor

Realmente @rvalyi a minha preocupação é semelhante a questão que o magno levantou aquela vez, para forçar valores igual ao fiscal, porém o contábil continuar errado(princiál impacto seria o financeiro).

@felipemotter
Copy link
Contributor

felipemotter commented Nov 13, 2023

Naquela época, eu até pensei em fazer algumas validações para que o valor total batesse com o fiscal, porém acabei não tendo tempo para tal. Isso forçaria a corrigir os problemas e com o tempo ficaria liso (o risco seria travar o faturamento de um usuário que estivesse com urgência)

@rpsjr
Copy link
Contributor

rpsjr commented Nov 14, 2023

@rpsjr foi corrigido.

Não consegui validar a correção nesta instância: http://oca-l10n-brazil-14-0-pr2781-755c47cf259a.runboat.odoo-community.org/

Quero dizer, o erro persiste.

@rvalyi
Copy link
Member Author

rvalyi commented Nov 14, 2023

@felipemotter so para registrar, eu matei a charada. pqp viu, esse tava digno daquele bug que a gente ficou cassando no inicio da v14... Assim que eu tiver tempo eu limpo um commit para subir...

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch 2 times, most recently from 0af4bf0 to abae60d Compare November 14, 2023 15:29
@rvalyi
Copy link
Member Author

rvalyi commented Nov 14, 2023

ai @felipemotter da uma testada com a sua NFe: é perfeito ou não é?

Screenshot from 2023-11-14 12-27-14
Screenshot from 2023-11-14 12-27-42

Screenshot from 2023-11-14 12-33-14

cc @antoniospneto @marcelsavegnago @renatonlima @mbcosta @hirokibastos @rpsjr

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from abae60d to b1dc5a2 Compare November 14, 2023 15:36
@rvalyi
Copy link
Member Author

rvalyi commented Nov 14, 2023

@felipemotter eu tive esses prints localmente com o modulo l10n_br_account_nfe instalado. Porem ao testar com a mesma nota no Runboat (depois eu detonei) eu tive problema com as unidades e valores das duas primeiras linhas da NFe, como se tivesse rolado alguma conversão de unidade. Eu ainda não sei qual a razão disso no Runboat, mas devemos estar perto pois aqui apenas com l10n_br_account_nfe a note foi um espelho perfeito eu acho.

EDIT: algo que eu vi é que no Runboat as linhas account.move.line vem numa ordem diferente do meu localhost. Tem lugar no PR onde eu preciso da ordem das linhas. Talvez eu preciso ordenar por id ou ver esse ponto da ordenação melhor...

@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from 8582b95 to fe382e7 Compare April 19, 2024 17:25
@rvalyi
Copy link
Member Author

rvalyi commented Apr 19, 2024

pessoal, dei um rebase ja que os sub-PR que eram dependencia entraram todos agora. Mas seria legal eu incluir uns testes unitários ainda. Vou tentar faze-lo esses dias que vem.

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 18, 2024
@github-actions github-actions bot closed this Sep 22, 2024
@rvalyi rvalyi reopened this Oct 1, 2024
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 6, 2024
@rvalyi rvalyi force-pushed the 14.0-nfe-account-move-import branch from f2554fd to 85bdeab Compare October 20, 2024 15:55
@rvalyi
Copy link
Member Author

rvalyi commented Oct 21, 2024

pessoal, dei um rebase, comecei a escrever uns testes, esses dias devo subir os testes e ai a gente vai poder fazer o merge e encerrar essa novela...

@mileo
Copy link
Member

mileo commented Oct 28, 2024

@rvalyi dei uma testada aqui localmente e segue funcionando. Senti falta de uma funcionalidade para gerar a fatura a partir de um documento fiscal já importado.

No wizard tem duas opções (importar documento e importar fatura), se o usuário importa o documento (ou o módulo de dfe faz isso) seria legal dar pra gerar um account.move a partir do doc importado.

Até por que em alguns casos isso pode mudar o lançamento contábil, a partir de algum ajuste feito pelo usuário (compra normal que vai ser transformada em um ativo imobilizado).

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.

7 participants