Short shipping multiple inventory units of the same line item mess the adjustments

[CallMeSH]

Hello everyone, I've noticed a weird behaviour of the Spree::OrderCancellations class that I would like to discuss with you.

Steps to reproduce

Create an order with multiple quantities of the same product.
You get as many inventory units as quantities you selected.
Then try to short ship all the inventory units of this line item (in the backend or directly with the service object, it doesn't matter).

Expected behavior

You supposedly get as many adjustments as there were inventory units and there value is equal to the price of the product.

Actual behavior

Unfortunately it all goes wrong due to a weird behaviour of ActiveRecord.
My research so far points out that
the compute_amount method of Spree::UnitCancel can't do its job properly because:

• The adjustment created in the adjust! method is ignored by ActiveRecord
• It is not accounted in the final_amount method of the line_item model
• The adjustment value calculated is now the original amount divided by a lesser quantity which makes the adjustment amount too high. (In some cases it can try to divide by 0)

This end up with an order completely messed up where you deduce more than the price of the original line item.

How to fix it

For now my easy fix is to just reload the line item in the compute_amount method of Spree::UnitCancel.
It does the trick as a quick fix but maybe you guys have a better idea ?

System configuration

Vanilla Solidus 1.4.
I didn't try to reproduce on 2.0 yet.

[jrochkind]

I'm not sure I understand why it's calculating based on line_item.inventory_units.not_canceled.reject(&:original_return_item ).size in the first place.

Is it trying to exclude cancelled items, becuase it expects previously cancelled items would have resulted in the LineItem#total being updated? Does that actually happen? I think maybe it doesn't?

Should this just be calculating based on a straight total/quantity? I'm not saying I'm sure, I'm just having trouble following what this is supposed to do, especially in cases where say one unit is cancelled, then a day later another unit is cancelled.