[FIX] product: variant sale price is wrong #87248

momegahed · 2022-03-25T08:26:44Z

If applied, this commit will fix the following bug by using the price
with the additional cost added instead of only the price

Steps to reproduce:

1- install sales
2- create a product p with variant pv
3- set an extra value for pv
4- go to Products>Product Variants
5- open the pv in form view
6- the Sales Price is the price of p not pv
(which should be p + extra value)
7- also the tax string is calculated on p not pv

Bug:

list_price is being used

Fix:

replace list_price with lst_price

OPW-2774199

robodoo · 2022-03-25T08:27:22Z

adwid

Considering how lst_price is computed (= list_price + extra_prices) + the fact that lst_price is the one used on the tree view of product.product:

odoo/addons/product/views/product_views.xml

Line 336 in d3ec94f

Your changes in the XML file seem legit

However, I don't understand why there is an issue with the computation of tax_string ? Note that the addition of non-stored fields should be avoided as much as possible. Also, the implementation of product_product._compute_tax_string is strange. product.product is a child of product.template, so the method overrides the original one while its body calls the original one

Also, could you be more precise in the steps-to-reproduce ? I'm not sure it is that clear that the user has to set an extra-price on a specific variant attribute (I have had to read the ticket again 😇)

momegahed · 2022-04-01T09:22:35Z

Considering how lst_price is computed (= list_price + extra_prices) + the fact that lst_price is the one used on the tree view of product.product:

odoo/addons/product/views/product_views.xml

Line 336 in d3ec94f

<field name="lst_price" optional="show" string="Sales Price"/>

Your changes in the XML file seem legit
However, I don't understand why there is an issue with the computation of tax_string ? Note that the addition of non-stored fields should be avoided as much as possible. Also, the implementation of product_product._compute_tax_string is strange. product.product is a child of product.template, so the method overrides the original one while its body calls the original one

Also, could you be more precise in the steps-to-reproduce ? I'm not sure it is that clear that the user has to set an extra-price on a specific variant attribute (I have had to read the ticket again 😇)

the inheritance between product.product and product.template is delegation inheritance not traditional inheritance

thus the method override wouldn't work

okay to the second point

adwid · 2022-04-01T09:58:39Z

My bad for the inheritance
I am still not convinced by the changes, I didn't find a use of tax_string anywhere but on a form of a product template:

odoo/addons/account/views/product_view.xml

Line 76 in 1f69445

but maybe I'm missing something ?
@agr-odoo Could you check this ?

momegahed · 2022-04-01T10:23:29Z

My bad for the inheritance I am still not convinced by the changes, I didn't find a use of tax_string anywhere but on a form of a product template:

odoo/addons/account/views/product_view.xml

Line 76 in 1f69445

<span class="ml-2"/><field name="tax_string"/>

but maybe I'm missing something ?
@agr-odoo Could you check this ?

product.product form view interits from product.template and the same field is needed there albeit it needs correction in the calculation thus the need for the changes

agr-odoo

I'm ok with the idea. This needs to be fixed as price and tax string of the variant product shows the template product data

On the actual implementation it would be better to have also the opinion of @Feyensv

addons/product/views/product_views.xml

addons/account/models/product.py

Feyensv · 2022-04-04T09:09:37Z

