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][IMP] account_asset_management: big code cleanup #1513

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

alexis-via
Copy link
Contributor

Use fields.Monetary() instead of fields.Float()
Use odoo's compare and round methods for floats
Add check_company=True where relevant
Stop using "Account" decimal precision... it doesn't exist any more! Replace compute() by _convert() for currency conversion

Use fields.Monetary() instead of fields.Float()
Use odoo's compare and round methods for floats
Add check_company=True where relevant
Stop using "Account" decimal precision... it doesn't exist any more!
Replace compute() by _convert() for currency conversion
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.

LGTM

@pedrobaeza pedrobaeza added this to the 14.0 milestone Nov 30, 2022
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Why not using currency_id name for the field and avoid all the explicit currency_field for cleaning a bit more?

@alexis-via
Copy link
Contributor Author

@pedrobaeza yes, why not. But "company_currency_id" is more meaningful, because the developper immediately understands that the currency of the field is from the company, not from a specific field of the asset.

@pedrobaeza
Copy link
Member

But the asset is handled in such currency, so the company meaning doesn't matter. In fact, if in the future a multi-currency asset system is put, we will have currency_id and company_currency_id, but this one would be converted to company_id, not to company_currency_id (although both will initialize as the same).

@pedrobaeza
Copy link
Member

@alexis-via do you see my explanation to simplify to currency_id and avoid to explicitly indicate the currency field?

@alexis-via
Copy link
Contributor Author

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-1513-by-alexis-via-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a7359f6 into OCA:14.0 Mar 9, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b53fa7e. Thanks a lot for contributing to OCA. ❤️

@luc-demeyer
Copy link
Contributor

great job ! Thanks.
Who will cherry-pick this to 15.0 and 16.0 ?

@MiquelRForgeFlow
Copy link
Contributor

@luc-demeyer
Knowing Akretion's aversion to odd versions of odoo, I took a dose of solidarity to migrate this to version 15.0 in #1606.

Also good news: FYI, no need to migrate this to v16.

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.

7 participants