[FIX] mrp: fix negative values in subcontracting BoM Overview #144702
Conversation
|
|
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
3 times, most recently
from
ff2df2f
to
dd0bee0
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
3 times, most recently
from
9a7bc61
to
e5ce20f
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
from
e5ce20f
to
82c64c5
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
4 times, most recently
from
1645389
to
fcd2c87
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
from
fcd2c87
to
3e1996d
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
3 times, most recently
from
19bfe04
to
29b9edd
Compare
| @@ -116,7 +116,8 @@ def _get_resupply_availability(self, route_info, components): | |||
| vendor_lead_time = route_info['supplier'].delay | |||
| manufacture_lead_time = route_info['bom'].produce_delay | |||
| subcontract_delay = resupply_delay if resupply_delay else 0 | |||
| subcontract_delay += max(vendor_lead_time, manufacture_lead_time + max_component_delay) | |||
| subcontract_delay += max(vendor_lead_time, manufacture_lead_time) + max_component_delay | |||
There was a problem hiding this comment.
Changeing this is wrong to me because of the formula introduced here 78e44dc
There was a problem hiding this comment.
I'm afraid that the original formula might be wrong 😅 I think that, in this case, the max_component_delay value represents either the availability from stock or from resupply. So, the final value is a max of vendor lead time and manufacture lead time (i.e. what the vendor declares vs the actual time it takes to manufacture a product) and then we add the delay of components (e.g. 0 if they are available or resupply time if they are not). We cannot start the manufacturing process until we get the components, so the component delay was moved out of the max calculation.
But I agree that this is all very complicated, and the names of variables don't always reflect on the actual information they hold :( Perhaps we can discuss this further, or involve one of our MGM in this conversation?
| self.env['stock.quant']._update_available_quantity(self.comp1, self.warehouse.lot_stock_id, 100) | ||
| self.env['stock.quant']._update_available_quantity(self.comp2, self.warehouse.lot_stock_id, 100) | ||
| self.env['stock.quant']._update_available_quantity(self.comp1, self.env.company.subcontracting_location_id, 100) | ||
| self.env['stock.quant']._update_available_quantity(self.comp2, self.env.company.subcontracting_location_id, 100) |
There was a problem hiding this comment.
I prefer you duplicate the test and change the update quantity in yours
There was a problem hiding this comment.
I'm afraid that the test was failing without this change :( I think that the old code was not working correctly, i.e. it was checking the wrong location or not checking it at all ... I implemented this a while ago, I will have to review my notes. But do you think that, in this case, warehouse.lot_stock_id is the correct place for subcontracting components? Doesn't it make more sense to store them in subcontracting_location_id ?
addons/mrp_subcontracting_purchase/tests/test_mrp_subcontracting_purchase.py
Outdated
Show resolved
Hide resolved
| @@ -535,7 +537,7 @@ def test_subcontracting_lead_days_on_overview(self): | |||
| bom_data = self.env['report.mrp.report_bom_structure']._get_bom_data(self.bom, self.warehouse, self.finished) | |||
| self.assertEqual(bom_data['lead_time'], 15 + 5 + 5 + 0, | |||
| "Lead time = Purchase lead time(finished) + Days to Purchase + Purchase security lead time + DTPMO on BOM") | |||
| self.assertEqual(bom_data['resupply_avail_delay'], 10 + 5 + 5 + 20, | |||
| self.assertEqual(bom_data['resupply_avail_delay'], 15 + 5 + 5 + 20, | |||
There was a problem hiding this comment.
OK, I think I figured this out 🙂
Normally:
resupply avail delay =
resupply delay +
max(vendor lead time, manufacture lead time) +
max component delay
In Purchase, we also need to add to that "Purchase security lead days" and "Days to Purchase".
In this case:
- resupply delay = 0 (False in
_get_resupply_availabilityin MRP, because route type is not "manufacture") - vendor lead time = 15 (
delayof the final product) - manufacture lead time = 10 (
produce_delayof the BoM) - Purchase security lead days = 5 (
po_lead) - Days to Purchase = 5 (
days_to_purchase) - max component delay = 20
The only surprise for me is the last part, which I would expect to be 10 (we have 2 components, with delays 10 and 6). But then I realized that in Purchase, we also add po_lead and days_to_purchase to component delay 😉 So it's 10 + 5 + 5 = 20.
The final value is 0 + max(15, 10) + 5 + 5 + 20 = 15 + 5 + 5 + 20, which is what the test checks after my changes 😄 I will update the assertion below, so we don't have to investigate this again 😉
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
from
29b9edd
to
53db399
Compare
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
from
53db399
to
a180b94
Compare
A subcontracting BoM sometimes shows negative values in Overview, if components are available in subcontractor's location. Task: 3607854
pfertyk
force-pushed
the
17.0-subcontracting-negative-values-pafe
branch
from
a180b94
to
24a2dc2
Compare
|
robodoo r+ |
A subcontracting BoM sometimes shows negative values in Overview, if components are available in subcontractor's location. closes odoo#144702 Task: 3607854 Signed-off-by: William Henrotin (whe) <whe@odoo.com>
Task: 3607854
Description of the issue/feature this PR addresses:
Current behavior before PR:
Desired behavior after PR is merged:
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr