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

[IMP] tools: float_utils, rounding new rules to validate parameters #30675

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@jpp-odoo
Copy link
Contributor

jpp-odoo commented Jan 30, 2019

Adding two new rules to verify that invalid parameters are forbidden:

  • The precision_rounding must always be bigger than 0
  • The precision_digits must always be equal or bigger than 0

Before this commit:

  • when a precision_round was 0 or smaller than 0 the float_utils
    functions gave as results:
    ** float_is_zero(0.0, precision_rounding=0.0) -> False
    ** float_round(1.25, precision_rounding=0.0) -> 0.0
    ** float_compare(1.0, 1.0, precision_rounding=0.0) -> 1
  • when a precision_digits was smaller than 0 the float_utils
    functions gave as results:
    ** float_is_zero(9.01, precision_digits=-2) -> True
    ** float_round(125.01, precision_digits=-2) -> 100.0
    ** float_compare(121.01, 122.02, precision_digits=-2) -> 0

These results where at least not correct at worst not logic.

Now, the function raises an error when the precision_rounding is smaller
or equal than 0 or when the precision_digits is smaller than 0.

@robodoo robodoo added the seen 🙂 label Jan 30, 2019

@kebeclibre kebeclibre requested review from rco-odoo , odony , nim-odoo and KangOl Jan 30, 2019

@C3POdoo C3POdoo added the RD label Jan 30, 2019

@KangOl

This comment has been minimized.

Copy link
Contributor

KangOl commented Jan 30, 2019

Needs fix in res.currency

@odony

This comment has been minimized.

Copy link
Contributor

odony commented Jan 30, 2019

May I ask what the use case is? If this is just for the purpose of avoiding manual coding mistakes I'd simply say: garbage in, garbage out. The docstrings are clear enough for me.

@rco-odoo

This comment has been minimized.

Copy link
Member

rco-odoo commented Jan 31, 2019

Regarding precision_digits < 0, the results are mathematically correct. Having precision_digits = -2 is equivalent to rounding at 100: 925 rounds to 900. This may make sense in a particular business domain.

The other point is valid to me: precision_rounding = 0 logically means infinite precision.
The expected results are:

  • float_is_zero(x, precision_rounding=0.0)x == 0.0
  • float_round(x, precision_rounding=0.0)x
  • float_compare(x, y, precision_rounding=0.0)x == y

However, having precision_rounding < 0 is weird. I don't think you can make any sense to it, unless you want to define a funky rounding method for complex numbers. I would consider that case as a programming error and would raise an exception.

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch Jan 31, 2019

@jpp-odoo

This comment has been minimized.

Copy link
Contributor Author

jpp-odoo commented Jan 31, 2019

@KangOl in res.currency, the rounding field already has a default value.

@rco-odoo indeed precision_digit < 0 is mathematically correct, and may make sense. I removed the raise for precision_digit.

For precision_rounding, I'm applying the specs that says that precision_rounding must be positive. See:

:param float precision_rounding: decimal number representing the minimum
non-zero value at the desired precision (for example, 0.01 for a
2-digit precision).

We can change the specs of the functions in float_utils, and we can decide that precision_rounding of 0 means infinite precision. In that case, we will need to change the code to return a consistent result.

@odony The aim of this PR is to avoid issues in stables versions of Odoo, and detect the problems in development.

For example, when we have an empty recordset, and we access a field the value will be the falsy value (0.0 in a float field). This falsy value is passed as parameter to the float_utils function, and returns illogical results, and can create bigger issues. For example, e13d449, in this case it's only a warning, but float_tools are mostly used for financial calculations. If you see all the tests that don't work any more in runbot, it gave you an idea of the potentials issues that we can, one day, find in stable.

@jpp-odoo

This comment has been minimized.

Copy link
Contributor Author

jpp-odoo commented Jan 31, 2019

Another example : b95affa

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch 2 times, most recently Jan 31, 2019

Show resolved Hide resolved addons/sale/models/sale.py Outdated
Show resolved Hide resolved addons/sale/views/sale_views.xml Outdated
@odony
Copy link
Contributor

odony left a comment

Thanks for clarifying the rationale!

Show resolved Hide resolved addons/account/models/account_bank_statement.py Outdated
Show resolved Hide resolved odoo/tools/float_utils.py Outdated

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch 4 times, most recently from 6f4d5c6 to 781e761 Feb 7, 2019

@robodoo robodoo added the CI 🤖 label Feb 11, 2019

@jpp-odoo

This comment has been minimized.

Copy link
Contributor Author

jpp-odoo commented Feb 11, 2019

I just commit a new fix, in sale_margin, now all tests are OK.

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch from 781e761 to 24be857 Feb 12, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 12, 2019

@rco-odoo
Copy link
Member

rco-odoo left a comment

That looks good to me. Thanks for the improvement!

@odony
Copy link
Contributor

odony left a comment

Looks good, I only have minor nitpicking now ;-)

  • Your commit messages are well detailed, nice! To make them perfect you would ideally format the subject line to read like "If merged, this commit will <subject>". Also, no need for trailing punctuation. And since we prefix with the module name, capitalization looks odd. I find that most of the comments in this article apply, though our official commit msg guidelines are really here.
  • see inline
Show resolved Hide resolved addons/sale_margin/models/sale_order.py Outdated
[IMP] tools: float_utils, precision rounding must be positive
Adding a new rule to verify that precision_rounding is positive.

Before this commit:
when a precision_round was 0 or smaller than 0 the float_utils
functions gave as results:
* float_is_zero(0.0, precision_rounding=0.0) -> False
* float_round(1.25, precision_rounding=0.0) -> 0.0
* float_compare(1.0, 1.0, precision_rounding=0.0) -> 1

These results where at least not correct at worst not logic.

Now, the function raises an error when the precision_rounding
is not positive

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch from 24be857 to e81bd0b Feb 13, 2019

@robodoo robodoo removed the CI 🤖 label Feb 13, 2019

jpp-odoo added some commits Jan 31, 2019

[FIX] account: validation error not raising
In an account_bank_statement, when the type of the journal wasn't bank
and the currency was not defined (the currency in account_bank_statement
is optional) and the amount was zero.

Before this commit, the validation error that raise when the amount is
zero, wasn't working. This because, self.currency_id.is_zero is always
False, when currency is not defined.

Now, we take into account if currency is defined.
[FIX] sale_margin: product margin round error
In a SO without customer:

Before this commit, the _product_margin wasn't working. This because,
as the customer is not defined, the pricelist isn't defined either,
and therefore currency of the pricelist is also undefined.
When you call the round function, in an undefined currency
an error is raised.

Now we take into account if customer is defined.

@jpp-odoo jpp-odoo force-pushed the odoo-dev:master-float_rounding_invalid_parameters-jpp branch from e81bd0b to 2ab3c44 Feb 13, 2019

@robodoo robodoo added the CI 🤖 label Feb 13, 2019

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Feb 13, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Feb 13, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 13, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward
@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Feb 13, 2019

robodoo rebase-ff

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 13, 2019

Merge method set to rebase and fast-forward

pniederlag pushed a commit to pniederlag/odoo that referenced this pull request Feb 13, 2019

[FIX] sale_margin: product margin round error
In a SO without customer:

Before this commit, the _product_margin wasn't working. This because,
as the customer is not defined, the pricelist isn't defined either,
and therefore currency of the pricelist is also undefined.
When you call the round function, in an undefined currency
an error is raised.

Now we take into account if customer is defined.

closes odoo#30675

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Feb 13, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 13, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 13, 2019

@jpp-odoo jpp-odoo deleted the odoo-dev:master-float_rounding_invalid_parameters-jpp branch Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment