Skip to content
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

[15.0][FIX] stock_request: Remove route_ids fields from requests #2184

Closed

Conversation

victoralmau
Copy link
Member

FWP from 14.0: #2179

Remove route_ids field from requests

It is not correct to set the route_ids field with the product routes, you must be able to select any route (similar to what is shown in the product form view).

Please @pedrobaeza and @carlos-lopez-tecnativa can you review it?

@Tecnativa TT50610

@pedrobaeza pedrobaeza added this to the 15.0 milestone Nov 5, 2024
It is not correct to set the route_ids field with the product routes, you must be able
to select any route (similar to what is shown in the product form view).

TT50610
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

just marking that I want to review this, please don't merge

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

As said in the v14 PR I do not agree with this change. I don't think this is correct. I have processes using routes in stock request that are not product selectable...

@pedrobaeza
Copy link
Member

@LoisRForgeFlow AFAIK it was more restricting before: only those routes that were marked in the product were available to select.

We consider this as a fix because legit operations that you can do in a stock request were not allowed. Example: you can have several stock routes defined for going one way or another, and you only check in the product the default one, and you can't mark all of them because of conflicts, but on a stock request, you may want to force such unchecked route to be used for that request.

@LoisRForgeFlow
Copy link
Contributor

@pedrobaeza you are switching to a different restriction, now I cannot select warehouse routes for instance.

If you have sereral routes applicable to a product and want one by default and another to be available in stock request, you mark both in the product and play with the sequence of the routes/rules to set the default one.

@pedrobaeza
Copy link
Member

If you have several routes applicable to a product and want one by default and another to be available in stock request, you mark both in the product and play with the sequence of the routes/rules to set the default one.

I'm afraid this didn't work in some cases. @victoralmau do you remember the case?

now I cannot select warehouse routes for instance.

Not sure if you was able to do that before. Can you confirm, @victoralmau? But if that's the problem, we can expand the selection, or even don't apply any restriction at all.

@LoisRForgeFlow
Copy link
Contributor

If you have several routes applicable to a product and want one by default and another to be available in stock request, you mark both in the product and play with the sequence of the routes/rules to set the default one.

I'm afraid this didn't work in some cases. @victoralmau do you remember the case?

now I cannot select warehouse routes for instance.

Not sure if you was able to do that before. Can you confirm, @victoralmau? But if that's the problem, we can expand the selection, or even don't apply any restriction at all.

I value the restriction to avoid user mistakes, I want them to choose only valid routes.

Yes, It can be that some case is not considered or even small bugs can be there, but deleting the restriction because of that sounds a bit drastic for me.

Let's wait @victoralmau technical feedback, I'm sure we can find a common ground

@victoralmau
Copy link
Member Author

An example of a use case.

We have set a product with MTO + Buy routes.
When creating a stock request we can only select Buy, but what happens if I want to Manufacture? I can't do it. I must select the Manufacture route on the product (I don't want to do it) to be able to do it.

Just as it happens when clicking on Replenish button on a product and/or the Replenishment menu, I must be able to select the same routes as there, i.e. it should not be restricted.

@LoisRForgeFlow
Copy link
Contributor

An example of a use case.

We have set a product with MTO + Buy routes. When creating a stock request we can only select Buy, but what happens if I want to Manufacture? I can't do it. I must select the Manufacture route on the product (I don't want to do it) to be able to do it.

Just as it happens when clicking on Replenish button on a product and/or the Replenishment menu, I must be able to select the same routes as there, i.e. it should not be restricted.

But does it generate any problem to have manufacture route selected for that product?

How I conceive routes is that you select where they are valid routes, if a product can be manufactured, it should have the manufacture route available (either via direct product selection, product category or warehouse). Once that is configured, you can configure which is the default route, by setting a lower sequence value in the purchase route. Is that not possible in your case? I mean: could you expand "I don't want to do it"?

Regarding replenishment menu, you can imagine that I'm not a big fan of it being myself a DDMRP advocate 😝 . But I have had a look and indeed that domain to allow you to select any product selectable route, but the fact that Odoo does something doesn't mean it is correct. As said, I value a lot being able to restrict the routes that are really applicable, having everything available could lead to mistakes in instances with a lot of routes configured. At least this is my PoV.

Now, if you don't like my approach or you can't apply it becouse you have conceived your routes configuration a bit differently, let's find intermediate approaches, ideas:

  1. You can customize the route_ids calculation. If needed more extensibility, new hooks can be added.
  2. We could add a stock request selectable checkbox in the routes, and add those routes on top of the current route_ids computation.

Thoughts?

@pedrobaeza
Copy link
Member

@LoisRForgeFlow we can't select both "Buy" and "Manufacture" in the products, as the priority can only be put once (or by default "Buy", or by default, "Manufacture"), but there are some products where by default we want to buy, and others that by default we want to manufacture, but for stock requests, we may want to use the other one. Checking both marks in all the products will provoke a bad default behavior.

This is a real need, where "Buy" means "Subcontracting", and sometimes the company decides to subcontract something if they don't have capacity, but by default they are manufactured, or the reverse, and as said, each product has its own default.

@LoisRForgeFlow
Copy link
Contributor

