Fix behaviour when deleting a StockLocation

Adds unit tests

InvenTree/stock/models.py

@@ -54,6 +54,35 @@ class StockLocation(InvenTreeTree):
     Stock locations can be heirarchical as required
     """
 
+    def delete(self, *args, **kwargs):
+        """
+        Custom model deletion routine, which updates any child locations or items.
+        This must be handled within a transaction.atomic(), otherwise the tree structure is damaged
+        """
+
+        with transaction.atomic():
+
+            parent = self.parent
+            tree_id = self.tree_id
+
+            # Update each part in the stock location
+            for item in self.stock_items.all():
+                item.location = self.parent
+                item.save()
+
+            # Update each child category
+            for child in self.children.all():
+                child.parent = self.parent
+                child.save()
+
+            super().delete(*args, **kwargs)
+
+            if parent is not None:
+                # Partially rebuild the tree (cheaper than a complete rebuild)
+                StockLocation.objects.partial_rebuild(tree_id)
+            else:
+                StockLocation.objects.rebuild()
+
     @staticmethod
     def get_api_url():
         return reverse('api-location-list')
@@ -159,20 +188,6 @@ class StockLocation(InvenTreeTree):
         return self.stock_item_count()
 
 
-@receiver(pre_delete, sender=StockLocation, dispatch_uid='stocklocation_delete_log')
-def before_delete_stock_location(sender, instance, using, **kwargs):
-
-    # Update each part in the stock location
-    for item in instance.stock_items.all():
-        item.location = instance.parent
-        item.save()
-
-    # Update each child category
-    for child in instance.children.all():
-        child.parent = instance.parent
-        child.save()
-
-
 class StockItemManager(TreeManager):
     """
     Custom database manager for the StockItem class.

InvenTree/stock/tests.py

@@ -524,6 +524,175 @@ class StockTest(TestCase):
         # Serialize the remainder of the stock
         item.serializeStock(2, [99, 100], self.user)
 
+    def test_location_tree(self):
+        """
+        Unit tests for stock location tree structure (MPTT).
+        Ensure that the MPTT structure is rebuilt correctly,
+        and the corrent ancestor tree is observed.
+
+        Ref: https://github.com/inventree/InvenTree/issues/2636
+        Ref: https://github.com/inventree/InvenTree/issues/2733
+        """
+
+        # First, we will create a stock location structure
+
+        A = StockLocation.objects.create(
+            name='A',
+            description='Top level location'
+        )
+
+        B1 = StockLocation.objects.create(
+            name='B1',
+            parent=A
+        )
+
+        B2 = StockLocation.objects.create(
+            name='B2',
+            parent=A
+        )
+
+        B3 = StockLocation.objects.create(
+            name='B3',
+            parent=A
+        )
+
+        C11 = StockLocation.objects.create(
+            name='C11',
+            parent=B1,
+        )
+
+        C12 = StockLocation.objects.create(
+            name='C12',
+            parent=B1,
+        )
+
+        C21 = StockLocation.objects.create(
+            name='C21',
+            parent=B2,
+        )
+
+        C22 = StockLocation.objects.create(
+            name='C22',
+            parent=B2,
+        )
+
+        C31 = StockLocation.objects.create(
+            name='C31',
+            parent=B3,
+        )
+
+        C32 = StockLocation.objects.create(
+            name='C32',
+            parent=B3
+        )
+
+        # Check that the tree_id is correct for each sublocation
+        for loc in [B1, B2, B3, C11, C12, C21, C22, C31, C32]:
+            self.assertEqual(loc.tree_id, A.tree_id)
+
+        # Check that the tree levels are correct for each node in the tree
+
+        self.assertEqual(A.level, 0)
+        self.assertEqual(A.get_ancestors().count(), 0)
+
+        for loc in [B1, B2, B3]:
+            self.assertEqual(loc.parent, A)
+            self.assertEqual(loc.level, 1)
+            self.assertEqual(loc.get_ancestors().count(), 1)
+
+        for loc in [C11, C12]:
+            self.assertEqual(loc.parent, B1)
+            self.assertEqual(loc.level, 2)
+            self.assertEqual(loc.get_ancestors().count(), 2)
+
+        for loc in [C21, C22]:
+            self.assertEqual(loc.parent, B2)
+            self.assertEqual(loc.level, 2)
+            self.assertEqual(loc.get_ancestors().count(), 2)
+
+        for loc in [C31, C32]:
+            self.assertEqual(loc.parent, B3)
+            self.assertEqual(loc.level, 2)
+            self.assertEqual(loc.get_ancestors().count(), 2)
+
+        # Spot-check for C32
+        ancestors = C32.get_ancestors(include_self=True)
+
+        self.assertEqual(ancestors[0], A)
+        self.assertEqual(ancestors[1], B3)
+        self.assertEqual(ancestors[2], C32)
+
+        # At this point, we are confident that the tree is correctly structured.
+
+        # Let's delete node B3 from the tree. We expect that:
+        # - C31 should move directly under A
+        # - C32 should move directly under A
+
+        # Add some stock items to B3
+        for i in range(10):
+            StockItem.objects.create(
+                part=Part.objects.get(pk=1),
+                quantity=10,
+                location=B3
+            )
+
+        self.assertEqual(StockItem.objects.filter(location=B3).count(), 10)
+        self.assertEqual(StockItem.objects.filter(location=A).count(), 0)
+
+        B3.delete()
+
+        A.refresh_from_db()
+        C31.refresh_from_db()
+        C32.refresh_from_db()
+
+        # Stock items have been moved to A
+        self.assertEqual(StockItem.objects.filter(location=A).count(), 10)
+
+        # Parent should be A
+        self.assertEqual(C31.parent, A)
+        self.assertEqual(C32.parent, A)
+
+        self.assertEqual(C31.tree_id, A.tree_id)
+        self.assertEqual(C31.level, 1)
+
+        self.assertEqual(C32.tree_id, A.tree_id)
+        self.assertEqual(C32.level, 1)
+
+        # Ancestor tree should be just A
+        ancestors = C31.get_ancestors()
+        self.assertEqual(ancestors.count(), 1)
+        self.assertEqual(ancestors[0], A)
+
+        ancestors = C32.get_ancestors()
+        self.assertEqual(ancestors.count(), 1)
+        self.assertEqual(ancestors[0], A)
+
+        # Delete A
+        A.delete()
+
+        # Stock items have been moved to top-level location
+        self.assertEqual(StockItem.objects.filter(location=None).count(), 10)
+
+        for loc in [B1, B2, C11, C12, C21, C22]:
+            loc.refresh_from_db()
+
+        self.assertEqual(B1.parent, None)
+        self.assertEqual(B2.parent, None)
+
+        self.assertEqual(C11.parent, B1)
+        self.assertEqual(C12.parent, B1)
+        self.assertEqual(C11.get_ancestors().count(), 1)
+        self.assertEqual(C12.get_ancestors().count(), 1)
+
+        self.assertEqual(C21.parent, B2)
+        self.assertEqual(C22.parent, B2)
+
+        ancestors = C21.get_ancestors()
+        print("C21 ancestors:", ancestors)
+
+        self.assertEqual(C21.get_ancestors().count(), 1)
+        self.assertEqual(C22.get_ancestors().count(), 1)
+
 
 class VariantTest(StockTest):
     """