[17.0][MIG] procurement_mto_analytic

[aisopuro]

The most notable change to the functionality is the way procurements avoid merging to existing purchases and purchase lines. I recommend reviewers pay particular attention to those parts of the code when reviewing, to make sure my logic and explanations are clear to everyone.

Depends on

• [17.0]]MIG]sale_stock_analytic: Migration to 17.0 #617

[AungKoKoLin1997]

@aisopuro I noticed three points.

• I think this module should only depends on sale_stock and purchase_stock because these are enough for the functionality of this module.
• I tested the functional with two sale order lines with different analytic_distribution. It creates two purchase order for each order line. I think it shouldn't.

• The last one is I create new sale order with the same sale order lines of previous order. It also creates new two purchase orders instead of merging into existing POs. I think they should be merged, isn't?

I created PR for this module for 16.0 migration. Can you you please review it and we make the design consistent?
#637

[aisopuro]

@AungKoKoLin1997 thanks for your review.

I think this module should only depends on sale_stock and purchase_stock because these are enough for the functionality of this module.

The method sale.order.line:_purchase_service_prepare_line_values is defined in sale_purchase, so I think we need to depend on that one.

If I remove the *_analytic modules from the dependencies, that causes all the related tests to fail. Do I need to extract them into a separate test_procurement_mto_analytic module?

Would it make more sense to split this module into 2 different modules, one for the "purchase services on sales" case, and the other for the "procure purchases for sold physical products" case?

I tested the functional with two sale order lines with different analytic_distribution. It creates two purchase order for each order line. I think it shouldn't.

In the previous version of this module, the extension of _make_po_get_domain indicated to me that this module in the previous version wanted to make it so that if you have two different analytic accounts on procurements, those should not go on the same purchase.

Analytic distributions are different in that they are basically sets of accounts, but I tried to emulate the previous logic as well as I could. Do you think we should change this behavior now so that this module doesn't care if lines with different analytics go on the same purchase?

I'm not opposed to the idea. I'm not sure this module should have been doing that kind of thing before anyway. Ensuring that procurements with different analytic distributions don't get merged into the same line is enough. But I just want to make sure we're all aware of the change in logic this implies.

The last one is I create new sale order with the same sale order lines of previous order. It also creates new two purchase orders instead of merging into existing POs. I think they should be merged, isn't?

That does seem strange, I'll look into why that happens.

I created PR for this module for 16.0 migration. Can you you please review it and we make the design consistent?

@yostashiro has added some comments to that PR, so I'm a little unclear which design I should emulate. After his comments you replaced the _make_po_get_domain with some new methods, but I can't find those methods in 17. So that PR seemed to me to be in flux. Is it now stable enough to use here?

[AungKoKoLin1997]

@aisopuro

The method sale.order.line:_purchase_service_prepare_line_values is defined in sale_purchase, so I think we need to depend on that one.

sale_purchase is auto_install=True. So, I think we don't need to depend on this module.

If I remove the *_analytic modules from the dependencies, that causes all the related tests to fail. Do I need to extract them into a separate test_procurement_mto_analytic module?

I think this is because of you removed extending _prepare_procurement_values of stock_move in this module for passing analyic_distribution values to procurements.

Would it make more sense to split this module into 2 different modules, one for the "purchase services on sales" case, and the other for the "procure purchases for sold physical products" case?

I failed to understand why we need to do it.

Analytic distributions are different in that they are basically sets of accounts, but I tried to emulate the previous logic as well as I could. Do you think we should change this behavior now so that this module doesn't care if lines with different analytics go on the same purchase?

I was thinking to add an option for choosing this behavior in company record(separate PO for different analytic_distribution order line or use same PO). What do you think?

you replaced the _make_po_get_domain with some new methods, but I can't find those methods in 17. So that PR seemed to me to be in flux. Is it now stable enough to use here?

I created refactoring PR in Odoo repo for this odoo/odoo#161916. This will be forward ported when it gets merged. This refactoring is needed for holding multiple analytic distributions in one purchase order.

[aisopuro]

Regarding your comments @AungKoKoLin1997 :

sale_purchase is auto_install=True. So, I think we don't need to depend on this module.

I disagree that auto_install=True makes it so we don't need to depend on the module. We extend one of its methods, and call super in it: thus our module will not work correctly if that other module is not installed, or if it does, it is by luck. I find it confusing to read module code that expects dependencies to be installed but does not list them in its manifest. I still think we should keep sale_purchase in the dependencies in order to be explicit about what the module expects.

I think this is because of you removed extending _prepare_procurement_values of stock_move in this module for passing analyic_distribution values to procurements.

Right, which I did since it seemed redundant to implement the same logic twice: once in this module and another time in stock_analytic (if I'm reading this right, yostashiro also had similar thoughts).

It seems a little strange to me that we would have a module in this repo whose explicit job it is to handle the propagation of analytics in stock procurements, and then not reuse that in this module.

This might indicate that this module currently does too many things. In this repo we have separate, small modules for every other part of standard procurement when it comes to propagating analytics:

• From sale to stock (sale_stock_analytic)
• From stock to stock (stock_analytic)
• From MRP to stock (mrp_stock_analytic) (mrp_account in core already handles the other way)

This module is the only one that bakes multiple different parts into a single module (sale to purchase, whether services or stockables).

I didn't want to do it in the migration, but now with these concerns being raised, should we consider eliminating this module and replacing it with a simpler version that only looks at whether there is analytic distribution information in the procurement? Then if people want the analytics from sales to go to purchase, they just need to install sale_stock_analytic, stock_analytic and this new purchase_procurement_analytic. It would make the whole logic much more modular and, in my opinion, more logical as well.

I failed to understand why we need to do it.

I meant this as a way to allow us to have the "purchase services directly"-logic without also needing to bring in stock-related dependencies. Even if we simplify the module, in core Odoo there is a distinction between purchasing services directly from sales and procuring the purchase of physical goods. These are different, independent features in core Odoo: you can use the first without using the second.

It seems a shame to me that in order to get the analytics propagated in the first case, you would need to install stock modules even if you don't need them (this module would at least need to depend on purchase_stock in order to extend the necessary purchase grouping logic, so I don't see how that can be avoided).

So basically since the two parts are split into two different modules in core Odoo, it seems sensible to me that we would do the same here.

I was thinking to add an option for choosing this behavior in company record(separate PO for different analytic_distribution order line or use same PO). What do you think?

That could work. I'll see if I can find a proper place for it in the settings. The other option would be to have a setting on the procurement rule itself, but I think that becomes cumbersome to manage for the user, so your idea makes more sense.

I created refactoring PR in Odoo repo for this odoo/odoo#161916. This will be forward ported when it gets merged. This refactoring is needed for holding multiple analytic distributions in one purchase order.

Did you conclude it isn't sensible to try and detect whether a PO already contains a line with the same analytic distribution in the current setup? Your PR originally had one attempt to use a raw SQL query, and then yostashiro suggested instead doing it with a filtered call. What was the reason a change in core Odoo was needed?

I'm mainly asking because I need this feature for a customer project, and I can't wait for Odoo to merge the change and forward port it. I might be able to think of a workaround for my project in the meantime though.

For this PR, I can make it wait for that core PR as well.

[aisopuro]

@AungKoKoLin1997 regarding the part of your first comment

I added a test that repeats your steps, but the test passes. I also can't replicate the issue through the UI.

What I did:

• I logged into the test database I had used to run the unit tests
• I activated route management in the settings
• I unarchived the MTO route
• I added the "Analytic accounting" group to the admin user
• I selected two products that were stockable and set the MTO and Buy routes on them
• I made a sale with one of each on separate lines, with different analytic distributions on each
• On confirmation, this made 2 different purchases
• I made another sale (without duplicating) with the same products and analytic distributions
• On confirmation, the quantities on the previously generated purchase orders went up from 1 to 2

Did you do anything in addition to what you described? Do you have any other modules installed besides procurement_mto_analytic? Can you excel-export the analytic distributions of those sale order lines and purchase lines?

There might be some interaction the unit test isn't catching.

[AungKoKoLin1997]

@aisopuro Sorry for late reply.

I disagree that auto_install=True makes it so we don't need to depend on the module. We extend one of its methods, and call super in it: thus our module will not work correctly if that other module is not installed, or if it does, it is by luck.

I disagree that. How is it worked by luck? sale_purchase will be auto installed if both sale and purchase are installed. This module can't be installed if these two are not installed. But I understand the point of having that module in the dependencies to avoid the confusion to read the module. So, I agree on that point.

Right, which I did since it seemed redundant to implement the same logic twice: once in this module and another time in stock_analytic (if I'm reading this right, yostashiro also had similar thoughts).

I still don't think we should depend on stock_analytic because we should avoid dependencies on modules that are not directly related to the current module. Otherwise, this can lead to unnecessary work and make the module more complex. In our case, we can just simply pass analytic_distribution of sale order line inside def _prepare_procurement_values(self). I think this module will pass analytic_distribution of sale order line and stock_analytic will pass analytic_distribution of stock move. It may be redundant if users use both sale_stock_analytic and stock_analytic with this module. But I don't think this is the case. Then, as you know, the older version of this module just depends on sale_stock and purchase_stock.

Did you conclude it isn't sensible to try and detect whether a PO already contains a line with the same analytic distribution in the current setup?

No. In the first approach, I overthink and used raw sql query instead of filtered. Actually, we can just use filtered.
The changes in Odoo core is not related with this at all.

I'm mainly asking because I need this feature for a customer project, and I can't wait for Odoo to merge the change and forward port it. I might be able to think of a workaround for my project in the meantime though.

For this PR, I can make it wait for that core PR as well.

I understand your situation. I am trying to make the design more configuration and optimal for the users. What I am not comfortable with current design is we can't keep different analytic_distribution lines in same PO even if they have same vendor and can keep in same sale order.

I added a test that repeats your steps, but the test passes. I also can't replicate the issue through the UI.

I will double check again. Thanks for the investigation.

[aisopuro]

@AungKoKoLin1997 continuing the discussion:

I still don't think we should depend on stock_analytic because we should avoid dependencies on modules that are not directly related to the current module.

But if this module in its readme says "Propagate the analytic information from the sale to the purchase", how can it not be directly related to a module that does exactly that?

If the issue is that this module might not need that dependency in certain circumstances, I agree. But in that case I believe that Odoo's base logic has changed so much that this module should be split in two:

• One that only handles "picking up" the analytics from the stock procurement if they are propagated.
• One that makes it so if you purchase services directly from sales, the analytics are propagated from sale to purchase.

That way our purchase-related modules are much more similar to the other modules in this repo, and become a much more logical part of the whole in my opinion.

The customer project that I mentioned is now shipped, and they use a separate, frozen branch with this code, so this can be updated as is best seen. However, that also means I will move on to other projects, and I suspect I will not have a lot of time to give to this. So if someone else comes along and needs/wants to continue where this discussion left off, feel free to open a replacing PR or link to patches.