Skip to content

[FIX] product: apply pricelist formula rounding AFTER currency conversion #102920

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[FIX] product: apply pricelist formula rounding AFTER currency conversion #102920

wants to merge 1 commit into from

Conversation

ushyme
Copy link
Contributor

@ushyme ushyme commented Oct 10, 2022

The setup

  • activate multicurrency and a secondary currency, for example the DKK. Make sure the exchange rate is not 1.
  • in Settings - Sales - Pricelists, activate Advanced Price Rules
  • create a new DKK pricelist with the following settings:
    • Compute Formula
    • Based on Sales Price
    • Discount 42% (or any another value)
    • Rounding Method 1.00
    • Apply on All Products
    • Min Quantity 10

Steps to reproduce

  • create a new sales order
  • set Pricelist = the DKK pricelist you created
  • add a product and set the quantity to 10

You should see that the total, after application of the pricelist, is not rounded.

Cause

The price is being rounded before conversion to the pricelist's currency.

Note

The issue is fixed as of v15.2

opw-3002376

@robodoo
Copy link
Contributor

robodoo commented Oct 10, 2022 •
edited
Loading

Pull request status dashboard

@ushyme
Copy link
Contributor Author

ushyme commented Oct 10, 2022

@fw-bot up to 15.0

@fw-bot
Copy link
Contributor

fw-bot commented Oct 10, 2022

Forward-porting to '15.0'.

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Oct 10, 2022
@ushyme ushyme force-pushed the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch from 55709fa to bb91155 Compare October 11, 2022 13:56
@ushyme ushyme force-pushed the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch 2 times, most recently from 0ac9ef8 to 6f81da6 Compare October 24, 2022 07:27
@ushyme ushyme marked this pull request as ready for review October 24, 2022 08:24
@ushyme ushyme requested a review from Feyensv October 24, 2022 08:24
Feyensv
Feyensv reviewed Oct 24, 2022
View reviewed changes
Copy link
Contributor

@Feyensv Feyensv left a comment

Choose a reason for hiding this comment

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

