Rework ExtendedURLValidator and tests

* Tidy implementation of validator
* Tests improved and moved to tests/integration (as they call resolve
  internally)

oscar/core/validators.py

@@ -9,50 +9,51 @@ from django.utils.translation import ugettext_lazy as _
 class ExtendedURLValidator(validators.URLValidator):
 
     def __init__(self, *args, **kwargs):
-        new_kwargs = kwargs.copy()
-        self.verify_exists = new_kwargs.pop('verify_exists', False)
-        super(ExtendedURLValidator, self).__init__(*args, **new_kwargs)
+        # 'verify_exists' has been removed in Django 1.5 and so we no longer
+        # pass it up to the core validator class
+        self.is_local_url = False
+        self.verify_exists = kwargs.pop('verify_exists', False)
+        super(ExtendedURLValidator, self).__init__(*args, **kwargs)
 
     def __call__(self, value):
         try:
             super(ExtendedURLValidator, self).__call__(value)
         except ValidationError:
-            if value:
+            # The parent validator will raise an exception if the URL does not
+            # exist and so we test here to see if the value is a local URL.
+            if self.verify_exists and value:
                 self.validate_local_url(value)
             else:
                 raise
 
     def validate_local_url(self, value):
-        """
-        Validate local URL name
-        """
+        value = self.clean_url(value)
         try:
-            value = self.fix_local_url(value)
-            if self.verify_exists:
-                resolve(value)
-            self.is_local_url = True
+            resolve(value)
         except Http404:
             # We load flatpages here as it causes a circular reference problem
             # sometimes.  FlatPages is None if not installed
             FlatPage = get_model('flatpages', 'FlatPage')
             if FlatPage is not None:
-                for page in FlatPage.objects.all().only(('url')):
-                    if value == page.url:
-                        return
+                try:
+                    FlatPage.objects.get(url=value)
+                except FlatPage.DoesNotExist:
+                    self.is_local_url = True
+                else:
+                    return
             raise ValidationError(_('Specified page does not exist'))
+        else:
+            self.is_local_url = True
 
-    def fix_local_url(self, value):
+    def clean_url(self, value):
         """
-        Puts preceding and trailing slashes to local URL name
+        Ensure url has a preceding slash and no query string
         """
         if value != '/':
             value = '/' + value.lstrip('/')
-
         q_index = value.find('?')
-
         if q_index > 0:
             value = value[:q_index]
-
         return value
 
 

tests/integration/core/validator_tests.py

@@ -0,0 +1,68 @@
+from django.test import TestCase
+from django.core.exceptions import ValidationError
+from django.contrib.flatpages.models import FlatPage
+
+from oscar.core.validators import ExtendedURLValidator
+from oscar.core.validators import URLDoesNotExistValidator
+
+
+class TestExtendedURLValidatorWithVerifications(TestCase):
+    """
+    ExtendedURLValidator with verify_exists=True
+    """
+
+    def setUp(self):
+        self.validator = ExtendedURLValidator(verify_exists=True)
+
+    def test_validates_local_url(self):
+        try:
+            self.validator('/')
+        except ValidationError:
+            self.fail('ExtendedURLValidator raised ValidationError'
+                      'unexpectedly!')
+
+    def test_validates_local_url_with_query_strings(self):
+        try:
+            self.validator('/?q=test')  # Query strings shouldn't affect validation
+        except ValidationError:
+            self.fail('ExtendedURLValidator raised ValidationError'
+                      'unexpectedly!')
+
+    def test_raises_validation_error_for_missing_urls(self):
+        with self.assertRaises(ValidationError):
+            self.validator('/invalid/')
+
+    def test_validates_urls_missing_preceding_slash(self):
+        try:
+            self.validator('catalogue/')
+        except ValidationError:
+            self.fail('ExtendedURLValidator raised ValidationError'
+                      'unexpectedly!')
+
+    def test_raises_validation_error_for_urls_without_trailing_slash(self):
+        with self.assertRaises(ValidationError):
+            self.validator('/catalogue')  # Missing the / is bad
+
+    def test_validates_flatpages_urls(self):
+        FlatPage.objects.create(title='test page', url='/test/page/')
+        try:
+            self.validator('/test/page/')
+        except ValidationError:
+            self.fail('ExtendedURLValidator raises ValidationError'
+                      'unexpectedly!')
+
+
+class TestExtendedURLValidatorWithoutVerifyExists(TestCase):
+    """
+    ExtendedURLValidator with verify_exists=False
+    """
+
+    def setUp(self):
+        self.validator = URLDoesNotExistValidator()
+
+    def test_raises_exception_for_local_urls(self):
+        self.assertRaises(ValidationError, self.validator, '/')
+
+    def test_raises_exception_for_flatpages(self):
+        FlatPage.objects.create(title='test page', url='/test/page/')
+        self.assertRaises(ValidationError, self.validator, '/test/page/')

tests/unit/core/loading_tests.py

@@ -1,17 +1,13 @@
 from os.path import dirname
 from django.test import TestCase
-from django.core.exceptions import ValidationError
 from django.conf import settings
-from django.contrib.flatpages.models import FlatPage
 from django.test.utils import override_settings
 
 import oscar
-from oscar.core.loading import (
-    import_module, AppNotFoundError, get_classes,
-    get_class, ClassNotFoundError)
-from oscar.core.validators import (
-    ExtendedURLValidator, URLDoesNotExistValidator)
 from tests import temporary_python_path
+from oscar.core.loading import (
+    import_module, AppNotFoundError,
+    get_classes, get_class, ClassNotFoundError)
 
 
 class TestImportModule(TestCase):
@@ -92,61 +88,6 @@ class ClassLoadingWithLocalOverrideTests(TestCase):
                 self.assertEqual('shipping.methods', Free.__module__)
 
 
-class TestExtendedURLValidator(TestCase):
-    """
-    ExtendedURLValidator
-    """
-
-    def test_validates_local_url(self):
-        v = ExtendedURLValidator(verify_exists=True)
-
-        try:
-            v('/')
-        except ValidationError:
-            self.fail('ExtendedURLValidator raised ValidationError'
-                      'unexpectedly!')
-
-        try:
-            v('/?q=test')  # Query strings shouldn't affect validation
-        except ValidationError:
-            self.fail('ExtendedURLValidator raised ValidationError'
-                      'unexpectedly!')
-
-        with self.assertRaises(ValidationError):
-            v('/invalid/')
-
-        with self.assertRaises(ValidationError):
-            v('/invalid/?q=test')  # Query strings shouldn't affect validation
-
-        try:
-            v('catalogue/')
-        except ValidationError:
-            self.fail('ExtendedURLValidator raised ValidationError'
-                      'unexpectedly!')
-
-        with self.assertRaises(ValidationError):
-            v('/catalogue')  # Missing the / is bad
-
-        FlatPage(title='test page', url='/test/page/').save()
-        try:
-            v('/test/page/')
-        except ValidationError:
-            self.fail('ExtendedURLValidator raises ValidationError'
-                      'unexpectedly!')
-
-    def test_raises_exception_for_missing_url(self):
-        validator = URLDoesNotExistValidator()
-        self.assertRaises(ValidationError, validator, '/')
-        try:
-            validator('/invalid/')
-        except ValidationError:
-            self.fail('URLDoesNotExistValidator raised ValidationError'
-                      'unexpectedly!')
-
-        FlatPage(title='test page', url='/test/page/').save()
-        self.assertRaises(ValidationError, validator, '/test/page/')
-
-
 class ClassLoadingWithLocalOverrideWithMultipleSegmentsTests(TestCase):
 
     def setUp(self):