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][IMP] l10n_br_fiscal: standardize data+demo load #3567

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

rvalyi
Copy link
Member

@rvalyi rvalyi commented Dec 30, 2024

Standardize the loading of all l10n_br_fiscal data + demo files.

In l10n_br_fiscal we have some very specific challenges:

  • loading all the production fiscal data for the tax engine takes 3 minutes and a lot of CPU, this is a no go for demo and tests. This is why we had these subset demo files for NCM, NBM, NBS, CEST...
  • for mission critical cases, some fiscal data might better be altered by some fiscal power user and keep noupdate=True. If noupdate were False, reloading the l10n_br_fiscal module would also take a lot of time. By default modules data files load with noupdate=False.

This explains the peculiar loading hook we used in l10n_br_fiscal/hooks.py until now.

But after working on the isolation of the tests from the generic demo data, I also got convinced we could finally standardize the loading of the data + demo files.

In fact all it takes is a custom monkey patch of the odoo.tools.convert.convert_csv_file method to force noupdate=True for some files and to be able to filter and load only some demo records in demo/test mode for some large csv files.

Benefits:

  • standard loading of data + demo files; simpler
  • pylint-odoo will now hunt for bugs in some 25 CSV and XML demo and data files that were not analysed before when they were not declared in the manifest.
  • no more "file-not-used" pylint warnings we could not even filter (in v18 warnings will we block the CI by default)
  • removed 800 lines of demo files we had to maintain
  • no more production/demo data discrepancies
  • will also help simplify the loading of fiscal fixtures in [16.0][REF][l10n_br_base][l10n_br_fiscal][l10n_br_account] tests don't depend on demo #3563

NOTES: in 2022 the TIPI data was updated but the demo TIPI csv file kept using the old IPI from 2021 or before. So now that the tests and demo are using the same production ncm CSV file (a subset of it actually) I had to update a few tests to change the IPI from 5% to 3.25% for some products.

@OCA-git-bot
Copy link
Contributor

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

@rvalyi
Copy link
Member Author

rvalyi commented Feb 18, 2025

pronto para revisão. Tirou tb mais de 500 linhas, coisa que é bom no modulo l10n_br_fiscal que já ta pesado demais...

@rvalyi rvalyi force-pushed the 16.0-better-loading branch from eaa6458 to e786252 Compare February 18, 2025 14:20
@rvalyi rvalyi force-pushed the 16.0-better-loading branch 3 times, most recently from 6bedbb0 to 8b60984 Compare February 18, 2025 15:14
Copy link
Contributor

@antoniospneto antoniospneto left a comment

Choose a reason for hiding this comment

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

Muito bom, parabéns pelo trabalho @rvalyi
Fica bem melhor essa nova abordagem.

@rvalyi rvalyi force-pushed the 16.0-better-loading branch from 8b60984 to 910d98e Compare February 18, 2025 20:16
@rvalyi
Copy link
Member Author

rvalyi commented Feb 18, 2025

@antoniospneto eu apenas fiz um force push para limpar o manifest https://github.com/OCA/l10n-brazil/compare/8b6098476cc28c11e350a0626c094d55fc4133c8..910d98e123fdc0a1e9e8c6afb6a8988f9f93c1b4

@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). 🤖

@rvalyi
Copy link
Member Author

rvalyi commented Feb 19, 2025

vamos ver se os testes vão passar...

@rvalyi
Copy link
Member Author

rvalyi commented Feb 19, 2025

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3567-by-rvalyi-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 19, 2025
Signed-off-by rvalyi
@OCA-git-bot
Copy link
Contributor

@rvalyi your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3567-by-rvalyi-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 Author

rvalyi commented Feb 19, 2025

aquele problema das datas no mes de fevereiro, vamos ver amanhã...

@rvalyi
Copy link
Member Author

rvalyi commented Feb 19, 2025

/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-3567-by-rvalyi-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 95cd9ea into OCA:16.0 Feb 19, 2025
6 checks passed
@OCA-git-bot
Copy link
Contributor

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