[16.0][FIX] rma_sale: Fix problems with link between RMA-sale.order and procurement.group-sale.order when creating from stock.picking.return wizard

[TheClaud99]

Module

rma_sale

Describe the bug

There's some bugs in the rma_sale module when creating a return from the stock.picking.return wizard with the create_rma flag set.

To Reproduce

Affected versions:

Steps to reproduce the behavior:

1. Create a sale.order with a picking
2. Validate the picking
3. Create a return with the create_rma flag set.

Expected behavior
The order_id on the RMA and sale_id on the new procurement.group are set

[OCA-git-bot]

Hi @chienandalu, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

[pedrobaeza]

I don't get why you need this. Can you please explain the goal of having these fields set?

[TheClaud99]

The field order_id is used for example as the inverse field in the rma_ids on sale.order, used for logic like the override of the function _get_invoiced. If you see my first commit 9787b8a there is a bug with a function name that causes it to never be called when it should set the order_id.

The field sale_id on the procurement.group is used for computing the field picking_ids on sale.order and in the actual version should be set by the function _prepare_procurement_group_vals in the module rma_sale but actually it never sets it because it checks for self.order_id but the function is called with no records in self.

[pedrobaeza]

OK, for the first commit. The second one seems weird to me and I don't get your explanation. If you see, the method is not covered by tests, so it seems still not called in usual flows. Other pending points:

You have to also check pre-commit CI, and the rest of the comments inline.

[TheClaud99]

Sorry for my bad explanation, I try to be more clear. When the method _prepare_procurement_group_vals is called from here

rma/rma/wizard/stock_picking_return.py

Line 83 in 3436ec6

vals = self.env["rma"]._prepare_procurement_group_vals()

the recordset in self is empty, so this condition will never be true because self.order_id will always be false:

rma/rma_sale/models/rma.py

Lines 167 to 171 in 3436ec6

	def _prepare_procurement_group_vals(self):
	vals = super()._prepare_procurement_group_vals()
	if not self.env.context.get("ignore_rma_sale_order") and self.order_id:
	vals["sale_id"] = self.order_id.id
	return vals

and this override in this case doesn't work. My commit tries to fix this case, but if it's not ok I'll try to find a cleaner solution.

[claudio-mano]

Hi @pedrobaeza, I updated the code to be clearer, now the pre-commit CI works except for the tests. That's because the rma_sale module doesn't have a test method for the creation from stock.picking.return wizard like the one in the rma module, so the method I've added is not covered

rma/rma/tests/test_rma.py

Lines 679 to 708 in 3436ec6

	def test_rma_from_picking_return(self):
	# Create a return from a delivery picking
	origin_delivery = self._create_delivery()
	stock_return_picking_form = Form(
	self.env["stock.return.picking"].with_context(
	active_ids=origin_delivery.ids,
	active_id=origin_delivery.id,
	active_model="stock.picking",
	)
	)
	stock_return_picking_form.create_rma = True
	return_wizard = stock_return_picking_form.save()
	picking_action = return_wizard.create_returns()
	# Each origin move is linked to a different RMA
	origin_moves = origin_delivery.move_ids
	self.assertTrue(origin_moves[0].rma_ids)
	self.assertTrue(origin_moves[1].rma_ids)
	rmas = origin_moves.rma_ids
	self.assertEqual(rmas.mapped("state"), ["confirmed"] * 2)
	# Each reception move is linked one of the generated RMAs
	reception = self.env["stock.picking"].browse(picking_action["res_id"])
	reception_moves = reception.move_ids
	self.assertTrue(reception_moves[0].rma_receiver_ids)
	self.assertTrue(reception_moves[1].rma_receiver_ids)
	self.assertEqual(reception_moves.rma_receiver_ids, rmas)
	# Validate the reception picking to set rmas to 'received' state
	reception_moves[0].quantity_done = reception_moves[0].product_uom_qty
	reception_moves[1].quantity_done = reception_moves[1].product_uom_qty
	reception.button_validate()
	self.assertEqual(rmas.mapped("state"), ["received"] * 2)

[pedrobaeza]

Thanks @TheClaud99. Now the code and the problem is much clearer now. I think the main problem is this call with no RMA:

rma/rma/wizard/stock_picking_return.py

Line 83 in 3436ec6

vals = self.env["rma"]._prepare_procurement_group_vals()

which is broken from the beginning. No partner is assigned as well.

What about creating the RMA, and then creating the procurement group from the RMA, so the rest of the code is valid? Putting also a self.ensure_one() in the prepare_vals will ensure no incorrect call is done.

[TheClaud99]

The problem is that multiple RMAs could be created from the same stock.picking.return, which, according to the current logic, all share the same procurement.group:

rma/rma/wizard/stock_picking_return.py

Lines 98 to 112 in 3436ec6

	def _prepare_rma_vals_list(self):
	vals_list = []
	for return_picking in self:
	global_vals = return_picking._prepare_rma_vals()
	for line in return_picking.product_return_moves:
	if (
	not line.move_id
	or float_compare(line.quantity, 0, line.product_id.uom_id.rounding)
	<= 0
	):
	continue
	vals = deepcopy(global_vals)
	vals.update(line._prepare_rma_vals())
	vals_list.append(vals)
	return vals_list

rma/rma/wizard/stock_picking_return.py

Lines 130 to 131 in 3436ec6

	vals_list = self._prepare_rma_vals_list()
	rmas = self.env["rma"].create(vals_list)

Maybe the best solution could be to create the group by calling the method on the first created RMA and then set that group also to the others?

[pedrobaeza]

Yes, I think the call should be done with a RMA in self. Then, we decide later when creating subsequent RMAs if the procurement group is the same and then just assign it from the previous create. If it's the same group for all unconditionally, then just assign it.

[TheClaud99]

Hi @pedrobaeza, another solution I thought is to use the same logic as here (function _group_delivery_if_needed()):

rma/rma/models/rma.py

Lines 1121 to 1124 in 09dd76a

	proc_group = self.env["procurement.group"].create(
	rmas._prepare_procurement_group_vals()
	)
	rmas.write({"procurement_group_id": proc_group.id})

I just also added this line

rma/rma/wizard/stock_picking_return.py

Line 128 in 910e73a

proc_group_vals["name"] = self.picking_id.name

to maintain the same logic as before for the group name

rma/rma/wizard/stock_picking_return.py

Lines 101 to 104 in 09dd76a

	origin = self.picking_id.name
	vals = self.env["rma"]._prepare_procurement_group_vals()
	vals["partner_id"] = partner_shipping.id
	vals["name"] = origin

[TheClaud99]

@pedrobaeza I also made another fix that may be added to this pr: eeb9f7f.
The field sale_line_id on the move can be useful for populating the move_ids field on sale.order.line here:
https://github.com/odoo/odoo/blob/98408d48eecd7416275fbfc8b30769d844dbacba/addons/sale_stock/models/sale_order_line.py#L17
which is used for example to compute the delivered quantity on the sale line.
Can I merge the commit in this branch or should I create a new pr?

[pedrobaeza]

This is done on purpose, as we don't want to affect the invoceability of the sales order. It's linked later.

[TheClaud99]

Ok sorry, I didn't see that it's linked later

[sbejaoui]

Hi @TheClaud99 ,

IMO, the issue is due to the non-unified of procurement groups creation.

When the picking return wizard is used, one procurement group is created for all RMAs. However, this is not the case when the sales order return wizard is used or when RMAs are created manually.

#423 addresses this issue by unifying the procurement group creation at RMA confirmation, regardless of the RMA origin.

I propose you revert these changes and keep the fix of the method name in rma_sale,

what do you think?

[TheClaud99]

Hi @sbejaoui, I think it should be ok in your way as well (with also the advantage of having procurement.group creation unified), I can completely revert my commit 5f73bf6 and wait for your pr to be merged