[bug] Transfer order fixes (#12367)
* Handle failed transfer order transaction * Prevent duplicate completion of transfer order
This commit is contained in:
Oliver
GitHub
parent
32ca78f65c
commit
0b3ba6c4eb
|
|
@ -3628,6 +3628,12 @@ class TransferOrder(Order):
|
|||
"""
|
||||
user = kwargs.pop('user', None)
|
||||
|
||||
# Lock this order against concurrent completion, and re-read the status
|
||||
# from the database. Without this, two simultaneous completion requests
|
||||
# can both observe status=ISSUED, and each would process every allocation
|
||||
# (duplicating all associated stock operations).
|
||||
self.status = TransferOrder.objects.select_for_update().get(pk=self.pk).status
|
||||
|
||||
if not self.can_complete(raise_error=True, **kwargs):
|
||||
return False
|
||||
|
||||
|
|
@ -3946,6 +3952,10 @@ class TransferOrderAllocation(models.Model):
|
|||
- Move the StockItem to the new location
|
||||
- Updates the transferred qty
|
||||
- If order is marked as "consume", reduce quantity rather than move
|
||||
|
||||
Raises:
|
||||
ValidationError: If the stock operation fails - the 'transferred' quantity
|
||||
is only updated once the stock has actually been moved / consumed.
|
||||
"""
|
||||
order: TransferOrder = self.line.order
|
||||
self.item: stock.models.StockItem # for type hints
|
||||
|
|
@ -3956,35 +3966,56 @@ class TransferOrderAllocation(models.Model):
|
|||
# This means allocations to transfer orders don't affect "available" stock
|
||||
# (otherwise it would permanently reduce available stock)
|
||||
|
||||
# The stock item may have been reduced since the allocation was made,
|
||||
# so limit the transfer to the quantity which is actually available
|
||||
transfer_quantity = min(self.quantity, self.item.quantity)
|
||||
|
||||
if transfer_quantity <= 0:
|
||||
# Nothing available to transfer (e.g. the item has since been depleted)
|
||||
return
|
||||
|
||||
if order.consume:
|
||||
# rather than transferring the stock, we simply reduce its quantity to release it from tracked inventory
|
||||
# NOTE: if delete_on_deplete is enabled, this will result in the "transferred stock" panel being empty
|
||||
# after completion. A more sophesticated immutable tracking that doesn't rely on allocations
|
||||
# after completion. A more sophisticated immutable tracking that doesn't rely on allocations
|
||||
# would be helpful here
|
||||
self.item.take_stock(
|
||||
quantity=self.quantity,
|
||||
if not self.item.take_stock(
|
||||
quantity=transfer_quantity,
|
||||
user=user,
|
||||
code=StockHistoryCode.STOCK_REMOVE,
|
||||
transferorder=order,
|
||||
)
|
||||
else:
|
||||
if self.quantity < self.item.quantity:
|
||||
# update our own reference to the StockItem which was split
|
||||
self.item = self.item.splitStock(
|
||||
quantity=self.quantity,
|
||||
location=order.destination,
|
||||
user=user,
|
||||
transferorder=order,
|
||||
):
|
||||
raise ValidationError(
|
||||
_('Failed to consume stock item against transfer order')
|
||||
)
|
||||
self.save()
|
||||
else:
|
||||
# move item directly, we don't have to split
|
||||
self.item.move(
|
||||
location=order.destination, user=user, transferorder=order, notes=''
|
||||
elif transfer_quantity < self.item.quantity:
|
||||
new_item = self.item.splitStock(
|
||||
quantity=transfer_quantity,
|
||||
location=order.destination,
|
||||
user=user,
|
||||
transferorder=order,
|
||||
)
|
||||
|
||||
if new_item is None:
|
||||
raise ValidationError(
|
||||
_('Failed to transfer stock item against transfer order')
|
||||
)
|
||||
|
||||
# update our own reference to the StockItem which was split
|
||||
self.item = new_item
|
||||
self.save()
|
||||
else:
|
||||
# move item directly, we don't have to split
|
||||
if not self.item.move(
|
||||
location=order.destination, user=user, transferorder=order, notes=''
|
||||
):
|
||||
raise ValidationError(
|
||||
_('Failed to transfer stock item against transfer order')
|
||||
)
|
||||
|
||||
# Update the transferred qty
|
||||
self.line.transferred += self.quantity
|
||||
# Note: use the quantity which was *actually* transferred
|
||||
self.line.transferred += transfer_quantity
|
||||
self.line.save()
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -3541,6 +3541,158 @@ class TransferOrderTest(OrderTest):
|
|||
location=destination,
|
||||
)
|
||||
|
||||
def test_transfer_order_depleted_allocation(self):
|
||||
"""Completion handles allocations whose stock was reduced after allocation.
|
||||
|
||||
Regression test: the 'transferred' quantity used to be incremented by the
|
||||
*allocated* quantity even when less (or no) stock was actually moved.
|
||||
"""
|
||||
self.assignRole('transfer_order.add')
|
||||
destination = StockLocation.objects.first()
|
||||
|
||||
to = models.TransferOrder.objects.create(
|
||||
reference='TO-54321', description='Test TO', destination=destination
|
||||
)
|
||||
|
||||
part = Part.objects.exclude(virtual=True).first()
|
||||
|
||||
line_a = models.TransferOrderLineItem.objects.create(
|
||||
order=to, part=part, quantity=10
|
||||
)
|
||||
line_b = models.TransferOrderLineItem.objects.create(
|
||||
order=to, part=part, quantity=10
|
||||
)
|
||||
|
||||
url = reverse('api-transfer-order-issue', kwargs={'pk': to.pk})
|
||||
self.post(url, {}, expected_code=201)
|
||||
|
||||
item_a = StockItem.objects.create(part=part, quantity=10, batch='to-reduced')
|
||||
item_b = StockItem.objects.create(part=part, quantity=10, batch='to-depleted')
|
||||
|
||||
models.TransferOrderAllocation.objects.create(
|
||||
quantity=10, line=line_a, item=item_a
|
||||
)
|
||||
models.TransferOrderAllocation.objects.create(
|
||||
quantity=10, line=line_b, item=item_b
|
||||
)
|
||||
|
||||
# Reduce the available stock *after* the allocations have been made
|
||||
item_a.quantity = 6
|
||||
item_a.save()
|
||||
|
||||
item_b.quantity = 0
|
||||
item_b.save()
|
||||
|
||||
url = reverse('api-transfer-order-complete', kwargs={'pk': to.pk})
|
||||
self.post(url, {}, expected_code=201)
|
||||
|
||||
to.refresh_from_db()
|
||||
self.assertEqual(to.status, TransferOrderStatus.COMPLETE.value)
|
||||
|
||||
# Only the quantity which was actually available has been transferred
|
||||
line_a.refresh_from_db()
|
||||
item_a.refresh_from_db()
|
||||
self.assertEqual(line_a.transferred, 6)
|
||||
self.assertEqual(item_a.location, destination)
|
||||
self.assertEqual(item_a.quantity, 6)
|
||||
|
||||
# The depleted allocation was skipped, without error
|
||||
line_b.refresh_from_db()
|
||||
item_b.refresh_from_db()
|
||||
self.assertEqual(line_b.transferred, 0)
|
||||
self.assertIsNone(item_b.location)
|
||||
|
||||
def test_transfer_order_consume_depleted_allocation(self):
|
||||
"""Consume-type completion only records the quantity actually consumed."""
|
||||
self.assignRole('transfer_order.add')
|
||||
|
||||
to = models.TransferOrder.objects.create(
|
||||
reference='TO-54322', description='Test TO', consume=True
|
||||
)
|
||||
|
||||
part = Part.objects.exclude(virtual=True).first()
|
||||
|
||||
line = models.TransferOrderLineItem.objects.create(
|
||||
order=to, part=part, quantity=10
|
||||
)
|
||||
|
||||
url = reverse('api-transfer-order-issue', kwargs={'pk': to.pk})
|
||||
self.post(url, {}, expected_code=201)
|
||||
|
||||
item = StockItem.objects.create(
|
||||
part=part, quantity=100, batch='to-consume', delete_on_deplete=False
|
||||
)
|
||||
|
||||
models.TransferOrderAllocation.objects.create(quantity=10, line=line, item=item)
|
||||
|
||||
# Reduce the available stock *after* the allocation has been made
|
||||
item.quantity = 4
|
||||
item.save()
|
||||
|
||||
url = reverse('api-transfer-order-complete', kwargs={'pk': to.pk})
|
||||
self.post(url, {}, expected_code=201)
|
||||
|
||||
to.refresh_from_db()
|
||||
self.assertEqual(to.status, TransferOrderStatus.COMPLETE.value)
|
||||
|
||||
# Only the quantity which was actually available has been consumed
|
||||
line.refresh_from_db()
|
||||
item.refresh_from_db()
|
||||
self.assertEqual(line.transferred, 4)
|
||||
self.assertEqual(item.quantity, 0)
|
||||
|
||||
def test_transfer_order_complete_stale_instance(self):
|
||||
"""A second completion attempt with a stale instance must be rejected.
|
||||
|
||||
Regression test: _action_complete() only checked the *in-memory* status,
|
||||
so two concurrent completion requests (each holding its own instance of
|
||||
the order) could both observe status=ISSUED and each process every
|
||||
allocation - duplicating all stock movements and double-counting the
|
||||
'transferred' quantity. The status is now re-read (under lock) from the
|
||||
database before the allocations are processed.
|
||||
"""
|
||||
self.assignRole('transfer_order.add')
|
||||
destination = StockLocation.objects.first()
|
||||
|
||||
to = models.TransferOrder.objects.create(
|
||||
reference='TO-54323', description='Test TO', destination=destination
|
||||
)
|
||||
|
||||
part = Part.objects.exclude(virtual=True).first()
|
||||
|
||||
line = models.TransferOrderLineItem.objects.create(
|
||||
order=to, part=part, quantity=10
|
||||
)
|
||||
|
||||
to.issue_order()
|
||||
to.refresh_from_db()
|
||||
self.assertEqual(to.status, TransferOrderStatus.ISSUED.value)
|
||||
|
||||
item = StockItem.objects.create(part=part, quantity=10, batch='to-stale')
|
||||
models.TransferOrderAllocation.objects.create(quantity=10, line=line, item=item)
|
||||
|
||||
# Two "concurrent" requests each hold their own instance of the order
|
||||
instance_a = models.TransferOrder.objects.get(pk=to.pk)
|
||||
instance_b = models.TransferOrder.objects.get(pk=to.pk)
|
||||
|
||||
self.assertTrue(instance_a.complete_order(None))
|
||||
|
||||
line.refresh_from_db()
|
||||
self.assertEqual(line.transferred, 10)
|
||||
|
||||
# The second (stale) instance still believes the order is ISSUED,
|
||||
# but completion must be rejected based on the database state
|
||||
self.assertEqual(instance_b.status, TransferOrderStatus.ISSUED.value)
|
||||
|
||||
with self.assertRaises(ValidationError) as err:
|
||||
instance_b.complete_order(None)
|
||||
|
||||
self.assertIn('Order is already complete', str(err.exception))
|
||||
|
||||
# The transferred quantity has not been double-counted
|
||||
line.refresh_from_db()
|
||||
self.assertEqual(line.transferred, 10)
|
||||
|
||||
def test_output_options(self):
|
||||
"""Test the output options for the TransferOrder detail endpoint."""
|
||||
self.run_output_test(
|
||||
|
|
|
|||
Loading…
Reference in New Issue