[ADD] product_pricelist_margin #1561

nguyenminhchien · 2024-03-21T04:17:53Z

This module shows the product's cost and margin from the pricelists.

santostelmo · 2024-03-21T07:08:54Z

product_pricelist_margins/models/product_pricelist_item.py

+    )
+
+    @api.depends("fixed_price", "cost")
+    def _compute_margin(self):


Can you please add a unit test the behavior of this method with using pricelist item with a different price computation ?
For example : percentage or formula

legalsylvain

Interesting module. However, if you want to compute margin you should depends on account to take into account the vat that can be included.

See similar module : https://github.com/OCA/product-attribute/blob/16.0/product_pricelist_simulation_margin/wizards/wizard_preview_pricelist.py#L42

santostelmo · 2024-03-22T06:58:32Z

product_pricelist_margins/tests/test_margin.py

+        with Form(self.line) as line:
+            line.compute_price = "percentage"
+            self.assertEqual(line.cost, 20.0)
+            self.assertEqual(line.margin, 0)


We apply 20% discount and the margin is 0 ? 🤔

cyrilmanuel · 2024-04-16T08:33:21Z

Hii @legalsylvain is it good for you ?

have a great day

legalsylvain · 2024-04-16T09:42:03Z

No. Margin is still bad computed. See previous comment.

nguyenminhchien · 2024-07-11T07:10:49Z

No. Margin is still bad computed. See previous comment.

Hi @legalsylvain , Mr. @chaule97 just improved the compute function. Please have a look. Thanks.

chaule97 · 2024-07-11T07:12:40Z

Hi @legalsylvain , I have updated as per your request. Hope it is good for you

legalsylvain

Hi. code review and test. Some remarks inline.
Also, UX improvment proposal, to save place :

Thanks a lot for your contribution !

legalsylvain · 2024-07-11T07:25:56Z

product_pricelist_margins/__manifest__.py

+    "website": "https://github.com/OCA/product-attribute",
+    "author": "Camptocamp, Odoo Community Association (OCA)",
+    "license": "AGPL-3",
+    "depends": ["product"],


Suggested change

"depends": ["product"],

"depends": ["account"],

account_tax.compute_all() function is in account module

This one is still pending.

legalsylvain · 2024-07-11T07:38:33Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+                <field name="margin" />
+                <field name="margin_percent" />


Suggested change

<field name="margin" />

<field name="margin_percent" />

<field name="margin" optional="show"/>

<field name="margin_percent" optional="show"/>

allow users to hide percent and / or net value if they don't need the information.

legalsylvain · 2024-07-11T07:40:40Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+                <field name="margin" />
+                <field name="margin_percent" />


Suggested change

<field name="margin" />

<field name="margin_percent" />

<field name="margin" optional="show" />

<field name="margin_percent" optional="show" />

legalsylvain · 2024-07-11T07:47:22Z

product_pricelist_margins/views/product_pricelist_views.xml

+                <field name="margin" />
+                <field name="margin_percent" />


Suggested change

<field name="margin" />

<field name="margin_percent" />

<field name="margin" optional="show" />

<field name="margin_percent" optional="show" />

legalsylvain · 2024-07-11T07:47:37Z

product_pricelist_margins/views/product_pricelist_views.xml

+                <field name="margin" />
+                <field name="margin_percent" />


Suggested change

<field name="margin" />

<field name="margin_percent" />

<field name="margin" optional="show"/>

<field name="margin_percent" optional="show"/>

legalsylvain · 2024-07-11T07:48:26Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+        <field name="model">product.pricelist.item</field>
+        <field name="arch" type="xml">
+            <field name="fixed_price" position="after">
+                <field name="cost" />


Suggested change

<field name="cost" />

<field name="cost" invisible="1" />

legalsylvain · 2024-07-11T07:48:37Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+        />
+        <field name="arch" type="xml">
+            <field name="fixed_price" position="after">
+                <field name="cost" />


Suggested change

<field name="cost" />

<field name="cost" invisible="1" />

product_pricelist_margins/models/product_pricelist_item.py

legalsylvain · 2024-07-11T07:54:30Z

product_pricelist_margins/models/product_pricelist_item.py

+        for item in self:
+            margin = percentage = 0
+            if not float_is_zero(
+                item.fixed_price, precision_digits=item.currency_id.rounding


Do not use fixed_price. better user the computation of the price done by "product.pricelist". so the value is correct for all computation type.

legalsylvain · 2024-07-11T07:56:18Z

product_pricelist_margins/models/product_pricelist_item.py

+        compute="_compute_margin",
+    )
+
+    @api.depends("fixed_price", "product_tmpl_id", "cost")


It depends on a lot of field in fact : compute_price, applied_on, percent_price, "base", "price_discount", price_surcharge, price_round, price_min_margin.

chaule97 · 2024-07-11T11:32:05Z

Hi @legalsylvain , I have updated. I hope it was fine

legalsylvain

Thanks ! some remaining points inline.
the rests looks great to me !

