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][REF] l10n_br_fiscal, l10n_br_sale, l10n_br_stock, l10n_br_stock_account, l10n_br_purchase, l10n_br_sale_stock: Changes to run CI in l10n_br_delivery migration #3629

Open
wants to merge 9 commits into
base: 16.0
Choose a base branch
from

Conversation

mbcosta
Copy link
Contributor

@mbcosta mbcosta commented Feb 18, 2025

English

Changes to run CI for l10n_br_delivery migration,

  • Create ir.property for link Journal in Fiscal Operation by method instead of XML to avoid error when run twice( this files was loaded by hook before)
  • Create Inventory for Demo Data by method to avoid error when CI load twice
  • Update README by pre-commit
  • Load Sale Orders Demo Data by hook to avoid error with Payment Mode
   raise UserError("\n".join(lines))
odoo.exceptions.UserError: Incompatible companies on records:
- 'SN l10n_br_sale - Produtos' belongs to company 'Empresa Simples Nacional' and 'Payment Mode' (payment_mode_id: 'Inbound Credit Trf Société Générale') belongs to another company.

This is the last commit and if the PR OCA/bank-payment#1421 will Approved become unnecessay and the commit can be removed and the file can back be loaded by manifest.

Português

Alterações necessárias para rodar o CI do PR de migração do l10n_br_delivery, apesar de alterar mais de um módulo nesse PR as alterações são simples, segue os detalhes:

  • Criação dos ir.property dos Diários Contábeis com as Operações Fiscais via Método Python ao invés de criar pelo XML, isso já era carregado pelo Hook então agora passa a ser sem o XML

  • Criação do "Inventário dos Dados de Demonstração" por método ao invés de XML, isso gera erro ao ser carregado duas vezes e com isso foi possível reduzir o código necessário e um simples método pode ser usado no l10n_br_stock_account, ou qualquer outro módulo, para inventariar algum Produto para uso nos Dados de Demonstração ou Testes

  • Atualizações automáticas no README feitas pelo pre-commit

  • Os únicos arquivos que ainda estão precisando serem carregados via Hook são os que criam os Pedidos de Vendas/sale.order porque ao carregar os valores defaults no hook https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_sale/hooks.py#L16 o programa retorna um Modo de Pagamento que não pertence a Empresa do Pedido de Vendas o que gera o erro

2025-02-17 18:14:15,535 239 WARNING test odoo.modules.loading: Module l10n_br_sale demo data failed to install, installed without demo data 
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/odoo/tools/convert.py", line 698, in _tag_root
    f(rec)
  File "/usr/local/lib/python3.10/site-packages/odoo/tools/convert.py", line 599, in _tag_record
    record = model._load_records([data], self.mode == 'update')
  File "/usr/local/lib/python3.10/site-packages/odoo/models.py", line 4412, in _load_records
    data['record']._load_records_write(data['values'])
  File "/usr/local/lib/python3.10/site-packages/odoo/models.py", line 4343, in _load_records_write
    self.write(values)
  File "/usr/local/lib/python3.10/site-packages/odoo/addons/mail/models/mail_thread.py", line 315, in write
    result = super(MailThread, self).write(values)
  File "/usr/local/lib/python3.10/site-packages/odoo/addons/mail/models/mail_activity_mixin.py", line 241, in write
    return super(MailActivityMixin, self).write(vals)
  File "/usr/local/lib/python3.10/site-packages/odoo/models.py", line 3851, in write
    self._check_company()
  File "/usr/local/lib/python3.10/site-packages/odoo/models.py", line 3486, in _check_company
    raise UserError("\n".join(lines))
odoo.exceptions.UserError: Incompatible companies on records:
- 'SN l10n_br_sale - Produtos' belongs to company 'Empresa Simples Nacional' and 'Payment Mode' (payment_mode_id: 'Inbound Credit Trf Société Générale') belongs to another company.

Eu inclui um comentário sobre isso e estou buscando resolver o problema em um PR no account_payment_sale basicamente no método que retorna o Modo de Pagamento Padrão passa a verificar se esse modo pertence a mesma Empresa do Pedido de Vendas, portanto se esse PR for Aprovado nós podemos remover esse último commit e voltar a carregar o arquivo pelo manifest, então será importante que os Desenvolvedores de Localização ajudem nessa Revisão.

IMPORTANTE: As mesmas alterações feitas no l10n_br_sale precisam ser feitas no l10n_br_sale_stock que são criar os ir.prorperty via método e carregar os Dados de Demonstração que criam os Pedidos de Vendas pelo hook enquanto o PR no account_payment_sale não é Aprovado, esse módulo ainda é um PR de migração #3570 então será importante avaliar de fazer esse Merge para poder fazer um PR com essas alterações e dar sequencia na migração do l10n_br_delivery.

Se necessário posso ver de extrair partes do PR, busquei resolver de acordo com a revisão feita no PR #3606

image

Acredito que as alterações estão de acordo e os únicos arquivos que restaram sendo carregados pelo hook são os dos Pedidos de Vendas devido o problema e com a solução que comentei acima.

cc @OCA/local-brazil-maintainers

@OCA-git-bot
Copy link
Contributor

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

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch 2 times, most recently from c9006fa to 53a2310 Compare February 18, 2025 12:03
@rvalyi
Copy link
Member

rvalyi commented Feb 18, 2025

ficou com cara bem melhor assim @mbcosta obrigado. Vou revisar assim que eu puder.

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch from 53a2310 to 40aff4c Compare February 20, 2025 14:19

def inform_journal_in_fiscal_operation(cr, company, values):
"""
Create or Inform ir.property for link Journal with Fiscal Operation
Copy link
Member

Choose a reason for hiding this comment

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

it's better to use fill_journal... or set_journal... than inform_journal... Fill/Set carry the notion of writing something when Inform would rather mean "reporting" about something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @rvalyi for review, changed for set_journal_...

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch from 40aff4c to 45ac78f Compare February 20, 2025 14:53
@mbcosta mbcosta changed the title [16.0][REF] l10n_br_fiscal, l10n_br_sale, l10n_br_stock, l10n_br_stock_account: Changes to run CI in l10n_br_delivery migration [16.0][REF] l10n_br_fiscal, l10n_br_sale, l10n_br_stock, l10n_br_stock_account, l10n_br_purchase, l10n_br_sale_stock: Changes to run CI in l10n_br_delivery migration Feb 20, 2025
f"Create or Inform Journal in Fiscal Operation for {company.name} Property ..."
)
for value in values:
env = api.Environment(cr, SUPERUSER_ID, {})
Copy link
Member

Choose a reason for hiding this comment

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

please move the env creation outside of the loop (optimization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -3,6 +3,8 @@

from odoo import SUPERUSER_ID, api, tools

from odoo.addons.l10n_br_fiscal.tools import set_journal_in_fiscal_operation


def post_init_hook(cr, registry):
cr.execute("select demo from ir_module_module where name='l10n_br_purchase';")
Copy link
Member

Choose a reason for hiding this comment

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

please change this line to env.ref('base.module_l10n_br_purchase').demo, would be cool if you could update in the other places as well. Seems cleaner, ripped from Odoo: https://github.com/odoo/odoo/blob/25feb5b11c2df401580b65e0108145863fcf8987/addons%2Fl10n_ar_withholding%2F__init__.py#L34C16-L34C63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, change made in l10n_br_sale, l10n_br_purchase, l10n_br_stock and l10n_br_stock_account

@mbcosta
Copy link
Contributor Author

mbcosta commented Feb 20, 2025

valeu @rvalyi pela Revisão, apenas atualizando para quem estiver acompanhando o PR:

  File "/home/odoo/app/external-src/account-invoicing-NEW-sale_stock_picking_invocing-13_01_2025/stock_picking_invoicing/wizards/stock_invoice_onshipping.py", line 366, in _build_invoice_values_from_pickings
    journal = self._get_journal()
  File "/home/odoo/app/external-src/l10n-brazil-REF-necessary_changes_to_run_CI_for_delivery_mig-18_02_2025/l10n_br_stock_account/wizards/stock_invoice_onshipping.py", line 60, in _get_journal
    raise UserError(
odoo.exceptions.UserError: Invalid Journal! There is not journal defined for this company: Sua Empresa (São Paulo) in fiscal operation: Venda!
  • Com o merge do l10n_br_sale_stock [16.0][MIG] l10n_br_sale_stock #3570 tornou possível incluir aqui a alteração necessária para rodar o CI do l10n_br_delivery que passou a ser simples, no momento de rodar os Testes chama o método que cria o ir.property do l10n_br_sale, para evitar erro similar ao do LOG acima ( aqui talvez exista algo que pode ser alterado tornando isso desnecessário, estou tentando descobrir, deixei um TODO)

  • Com inclusão do método que cria os ir.property para associar os Diários Contábeis as Operações Fiscais também foi removido do Hook o carregamento dos arquivos XML

image

Com isso os seguintes arquivos foram removidos
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_sale/demo/fiscal_operation_generic.xml https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_sale/demo/fiscal_operation_simple.xml
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_stock_account/demo/fiscal_operation_generic.xml
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_stock_account/demo/fiscal_operation_simple.xml
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_purchase/demo/fiscal_operation_generic.xml
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_purchase/demo/fiscal_operation_simple.xml

Isso deve atender o ponto de "minimizar a quantidade de arquivos carregados pelo hook", com esse PR os módulos l10n_br_sale, l10n_br_purchase e l10n_br_stock_account deixam de ter esses casos.

  • A criação do Inventário Inicial dos Dados de Demonstração por Método ao invés de XML foi necessária para evitar o erro:
2025-02-18 14:16:43,329 53 WARNING test odoo.modules.loading: Module l10n_br_stock demo data failed to install, installed without demo data 
Traceback (most recent call last):
.
.
.
  File "/usr/local/lib/python3.10/site-packages/odoo/addons/stock/models/stock_quant.py", line 351, in write
    raise UserError(_("Quant's editing is restricted, you can't do this operation."))
odoo.exceptions.UserError: Quant's editing is restricted, you can't do this operation.

The above exception was the direct cause of the following exception:

.
.
.
  File "/usr/local/lib/python3.10/site-packages/odoo/tools/convert.py", line 711, in _tag_root
    raise ParseError('while parsing %s:%s, somewhere inside\n%s' % (
odoo.tools.convert.ParseError: while parsing /home/odoo/app/external-src/l10n-brazil-MIG-l10n_br_delivery-11_02_2025/l10n_br_stock/demo/stock_inventory_demo.xml:9, somewhere inside
<record id="stock_inventory_sn_1" model="stock.quant">
            <field name="product_id" ref="product.product_product_24"/>
            <field name="inventory_quantity">16.0</field>
            <field name="location_id" model="stock.location" eval="obj().env.ref('l10n_br_stock.wh_empresa_simples_nacional').lot_stock_id.id"/>
        </record>

Com isso estão sendo removidos os arquivos
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_stock/demo/stock_inventory_demo.xml
https://github.com/OCA/l10n-brazil/blob/16.0/l10n_br_stock_account/demo/stock_inventory_demo.xml

Posso ver de extrair a parte que Cria o Inventário por Método se acharem melhor, acredito ser desnecessário porque apesar do PR alterar vários módulos são alterações repetitivas.

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch from 6f38bf5 to 7c19e58 Compare February 20, 2025 19:21
@rvalyi rvalyi self-requested a review February 25, 2025 15:00
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.

@mbcosta de uma forma geral ta legal, mas vi esse detalhe: vc usou sempre if env.ref("base.module_base").demo mas na real, em tese cada modulo pode ser carregado com dados de demo ou não. Então seria mais robusto se testar se o env.ref("base.module_XY").demo estiver True ou não. Com certeza no Odoo e na localização teria varios problemas se começar a carregar alguns modulos com dados de demo e outros não, mas enfim o codigo seria mais robusto se tiver mais precisão testando env.ref("base.module_XY").demo. Nos post_init_hook eu accredito que da para botar o nome do modulo onde esta o hook, por exemplo env.ref("base.module_l10n_br_stock_account").demo.

No pre_init_hook, até onde eu vi pode ser que o flag demo do modulo ainda não valer True ainda na hora do pre_init_hook do modulo. Nisso poderia testar algum modulo parente como testando env.ref("base.module_l10n_br_base").demo já seria melhor do que testar apenas com o base. Deu para entender?

for value in values:
fiscal_operation = value.get("fiscal_operation")
journal = value.get("journal")
env.ref(fiscal_operation)
Copy link
Contributor

Choose a reason for hiding this comment

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

acho que ficou faltando atribuir o valor a uma variavel aqui

Copy link
Member

Choose a reason for hiding this comment

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

parece que era apenas um teste para a linha depois str(env.ref(fiscal_operation).id tou tirando...

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch from 7c19e58 to 2ccef8b Compare February 26, 2025 23:03
env = api.Environment(cr, SUPERUSER_ID, {})
env = api.Environment(cr, SUPERUSER_ID, {})
if env.ref("base.module_l10n_br_stock_account").demo:
create_quants_for_inventory(
Copy link
Member

Choose a reason for hiding this comment

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

@mbcosta vc esqueceu mudar o nome do metodo para create_locations_quants aqui

@rvalyi rvalyi force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch 3 times, most recently from 4fad57a to 6e8ced0 Compare February 27, 2025 01:35
@rvalyi
Copy link
Member

rvalyi commented Feb 27, 2025

@mbcosta @antoniospneto @renatonlima dei um force push arrumando os detalhes que @antoniospneto e eu falamos nos ultimos comentarios (tirar linha inútil e alterar o método para create_locations_quants) . Ficou verde de novo.

No geral parece legal, mas pode ser que tem alguma margem de melhoria. Tou dando rebase da migração do delivery por cima...

@mbcosta mbcosta force-pushed the 16.0-REF-necessary_changes_to_run_CI_for_delivery_mig branch from 6e8ced0 to d75d22d Compare February 27, 2025 02:07
@antoniospneto
Copy link
Contributor

Pessoal se puderem dar uma olhada na PR #3644 antes, tenho uma proposta alternativa

@rvalyi rvalyi self-requested a review February 28, 2025 02:56
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.

4 participants