From 6db845e4e87f70ffd15a610bd3fc84284c561ad4 Mon Sep 17 00:00:00 2001 From: Tom Date: Wed, 18 Sep 2024 14:19:07 +0200 Subject: [PATCH] [RFR] Refactor in order to solve bugs - One bug in particular related to the amount of stock.move.line records on PO picking being unequal to those on the SO picking side, and the resulting mismatch prevented the confirm to continue. This is actually a common case that happens when you confirm early and want to make a backorder; in such a case the PO side will still a higher reserved qty and a matching number of move lines. Solve for this case by removing any excess PO-side stock.move.line records upon sync. - Another bug was that if for whatever reason a certain move could not be synced, it would still continue to set qty_done for that move, instead of skipping it. This led to errors. Furthermore, in such a case it would still create the serial numbers in the destination company but not actually link them to the move lines, leading to errors about the serial number already existing if people want to solve the situation manually by typing in the serial numbers on the picking. In order to solve this, I had to merge the 'button_validate' and '_action_done' overrides into one single unified function override. Because the '_action_done' override dealt with the syncing of lot numbers, and the 'button_validate' override dealt with the syncing of 'qty_done', but if the first fails, you want to also skip the other, which is difficult if they are executed in different functions. --- .../models/stock_picking.py | 267 +++++++----------- .../tests/test_inter_company_purchase_sale.py | 100 ++++++- 2 files changed, 198 insertions(+), 169 deletions(-) diff --git a/purchase_sale_inter_company/models/stock_picking.py b/purchase_sale_inter_company/models/stock_picking.py index 051a6ed08aa..e866af31eee 100644 --- a/purchase_sale_inter_company/models/stock_picking.py +++ b/purchase_sale_inter_company/models/stock_picking.py @@ -83,34 +83,53 @@ def _sync_lots(self, ml, po_ml): po_ml.lot_id = dest_lot_id def _action_done(self): + ret = super()._action_done() + # sync lots for move lines on returns + # TODO: integrate with the non-return part just below this clause for pick in self.filtered(lambda x: x.intercompany_return_picking_id).sudo(): - ic_picking = pick.intercompany_return_picking_id - dest_company = ic_picking.sudo().company_id + ic_pick = pick.intercompany_return_picking_id + dest_company = ic_pick.sudo().company_id if not dest_company.sync_picking: continue - intercompany_user = dest_company.intercompany_sale_user_id - for product in pick.move_lines.mapped("product_id"): - if product.tracking == "none": - continue - move_lines = ( - pick.move_lines.filtered( - lambda x, prod=product: x.product_id == product + ic_user = dest_company.intercompany_sale_user_id + ic_pick = ic_pick.with_user(ic_user) + try: + dest_move_qty_update_dict = {} + for move in pick.move_lines: + product = move.product_id + move_lines = move.move_line_ids + po_move = ic_pick.move_lines.filtered( + lambda x, prod=product: x.product_id == prod ) - .mapped("move_line_ids") - .filtered("lot_id") - ) - po_move_lines = ( - ic_picking.with_user(intercompany_user) - .move_lines.filtered(lambda x, prod=product: x.product_id == prod) - .mapped("move_line_ids") - ) - if len(move_lines) != len(po_move_lines): - pick._warn_move_line_mismatch( - ic_picking, product, move_lines, po_move_lines + po_move_lines = po_move.mapped("move_line_ids") + # Don't support one move splitting into multiple moves with the same product + # (Not sure how to even achieve that in Odoo) + # And don't support cases with more stock.move.line SO-side than PO side + if len(po_move) != 1 or len(move_lines) > len(po_move_lines): + pick._warn_move_line_mismatch( + ic_pick, product, move_lines, po_move_lines + ) + continue + # Delete excess stock.move.line on PO side + if len(po_move_lines) > len(move_lines): + po_move_lines[len(move_lines) :].unlink() + for ml, po_ml in zip(move_lines, po_move_lines): + pick._sync_lots(ml, po_ml) + po_ml.qty_done = ml.qty_done + dest_move_qty_update_dict.setdefault(po_move, 0.0) + dest_move_qty_update_dict[po_move] += move.quantity_done + for dest_move, qty_done in dest_move_qty_update_dict.items(): + dest_move.quantity_done = qty_done + ic_pick._action_done() + except Exception as e: + if dest_company.sync_picking_failure_action == "raise": + raise + else: + pick._notify_picking_problem( + pick.sale_id.auto_purchase_order_id, + additional_note=str(e), ) - for ml, po_ml in zip(move_lines, po_move_lines): - pick._sync_lots(ml, po_ml) # sync lots for move lines on pickings for pick in self.filtered( @@ -119,43 +138,70 @@ def _action_done(self): purchase = pick.sale_id.auto_purchase_order_id if not purchase: continue - purchase.picking_ids.write({"intercompany_picking_id": pick.id}) - if not pick.intercompany_picking_id and purchase.picking_ids[0]: - pick.write({"intercompany_picking_id": purchase.picking_ids[0]}) - pick._action_done_intercompany_actions(purchase) - return super()._action_done() - - def _action_done_intercompany_actions(self, purchase): - self.ensure_one() - try: - pick = self - for move in pick.move_lines: - move_lines = move.move_line_ids - po_move_lines = move.sale_line_id.auto_purchase_line_id.move_ids.filtered( - lambda x, ic_pick=pick.intercompany_picking_id, prod=move.product_id: x.picking_id # noqa - == ic_pick - and x.product_id == prod - ).mapped( - "move_line_ids" - ) - if len(move_lines) != len(po_move_lines): - self._notify_picking_problem( - purchase, - additional_note=_( - "Mismatch between move lines with the " - "corresponding PO %s for assigning " - "quantities and lots from %s for product %s" + dest_company = purchase.company_id + if not dest_company.sync_picking: + continue + try: + if not purchase.picking_ids: + raise UserError(_("PO does not exist or has no receipts")) + ic_user = dest_company.intercompany_sale_user_id + purchase.picking_ids.write({"intercompany_picking_id": pick.id}) + ic_pick = pick.intercompany_picking_id.with_user(ic_user) + if not ic_pick and purchase.picking_ids[0]: + pick.write({"intercompany_picking_id": purchase.picking_ids[0]}) + # formerly in action_done_intercompany_actions + dest_move_qty_update_dict = {} + for move in pick.move_lines: + move_lines = move.move_line_ids + po_move = move.sale_line_id.auto_purchase_line_id.move_ids.filtered( + lambda x, ic_picking=ic_pick, prod=move.product_id: x.picking_id # noqa + == ic_picking + and x.product_id == prod + ) + po_move_lines = po_move.mapped("move_line_ids") + # Don't support one move splitting into multiple moves with the same product + # (Not sure how to even achieve that in Odoo) + # And don't support cases with more stock.move.line SO-side than PO side + if len(po_move) != 1 or len(move_lines) > len(po_move_lines): + self._notify_picking_problem( + purchase, + additional_note=_( + "Mismatch between move lines with the " + "corresponding PO %s for assigning " + "quantities and lots from %s for product %s" + ) + % (purchase.name, pick.name, move.product_id.name), ) - % (purchase.name, pick.name, move.product_id.name), + continue + # Delete excess stock.move.line on PO side + if len(po_move_lines) > len(move_lines): + po_move_lines[len(move_lines) :].unlink() + for ml, po_ml in zip(move_lines, po_move_lines): + pick._sync_lots(ml, po_ml) + po_ml.qty_done = ml.qty_done + dest_move_qty_update_dict.setdefault(po_move, 0.0) + dest_move_qty_update_dict[po_move] += move.quantity_done + # formerly in sync_receipt_to_delivery + # "No backorder" case splits SO moves in two while PO stays the same. + # Aggregating writes per each PO move makes sure qty does not get overwritten + for dest_move, qty_done in dest_move_qty_update_dict.items(): + dest_move.quantity_done = qty_done + ic_pick.with_context( + cancel_backorder=bool( + self.env.context.get("picking_ids_not_to_backorder") + ) + )._action_done() + + except Exception as e: + if dest_company.sync_picking_failure_action == "raise": + raise + else: + pick._notify_picking_problem( + pick.sale_id.auto_purchase_order_id, + additional_note=str(e), ) - for ml, po_ml in zip(move_lines, po_move_lines): - pick._sync_lots(ml, po_ml) - except Exception as e: - if self.env.company_id.sync_picking_failure_action == "raise": - raise - else: - self._notify_picking_problem(purchase, additional_note=str(e)) + return ret def _notify_picking_problem(self, purchase, additional_note=False): self.ensure_one() @@ -166,6 +212,7 @@ def _notify_picking_problem(self, purchase, additional_note=False): ) % (purchase.name, self.name) if additional_note: note += _(" Additional info: ") + additional_note + _logger.warning(note) user_id = self.sudo()._get_user_to_notify(purchase) self.sudo().activity_schedule( "mail.mail_activity_data_warning", @@ -186,37 +233,6 @@ def _get_user_to_notify(self, purchase): def button_validate(self): res = super().button_validate() - for record in self.sudo(): - dest_company = ( - record.sale_id.partner_id.commercial_partner_id.ref_company_ids - ) - if ( - dest_company - and dest_company.sync_picking - # only if it worked, not if wizard was raised - and record.state == "done" - ): - try: - if ( - record.picking_type_code == "outgoing" - and record.intercompany_picking_id - ): - record._sync_receipt_with_delivery( - dest_company, - record.sale_id, - ) - elif ( - record.picking_type_code == "incoming" - and record.intercompany_return_picking_id - ): - record._sync_receipt_with_delivery(dest_company, None) - except Exception: - if record.company_id.sync_picking_failure_action == "raise": - raise - else: - record._notify_picking_problem( - record.sale_id.auto_purchase_order_id - ) # if the flag is set, block the validation of the picking in the destination company if self.env.company.block_po_manual_picking_validation: @@ -233,83 +249,6 @@ def button_validate(self): ) return res - def _sync_receipt_with_delivery(self, dest_company, sale_order): - self.ensure_one() - intercompany_user = dest_company.intercompany_sale_user_id - - # sync SO return to PO return - if self.intercompany_return_picking_id: - moves = [(m, m.quantity_done) for m in self.move_ids_without_package] - dest_picking = self.intercompany_return_picking_id.with_user( - intercompany_user - ) - all_dest_moves = self.intercompany_return_picking_id.with_user( - intercompany_user - ).move_lines - for move, qty in moves: - dest_moves = all_dest_moves.filtered( - lambda x, prod=move.product_id: x.product_id == prod - ) - remaining_qty = qty - remaining_ml = move.move_line_ids - while dest_moves and remaining_qty > 0.0: - dest_move = dest_moves[0] - to_assign = min( - remaining_qty, - dest_move.product_uom_qty - dest_move.quantity_done, - ) - final_qty = dest_move.quantity_done + to_assign - for line, dest_line in zip(remaining_ml, dest_move.move_line_ids): - # Assuming the order of move lines is the same on both moves - # is risky but what would be a better option? - dest_line.sudo().write( - { - "qty_done": line.qty_done, - } - ) - dest_move.quantity_done = final_qty - remaining_qty -= to_assign - if dest_move.quantity_done == dest_move.product_qty: - dest_moves -= dest_move - dest_picking._action_done() - - # sync SO to PO picking - if self.intercompany_picking_id: - purchase_order = sale_order.auto_purchase_order_id.sudo() - if not (purchase_order and purchase_order.picking_ids): - raise UserError(_("PO does not exist or has no receipts")) - dest_picking = self.intercompany_picking_id.with_user(intercompany_user.id) - dest_move_qty_update_dict = {} - for move in self.move_ids_without_package.sudo(): - # To identify the correct move to write to, - # use both the SO-PO link and the intercompany_picking_id link - # as well as the product, to support the "kit" case where an order line - # unpacks into several move lines - dest_move = move.sale_line_id.auto_purchase_line_id.move_ids.filtered( - lambda x, pick=dest_picking, prod=move.product_id: x.picking_id - == pick - and x.product_id == prod - ) - for line, dest_line in zip(move.move_line_ids, dest_move.move_line_ids): - # Assuming the order of move lines is the same on both moves - # is risky but what would be a better option? - dest_line.sudo().write( - { - "qty_done": line.qty_done, - } - ) - dest_move_qty_update_dict.setdefault(dest_move, 0.0) - dest_move_qty_update_dict[dest_move] += move.quantity_done - # "No backorder" case splits SO moves in two while PO stays the same. - # Aggregating writes per each PO move makes sure qty does not get overwritten - for dest_move, qty_done in dest_move_qty_update_dict.items(): - dest_move.quantity_done = qty_done - dest_picking.sudo().with_context( - cancel_backorder=bool( - self.env.context.get("picking_ids_not_to_backorder") - ) - )._action_done() - def _update_extra_data_in_picking(self, picking): if hasattr(self, "_cal_weight"): # from delivery module self._cal_weight() diff --git a/purchase_sale_inter_company/tests/test_inter_company_purchase_sale.py b/purchase_sale_inter_company/tests/test_inter_company_purchase_sale.py index 674cdf61ae9..f944281562b 100644 --- a/purchase_sale_inter_company/tests/test_inter_company_purchase_sale.py +++ b/purchase_sale_inter_company/tests/test_inter_company_purchase_sale.py @@ -84,6 +84,7 @@ def setUpClass(cls): "type": "product", "tracking": "serial", "categ_id": cls.env.ref("product.product_category_all").id, + "company_id": None, } ) @@ -414,8 +415,8 @@ def _assert_picking_equal_lots(self, pick1, pick2): def test_sync_picking_no_backorder(self): self.company_a.sync_picking = True self.company_b.sync_picking = True - self.company_a.sync_picking_state = True - self.company_b.sync_picking_state = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False purchase = self._create_purchase_order( self.partner_company_b, self.consumable_product @@ -429,7 +430,7 @@ def test_sync_picking_no_backorder(self): so_picking_id = sale.picking_ids # check po_picking state - self.assertEqual(po_picking_id.state, "waiting") + self.assertEqual(po_picking_id.state, "assigned") # validate the SO picking so_picking_id.move_lines.quantity_done = 2 @@ -502,6 +503,7 @@ def test_sync_picking_lot(self): Test that the lot is synchronized on the moves by searching or creating a new lot in the company of destination """ + """ Sad flow for lot picking """ # lot 3 already exists in company_a serial_3_company_a = self._create_serial_and_quant( self.stockable_product_serial, @@ -511,8 +513,10 @@ def test_sync_picking_lot(self): ) self.company_a.sync_picking = True self.company_b.sync_picking = True - self.company_a.sync_picking_state = True - self.company_b.sync_picking_state = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False + self.company_a.sync_picking_failure_action = "raise" + self.company_b.sync_picking_failure_action = "raise" purchase = self._create_purchase_order( self.partner_company_b, @@ -644,6 +648,78 @@ def test_sync_picking_lot(self): self._assert_picking_equal_lines(return_pick_po, return_pick) self._assert_picking_equal_lots(return_pick_po, return_pick) + def test_sync_picking_lot_fail(self): + """Sad flow for lot picking""" + self.company_a.sync_picking = True + self.company_b.sync_picking = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False + + purchase = self._create_purchase_order( + self.partner_company_b, + self.stockable_product_serial + self.consumable_product, + ) + sale = self._approve_po(purchase) + + # validate the SO picking + po_picking_id = purchase.picking_ids + so_picking_id = sale.picking_ids + + so_moves = so_picking_id.move_lines + so_moves[1].quantity_done = 2 + so_moves[0].move_line_ids = [ + ( + 0, + 0, + { + "location_id": so_moves[0].location_id.id, + "location_dest_id": so_moves[0].location_dest_id.id, + "product_id": self.stockable_product_serial.id, + "product_uom_id": self.stockable_product_serial.uom_id.id, + "qty_done": 1, + "lot_id": self.serial_1.id, + "picking_id": so_picking_id.id, + }, + ), + ( + 0, + 0, + { + "location_id": so_moves[0].location_id.id, + "location_dest_id": so_moves[0].location_dest_id.id, + "product_id": self.stockable_product_serial.id, + "product_uom_id": self.stockable_product_serial.uom_id.id, + "qty_done": 1, + "lot_id": self.serial_3.id, + "picking_id": so_picking_id.id, + }, + ), + ] + wizard_data = so_picking_id.with_user(self.user_company_b).button_validate() + wizard = ( + self.env["stock.backorder.confirmation"] + .with_context(**wizard_data.get("context")) + .create({}) + ) + wizard.with_user(self.user_company_b).process() + self.assertEqual(so_picking_id.state, "done") + self.assertNotEqual((sale.picking_ids - so_picking_id).state, "done") + self._assert_picking_equal_lots(so_picking_id, po_picking_id) + + # A backorder should have been made for both + so_back_pick_id = sale.picking_ids - so_picking_id + po_back_pick_id = purchase.picking_ids - po_picking_id + self.assertEqual(len(so_back_pick_id), 1) + self.assertEqual(len(po_back_pick_id), 1) + + # TODO: somehow simulate a failure here, eg by inserting extra stock move lines on + # the SO side that don't exist on the PO side + # Then test whether not only the picking doesn't go to done, but also, + # it refrains from creating the lot_ids on the PO company side, + # So that user can do it manually again without running into 'already exist' error + # self.assertEqual(so_picking_id.state, "done") + # self.assertEqual(po_picking_id.state, "done") + def test_sync_picking_same_product_multiple_lines(self): """ Picking synchronization should work even when there @@ -805,6 +881,8 @@ def test_block_manual_validation(self): def test_notify_picking_problem(self): self.company_a.sync_picking = True self.company_b.sync_picking = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False self.company_a.sync_picking_failure_action = "notify" self.company_b.sync_picking_failure_action = "notify" self.company_a.notify_user_id = self.user_company_a @@ -845,9 +923,15 @@ def test_notify_picking_problem(self): warning_activity.user_id, so_picking_id.company_id.notify_user_id ) + # The PO picking will not even be assigned + po_picking_id = purchase.picking_ids + self.assertEqual(po_picking_id.state, "assigned") + def test_notify_picking_problem_dest_company(self): self.company_a.sync_picking = True self.company_b.sync_picking = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False self.company_a.sync_picking_failure_action = "notify" self.company_b.sync_picking_failure_action = "notify" self.company_a.notification_side = "po" @@ -882,9 +966,15 @@ def test_notify_picking_problem_dest_company(self): # Test the user assigned to the activity self.assertEqual(warning_activity.user_id, self.user_company_a) + # The picking should still be in confirmed state + po_picking_id = purchase.picking_ids + self.assertEqual(po_picking_id.state, "assigned") + def test_raise_picking_problem(self): self.company_a.sync_picking = True self.company_b.sync_picking = True + self.company_a.sync_picking_state = False + self.company_b.sync_picking_state = False self.company_a.sync_picking_failure_action = "raise" self.company_b.sync_picking_failure_action = "raise"