-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
[14.0][IMP/ADD] stock_product_variant_mto, sale_stock_mto_as_mts_orderpoint_product_variant: Handle mto route at variant level #1461
base: 14.0
Are you sure you want to change the base?
Conversation
e394efc
to
4cccd24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the issue at hand couldn't be solved by redefining route_ids
as a computed field on product.product
and then select the MTO route in the compute function only if the is_mto
field is marked?
Maybe we should discuss this together.
5badb3f
to
e694809
Compare
@api.onchange("route_ids") | ||
def onchange_route_ids(self): | ||
mto_route = self.env.ref("stock.route_warehouse0_mto", raise_if_not_found=False) | ||
if mto_route not in self._origin.route_ids and mto_route in self.route_ids._origin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can make the condition more selective and raise the warning only if we have multiple variants, and these variants have different MTO settings.
"message": _("Activating MTO route will reset `Variant is MTO` setting on the variants.") | ||
} | ||
} | ||
if mto_route in self._origin.route_ids and mto_route not in self.route_ids._origin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
templates_mto_after = self.filtered(lambda t: mto_route in t.route_ids) | ||
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_mto_removed).product_variant_ids._compute_is_mto() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return res
missing
f4bc814
to
6d7b34d
Compare
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
@mmequignon to be reopened? |
yes please |
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.
6d7b34d
to
d634638
Compare
self.ensure_one() | ||
return self.is_mto | ||
|
||
def _inverse_is_mto(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this defined on the field? I don't think this is called.
Check you have a unit test for this (archiving orderpoint when variant is not MTO anymore)
I'll rollback @grindtildeath changes as it doesn't work. |
d634638
to
bc17928
Compare
For reviewers, I know a test is failing, this is because in |
bc17928
to
4ed01c0
Compare
There were multiple issues: - Inverse function on storable field is not reliable and we should prefer overriding write to add any processing when the field value is changed. Moreover, the field is_mto on product.product did not have its inverse function set on the field, so the function was not called. - When mto_as_mts is changed on the warehouse, we do not want to archive MTO rule for this warehouse (because we could still want to do MTO but not "as MTS").
4ed01c0
to
3d81b69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@@ -0,0 +1,19 @@ | |||
# Copyright 2024 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent years
# Copyright 2024 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
@@ -0,0 +1,39 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
return res | ||
|
||
def _get_orderpoints_to_archive_domain(self, warehouse): | ||
# Orderpoints to archive are those where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete comment?
@@ -0,0 +1,155 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
@@ -0,0 +1,18 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
|
||
IS_MTO_HELP = """ | ||
Check or Uncheck this field to enable the Make To Order on the variant, | ||
independantly from its template configuration.\n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
independantly from its template configuration.\n | |
independently from its template configuration.\n |
@@ -0,0 +1,44 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
@@ -0,0 +1,80 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
@@ -0,0 +1,108 @@ | |||
# Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Copyright 2023 Camptocamp SA | |
# Copyright 2025 Camptocamp SA |
@@ -0,0 +1,32 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- Copyright 2023 Camptocamp SA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Copyright 2023 Camptocamp SA | |
<!-- Copyright 2025 Camptocamp SA |
help=IS_MTO_HELP, | ||
) | ||
|
||
route_ids = fields.Many2many( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If find having this field on product.product
for handling the feature of this module worrisome and could be conflicting with other modules or implementation...
I have to move commits in dedicated PRs, but pushed everything here for the time being