-
-
Notifications
You must be signed in to change notification settings - Fork 723
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][12.0] Add stock_picking_type_routing_operation - alpha version #639
Conversation
When a move is assigned and its source location (or one of its parent) corresponds to a picking type flagged "is_zone", a new move will be inserted with this picking type.
1d5fa2e
to
4ab2334
Compare
@guewen Is this linked to the doc @jgrandguillaume wrote ? If yes, it should be useful to write an issue with tasks. What do yout think ? |
Hi @rousseldenis , Yes it is. I wanted to do it, but truth is the deadline for the first POC are tight and I didn't cope with the loads... I try to fix this before end of next week, updating both https://docs.google.com/document/d/1mct6bFFWJqW01wGFcjc-uQNEjyCxvh6Y9TuFdRhe-b0/edit#heading=h.49w4ly4e5y8g and creating an issue here. Regards, Joël |
Hi @guewen , Looks very promising :) I found one problem for now.
So to try to be more concrete, do the following with same config [1]:
The WH/PICK/00003 is available to pick the "Pedal Bin" and the WH/PICK/00004 is waiting to get the goods out from the highbay. We expect:
It means, after creating the additional move, we should assign the move in a picking (using assign_picking method). In that case we should know we already have an existing picking to classify this move in. Regardless the number of goods to ship and number of zone, a roundtrip picking of a certain level in the location tree always contain all operation of its childs in a same picking. This is the essence of the roudtrip picking it-self. If we have more than one PICK, it's wrong. Hope this is clear. A part from that, I think it works very well. I try to make a few inline comments on the naming or definition of fields if I can. Regards, Joël [1]
|
@rousseldenis I've done the issue. Let me know if this works for you like this:
Hope this works for you. feel free to tell me if you want anything different. cc @pedrobaeza |
@jgrandguillaume thanks for the feedback! The issue you mention is solved. |
@jgrandguillaume I added support for splitting moves when they are sourced from different locations amongst which we have different zones/non-zones. |
When a move has several move lines because it is sourced from different locations, and these locations come from different zones, or from a zone and another location which is not a zone, we have to split the move in as many moves as we have zones, and chain them properly. This is needed because otherwise, the move for, for instance taking goods from a shelf would have to wait on the move taking from the Higbay which has been added in front of it. We expect that we can already process the Shelf one. The algorithm is to find all the zones of a move, split the move if any. But to do so, we have to unreserve the move first. As we want to keep the same quantities from the same locations (eg. 6 from the highbay bin 1), when we reserve again the quants, we have to force the reservation system to take the goods from the same location than we had originally (otherwise, the quantities that we used to split the move may change and we are back to the beginning).
220768f
to
f8a5a7c
Compare
Build error is in |
@guewen Works as expected. Thanks fort the work.
Setup with pick + pack + ship. Ordered quantities of product in each zones + in the highbay split of quantities in 2 separate bin. Let's see the feeback after customer workshop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, functional test ok, Add the development status (aplha) and ready to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First little review
# it is important to assign the zones first | ||
for location_id, new_move_ids in new_move_per_location.items(): | ||
new_moves = self.browse(new_move_ids) | ||
new_moves.with_context( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guewen Using new context should be avoided as possible (especially in a loop).
Did you try to override the 'should_bypass_reservation()' function defined on location model ?
That one would bypass the calls to '_update_reserved_quantity' etc... on quant model. So, you can call your process to reserve.
For the exclude_apply_zone, would a test on move location_id is a zone on _action_assign will work ?
def _action_assign(self):
super()._action_assign()
moves_not_zone = self.filtered(lambda m: m.<is not a zone()>)
moves = moves_not_zone._split_per_zone()
moves._apply_move_location_zone()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll have a look.
continue | ||
|
||
move._do_unreserve() | ||
move_to_assign_ids.add(move.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using recordset directly? So, under you can avoid a browse()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had awful performance issues with adding ids to a recordset in a loop. Maybe it's not an issue anymore in 12.0 though...
if move.state not in ('assigned', 'partially_available'): | ||
continue | ||
|
||
pick_type_model = self.env['stock.picking.type'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put it outside the loop
# At this point, we should not have lines with different zones, | ||
# they have been split in _split_per_zone(), so we can take the | ||
# first one | ||
source = move.move_line_ids[0].location_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using first:
from odoo.fields import first
first(move.move_line_ids).location_id
class StockPickingType(models.Model): | ||
_inherit = 'stock.picking.type' | ||
|
||
is_zone = fields.Boolean( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll probably want to rename this (as well as the module's name) by something more specific than zone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, usage of zone is a bit confusing as the vars sometimes refer to a picking.type and sometimes to a stock.location.
Move is_zone flag to stock.location picking_type M2o Replace occurences of zone for routing operation
2cb35d4
to
87ee5a7
Compare
Rename the existing field and methods to differentiate source and destination routing operations.
The reason is that we have to keep moves that come from the same source in the same picking. If we have 2 products, one with a routing, the other without, we expect: +-----------------------------------------------+ | IN/xxxx | | Product1 Supplier → Input | | Product2 Supplier → Input | +-----------------------------------------------+ +-----------------------------------------------+ | INT/xxxx | | Product1 Input → Stock/Handover | | Product2 Input → Shelf1 | +-----------------------------------------------+ the new one with our routing picking type: +-----------------------------------------------+ | HO/xxxx | | Product1 Stock/Highbay/Handover → Highbay1-2 | +-----------------------------------------------+ And not: +-----------------------------------------------+ | IN/xxxx | | Product1 Supplier → Input | | Product2 Supplier → Input | +-----------------------------------------------+ +-----------------------------------------------+ | HO/xxxx | | Product1 Input → Stock/Handover | +-----------------------------------------------+ the new one with our routing picking type: +-----------------------------------------------+ | INT/xxxx | | Product1 Stock/Highbay/Handover → Highbay1-2 | | Product2 Input → Shelf1 | +-----------------------------------------------+
From @jbaudoux comment on review: "action_assign can create package level, so you need to destroy it when you unreserve" Co-Authored-By: Jacques-Etienne Baudoux <[email protected]>
Skip creation of a routing move when the destination is already the destination of the routing picking type
d7e3c4d
to
6f7d88c
Compare
fc3ea1e
to
d7bb61e
Compare
Hi @guewen With the module as it is, it seems that applying the move_routing_operation is kind of systematic. In order to be a bit more flexible, I'd like to add a possibility to avoid this logic at move level. I was also wondering about the problem that could bring a wrong stock level. Let's take an example with the High-Bay/Handover case. A picking PICK (WH/stock => WH/output) containing 1 move with 1 product A. At reservation time, we will have :
Then, when the inventory is done, there is not more product A in Highbay, but the pickings PICK-HIGHBAY and PICK are still there. This way, the move would still be stuck in Highbay, which is still not good but seems less a problem than reserving the stock in a location that has nothing to do with the Highbay or Handover location... |
move_lines = {} | ||
for move_line in move.move_line_ids: | ||
location = move_line.location_id | ||
move_lines[location] = sum(move_line.mapped("product_uom_qty")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guewen
Here we consider product_uom_qty
, but later in the split, Odoo expects the quantity in default product uom.
I think we should consider the product_qty
instead.
I made a quick test, and It seems it ends on working, but not the way we expect.
I have created a product A with 6 qty in WH/Stock/highbay and 18 in WH/Stock/shelf 1
I left default uom as unit.
Then, I have created a delivery order with qty 2 / product_uom dozen
After reservation, I have
The Highbay=>Handover with 2 moves with unit as uom (with qty 0.5 and 5.5)
Then I also have a delivery order with one move, with dozen as uom and qty 1.5.
Well, at last, it is all kind of good, but it is weird that the uom of stock move changes during reservation, and it is weird that we have 2 moves with qty 0.5 and 5.5 in the same picking.
I guess if we work with qty in product uom, it will resolve all of this.
I did not test, but I guess something similar happens in _split_per_dest_routing_operation
@florian-dacosta thanks for your comment!
Hmm beware, I'll do several changes in the data model and I'm not happy with the overall complexity. It's a moving target.
The particularity of this addon is that it modifies the routing depending on the move lines, so only after the reservation is done, and we know from where we have to take goods. The downside of this is the relative inefficiency, we have to reserve to know where to pick, unreserve, maybe split if we have different sources, and reserve again. If your use case does not depend on the move lines, it seems sub-optimal.
I merged it in my branch #788 but again, things will move.
Same thing as if you are in a 2/3 steps delivery I guess? You can probably move products in the location with an internal move or cancel the picking. @jgrandguillaume may have more insights.
Maybe. |
Not a big issue, actually, I probably won't share it to the OCA, as it seems a lot more complexe to do in v8 with operations instead of stock move lines.
If the module changes but we keep the possibility to by pass the logic for a specific move, that's perfect. Anyway, I'll keep following. |
Well my case also depend on the stock move line. |
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:
them in two different picking type
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:
The initial setup in the demo data contains locations:
The "Highbay" location (and children) is configured to:
goods are taken from Highbay (using a new picking type Highbay → Handover)
goods are put to Highbay (using a new picking type Handover → Highbay)
Steps to try the Source Routing Operation:
Steps to try the Destination Routing Operation:
In the "WH/Stock" location, create a Put-Away Strategy with:
Create a new purchase order of:
Confirm the purchase
You'll have 2 transfers: