Refactor product attribute validation

* The get_validator method was dropped. Elsewhere we use the explicit
  lookup by type, so why not there? It only makes adding custom
  validation for e.g. richt text values more verbose, as both adding a
  validation method and overriding get_validator is needed.
  get_validator() was not used elsewhere in the codebase.
* The unused is_value_valid (used for option values) has been dropped.
  The same check is already done in _validate_option.

docs/source/releases/v0.8.rst

@@ -315,6 +315,10 @@ Misc
 
   After: ``{% basket_form request product 'single' as basket_form %}``
 
+* Product attribute validation has been cleaned up. As part of that, the
+  trivial ``ProductAttribute.get_validator`` and the unused
+  ``ProductAttribute.is_value_valid`` methods have been removed.
+
 .. _rewritten: https://github.com/tangentlabs/django-oscar/commit/d8b4dbfed17be90846ea4bc47b5f7b39ad944c24
 
 Basket line stockrecords

oscar/apps/catalogue/abstract_models.py

@@ -643,6 +643,7 @@ class AbstractProductAttribute(models.Model):
     def _validate_text(self, value):
         if not isinstance(value, six.string_types):
             raise ValidationError(_("Must be str or unicode"))
+    _validate_richtext = _validate_text
 
     def _validate_float(self, value):
         try:
@@ -680,8 +681,8 @@ class AbstractProductAttribute(models.Model):
                 _("Must be an AttributeOption model object instance"))
         if not value.pk:
             raise ValidationError(_("AttributeOption has not been saved yet"))
-        valid_values = self.option_group.options.values_list('option',
-                                                             flat=True)
+        valid_values = self.option_group.options.values_list(
+            'option', flat=True)
         if value.option not in valid_values:
             raise ValidationError(
                 _("%(enum)s is not a valid choice for %(attr)s") %
@@ -690,40 +691,27 @@ class AbstractProductAttribute(models.Model):
     def _validate_file(self, value):
         if value and not isinstance(value, File):
             raise ValidationError(_("Must be a file field"))
+    _validate_image = _validate_file
 
-    def get_validator(self):
-        DATATYPE_VALIDATORS = {
-            'text': self._validate_text,
-            'integer': self._validate_int,
-            'boolean': self._validate_bool,
-            'float': self._validate_float,
-            'richtext': self._validate_text,
-            'date': self._validate_date,
-            'entity': self._validate_entity,
-            'option': self._validate_option,
-            'file': self._validate_file,
-            'image': self._validate_file,
-        }
-
-        return DATATYPE_VALIDATORS[self.type]
+    def validate_value(self, value):
+        validator = getattr(self, '_validate_%s' % self.type)
+        validator(value)
 
     def __unicode__(self):
         return self.name
 
-    def save(self, *args, **kwargs):
-        super(AbstractProductAttribute, self).save(*args, **kwargs)
-
     def save_value(self, product, value):
+        ProductAttributeValue = get_model('catalogue', 'ProductAttributeValue')
         try:
             value_obj = product.attribute_values.get(attribute=self)
-        except get_model('catalogue', 'ProductAttributeValue').DoesNotExist:
-            # FileField uses False for anouncing deletion of the file
+        except ProductAttributeValue.DoesNotExist:
+            # FileField uses False for announcing deletion of the file
             # not creating a new value
             delete_file = self.is_file and value is False
             if value is None or value == '' or delete_file:
                 return
-            model = get_model('catalogue', 'ProductAttributeValue')
-            value_obj = model.objects.create(product=product, attribute=self)
+            value_obj = ProductAttributeValue.objects.create(
+                product=product, attribute=self)
 
         if self.is_file:
             # File fields in Django are treated differently, see
@@ -746,19 +734,6 @@ class AbstractProductAttribute(models.Model):
                 value_obj.value = value
                 value_obj.save()
 
-    def validate_value(self, value):
-        self.get_validator()(value)
-
-    def is_value_valid(self, value):
-        """
-        Check whether the passed value is valid for this attribute
-        """
-        if self.type == 'option':
-            valid_values = self.option_group.options.values_list('option',
-                                                                 flat=True)
-            return value in valid_values
-        return True
-
 
 class AbstractProductAttributeValue(models.Model):
     """

tests/integration/catalogue/product_tests.py

@@ -120,10 +120,10 @@ class ProductAttributeCreationTests(TestCase):
         pa = factories.ProductAttribute(
             type='option', option_group=option_group)
 
-        self.assertRaises(ValidationError, pa.get_validator(), 'invalid')
-        pa.get_validator()(option_1)
-        pa.get_validator()(option_2)
+        self.assertRaises(ValidationError, pa.validate_value, 'invalid')
+        pa.validate_value(option_1)
+        pa.validate_value(option_2)
 
         invalid_option = AttributeOption(option='invalid option')
         self.assertRaises(
-            ValidationError, pa.get_validator(), invalid_option)
+            ValidationError, pa.validate_value, invalid_option)