[16.0][FIX] sale_order_lot_selection: Fixed, that lot_id only gets propagat…

[baguenth]

…ed to stock moves, if lot_id is actually filled

[OCA-git-bot]

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

[baguenth]

Hey @Kev-Roche,

I have discovered some issues with this module after this pull request #3057 was integrated.

My scenario is:

• Sale order line doesn't have a lot id
• Related stock moves get a restrict_lot_id independently from the sale order line
• Sale order is changed and saved, lot_id was not changed but is in the vals for write function (it's still false)

Since the sale order line propagates the lot_id, no matter if it is filled or not, we try to set restrict_lot_id = False on stock moves which have a restrict_lot_id and are may already done. This is the moment, where the validation in stock_restrict_lot kicks in.

https://github.com/OCA/stock-logistics-workflow/blob/16.0/stock_restrict_lot/models/stock_move.py#L147

I am not exactly sure if my fix is what we want and if it is complete.

I think in general we should question:

• Should we propagate empty lot_id to move ids
• Should we propagate lot_id to move ids, regardless of the move ids state

@bodedra Maybe also interesting for you.

[rousseldenis]

@baguenth Your fix is not correct.

In vals.get("lot_id") you'll get an... id. But you want to compare it with a dict.

[baguenth]

@rousseldenis Sorry, my bad. 🙈

[baguenth]

Even though this will fix the issue, I am wondering if we need a more sophisticated check like:

    def write(self, vals):
        # Capture original lot ids to detect actual changes per record
        original_lots = {rec.id: rec.lot_id.id for rec in self}
        res = super().write(vals)
        if "lot_id" not in vals:
            return res
        allow_to_change_lot = self.env.company.allow_to_change_lot_on_confirmed_so
        for line in self:
            old_lot = original_lots.get(line.id)
            new_lot = line.lot_id.id
            # Only act if there is an actual change
            if new_lot == old_lot:
                continue
            if allow_to_change_lot or line.order_id.state not in ["sale", "done"]:
                # Propagate the new lot restriction to related stock moves
                line.move_ids.write({"restrict_lot_id": new_lot})
            else:
                # Disallow changing the lot on confirmed SO if company setting forbids it
                raise UserError(_("You can't change the lot on confirmed sale order."))
        return res

With this we only do something, if the lot has actually changed for a sale order line.

[rousseldenis]

@baguenth Could you add a test to avoid regression ? Thanks

[baguenth]

@rousseldenis I have update the pr with a more general solution.

We now check, if the lot has actually changed, before propagating it to the stock moves. This doesn't lead to a functional change and avoids unnecessary propagations of lots to related stock moves.

[baguenth]

Hey @rousseldenis and @bodedra,

kindly asking for (another) review.

Thanks :)

[baguenth]

Someone? :) @rousseldenis @bodedra @Kev-Roche

[baguenth]

Hey @pedrobaeza,

I saw you are also active on this module. Can I may ask you for a review / merge? My PR is stuck.

[pedrobaeza]

@rousseldenis should confirm this due to his "Requested changes". You should also squash the commits into one.

[baguenth]

@pedrobaeza I unfortunately cannot squash the commits, since the source branch is a production branch. I would need to recreate the pull request from a new source branch.

[pedrobaeza]

Not really. You can rebase interactively and do the fixup, but if not able to, just open another PR.

[baguenth]

FYI @rousseldenis @pedrobaeza

Closing the PR in favor of #4029.

There all commits have been squashed.