diff --git a/src/backend/InvenTree/order/models.py b/src/backend/InvenTree/order/models.py index f83c9d7d55..506b03f571 100644 --- a/src/backend/InvenTree/order/models.py +++ b/src/backend/InvenTree/order/models.py @@ -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() diff --git a/src/backend/InvenTree/order/test_api.py b/src/backend/InvenTree/order/test_api.py index 99a70e534f..ced4b62076 100644 --- a/src/backend/InvenTree/order/test_api.py +++ b/src/backend/InvenTree/order/test_api.py @@ -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(