[IMP] product: revamp #165016

mash-odoo · 2024-05-09T05:07:38Z

The following changes have been made in this commit:

In the product form, several strings have been renamed.
Moved a field Purchase taxes from the purchase tab to general information which on change of sales taxes recomputes purchase tax.
Moved the version and Product Template Tags to inventory and sales tabs respectively.
Hiding some fields when not required.
Enable tracking of invoicing policy and list price in the chatter.
A new tab for POS is introduced with necessary fields.
The Combo Choices Tab is removed and the fields under the tab are now pasted in the general info tab.
The stat buttons are reordered.
Removing the All Category and making a flat hierarchy.

Target version: master

Task: 3526304

robodoo · 2024-05-09T05:08:13Z

kcv-odoo

Thank for your work!!
Had first look at your PR looks like more things are broken then working need to rework and do proper testing 🙁
Could you move your demo data related and stat button position related changes in separate commit which we are sure they are not break anything? 🙂

I'll continue tomorrow if this is ready to review again and need to rework some points 😪

kcv-odoo · 2024-05-09T10:44:23Z

addons/account/models/product.py

+    @api.onchange('taxes_id')
+    def _onchange_taxes_id(self):
+        # On the change of sales tax, purchase tax should be set if the same amount is present else remain as it is
+        for product in self:
+            sales_taxes = product.taxes_id.filtered(lambda x: x.type_tax_use == 'sale')
+            # Get existing purchase taxes
+            existing_purchase_taxes = product.supplier_taxes_id.ids
+            purchase_taxes_ids = []
+
+            for tax in sales_taxes:
+                # Find purchase tax with the same amount as sales tax
+                matching_purchase_tax = self.env['account.tax'].search([('type_tax_use', '=', 'purchase'),
+                                                                        ('amount', '=', tax.amount)])
+                if matching_purchase_tax:
+                    purchase_taxes_ids.extend(matching_purchase_tax.ids)
+
+            # Update supplier_taxes_id only if there are new purchase taxes
+            if purchase_taxes_ids:
+                updated_purchase_taxes = list(set(existing_purchase_taxes) | set(purchase_taxes_ids))
+                product.supplier_taxes_id = [(6, 0, updated_purchase_taxes)]
+            else:
+                # If no matching purchase tax is found, retain the existing purchase taxes
+                product.supplier_taxes_id = [(6, 0, existing_purchase_taxes)]
+


Oh :()
What are you trying to do here 🤔
This look huge performance killer because you use search inside two for loops
Also why you need onchange method? can't this be achieved via compute? 👀

addons/account/views/product_view.xml

addons/point_of_sale/views/product_view.xml

addons/pos_self_order/views/product_views.xml

kcv-odoo · 2024-05-13T12:52:23Z

addons/event_sale/static/src/js/sale_product_field.js

+            ],
+            ['event_id'],
+        );
+    },


This seems too heavy 😢
May you could add a compute field checking that in python and add condition for that field here

Isn't there a way to specify a limit to the ´search_read` call ?
(same comment for the event_booth logic)

kcv-odoo · 2024-05-13T12:57:31Z

addons/website_sale_slides/models/website.py

    def sale_product_domain(self):
        return expression.AND([
            super(Website, self).sale_product_domain(),
-            [('detailed_type', '!=', 'course')],
+            [('detailed_type', '!=', 'service')],
        ])


This seems wrong
I don't think we need this condition here :(

kcv-odoo · 2024-05-13T13:02:35Z

addons/website_event_booth_sale/models/product_template.py

-
-    @api.model
-    def _get_product_types_allow_zero_price(self):
-        return super()._get_product_types_allow_zero_price() + ["event_booth"]


This has higher chances of breaking eCom flows
Like before if I add any product with zero price and this method return detailed type then it let me add product now I can't
For Ex Before this I can add Event product from eCom with Zero price but now I can't and it'll raise an error stating you can't add zero rupee product

Indeed, this logic must be somehow replaced.

kcv-odoo · 2024-05-13T13:05:10Z

addons/website_event_booth_sale/models/website.py

@@ -8,4 +8,4 @@ class Website(models.Model):
    _inherit = 'website'

    def sale_product_domain(self):
-        return ['&'] + super(Website, self).sale_product_domain() + [('detailed_type', '!=', 'event_booth')]
+        return ['&'] + super().sale_product_domain() + [('detailed_type', '!=', 'service')]


This is wrong 🙈
Remove this method

kcv-odoo · 2024-05-13T13:05:29Z

addons/website_event_sale/models/product.py

-
-    @api.model
-    def _get_product_types_allow_zero_price(self):
-        return super()._get_product_types_allow_zero_price() + ["event"]


same as above

FYI
This method does not called now for events so removing this method for event, still this case happen for booth and course I guess
Related PR to remove zero price event in cart and create so line

Need to check with SM PO if we can do same for booth and course 🤔 (In new task obviously in SM )

svs-odoo · 2024-05-13T13:13:01Z

addons/product/data/product_data.xml

            <field name="name">Expenses</field>
        </record>
+        <record id="cat_services" model="product.category">


I think it could be good to have more consistent name. I suggest you product_category_whatever so we have the model's name followed by the actual category.
I think having only category_whatever could also be OK in this case since we use the module name to find the record (product.category_whatever), but if product category data are defined in another module where other kind of category are created (UOM or tagt category for example), it could be annoying.
Anyway, having consistent name is better, since this PR targets master, nothing stop you to make it so :)

svs-odoo · 2024-05-13T16:00:49Z

addons/product/data/product_category_demo.xml

    <record id="product_category_4" model="product.category">
-        <field name="parent_id" ref="product.product_category_1"/>
+        <field name="parent_id" ref="product.cat_services"/>


Should be cool to give better and more relevant name to those categories too

svs-odoo · 2024-05-13T16:02:21Z

addons/product/models/product_template.py

@@ -22,7 +22,7 @@ class ProductTemplate(models.Model):
    @tools.ormcache()
    def _get_default_category_id(self):
        # Deletion forbidden (at least through unlink)
-        return self.env.ref('product.product_category_all')
+        return self.env.ref('product.product_category_5')


Previously, the default category was a data, now it's a demo data, so not always installed. Sound like a bad idea IMO

svs-odoo · 2024-05-13T16:04:13Z

addons/stock/tests/test_inventory.py

@@ -19,13 +19,13 @@ def setUpClass(cls):
        cls.product1 = cls.env['product.product'].create({
            'name': 'Product A',
            'type': 'product',
-            'categ_id': cls.env.ref('product.product_category_all').id,
+            'categ_id': cls.env.ref('product.cat_services').id,


For all the stock related tests, you should use a category who reflect better a storable product.
It is not so meaningful for now but it would better reflect real use case.
I guess furniture should be more appropriate

Feyensv

Well, it seems like a loooot of side-effects and existing logics were not properly considered in the current task specification.

Detailed types: Need more time to think about it, but I feel like it won't be trivial to handle all existing flows.

I believe this should be done in separate ordered commits:

UX changes (moving fields from one page to another, button ordering, field renamings, ...)
Merging product and consu into Goods
Other detailed types changes
Categories cleanup
SLA improvements (already seems to be in a separate commit on enterprise 👍)

This will allow us to see more clearly what's done for what part of the spec, what is working fine or not (and maybe has to be postponed for next freeze).

Edit 4PM: Categories are still required, but since we can now create a category on the fly, it's ok (will maybe annoy a bit the product imports, but can be easily solved imho). Making the categories not required will be for another task if needed, but won't be done here.

Feyensv · 2024-05-14T11:08:48Z

addons/event_sale/models/product.py

@@ -1,24 +1,6 @@
 # -*- coding: utf-8 -*-


replace coding by license, and rename file to product_product.py

Feyensv · 2024-05-14T11:09:29Z

addons/event_sale/models/sale_order.py

@@ -15,7 +15,7 @@ def write(self, vals):
        in website_sale controller shop/address that updates customer, but not
        only. """
        result = super(SaleOrder, self).write(vals)
-        if any(line.product_type == 'event' for line in self.order_line) and vals.get('partner_id'):
+        if any(line.product_type == 'service' for line in self.order_line) and vals.get('partner_id'):


Suggested change

if any(line.product_type == 'service' for line in self.order_line) and vals.get('partner_id'):

if any(line.event_id for line in self.order_line) and vals.get('partner_id'):

Feyensv · 2024-05-14T11:10:45Z

addons/event_sale/models/sale_order.py

@@ -66,4 +63,4 @@ def _get_product_catalog_domain(self):
        :rtype: list
        """
        domain = super()._get_product_catalog_domain()
-        return expression.AND([domain, [('detailed_type', '!=', 'event')]])
+        return expression.AND([domain, [('detailed_type', '!=', 'service')]])


With this, you won't see any service in the catalog, this is not the expected behavior

Feyensv · 2024-05-14T11:12:01Z

addons/event_sale/static/src/js/event_configurator_controller.js

@@ -6,7 +6,7 @@ import { formView } from "@web/views/form/form_view";

 /**
 * This controller is overridden to allow configuring sale_order_lines through a popup
- * window when a product with 'detailed_type' == 'event' is selected.
+ * window when a product with 'detailed_type' == 'service' is selected.


Suggested change

* window when a product with 'detailed_type' == 'service' is selected.

* window when a service product linked to events is selected.

Feyensv · 2024-05-14T11:12:49Z

addons/event_sale/static/src/js/sale_product_field.js

+            ],
+            ['event_id'],
+        );
+    },


Isn't there a way to specify a limit to the ´search_read` call ?
(same comment for the event_booth logic)

Feyensv · 2024-05-14T11:26:24Z

addons/product_matrix/data/product_matrix_demo.xml

@@ -72,7 +72,7 @@

    <record id="matrix_product_template_shirt" model="product.template">


Is the tshirt category used somewhere else?
If not, it can be moved into the demo data of product_matrix

Feyensv · 2024-05-14T11:29:06Z

addons/purchase/views/product_views.xml

@@ -116,7 +116,7 @@
            <field name="model">product.template</field>
            <field name="inherit_id" ref="product.product_template_only_form_view"/>
            <field name="arch" type="xml">
-                <div name="button_box" position="inside">


Instead of targeting specific elements, can't this be handled with views priorities ?

Feyensv · 2024-05-14T11:29:49Z

addons/repair/views/product_views.xml

@@ -6,7 +6,7 @@
        <field name="model">product.template</field>
        <field name="inherit_id" ref="product.product_template_form_view"/>
        <field name="arch" type="xml">
-            <xpath expr="//group[@name='group_general']/field[@name='uom_po_id']" position="after">
+            <xpath expr="//group[@name='group_general']" position="inside">


Suggested change

<xpath expr="//group[@name='group_general']" position="inside">

<group name="group_general" position="inside">

Feyensv · 2024-05-14T11:33:14Z

addons/website_event_booth_sale/models/product_template.py

-
-    @api.model
-    def _get_product_types_allow_zero_price(self):
-        return super()._get_product_types_allow_zero_price() + ["event_booth"]


Indeed, this logic must be somehow replaced.

Feyensv · 2024-05-14T11:33:53Z

addons/website_event_booth_sale/models/sale_order.py

@@ -26,7 +26,7 @@ def _cart_find_product_line(
    def _verify_updated_quantity(self, order_line, product_id, new_qty, **kwargs):
        """Forbid quantity updates on event booth lines."""
        product = self.env['product.product'].browse(product_id)
-        if product.detailed_type == 'event_booth' and new_qty > 1:
+        if product.detailed_type == 'service' and new_qty > 1:


Nope, this will fail whenever we increase the quantity of a SOL with a service product.

tde-banana-odoo · 2024-05-17T09:22:26Z

addons/event_booth_sale/models/product_product.py

+        search="_search_is_event_booth",
+    )
+
+    def _compute_is_event_booth(self):


That's a really bad idea. It means you cannot create your product then link it. You are going to add a lot of code complexity (read group, ...) and break flows just to remove one field ? Not sure it makes sense...

You can create a service product and then link it, since the domain on events is now products of service type.
Nevertheless, I kinda agree that the detailed type changes are not the best part of this spec :D (fp-request though :c)

I suggest to postpone to next freeze so that we calmy check and test everything, especially LNA is waaaay busy those days. Don't think people will die if they wait for 2 months, will they ?

Well, our PO is on holidays for the freeze, and he didn't test the task either :D.
We'll try to merge the true back 2 basics change of this task for the freeze, and the rest will probably wait for next freeze (and more debates/testing with PO's/FP)

Indeed, don't think we have to rush model changes in intermediate saas, does not seem to be the critical point right now.

kcv-odoo · 2024-05-17T13:14:08Z

Remove UX and Service related commit in favor of #165935 and https://github.com/odoo/enterprise/pull/62741

* Drop hide_from_shop logic, we don't want that, users shouldn't publish the event/event_booth/... products * make is_event_ticket/is_event_booth work, only check if there are linked booths/tickets, not if event is valid or in the future, they shouldn't use those products for other logics * restore the configurator logic as before, keep changes minimal when possible * is_add_to_cart_allowed now gets the cart update params, to know more cleanly/easily when one type of product is allowed or not.

kcv-odoo · 2024-05-23T10:16:43Z

Close in favor of #166497

C3POdoo added the RD research & development, internal work label May 9, 2024

mash-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch from 3b29d2a to e4c5e23 Compare May 13, 2024 11:36

kcv-odoo reviewed May 13, 2024

View reviewed changes

mash-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch from e4c5e23 to 0c0c28a Compare May 13, 2024 13:47

svs-odoo reviewed May 13, 2024

View reviewed changes

mash-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch 4 times, most recently from dbf894b to baaf075 Compare May 14, 2024 11:06

Feyensv requested changes May 14, 2024

View reviewed changes

mash-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch from baaf075 to 6cd6eec Compare May 14, 2024 12:14

kcv-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch 18 times, most recently from 760826e to de24f62 Compare May 16, 2024 05:18

kcv-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch 8 times, most recently from e70f4b2 to c5b406c Compare May 16, 2024 13:25

Feyensv force-pushed the master-product-imp-back-2-basics-part-1-mash branch 3 times, most recently from 26a36a4 to 8fa4c1b Compare May 16, 2024 20:32

kcv-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch from 7ccdb33 to 1069418 Compare May 17, 2024 09:20

tde-banana-odoo reviewed May 17, 2024

View reviewed changes

kcv-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch 2 times, most recently from f2a4715 to 9162cca Compare May 17, 2024 13:12

kcv-odoo and others added 4 commits May 21, 2024 14:37

[IMP] event: wip revamp

1abe77b

[IMP] FIX up runbot

f1f3a90

[IMP] LNA review

eeb6b4f

kcv-odoo force-pushed the master-product-imp-back-2-basics-part-1-mash branch from 9162cca to eeb6b4f Compare May 21, 2024 09:46

kcv-odoo closed this May 23, 2024

kcv-odoo deleted the master-product-imp-back-2-basics-part-1-mash branch May 23, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] product: revamp #165016

[IMP] product: revamp #165016

mash-odoo commented May 9, 2024

robodoo commented May 9, 2024

kcv-odoo left a comment •

edited

kcv-odoo May 9, 2024

kcv-odoo May 13, 2024

Feyensv May 14, 2024

kcv-odoo May 13, 2024

kcv-odoo May 13, 2024

Feyensv May 14, 2024

kcv-odoo May 13, 2024

kcv-odoo May 13, 2024 •

edited

kcv-odoo May 16, 2024 •

edited

svs-odoo May 13, 2024 •

edited

svs-odoo May 13, 2024

svs-odoo May 13, 2024

svs-odoo May 13, 2024

Feyensv left a comment •

edited

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

Feyensv May 14, 2024

tde-banana-odoo May 17, 2024

Feyensv May 17, 2024

tde-banana-odoo May 17, 2024

Feyensv May 17, 2024

tde-banana-odoo May 17, 2024

kcv-odoo commented May 17, 2024

kcv-odoo commented May 23, 2024

	if any(line.product_type == 'service' for line in self.order_line) and vals.get('partner_id'):
	if any(line.event_id for line in self.order_line) and vals.get('partner_id'):

	* window when a product with 'detailed_type' == 'service' is selected.
	* window when a service product linked to events is selected.

		@@ -72,7 +72,7 @@

		<record id="matrix_product_template_shirt" model="product.template">

	<xpath expr="//group[@name='group_general']" position="inside">
	<group name="group_general" position="inside">

[IMP] product: revamp #165016

[IMP] product: revamp #165016

Conversation

mash-odoo commented May 9, 2024

robodoo commented May 9, 2024

kcv-odoo left a comment • edited

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

kcv-odoo May 13, 2024 • edited

Choose a reason for hiding this comment

kcv-odoo May 16, 2024 • edited

Choose a reason for hiding this comment

svs-odoo May 13, 2024 • edited

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

Feyensv left a comment • edited

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

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

kcv-odoo commented May 17, 2024

kcv-odoo commented May 23, 2024

kcv-odoo left a comment •

edited

kcv-odoo May 13, 2024 •

edited

kcv-odoo May 16, 2024 •

edited

svs-odoo May 13, 2024 •

edited

Feyensv left a comment •

edited