[FIX] stock_dynamic_routing: Prevent several splits when first source…

[xAdrianCif]

In test cycles, the following issue was detected:

Starting from two locations, both under the WH/STOCK hierarchy:

• WH/STOCK/SHELVING/SHELF1 (normal stock)
• WH/STOCK/HIGHBAY/HIGHBAY1 (height stock used in dynamic routing)

picking_type_dynamic_routing:

• name: Dynamic Routing
• code: internal
• default_location_src_id: WH/STOCK/HIGHBAY
• default_location_dest_id: WH/STOCK/HANDOVER

dynamic routing:

• location_id: WH/STOCK/HIGHBAY
• picking_type_id: picking
  • ubicación de origen: WH/STOCK
  • ubicación de destino: WH/STOCK/OUTPUT
• rule
  • method: pull
  • picking_type_id: picking_type_dynamic_routing

When we have a delivery note susceptible to be supplied by dynamic routing,
if we adjust the stock first in the stock location (SHELF1) and then in the height location (HIGHBAY1),
and the quantities in SHELF1 are less than half of the demand, the system generates more splits than necessary
and makes incorrect allocations in the PICK delivery note.

This is explained with an example, implemented in the test test_partial_availability_stock_and_highbay, which
fails if this fix is not implemented:

• We create two delivery pickings (two-step delivery, PICK and OUT) of product1 with 30 units
• We input 10 units in SHELF1 and 20 units in HIGHBAY1, in that order
• We check availability
• The expected result in the reservations is:
  • Supply:
    • 20 units from HIGHBAY1 to HANDOVER -> assigned
  • PICK:
    • 10 units from SHELF1 to OUTPUT -> assigned
    • 20 units from HANDOVER to OUTPUT -> Waiting
  • OUT:
    • 30 units from OUTPUT to CUSTOMER -> Waiting
• Instead, without the proposed fix, the result in the reservations is:
  • Supply:
    • 10 units from SHELF1 to HANDOVER -> assigned
    • 10 units from HIGHBAY1 to HANDOVER -> assigned
  • PICK:
    • 10 units from HIGHBAY1 to OUTPUT -> assigned
    • 10 units from HANDOVER to OUTPUT -> Waiting
    • 10 units from HANDOVER to OUTPUT -> Waiting
  • OUT:
    • 30 units from OUTPUT to CUSTOMER -> Waiting

This is because at the moment of checking the move of the 30 units from picking,
a split of 20 units is made, leaving the 10 units to which the units from SHELF1 should be assigned,
but the assignment is not made, first the confirmation of the 20 split units is made,
which causes that out of those 20 units, 10 are taken from shelf1 and 10 from highbay,
this causes that the picking movement, when it has to make its reservation, does not have
available quantities and has to use those from highbay.

To prevent this, the following is introduced:

• By using a SAVEPOINT and a ROLLBACK, after making the split, we check if any
  of the made reservations has taken stock from a location included in the routing rules
  and if so, we rollback. We only leave the assigned movement in case the location
  of the reservation is not included in the assigned routing rules.
• We use savepoints because deleting the reservation later could cause
  undesired splits that we wouldn't be able to undo. In addition to preventing
  it from affecting subsequent movements.
• The mechanics of savepoint, rollback, and release have been abstracted into a method
  for efficiency and maintainability, as it is used twice.

As mentioned above, a new test case for the detected scenario is introduced,
ensuring that all flows continue to function as expected so far and that existing
tests continue to run as programmed.

[aitoreizmendi]

Looks good to me. Technical review ok. Nice job.

[alejandro-vallejoFL]

Technical review ok, nice work!

[xAdrianCif]

Moved to draft due to infinite loop found in some cases, working on it.

Edit: error due to misconfiguration, functionally validated.

[serCliff]

When I tested this case on runboat the result was:

Preparation (stocks, dynamic routing config and sale order)

• Stock image
• Dynamic routing configuration image
• Sale order: image

Obtained result

• Picking have many unnecesary splits: image
• Highbay to Handover have:
  • Product splitted as we sawn image and it have a problem with the reservation
  • The detailed operations haven`t any reservation image
  • All the reservations are in the picking transfer image

Corrections needed

• Avoid making unnecessary splits (we only need to have 2 splits: 2 units for the picking and 7 for the procurement transfer).
• Properly assign the stock.move.line to its transfer.

---

Now I will repeat a similar test on a server that has the fix proposed in its pull request.

Preparation

• The stock of the FLOWWATER-S are: 2 in picking y 9 in ZAbastecimiento
• The dynamic routing config is: image
• The sale order: image

The expected result

The expected result when confirming the sales order is:

• A split in the picking transfer, with 2 units being picked from 'Picking' and 7 from 'ZAbastecimiento'.
• Another transfer should be made from 'ZAbastecimiento' to fulfill the 7 units needed.

Obtained result

Transfers made

We have 2 units from the picking process, and the other 7 units were obtained from a procurement transfer.

The procurement transfer has properly reserved its items

---

Summarizing, based on the provided tests, we can see how this pull request resolves the issue of splits and reservations made by the module when the available units in picking are less than half of what needs to be requested in Highbay.

👌

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[xAdrianCif]

Hi @rousseldenis @jbaudoux and @lmignon

I've noticed that you have participated in other PRs for this module, since there is no official maintainer for it, would you mind reviewing to see if we can merge?

Thank you very much in advance.

[jbaudoux]

Thanks for the detailed explanation.
Is it a use case that you really face and cause issues?
In our case, we are using stock_reserve_rule module where you can declare that you take from highbay only if a certain condition is met, otherwise you take from shelf. So I can imagine we never faced the issue you raise.
I need to carefully investigate that use case to ensure we don't break other use cases.
In the solution, we need to pay attention to performance as we could have to process thousands of locations.

[xAdrianCif]

Hello @jbaudoux, thank you for response!

We arrived at this use case due to flow testing with real configurations adapted to current use cases. In this context, we detected that it might be fixable with configuration, or as you suggest, with other modules. However, we believed that, since this situation can occur, the correct action is fix it. (maybe whe should declare useful modules in readme)

Regarding the different use cases, we have conducted many tests and have not detected problems in other flows, besides verifying that all tests are correctly fulfilled. However, it is true that we might be missing some flows.

Lastly, regarding code efficiency, the rollback only executes in cases where the split and reservations are incorrect. Therefore, when weighing an unexpected result against low efficiency, we believed that the latter was always the better choice.

I'm at your disposal if I can answer any questions or help with anything, thank you very much, cheers!

[serCliff]

I'm agree with @xAdrianCif

In spite of we can avoid the bug using another modules or configurations we should provide the functionality as stronger as we can. More when we saw what the problems wich occurs bring a state in the pickings very difficult to recover.