Fix is good, but the API change in stable will probably break custom code 🤔 (cause the methods is now supposed to return the price in the pricelist currency, when it wasn't the case before ...).

cc: @len-foss (#60519)

@len-foss
Copy link
Contributor

Fix is good, but the API change in stable will probably break custom code thinking (cause the methods is now supposed to return the price in the pricelist currency, when it wasn't the case before ...).

Thank you for the ping ;-)

I mean, you already know this unambiguously breaks the stability policy...

I don't have a test case under hand, but I think the best way to do it anyway would be to:

  • extract the conversion function into an instance level private function, say self._convert_to_base_currency
  • have the signature be a superset of _compute_price's signature
  • have the last line be return self._convert_to_base_currency(...) (which does nothing if no conversion is necessary)

This way any overriding code can be adapted to the new signature by also overriding the conversion method. Whereas right now it wouldn't be possible without patching the standard. It would still be painful to discover the breaking change inadvertently, but at least there would be a clear path to a fix.

@ushyme
Copy link
Contributor Author

ushyme commented Oct 28, 2022 •
edited
Loading

Hello @len-foss, thanks for the reply, and sorry for the late reply.

I'm not sure I understand what you have in mind. Could you elaborate ?

  • have the last line be return self._convert_to_base_currency(...) (which does nothing if no conversion is necessary)

What last line are we talking about ?

@Feyensv Feyensv force-pushed the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch 2 times, most recently from 18df1c3 to b31658c Compare January 9, 2023 14:35
@C3POdoo C3POdoo requested a review from a team January 9, 2023 14:42
@Feyensv Feyensv force-pushed the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch 2 times, most recently from 71186a7 to cfd5e22 Compare January 9, 2023 15:02
@Feyensv Feyensv requested review from Demesmaeker and removed request for a team January 9, 2023 15:43
@ushyme @Feyensv
[FIX] product: apply pricelist formula rounding AFTER currency conver…
0ee7173 
…sion

The setup:
* activate multicurrency and a secondary currency, for example the DKK.
  Make sure the exchange rate is not 1.
* in Settings - Sales - Pricelists, activate Advanced Price Rules
* create a new DKK pricelist with the following settings:
  * Compute Formula
  * Based on Sales Price
  * Discount 42% (or any another value)
  * Rounding Method 1.00
  * Apply on All Products
  * Min Quantity 10

Steps to reproduce:
* create a new sales order
* set Pricelist = the DKK pricelist you created
* add a product and set the quantity to 10

You should see that the total, after application of the pricelist, is
not rounded.

Cause:
The price is being rounded before conversion to the pricelist's
currency.

Solution:

Finetuning of b7ec0a2
to make sure the pricelist base price is consistently rounded
before applying the rule formula (surcharge, margins, rounding, ...).

Previously, the price given to _compute_price was:

* in the pricelist currency if the rule is based on another pricelist
* in the product currency if the rule is based on the sales price
* in the product cost currency if the rule is based on the cost

Now, the price given is always in the pricelist currency.
This modifies the API of the method in stable (modifying the currency
of the price input), but is quite a logical change and required to fix
the issue at hand.

Note:
The issue is fixed as of v15.2

opw-3002376
@Feyensv Feyensv force-pushed the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch from cfd5e22 to 0ee7173 Compare January 10, 2023 10:08
@Feyensv
Copy link
Contributor

Feyensv commented Jan 10, 2023

@robodoo r+

robodoo pushed a commit that referenced this pull request Jan 10, 2023
@ushyme @Feyensv
[FIX] product: apply pricelist formula rounding AFTER currency conver…
ab6fe46 
…sion

The setup:
* activate multicurrency and a secondary currency, for example the DKK.
  Make sure the exchange rate is not 1.
* in Settings - Sales - Pricelists, activate Advanced Price Rules
* create a new DKK pricelist with the following settings:
  * Compute Formula
  * Based on Sales Price
  * Discount 42% (or any another value)
  * Rounding Method 1.00
  * Apply on All Products
  * Min Quantity 10

Steps to reproduce:
* create a new sales order
* set Pricelist = the DKK pricelist you created
* add a product and set the quantity to 10

You should see that the total, after application of the pricelist, is
not rounded.

Cause:
The price is being rounded before conversion to the pricelist's
currency.

Solution:

Finetuning of b7ec0a2
to make sure the pricelist base price is consistently rounded
before applying the rule formula (surcharge, margins, rounding, ...).

Previously, the price given to _compute_price was:

* in the pricelist currency if the rule is based on another pricelist
* in the product currency if the rule is based on the sales price
* in the product cost currency if the rule is based on the cost

Now, the price given is always in the pricelist currency.
This modifies the API of the method in stable (modifying the currency
of the price input), but is quite a logical change and required to fix
the issue at hand.

Note:
The issue is fixed as of v15.2

opw-3002376

closes #102920

Signed-off-by: Victor Feyens (vfe) <vfe@odoo.com>
@robodoo robodoo temporarily deployed to merge January 10, 2023 11:37 Inactive
@robodoo robodoo closed this Jan 10, 2023
This was referenced Jan 10, 2023
Closed
Closed
gamarino pushed a commit to numaes/numa-public-odoo that referenced this pull request Jan 11, 2023
@ushyme @gamarino
[FIX] product: apply pricelist formula rounding AFTER currency conver…
7d6484f 
…sion

The setup:
* activate multicurrency and a secondary currency, for example the DKK.
  Make sure the exchange rate is not 1.
* in Settings - Sales - Pricelists, activate Advanced Price Rules
* create a new DKK pricelist with the following settings:
  * Compute Formula
  * Based on Sales Price
  * Discount 42% (or any another value)
  * Rounding Method 1.00
  * Apply on All Products
  * Min Quantity 10

Steps to reproduce:
* create a new sales order
* set Pricelist = the DKK pricelist you created
* add a product and set the quantity to 10

You should see that the total, after application of the pricelist, is
not rounded.

Cause:
The price is being rounded before conversion to the pricelist's
currency.

Solution:

Finetuning of b7ec0a2
to make sure the pricelist base price is consistently rounded
before applying the rule formula (surcharge, margins, rounding, ...).

Previously, the price given to _compute_price was:

* in the pricelist currency if the rule is based on another pricelist
* in the product currency if the rule is based on the sales price
* in the product cost currency if the rule is based on the cost

Now, the price given is always in the pricelist currency.
This modifies the API of the method in stable (modifying the currency
of the price input), but is quite a logical change and required to fix
the issue at hand.

Note:
The issue is fixed as of v15.2

opw-3002376

closes odoo/odoo#102920

Signed-off-by: Victor Feyens (vfe) <vfe@odoo.com>
@fw-bot fw-bot deleted the 13.0-opw-3002376-apply_pricelist_formula_rounding_after_currency_conversion-sesn branch January 24, 2023 11:46
zamberjo pushed a commit to aurestic/OpenUpgrade that referenced this pull request Mar 1, 2023
@ushyme
[FIX] product: apply pricelist formula rounding AFTER currency conver…
f7a7418 
…sion

The setup:
* activate multicurrency and a secondary currency, for example the DKK.
  Make sure the exchange rate is not 1.
* in Settings - Sales - Pricelists, activate Advanced Price Rules
* create a new DKK pricelist with the following settings:
  * Compute Formula
  * Based on Sales Price
  * Discount 42% (or any another value)
  * Rounding Method 1.00
  * Apply on All Products
  * Min Quantity 10

Steps to reproduce:
* create a new sales order
* set Pricelist = the DKK pricelist you created
* add a product and set the quantity to 10

You should see that the total, after application of the pricelist, is
not rounded.

Cause:
The price is being rounded before conversion to the pricelist's
currency.

Solution:

Finetuning of b7ec0a2
to make sure the pricelist base price is consistently rounded
before applying the rule formula (surcharge, margins, rounding, ...).

Previously, the price given to _compute_price was:

* in the pricelist currency if the rule is based on another pricelist
* in the product currency if the rule is based on the sales price
* in the product cost currency if the rule is based on the cost

Now, the price given is always in the pricelist currency.
This modifies the API of the method in stable (modifying the currency
of the price input), but is quite a logical change and required to fix
the issue at hand.

Note:
The issue is fixed as of v15.2

opw-3002376

closes odoo/odoo#102920

Signed-off-by: Victor Feyens (vfe) <vfe@odoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Feyensv Feyensv Feyensv left review comments

+1 more reviewer

@Demesmaeker Demesmaeker Demesmaeker approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ushyme @robodoo @fw-bot @len-foss @Feyensv @Demesmaeker @C3POdoo