LoisRForgeFlow commented Nov 5, 2024

@pedrobaeza You can mark both routes available in the category or warehouse and then mark the preferred in the product (product routes have preference over category and warehouse routes). This way you signal the default by product and do not need any change in stock_request.

If you are still not happy with that, what are your thoughts on my previous proposals?

Now, if you don't like my approach or you can't apply it because you have conceived your routes configuration a bit differently, let's find intermediate approaches, ideas:

  1. You can customize the route_ids calculation. If needed more extensibility, new hooks can be added.
  2. We could add a stock request selectable checkbox in the routes, and add those routes on top of the current route_ids computation.

@pedrobaeza
Copy link
Member

I don't feel comfortable having own custom rules only for stock_request module, while Odoo is using other rules for managing the stock routes. Why not using the standard the same as in "Replenishment"?

@LoisRForgeFlow
Copy link
Contributor

As said, I value a lot being able to restrict the routes that are really applicable, having everything available could lead to mistakes in instances with a lot of routes configured. At least this is my PoV.

As said, I value a lot being able to restrict the routes that are really applicable, having everything available could lead to mistakes in instances with a lot of routes configured. At least this is my PoV.

@pedrobaeza
Copy link
Member

But you are restricting in an artificial way that Odoo is handling differently. For example, by default "Buy" is only configurable at product level:

imagen

Having to mark it at warehouse or product category level seems unreasonable for a good new users onboarding. But not only that, even configuring to be shown at that levels, and marking the checks, may lead to incorrect routes when Odoo has to decide which route to select in certain cases. Please check the code that is doing this:

https://github.com/odoo/odoo/blob/3a28e5b0adbb36bdb1155a6854cdfbe4e7f9b187/addons/stock/models/stock_rule.py#L501

@rousseldenis
Copy link
Contributor

2. We could add a stock request selectable checkbox in the routes, and add those routes on top of the current route_ids computation.

This is the more reasonable option I see and follows the Odoo way of route availability management.

@LoisRForgeFlow
Copy link
Contributor

@pedrobaeza yep, of course a different routes estructure lead to a different training and to a different way of setting up products. Just wanted to give you an extra idea as I find very useful to assign routes by product category.

So, if this is also not good for you, we are left with the last option as @rousseldenis points out. Personally, I don't love it but I don't see much more options, maybe you can come with another intermediate solution.

@rousseldenis
Copy link
Contributor

as I find very useful to assign routes by product category

👍

@pedrobaeza
Copy link
Member

OK for the check at stock.route level, but being the only source, not on top of the other computation. We can provide migration scripts for marking that check according the old rules.

@LoisRForgeFlow
Copy link
Contributor

LoisRForgeFlow commented Nov 5, 2024

OK for the check at stock.route level, but being the only source, not on top of the other computation. We can provide migration scripts for marking that check according the old rules.

Being the only source will not work for me, as I said I want the options to be dynamic by product based on the configuration and to only allow valid routes. The checkbox will allow to set "global" stock request routes, so I could not keep my current flows if the checkbox is the only source.

If you want to make it the only source, I give you one more option, make it in an extra module that I can keep uninstalled.

@rousseldenis
Copy link
Contributor

@pedrobaeza
Copy link
Member

Well, I think it should be the reverse: that constraints you are mentioning should go on an extra module. If you think twice, they are not generalist, but specific to your projects, and they are not newcomers friendly. We can do such split if you agree.

If not, as this is not our module, we won't complain more, will close this, and will remove the restriction in our custom.

@LoisRForgeFlow
Copy link
Contributor

Well, I think it should be the reverse: that constraints you are mentioning should go on an extra module. If you think twice, they are not generalist, but specific to your projects, and they are not newcomers friendly. We can do such split if you agree.

If not, as this is not our module, we won't complain more, will close this, and will remove the restriction in our custom.

It is the approach I follow here and in ddmrp and MRP multi level, so it is aligned with our approach in many solutions proposed to OCA. As said, just because Odoo does something is doesn't mean is the best or only way to manage something. Anyway, I can accept that it is opinionated (as many module implementations in the OCA), so I guess in this case I will play the author/maintainer card and keep the logic as is.

Thanks for understanding!

@pedrobaeza
Copy link
Member

OK, I will use this case when I need to play such card in my modules 😉

@pedrobaeza pedrobaeza closed this Nov 5, 2024
@pedrobaeza pedrobaeza deleted the 15.0-fix-stock_request-TT50610 branch November 5, 2024 15:04
@victoralmau
Copy link
Member Author

I am really surprised that this (already merged in v14) is discarded, defend that the current behavior (deformed according to standard) is as expected I think it is not the right thing to do (this reminds me of #1953). It makes me wonder if new fixes will be accepted and/or will be “expected behavior”.

@rousseldenis
Copy link
Contributor

I am really surprised that this (already merged in v14) is discarded, defend that the current behavior (deformed according to standard) is as expected I think it is not the right thing to do (this reminds me of #1953). It makes me wonder if new fixes will be accepted and/or will be “expected behavior”.

@victoralmau Original authors were not informed of your changes there: #2179 (approved internally...)

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

Successfully merging this pull request may close these issues.

4 participants