From afbe3b3ab9be0548e4aecdb562f235d1fe645c70 Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 14 Jul 2026 16:00:26 +1000 Subject: [PATCH] Bug fixes (#12365) * Bug fixes: - Enforce serial number requirements when receiving items - Stricter merge checking - Fix count-into-location bug - Unit testing * Additional unit tests --- src/backend/InvenTree/order/serializers.py | 26 ++++++ src/backend/InvenTree/order/test_api.py | 37 +++++++++ src/backend/InvenTree/stock/models.py | 32 ++++++-- src/backend/InvenTree/stock/test_api.py | 95 ++++++++++++++++++++++ src/backend/InvenTree/stock/tests.py | 48 +++++++++++ 5 files changed, 231 insertions(+), 7 deletions(-) diff --git a/src/backend/InvenTree/order/serializers.py b/src/backend/InvenTree/order/serializers.py index 067d78a169..9fcabc1cff 100644 --- a/src/backend/InvenTree/order/serializers.py +++ b/src/backend/InvenTree/order/serializers.py @@ -1010,6 +1010,13 @@ class PurchaseOrderReceiveSerializer(serializers.Serializer): # Ensure barcodes are unique unique_barcodes = set() + # Ensure serial numbers are unique across all line items in this request + # (each line is only validated against the *database* individually) + unique_serials = set() + serials_globally_unique = get_global_setting( + 'SERIAL_NUMBER_GLOBALLY_UNIQUE', False + ) + # Check if the location is not specified for any particular item for item in items: line = item['line_item'] @@ -1035,6 +1042,25 @@ class PurchaseOrderReceiveSerializer(serializers.Serializer): else: unique_barcodes.add(barcode) + if serials := item.get('serials'): + if line.part: + # Scope uniqueness the same way as the database check: + # per part tree, or globally (if so configured) + for serial in serials: + key = ( + str(serial) + if serials_globally_unique + else (line.part.part.tree_id, str(serial)) + ) + + if key in unique_serials: + raise ValidationError( + _('Supplied serial numbers must be unique') + + f': {serial}' + ) + + unique_serials.add(key) + return data def save(self) -> list[stock.models.StockItem]: diff --git a/src/backend/InvenTree/order/test_api.py b/src/backend/InvenTree/order/test_api.py index 3942000de2..1012f41a68 100644 --- a/src/backend/InvenTree/order/test_api.py +++ b/src/backend/InvenTree/order/test_api.py @@ -1369,6 +1369,43 @@ class PurchaseOrderReceiveTest(OrderTest): self.assertEqual(item.quantity, 10) self.assertEqual(item.batch, 'B-xyz-789') + def test_duplicate_serial_numbers_across_items(self): + """Duplicate serial numbers across line items in a single request are rejected. + + Regression test: each line's serials used to be validated against the + database only, so two lines could claim the same serial number and create + duplicate serialized stock items. + """ + data = { + 'items': [ + {'line_item': 1, 'quantity': 3, 'serial_numbers': '100-102'}, + {'line_item': 1, 'quantity': 3, 'serial_numbers': '102-104'}, + ], + 'location': 1, + } + + # Serial 102 is claimed by both entries - request must be rejected + response = self.post(self.url, data, expected_code=400) + + self.assertIn('Supplied serial numbers must be unique', str(response.data)) + + # No new stock items have been created + self.assertEqual(self.n, StockItem.objects.count()) + + # Non-overlapping serial numbers are accepted + data['items'][1]['serial_numbers'] = '103-105' + + self.post(self.url, data, expected_code=201, max_query_count=250) + + self.assertEqual(self.n + 6, StockItem.objects.count()) + + for serial in range(100, 106): + self.assertEqual( + StockItem.objects.filter(serial=str(serial)).count(), + 1, + f'Expected exactly one stock item with serial {serial}', + ) + def test_receive_large_quantity(self): """Test receipt of a large number of items.""" from stock.status_codes import StockStatus diff --git a/src/backend/InvenTree/stock/models.py b/src/backend/InvenTree/stock/models.py index 3336c1c8d2..99e97c5b29 100644 --- a/src/backend/InvenTree/stock/models.py +++ b/src/backend/InvenTree/stock/models.py @@ -2491,6 +2491,11 @@ class StockItem( if location is None: return None + # This item must itself be in a state which allows merging + # (e.g. not serialized, in production, installed, or assigned to an order / customer) + if not self.can_merge(): + return None + candidates = list( StockItem.objects .filter(part=self.part, location=location) @@ -2567,10 +2572,14 @@ class StockItem( pricing_data.append([self.purchase_price, self.quantity]) for other in other_items: - # If the stock item cannot be merged, return - if not self.can_merge(other, raise_error=raise_error, **kwargs): + # Check the merge in both directions, so that the generic state checks + # (serialized, in production, installed, assigned to an order / customer) + # are applied to the incoming items as well as this one + if not self.can_merge( + other, raise_error=raise_error, **kwargs + ) or not other.can_merge(self, raise_error=raise_error, **kwargs): logger.warning( - 'Stock item <%s> could not be merge into <%s>', other.pk, self.pk + 'Stock item <%s> could not be merged into <%s>', other.pk, self.pk ) return @@ -3060,6 +3069,15 @@ class StockItem( status = self._resolve_status_kwarg(kwargs) self._apply_status_change(status, tracking_info) + + # Optional fields which can be supplied in a 'stocktake' call + self._apply_optional_transfer_fields(kwargs, tracking_info) + + # Have any fields (other than quantity) been updated? + # Must be determined *before* model reference deltas are extracted, + # as those only affect the tracking entry (not the model instance) + fields_updated = len(tracking_info) > 0 + self._apply_model_reference_fields(kwargs, tracking_info) quantity_updated = self.serialized or self.updateQuantity(count) @@ -3068,13 +3086,13 @@ class StockItem( # (self.quantity is updated by updateQuantity() even if the item was deleted) tracking_info['quantity'] = 1 if self.serialized else float(self.quantity) - if quantity_updated: + # Save if the quantity or any other field was changed. + # Note that updateQuantity() may have *deleted* the item (depleted to zero), + # in which case there is nothing left to save. + if self.pk and (quantity_updated or fields_updated): self.stocktake_date = InvenTree.helpers.current_date() self.stocktake_user = user - # Optional fields which can be supplied in a 'stocktake' call - self._apply_optional_transfer_fields(kwargs, tracking_info) - self.save(add_note=False) trigger_event( diff --git a/src/backend/InvenTree/stock/test_api.py b/src/backend/InvenTree/stock/test_api.py index 6fbcbd650b..2cad462ff4 100644 --- a/src/backend/InvenTree/stock/test_api.py +++ b/src/backend/InvenTree/stock/test_api.py @@ -2945,6 +2945,56 @@ class StocktakeTest(StockAPITestCase): str(response.data['location']), ) + def test_count_unchanged_quantity_saves_field_changes(self): + """Location / status changes are saved even if the counted quantity is unchanged. + + Regression test: counting an item to its *existing* quantity used to skip + the model save entirely, losing any location / status change while still + recording the change in stock history. + """ + item = StockItem.objects.get(pk=1234) + + quantity = item.quantity + self.assertEqual(item.location.pk, 5) + self.assertEqual(item.status, StockStatus.OK.value) + + # Clear any existing stocktake date so we can verify it gets set + item.stocktake_date = None + item.save() + + self.post( + reverse('api-stock-count'), + { + 'items': [ + { + 'pk': item.pk, + 'quantity': float(quantity), + 'status': StockStatus.DAMAGED.value, + } + ], + 'location': 1, + }, + expected_code=201, + ) + + item.refresh_from_db() + + # Quantity is unchanged, but the location and status changes must be saved + self.assertEqual(item.quantity, quantity) + self.assertEqual(item.location.pk, 1) + self.assertEqual(item.status, StockStatus.DAMAGED.value) + self.assertIsNotNone(item.stocktake_date) + + # The stock history entry must agree with the saved state + entry = StockItemTracking.objects.filter( + item=item, tracking_type=StockHistoryCode.STOCK_COUNT + ).latest('date') + self.assertEqual(entry.deltas.get('quantity'), float(quantity)) + self.assertEqual(entry.deltas.get('location'), 1) + self.assertEqual(entry.deltas.get('old_location'), 5) + self.assertEqual(entry.deltas.get('status'), StockStatus.DAMAGED.value) + self.assertEqual(entry.deltas.get('old_status'), StockStatus.OK.value) + def test_bulk_count_query_benchmark(self): """Benchmark: measure the number of DB queries required to count 100 stock items at once.""" InvenTreeSetting.set_setting('ENABLE_PLUGINS_EVENTS', True, change_user=None) @@ -3271,6 +3321,51 @@ class StockTransferMergeTest(StockAPITestCase): ).exists() ) + def test_transfer_merge_skips_protected_source(self): + """Merge-on-transfer must not absorb items in a protected state. + + Regression test: only the *target* item used to be validated, so a + transfer with merge=True could absorb (and delete) an in-production + build output. Such items must be moved as a separate lot instead. + """ + existing = StockItem.objects.create( + part=self.part, location=self.dest, quantity=100 + ) + + # An "in production" build output + self.part.assembly = True + self.part.save() + + bo = build.models.Build.objects.create( + reference='BO-9999', part=self.part, title='Merge test build', quantity=50 + ) + + building = StockItem.objects.create( + part=self.part, + location=self.source_loc, + quantity=50, + build=bo, + is_building=True, + ) + + self.post( + self.url, + { + 'items': [{'pk': building.pk, 'quantity': 50, 'merge': True}], + 'location': self.dest.pk, + }, + expected_code=201, + ) + + # The build output must survive - transferred as a separate lot instead + building.refresh_from_db() + self.assertTrue(building.is_building) + self.assertEqual(building.location, self.dest) + self.assertEqual(building.quantity, 50) + + existing.refresh_from_db() + self.assertEqual(existing.quantity, 100) + class StockItemDeletionTest(StockAPITestCase): """Tests for stock item deletion via the API.""" diff --git a/src/backend/InvenTree/stock/tests.py b/src/backend/InvenTree/stock/tests.py index dc186aeef3..6e49a81749 100644 --- a/src/backend/InvenTree/stock/tests.py +++ b/src/backend/InvenTree/stock/tests.py @@ -1074,6 +1074,54 @@ class StockTest(StockTestBase): # Final purchase price should be the weighted average self.assertAlmostEqual(s1.purchase_price.amount, 16.875, places=3) + def test_merge_protected_items(self): + """Stock items in a protected state cannot be absorbed by a merge. + + Regression test: merge_stock_items() used to run the generic state checks + (in production, assigned to customer, etc.) against the *target* item only, + allowing e.g. a build output to be merged away and deleted. + """ + part = Part.objects.first() + part.stock_items.all().delete() + + target = StockItem.objects.create(part=part, quantity=10) + + # The incoming item is "in production" (a build output) + part.assembly = True + part.save() + + bo = Build.objects.create( + reference='BO-9998', part=part, title='Merge test build', quantity=20 + ) + + building = StockItem.objects.create( + part=part, quantity=20, build=bo, is_building=True + ) + + # Without raise_error, the merge is refused silently + target.merge_stock_items([building]) + + target.refresh_from_db() + building.refresh_from_db() + + self.assertEqual(part.stock_items.count(), 2) + self.assertEqual(target.quantity, 10) + self.assertEqual(building.quantity, 20) + + # With raise_error, the merge raises a ValidationError + with self.assertRaises(ValidationError): + target.merge_stock_items([building], raise_error=True) + + # An item assigned to a customer is likewise protected + customer = Company.objects.create(name='MergeCust', is_customer=True) + assigned = StockItem.objects.create(part=part, quantity=5, customer=customer) + + target.merge_stock_items([assigned]) + + target.refresh_from_db() + self.assertEqual(target.quantity, 10) + self.assertTrue(StockItem.objects.filter(pk=assigned.pk).exists()) + def test_notify_low_stock(self): """Test that the 'notify_low_stock' task is triggered correctly.""" FUNC_NAME = 'part.tasks.notify_low_stock_if_required'