Cleanup for the `Block.__eq__` method (#4378)

* Fix incorrect assertion in TestSystemCheck

It only worked by accident, due to the definition of equality on block objects.

* Explicitly cast child_blocks.items() to a list when deconstructing blocks

The result of items() is a lazily-evaluated ItemsView object, which Django's deep_deconstruct function (used to detect changes for migrations) doesn't recognise; as a result, the blocks inside it don't get deconstructed. Luckily, this doesn't break migration change detection, because we define __eq__ on Block so that two blocks with matching definitions are considered equal. Nevertheless, it's best that we don't rely on that behaviour; it was only implemented originally as a workaround for https://code.djangoproject.com/ticket/24340 in Django <1.9 (where deep_deconstruct didn't recurse into lists either).

* Bring comment for Block.__eq__ up to date

wagtail/core/blocks/base.py

@@ -358,19 +358,19 @@ class Block(metaclass=BaseBlock):
 
     def __eq__(self, other):
         """
-        The deep_deconstruct method in django.db.migrations.autodetector.MigrationAutodetector does not
-        recurse into arbitrary lists and dicts. As a result, when it is passed a field such as:
-            StreamField([
-                ('heading', CharBlock()),
-            ])
-        the CharBlock object will be left in its constructed form. This causes problems when
-        MigrationAutodetector compares two separate instances of the StreamField from different project
-        states: since the CharBlocks are different objects, it will report a change where there isn't one.
+        Implement equality on block objects so that two blocks with matching definitions are considered
+        equal. (Block objects are intended to be immutable with the exception of set_name(), so here
+        'matching definitions' means that both the 'name' property and the constructor args/kwargs - as
+        captured in _constructor_args - are equal on both blocks.)
 
-        To prevent this, we implement the equality operator on Block instances such that the two CharBlocks
-        are reported as equal. Since block objects are intended to be immutable with the exception of
-        set_name(), it is sufficient to compare the 'name' property and the constructor args/kwargs of the
-        two block objects. The 'deconstruct' method provides a convenient way to access the latter.
+        This was originally necessary as a workaround for https://code.djangoproject.com/ticket/24340
+        in Django <1.9; the deep_deconstruct function used to detect changes for migrations did not
+        recurse into the block lists, and left them as Block instances. This __eq__ method therefore
+        came into play when identifying changes within migrations.
+
+        As of Django >=1.9, this *probably* isn't required any more. However, it may be useful in
+        future as a way of identifying blocks that can be re-used within StreamField definitions
+        (https://github.com/wagtail/wagtail/issues/4298#issuecomment-367656028).
         """
 
         if not isinstance(other, Block):

wagtail/core/blocks/stream_block.py

@@ -298,7 +298,7 @@ class BaseStreamBlock(Block):
         to a custom subclass in the user's models.py that may or may not stick around.
         """
         path = 'wagtail.core.blocks.StreamBlock'
-        args = [self.child_blocks.items()]
+        args = [list(self.child_blocks.items())]
         kwargs = self._constructor_kwargs
         return (path, args, kwargs)
 

wagtail/core/blocks/struct_block.py

@@ -183,7 +183,7 @@ class BaseStructBlock(Block):
         to a custom subclass in the user's models.py that may or may not stick around.
         """
         path = 'wagtail.core.blocks.StructBlock'
-        args = [self.child_blocks.items()]
+        args = [list(self.child_blocks.items())]
         kwargs = self._constructor_kwargs
         return (path, args, kwargs)
 

wagtail/core/fields.py

@@ -56,7 +56,7 @@ class StreamField(models.Field):
 
     def deconstruct(self):
         name, path, _, kwargs = super().deconstruct()
-        block_types = self.stream_block.child_blocks.items()
+        block_types = list(self.stream_block.child_blocks.items())
         args = [block_types]
         return name, path, args, kwargs
 

wagtail/core/tests/test_blocks.py

@@ -3010,7 +3010,7 @@ class TestSystemCheck(TestCase):
         self.assertEqual(errors[0].obj, failing_block_1)
         self.assertEqual(errors[1].id, 'wagtailcore.E001')
         self.assertEqual(errors[1].hint, "Block names cannot contain spaces")
-        self.assertEqual(errors[0].obj, failing_block_2)
+        self.assertEqual(errors[1].obj, failing_block_2)
 
 
 class TestTemplateRendering(TestCase):