Skip to content

Conversation

chaule97
Copy link

@chaule97 chaule97 commented Jan 6, 2025

mmequignon and others added 4 commits January 2, 2025 10:19
Define route_ids as computed on the product variant, in order to
reuse route_ids from its template and add or remove MTO route
according to the setting on the variant.

In case MTO route is changed on the template, it must reset any
variant specific setting.
@chaule97 chaule97 force-pushed the 18.0-mig-stock_product_variant_mto branch 2 times, most recently from 4889262 to d557475 Compare January 10, 2025 08:00
@rousseldenis
Copy link
Contributor

/ocabot migration stock_product_variant_mto

Copy link
Contributor

@grindtildeath grindtildeath 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 mig.

However, IMO we should keep migration commit to a minimum in order to highlight the required changes and avoid rewriting for cosmetic changes or fixing typos

compute="_compute_route_ids",
domain="[('product_selectable', '=', True)]",
store=False,
store=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we define a search function instead?

Copy link
Author

Choose a reason for hiding this comment

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

I think not, because compute can track when is_mto was changed

product.route_ids = product.product_tmpl_id.route_ids + mto_route
continue
product.route_ids = product.product_tmpl_id.route_ids
new_route_ids = product.product_tmpl_id.route_ids
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking a little, but this is a recordset, not a list of ids. Also these ain't new routes but the routes from the template. I would call this variable something like template_routes or product_routes.

However, all this rewriting does not bring anything in the context of module migration as the conditions are the same as before... IMO it only adds more work for the reviewer...

Comment on lines 21 to 33
templates_mto_added = template_not_mto_before & templates_mto_after
templates_mto_removed = (self - template_not_mto_before) & (
self - templates_mto_after
)
(
templates_mto_added = templates_not_mto_before & templates_mto_after
templates_mto_removed = self - templates_mto_after - templates_not_mto_before

affected_product_variants = (
templates_mto_added | templates_mto_removed
).product_variant_ids._compute_is_mto()
).product_variant_ids
if affected_product_variants:
affected_product_variants._compute_is_mto()
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically these are pure cosmetic change that will only obfuscate the original contribution when blaming the code, I don't think it must be part of a migration.

@chaule97 chaule97 force-pushed the 18.0-mig-stock_product_variant_mto branch from d557475 to bbc05c8 Compare February 5, 2025 04:24
@rousseldenis
Copy link
Contributor

@chaule97 @grindtildeath @jbaudoux Maybe it should have been great to merge this one with that one : https://github.com/OCA/product-attribute/tree/16.0/product_route_mto

Comment on lines +1 to +2
This module allows to define if a product variant can use the Make To
Order route without any dependency on its template routes settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a big warning that the routes are moved from product template to product product !

@jbaudoux
Copy link
Contributor

jbaudoux commented Feb 5, 2025

@chaule97 @grindtildeath @jbaudoux Maybe it should have been great to merge this one with that one : https://github.com/OCA/product-attribute/tree/16.0/product_route_mto

Certainly NOT. However, this module should be in product-attribute repo and I would rename it product_variant_route_mto

@chaule97 chaule97 force-pushed the 18.0-mig-stock_product_variant_mto branch from bbc05c8 to 5e0cc07 Compare February 5, 2025 09:11
@chaule97 chaule97 requested a review from jbaudoux February 5, 2025 09:12
@chaule97
Copy link
Author

chaule97 commented Feb 5, 2025

Certainly NOT. However, this module should be in product-attribute repo and I would rename it product_variant_route_mto

Do you or I move it?

@jbaudoux
Copy link
Contributor

jbaudoux commented Feb 5, 2025

Close this PR and open a new one on product attribute

@chaule97
Copy link
Author

chaule97 commented Feb 6, 2025

Hello everyone, I move it to OCA/product-attribute#1879, please review it in new PR, thanks

@chaule97 chaule97 closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants