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][spec_driven_model][l10n_br_nfe] multi schemas support - round 2 #3431

Merged
merged 11 commits into from
Oct 16, 2024

Conversation

rvalyi
Copy link
Member

@rvalyi rvalyi commented Oct 10, 2024

#3424 was a first step but here I went further:

  1. scope every mapping settings by schema and version
  2. don't use a dict like _nfe40_spec_settings to scope dynamic mapping attributes like in [14.0][spec_driven_model][l10n_br_nfe] multi-schemas compatibility - round 1 #3424 because values inside the dict were not easy to inherit. So I flattened everything and prefixed directly what were the dict keys in [14.0][spec_driven_model][l10n_br_nfe] multi-schemas compatibility - round 1 #3424
  3. refactor some poor XML serialization code (specially renamed export_ds to _buid_binding, notice that the public export_ds was possibly also small security issue...)

EDIT: explanations

So the general idea, is StackedModel's have special mapping settings like to which generated "spec" mixin they map (like l10n_br_fiscal.document.line maps on nfe.40.det, where is the Python binding xsdata package they should use for XML serialization and deserialization, which many2one fields should be de-normalized into the stacked object even if they are optional (stacking_force_paths) which required many2one should on the contrary not be stacked (stacking_skip_paths))
These mapping settings attributes are schema specific so they should not mixed between NFe, CTe, MDFe etc... even when some objects like l10n_br_fiscal.document or l10n_br_fiscal_document.line should be mapped on the same objects)

So I scoped all these mapping settings attributes. And the the challenge was to ensure that mapping methods use the proper mapping attributes. These mapping methods have roughly 3 entry points:

  1. building the "Stacked" data model when _build_model is called
  2. importing XML Fiscal documents (build_from_binding)
  3. exporting XML Fiscal documents (_build_binding)

For all these entry points I ensured the schema name and versions are passed in:

  • as parameters
  • or as class attributes for _build_model

Once the entry point are entered with the proper schema name and version it sets it into the context so sub mapping methods will be have access to the proper schemas and version.

@OCA-git-bot
Copy link
Contributor

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

@rvalyi rvalyi marked this pull request as ready for review October 12, 2024 21:48
@rvalyi
Copy link
Member Author

rvalyi commented Oct 12, 2024

@renatonlima @marcelsavegnago @antoniospneto eu resolvi o ultimo problema com a importação da Nfe, era apenas que eu tinha botado o field_prefix como _nfe40 em vez de nfe40_

Se realmente for um problema eu poderia extrair 1 ou 2 sub-PRs. Mas enfim é tempo que eu teria a menos para arrumar outras coisas importantes. Então se der para revisar assim... Eu fico curioso do feedback com a MDF-e.

EDIT: ainda falta matar o a questão do attributo _spec_module. Tou vendo qual seria a melhor forma de entrar no _register_hook especificando o schema e a versão...

@rvalyi rvalyi marked this pull request as ready for review October 14, 2024 14:15
@rvalyi
Copy link
Member Author

rvalyi commented Oct 14, 2024

agora sim ficou realmente multi esquemas. Eu tinha esquecido algumas propriedades nos mixins como spec.mixin.nfe. Agora realmente todos atributos tem um prefix por esquema e versão.

cc @renatonlima @marcelsavegnago @antoniospneto @mileo

@antoniospneto
Copy link
Contributor

@renatonlima @marcelsavegnago @antoniospneto eu resolvi o ultimo problema com a importação da Nfe, era apenas que eu tinha botado o field_prefix como _nfe40 em vez de nfe40_

Se realmente for um problema eu poderia extrair 1 ou 2 sub-PRs. Mas enfim é tempo que eu teria a menos para arrumar outras coisas importantes. Então se der para revisar assim... Eu fico curioso do feedback com a MDF-e.

EDIT: ainda falta matar o a questão do attributo _spec_module. Tou vendo qual seria a melhor forma de entrar no _register_hook especificando o schema e a versão...

Certas coisas eu tbm prefiro que fique em um PR só, facilita na revisão pq ai é uma só vez tbm.

@mileo mileo self-requested a review October 14, 2024 16:26
@rvalyi rvalyi marked this pull request as draft October 14, 2024 19:01
@rvalyi
Copy link
Member Author

rvalyi commented Oct 15, 2024

botei o PR como rascunho de novo, pois quando instalar apenas o modulo l10n_br_nfe, tem uma regressão ao tentar adicionar uma duplicata numa NFe (campo currency_id não existe).
@marcelsavegnago eu ainda não resolvi mas pude ver que a regressão ocorreu dentro desses 3 ultimos commits (pois antes tava OK):

commit ee2b04dd353089aa5a85d3f63f800d778377690f
Author: Raphaël Valyi <rvalyi@akretion.com>
Date:   Mon Oct 14 06:22:03 2024 +0000

    [REF] l10n_br_nfe: further multi-schemas

commit 6c8e015fcc48dbe85202b03ba621d1cce222d1ce
Author: Raphaël Valyi <rvalyi@akretion.com>
Date:   Mon Oct 14 06:23:41 2024 +0000

    [REF] l10n_br_nfe_spec: multi-schemas + renamed

commit 87da59b7e96004dc323f108e014f692b766dc286
Author: Raphaël Valyi <rvalyi@akretion.com>
Date:   Mon Oct 14 06:20:33 2024 +0000

    [REF] spec_driven_model: further multi-schemas

Esses 3 ultimos commits evitam de ter algum atributo herdado de um mixin como spec.mixin.nfe e que poderia conflitar entre 2 esquemas (até em objetos não "stacked" como res.partner). Tou caçando o problema mas tb cuidando de projetos de clientes...

cc @antoniospneto

@rvalyi rvalyi force-pushed the 14.0-multi-schemas3 branch 3 times, most recently from 74e90d5 to 3b01a90 Compare October 15, 2024 22:32
@rvalyi
Copy link
Member Author

rvalyi commented Oct 15, 2024

@marcelsavegnago o problema com a edição das duplicatas direitamente no l10n_br_fiscal.document esta solucionado com o ultimo commit. Eu tb dei um amend no commit antérior do l10n_br_nfe para chamar o super no _generate_key(). Como ficou com a MDF-e ai?

2024-10-15_22-57

cc @antoniospneto

@rvalyi rvalyi marked this pull request as ready for review October 15, 2024 22:59
@rvalyi
Copy link
Member Author

rvalyi commented Oct 16, 2024

pessoal, o objetivo do ultimo PR era de resolver o _register_hook quando entra pelo odoo/registry.py sem ter um contexto de mapping de documento fiscal (ao contrario de importar ou exportar uma NFe por examplo). Ai eu removi os atributos dos mixins como spec.mixin.nfe porque isso daria problema: por exemplo res.partner podia herdar de spec.mixin.nfe e de spec.mixin.mdfe e ai ia conflitar. Agora analiso o mro(), vejo onde a class foi definida e tanto ler o spec_schema e spec_version do modulo que eh algo que nao eh herado e nao iria conflitar. Entao por examplo pego do l10n_br_nfe/models/__init__.py. Mas o problema eh que eu paro no primeiro que eu encontro.

Eu deveria retornar uma lista com os varios esquemas encontrados. por exemplo [(nfe, 40), (mdfe, 30)] e chamar o _register_hook para cada um dos esquemas. Vou ajustar isso...

@rvalyi rvalyi marked this pull request as draft October 16, 2024 15:55
@marcelsavegnago
Copy link
Member

marcelsavegnago commented Oct 16, 2024

Aqui na classe product.product do modulo l10n_br_nfe, ou ela vira spec_models.SpecModel ou precisa definir _nfe40_odoo_module

class ProductProduct(models.Model):
    _inherit = "product.product"
    _nfe40_odoo_module = "odoo.addons.l10n_br_nfe_spec.models.v4_0.leiaute_nfe_v4_00"
    _nfe_search_keys = ["default_code", "barcode"]

De qq forma já estou aprovando a PR :D. Testei com o modulo mdfe, ajustei o mesmo com as mudanças do spec_driven_model e testei instalação, atualização, instalação de um modulo depois o outro, os dois juntos, testei a geração do xml e afins.. a principio OK

@marcelsavegnago
Copy link
Member

bora mudar para pronta para revisão :D

@rvalyi rvalyi marked this pull request as ready for review October 16, 2024 16:48
@rvalyi
Copy link
Member Author

rvalyi commented Oct 16, 2024

Valeu pelo retorno @marcelsavegnago . Sobre o atributo no product.product, nao me choca mas vamos guardar ele pro PR da MDFe para sinalizar que foi feito para a MDFe passar e nao apenas para a NFe.

Esse problema que eu relatei de chamar o _register_hook apenas pro primeiro esquema que ele encontrar nos ancestrais do modelo, na real eh pouco provavel que pega. Porque para a NFe e MDFe vai ter pelo menos algum objeto Odoo que vai ser extendido apenas para um dos esquemas e ele vai entao chamar o _register_hook pros 2 esquemas. E se usar apenas a NFe nao tem risco nenhum. Entao podemos guardar como melhoria para fazer depois e ja fazer o merge disso para fazer entrar a MDFe como alfa na sequencia.

@marcelsavegnago
Copy link
Member

Valeu pelo retorno @marcelsavegnago . Sobre o atributo no product.product, nao me choca mas vamos guardar ele pro PR da MDFe para sinalizar que foi feito para a MDFe passar e nao apenas para a NFe.

Esse problema que eu relatei de chamar o _register_hook apenas pro primeiro esquema que ele encontrar nos ancestrais do modelo, na real eh pouco provavel que pega. Porque para a NFe e MDFe vai ter pelo menos algum objeto Odoo que vai ser extendido apenas para um dos esquemas e ele vai entao chamar o _register_hook pros 2 esquemas. E se usar apenas a NFe nao tem risco nenhum. Entao podemos guardar como melhoria para fazer depois e ja fazer o merge disso para fazer entrar a MDFe como alfa na sequencia.

Concordo.. bora mesclar então :D

@rvalyi
Copy link
Member Author

rvalyi commented Oct 16, 2024

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-3431-by-rvalyi-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9b7c251 into OCA:14.0 Oct 16, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

4 participants