Fix URL character validation in dashboard PageUpdateForm.

Also fix the max_length on the url field. The underlying model field has
a max_length of 100, so it is invalid to have a larger value here.

DRY out dashboard page update/create views.

Refactor tests - perform tests of form logic directly rather than indirectly through view code.

Fixes #2559.

docs/source/releases/v1.6.rst

@@ -47,6 +47,9 @@ Minor changes
    initiation.
  - Added ``get_stock_info`` hook to ``oscar.apps.basket.models.Basket``  for
    implementing strategies that depend on product options.
+ - Fixed the page create/update views in the dashboard to correctly validate
+   URLs. Invalid characters in URLs will raise a validation error, as will
+   URLs longer than 100 characters.
 
 .. _incompatible_in_1.6:
 

src/oscar/apps/dashboard/pages/forms.py

@@ -22,8 +22,19 @@ class PageUpdateForm(forms.ModelForm):
     and *content* field. The specified URL will be validated and check if
     the same URL already exists in the system.
     """
-    url = forms.CharField(max_length=128, required=False, label=_("URL"),
-                          help_text=_("Example: '/about/contact/'."))
+    url = forms.RegexField(
+        label=_("URL"),
+        max_length=100,
+        regex=r'^[-\w/\.~]+$',
+        required=False,
+        help_text=_("Example: '/about/contact/'."),
+        error_messages={
+            "invalid": _(
+                "This value must contain only letters, numbers, dots, "
+                "underscores, dashes, slashes or tildes."
+            ),
+        },
+    )
 
     def clean_url(self):
         """

src/oscar/apps/dashboard/pages/views.py

@@ -63,12 +63,30 @@ class PageListView(ListView):
         return context
 
 
-class PageCreateView(generic.CreateView):
+class PageCreateUpdateMixin(object):
+
     template_name = 'dashboard/pages/update.html'
     model = FlatPage
     form_class = forms.PageUpdateForm
     context_object_name = 'page'
 
+    def get_success_url(self):
+        msg = render_to_string('oscar/dashboard/pages/messages/saved.html',
+                               {'page': self.object})
+        messages.success(self.request, msg, extra_tags='safe noicon')
+        return reverse('dashboard:page-list')
+
+    def form_valid(self, form):
+        # Ensure saved page is added to the current site
+        page = form.save()
+        if not page.sites.exists():
+            page.sites.add(Site.objects.get_current())
+        self.object = page
+        return HttpResponseRedirect(self.get_success_url())
+
+
+class PageCreateView(PageCreateUpdateMixin, generic.CreateView):
+
     def get_context_data(self, **kwargs):
         ctx = super(PageCreateView, self).get_context_data(**kwargs)
         ctx['title'] = _('Create New Page')
@@ -76,9 +94,8 @@ class PageCreateView(generic.CreateView):
 
     def form_valid(self, form):
         """
-        Store new flatpage from form data. Checks wether a site
-        is specified for the flatpage or sets the current site by
-        default. Additionally, if URL is left blank, a slugified
+        Store new flatpage from form data.
+        Additionally, if URL is left blank, a slugified
         version of the title will be used as URL after checking
         if it is valid.
         """
@@ -93,51 +110,22 @@ class PageCreateView(generic.CreateView):
         except ValidationError:
             pass
         else:
-            # use current site as default for new page
-            page.save()
-            page.sites.add(Site.objects.get_current())
-
-            return HttpResponseRedirect(self.get_success_url(page))
+            return super(PageCreateView, self).form_valid(form)
 
         ctx = self.get_context_data()
         ctx['form'] = form
         return self.render_to_response(ctx)
 
-    def get_success_url(self, page):
-        msg = render_to_string('oscar/dashboard/pages/messages/saved.html',
-                               {'page': page})
-        messages.success(self.request, msg, extra_tags='safe noicon')
-        return reverse('dashboard:page-list')
 
-
-class PageUpdateView(generic.UpdateView):
+class PageUpdateView(PageCreateUpdateMixin, generic.UpdateView):
     """
     View for updating flatpages from the dashboard.
     """
-    template_name = 'dashboard/pages/update.html'
-    model = FlatPage
-    form_class = forms.PageUpdateForm
-    context_object_name = 'page'
-
     def get_context_data(self, **kwargs):
         ctx = super(PageUpdateView, self).get_context_data(**kwargs)
         ctx['title'] = self.object.title
         return ctx
 
-    def form_valid(self, form):
-        # Ensure saved page is added to the current site
-        page = form.save(commit=False)
-        if not page.sites.exists():
-            page.sites.add(Site.objects.get_current())
-        page.save()
-        return HttpResponseRedirect(self.get_success_url())
-
-    def get_success_url(self):
-        msg = render_to_string('oscar/dashboard/pages/messages/saved.html',
-                               {'page': self.object})
-        messages.success(self.request, msg, extra_tags='safe noicon')
-        return reverse('dashboard:page-list')
-
 
 class PageDeleteView(generic.DeleteView):
     template_name = 'dashboard/pages/delete.html'

tests/functional/dashboard/test_page.py

@@ -1,5 +1,8 @@
 from django.core.urlresolvers import reverse
 from django.contrib.flatpages.models import FlatPage
+from django.test import TestCase
+
+from oscar.apps.dashboard.pages.forms import PageUpdateForm
 from oscar.test.testcases import WebTestCase
 
 
@@ -28,29 +31,13 @@ class TestPageDashboard(WebTestCase):
         self.assertTrue(self.flatpage_1 in objects)
         self.assertTrue(self.flatpage_2 in objects)
 
-    def test_doesnt_allow_existing_pages_to_be_clobbered(self):
-        self.assertEqual(FlatPage.objects.count(), 2)
-
-        page = self.get(reverse('dashboard:page-create'))
-        form = page.form
-        form['title'] = 'test'
-        form['url'] = '/dashboard/pages/'
-        response = form.submit()
-        self.assertEqual(200, response.status_code)
-        self.assertEqual("Specified page already exists!",
-                         response.context['form'].errors['url'][0])
-        self.assertEqual(FlatPage.objects.count(), 2)
-
-    def test_allows_page_to_be_created(self):
-        page = self.get(reverse('dashboard:page-create'))
-        form = page.form
-        form['title'] = 'test'
-        form['url'] = '/my-new-url/'
-        form['content'] = 'my content here'
-        response = form.submit()
+    def test_dashboard_delete_pages(self):
+        page = self.get(reverse('dashboard:page-list'))
+        delete_page = page.click(linkid="delete_page_%s" % self.flatpage_1.id)
+        response = delete_page.form.submit()
 
         self.assertIsRedirect(response)
-        self.assertEqual(FlatPage.objects.count(), 3)
+        self.assertEqual(FlatPage.objects.count(), 1)
 
     def test_dashboard_create_page_with_slugified_url(self):
         page = self.get(reverse('dashboard:page-create'))
@@ -60,53 +47,120 @@ class TestPageDashboard(WebTestCase):
         response = form.submit()
 
         self.assertIsRedirect(response)
-        self.assertEqual(FlatPage.objects.count(), 3)
 
-    def test_dashboard_create_page_with_exisiting_url_does_not_work(self):
+    def test_dashboard_create_page_with_duplicate_slugified_url_fails(self):
         page = self.get(reverse('dashboard:page-create'))
         form = page.form
-        form['title'] = 'test'
-        form['url'] = '/url1/'  # already exists
+        form['title'] = 'url1'  # This will slugify to url1
         form['content'] = 'my content here'
         response = form.submit()
 
         self.assertEqual(200, response.status_code)
-        self.assertEqual("Specified page already exists!",
-                         response.context['form'].errors['url'][0])
-        self.assertEqual(FlatPage.objects.count(), 2)
 
-    def test_dashboard_update_page_valid_url(self):
-        page = self.get(reverse('dashboard:page-update',
-                                kwargs={'pk': self.flatpage_1.pk}))
+    def test_default_site_added_for_new_pages(self):
+        page = self.get(reverse('dashboard:page-create'))
         form = page.form
         form['title'] = 'test'
-        form['url'] = '/new/url/'  # already exists
-        form['content'] = 'my content here'
-        response = form.submit()
+        form['url'] = '/hello-world/'
+        form.submit()
 
-        self.assertIsRedirect(response)
+        p = FlatPage.objects.get(url='/hello-world/')
+        self.assertEqual(p.sites.count(), 1)
 
-        page = FlatPage.objects.get(pk=self.flatpage_1.pk)
-        self.assertEqual(page.title, 'test')
-        self.assertEqual(page.url, '/new/url/')
-        self.assertEqual(page.content, "my content here")
-        self.assertEqual(page.sites.count(), 1)
 
-    def test_dashboard_update_page_invalid_url(self):
-        page = self.get(reverse('dashboard:page-update',
-                                kwargs={'pk': self.flatpage_1.pk}))
-        form = page.form
-        form['url'] = '/url2/'  # already exists
-        response = form.submit()
+class DashboardPageUpdateFormTestCase(TestCase):
 
-        self.assertEqual(200, response.status_code)
-        self.assertEqual("Specified page already exists!",
-                         response.context['form'].errors['url'][0])
+    def setUp(self):
+        self.flatpage_1 = FlatPage.objects.create(
+            title='title1', url='/url1/',
+            content='some content')
+        self.flatpage_2 = FlatPage.objects.create(
+            title='title2', url='/url2/',
+            content='other content')
 
-    def test_dashboard_delete_pages(self):
-        page = self.get(reverse('dashboard:page-list'))
-        delete_page = page.click(linkid="delete_page_%s" % self.flatpage_1.id)
-        response = delete_page.form.submit()
+    def test_doesnt_allow_existing_pages_to_be_clobbered(self):
+        form = PageUpdateForm(data={
+            'title': 'test',
+            'url': '/dashboard/pages/',
+        })
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors['url'],
+            ['Specified page already exists!']
+        )
 
-        self.assertIsRedirect(response)
-        self.assertEqual(FlatPage.objects.count(), 1)
+    def test_allows_page_to_be_created(self):
+        form = PageUpdateForm(data={
+            'title': 'test',
+            'url': '/my-new-url/',
+            'content': 'my content here'
+        })
+
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(FlatPage.objects.count(), 3)
+
+    def test_create_page_with_slugified_url(self):
+        form = PageUpdateForm(data={
+            'title': 'test',
+            'content': 'my content here'
+        })
+
+        self.assertTrue(form.is_valid())
+        form.save()
+        self.assertEqual(FlatPage.objects.count(), 3)
+
+    def test_create_page_with_existing_url_does_not_work(self):
+        form = PageUpdateForm(data={
+            'title': 'test',
+            'url': '/url1/',  # already exists
+            'content': 'my content here'
+        })
+
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors['url'],
+            ['Specified page already exists!']
+        )
+
+    def test_update_page_valid_url(self):
+        form = PageUpdateForm(instance=self.flatpage_1, data={
+            'title': 'test',
+            'url': '/new/url/',
+            'content': 'my content here'
+        })
+
+        form.save()
+
+        self.flatpage_1.refresh_from_db()
+        page = self.flatpage_1
+        self.assertEqual(page.title, 'test')
+        self.assertEqual(page.url, '/new/url/')
+        self.assertEqual(page.content, "my content here")
+
+    def test_invalid_chars_in_url(self):
+        form = PageUpdateForm(data={
+            'url': '/%* /',
+            'title': 'Title',
+            'content': 'Content',
+        })
+
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors['url'],
+            ['This value must contain only letters, numbers, dots, underscores, dashes, slashes or tildes.']
+        )
+
+    def test_invalid_url_length(self):
+        form = PageUpdateForm(data={
+            'url': '/this_url_is_more_than_100_characters_long_which_is_invalid'
+                   '_because_the_model_field_has_a_max_length_of_100',
+            'title': 'Title',
+            'content': 'Content',
+        })
+
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors['url'],
+            ['Ensure this value has at most 100 characters (it has 107).']
+        )