-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
[16.0][MIG] - shopfloor #679
Conversation
65cb513
to
4a4c669
Compare
c798905
to
3042604
Compare
a9d4c4e
to
b88249c
Compare
6c74ad3
to
0d339e8
Compare
@sbejaoui Why you dropped the migration scripts ? |
f1a4615
to
a5062fc
Compare
for picking in lines.batch_id.picking_ids: | ||
picking_lines = lines.filtered(lambda l, p=picking: l.picking_id == p) | ||
self._unload_set_picking_to_done(picking, picking_lines) | ||
|
||
def _unload_set_picking_to_done(self, picking, picking_lines): | ||
if picking.state == "done": | ||
return | ||
# We set the picking to done only when the last line is | ||
# unloaded to avoid backorders. | ||
if all(line.shopfloor_unloaded for line in picking.move_line_ids): | ||
picking._action_done() | ||
if self.work.menu.unload_package_at_destination: | ||
lines.result_package_id = False | ||
picking_lines.result_package_id = False |
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.
@sbejaoui It's difficult to know what's the semantic behind this new method '_unload_set_picking_to_done'. In your change, you still iterate multi times n the lines.....
from collections import namedtuple
def _unload_write_destination_on_lines(self, lines, location):
lines.write({"shopfloor_unloaded": True, "location_dest_id": location.id})
lines.package_level_id.location_dest_id = location
LineIdsByPickingValue = namedtuple("LINESBYP", ["ids", "all_unloaded_status"])
bypicking = {}
for line in lines:
if line.picking_id.state == "done":
continue
value = lines_by_picking_value[line.picking_id]
if value:
value = LineIdsByPickingValue(ids = value.ids + line.ids, all_unloaded_status = value.all_unloaded_status and line.shopfloor_unloaded)
else:
value = LineIdsByPickingValue(ids=line.ids, all_unloaded_status=line.shopfloor_unloaded)
lines_by_picking_value[line.picking_id] = value
for picking, line_values in lines_by_picking_value.items():
if line_values.all_unloaded_status:
picking._action_done()
if self.work.menu.unload_package_at_destination:
self.env["stock.move.line"].browse(line_values.ids).write({"result_package_id" : False})
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.
This refactoring has the purpose of separating the picking validation from the line location update. The idea behind it is to enable the addition of custom behavior during picking validation. Your proposition won't help, as we are still handling all picking in the same method
picking_lines = picking.mapped("move_line_ids") | ||
if all(line.shopfloor_unloaded for line in picking_lines): | ||
picking._action_done() | ||
for picking in lines.batch_id.picking_ids: |
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.
@lmignon ,I replaced the loop on the lines with a loop on the pickings. I don't understand how it iterates multiple times on the lines
# If a move line reference a move linked to others move lines (different | ||
# locations), we have to split the move to process only the lines related to | ||
# the current location. | ||
move_line._extract_in_split_order({"user_id": self.env.uid}) |
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.
How is that possible as when you scan the location at the start, you isolate the move lines from that location in a split order? So at this stage inside the picking we should only have move lines from that location.
In set_destination_all and set_destination_package, I think such is not done. We should have the same behavior in all 3 cases.
I would have dropped that line also unless you can reproduce the use case.
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.
It's required by a test (https://github.com/OCA/wms/pull/679/files#diff-d468159958b3bfcb32afaa09c6a77a69e1811aa92d2abe6d4022f1be66a9ad19R637) but reading the code, It seems that the test is not right since the move_line to process is not selected by using the scan_location
method and therefore the initial split is not done... I'll fix the test and remove the line.
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.
a5c06eb
to
dbab97d
Compare
dbab97d
to
268f402
Compare
f72f726
to
8bc111b
Compare
/ocabot migration shopfloor |
When an inventory is created and must be validated, the 'inventory_quantity_set' field is set to True on the quant
c36df2f
to
3edc0d3
Compare
3edc0d3
to
7aa1275
Compare
@rousseldenis @jbaudoux This one is finally 🟢 Can we proceed to the merge so I can move forward with others PRs and thereafter forward port the last changes from 14.0 |
An important element for the future. Shopfloor should be split into several smaller modules (1 per scenario). The current approach is not healthy for maintenance. |
@@ -51,8 +51,10 @@ def _get_existing_quant(self, location, product, package=None, lot=None, limit=1 | |||
domain.append(("lot_id", "=", False)) | |||
return self.inventory_model.search(domain, limit=limit) | |||
|
|||
def _create_draft_inventory(self, location, product, lot=None): | |||
quants = self._get_existing_quant(location, product, lot=lot, limit=None) | |||
def _create_draft_inventory(self, location, product, package=None, lot=None): |
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.
This should be completed with owner_id field too.
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.
maybe after the forward port from v14
ping @simahawk Can we merge this.... It works in production for months (cluster + location_content) and we want to finalize all the pending PRs on shopfloor for v16 and forward port the last changes from v14 to have both branches aligned. |
Let's merge this one. It will allow to do enhancements in separate PR's. /ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
@rousseldenis your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-679-by-rousseldenis-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
Makes the sorting of stock move line more predictable if move lines share the same attributes
/ocabot merge nobump |
On my way to merge this fine PR! |
Congratulations, your PR was merged at 434a25d. Thanks a lot for contributing to OCA. ❤️ |
needs: