Skip to content

[18.0] [MIG] product_configurator: Migration to 18.0 #150

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

Open
wants to merge 529 commits into
base: 18.0
Choose a base branch
from

Conversation

bizzappdev
Copy link

No description provided.

Pooja Khandelwal and others added 30 commits February 3, 2025 11:30
…ct_configurator_purchase : create test case for after or before validate the product then it id or sol is equal or not
…bute and compute_domain and solved the error come in _check_constraint_min_max_value method
@bizzappdev bizzappdev force-pushed the 18.0-mig-product_configurator-BAD branch from 5bf3c75 to 439b635 Compare February 13, 2025 08:23
@bizzappdev bizzappdev marked this pull request as ready for review February 14, 2025 08:02
Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Thanks for the work done! 🙏🏻

@@ -421,7 +422,10 @@ def get_cfg_weight(self, value_ids=None, custom_vals=None):

self = self.with_context(active_id=product_tmpl.id)

value_ids = flatten(value_ids)
# Use the updated flatten method (using itertools.chain)

Choose a reason for hiding this comment

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

Suggested change
# Use the updated flatten method (using itertools.chain)

no need to clarify this in the code

value_ids = flatten(value_ids)
# Use the updated flatten method (using itertools.chain)
value_ids = list(
chain.from_iterable(v if isinstance(v, list) else [v] for v in value_ids)

Choose a reason for hiding this comment

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

Consider also other iterables (ie: tuple)

from collections.abc import Iterable
Suggested change
chain.from_iterable(v if isinstance(v, list) else [v] for v in value_ids)
chain.from_iterable(v if isinstance(v, Iterable) else [v] for v in value_ids)

@@ -504,8 +508,6 @@ def _compute_currency_id(self):
product_preset_id = fields.Many2one(
comodel_name="product.product",
string="Preset",
domain="[('product_tmpl_id', '=', product_tmpl_id),\
('config_preset_ok', '=', True)]",
Copy link

@ivantodorovich ivantodorovich Feb 17, 2025

Choose a reason for hiding this comment

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

What's the motivation for removing the domain here and moving it to the view? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

We will have to move the domain to the level of XML
As in v18, domains based on related fields are not carried forward. https://github.com/odoo/odoo/blob/18.0/odoo/fields.py#L3067
product_preset_id field is defined at the product.config.session level and the product.configurator inherits the original object. Which makes that field related.

open_step_lines = list(
map(lambda x: "%s" % (x), self.get_open_step_lines().ids)
)
open_step_lines = list(map(lambda x: f"{x}", self.get_open_step_lines().ids))

Choose a reason for hiding this comment

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

Suggested change
open_step_lines = list(map(lambda x: f"{x}", self.get_open_step_lines().ids))
open_step_lines = list(map(str, self.get_open_step_lines().ids))

@@ -1173,7 +1173,7 @@ def check_and_open_incomplete_step(self, value_ids=None, custom_value_ids=None):
step_to_open = step
break
if step_to_open:
return "%s" % (step_to_open.id)
return f"{step_to_open.id}"

Choose a reason for hiding this comment

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

str(step_to_open.id)

}
}

.pull-right {

Choose a reason for hiding this comment

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

We can just use bootstrap's float-end class and remove this one


from lxml import etree

from odoo import _, api, fields, models

Choose a reason for hiding this comment

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

Use self.env._ instead


TODO: This only works when activating the selection not when typing
"""
product_tmpl_id = self.env.context.get("_cfg_product_tmpl_id")

Choose a reason for hiding this comment

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

Can we take the opportunity to write unit tests for this method? 🙏🏻

Copy link
Author

Choose a reason for hiding this comment

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

We take care of the test cases also in this MR

]
return prices

def encode_custom_values(self, custom_vals):

Choose a reason for hiding this comment

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

Is this method actually used somewhere? 🤔
if not, let's remove it

)
return list(flat_val_ids)

def formatPrices(self, prices=None, dp="Product Price"):

Choose a reason for hiding this comment

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

Is this method actually used somewhere? 🤔
if not, let's remove it

Copy link
Author

Choose a reason for hiding this comment

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

In the core modules, it is not used, But I still remember some of the custom modules and other companies were also using this method in their modules.

@bizzappdev bizzappdev marked this pull request as draft February 18, 2025 05:53
@bizzappdev bizzappdev force-pushed the 18.0-mig-product_configurator-BAD branch 3 times, most recently from 5faec59 to 16a3618 Compare February 19, 2025 14:24
@bizzappdev
Copy link
Author

@ivantodorovich Thank you so much for your review and for pointing out the missing points and improvements. We have made the necessary changes.

@bizzappdev bizzappdev marked this pull request as ready for review February 19, 2025 14:28
@bizzappdev bizzappdev force-pushed the 18.0-mig-product_configurator-BAD branch from 16a3618 to 5ee2444 Compare February 27, 2025 07:57
@bizzappdev bizzappdev force-pushed the 18.0-mig-product_configurator-BAD branch from 5ee2444 to 65d031f Compare February 27, 2025 09:37
Copy link

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

@yterrettaz yterrettaz left a comment

Choose a reason for hiding this comment

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

Functional test OK on the base data part of the product configurator. Variant management, restrictions, images, and names are handled. Test based on the PR available on the runbot (MRP). The sales part is still missin

@bizzappdev
Copy link
Author

The sales part is still missing

@yterrettaz
For the sales part, you are most welcome to test and provide your feedback at #151

@yterrettaz
Copy link

The sales part is still missing

@yterrettaz For the sales part, you are most welcome to test and provide your feedback at #151

Yes, with pleasure, but I don't have a runbot for that yet.

@Killance
Copy link

Killance commented May 28, 2025

Hello, I would like to suggest a commit.

The changes concern an optimization that avoid the recompute of the attribute's conditions for every view generation. These are now computed and stored on the product.template.attribute.line. The difference is notable mainly for complexe configuration and for the portal part.

Secondly, the changes split the compute of the conditions into three distinct methods, it should be easier to inherit them to have more complexe attribute's conditions.

Killance@cd4c617

@BenjaHe
Copy link

BenjaHe commented Jun 3, 2025

Hello, I would like to suggest a commit.

The changes concern an optimization that avoid the recompute of the attribute's conditions for every view generation. These are now computed and stored on the product.template.attribute.line. The difference is notable mainly for complexe configuration and for the portal part.

Secondly, the changes split the compute of the conditions into three distinct methods, it should be easier to inherit them to have more complexe attribute's conditions.

Killance@63f4677

This change make the required, readonly and invisible conditions easier to understand.
It would be appreciate to have it in the product_configurator module.
@bizzappdev , what do you think about it ?
Could you cherry-pick this commit in your branche ?

@bizzappdev
Copy link
Author

@BenjaHe We will check and test for functionality and performance. also might be for future compatibility for website module.

@gurneyalex
Copy link
Member

@bizzappdev Any news on the handling of the proposed patch?

@yterrettaz
Copy link

I just opened an issue for version 18 -> #161

@bizzappdev
Copy link
Author

@bizzappdev Any news on the handling of the proposed patch?

@gurneyalex We have scheduled this for this week's sprint. Most probably it will be ready by next Monday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.