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

[WMS][13.0] Add stock_dynamic_routing - alpha version #788

Closed
wants to merge 31 commits into from

Conversation

guewen
Copy link
Member

@guewen guewen commented Dec 13, 2019

Introduction

Port of #639

Detailed commit history for the past developments
can be found in https://github.com/guewen/stock-logistics-warehouse/tree/12.0-add-stock_picking_zone

Additional changes in data model and features to expect, so the PR is a draft now.

Description

Route explains the steps you want to produce whereas the “picking routing
operation” defines how operations are grouped according to their final source
and destination location.

This allows for example:

  • To parallelize picking operations in two locations of a warehouse, splitting
    them in two different picking type
  • To define pre-picking (wave) in some sub-locations, then roundtrip picking of
    the sub-location waves

Context for the use cases:

In the warehouse, you have a High-Bay which requires to place goods in a
handover when you move goods in or out of it. The High-Bay contains many
sub-locations.

A product can be stored either in the High-Bay, either in the Shelving zone.

When picking:

When there is enough stock in the Shelving, you expect the moves to have the
usual Pick(Highbay)-Pack-Ship steps. If the good is picked from the High-Bay, you will
need an extra operation: Pick(Highbay)-Handover-Pack-Ship.

This is what this feature is doing: on the High-Bay location, you define
a "routing operation". A routing operation is based on a picking type.
The extra operation will have the selected picking type, and the new move
will have the source destination of the picking type.

When putting away:

A put-away rule targets the High-Bay location.
An operation Input-Highbay is created. You expect Input-Handover-Highbay.

You can configure a routing operation for the put-away on the High-Bay Location.
The picking type of the new Handover move will the routing operation selected,
and its destination will be the destination of the picking type.

This is the implementation of "Warehouse operations by zones" RFC: #640 original document is the WMS document:
https://docs.google.com/document/d/1mct6bFFWJqW01wGFcjc-uQNEjyCxvh6Y9TuFdRhe-b0/edit#heading=h.49w4ly4e5y8g

Try on runbot

  • In Inventory Settings, activate:

    • Storage Locations
    • Multi-Warehouses
    • Multi-Step Routes

The initial setup in the demo data contains locations:

  • WH/Stock/Highbay
  • WH/Stock/Highbay/Bin 1
  • WH/Stock/Highbay/Bin 2
  • WH/Stock/Handover

The "Highbay" location (and children) is configured to:

  • create a source routing operation from Highbay to Handover when
    goods are taken from Highbay (using a new picking type Highbay → Handover)
  • create a destination routing operation from Handover to Highbay when
    goods are put to Highbay (using a new picking type Handover → Highbay)

Steps to try the Source Routing Operation:

  • In the main Warehouse, configure outgoing shipments to "Send goods in output and then deliver (2 steps)"
  • Inventory a product, for instance "[FURN_8999] Three-Seat Sofa", add 50 items in "WH/Stock/Highbay/Bay A/Bin 1", and nowhere else
  • Create a sales order with 5 "[FURN_8999] Three-Seat Sofa", confirm
  • You'll have 3 transfers; a new one has been created dynamically for Highbay -> Handover.

Steps to try the Destination Routing Operation:

  • In the "WH/Stock" location, create a Put-Away Strategy with:

    • "[DESK0004] Customizable Desk (Aluminium, Black)" to location "WH/Stock/Highbay/Bay A/Bin 1"
    • "[E-COM06] Corner Desk Right Sit" to location "WH/Stock/Shelf 1"
  • Create a new purchase order of:

    • 5 "[DESK0004] Customizable Desk (Aluminium, Black)"
    • 5 "[E-COM06] Corner Desk Right Sit"
  • Confirm the purchase

  • You'll have 2 transfers:

    • one to move DESK0004 from Supplier → Handover and E-COM06 from Supplier → Shelf 1
    • one waiting on the other to move DESK0004 from Handover → WH/Stock/Highbay/Bay A/Bin 1 (the final location of the put-away)

@guewen
Copy link
Member Author

guewen commented Dec 13, 2019

Check this: #639 (comment)

