search

OCA / l10n-brazil

Localização brasileira oficial do Odoo.
https://odoo-community.org/psc-teams/brazil-66
GNU Affero General Public License v3.0
235 stars 244 forks source link

[14.0][IMP] l10n_br_fiscal: Resolvendo o problema de cálculo da estimativa de imposto nacional e importada #3152

Open corredato opened 2 months ago

corredato commented 2 months ago

Os campos estimate_tax_national e estimate_tax_imported dentro dos registros do NCM não estavam sendo calculados corretamente para o cálculo dos impostos estimados na linha da fatura.

rvalyi commented 2 months ago

os testes estão falhando no projeto porque ta puxando a nova nfelib 2.1.1 que nao tem mais os bindings legacy generateDS, eu comentei aqui https://github.com/OCA/l10n-brazil/pull/3150 ou seja não tem a ver com o fix. Eu acho que eu consigo arrumar amanhã.

rvalyi commented 2 months ago

@corredato vc poderia dar um rebase da sua branch em cima da 14.0 atualizada e fazer um force push da sua branch. Isso vai resolver o problema dos testes com a versão da nfelib e vai permitir da gente avaliar melhor o seu PR...

mileo commented 1 month ago

Se for pra manter dessa forma vai ser necessário um script de migração.

rvalyi commented 1 month ago

@mileo da para vc dar uma prioridade para se posicionar sobre esse PR por favor? Se trata de bastante codigo bem central... https://github.com/OCA/l10n-brazil/pull/3081

mileo commented 1 month ago

@renatonlima pode revisar novamente?

mileo commented 1 month ago

No geral acho bem vinda essa alteração, realmente a configuração dos impostos estimados tem que ser por empresa, conforme o @mileo explicou.

Minha unica sugestão é alterar o nome do método de update_compute_amount para update_estimated_taxes

Feito

antoniospneto commented 1 month ago

@mileo acho que é necessário script de migração também não?

mileo commented 1 month ago

@mileo acho que é necessário script de migração também não?

Vamos providenciar.

mileo commented 1 month ago

@mileo acho que é necessário script de migração também não?

Verificando com o time eles já tinham feito, eu acho q eu sobrescrevi a branch :@

mileo commented 1 month ago

@mileo acho que é necessário script de migração também não?

Pronto para revisão:

@renatonlima @antoniospneto @marcelsavegnago @rvalyi

rvalyi commented 1 month ago

pessoal, eh bom verificar a colocaçao do @renatonlima aqui https://github.com/OCA/l10n-brazil/pull/3152#discussion_r1698647061

antoniospneto commented 1 month ago

O design desse PR está errado como expliquei no meu contrário anterior, os dados dos impostos aproximados são igual para todos regimes tributário da mesma UF abaixo segue a explicação no FAQ do IBPT

image

Mas e se as empresas forem de estados diferentes? Será que o hoje o sistema suporta esse caso de uso? Será que esse é o seu caso de uso @mileo ?

mileo commented 1 month ago

O design desse PR está errado como expliquei no meu contrário anterior, os dados dos impostos aproximados são igual para todos regimes tributário da mesma UF abaixo segue a explicação no FAQ do IBPT image

Mas e se as empresas forem de estados diferentes? Será que o hoje o sistema suporta esse caso de uso? Será que esse é o seu caso de uso @mileo ?

No inicio começamos resolvendo o BUG do multi company, onde uma empresa tinha impostos estimados e a outra ficava com o valor zerado.

Então separamos a implementação por empresa para facilitar o controle individualmente.

Conforme entendimento dos comentários acima cada estado pode ter seus totais estimados, então vai ser necessário novas melhorias, não que atualmente esteja errado, só acaba tendo uma certa duplicidade de dados no cenário multi company.

Podemos fazer o merge desse PR e providenciar essa melhoria posteriormente, ou manter o bug e o PR em aberto até termos tempo de implementar isso.

renatonlima commented 1 month ago

@antoniospneto e @mileo

O objeto l10n_br_fiscal.tax.estimate tem o campo state_id, inclusive existe o campo company_id que poderia ser removido porque esse objeto não tem regra de segurança. (pelo menos não deveria ter)

A ideia de colocar nos objetos l10n_br_fiscal.ncm e l10n_br_fiscal.nbm os campos calculados estimate_tax_national, estimate_tax_imported era para ficar fácil nos cálculos na linha do documento fiscal fazer line.ncm_id. estimate_tax_national e já acessar o último percentual dos impostos aproximados.

Na maioria dos casos quando existe uma empresa ou várias empresas (multi-company) mas na mesma UF vai funcionar, mas se existir empresas em diferentes UFs, isso já não vai funcionar, deixar o campo property até resolveria o problema mas vai duplicar esses valores seja nos casos de empresas na mesma UF ou em UFs diferentes.

Acho que existem dois caminhos:

  1. Deixar os campos estimate_tax_national e estimate_tax_imported como campos calculados store=False, (Eu não sinto que seja a melhor solução) porque não sei se isso vai gerar algum impacto significativo na performance, pois sempre que acessar os objetos os campos serão calculados em tempo real.

  2. Remover os campos calculados estimate_tax_national e estimate_tax_imported nos objetos l10n_br_fiscal.ncm e l10n_br_fiscal.nbm, no objeto l10n_br_fiscal.tax.estimate remover o campo company_id e implementar um campo active para manter somente o registro atual visível de cada estado, dessa forma o usuário vai conseguir visualizar os impostos aproximados de cada UF utilizada no sistema e ter um método nos objetos l10n_br_fiscal.ncm e l10n_br_fiscal.nbm para retornar esses dados no calculo dos impostos.

@mileo,

Não acho bom fazer o merge desse PR da forma como esta agora, pois vai mudar a estrutura de dados, mesmo com o script de migração, depois vai ter que ser feito outro PR e outro script de migração, é mais simples já corrigir de forma adequada o bug e fazer o merge.