From 1061caa5eff53f450520284b618e615fc2a809c8 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Mon, 26 Apr 2021 19:08:04 +0100 Subject: [PATCH] Fix inconsistent StreamField ValidationError nesting Fixes #7086. As per https://github.com/wagtail/wagtail/issues/7086#issuecomment-826945031, ensure that .as_data() is consistently called when telepath-packing ErrorList objects (so that we preserve any embedded ValidationError objects instead of casting them to strings), and introduce an explicit ValidationError class on the client side to make mismatches more obvious (and for future extensibility in case we need to attach more fancy logic to ValidationError). Also add tests for setError, and fix rendering of StreamBlock non-field errors (selector to clear old errors was incorrect, and jest apparently doesn't support innerText). --- .../StreamField/blocks/FieldBlock.js | 4 +- .../StreamField/blocks/FieldBlock.test.js | 14 ++ .../StreamField/blocks/ListBlock.test.js | 18 ++- .../StreamField/blocks/StreamBlock.js | 6 +- .../StreamField/blocks/StreamBlock.test.js | 23 +++- .../StreamField/blocks/StructBlock.test.js | 17 ++- .../__snapshots__/FieldBlock.test.js.snap | 11 ++ .../__snapshots__/ListBlock.test.js.snap | 109 ++++++++++++++++ .../__snapshots__/StreamBlock.test.js.snap | 123 ++++++++++++++++++ .../__snapshots__/StructBlock.test.js.snap | 32 +++++ .../src/entrypoints/admin/telepath/widgets.js | 7 + wagtail/core/blocks/stream_block.py | 2 +- wagtail/core/blocks/struct_block.py | 10 +- wagtail/core/widget_adapters.py | 19 +++ 14 files changed, 385 insertions(+), 10 deletions(-) diff --git a/client/src/components/StreamField/blocks/FieldBlock.js b/client/src/components/StreamField/blocks/FieldBlock.js index d0e84be17b..d47a924e46 100644 --- a/client/src/components/StreamField/blocks/FieldBlock.js +++ b/client/src/components/StreamField/blocks/FieldBlock.js @@ -27,7 +27,7 @@ export class FieldBlock { // eslint-disable-next-line no-console console.error(e); this.setError([ - ['This widget failed to render, please check the console for details'] + { messages: ['This widget failed to render, please check the console for details'] } ]); return; } @@ -78,7 +78,7 @@ export class FieldBlock { const errorElement = document.createElement('p'); errorElement.classList.add('error-message'); - errorElement.innerHTML = errorList.map(error => `${h(error[0])}`).join(''); + errorElement.innerHTML = errorList.map(error => `${h(error.messages[0])}`).join(''); this.element.querySelector('.field-content').appendChild(errorElement); } else { this.element.classList.remove('error'); diff --git a/client/src/components/StreamField/blocks/FieldBlock.test.js b/client/src/components/StreamField/blocks/FieldBlock.test.js index 756acd0c61..c5d9c78cd2 100644 --- a/client/src/components/StreamField/blocks/FieldBlock.test.js +++ b/client/src/components/StreamField/blocks/FieldBlock.test.js @@ -41,6 +41,12 @@ class DummyWidgetDefinition { } } +class ValidationError { + constructor(messages) { + this.messages = messages; + } +} + describe('telepath: wagtail.blocks.FieldBlock', () => { let boundBlock; @@ -117,6 +123,14 @@ describe('telepath: wagtail.blocks.FieldBlock', () => { expect(focus.mock.calls.length).toBe(1); expect(focus.mock.calls[0][0]).toBe('The widget'); }); + + test('setError() renders errors', () => { + boundBlock.setError([ + new ValidationError(['Field must not contain the letter E']), + new ValidationError(['Field must contain a story about kittens']), + ]); + expect(document.body.innerHTML).toMatchSnapshot(); + }); }); describe('telepath: wagtail.blocks.FieldBlock with comments enabled', () => { diff --git a/client/src/components/StreamField/blocks/ListBlock.test.js b/client/src/components/StreamField/blocks/ListBlock.test.js index eec7c11c1a..823adb9644 100644 --- a/client/src/components/StreamField/blocks/ListBlock.test.js +++ b/client/src/components/StreamField/blocks/ListBlock.test.js @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import { FieldBlockDefinition } from './FieldBlock'; -import { ListBlockDefinition } from './ListBlock'; +import { ListBlockDefinition, ListBlockValidationError } from './ListBlock'; import $ from 'jquery'; window.$ = $; @@ -37,6 +37,12 @@ class DummyWidgetDefinition { } } +class ValidationError { + constructor(messages) { + this.messages = messages; + } +} + describe('telepath: wagtail.blocks.ListBlock', () => { let boundBlock; @@ -202,4 +208,14 @@ describe('telepath: wagtail.blocks.ListBlock', () => { expect(document.body.innerHTML).toMatchSnapshot(); }); + + test('setError passes error messages to children', () => { + boundBlock.setError([ + new ListBlockValidationError([ + null, + [new ValidationError(['Not as good as the first one'])], + ]), + ]); + expect(document.body.innerHTML).toMatchSnapshot(); + }); }); diff --git a/client/src/components/StreamField/blocks/StreamBlock.js b/client/src/components/StreamField/blocks/StreamBlock.js index da19c50d26..b308f91a46 100644 --- a/client/src/components/StreamField/blocks/StreamBlock.js +++ b/client/src/components/StreamField/blocks/StreamBlock.js @@ -330,15 +330,15 @@ export class StreamBlock extends BaseSequenceBlock { // Non block errors const container = this.container[0]; - container.querySelectorAll(':scope > .help-block .help-critical').forEach(element => element.remove()); + container.querySelectorAll(':scope > .help-block.help-critical').forEach(element => element.remove()); if (error.nonBlockErrors.length > 0) { // Add a help block for each error raised - error.nonBlockErrors.forEach(errorText => { + error.nonBlockErrors.forEach(nonBlockError => { const errorElement = document.createElement('p'); errorElement.classList.add('help-block'); errorElement.classList.add('help-critical'); - errorElement.innerText = errorText; + errorElement.innerHTML = h(nonBlockError.messages[0]); container.insertBefore(errorElement, container.childNodes[0]); }); } diff --git a/client/src/components/StreamField/blocks/StreamBlock.test.js b/client/src/components/StreamField/blocks/StreamBlock.test.js index 310612067d..5b6fb1f254 100644 --- a/client/src/components/StreamField/blocks/StreamBlock.test.js +++ b/client/src/components/StreamField/blocks/StreamBlock.test.js @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import { FieldBlockDefinition } from './FieldBlock'; -import { StreamBlockDefinition } from './StreamBlock'; +import { StreamBlockDefinition, StreamBlockValidationError } from './StreamBlock'; import $ from 'jquery'; window.$ = $; @@ -37,6 +37,12 @@ class DummyWidgetDefinition { } } +class ValidationError { + constructor(messages) { + this.messages = messages; + } +} + describe('telepath: wagtail.blocks.StreamBlock', () => { let boundBlock; @@ -272,6 +278,21 @@ describe('telepath: wagtail.blocks.StreamBlock', () => { expect(document.body.innerHTML).toMatchSnapshot(); }); + + test('setError renders error messages', () => { + boundBlock.setError([ + new StreamBlockValidationError( + [ + /* non-block error */ + new ValidationError(['At least three blocks are required']), + ], + { + /* block error */ + 1: [new ValidationError(['Not as good as the first one'])], + }), + ]); + expect(document.body.innerHTML).toMatchSnapshot(); + }); }); describe('telepath: wagtail.blocks.StreamBlock with labels that need escaping', () => { diff --git a/client/src/components/StreamField/blocks/StructBlock.test.js b/client/src/components/StreamField/blocks/StructBlock.test.js index c6bb1d33e7..b981e61178 100644 --- a/client/src/components/StreamField/blocks/StructBlock.test.js +++ b/client/src/components/StreamField/blocks/StructBlock.test.js @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import { FieldBlockDefinition } from './FieldBlock'; -import { StructBlockDefinition } from './StructBlock'; +import { StructBlockDefinition, StructBlockValidationError } from './StructBlock'; import $ from 'jquery'; window.$ = $; @@ -37,6 +37,12 @@ class DummyWidgetDefinition { } } +class ValidationError { + constructor(messages) { + this.messages = messages; + } +} + describe('telepath: wagtail.blocks.StructBlock', () => { let boundBlock; @@ -152,6 +158,15 @@ describe('telepath: wagtail.blocks.StructBlock', () => { expect(focus.mock.calls.length).toBe(1); expect(focus.mock.calls[0][0]).toBe('Heading widget'); }); + + test('setError passes error messages to children', () => { + boundBlock.setError([ + new StructBlockValidationError({ + size: [new ValidationError(['This is too big'])], + }), + ]); + expect(document.body.innerHTML).toMatchSnapshot(); + }); }); describe('telepath: wagtail.blocks.StructBlock with formTemplate', () => { diff --git a/client/src/components/StreamField/blocks/__snapshots__/FieldBlock.test.js.snap b/client/src/components/StreamField/blocks/__snapshots__/FieldBlock.test.js.snap index 8204bf6f4c..a658555af2 100644 --- a/client/src/components/StreamField/blocks/__snapshots__/FieldBlock.test.js.snap +++ b/client/src/components/StreamField/blocks/__snapshots__/FieldBlock.test.js.snap @@ -22,6 +22,17 @@ exports[`telepath: wagtail.blocks.FieldBlock it renders correctly 1`] = ` " `; +exports[`telepath: wagtail.blocks.FieldBlock setError() renders errors 1`] = ` +"
+
+
+

