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][REF] l10n_br_fiscal, l10n_br_account: Cálculo e Visualização das Alíquotas Efetivas do Simples Nacional #2822

Draft
wants to merge 4 commits into
base: 14.0
Choose a base branch
from

Conversation

antoniospneto
Copy link
Contributor

@antoniospneto antoniospneto commented Dec 4, 2023

Migração da PR #1831 (12.0)

Esta PR refatora a forma como as alíquotas do Simples Nacional são calculadas. A atualização visa principalmente resolver o problema que não permitia que uma mesma empresa exercesse atividades de indústria e comércio simultaneamente, e melhorar a eficiência do cálculo. Além disso, foi aprimorado o comentário sobre o aproveitamento de crédito do ICMS incluído nas notas fiscais de venda para empresas no regime tributário Simples Nacional, tornando-o mais dinâmico para cobrir todos os casos. Essa mudança traz benefícios como maior clareza na visualização das alíquotas efetivas calculadas para a empresa do Simples Nacional.

@antoniospneto antoniospneto changed the title [14.0] l10n_br_fiscal, l10n_br_account: Refatoração do Cálculo e Visualização das Alíquotas Efetivas do Simples Nacional [14.0][REF] l10n_br_fiscal, l10n_br_account: Cálculo e Visualização das Alíquotas Efetivas do Simples Nacional Dec 4, 2023
@antoniospneto antoniospneto marked this pull request as draft December 4, 2023 01:31
@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch 3 times, most recently from 6989f7e to e7a3931 Compare December 6, 2023 00:10
@antoniospneto antoniospneto marked this pull request as ready for review December 6, 2023 00:27
@antoniospneto
Copy link
Contributor Author

PR pronta para revisão

cc @mileo @renatonlima

@mileo
Copy link
Member

mileo commented Dec 8, 2023

Pode verificar os conflitos?

@antoniospneto
Copy link
Contributor Author

Pode verificar os conflitos?

Opa, esses arquivos do README não era pra eu ter subido junto, já arrumo

@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch 2 times, most recently from 69a9a39 to 1010ce2 Compare December 8, 2023 18:44
rvalyi
rvalyi previously requested changes Dec 9, 2023
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.

Não domino bem essa parte funcional para opinar.
Sobre a parte técnica para mim ta quase tudo top, mas tem 2 create para arrumar e perguntei uma coisa sobre a herança do tax_framework tb.

l10n_br_fiscal/models/res_company.py Show resolved Hide resolved
l10n_br_fiscal/models/simplified_tax.py Outdated Show resolved Hide resolved
@antoniospneto antoniospneto marked this pull request as draft December 9, 2023 14:45
@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch from 1010ce2 to d2b285a Compare December 14, 2023 17:50
@antoniospneto antoniospneto requested a review from rvalyi December 14, 2023 18:07
@antoniospneto antoniospneto marked this pull request as ready for review December 14, 2023 18:07
@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). 🤖

@mileo
Copy link
Member

mileo commented Mar 15, 2024

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-2822-by-mileo-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 15, 2024
Signed-off-by mileo
@OCA-git-bot
Copy link
Contributor

@mileo your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-2822-by-mileo-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rvalyi
Copy link
Member

rvalyi commented Mar 15, 2024

@mileo @antoniospneto esse PR é bom o @renatonlima revisar porque até onde me falou não achou legal...
@renatonlima consegue se posicionar sobre esse PR? Já faz muito tempo...

@mileo
Copy link
Member

mileo commented Mar 15, 2024

@mileo @antoniospneto esse PR é bom o @renatonlima revisar porque até onde me falou não achou legal... @renatonlima consegue se posicionar sobre esse PR? Já faz muito tempo...

É uma bug importante, será que você lembra oq o @renatonlima falou @rvalyi ?

@mileo
Copy link
Member

mileo commented Mar 25, 2024

@rvalyi sem novidades da parte do @renatonlima ? Podemos prosseguir com o merge e depois se necessário ele abre um issue?

@marcelsavegnago
Copy link
Member

@renatonlima consegue liberar um tempinho e dar retorno aqui por favor ?

@marcelsavegnago
Copy link
Member

marcelsavegnago commented Mar 25, 2024

@rvalyi minha sugestão neste caso para não ficar a incognita do que não está legal para o @renatonlima seria vc pegar com ele e nos passar.. o que acha ?

@rvalyi
Copy link
Member

rvalyi commented Mar 25, 2024

pessoal, o @renatonlima tava numa corrida importante até hoje para um cliente que vai migrar da 12 para a 16. Vou ver com ele se vai aliviar agora. Eh que vejam, é ruim algum design problematico entrar, ter port nas outras branches e depois a gente esquecer, começar a ter modulos que vão depender da nova forma, depois a gente ter que corrigir em todas as branches etc...
Todo mundo é adulto aqui e sabe usar o git-aggregate nesse tempo...

@mileo
Copy link
Member

mileo commented Mar 26, 2024

pessoal, o @renatonlima tava numa corrida importante até hoje para um cliente que vai migrar da 12 para a 16. Vou ver com ele se vai aliviar agora. Eh que vejam, é ruim algum design problematico entrar, ter port nas outras branches e depois a gente esquecer, começar a ter modulos que vão depender da nova forma, depois a gente ter que corrigir em todas as branches etc... Todo mundo é adulto aqui e sabe usar o git-aggregate nesse tempo...

Enquanto isso quem não sabe e quer testar a localização, percebe que a tributação esta incorreta. Vamos ver se o @renatonlima tem um tempinho essa semana.

@rvalyi
Copy link
Member

rvalyi commented Mar 26, 2024

Eh que não é um corretivo simples de 10 linhas, toca 23 arquivos, adiciona 442 linhas e tira 123. Nisso eu acho indispensável o Renato que fez uns 80% do modulo esses 10 anos poder validar com calma. Já teve varios casos, como PR em volta dos registros dummy onde quem não tinha muita experiencia quis propor soluções que a gente vé hoje que não era legal. Vc mesmo @mileo tem uma boa dezena de PR que vc insistia para botar e que ficou claro que não era coisas legais para não dizer outra coisa.

O PR ta aqui uns 4 meses, é bastante mas tb é não é nada louco para um fix desse tamanho na OCA. @mileo eu acho que tem modulos ou libs que vcs se dizem mantenedores que vcs deixaram PRs de fix críticos sem revisão por muito mais tempo do que isso e nem ajudaram em muita coisa no projeto durante os ultimos 3 anos, em especial quando a localização teve um dos maiores desafios qdo migramos o modulo l10n_br_account para a v14 para fazer essa pressão toda né...

De qualquer forma, é importante explicar para a galera que tem sim que botar a mão na massa e que vai precisar fazer merge de alguns PRs, seja aqui, seja para varios outros modulos da OCA. A diferença é que depois qdo tiver que atualizar, vai impactar apenas quem teve que usar o PR e não todo mundo, em especial não quem faz a manutenção do projeto.
A principio o Renato deve ter mais tempo logo.

@mileo
Copy link
Member

mileo commented Mar 26, 2024

Eh que não é um corretivo simples de 10 linhas, toca 23 arquivos, adiciona 442 linhas e tira 123. Nisso eu acho indispensável o Renato que fez uns 80% do modulo esses 10 anos poder validar com calma. Já teve varios casos, como PR em volta dos registros dummy onde quem não tinha muita experiencia quis propor soluções que a gente vé hoje que não era legal. Vc mesmo @mileo tem uma boa dezena de PR que vc insistia para botar e que ficou claro que não era coisas legais para não dizer outra coisa.

O PR ta aqui uns 4 meses, é bastante mas tb é não é nada louco para um fix desse tamanho na OCA. @mileo eu acho que tem modulos ou libs que vcs se dizem mantenedores que vcs deixaram PRs de fix críticos sem revisão por muito mais tempo do que isso e nem ajudaram em muita coisa no projeto durante os ultimos 3 anos, em especial quando a localização teve um dos maiores desafios qdo migramos o modulo l10n_br_account para a v14 para fazer essa pressão toda né...

De qualquer forma, é importante explicar para a galera que tem sim que botar a mão na massa e que vai precisar fazer merge de alguns PRs, seja aqui, seja para varios outros modulos da OCA. A diferença é que depois qdo tiver que atualizar, vai impactar apenas quem teve que usar o PR e não todo mundo, em especial não quem faz a manutenção do projeto. A principio o Renato deve ter mais tempo logo.

Muitos PRs que fizemos nos últimos anos apesar alguns não terem sido feito o merge, pois foram feitas propostas alterativas, são problemas que precisam de solução.

E várias delas não estavam nem no radar de ninguém por aqui, visto que nenhum issue foi aberto para falar do assunto.

Esta é inclusive uma delas, que a primeira abordagem de melhoria foi feita pela gente faz um tempo #1238 e nem sempre as soluções são perfeitas de primeira, da pra citar muitas delas em todos os módulos que mantemos, mas a ideia é evoluir.

@rvalyi
Copy link
Member

rvalyi commented Mar 26, 2024

"solução alternativa" de 10 ou 20 linhas para problema critico é tranquilo fazer merge. O problema é um problema de "entropia": não da para experimentar com PR de 400 linhas na branch principal, isso é uma tremenda falta de respeito para quem faz a imensa maioria do trabalho nos tais módulos. Eh como vc apertar a pasta de dente e pensar que se mudar de ideia a vc vai desapertar e a pasta vai volta no tubo. Ela não volta no tubo. Isso é uma questão de entropia e é algo básico da engenharia de software. Da mesma forma que como não tinha muita "entropia" foi facil substituir o generateDS pelo xsdata para emitir NFe. Se fosse da forma como a gente fazia tudo manual como na v8 so que 5x mais completo e com cada um que bota seu fix qdo precisa, esse tipo de troca vc não fazia mais nem fodando (e alias é por isso que digo boa sorte pros caras que vão confiar na Odoo SA para a parte fiscal).

@rvalyi
Copy link
Member

rvalyi commented Mar 26, 2024

Alias @mileo vc quase não fez mais codigo os 3 ultimos anos porque vc escolhou de delegar para desenvolvedores iniciantes. Mas ainda assim vc sempre insiste para querer dar palpite de como deveriam ser as coisas no projeto... Me corrige se eu tiver errado, mas em 10 anos, a KMEE conseguiu emplacar apenas 1 modulo em algum outro repo da OCA que é esse modulo grandioso: https://github.com/OCA/pos/tree/14.0/pos_show_clock
Eu esqueci outros modulos? Vc não acha que se vc soubesse tão bem como fazer as coisas como pretende aqui vc teria conseguido emplacar algum outro modulo talvez? Procure os modulos da Akretion ou mesmo da Escodoo que começou a contribuir nem 4 anos atras que vc vai ver o que é ter plebiscito...

Novamente sobre o PR, eu acho ele bem feito e geralmente confio nas coisas do Antonio. Porem esse tipo de coisa o Renato precisa ver com calma.

@@ -376,7 +376,6 @@ def _compute_taxes_mapped(self, base_line):
fiscal_price=base_line.fiscal_price,
fiscal_quantity=base_line.fiscal_quantity,
uot_id=base_line.uot_id,
icmssn_range=base_line.icmssn_range_id,
Copy link
Member

Choose a reason for hiding this comment

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

Eu havia mantido a informação da faixa do anexo porque uma empresa do simples pode ter mais de um anexo, o que facilita depois segregar o faturamento de cada anexo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@renatonlima Entendi, vou trazer essa informação de volta.

@@ -349,7 +349,7 @@ def _get_price_total_and_subtotal(
fiscal_price=self.fiscal_price,
fiscal_quantity=self.fiscal_quantity,
uot_id=self.uot_id,
icmssn_range=self.icmssn_range_id,
cfop=self.cfop_id,
Copy link
Member

Choose a reason for hiding this comment

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

Além de utilizar para segregar o faturamento por anexos do simples, eu acho melhor usar a faixa do anexo ao invés do CFOP porque no caso de notas de serviços não tem CFOP, Também no caso de serviços deveria ter uma relação entre o tipo de serviço e o CNAE para ser preenchido automático.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certo, vou por o icmssn_range de volta e calcular ele com base no CFOP no caso de produtos e no caso do serviços com base no CNAE

from odoo import api, fields, models


class SimplifiedTaxEffective(models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

Eu acho necessário ter um objeto para armazenar os anexos e as faixas atuais, mas o calculo dos percentuais efetivos poderia estar encapsulado no objeto l10n_br_fiscal.simplified.tax.range e os valores calculados no _compute_icmssn do objeto l10n_br_fiscal.tax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, acho que é tranquilo fazer dessa forma também, vou ajustar a PR para propor assim.

"based on the range the company currently falls into.",
)

icmssn_credit_industry = fields.Float(
Copy link
Member

Choose a reason for hiding this comment

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

Não acho necessário existir os campos icmssn_credit_industry e icmssn_credit_commerce se os valores forem calculados no método _compute_icmssn do objeto l10n_br_fiscal.tax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optei por colocar essa informação no modelo da empresa, pois ela não depende de outras informações contidas no modelo l10n_br_fiscal.tax, que é mais complexo. A intenção é reduzir um pouco a complexidade do modelo l10n_br_fiscal.tax e manter a simplicidade e clareza no código.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah eu tbm faço o uso dessa informação para gerar o comentário do direito ao crédito que vai nas informações complementares da nfe do simples nacional, veja:

${format_number(doc.company_id.icmssn_credit_commerce)}%

@antoniospneto
Copy link
Contributor Author

Estou colocando a PR em rascunho para fazer as melhorias propostas pelo @renatonlima.
Notifico assim que estiver pronto para revisão novamente.

Obrigado pela revisão, @renatonlima!

@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch from 0d5c17b to 5b38068 Compare August 29, 2024 00:16
@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch 2 times, most recently from d306f67 to 9352e5f Compare September 9, 2024 23:09
@antoniospneto antoniospneto force-pushed the 14.0-ref-simples-nacional branch from 9352e5f to 0a61627 Compare September 14, 2024 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants