From c62ba5eb1246eda9ecf29db33763fc39b43a3c95 Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 22 Jun 2021 10:09:19 +1000 Subject: [PATCH] Perform a "full_clean" on serialized model - DRF does not by deault run validate_unique on the model - Need to check if we are "creating" or "updating" a model - Catch and re-throw errors in the correct format - Unit tests --- InvenTree/InvenTree/serializers.py | 31 +++- InvenTree/part/test_api.py | 164 ++++++++++++++++++ InvenTree/part/test_part.py | 11 +- .../migrations/0061_auto_20210511_0911.py | 3 +- .../migrations/0064_auto_20210621_1724.py | 6 +- 5 files changed, 204 insertions(+), 11 deletions(-) diff --git a/InvenTree/InvenTree/serializers.py b/InvenTree/InvenTree/serializers.py index fa7674723c..00be9949f4 100644 --- a/InvenTree/InvenTree/serializers.py +++ b/InvenTree/InvenTree/serializers.py @@ -6,12 +6,15 @@ Serializers used in various InvenTree apps # -*- coding: utf-8 -*- from __future__ import unicode_literals -from rest_framework import serializers - import os from django.conf import settings from django.contrib.auth.models import User +from django.core.exceptions import ValidationError as DjangoValidationError + +from rest_framework import serializers +from rest_framework.fields import empty +from rest_framework.exceptions import ValidationError class UserSerializer(serializers.ModelSerializer): @@ -39,18 +42,34 @@ class InvenTreeModelSerializer(serializers.ModelSerializer): but also ensures that the underlying model class data are checked on validation. """ - def validate(self, data): + def run_validation(self, data=empty): """ Perform serializer validation. In addition to running validators on the serializer fields, this class ensures that the underlying model is also validated. """ # Run any native validation checks first (may throw an ValidationError) - data = super(serializers.ModelSerializer, self).validate(data) + data = super().run_validation(data) # Now ensure the underlying model is correct - instance = self.Meta.model(**data) - instance.clean() + + if not hasattr(self, 'instance') or self.instance is None: + # No instance exists (we are creating a new one) + instance = self.Meta.model(**data) + else: + # Instance already exists (we are updating!) + instance = self.instance + + # Update instance fields + for attr, value in data.items(): + setattr(instance, attr, value) + + # Run a 'full_clean' on the model. + # Note that by default, DRF does *not* perform full model validation! + try: + instance.full_clean() + except (ValidationError, DjangoValidationError) as exc: + raise ValidationError(detail=serializers.as_serializer_error(exc)) return data diff --git a/InvenTree/part/test_api.py b/InvenTree/part/test_api.py index 0f5f59d3a3..b7472a3b1c 100644 --- a/InvenTree/part/test_api.py +++ b/InvenTree/part/test_api.py @@ -1,3 +1,4 @@ +from django.db import models from rest_framework import status from django.urls import reverse @@ -293,6 +294,169 @@ class PartAPITest(InvenTreeAPITestCase): self.assertEqual(len(data['results']), n) +class PartDetailTests(InvenTreeAPITestCase): + """ + Test that we can create / edit / delete Part objects via the API + """ + + fixtures = [ + 'category', + 'part', + 'location', + 'bom', + 'test_templates', + ] + + roles = [ + 'part.change', + 'part.add', + 'part.delete', + 'part_category.change', + 'part_category.add', + ] + + def setUp(self): + super().setUp() + + def test_part_operations(self): + n = Part.objects.count() + + # Create a part + response = self.client.post( + reverse('api-part-list'), + { + 'name': 'my test api part', + 'description': 'a part created with the API', + 'category': 1, + } + ) + + self.assertEqual(response.status_code, 201) + + pk = response.data['pk'] + + # Check that a new part has been added + self.assertEqual(Part.objects.count(), n + 1) + + part = Part.objects.get(pk=pk) + + self.assertEqual(part.name, 'my test api part') + + # Edit the part + url = reverse('api-part-detail', kwargs={'pk': pk}) + + # Let's change the name of the part + + response = self.client.patch(url, { + 'name': 'a new better name', + }) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['pk'], pk) + self.assertEqual(response.data['name'], 'a new better name') + + part = Part.objects.get(pk=pk) + + # Name has been altered + self.assertEqual(part.name, 'a new better name') + + # Part count should not have changed + self.assertEqual(Part.objects.count(), n + 1) + + # Now, try to set the name to the *same* value + # 2021-06-22 this test is to check that the "duplicate part" checks don't do strange things + response = self.client.patch(url, { + 'name': 'a new better name', + }) + + self.assertEqual(response.status_code, 200) + + # Try to remove the part + response = self.client.delete(url) + + self.assertEqual(response.status_code, 204) + + # Part count should have reduced + self.assertEqual(Part.objects.count(), n) + + def test_duplicates(self): + """ + Check that trying to create 'duplicate' parts results in errors + """ + + # Create a part + response = self.client.post(reverse('api-part-list'), { + 'name': 'part', + 'description': 'description', + 'IPN': 'IPN-123', + 'category': 1, + 'revision': 'A', + }) + + self.assertEqual(response.status_code, 201) + + n = Part.objects.count() + + # Check that we cannot create a duplicate in a different category + response = self.client.post(reverse('api-part-list'), { + 'name': 'part', + 'description': 'description', + 'IPN': 'IPN-123', + 'category': 2, + 'revision': 'A', + }) + + self.assertEqual(response.status_code, 400) + + # Check that only 1 matching part exists + parts = Part.objects.filter( + name='part', + description='description', + IPN='IPN-123' + ) + + self.assertEqual(parts.count(), 1) + + # A new part should *not* have been created + self.assertEqual(Part.objects.count(), n) + + # But a different 'revision' *can* be created + response = self.client.post(reverse('api-part-list'), { + 'name': 'part', + 'description': 'description', + 'IPN': 'IPN-123', + 'category': 2, + 'revision': 'B', + }) + + self.assertEqual(response.status_code, 201) + self.assertEqual(Part.objects.count(), n + 1) + + # Now, check that we cannot *change* an existing part to conflict + pk = response.data['pk'] + + url = reverse('api-part-detail', kwargs={'pk': pk}) + + # Attempt to alter the revision code + response = self.client.patch(url, + { + 'revision': 'A', + }, + format='json', + ) + + self.assertEqual(response.status_code, 400) + + # But we *can* change it to a unique revision code + response = self.client.patch(url, + { + 'revision': 'C', + } + ) + + self.assertEqual(response.status_code, 200) + + class PartAPIAggregationTest(InvenTreeAPITestCase): """ Tests to ensure that the various aggregation annotations are working correctly... diff --git a/InvenTree/part/test_part.py b/InvenTree/part/test_part.py index bcae74a0be..cdbf19f485 100644 --- a/InvenTree/part/test_part.py +++ b/InvenTree/part/test_part.py @@ -109,13 +109,14 @@ class PartTest(TestCase): try: part.save() - assert(False) + self.assertTrue(False) except: pass self.assertEqual(Part.objects.count(), n + 1) - Part.objects.create( + # But we should be able to create a part with a different revision + part_2 = Part.objects.create( category=cat, name='part', description='description', @@ -125,6 +126,12 @@ class PartTest(TestCase): self.assertEqual(Part.objects.count(), n + 2) + # Now, check that we cannot *change* part_2 to conflict + part_2.revision = 'A' + + with self.assertRaises(ValidationError): + part_2.validate_unique() + def test_metadata(self): self.assertEqual(self.r1.name, 'R_2K2_0805') self.assertEqual(self.r1.get_absolute_url(), '/part/3/') diff --git a/InvenTree/stock/migrations/0061_auto_20210511_0911.py b/InvenTree/stock/migrations/0061_auto_20210511_0911.py index 0ab37250c8..ba0ecc5207 100644 --- a/InvenTree/stock/migrations/0061_auto_20210511_0911.py +++ b/InvenTree/stock/migrations/0061_auto_20210511_0911.py @@ -199,7 +199,8 @@ def update_history(apps, schema_editor): update_count += 1 - print(f"\n==========================\nUpdated {update_count} StockItemHistory entries") + if update_count > 0: + print(f"\n==========================\nUpdated {update_count} StockItemHistory entries") def reverse_update(apps, schema_editor): diff --git a/InvenTree/stock/migrations/0064_auto_20210621_1724.py b/InvenTree/stock/migrations/0064_auto_20210621_1724.py index 71314f366c..361ad02c80 100644 --- a/InvenTree/stock/migrations/0064_auto_20210621_1724.py +++ b/InvenTree/stock/migrations/0064_auto_20210621_1724.py @@ -26,7 +26,8 @@ def extract_purchase_price(apps, schema_editor): # Find all the StockItem objects without a purchase_price which point to a PurchaseOrder items = StockItem.objects.filter(purchase_price=None).exclude(purchase_order=None) - print(f"Found {items.count()} stock items with missing purchase price information") + if items.count() > 0: + print(f"Found {items.count()} stock items with missing purchase price information") update_count = 0 @@ -56,7 +57,8 @@ def extract_purchase_price(apps, schema_editor): break - print(f"Updated pricing for {update_count} stock items") + if update_count > 0: + print(f"Updated pricing for {update_count} stock items") def reverse_operation(apps, schema_editor): """