I also don't understand the need of the tax_string code and I don't see any mention of it in the source ticket.
If nobody needs it, don't add it, it's not like something is wrong without the tax explanation (furthermore, existing database won't have the change anyway).

For the lst_price change, could you investigate why it works fine in previous versions, and maybe link the commit/PR that caused the problem ? (probably #75862 but I cannot say for sure)

agr-odoo · 2022-04-04T09:28:24Z

I also don't understand the need of the tax_string code and I don't see any mention of it in the source ticket. If nobody needs it, don't add it, it's not like something is wrong without the tax explanation (furthermore, existing database won't have the change anyway).

The tax explanation needs to be fixed (or hidden) as it uses the values of the template product
Example:
Template product have a sales price 750.00 with 15% tax not included in price
-> tax string in template product is "(= $ 862.50 Incl. Taxes)"
Variant product add +50.00
-> tax string in variant prod before patch is the same "(= $ 862.50 Incl. Taxes)" (see screenshot)
-> tax string in variant prod after patch is "(= $ 920.00 Incl. Taxes)"

Feyensv · 2022-04-04T10:02:38Z

Okay, makes sense now, but there really should be some explanation about it in the PR/commit, which isn't the case currently ...

addons/product/views/product_views.xml

addons/account/models/product.py

william-andre

Please mark the PR as ready so that I can approve it when you are finished with the last changes :)

william-andre · 2022-04-04T14:36:14Z

addons/account/models/product.py

+
+    def _compute_tax_string(self):
+        for record in self:
+            record.tax_string = record.product_tmpl_id._construct_tax_string(record, record.lst_price)


This tracebacks, and it is missing dependencies

Please mark the PR as ready so that I can approve it when you are finished with the last changes :)

well, my fear is that because now _construct_tax_string is an instance method and the inheritance is delegation (so polymorphism won't work), it will be very confusing later when extending this. for now it is not an issue because the only attribute used to calculate the tax string is the price but imagine wanting to add other variables that might differ between product.product and product.template.

tl;dr
self in _construct_tax_string is always product_tmpl_id regardless of where it was called from

also why the style check fails with C0305, I added one line at the end

https://github.com/odoo/odoo/pull/87248/files#diff-6193fe8fb1a4b291ce7c82ef509968e9b381312909738d621adda02b2177807fR169

also why the style check fails with C0305, I added one line at the end

You have 2 end of line char instead of one, leading in one blank/useless line

self in _construct_tax_string is always product_tmpl_id regardless of where it was called from

How is that answering any of my remarks?
And why is that an issue?

I did fix the errors, I don't know what dependencies it misses.

this was another point.

the issue is that the intuitive behavior when someone sees this implementation will be that this is a normal case of polymorphism and can thus be extended further which is not the case

addons/account/models/product.py

If applied, this commit will fix the following bug by using the price with the additional cost added instead of only the price Steps to reproduce: 1- install sales 2- create a product p with variant pv 3- set an extra value for pv 4- go to Products>Product Variants 5- open the pv in form view 6- the Sales Price is the price of p not pv (which should be p + extra value) 7- also the tax string is calculated on p not pv Bug: ```list_price``` is being used Fix: replace ```list_price``` with ```lst_price``` OPW-2774199

william-andre

@robodoo r+

C3POdoo added the OE the report is linked to a support ticket (opw-...) label Mar 25, 2022

momegahed force-pushed the 15.0-product-OPW-2774199-fix-mome branch 5 times, most recently from 5853016 to b01d367 Compare March 30, 2022 14:49

adwid requested changes Mar 31, 2022

View reviewed changes

momegahed requested a review from adwid April 1, 2022 13:18

agr-odoo requested changes Apr 1, 2022

View reviewed changes

addons/product/views/product_views.xml Show resolved Hide resolved

addons/account/models/product.py Show resolved Hide resolved

Feyensv requested a review from william-andre April 4, 2022 10:34

momegahed force-pushed the 15.0-product-OPW-2774199-fix-mome branch 2 times, most recently from e8d78f6 to f141a15 Compare April 4, 2022 10:46

william-andre reviewed Apr 4, 2022

View reviewed changes

addons/product/views/product_views.xml Show resolved Hide resolved

addons/account/models/product.py Outdated Show resolved Hide resolved

addons/account/models/product.py Show resolved Hide resolved

momegahed force-pushed the 15.0-product-OPW-2774199-fix-mome branch 2 times, most recently from c136cb0 to a167afd Compare April 4, 2022 14:27

william-andre reviewed Apr 4, 2022

View reviewed changes

momegahed force-pushed the 15.0-product-OPW-2774199-fix-mome branch from a167afd to b1dbb41 Compare April 5, 2022 07:56

william-andre reviewed Apr 5, 2022

View reviewed changes

addons/account/models/product.py Show resolved Hide resolved

momegahed force-pushed the 15.0-product-OPW-2774199-fix-mome branch from b1dbb41 to 0503be2 Compare April 6, 2022 10:56

momegahed marked this pull request as ready for review April 6, 2022 11:04

C3POdoo requested a review from a team April 6, 2022 11:06

william-andre approved these changes Apr 7, 2022

View reviewed changes

robodoo closed this in 46526f6 Apr 7, 2022

robodoo temporarily deployed to merge April 7, 2022 20:01 Inactive

This was referenced Apr 7, 2022

[FW][FIX] product: variant sale price is wrong #88268

Closed

[FW][FIX] product: variant sale price is wrong #88269

Closed

[FW][FIX] product: variant sale price is wrong #88270

Closed

fw-bot deleted the 15.0-product-OPW-2774199-fix-mome branch April 21, 2022 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] product: variant sale price is wrong #87248

[FIX] product: variant sale price is wrong #87248

momegahed commented Mar 25, 2022 •

edited

Loading

robodoo commented Mar 25, 2022 •

edited

Loading

adwid left a comment

momegahed commented Apr 1, 2022

adwid commented Apr 1, 2022

momegahed commented Apr 1, 2022

agr-odoo left a comment •

edited

Loading

Feyensv commented Apr 4, 2022

agr-odoo commented Apr 4, 2022 •

edited

Loading

Feyensv commented Apr 4, 2022

william-andre left a comment

william-andre Apr 4, 2022

momegahed Apr 5, 2022

momegahed Apr 5, 2022

william-andre Apr 5, 2022

momegahed Apr 5, 2022

william-andre left a comment

[FIX] product: variant sale price is wrong #87248

[FIX] product: variant sale price is wrong #87248

Conversation

momegahed commented Mar 25, 2022 • edited Loading

robodoo commented Mar 25, 2022 • edited Loading

adwid left a comment

Choose a reason for hiding this comment

momegahed commented Apr 1, 2022

adwid commented Apr 1, 2022

momegahed commented Apr 1, 2022

agr-odoo left a comment • edited Loading

Choose a reason for hiding this comment

Feyensv commented Apr 4, 2022

agr-odoo commented Apr 4, 2022 • edited Loading

Feyensv commented Apr 4, 2022

william-andre left a comment

Choose a reason for hiding this comment

william-andre Apr 4, 2022

Choose a reason for hiding this comment

momegahed Apr 5, 2022

Choose a reason for hiding this comment

momegahed Apr 5, 2022

Choose a reason for hiding this comment

william-andre Apr 5, 2022

Choose a reason for hiding this comment

momegahed Apr 5, 2022

Choose a reason for hiding this comment

william-andre left a comment

Choose a reason for hiding this comment

momegahed commented Mar 25, 2022 •

edited

Loading

robodoo commented Mar 25, 2022 •

edited

Loading

agr-odoo left a comment •

edited

Loading

agr-odoo commented Apr 4, 2022 •

edited

Loading