From 87dc2bc614693d0f6e7385db00740e2db8ced9f8 Mon Sep 17 00:00:00 2001 From: Adrien Widart Date: Fri, 7 Jan 2022 14:09:19 +0000 Subject: [PATCH] [FIX] stock_account, sale_stock: reverse AML using the returned SM value When reversing an invoice linked to a sale order, the value of the new anglo saxon AML should be based on the value of the returned product To reproduce the issue: (Need account_accountant,purchase) 1. Create a product category PC: - Costing Method: FIFO - Inventory Valuation: Automated 2. Create a product P: - Type: Storable - Product Category: PC 3. For C in [10, 20, 30]: - Create a purchase order with 1 x P at cost C - Receive P 4. Create a SO with 3 x P 5. Deliver the products one by one 6. Create and Post the invoice INV linked to SO 7. Return the second delivery 8. Add a credit note to INV01: - Credit Method: Partial Refund 9. Set the quantity to 1 10. Post the new invoice INV02 Error: The anglo saxon lines are listed in the journal items, which is correct, but their value is $30 while it should be $20 (i.e., the value of the returned product) When delivering the last product, its standard price is updated with the new value ($30): https://github.com/odoo/odoo/blob/6b96ed418cb626678f4fa5baec25a903d7a74eda/addons/stock_account/models/product.py#L305-L307 Later on, when posting the credit note, the module gets the anglo saxon unit price (`_stock_account_get_anglo_saxon_price_unit`). However, it does not consider that the current account move line is reversing another one. In the reversing process, the computation of the invoiced quantity should be based on the invoices lines that are reversing too. Moreover, when computing the average unit price of the returned product, the module should use the stock moves that are returning the product. In the use case, because of the two issued noted above, `_compute_average_price` does not find any stock valuation layer to compute the average unit price and uses the fallback, i.e. the standard price of the product: https://github.com/odoo/odoo/blob/6b96ed418cb626678f4fa5baec25a903d7a74eda/addons/stock_account/models/product.py#L652-L656 That's the reason why, in the above case, the value is $30 instead of $20. Note: a similar use case can be reproduced with a kit OPW-2646926 OPW-2628215 closes odoo/odoo#82978 X-original-commit: 524d0d5e8817e1c7bcc99204c4cbb90fcfb6074c Signed-off-by: William Henrotin (whe) Signed-off-by: Adrien Widart --- addons/mrp_account/models/product.py | 8 +- addons/sale_mrp/models/account_move.py | 7 +- addons/sale_mrp/tests/test_sale_mrp_flow.py | 115 ++++++++++++++++++ addons/sale_stock/models/account_move.py | 9 +- .../tests/test_anglo_saxon_valuation.py | 94 ++++++++++++++ addons/stock_account/models/product.py | 5 +- 6 files changed, 227 insertions(+), 11 deletions(-) diff --git a/addons/mrp_account/models/product.py b/addons/mrp_account/models/product.py index cd8c86e53f7..b2a8a4ee082 100644 --- a/addons/mrp_account/models/product.py +++ b/addons/mrp_account/models/product.py @@ -46,13 +46,13 @@ class ProductProduct(models.Model): if price: self.standard_price = price - def _compute_average_price(self, qty_invoiced, qty_to_invoice, stock_moves): + def _compute_average_price(self, qty_invoiced, qty_to_invoice, stock_moves, is_returned=False): self.ensure_one() if stock_moves.product_id == self: - return super()._compute_average_price(qty_invoiced, qty_to_invoice, stock_moves) + return super()._compute_average_price(qty_invoiced, qty_to_invoice, stock_moves, is_returned=is_returned) bom = self.env['mrp.bom']._bom_find(self, company_id=stock_moves.company_id.id, bom_type='phantom')[self] if not bom: - return super()._compute_average_price(qty_invoiced, qty_to_invoice, stock_moves) + return super()._compute_average_price(qty_invoiced, qty_to_invoice, stock_moves, is_returned=is_returned) dummy, bom_lines = bom.explode(self, 1) bom_lines = {line: data for line, data in bom_lines} value = 0 @@ -66,7 +66,7 @@ class ProductProduct(models.Model): else: # bom was altered (i.e. bom line removed) after being used line_qty = move.product_qty - value += line_qty * move.product_id._compute_average_price(qty_invoiced * line_qty, qty_to_invoice * line_qty, move) + value += line_qty * move.product_id._compute_average_price(qty_invoiced * line_qty, qty_to_invoice * line_qty, move, is_returned=is_returned) return value def _compute_bom_price(self, bom, boms_to_recompute=False, byproduct_bom=False): diff --git a/addons/sale_mrp/models/account_move.py b/addons/sale_mrp/models/account_move.py index d08f2e92e61..f1de1900838 100644 --- a/addons/sale_mrp/models/account_move.py +++ b/addons/sale_mrp/models/account_move.py @@ -15,8 +15,10 @@ class AccountMoveLine(models.Model): lambda b: not b.company_id or b.company_id == so_line.company_id )[:1] if bom and bom.type == 'phantom': + is_line_reversing = bool(self.move_id.reversed_entry_id) qty_to_invoice = self.product_uom_id._compute_quantity(self.quantity, self.product_id.uom_id) - qty_invoiced = sum([x.product_uom_id._compute_quantity(x.quantity, x.product_id.uom_id) for x in so_line.invoice_lines if x.move_id.state == 'posted']) + posted_invoice_lines = so_line.invoice_lines.filtered(lambda l: l.move_id.state == 'posted' and bool(l.move_id.reversed_entry_id) == is_line_reversing) + qty_invoiced = sum([x.product_uom_id._compute_quantity(x.quantity, x.product_id.uom_id) for x in posted_invoice_lines]) moves = so_line.move_ids average_price_unit = 0 components_qty = so_line._get_bom_component_qty(bom) @@ -26,7 +28,8 @@ class AccountMoveLine(models.Model): prod_moves = moves.filtered(lambda m: m.product_id == product) prod_qty_invoiced = factor * qty_invoiced prod_qty_to_invoice = factor * qty_to_invoice - average_price_unit += factor * product.with_company(self.company_id)._compute_average_price(prod_qty_invoiced, prod_qty_to_invoice, prod_moves) + product = product.with_company(self.company_id) + average_price_unit += factor * product._compute_average_price(prod_qty_invoiced, prod_qty_to_invoice, prod_moves, is_returned=is_line_reversing) price_unit = average_price_unit / bom.product_qty or price_unit price_unit = self.product_id.uom_id._compute_price(price_unit, self.product_uom_id) return price_unit diff --git a/addons/sale_mrp/tests/test_sale_mrp_flow.py b/addons/sale_mrp/tests/test_sale_mrp_flow.py index 454eec2f316..17781849af9 100644 --- a/addons/sale_mrp/tests/test_sale_mrp_flow.py +++ b/addons/sale_mrp/tests/test_sale_mrp_flow.py @@ -5,6 +5,7 @@ from odoo.addons.stock_account.tests.test_anglo_saxon_valuation_reconciliation_c from odoo.tests import common, Form from odoo.exceptions import UserError from odoo.tools import mute_logger, float_compare +from odoo.addons.stock_account.tests.test_stockvaluation import _create_accounting_data # these tests create accounting entries, and therefore need a chart of accounts @@ -1926,3 +1927,117 @@ class TestSaleMrpFlow(ValuationReconciliationTestCommon): so.action_draft() so.action_confirm() self.assertEqual(len(so.picking_ids), 1, "The product was already delivered, no need to re-create a delivery order") + + def test_anglo_saxo_return_and_credit_note(self): + """ + When posting a credit note for a returned kit, the value of the anglo-saxo lines + should be based on the returned component's value + """ + stock_input_account, stock_output_account, stock_valuation_account, expense_account, stock_journal = _create_accounting_data(self.env) + fifo = self.env['product.category'].create({ + 'name': 'FIFO', + 'property_valuation': 'real_time', + 'property_cost_method': 'fifo', + 'property_stock_account_input_categ_id': stock_input_account.id, + 'property_stock_account_output_categ_id': stock_output_account.id, + 'property_stock_valuation_account_id': stock_valuation_account.id, + 'property_stock_journal': stock_journal.id, + }) + + kit = self._create_product('Simple Kit', self.uom_unit) + (kit + self.component_a).categ_id = fifo + kit.property_account_expense_id = expense_account + + self.env['mrp.bom'].create({ + 'product_tmpl_id': kit.product_tmpl_id.id, + 'product_qty': 1.0, + 'type': 'phantom', + 'bom_line_ids': [(0, 0, {'product_id': self.component_a.id, 'product_qty': 1.0})] + }) + + # Receive 3 components: one @10, one @20 and one @60 + in_moves = self.env['stock.move'].create([{ + 'name': 'IN move @%s' % p, + 'product_id': self.component_a.id, + 'location_id': self.env.ref('stock.stock_location_suppliers').id, + 'location_dest_id': self.company_data['default_warehouse'].lot_stock_id.id, + 'product_uom': self.component_a.uom_id.id, + 'product_uom_qty': 1, + 'price_unit': p, + } for p in [10, 20, 60]]) + in_moves._action_confirm() + in_moves.quantity_done = 1 + in_moves._action_done() + + # Sell 3 kits + so = self.env['sale.order'].create({ + 'partner_id': self.env.ref('base.res_partner_1').id, + 'order_line': [ + (0, 0, { + 'name': kit.name, + 'product_id': kit.id, + 'product_uom_qty': 3.0, + 'product_uom': kit.uom_id.id, + 'price_unit': 100, + 'tax_id': False, + })], + }) + so.action_confirm() + + # Deliver the components: 1@10, then 1@20 and then 1@60 + pickings = [] + picking = so.picking_ids + while picking: + pickings.append(picking) + picking.move_ids.quantity_done = 1 + action = picking.button_validate() + if isinstance(action, dict): + wizard = Form(self.env[action['res_model']].with_context(action['context'])).save() + wizard.process() + picking = picking.backorder_ids + + invoice = so._create_invoices() + invoice.action_post() + + # Receive one @100 + in_moves = self.env['stock.move'].create({ + 'name': 'IN move @100', + 'product_id': self.component_a.id, + 'location_id': self.env.ref('stock.stock_location_suppliers').id, + 'location_dest_id': self.company_data['default_warehouse'].lot_stock_id.id, + 'product_uom': self.component_a.uom_id.id, + 'product_uom_qty': 1, + 'price_unit': 100, + }) + in_moves._action_confirm() + in_moves.quantity_done = 1 + in_moves._action_done() + + # Return the second picking (i.e. one component @20) + ctx = {'active_id': pickings[1].id, 'active_model': 'stock.picking'} + return_wizard = Form(self.env['stock.return.picking'].with_context(ctx)).save() + return_picking_id, dummy = return_wizard._create_returns() + return_picking = self.env['stock.picking'].browse(return_picking_id) + return_picking.move_ids.quantity_done = 1 + return_picking.button_validate() + + # Add a credit note for the returned kit + ctx = {'active_model': 'account.move', 'active_ids': invoice.ids} + refund_wizard = self.env['account.move.reversal'].with_context(ctx).create({ + 'refund_method': 'refund', + 'journal_id': invoice.journal_id.id, + }) + action = refund_wizard.reverse_moves() + reverse_invoice = self.env['account.move'].browse(action['res_id']) + with Form(reverse_invoice) as reverse_invoice_form: + with reverse_invoice_form.invoice_line_ids.edit(0) as line: + line.quantity = 1 + reverse_invoice.action_post() + + amls = reverse_invoice.line_ids + stock_out_aml = amls.filtered(lambda aml: aml.account_id == stock_output_account) + self.assertEqual(stock_out_aml.debit, 20, 'Should be to the value of the returned component') + self.assertEqual(stock_out_aml.credit, 0) + cogs_aml = amls.filtered(lambda aml: aml.account_id == expense_account) + self.assertEqual(cogs_aml.debit, 0) + self.assertEqual(cogs_aml.credit, 20, 'Should be to the value of the returned component') diff --git a/addons/sale_stock/models/account_move.py b/addons/sale_stock/models/account_move.py index a5ae7592f2c..5cd37dff339 100644 --- a/addons/sale_stock/models/account_move.py +++ b/addons/sale_stock/models/account_move.py @@ -116,10 +116,13 @@ class AccountMoveLine(models.Model): so_line = self.sale_line_ids and self.sale_line_ids[-1] or False if so_line: + is_line_reversing = bool(self.move_id.reversed_entry_id) qty_to_invoice = self.product_uom_id._compute_quantity(self.quantity, self.product_id.uom_id) - qty_invoiced = sum([x.product_uom_id._compute_quantity(x.quantity, x.product_id.uom_id) for x in so_line.invoice_lines if x.move_id.state == 'posted']) - average_price_unit = self.product_id.with_company(self.company_id)._compute_average_price(qty_invoiced, qty_to_invoice, so_line.move_ids) + posted_invoice_lines = so_line.invoice_lines.filtered(lambda l: l.move_id.state == 'posted' and bool(l.move_id.reversed_entry_id) == is_line_reversing) + qty_invoiced = sum([x.product_uom_id._compute_quantity(x.quantity, x.product_id.uom_id) for x in posted_invoice_lines]) + + product = self.product_id.with_company(self.company_id) + average_price_unit = product._compute_average_price(qty_invoiced, qty_to_invoice, so_line.move_ids, is_returned=is_line_reversing) if average_price_unit: price_unit = self.product_id.uom_id.with_company(self.company_id)._compute_price(average_price_unit, self.product_uom_id) - return price_unit diff --git a/addons/sale_stock/tests/test_anglo_saxon_valuation.py b/addons/sale_stock/tests/test_anglo_saxon_valuation.py index 21221208c6b..2bee3aac9ee 100644 --- a/addons/sale_stock/tests/test_anglo_saxon_valuation.py +++ b/addons/sale_stock/tests/test_anglo_saxon_valuation.py @@ -1296,3 +1296,97 @@ class TestAngloSaxonValuation(ValuationReconciliationTestCommon): # Expenses self.assertEqual(aml[3].debit, 12.0) self.assertEqual(aml[3].credit, 0.0) + + def test_fifo_return_and_credit_note(self): + """ + When posting a credit note for a returned product, the value of the anglo-saxo lines + should be based on the returned product's value + """ + self.product.categ_id.property_cost_method = 'fifo' + + # Receive one @10, one @20 and one @60 + in_moves = self.env['stock.move'].create([{ + 'name': 'IN move @%s' % p, + 'product_id': self.product.id, + 'location_id': self.env.ref('stock.stock_location_suppliers').id, + 'location_dest_id': self.company_data['default_warehouse'].lot_stock_id.id, + 'product_uom': self.product.uom_id.id, + 'product_uom_qty': 1, + 'price_unit': p, + } for p in [10, 20, 60]]) + in_moves._action_confirm() + in_moves.quantity_done = 1 + in_moves._action_done() + + # Sell 3 units + so = self.env['sale.order'].create({ + 'partner_id': self.partner_a.id, + 'order_line': [ + (0, 0, { + 'name': self.product.name, + 'product_id': self.product.id, + 'product_uom_qty': 3.0, + 'product_uom': self.product.uom_id.id, + 'price_unit': 100, + 'tax_id': False, + })], + }) + so.action_confirm() + + # Deliver 1@10, then 1@20 and then 1@60 + pickings = [] + picking = so.picking_ids + while picking: + pickings.append(picking) + picking.move_ids.quantity_done = 1 + action = picking.button_validate() + if isinstance(action, dict): + wizard = Form(self.env[action['res_model']].with_context(action['context'])).save() + wizard.process() + picking = picking.backorder_ids + + invoice = so._create_invoices() + invoice.action_post() + + # Receive one @100 + in_moves = self.env['stock.move'].create({ + 'name': 'IN move @100', + 'product_id': self.product.id, + 'location_id': self.env.ref('stock.stock_location_suppliers').id, + 'location_dest_id': self.company_data['default_warehouse'].lot_stock_id.id, + 'product_uom': self.product.uom_id.id, + 'product_uom_qty': 1, + 'price_unit': 100, + }) + in_moves._action_confirm() + in_moves.quantity_done = 1 + in_moves._action_done() + + # Return the second picking (i.e. 1@20) + ctx = {'active_id': pickings[1].id, 'active_model': 'stock.picking'} + return_wizard = Form(self.env['stock.return.picking'].with_context(ctx)).save() + return_picking_id, dummy = return_wizard._create_returns() + return_picking = self.env['stock.picking'].browse(return_picking_id) + return_picking.move_ids.quantity_done = 1 + return_picking.button_validate() + + # Add a credit note for the returned product + ctx = {'active_model': 'account.move', 'active_ids': invoice.ids} + refund_wizard = self.env['account.move.reversal'].with_context(ctx).create({ + 'refund_method': 'refund', + 'journal_id': invoice.journal_id.id, + }) + action = refund_wizard.reverse_moves() + reverse_invoice = self.env['account.move'].browse(action['res_id']) + with Form(reverse_invoice) as reverse_invoice_form: + with reverse_invoice_form.invoice_line_ids.edit(0) as line: + line.quantity = 1 + reverse_invoice.action_post() + + amls = reverse_invoice.line_ids + stock_out_aml = amls.filtered(lambda aml: aml.account_id == self.company_data['default_account_stock_out']) + self.assertEqual(stock_out_aml.debit, 20, 'Should be to the value of the returned product') + self.assertEqual(stock_out_aml.credit, 0) + cogs_aml = amls.filtered(lambda aml: aml.account_id == self.company_data['default_account_expense']) + self.assertEqual(cogs_aml.debit, 0) + self.assertEqual(cogs_aml.credit, 20, 'Should be to the value of the returned product') diff --git a/addons/stock_account/models/product.py b/addons/stock_account/models/product.py index 04ff048e733..6662cb4c953 100644 --- a/addons/stock_account/models/product.py +++ b/addons/stock_account/models/product.py @@ -646,7 +646,7 @@ class ProductProduct(models.Model): return price or 0.0 return self.uom_id._compute_price(price, uom) - def _compute_average_price(self, qty_invoiced, qty_to_invoice, stock_moves): + def _compute_average_price(self, qty_invoiced, qty_to_invoice, stock_moves, is_returned=False): """Go over the valuation layers of `stock_moves` to value `qty_to_invoice` while taking care of ignoring `qty_invoiced`. If `qty_to_invoice` is greater than what's possible to value with the valuation layers, use the product's standard price. @@ -654,6 +654,7 @@ class ProductProduct(models.Model): :param qty_invoiced: quantity already invoiced :param qty_to_invoice: quantity to invoice :param stock_moves: recordset of `stock.move` + :param is_returned: if True, consider the incoming moves :returns: the anglo saxon price unit :rtype: float """ @@ -667,7 +668,7 @@ class ProductProduct(models.Model): returned_quantities[move.origin_returned_move_id.id] += abs(sum(move.sudo().stock_valuation_layer_ids.mapped('quantity'))) candidates = stock_moves\ .sudo()\ - .filtered(lambda m: not (m.origin_returned_move_id and sum(m.stock_valuation_layer_ids.mapped('quantity')) >= 0))\ + .filtered(lambda m: is_returned == bool(m.origin_returned_move_id and sum(m.stock_valuation_layer_ids.mapped('quantity')) >= 0))\ .mapped('stock_valuation_layer_ids')\ .sorted() qty_to_take_on_candidates = qty_to_invoice