From d246169c9310db5cb5a4f84c5811bfd279c41d1e Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 13 Dec 2022 11:07:35 +1100 Subject: [PATCH] Add unit test for deleting part which has pricing information (#3986) * Add unit test for deleting part which has pricing information * Test delete of part without pricing * Deal with post_delete errors 1. Deleting a Part deleted related objects (e.g. InternalPriceBreak) 2. post_delete signal is called for InternalPriceBreak 3. Subsequent updates to Part / PartPricing throw error * Add unit test for deleting a Part via the API - Add InternalPriceBreak so the previous error condition is checked * Fix for unit test * More unit test fixes (cherry picked from commit fa346257f5fe87f8e9b64c211b0eed0536b2d231) * Ensure unique part names for unit testing * Further unit test fixes --- .gitignore | 1 + InvenTree/company/models.py | 4 +- InvenTree/order/models.py | 4 +- InvenTree/part/models.py | 54 +++++++++++++++++++++----- InvenTree/part/test_api.py | 69 +++++++++++++++++++++++----------- InvenTree/part/test_pricing.py | 41 ++++++++++++++++++++ 6 files changed, 138 insertions(+), 35 deletions(-) diff --git a/.gitignore b/.gitignore index be9c91ecb9..83fef7d272 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ dummy_image.* _tmp.csv inventree/label.pdf inventree/label.png +inventree/my_special* # Sphinx files docs/_build diff --git a/InvenTree/company/models.py b/InvenTree/company/models.py index 86463228f8..2693b13932 100644 --- a/InvenTree/company/models.py +++ b/InvenTree/company/models.py @@ -704,7 +704,7 @@ def after_save_supplier_price(sender, instance, created, **kwargs): if InvenTree.ready.canAppAccessDatabase() and not InvenTree.ready.isImportingData(): if instance.part and instance.part.part: - instance.part.part.pricing.schedule_for_update() + instance.part.part.schedule_pricing_update() @receiver(post_delete, sender=SupplierPriceBreak, dispatch_uid='post_delete_supplier_price_break') @@ -714,4 +714,4 @@ def after_delete_supplier_price(sender, instance, **kwargs): if InvenTree.ready.canAppAccessDatabase() and not InvenTree.ready.isImportingData(): if instance.part and instance.part.part: - instance.part.part.pricing.schedule_for_update() + instance.part.part.schedule_pricing_update() diff --git a/InvenTree/order/models.py b/InvenTree/order/models.py index 02614ffde7..c3f66d74a6 100644 --- a/InvenTree/order/models.py +++ b/InvenTree/order/models.py @@ -390,7 +390,7 @@ class PurchaseOrder(Order): # Schedule pricing update for any referenced parts for line in self.lines.all(): if line.part and line.part.part: - line.part.part.pricing.schedule_for_update() + line.part.part.schedule_pricing_update() trigger_event('purchaseorder.completed', id=self.pk) @@ -778,7 +778,7 @@ class SalesOrder(Order): # Schedule pricing update for any referenced parts for line in self.lines.all(): - line.part.pricing.schedule_for_update() + line.part.schedule_pricing_update() trigger_event('salesorder.completed', id=self.pk) diff --git a/InvenTree/part/models.py b/InvenTree/part/models.py index 4c77306fe0..3407e2ebf6 100644 --- a/InvenTree/part/models.py +++ b/InvenTree/part/models.py @@ -771,6 +771,10 @@ class Part(InvenTreeBarcodeMixin, MetadataMixin, MPTTModel): 'IPN': _('Duplicate IPN not allowed in part settings'), }) + # Ensure unique across (Name, revision, IPN) (as specified) + if Part.objects.exclude(pk=self.pk).filter(name=self.name, revision=self.revision, IPN=self.IPN).exists(): + raise ValidationError(_("Part with this Name, IPN and Revision already exists.")) + def clean(self): """Perform cleaning operations for the Part model. @@ -1699,6 +1703,20 @@ class Part(InvenTreeBarcodeMixin, MetadataMixin, MPTTModel): return pricing + def schedule_pricing_update(self): + """Helper function to schedule a pricing update. + + Importantly, catches any errors which may occur during deletion of related objects, + in particular due to post_delete signals. + + Ref: https://github.com/inventree/InvenTree/pull/3986 + """ + + try: + self.pricing.schedule_for_update() + except (PartPricing.DoesNotExist, IntegrityError): + pass + def get_price_info(self, quantity=1, buy=True, bom=True, internal=False): """Return a simplified pricing string for this part. @@ -2293,23 +2311,35 @@ class PartPricing(models.Model): def schedule_for_update(self, counter: int = 0): """Schedule this pricing to be updated""" - if self.pk is None: - self.save() + try: + self.refresh_from_db() + except (PartPricing.DoesNotExist, IntegrityError): + # Error thrown if this PartPricing instance has already been removed + return - self.refresh_from_db() + # Ensure that the referenced part still exists in the database + try: + p = self.part + p.refresh_from_db() + except IntegrityError: + return if self.scheduled_for_update: # Ignore if the pricing is already scheduled to be updated - logger.info(f"Pricing for {self.part} already scheduled for update - skipping") + logger.info(f"Pricing for {p} already scheduled for update - skipping") return if counter > 25: # Prevent infinite recursion / stack depth issues - logger.info(counter, f"Skipping pricing update for {self.part} - maximum depth exceeded") + logger.info(counter, f"Skipping pricing update for {p} - maximum depth exceeded") return - self.scheduled_for_update = True - self.save() + try: + self.scheduled_for_update = True + self.save() + except IntegrityError: + # An IntegrityError here likely indicates that the referenced part has already been deleted + return import part.tasks as part_tasks @@ -2372,7 +2402,11 @@ class PartPricing(models.Model): # Update the currency which was used to perform the calculation self.currency = currency_code_default() - self.update_overall_cost() + try: + self.update_overall_cost() + except IntegrityError: + # If something has happened to the Part model, might throw an error + pass super().save(*args, **kwargs) @@ -3557,7 +3591,7 @@ def update_pricing_after_edit(sender, instance, created, **kwargs): # Update part pricing *unless* we are importing data if InvenTree.ready.canAppAccessDatabase() and not InvenTree.ready.isImportingData(): - instance.part.pricing.schedule_for_update() + instance.part.schedule_pricing_update() @receiver(post_delete, sender=BomItem, dispatch_uid='post_delete_bom_item') @@ -3568,7 +3602,7 @@ def update_pricing_after_delete(sender, instance, **kwargs): # Update part pricing *unless* we are importing data if InvenTree.ready.canAppAccessDatabase() and not InvenTree.ready.isImportingData(): - instance.part.pricing.schedule_for_update() + instance.part.schedule_pricing_update() class BomItemSubstitute(models.Model): diff --git a/InvenTree/part/test_api.py b/InvenTree/part/test_api.py index 173e2fd2af..57a35775d8 100644 --- a/InvenTree/part/test_api.py +++ b/InvenTree/part/test_api.py @@ -126,7 +126,7 @@ class PartCategoryAPITest(InvenTreeAPITestCase): # Create parts in this category for jj in range(10): Part.objects.create( - name=f"Part xyz {jj}", + name=f"Part xyz {jj}_{ii}", description="A test part", category=child ) @@ -339,7 +339,7 @@ class PartCategoryAPITest(InvenTreeAPITestCase): # Create parts in the category to be deleted for jj in range(3): parts.append(Part.objects.create( - name=f"Part xyz {jj}", + name=f"Part xyz {i}_{jj}", description="Child part of the deleted category", category=cat_to_delete )) @@ -349,7 +349,7 @@ class PartCategoryAPITest(InvenTreeAPITestCase): # Create child categories under the category to be deleted for ii in range(3): child = PartCategory.objects.create( - name=f"Child parent_cat {ii}", + name=f"Child parent_cat {i}_{ii}", description="A child category of the deleted category", parent=cat_to_delete ) @@ -358,7 +358,7 @@ class PartCategoryAPITest(InvenTreeAPITestCase): # Create parts in the child categories for jj in range(3): child_categories_parts.append(Part.objects.create( - name=f"Part xyz {jj}", + name=f"Part xyz {i}_{jj}_{ii}", description="Child part in the child category of the deleted category", category=child )) @@ -881,11 +881,15 @@ class PartAPITest(InvenTreeAPITestCase): """ url = reverse('api-part-list') - response = self.post(url, { - 'name': 'all defaults', - 'description': 'my test part', - 'category': 1, - }) + response = self.post( + url, + { + 'name': 'all defaults', + 'description': 'my test part', + 'category': 1, + }, + expected_code=201, + ) data = response.data @@ -903,23 +907,31 @@ class PartAPITest(InvenTreeAPITestCase): self.user ) - response = self.post(url, { - 'name': 'all defaults', - 'description': 'my test part 2', - 'category': 1, - }) + response = self.post( + url, + { + 'name': 'all defaults 2', + 'description': 'my test part 2', + 'category': 1, + }, + expected_code=201, + ) # Part should now be purchaseable by default self.assertTrue(response.data['purchaseable']) # "default" values should not be used if the value is specified - response = self.post(url, { - 'name': 'all defaults', - 'description': 'my test part 2', - 'category': 1, - 'active': False, - 'purchaseable': False, - }) + response = self.post( + url, + { + 'name': 'all defaults 3', + 'description': 'my test part 3', + 'category': 1, + 'active': False, + 'purchaseable': False, + }, + expected_code=201 + ) self.assertFalse(response.data['active']) self.assertFalse(response.data['purchaseable']) @@ -2752,3 +2764,18 @@ class PartInternalPriceBreakTest(InvenTreeAPITestCase): round(Decimal(data['price']), 4), round(Decimal(p), 4) ) + + # Now, ensure that we can delete the Part via the API + # In particular this test checks that there are no circular post_delete relationships + # Ref: https://github.com/inventree/InvenTree/pull/3986 + + # First, ensure the part instance can be deleted + p = Part.objects.get(pk=1) + p.active = False + p.save() + + response = self.delete(reverse("api-part-detail", kwargs={"pk": 1})) + self.assertEqual(response.status_code, 204) + + with self.assertRaises(Part.DoesNotExist): + p.refresh_from_db() diff --git a/InvenTree/part/test_pricing.py b/InvenTree/part/test_pricing.py index fd974ef64f..9d903be9b0 100644 --- a/InvenTree/part/test_pricing.py +++ b/InvenTree/part/test_pricing.py @@ -328,3 +328,44 @@ class PartPricingTests(InvenTreeTestCase): self.assertEqual(pricing.purchase_cost_min, Money('1.333333', 'USD')) self.assertEqual(pricing.purchase_cost_max, Money('1.764706', 'USD')) + + def test_delete_with_pricing(self): + """Test for deleting a part which has pricing information""" + + # Create some pricing data + self.create_price_breaks() + + # Check that pricing does exist + pricing = self.part.pricing + + pricing.update_pricing() + pricing.save() + + self.assertIsNotNone(pricing.overall_min) + self.assertIsNotNone(pricing.overall_max) + + self.part.active = False + self.part.save() + + # Remove the part from the database + self.part.delete() + + # Check that the pricing was removed also + with self.assertRaises(part.models.PartPricing.DoesNotExist): + pricing.refresh_from_db() + + def test_delete_without_pricing(self): + """Test that we can delete a part which does not have pricing information""" + + pricing = self.part.pricing + + self.assertIsNone(pricing.pk) + + self.part.active = False + self.part.save() + + self.part.delete() + + # Check that part was actually deleted + with self.assertRaises(part.models.Part.DoesNotExist): + self.part.refresh_from_db()