The widget

+ +
+

drink more water

Field must not contain the letter EField must contain a story about kittens

+
" +`; + exports[`telepath: wagtail.blocks.FieldBlock with comments enabled it renders correctly 1`] = ` "
diff --git a/client/src/components/StreamField/blocks/__snapshots__/ListBlock.test.js.snap b/client/src/components/StreamField/blocks/__snapshots__/ListBlock.test.js.snap index 1ceb33cf1a..196b591a68 100644 --- a/client/src/components/StreamField/blocks/__snapshots__/ListBlock.test.js.snap +++ b/client/src/components/StreamField/blocks/__snapshots__/ListBlock.test.js.snap @@ -591,3 +591,112 @@ exports[`telepath: wagtail.blocks.ListBlock it renders correctly 1`] = `
" `; + +exports[`telepath: wagtail.blocks.ListBlock setError passes error messages to children 1`] = ` +" +
+
?
+ use a few of these +
+
+ + +
+ + + + + +
+
+
+
+ + + +

+
+ + + + + +
+
+
+
+
+
+
+

The widget

+ +
+
+
+
+
+
+
+
+
+ + + + + +
+
+
+
+ + + +

+
+ + + + + +
+
+
+
+
+
+
+

The widget

+ +
+

Not as good as the first one

+
+
+
+
+
+
+
+
" +`; diff --git a/client/src/components/StreamField/blocks/__snapshots__/StreamBlock.test.js.snap b/client/src/components/StreamField/blocks/__snapshots__/StreamBlock.test.js.snap index 62130fff4a..50627d3d31 100644 --- a/client/src/components/StreamField/blocks/__snapshots__/StreamBlock.test.js.snap +++ b/client/src/components/StreamField/blocks/__snapshots__/StreamBlock.test.js.snap @@ -677,6 +677,129 @@ exports[`telepath: wagtail.blocks.StreamBlock it renders menus on opening 1`] = " `; +exports[`telepath: wagtail.blocks.StreamBlock setError renders error messages 1`] = ` +" +
+
?
+ use plenty of these +
+

At least three blocks are required

+ +
+ +
+
+
+
+ + + + + +
+
+
+
+ + + +

+
+ Test Block A + + + + +
+
+
+
+
+
+
+

Block A widget

+ +
+
+
+
+
+
+
+
+
+ +
+
+
+
+ + + + + +
+
+
+
+ + + +

+
+ Test Block B + + + + +
+
+
+
+
+
+
+

Block B widget

+ +
+

Not as good as the first one

+
+
+
+
+
+
+
+ +
+
+
+
+
" +`; + exports[`telepath: wagtail.blocks.StreamBlock with labels that need escaping it renders correctly 1`] = ` "
diff --git a/client/src/components/StreamField/blocks/__snapshots__/StructBlock.test.js.snap b/client/src/components/StreamField/blocks/__snapshots__/StructBlock.test.js.snap index 1a168bf9cd..76aac31df6 100644 --- a/client/src/components/StreamField/blocks/__snapshots__/StructBlock.test.js.snap +++ b/client/src/components/StreamField/blocks/__snapshots__/StructBlock.test.js.snap @@ -32,6 +32,38 @@ exports[`telepath: wagtail.blocks.StructBlock it renders correctly 1`] = `
" `; +exports[`telepath: wagtail.blocks.StructBlock setError passes error messages to children 1`] = ` +"
+ + +
+
?
+ use lots of these +
+ +
+ +
+
+
+

Heading widget

+ +
+
+
+
+ +
+
+
+

Size widget

+ +
+

This is too big

+
+
" +`; + exports[`telepath: wagtail.blocks.StructBlock with formTemplate it renders correctly 1`] = ` "

here comes the first field:

diff --git a/client/src/entrypoints/admin/telepath/widgets.js b/client/src/entrypoints/admin/telepath/widgets.js index 3414a608d9..c8434e3e48 100644 --- a/client/src/entrypoints/admin/telepath/widgets.js +++ b/client/src/entrypoints/admin/telepath/widgets.js @@ -235,3 +235,10 @@ class AdminDateTimeInput extends BaseDateTimeWidget { initChooserFn = window.initDateTimeChooser; } window.telepath.register('wagtail.widgets.AdminDateTimeInput', AdminDateTimeInput); + +class ValidationError { + constructor(messages) { + this.messages = messages; + } +} +window.telepath.register('wagtail.errors.ValidationError', ValidationError); diff --git a/wagtail/core/blocks/stream_block.py b/wagtail/core/blocks/stream_block.py index 76b19aae62..14d9829dfa 100644 --- a/wagtail/core/blocks/stream_block.py +++ b/wagtail/core/blocks/stream_block.py @@ -38,7 +38,7 @@ class StreamBlockValidationErrorAdapter(Adapter): js_constructor = 'wagtail.blocks.StreamBlockValidationError' def js_args(self, error): - return [error.non_block_errors, { + return [error.non_block_errors.as_data(), { block_id: child_errors.as_data() for block_id, child_errors in error.block_errors.items() }] diff --git a/wagtail/core/blocks/struct_block.py b/wagtail/core/blocks/struct_block.py index db15985550..2ea3c98928 100644 --- a/wagtail/core/blocks/struct_block.py +++ b/wagtail/core/blocks/struct_block.py @@ -27,7 +27,15 @@ class StructBlockValidationErrorAdapter(Adapter): js_constructor = 'wagtail.blocks.StructBlockValidationError' def js_args(self, error): - return [error.block_errors] + if error.block_errors is None: + return [None] + else: + return [ + { + name: error_list.as_data() + for name, error_list in error.block_errors.items() + } + ] @cached_property def media(self): diff --git a/wagtail/core/widget_adapters.py b/wagtail/core/widget_adapters.py index 6f2c4b32d6..28c8aa41ee 100644 --- a/wagtail/core/widget_adapters.py +++ b/wagtail/core/widget_adapters.py @@ -5,6 +5,7 @@ and extract field values. """ from django import forms +from django.core.exceptions import ValidationError from django.utils.functional import cached_property from wagtail.admin.staticfiles import versioned_static @@ -49,3 +50,21 @@ class RadioSelectAdapter(WidgetAdapter): register(RadioSelectAdapter(), forms.RadioSelect) + + +class ValidationErrorAdapter(Adapter): + js_constructor = 'wagtail.errors.ValidationError' + + def js_args(self, error): + return [ + error.messages, + ] + + @cached_property + def media(self): + return forms.Media(js=[ + versioned_static('wagtailadmin/js/telepath/widgets.js'), + ]) + + +register(ValidationErrorAdapter(), ValidationError)