[FIX] repair,mrp_repair: add kit products to confirmed repair orders

[naja628]

Problem

When adding a kit manufactured product to a confirmed repair order,
an error is thrown upon saving, because we're working with a move
that was already deleted/exploded

Steps

• install repair,mrp
• create a repair order and confirm it
• add a kit manufactured product to it
• the error is thrown (Record already deleted)

Fix

When confirming moves, we explode them, so when we're adding directly to
the repair order, we need to make sure we take the new moves.

opw-3889564

---

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

[robodoo]

Pull request status dashboard.

[naja628]

Note:
When the repair order is not yet confirmed, the flow seems to go through here:

odoo/addons/repair/models/repair.py

Lines 542 to 554 in a9d2771

	def _action_repair_confirm(self):
	""" Repair order state is set to 'Confirmed'.
	@param *arg: Arguments
	@return: True
	"""
	repairs_to_confirm = self.filtered(lambda repair: repair.state == 'draft')
	repairs_to_confirm._check_company()
	repairs_to_confirm.move_ids._check_company()
	repairs_to_confirm.move_ids._adjust_procure_method()
	repairs_to_confirm.move_ids._action_confirm()
	repairs_to_confirm.move_ids._trigger_scheduler()
	repairs_to_confirm.write({'state': 'confirmed'})
	return True

and I'm not sure I fully understand why we don't have the "same" problem

[adwid]

I agree with the idea of the fix
Just few improvements :)

Note: PR's target is ok, the issue starts from 16.4, since the refactoring (eaa6e1a)

[adwid]

(Nitpicking)
In a lot of cases, the method will return the same move. So let's make the name simpler

Suggested change

	exploded = move._action_confirm()
	moves = move._action_confirm()

[adwid]

Why not something like ?

Suggested change

	repair_moves |= exploded
	else:
	repair_moves |= move
	repair_moves |= moves

Where moves is initiated before the if-block ?

[adwid]

Always better to work in batch

Suggested change

	repaired = self.env['product.product'].create({'name': 'Repaired'})
	part = self.env['product.product'].create({'name': 'Kit Component'})
	kit = self.env['product.product'].create({
	'name': 'Kit',
	'type': 'product',
	})
	repaired, part, kit = self.env['product.product'].create([
	{...},
	...
	])

[adwid]

Why not directly add the line while creating the BoM ?
We could then avoid a useless call to write
Also, be careful when dealing with 2many fields, you need to use some commands

Suggested change

	})
	bom.write({
	'bom_line_ids': [self.env['mrp.bom.line'].create({
	'bom_line_ids': [(0, 0, {

[adwid]

Suggested change

# create repair order and confirm it

The code is clear enough :)

[adwid]

Suggested change

# add kit

Same

[adwid]

We don't use try-except to test such use cases
Just do your calls directly in the test body, and if one call raises an error, the test will fail
Else, if there isn't any raised error, the test will pass
That being said, it's then a bit weird to have a test without any assert
So it's always worth adding at least one
Here for instance, you could check that the line has been replaced with a new line for the component

[adwid]

You're not using any lot here, so I don't get the reason why it would be useful ?

Suggested change

	@classmethod
	def setUpClass(cls):
	super().setUpClass()
	cls.env.ref('base.group_user').write({'implied_ids': [(4, cls.env.ref('stock.group_production_lot').id)]})

[adwid]

and I'm not sure I fully understand why we don't have the "same" problem

When we _action_confirm a SM and if it's a kit, we will unlink the SM and rather create/confirm the SM of the components (hence the error you had, and the fix you did). You don't have any issue in the method you mention because, after the confirm, we take the SM thanks to repairs_to_confirm.move_ids: if in the meantime, the moves have changed, you will have the new ones

[adwid]

Also, for the commit redaction, it's always better to explain why the issue occurs, instead of what the commit does as we can see that directly in the diff

See the guidelines and the section "Write a commit" in our documentation :)

So here, instead of

Fix

make sure we're working with the moves after they are exploded.

You can rather explain that, when confirming the SM and since it's a kit, we will explode it:

odoo/addons/mrp/models/stock_move.py

Lines 375 to 376 in 46931a3

	def _action_confirm(self, merge=True, merge_into=False):
	moves = self.action_explode()

And therefore, keeping a reference to the initial move is incorrect as it has been deleted during its confirmation

[adwid]

Few small things, then it should be ok
I think there is an error in the commit description "When confirming moves, we explode them, so when we're adding directly to the repair order, we need to make we take the new moves."

[adwid]

from odoo.fields import Command

[adwid]

Setting the template is useless, and imo setting the quantity would make it clearer

Suggested change

	'product_tmpl_id': part1.product_tmpl_id.id,
	'product_qty': 1.0,

[adwid]

Using the form is a bit weird, it's better to use the record

Also, it could be nice to also ensure that move_ids are based on the components: either by using assertRecordValues (and getting rid of the assertEqual), or by using another assertEqual on ro.move_ids.product_id.
That way, you can also remove the error message as the assert will be clear enough

[adwid]

Suggested change

	self.assertEqual(ro.move_ids.mapped('product_id'), part1 | part2)
	self.assertEqual(ro.move_ids.product_id, part1 | part2)

[adwid]

Nitpicking 🙈

Suggested change

	self.env['stock.move'].create({
	'repair_id': ro.id,
	'product_id': kit.id,
	'repair_line_type': 'add',
	})
	self.env['stock.move'].create({
	'repair_id': ro.id,
	'product_id': kit.id,
	'repair_line_type': 'add',
	})