From d95416c5592c122685db1227e35bddc47be7dbe5 Mon Sep 17 00:00:00 2001 From: Oliver Date: Mon, 12 Dec 2022 09:25:25 +1100 Subject: [PATCH 1/3] form.is_valid (#4046) --- InvenTree/InvenTree/views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/InvenTree/InvenTree/views.py b/InvenTree/InvenTree/views.py index ecb89af275..13f098ee7d 100644 --- a/InvenTree/InvenTree/views.py +++ b/InvenTree/InvenTree/views.py @@ -642,6 +642,9 @@ class CustomLoginView(LoginView): # Initiate form form = self.get_form_class()(request.GET.dict(), request=request) + # Validate form data + form.is_valid() + # Try to login form.full_clean() return form.login(request) From 2f7be702874118ba8b310074f4aa59167966ddb7 Mon Sep 17 00:00:00 2001 From: Oliver Date: Mon, 12 Dec 2022 19:17:16 +1100 Subject: [PATCH 2/3] Append correct python path if not present (only during testing) (#4048) * Append correct python path if not present (only during testing) * Add a bunch more debug info --- .github/workflows/docker.yaml | 4 ++++ InvenTree/InvenTree/config.py | 7 +++++++ InvenTree/InvenTree/settings.py | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index 071e1d7d1b..05e068b6c9 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -21,6 +21,10 @@ on: branches: - 'master' + pull_request: + branches: + - 'master' + jobs: # Build the docker image diff --git a/InvenTree/InvenTree/config.py b/InvenTree/InvenTree/config.py index 27722435f3..fe9a4e4914 100644 --- a/InvenTree/InvenTree/config.py +++ b/InvenTree/InvenTree/config.py @@ -59,6 +59,13 @@ def get_config_file(create=True) -> Path: def load_config_data() -> map: """Load configuration data from the config file.""" + import sys + + print("load_config_data()") + print("- cwd:", os.getcwd()) + print("- exe:", sys.executable) + print("- path:", sys.path) + import yaml cfg_file = get_config_file() diff --git a/InvenTree/InvenTree/settings.py b/InvenTree/InvenTree/settings.py index d8c8e45983..bd8bba0756 100644 --- a/InvenTree/InvenTree/settings.py +++ b/InvenTree/InvenTree/settings.py @@ -31,6 +31,16 @@ INVENTREE_NEWS_URL = 'https://inventree.org/news/feed.atom' # Determine if we are running in "test" mode e.g. "manage.py test" TESTING = 'test' in sys.argv +# Note: The following fix is "required" for docker build workflow +# Note: 2022-12-12 still unsure why... +if TESTING: + # Ensure that sys.path includes global python libs + python_dir = os.path.dirname(sys.executable) + python_lib = os.path.join(python_dir, "lib", "site-packages") + + if python_lib not in sys.path: + sys.path.append(python_lib) + # Are environment variables manipulated by tests? Needs to be set by testing code TESTING_ENV = False From d246169c9310db5cb5a4f84c5811bfd279c41d1e Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 13 Dec 2022 11:07:35 +1100 Subject: [PATCH 3/3] 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()