[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 9787b8acbe0be5312a3bdce8bf17c05b5587d484 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.

[TheClaud99]

Sorry for my bad explanation, I try to be more clear. When the method _prepare_procurement_group_vals is called from here https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma/wizard/stock_picking_return.py#L83

the recordset in self is empty, so this condition will never be true because self.order_id will always be false: https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma_sale/models/rma.py#L167-L171 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 https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma/tests/test_rma.py#L679-L708

[pedrobaeza]

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

https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma/wizard/stock_picking_return.py#L83

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: https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma/wizard/stock_picking_return.py#L98-L112 https://github.com/OCA/rma/blob/3436ec67afb63e438252a043e35a51173548bf0b/rma/wizard/stock_picking_return.py#L130-L131 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()): https://github.com/OCA/rma/blob/09dd76a7a637f7a4c68945ff2bc5ce1098088649/rma/models/rma.py#L1121-L1124

I just also added this line https://github.com/OCA/rma/blob/910e73a124cdafa8ddb972ed8132524970025db7/rma/wizard/stock_picking_return.py#L128 to maintain the same logic as before for the group name https://github.com/OCA/rma/blob/09dd76a7a637f7a4c68945ff2bc5ce1098088649/rma/wizard/stock_picking_return.py#L101-L104

[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