Display non_field_errors from the page editor form as part of the header error message

Fixes #536

wagtail/tests/testapp/models.py

@@ -7,6 +7,7 @@ import os
 from django.conf import settings
 from django.contrib.contenttypes.fields import GenericForeignKey
 from django.contrib.contenttypes.models import ContentType
+from django.core.exceptions import ValidationError
 from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
 from django.core.serializers.json import DjangoJSONEncoder
 from django.db import models
@@ -205,6 +206,22 @@ class EventCategory(models.Model):
         return self.name
 
 
+# Override the standard WagtailAdminPageForm to add validation on start/end dates
+# that appears as a non-field error
+
+class EventPageForm(WagtailAdminPageForm):
+    def clean(self):
+        cleaned_data = super(EventPageForm, self).clean()
+
+        # Make sure that the event starts before it ends
+        start_date = cleaned_data['date_from']
+        end_date = cleaned_data['date_to']
+        if start_date and end_date and start_date > end_date:
+            raise ValidationError('The end date must be after the start date')
+
+        return cleaned_data
+
+
 class EventPage(Page):
     date_from = models.DateField("Start date", null=True)
     date_to = models.DateField(
@@ -236,6 +253,7 @@ class EventPage(Page):
     ]
 
     password_required_template = 'tests/event_page_password_required.html'
+    base_form_class = EventPageForm
 
 
 EventPage.content_panels = [

wagtail/wagtailadmin/messages.py

@@ -1,7 +1,9 @@
 from __future__ import absolute_import, unicode_literals
 
 from django.contrib import messages
+from django.core.exceptions import NON_FIELD_ERRORS
 from django.template.loader import render_to_string
+from django.utils.html import format_html, format_html_join
 
 
 def render(message, buttons):
@@ -31,5 +33,32 @@ def error(request, message, buttons=None):
     return messages.error(request, render(message, buttons))
 
 
+def validation_error(request, message, form, buttons=None):
+    if not form.non_field_errors():
+        # just output the generic "there were validation errors" message, and leave
+        # the per-field highlighting to do the rest
+        full_message = message
+    else:
+        # display the full list of field and non-field validation errors
+        all_errors = []
+        for field_name, errors in form.errors.items():
+            if field_name == NON_FIELD_ERRORS:
+                prefix = ''
+            else:
+                try:
+                    field_label = form[field_name].label
+                except KeyError:
+                    field_label = field_name
+                prefix = "%s: " % field_label
+
+            for error in errors:
+                all_errors.append(prefix + error)
+
+        errors_html = format_html_join('\n', '<li>{}</li>', ((e,) for e in all_errors))
+        full_message = format_html("""{} <ul class="errorlist">{}</ul>""", message, errors_html)
+
+    return messages.error(request, render(full_message, buttons))
+
+
 def button(url, text, new_window=False):
     return url, text, new_window

wagtail/wagtailadmin/static_src/wagtailadmin/scss/_mixins.scss

@@ -30,6 +30,22 @@
     }
 }
 
+@mixin unlistimmediate() {
+    /* remove list styles, but only for the immediate element - allow nested lists inside it
+    to keep the default style */
+
+    margin-top: 0;
+    margin-bottom: 0;
+    padding-left: 0;
+    list-style-type: none;
+    font-style: normal;
+
+    > li {
+        list-style-type: none;
+        font-style: normal;
+    }
+}
+
 @mixin transition($transition...) {
     body.ready & {
         transition: $transition;

wagtail/wagtailadmin/static_src/wagtailadmin/scss/components/_messages.scss

@@ -12,14 +12,14 @@
         margin-left: 1em;
     }
 
-    ul {
-        @include unlist();
+    > ul {
+        @include unlistimmediate();
         position: relative;
         top: -100px;
         opacity: 0;
     }
 
-    li {
+    > ul > li {
         // @include nice-padding;
         padding-top: 1.6em;
         padding-right: 3em;
@@ -28,7 +28,7 @@
         color: $color-white;
     }
 
-    li:before {
+    > ul > li:before {
         margin-right: 0.5em;
         font-size: 1.5em;
         vertical-align: middle;
@@ -76,22 +76,26 @@
             color: $color-white;
         }
     }
+
+    .errorlist {
+        margin: 0.5em 0 0 1em;
+    }
 }
 
-.messages.new ul {
+.messages.new > ul {
     transition: none;
     top: -100px;
 }
 
-.ready .messages ul,
-.messages.appear ul {
+.ready .messages > ul,
+.messages.appear > ul {
     transition: top 0.5s ease, opacity 0.5s ease, max-height 1.2s ease;
     opacity: 1;
     top: 0;
 }
 
 @media screen and (min-width: $breakpoint-mobile) {
-    .messages ul li {
+    .messages > ul > li {
         padding-left: 1.6em;
         padding-right: 3em;
     }

wagtail/wagtailadmin/tests/test_pages_views.py

@@ -3953,3 +3953,118 @@ class TestParentalM2M(TestCase, WagtailTestUtils):
         self.assertEqual(2, updated_page.categories.count())
         self.assertIn(self.holiday_category, updated_page.categories.all())
         self.assertIn(self.men_with_beards_category, updated_page.categories.all())
+
+
+class TestValidationErrorMessages(TestCase, WagtailTestUtils):
+    fixtures = ['test.json']
+
+    def setUp(self):
+        self.events_index = Page.objects.get(url_path='/home/events/')
+        self.christmas_page = Page.objects.get(url_path='/home/events/christmas/')
+        self.user = self.login()
+
+    def test_field_error(self):
+        """Field errors should be shown against the relevant fields, not in the header message"""
+        post_data = {
+            'title': "",
+            'date_from': "2017-12-25",
+            'slug': "christmas",
+            'audience': "public",
+            'location': "The North Pole",
+            'cost': "Free",
+            'carousel_items-TOTAL_FORMS': 0,
+            'carousel_items-INITIAL_FORMS': 0,
+            'carousel_items-MIN_NUM_FORMS': 0,
+            'carousel_items-MAX_NUM_FORMS': 0,
+            'speakers-TOTAL_FORMS': 0,
+            'speakers-INITIAL_FORMS': 0,
+            'speakers-MIN_NUM_FORMS': 0,
+            'speakers-MAX_NUM_FORMS': 0,
+            'related_links-TOTAL_FORMS': 0,
+            'related_links-INITIAL_FORMS': 0,
+            'related_links-MIN_NUM_FORMS': 0,
+            'related_links-MAX_NUM_FORMS': 0,
+        }
+        response = self.client.post(
+            reverse('wagtailadmin_pages:edit', args=(self.christmas_page.id, )),
+            post_data
+        )
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, "The page could not be saved due to validation errors")
+        # the error should only appear once: against the field, not in the header message
+        self.assertContains(response, """<p class="error-message"><span>This field is required.</span></p>""", count=1, html=True)
+        self.assertContains(response, "This field is required", count=1)
+
+    def test_non_field_error(self):
+        """Non-field errors should be shown in the header message"""
+        post_data = {
+            'title': "Christmas",
+            'date_from': "2017-12-25",
+            'date_to': "2017-12-24",
+            'slug': "christmas",
+            'audience': "public",
+            'location': "The North Pole",
+            'cost': "Free",
+            'carousel_items-TOTAL_FORMS': 0,
+            'carousel_items-INITIAL_FORMS': 0,
+            'carousel_items-MIN_NUM_FORMS': 0,
+            'carousel_items-MAX_NUM_FORMS': 0,
+            'speakers-TOTAL_FORMS': 0,
+            'speakers-INITIAL_FORMS': 0,
+            'speakers-MIN_NUM_FORMS': 0,
+            'speakers-MAX_NUM_FORMS': 0,
+            'related_links-TOTAL_FORMS': 0,
+            'related_links-INITIAL_FORMS': 0,
+            'related_links-MIN_NUM_FORMS': 0,
+            'related_links-MAX_NUM_FORMS': 0,
+        }
+        response = self.client.post(
+            reverse('wagtailadmin_pages:edit', args=(self.christmas_page.id, )),
+            post_data
+        )
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, "The page could not be saved due to validation errors")
+        self.assertContains(response, "<li>The end date must be after the start date</li>", count=1)
+
+    def test_field_and_non_field_error(self):
+        """
+        If both field and non-field errors exist, all errors should be shown in the header message
+        with appropriate context to identify the field; and field errors should also be shown
+        against the relevant fields.
+        """
+        post_data = {
+            'title': "",
+            'date_from': "2017-12-25",
+            'date_to': "2017-12-24",
+            'slug': "christmas",
+            'audience': "public",
+            'location': "The North Pole",
+            'cost': "Free",
+            'carousel_items-TOTAL_FORMS': 0,
+            'carousel_items-INITIAL_FORMS': 0,
+            'carousel_items-MIN_NUM_FORMS': 0,
+            'carousel_items-MAX_NUM_FORMS': 0,
+            'speakers-TOTAL_FORMS': 0,
+            'speakers-INITIAL_FORMS': 0,
+            'speakers-MIN_NUM_FORMS': 0,
+            'speakers-MAX_NUM_FORMS': 0,
+            'related_links-TOTAL_FORMS': 0,
+            'related_links-INITIAL_FORMS': 0,
+            'related_links-MIN_NUM_FORMS': 0,
+            'related_links-MAX_NUM_FORMS': 0,
+        }
+        response = self.client.post(
+            reverse('wagtailadmin_pages:edit', args=(self.christmas_page.id, )),
+            post_data
+        )
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, "The page could not be saved due to validation errors")
+        self.assertContains(response, "<li>The end date must be after the start date</li>", count=1)
+
+        # Error on title shown against the title field
+        self.assertContains(response, """<p class="error-message"><span>This field is required.</span></p>""", count=1, html=True)
+        # Error on title shown in the header message
+        self.assertContains(response, "<li>Title: This field is required.</li>", count=1)

wagtail/wagtailadmin/views/pages.py

@@ -269,7 +269,9 @@ def create(request, content_type_app_name, content_type_model_name, parent_page_
                     target_url += '?next=%s' % urlquote(next_url)
                 return redirect(target_url)
         else:
-            messages.error(request, _("The page could not be created due to validation errors"))
+            messages.validation_error(
+                request, _("The page could not be created due to validation errors"), form
+            )
             edit_handler = edit_handler_class(instance=page, form=form)
             has_unsaved_changes = True
     else:
@@ -462,7 +464,9 @@ def edit(request, page_id):
             if page.locked:
                 messages.error(request, _("The page could not be saved as it is locked"))
             else:
-                messages.error(request, _("The page could not be saved due to validation errors"))
+                messages.validation_error(
+                    request, _("The page could not be saved due to validation errors"), form
+                )
 
             edit_handler = edit_handler_class(instance=page, form=form)
             errors_debug = (