@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from 087814d to 8cd53aa Compare December 13, 2019 14:46
@max3903 max3903 added this to the 13.0 milestone Dec 31, 2019
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from 8cd53aa to 8ab3d06 Compare January 7, 2020 06:25
Copy link
Member

@jgrandguillaume jgrandguillaume left a comment

Choose a reason for hiding this comment

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

Functional test ok. I suggest to merge it as Alpha. We have a roadmap for the next steps and wants to extract this routing things in another separate model.

guewen and others added 6 commits April 6, 2020 11:22
* if the move's dest location is a child of the routing's dest location:
  it's more precise so change only the picking type
* if the move's dest location is a parent of the routing's dest
  location: change the dest location to the routing's dest location and
  change the picking type
* if the move's dest location is outside of the routing's dest location:
  add a routing operation before

It means, when there is a routing, even if the location was already
correct, the picking type is changed so users handle transfers the
same way.

The same changes will be done on the destination routing
We can probably optimize it by appling the domain only once
for all the moves of a routing.
The same thing should be applied on destination routing.
when we have to apply a routing instead of using unreserve: any
side-effect will be cancelled before doing the routing and calling
assign again, thus we avoid leaving things behind.
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from a4618a8 to 6bd18f4 Compare April 6, 2020 09:31
guewen added 6 commits April 6, 2020 14:00
The relation from the move doesn't always exist, if we delete the
package level before the unreserve, they are properly deleted.
Rules can be ordered and excluded by domains.
Store the rule chosen for a move to avoid doing it twice.
A split must occur to handle the routing only on the available quantity.
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from 745b1f5 to db5518a Compare April 8, 2020 14:57
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from 67c425a to de2417f Compare April 23, 2020 05:59
guewen added 4 commits April 24, 2020 09:30
When a move source location changes or a new move is inserted in the
chain, reevaluate the routing rules so we can chain several rules.
The _split method expects product_qty, not product_uom_qty.
So get the missing reserved quantity and convert it in the unit of
product_qty (as done in StockMove._action_assign())
In some conditions, new_moves can contain the same moves as self, so
_action_assign() would be called twice on the same move and would have
duplicate move lines.
No longer needed after the last refactoring
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from f38c5ad to 6740b48 Compare April 24, 2020 07:36
Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work, the module is really flexible and seems quite clean/well done to me!
I like the flexibility and it meets my use case now without need of adding several hooks and so on.

I see you are still working on it, so I keep a complete review for later, but still, there is one thing I have a hard time to understand, why do you have to call an _action_assign (and then roll back it) during the process?
What do we really gain in comparison to not doing it and just call the whole routing process after the _action_assign super, instead of before? (like it was in the previous version)

I think it adds a lot of complexity, and in some case may lead to unwanted/not needed action.
I mean, we could, after a change in the configuration for example, want to call the _split_and_apply_routing method on already assigned moves. Then we do not need to reassign it (and rollback it!)
Also, I wonder if it could be a problem to only call the super and not the whole method. It could probably lead to unwanted behaviors depending on action_assign override by depending modules.

stock_routing_operation/models/stock_move.py Outdated Show resolved Hide resolved
stock_routing_operation/models/stock_move.py Outdated Show resolved Hide resolved
@guewen
Copy link
Member Author

guewen commented Apr 28, 2020

todo: rename to stock_dynamic_routing and move to OCA/wms

@guewen
Copy link
Member Author

guewen commented Apr 29, 2020

I see you are still working on it, so I keep a complete review for later, but still, there is one thing I have a hard time to understand, why do you have to call an _action_assign (and then roll back it) during the process?

We have to "peek" at what will be reserved to know the available quantities in which source location, so we can split the move in the parts that will be reserved and the parts that cannot be reserved and apply different routing rules. The part of the move that will be reserved will have its routing applied, the other part will not change. We cannot do it after the assign is done, because applying the routing may change the source of the moves, change the picking of the move, ...

What do we really gain in comparison to not doing it and just call the whole routing process after the _action_assign super, instead of before? (like it was in the previous version)

It wasn't. The previous version had to call action_assign and unreserve before applying the routing.

I think it adds a lot of complexity, and in some case may lead to unwanted/not needed action.
I mean, we could, after a change in the configuration for example, want to call the _split_and_apply_routing method on already assigned moves. Then we do not need to reassign it (and rollback it!)

It's much more simple and clean to release a savepoint that the previous version which was calling super on _action_assign and then calling unreserve -> we are guaranteed that anything done for the first "action_assign" is rollbacked, while in the previous version we could have leftovers, or any action done by another module could be kept.

If you think it adds complexity, I think you missed how complex it was before ;)
As for calling _split_and_apply_routing on an assigned move, well, you can't.

Also, I wonder if it could be a problem to only call the super and not the whole method. It could probably lead to unwanted behaviors depending on action_assign override by depending modules.

As for any module overriding a method, the order of the calls to the method could lead to issues sometimes, but I don't see more risk here than any other override.
We call super in every execution path, and the first call to action_assign is either kept if no routing, or rollbacked entirely if a routing has to be called (which means anything done by another module is rollbacked and will be done again when assign is called on the routed moves).

The annoying point is that we have to execute _action_assign twice (only when a routing is applied), which is not great for performance, but I reckon there is no other way.

guewen added 6 commits April 29, 2020 08:15
* Prevent calling _action_assign() twice in case the savepoint was
  released
* Do not apply routing rules on the move we just routed as the routing
  rule to apply will be the same and already done
* Add comments explaining why they are called
In case of a put-away, when using a push routing rule, we want
to keep the expected destination.

For instance, we have a move:

* Input -> Highbay (draft)

It's assigned and the put-away rules compute a move line going to
Highbay/X1Y2.

When a routing is applied, before this commit, it would add a move at
the end, it would look like:

* Input -> Handover (assigned)
* Handover -> Highbay (confirmed)

On reservation of the second move (Handover -> Highbay), it would
compute again the put away rules to find a place in the whole highbay.

With the change now, the moves would look like:

* Input -> Handover (assigned)
* Handover -> Highbay/X1Y2 (confirmed)

So the move line cannot go elsewhere.

To implement this, I decided to remove the 'routing_rule_id' field
stored on 'stock.move', as this field is only needed by the methods
_apply_routing_rule_pull/push and never afterwards. The original
location has to be propagated to these methods as well, so now there
is a single dict of values used to apply the rules.
When an ormcache is cleared, it propagates the invalidation to other
workers, blowing all their caches for all models. Since we use this
cache only for the duration of a method, it's pointless.
By using the parent path of the records, we can avoid many SQL
requests to compare children or find parent locations
@guewen guewen changed the title [WMS][13.0] Add stock_routing_operation - alpha version [WMS][13.0] Add stock_dynamic_routing - alpha version May 15, 2020
guewen added 5 commits May 18, 2020 11:12
The picking type must be selected to filter on routing to apply
There is no real reason to force using the exact same location.
A sub-location should be fine.
@jgrandguillaume
Copy link
Member

@guewen Would be really nice to move it to WMS repo, I'm trying to update the issue to help people followup the overall work done

Copy link
Contributor

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

This works now like a charm. This is not alpha version anymore.
This module is stunning !

On stock.move, the `package_level_id` field did not have a `copy=False`
attribute when version 13.0 of Odoo was released. So when we create
a new move, the package level of the move being copied was linked
as well to the new move.

It has been fixed recently in odoo in
odoo/odoo@ecf726a
but to be on the safe side if the code is not up-to-date,
it's anyway better to force a False value.
@guewen guewen force-pushed the 13.0-add-stock_routing_operation branch from 6f0aa91 to 53c0b8b Compare June 3, 2020 10:22
@OCA-git-bot
Copy link
Contributor

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). 🤖

@guewen
Copy link
Member Author

guewen commented Jun 10, 2020

It should therefore be ready to merge by a maintainer

Note:
@jgrandguillaume wants me to move it to OCA/wms, so should not be merged here.

@guewen
Copy link
Member Author

guewen commented Jun 12, 2020

@jgrandguillaume Moved to OCA/wms#31 and changed to Beta.

cc @florian-dacosta

@guewen guewen closed this Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants