Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[16.0][FIX] rma: same procurement group for reception and delivery #421

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from
+19 −28
Open

[16.0][FIX] rma: same procurement group for reception and delivery #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions rma/models/rma.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ def _prepare_reception_procurements(self):
group = rma.procurement_group_id
if not group:
group = group_model.create(rma._prepare_procurement_group_vals())
rma.procurement_group_id = group
procurements.append(
group_model.Procurement(
rma.product_id,
Expand Down Expand Up @@ -1111,12 +1112,9 @@ def _group_delivery_if_needed(self):
key=lambda rma: [rma._delivery_group_key()],
)
for _group, rmas in grouped_rmas:
rmas = (
self.browse()
.concat(*list(rmas))
.filtered(lambda rma: not rma.procurement_group_id)
)
if not rmas:
rmas = self.browse().concat(*list(rmas))
if not rmas or len(rmas.procurement_group_id) == 1:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if some rma in the group don't have a procurement group, then you don't give them one anymore. I don't think you need to change the test

# if the rmas already grouped, no need to create a new proc group
continue
proc_group = self.env["procurement.group"].create(
rmas._prepare_procurement_group_vals()
Expand Down
15 changes: 15 additions & 0 deletions rma/tests/test_rma.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,3 +848,18 @@ def test_autoconfirm_email(self):
)
self.assertTrue(rma.name in mail_receipt.subject)
self.assertTrue("products received" in mail_receipt.subject)

def test_same_procurement_group_for_reception_and_delivery(self):
rma = self._create_confirm_receive(self.partner, self.product, 10, self.rma_loc)
self.assertTrue(rma.procurement_group_id)
delivery_form = Form(
self.env["rma.delivery.wizard"].with_context(
active_ids=rma.ids,
rma_delivery_type="return",
)
)
delivery_form.product_uom_qty = 2
delivery_wizard = delivery_form.save()
delivery_wizard.action_deliver()
self.assertEqual(rma.delivery_move_ids.group_id, rma.procurement_group_id)
self.assertEqual(rma.delivery_move_ids.group_id, rma.reception_move_id.group_id)
12 changes: 0 additions & 12 deletions rma_sale/models/rma.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,3 @@ def _prepare_refund_line_vals(self):
):
vals["sale_line_ids"] = [(4, line.id)]
return vals

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

def _prepare_delivery_procurements(self, scheduled_date=None, qty=None, uom=None):
self = self.with_context(ignore_rma_sale_order=True)
return super()._prepare_delivery_procurements(
scheduled_date=scheduled_date, qty=qty, uom=uom
)
Comment on lines -166 to -177
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedrobaeza ,

I am not aware of the history behind this code, but I believe it was implemented this way to avoid linking the rma delivery pickings to the sales order, as in recent versions, this linkage creates a sales line if the procurement group is associated to a sale order without an existing line.

This approach has led to:

  • Duplication of procurement groups, one for the reception and another for the deliveries.
  • In the sales order, the RMA reception picking appears among the sales order pickings, but the delivery pickings do not.
  • This unclear usage of context keys.

I propose to completely cut the link between the RMA procurement group and the sales order, maintaining a single procurement group for all RMA pickings.

In #417, I am working on a different approach to establish this link between the RMA stock moves and the sales line, allowing standard refunding to be configurable based on the operation.

4 changes: 0 additions & 4 deletions rma_sale/tests/test_rma_sale.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ def test_create_rma_from_so(self):
rma.reception_move_id.origin_returned_move_id,
self.order_out_picking.move_ids,
)
self.assertEqual(
rma.reception_move_id.picking_id + self.order_out_picking,
order.picking_ids,
)
user = self.env["res.users"].create(
{"login": "test_refund_with_so", "name": "Test"}
)
Expand Down
6 changes: 0 additions & 6 deletions rma_sale_mrp/tests/test_rma_sale_mrp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,6 @@ def test_create_rma_from_so(self):
rma_2.mapped("reception_move_id.origin_returned_move_id"),
move_2,
)
self.assertEqual(
rmas.mapped("reception_move_id.picking_id")
+ self.order_out_picking
+ self.backorder,
order.picking_ids,
)
# Refund the RMA
user = self.env["res.users"].create(
{"login": "test_refund_with_so", "name": "Test"}
Expand Down
Loading