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

[16.0] l10n_br_fiscal: Renomeação de métodos compute para evitar ambiguidades em heranças de modelos #3549

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

antoniospneto
Copy link
Contributor

@antoniospneto antoniospneto commented Dec 17, 2024

Esta PR renomeia os métodos compute responsáveis pelos montantes no documento e nas linhas do documento fiscal. A alteração visa evitar ambiguidades e possíveis conflitos durante heranças em modelos nativos do Odoo, que podem surgir devido a nomes de métodos semelhantes.

Extraido da PR #3532 [1]

@OCA-git-bot
Copy link
Contributor

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

Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

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

@mbcosta
Copy link
Contributor

mbcosta commented Dec 17, 2024

Algumas considerações:

  • talvez seja melhor deixar claro que se trata do caso Brasil por exemplo _compute_br_amount ou _compute_br_fiscal_amount ou mesmo _compute_brazil_amount para quem trabalha na Localização pode ser obvio do se trata, mas em casos multi-localizações para desenvolvedores de outros países isso pode não ser tão obvio já que Fiscal é algo genérico é nesse caso é o Fiscal do Brasil

  • na v14 eu estava fazendo testes e acabei vendo que ao chamar o método __compute_amount o método do account.move afetava o teste e para resolver era preciso chamar apenas o _compute_amount do Fiscal mas ao invés de renomear o método eu crie um novo e passei todo o código para lá e no antigo método eu chamo esse novo método, isso mantém a compatibilidade sem grandes alterações assim os casos _compute_amount continuam funcionando mas quando é necessário permite chamar apenas o do Fiscal

    def _compute_amount(self):
        self._compute_br_amount()

    def _compute_br_amount(self):
        fields = self._get_amount_fields()
        for doc in self:
            values = {key: 0.0 for key in fields}
            for line in doc._get_amount_lines():
  • Existe um ROADMAP de deixar a Localização compatível com o caso Multi-Localização e alguns dos casos Compute acabam não retornando o super(), isso também ocorre na v14, mas é bom ter isso como ROADMAP para lembrar que pode existir o caso que não é o Fiscal Brasil, hoje é usado o campo Operação Fiscal/fiscal_operation_id para identificar quando é ou não esse caso.

  • O método _amount_all parece ser desnecessário no stock.picking porque não existe no Mixin Fiscal e não existe no ADDONS ( viria de outro lugar? ), acredito que acabamos colocando seguindo a lógica do caso Vendas e Compras que na v14 possuem esses métodos no ADDONS, mas agora na v16 parece existir apenas em Compras módulo purchase

src$ grep -rn '_amount_all' --include=\*.py . --color=always
./addons/sale_loyalty/tests/test_program_numbers.py:73:        # This is because _amount_all() is summing all SO lines (so + (-b.value)) and again in _check_promo_code() order.amount_untaxed + order.reward_amount | amount_untaxed has already free product value substracted (_amount_all)
./addons/point_of_sale/models/pos_order.py:388:    def _onchange_amount_all(self):
./addons/point_of_sale/models/pos_order.py:399:    def _compute_batch_amount_all(self):
./addons/point_of_sale/models/pos_order.py:401:        Does essentially the same thing as `_onchange_amount_all` but only for actually existing records
./addons/purchase/models/purchase.py:25:    def _amount_all(self):
./addons/purchase/models/purchase.py:125:    amount_untaxed = fields.Monetary(string='Untaxed Amount', store=True, readonly=True, compute='_amount_all', tracking=True)
./addons/purchase/models/purchase.py:127:    amount_tax = fields.Monetary(string='Taxes', store=True, readonly=True, compute='_amount_all')
./addons/purchase/models/purchase.py:128:    amount_total = fields.Monetary(string='Total', store=True, readonly=True, compute='_amount_all')

@antoniospneto
Copy link
Contributor Author

@mbcosta, obrigado pelas considerações!

Sobre o nome do método, acredito que compute_br_fiscal_amount seja uma boa sugestão. Acho importante manter o termo "fiscal" para reforçar a associação ao módulo fiscal. cc @rvalyi

Quanto ao ROADMAP da multi-localização, estou ciente e sempre que possível procuro contribuir com melhorias relacionadas a isso. De qualquer forma, é importante destacar o tema para todos os colaboradores.

Em relação ao método _amount_all, não cheguei a investigar a fundo, mas posso aproveitar esta oportunidade para removê-lo. Por ora, meu foco principal aqui é apenas a renomeação dos métodos.

@rvalyi
Copy link
Member

rvalyi commented Dec 18, 2024

@mbcosta , sobre o prefixo fiscal ser "do Brasil" eu acho que vai ser obvio porque o que acontece é que a parte fiscal dos outros paises, o Odoo tende a fazer relativamente bem (de uma forma geral, as vezes um tipo de taxa ou outro ainda precisa de um modulo OCA) no próprio módulo account. Pelo menos é o caso para todos paises do continente americano e os paises da Europa por exemplo. Nisso eu acho o prefix fiscal_, já vai ficar explícito suficiente eu acho.

@rvalyi
Copy link
Member

rvalyi commented Dec 18, 2024

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3549-by-rvalyi-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 102f667 into OCA:16.0 Dec 18, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

6 participants