From 5022f3118df5d0ba8c0d8551c41ffb8df90dcb69 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Thu, 18 May 2017 09:52:58 +0100 Subject: [PATCH] Assign persistent IDs to StreamBlock items (#3593) --- wagtail/api/v2/tests/test_pages.py | 11 +- .../contrib/wagtailapi/tests/test_pages.py | 8 +- .../block_forms/stream_member.html | 1 + wagtail/wagtailadmin/tests/api/test_pages.py | 8 +- wagtail/wagtailcore/blocks/stream_block.py | 59 ++++++++--- wagtail/wagtailcore/tests/test_blocks.py | 100 ++++++++++++++---- 6 files changed, 143 insertions(+), 44 deletions(-) diff --git a/wagtail/api/v2/tests/test_pages.py b/wagtail/api/v2/tests/test_pages.py index 1851649592..a84bc265b3 100644 --- a/wagtail/api/v2/tests/test_pages.py +++ b/wagtail/api/v2/tests/test_pages.py @@ -1026,7 +1026,10 @@ class TestPageDetailWithStreamField(TestCase): self.assertIn('id', content) self.assertEqual(content['id'], stream_page.id) self.assertIn('body', content) - self.assertEqual(content['body'], [{'type': 'text', 'value': 'foo'}]) + self.assertEqual(len(content['body']), 1) + self.assertEqual(content['body'][0]['type'], 'text') + self.assertEqual(content['body'][0]['value'], 'foo') + self.assertTrue(content['body'][0]['id']) def test_image_block(self): stream_page = self.make_stream_page('[{"type": "image", "value": 1}]') @@ -1036,7 +1039,8 @@ class TestPageDetailWithStreamField(TestCase): content = json.loads(response.content.decode('utf-8')) # ForeignKeys in a StreamField shouldn't be translated into dictionary representation - self.assertEqual(content['body'], [{'type': 'image', 'value': 1}]) + self.assertEqual(content['body'][0]['type'], 'image') + self.assertEqual(content['body'][0]['value'], 1) def test_image_block_with_custom_get_api_representation(self): stream_page = self.make_stream_page('[{"type": "image", "value": 1}]') @@ -1048,7 +1052,8 @@ class TestPageDetailWithStreamField(TestCase): content = json.loads(response.content.decode('utf-8')) # the custom get_api_representation returns a dict of id and title for the image - self.assertEqual(content['body'], [{'type': 'image', 'value': {'id': 1, 'title': 'A missing image'}}]) + self.assertEqual(content['body'][0]['type'], 'image') + self.assertEqual(content['body'][0]['value'], {'id': 1, 'title': 'A missing image'}) @override_settings( diff --git a/wagtail/contrib/wagtailapi/tests/test_pages.py b/wagtail/contrib/wagtailapi/tests/test_pages.py index 3c838a20ca..980746a22f 100644 --- a/wagtail/contrib/wagtailapi/tests/test_pages.py +++ b/wagtail/contrib/wagtailapi/tests/test_pages.py @@ -677,7 +677,10 @@ class TestPageDetailWithStreamField(TestCase): self.assertIn('id', content) self.assertEqual(content['id'], stream_page.id) self.assertIn('body', content) - self.assertEqual(content['body'], [{'type': 'text', 'value': 'foo'}]) + self.assertEqual(len(content['body']), 1) + self.assertEqual(content['body'][0]['type'], 'text') + self.assertEqual(content['body'][0]['value'], 'foo') + self.assertTrue(content['body'][0]['id']) def test_image_block(self): stream_page = self.make_stream_page('[{"type": "image", "value": 1}]') @@ -687,7 +690,8 @@ class TestPageDetailWithStreamField(TestCase): content = json.loads(response.content.decode('utf-8')) # ForeignKeys in a StreamField shouldn't be translated into dictionary representation - self.assertEqual(content['body'], [{'type': 'image', 'value': 1}]) + self.assertEqual(content['body'][0]['type'], 'image') + self.assertEqual(content['body'][0]['value'], 1) @override_settings( diff --git a/wagtail/wagtailadmin/templates/wagtailadmin/block_forms/stream_member.html b/wagtail/wagtailadmin/templates/wagtailadmin/block_forms/stream_member.html index 027ebde6b0..18cf4f74b3 100644 --- a/wagtail/wagtailadmin/templates/wagtailadmin/block_forms/stream_member.html +++ b/wagtail/wagtailadmin/templates/wagtailadmin/block_forms/stream_member.html @@ -3,6 +3,7 @@ {% block hidden_fields %} + {% endblock %} {% block header_controls %} diff --git a/wagtail/wagtailadmin/tests/api/test_pages.py b/wagtail/wagtailadmin/tests/api/test_pages.py index 8c5d03b697..9c35145d1b 100644 --- a/wagtail/wagtailadmin/tests/api/test_pages.py +++ b/wagtail/wagtailadmin/tests/api/test_pages.py @@ -568,7 +568,10 @@ class TestAdminPageDetailWithStreamField(AdminAPITestCase): self.assertIn('id', content) self.assertEqual(content['id'], stream_page.id) self.assertIn('body', content) - self.assertEqual(content['body'], [{'type': 'text', 'value': 'foo'}]) + self.assertEqual(len(content['body']), 1) + self.assertEqual(content['body'][0]['type'], 'text') + self.assertEqual(content['body'][0]['value'], 'foo') + self.assertTrue(content['body'][0]['id']) def test_image_block(self): stream_page = self.make_stream_page('[{"type": "image", "value": 1}]') @@ -578,4 +581,5 @@ class TestAdminPageDetailWithStreamField(AdminAPITestCase): content = json.loads(response.content.decode('utf-8')) # ForeignKeys in a StreamField shouldn't be translated into dictionary representation - self.assertEqual(content['body'], [{'type': 'image', 'value': 1}]) + self.assertEqual(content['body'][0]['type'], 'image') + self.assertEqual(content['body'][0]['value'], 1) diff --git a/wagtail/wagtailcore/blocks/stream_block.py b/wagtail/wagtailcore/blocks/stream_block.py index 32277d6f65..6c9093fdb4 100644 --- a/wagtail/wagtailcore/blocks/stream_block.py +++ b/wagtail/wagtailcore/blocks/stream_block.py @@ -1,6 +1,7 @@ from __future__ import absolute_import, unicode_literals import collections +import uuid from django import forms from django.contrib.staticfiles.templatetags.staticfiles import static @@ -59,7 +60,7 @@ class BaseStreamBlock(Block): """ return StreamValue(self, self.meta.default) - def render_list_member(self, block_type_name, value, prefix, index, errors=None): + def render_list_member(self, block_type_name, value, prefix, index, errors=None, id=None): """ Render the HTML for a single list item. This consists of an
  • wrapper, hidden fields to manage ID/deleted state/type, delete/reorder buttons, and the child block's own HTML. @@ -72,6 +73,7 @@ class BaseStreamBlock(Block): 'prefix': prefix, 'child': child, 'index': index, + 'block_id': id, }) def html_declarations(self): @@ -133,7 +135,7 @@ class BaseStreamBlock(Block): list_members_html = [ self.render_list_member(child.block_type, child.value, "%s-%d" % (prefix, i), i, - errors=error_dict.get(i)) + errors=error_dict.get(i), id=child.id) for (i, child) in enumerate(valid_children) ] @@ -162,13 +164,14 @@ class BaseStreamBlock(Block): int(data['%s-%d-order' % (prefix, i)]), block_type_name, child_block.value_from_datadict(data, files, '%s-%d-value' % (prefix, i)), + data.get('%s-%d-id' % (prefix, i)), ) ) values_with_indexes.sort() return StreamValue(self, [ - (child_block_type_name, value) - for (index, child_block_type_name, value) in values_with_indexes + (child_block_type_name, value, block_id) + for (index, child_block_type_name, value, block_id) in values_with_indexes ]) def value_omitted_from_data(self, data, files, prefix): @@ -177,10 +180,10 @@ class BaseStreamBlock(Block): def clean(self, value): cleaned_data = [] errors = {} - for i, child in enumerate(value): # child is a BoundBlock instance + for i, child in enumerate(value): # child is a StreamChild instance try: cleaned_data.append( - (child.block.name, child.block.clean(child.value)) + (child.block.name, child.block.clean(child.value), child.id) ) except ValidationError as e: errors[i] = ErrorList([e]) @@ -193,7 +196,8 @@ class BaseStreamBlock(Block): return StreamValue(self, cleaned_data) def to_python(self, value): - # the incoming JSONish representation is a list of dicts, each with a 'type' and 'value' field. + # the incoming JSONish representation is a list of dicts, each with a 'type' and 'value' field + # (and possibly an 'id' too). # This is passed to StreamValue to be expanded lazily - but first we reject any unrecognised # block types from the list return StreamValue(self, [ @@ -207,8 +211,13 @@ class BaseStreamBlock(Block): return [] return [ - {'type': child.block.name, 'value': child.block.get_prep_value(child.value)} - for child in value # child is a BoundBlock instance + { + 'type': child.block.name, + 'value': child.block.get_prep_value(child.value), + # assign a new ID on save if it didn't have one already + 'id': child.id or str(uuid.uuid4()), + } + for child in value # child is a StreamChild instance ] def get_api_representation(self, value, context=None): @@ -217,8 +226,12 @@ class BaseStreamBlock(Block): return [] return [ - {'type': child.block.name, 'value': child.block.get_api_representation(child.value, context=context)} - for child in value # child is a BoundBlock instance + { + 'type': child.block.name, + 'value': child.block.get_api_representation(child.value, context=context), + 'id': child.id + } + for child in value # child is a StreamChild instance ] def render_basic(self, value, context=None): @@ -285,6 +298,10 @@ class StreamValue(collections.Sequence): children of StreamField, but not necessarily elsewhere that BoundBlock is used """ + def __init__(self, *args, **kwargs): + self.id = kwargs.pop('id') + super(StreamValue.StreamChild, self).__init__(*args, **kwargs) + @property def block_type(self): """ @@ -307,7 +324,7 @@ class StreamValue(collections.Sequence): Passing is_lazy=False means that stream_data consists of immediately usable native values. In this mode, stream_data is a list of (type_name, value) - tuples. + or (type_name, value, id) tuples. raw_text exists solely as a way of representing StreamField content that is not valid JSON; this may legitimately occur if an existing text field is @@ -332,11 +349,17 @@ class StreamValue(collections.Sequence): return self._bound_blocks[i] else: value = child_block.to_python(raw_value['value']) + block_id = raw_value.get('id') else: - type_name, value = self.stream_data[i] + try: + type_name, value, block_id = self.stream_data[i] + except ValueError: + type_name, value = self.stream_data[i] + block_id = None + child_block = self.stream_block.child_blocks[type_name] - self._bound_blocks[i] = StreamValue.StreamChild(child_block, value) + self._bound_blocks[i] = StreamValue.StreamChild(child_block, value, id=block_id) return self._bound_blocks[i] @@ -346,14 +369,20 @@ class StreamValue(collections.Sequence): This prevents n queries for n blocks of a specific type. """ + # create a mapping of all the child blocks matching the given block type, + # mapping (index within the stream) => (raw block value) raw_values = collections.OrderedDict( (i, item['value']) for i, item in enumerate(self.stream_data) if item['type'] == type_name ) + # pass the raw block values to bulk_to_python as a list converted_values = child_block.bulk_to_python(raw_values.values()) + # reunite the converted values with their stream indexes for i, value in zip(raw_values.keys(), converted_values): - self._bound_blocks[i] = StreamValue.StreamChild(child_block, value) + # also pass the block ID to StreamChild, if one exists for this stream index + block_id = self.stream_data[i].get('id') + self._bound_blocks[i] = StreamValue.StreamChild(child_block, value, id=block_id) def __eq__(self, other): if not isinstance(other, StreamValue): diff --git a/wagtail/wagtailcore/tests/test_blocks.py b/wagtail/wagtailcore/tests/test_blocks.py index faa32795a0..a9c0ad13b1 100644 --- a/wagtail/wagtailcore/tests/test_blocks.py +++ b/wagtail/wagtailcore/tests/test_blocks.py @@ -1707,7 +1707,7 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): api_representation = block.get_api_representation( block.to_python([ {'type': 'language', 'value': 'en'}, - {'type': 'author', 'value': 'wagtail'}, + {'type': 'author', 'value': 'wagtail', 'id': '111111'}, ]), context={ 'en': 'English', @@ -1717,8 +1717,8 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): self.assertListEqual( api_representation, [ - {'type': 'language', 'value': 'English'}, - {'type': 'author', 'value': 'Wagtail!'}, + {'type': 'language', 'value': 'English', 'id': None}, + {'type': 'author', 'value': 'Wagtail!', 'id': '111111'}, ] ) @@ -1846,6 +1846,7 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): { 'type': 'heading', 'value': "My title", + 'id': '123123123', }, { 'type': 'paragraph', @@ -1880,6 +1881,13 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): self.assertIn('', html) self.assertIn('', html) + def test_render_form_id_fields(self): + html = self.render_form() + + self.assertIn('', html) + self.assertIn('', html) + self.assertIn('', html) + def test_render_form_type_fields(self): html = self.render_form() @@ -1933,24 +1941,12 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): url = blocks.URLBlock() block = ValidatedBlock() - value = [ - blocks.BoundBlock( - block=block.child_blocks['char'], - value='', - ), - blocks.BoundBlock( - block=block.child_blocks['char'], - value='foo', - ), - blocks.BoundBlock( - block=block.child_blocks['url'], - value='http://example.com/', - ), - blocks.BoundBlock( - block=block.child_blocks['url'], - value='not a url', - ), - ] + value = blocks.StreamValue(block, [ + ('char', ''), + ('char', 'foo'), + ('url', 'http://example.com/'), + ('url', 'not a url'), + ]) with self.assertRaises(ValidationError) as catcher: block.clean(value) @@ -2015,6 +2011,8 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): block = ArticleBlock() html = block.html_declarations() + self.assertTagInTemplateScript('', html) + self.assertTagInTemplateScript('', html) self.assertTagInTemplateScript('', html) self.assertTagInTemplateScript( '', @@ -2081,11 +2079,13 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): 'article-%d-deleted' % i: '', 'article-%d-order' % i: str(2 - i), 'article-%d-type' % i: 'heading', - 'article-%d-value' % i: "heading %d" % i + 'article-%d-value' % i: "heading %d" % i, + 'article-%d-id' % i: "000%d" % i, }) block_value = block.value_from_datadict(post_data, {}, 'article') self.assertEqual(block_value[2].value, "heading 0") + self.assertEqual(block_value[2].id, "0000") def test_ordering_in_form_submission_is_numeric(self): class ArticleBlock(blocks.StreamBlock): @@ -2234,6 +2234,62 @@ class TestStreamBlock(WagtailTestUtils, SimpleTestCase): self.assertIn('

    group1

    ', html) self.assertIn('

    group2

    ', html) + def test_value_from_datadict(self): + class ArticleBlock(blocks.StreamBlock): + heading = blocks.CharBlock() + paragraph = blocks.CharBlock() + + block = ArticleBlock() + + value = block.value_from_datadict({ + 'foo-count': '3', + 'foo-0-deleted': '', + 'foo-0-order': '2', + 'foo-0-type': 'heading', + 'foo-0-id': '0000', + 'foo-0-value': 'this is my heading', + 'foo-1-deleted': '1', + 'foo-1-order': '1', + 'foo-1-type': 'heading', + 'foo-1-id': '0001', + 'foo-1-value': 'a deleted heading', + 'foo-2-deleted': '', + 'foo-2-order': '0', + 'foo-2-type': 'paragraph', + 'foo-2-id': '', + 'foo-2-value': '

    this is a paragraph

    ', + }, {}, prefix='foo') + + self.assertEqual(len(value), 2) + self.assertEqual(value[0].block_type, 'paragraph') + self.assertEqual(value[0].id, '') + self.assertEqual(value[0].value, '

    this is a paragraph

    ') + + self.assertEqual(value[1].block_type, 'heading') + self.assertEqual(value[1].id, '0000') + self.assertEqual(value[1].value, 'this is my heading') + + def test_get_prep_value(self): + class ArticleBlock(blocks.StreamBlock): + heading = blocks.CharBlock() + paragraph = blocks.CharBlock() + + block = ArticleBlock() + + value = blocks.StreamValue(block, [ + ('heading', 'this is my heading', '0000'), + ('paragraph', '

    this is a paragraph

    ') + ]) + jsonish_value = block.get_prep_value(value) + + self.assertEqual(len(jsonish_value), 2) + self.assertEqual(jsonish_value[0], {'type': 'heading', 'value': 'this is my heading', 'id': '0000'}) + self.assertEqual(jsonish_value[1]['type'], 'paragraph') + self.assertEqual(jsonish_value[1]['value'], '

    this is a paragraph

    ') + # get_prep_value should assign a new (random and non-empty) ID to this block, as it didn't + # have one already + self.assertTrue(jsonish_value[1]['id']) + class TestPageChooserBlock(TestCase): fixtures = ['test.json']