From a91ad071c0e43819b9eeaf315a12dc1a99c47ad3 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Thu, 28 May 2015 17:06:29 +0100 Subject: [PATCH] Avoid calling to_python when retrieving form field values in FieldBlock. to_python and get_prep_value should only be used for serialising / deserialising between native block values and their JSONish representations. Instead, we add two new methods value_for_form and value_from_form to handle any necessary conversions between form values and block values. --- wagtail/wagtailcore/blocks/field_block.py | 53 +++++++++++++-- wagtail/wagtailcore/tests/test_blocks.py | 81 +++++++++++++++++++++++ 2 files changed, 127 insertions(+), 7 deletions(-) diff --git a/wagtail/wagtailcore/blocks/field_block.py b/wagtail/wagtailcore/blocks/field_block.py index f9bd05cbc..132e65081 100644 --- a/wagtail/wagtailcore/blocks/field_block.py +++ b/wagtail/wagtailcore/blocks/field_block.py @@ -17,6 +17,7 @@ from .base import Block class FieldBlock(Block): + """A block that wraps a Django form field""" class Meta: default = None @@ -33,11 +34,13 @@ class FieldBlock(Block): widget_attrs = {'id': prefix, 'placeholder': self.label} + field_value = self.value_for_form(value) + if hasattr(widget, 'render_with_errors'): - widget_html = widget.render_with_errors(prefix, value, attrs=widget_attrs, errors=errors) + widget_html = widget.render_with_errors(prefix, field_value, attrs=widget_attrs, errors=errors) widget_has_rendered_errors = True else: - widget_html = widget.render(prefix, value, attrs=widget_attrs) + widget_html = widget.render(prefix, field_value, attrs=widget_attrs) widget_has_rendered_errors = False return render_to_string('wagtailadmin/block_forms/field.html', { @@ -50,11 +53,34 @@ class FieldBlock(Block): 'errors': errors if (not widget_has_rendered_errors) else None }) + def value_from_form(self, value): + """ + The value that we get back from the form field might not be the type + that this block works with natively; for example, the block may want to + wrap a simple value such as a string in an object that provides a fancy + HTML rendering (e.g. EmbedBlock). + + We therefore provide this method to perform any necessary conversion + from the form field value to the block's native value. As standard, + this returns the form field value unchanged. + """ + return value + + def value_for_form(self, value): + """ + Reverse of value_from_form; convert a value of this block's native value type + to one that can be rendered by the form field + """ + return value + def value_from_datadict(self, data, files, prefix): - return self.to_python(self.field.widget.value_from_datadict(data, files, prefix)) + return self.value_from_form(self.field.widget.value_from_datadict(data, files, prefix)) def clean(self, value): - return self.field.clean(value) + # We need an annoying value_for_form -> value_from_form round trip here to account for + # the possibility that the form field is set up to validate a different value type to + # the one this block works with natively + return self.value_from_form(self.field.clean(self.value_for_form(value))) class CharBlock(FieldBlock): @@ -248,7 +274,8 @@ class ChooserBlock(FieldBlock): return forms.ModelChoiceField(queryset=self.target_model.objects.all(), widget=self.widget, required=self.required) def to_python(self, value): - if value is None or isinstance(value, self.target_model): + # the incoming serialised value should be None or an ID + if value is None: return value else: try: @@ -257,10 +284,21 @@ class ChooserBlock(FieldBlock): return None def get_prep_value(self, value): - if isinstance(value, self.target_model): - return value.id + # the native value (a model instance or None) should serialise to an ID or None + if value is None: + return None else: + return value.id + + def value_from_form(self, value): + # ModelChoiceField sometimes returns an ID, and sometimes an instance; we want the instance + if value is None or isinstance(value, self.target_model): return value + else: + try: + return self.target_model.objects.get(pk=value) + except self.target_model.DoesNotExist: + return None def clean(self, value): # ChooserBlock works natively with model instances as its 'value' type (because that's what you @@ -274,6 +312,7 @@ class ChooserBlock(FieldBlock): value = value.pk return super(ChooserBlock, self).clean(value) + class PageChooserBlock(ChooserBlock): @cached_property def target_model(self): diff --git a/wagtail/wagtailcore/tests/test_blocks.py b/wagtail/wagtailcore/tests/test_blocks.py index 307d9c848..154a64201 100644 --- a/wagtail/wagtailcore/tests/test_blocks.py +++ b/wagtail/wagtailcore/tests/test_blocks.py @@ -3,8 +3,12 @@ import unittest from django import forms from django.forms.utils import ErrorList from django.core.exceptions import ValidationError +from django.test import TestCase from wagtail.wagtailcore import blocks +from wagtail.wagtailcore.models import Page + +import base64 class TestFieldBlock(unittest.TestCase): @@ -81,6 +85,26 @@ class TestFieldBlock(unittest.TestCase): self.assertEqual(content, ["Choice 1"]) + def test_form_handling_is_independent_of_serialisation(self): + class Base64EncodingCharBlock(blocks.CharBlock): + """A CharBlock with a deliberately perverse JSON (de)serialisation format + so that it visibly blows up if we call to_python / get_prep_value where we shouldn't""" + + def to_python(self, jsonish_value): + # decode as base64 on the way out of the JSON serialisation + return base64.b64decode(jsonish_value) + + def get_prep_value(self, native_value): + # encode as base64 on the way into the JSON serialisation + return base64.b64encode(native_value) + + block = Base64EncodingCharBlock() + form_html = block.render_form('hello world', 'title') + self.assertIn('value="hello world"', form_html) + + value_from_form = block.value_from_datadict({'title': 'hello world'}, {}, 'title') + self.assertEqual('hello world', value_from_form) + class TestChoiceBlock(unittest.TestCase): def setUp(self): @@ -1019,3 +1043,60 @@ class TestStreamBlock(unittest.TestCase): self.assertEqual(len(stream_value), 1) self.assertEqual(stream_value[0].block_type, 'heading') self.assertEqual(stream_value[0].value, 'A different default heading') + + +class TestPageChooserBlock(TestCase): + fixtures = ['test.json'] + + def test_serialize(self): + """The value of a PageChooserBlock (a Page object) should serialize to an ID""" + block = blocks.PageChooserBlock() + christmas_page = Page.objects.get(slug='christmas') + + self.assertEqual(block.get_prep_value(christmas_page), christmas_page.id) + + # None should serialize to None + self.assertEqual(block.get_prep_value(None), None) + + def test_deserialize(self): + """The serialized value of a PageChooserBlock (an ID) should deserialize to a Page object""" + block = blocks.PageChooserBlock() + christmas_page = Page.objects.get(slug='christmas') + + self.assertEqual(block.to_python(christmas_page.id), christmas_page) + + # None should deserialize to None + self.assertEqual(block.to_python(None), None) + + def test_form_render(self): + block = blocks.PageChooserBlock() + + empty_form_html = block.render_form(None, 'page') + self.assertIn('', empty_form_html) + + christmas_page = Page.objects.get(slug='christmas') + christmas_form_html = block.render_form(christmas_page, 'page') + expected_html = '' % christmas_page.id + self.assertIn(expected_html, christmas_form_html) + + def test_form_response(self): + block = blocks.PageChooserBlock() + christmas_page = Page.objects.get(slug='christmas') + + value = block.value_from_datadict({'page': str(christmas_page.id)}, {}, 'page') + self.assertEqual(value, christmas_page) + + empty_value = block.value_from_datadict({'page': ''}, {}, 'page') + self.assertEqual(empty_value, None) + + def test_clean(self): + required_block = blocks.PageChooserBlock() + nonrequired_block = blocks.PageChooserBlock(required=False) + christmas_page = Page.objects.get(slug='christmas') + + self.assertEqual(required_block.clean(christmas_page), christmas_page) + with self.assertRaises(ValidationError): + required_block.clean(None) + + self.assertEqual(nonrequired_block.clean(christmas_page), christmas_page) + self.assertEqual(nonrequired_block.clean(None), None)