legalsylvain · 2024-07-11T14:33:39Z

product_pricelist_margins/__manifest__.py

+    "website": "https://github.com/OCA/product-attribute",
+    "author": "Camptocamp, Odoo Community Association (OCA)",
+    "license": "AGPL-3",
+    "depends": ["product"],


This one is still pending.

legalsylvain · 2024-07-11T14:36:03Z

product_pricelist_margins/models/product_pricelist_item.py

+        ):
+            margin = percentage = 0
+
+            price_rule = item.pricelist_id._compute_price_rule(item.product_tmpl_id, 1)


I guess that 1 is the quantity. If yes, it should be replaced by the min quantity of the item. don't you think ?

legalsylvain · 2024-07-11T14:36:32Z

product_pricelist_margins/models/product_pricelist_item.py

+    def _compute_margin(self):
+        for item in self.filtered(
+            lambda x: x.applied_on in ("1_product", "0_product_variant")
+            and x.compute_price == "fixed"


Suggested change

and x.compute_price == "fixed"

I don't understand this line. The algorithm can work for all compute_price method. don't you think ?

legalsylvain · 2024-07-11T14:36:42Z

product_pricelist_margins/models/product_pricelist_item.py

+
+        for item in self.filtered(
+            lambda x: x.applied_on not in ("1_product", "0_product_variant")
+            or x.compute_price != "fixed"


Suggested change

or x.compute_price != "fixed"

same here.

chaule97 · 2024-07-15T03:57:57Z

Hi @legalsylvain , I have updated it. please review again

cyrilmanuel

not an expert but LGTM

product_pricelist_margins/models/product_pricelist_item.py

santostelmo

Minor comment (optional). Otherwise LG

simahawk · 2024-07-22T11:58:12Z

product_pricelist_margins/__manifest__.py

+# Copyright 2024 Camptocamp SA
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+{
+    "name": "Product Pricelist Margins",


module name should not end w/ plural form. Please rename it to _margin.
See pt 1 https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#11modules

Hi @simahawk, I have updated it. Thanks for your suggestion

simahawk · 2024-07-22T11:59:42Z

product_pricelist_margins/tests/test_margin.py

+
+
+@tagged("post_install", "-at_install")
+class TestMargin(TransactionCase):


pls usa BaseCommon as base class to disable tracking

legalsylvain · 2024-07-22T19:04:16Z

product_pricelist_margins/models/product_pricelist_item.py

+        "price_min_margin",
+        "product_tmpl_id",
+        "cost",
+        "min_quantity",


Suggested change

"min_quantity",

"min_quantity",

"fixed_price",

legalsylvain · 2024-07-22T19:09:49Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+        <field name="inherit_id" ref="product.product_pricelist_item_tree_view" />
+        <field name="arch" type="xml">
+            <field name="price" position="after">
+                <field name="cost" />


Suggested change

<field name="cost" />

<field name="cost" invisible="1"/>

legalsylvain · 2024-07-22T19:11:28Z

product_pricelist_margins/views/product_pricelist_views.xml

+                <field name="margin" optional="show" />
+                <field name="margin_percent" optional="show" />


Suggested change

<field name="margin" optional="show" />

<field name="margin_percent" optional="show" />

<field name="margin" optional="show" attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}" />

<field name="margin_percent" optional="show" attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}" />

legalsylvain · 2024-07-22T19:12:00Z

product_pricelist_margins/views/product_pricelist_views.xml

+                <field name="margin" optional="show" />
+                <field name="margin_percent" optional="show" />


Suggested change

<field name="margin" optional="show" />

<field name="margin_percent" optional="show" />

<field name="margin" optional="show" attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}"/>

<field name="margin_percent" optional="show" attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}"/>

legalsylvain · 2024-07-22T19:15:29Z

product_pricelist_margins/views/product_pricelist_item_views.xml

+        <field name="model">product.pricelist.item</field>
+        <field name="arch" type="xml">
+            <xpath expr="//group[@name='pricelist_rule_base']/div" position="after">
+                <group attrs="{'invisible': [('compute_price', '!=', 'fixed')]}">


Suggested change

<group attrs="{'invisible': [('compute_price', '!=', 'fixed')]}">

<group attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}">

chaule97 · 2024-07-23T03:31:36Z

Hi @legalsylvain , I have updated it. please review again

legalsylvain

Thanks for your contribution !

simahawk

A little bit of cleanup is still required 😉
Please:

find n replace product_pricelist_margins everywhere (eg: translations)
rewrite the commit message as well

chaule97 · 2024-07-25T08:15:00Z

A little bit of cleanup is still required 😉 Please:
* find n replace `product_pricelist_margins` everywhere (eg: translations)

* rewrite the commit message as well

Thanks @simahawk . I have replaced all of them

santostelmo · 2024-11-05T08:08:11Z

product_pricelist_margin/models/product_pricelist_item.py

+                item.margin_percent = 0
+                continue
+
+            res = item.product_tmpl_id.taxes_id.compute_all(


Suggested change

res = item.product_tmpl_id.taxes_id.compute_all(

current_company = self.env.company

res = item.product_tmpl_id.taxes_id.filtered(lambda t: t.company_id and t.company_id == current_company).compute_all(

price,

item.currency_id,

product=item.product_tmpl_id,

)

taxe_id is a M2M. In multicompany mode all taxes linked to this product (doesn't matter the company) will be taken into account to compute total_excluded. That is not correct from the point of view of the user. IMO we should take into account only the products taxes belonging to the current logged company.

I have updated it

legalsylvain

quick review. no test.
LGTM. Thanks for applying change requests !

nguyenminhchien marked this pull request as ready for review March 21, 2024 04:21

nguyenminhchien force-pushed the 16.0-mig-product_pricelist_margins branch from d577643 to 9e28a94 Compare March 21, 2024 04:43

santostelmo suggested changes Mar 21, 2024

View reviewed changes

legalsylvain requested changes Mar 21, 2024

View reviewed changes

nguyenminhchien force-pushed the 16.0-mig-product_pricelist_margins branch from 9e28a94 to 03916fe Compare March 22, 2024 02:13

nguyenminhchien requested a review from santostelmo March 22, 2024 02:20

santostelmo reviewed Mar 22, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch 3 times, most recently from 0905b26 to a238034 Compare July 11, 2024 05:21

nguyenminhchien requested review from legalsylvain and santostelmo July 11, 2024 07:09

legalsylvain requested changes Jul 11, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from a238034 to 87b6e36 Compare July 11, 2024 11:29

legalsylvain reviewed Jul 11, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch 2 times, most recently from 8b93a6b to aa06320 Compare July 15, 2024 03:52

cyrilmanuel approved these changes Jul 15, 2024

View reviewed changes

santostelmo reviewed Jul 22, 2024

View reviewed changes

product_pricelist_margins/models/product_pricelist_item.py Outdated Show resolved Hide resolved

santostelmo approved these changes Jul 22, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from aa06320 to f2b6cdb Compare July 22, 2024 05:24

simahawk requested changes Jul 22, 2024

View reviewed changes

legalsylvain reviewed Jul 22, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from f2b6cdb to 8fc7577 Compare July 23, 2024 03:25

legalsylvain approved these changes Jul 23, 2024

View reviewed changes

legalsylvain requested a review from simahawk July 23, 2024 10:00

simahawk changed the title ~~[ADD] product_pricelist_margins~~ [ADD] product_pricelist_margin Jul 25, 2024

simahawk requested changes Jul 25, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from 8fc7577 to 96e33be Compare July 25, 2024 08:09

santostelmo suggested changes Nov 5, 2024

View reviewed changes

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from 96e33be to 612e178 Compare November 5, 2024 08:23

legalsylvain approved these changes Nov 5, 2024

View reviewed changes

chaule97 mentioned this pull request Nov 7, 2024

[16.0][FIX] add addClassCleanup in testcase #1763

Merged

[ADD] product_pricelist_margin

2dc3934

chaule97 force-pushed the 16.0-mig-product_pricelist_margins branch from 612e178 to 2dc3934 Compare December 1, 2024 01:12



		@tagged("post_install", "-at_install")
		class TestMargin(TransactionCase):

		<field name="margin" optional="show" />
		<field name="margin_percent" optional="show" />

	<group attrs="{'invisible': [('compute_price', '!=', 'fixed')]}">
	<group attrs="{'invisible': [('applied_on', 'not in', ["1_product", "0_product_variant"])]}">

-            res = item.product_tmpl_id.taxes_id.compute_all(
+            current_company = self.env.company
+            res = item.product_tmpl_id.taxes_id.filtered(lambda t: t.company_id and t.company_id == current_company).compute_all(
+                price,
+                item.currency_id,
+                product=item.product_tmpl_id,
+            )

[ADD] product_pricelist_margin #1561

Are you sure you want to change the base?

[ADD] product_pricelist_margin #1561

Conversation

nguyenminhchien commented Mar 21, 2024 • edited by simahawk Loading

Choose a reason for hiding this comment

legalsylvain left a comment

Choose a reason for hiding this comment

santostelmo Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

cyrilmanuel commented Apr 16, 2024

legalsylvain commented Apr 16, 2024

nguyenminhchien commented Jul 11, 2024

chaule97 commented Jul 11, 2024

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaule97 commented Jul 11, 2024

legalsylvain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaule97 commented Jul 15, 2024

cyrilmanuel left a comment

Choose a reason for hiding this comment

santostelmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaule97 commented Jul 23, 2024

legalsylvain left a comment

Choose a reason for hiding this comment

simahawk left a comment

Choose a reason for hiding this comment

chaule97 commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legalsylvain left a comment

Choose a reason for hiding this comment

nguyenminhchien commented Mar 21, 2024 •

edited by simahawk

Loading

santostelmo Mar 22, 2024 •

edited

Loading

chaule97 commented Jul 25, 2024 •

edited

Loading