Multiple listing blocks: Rendering fails for copy-pasted listing blocks

[ksuess]

To Reproduce

• Create listing block.
• copy paste listing block and modify criteria
• both blocks are rendering the same despite different criteria

Software (please complete the following information):
Current Volto

[ksuess]

Problem is, that for the rendering, the block id included in content.blocks[blockid] is used. This is the same for original and copy. :-(

[ksuess]

Question here: Why is the block id included in content.blocks[blockid] at all? It's redundant and raises an error when duplicating listing blocks.

[ksuess]

I would like to modify the copy change action instead of eliminating the block id. Eliminating the redundant block id would mean a breaking change.

Example for page with duplicated and modified listing blocks:

{
  "content": {
    "@components": "{actions: {…}, aliases: {…}, breadcrumbs: {…}, cata…}",
    "@id": "http://igib.example.com/api/dokumentation/manual-navigations-seiten/uebersicht-komponente-lena",
    "@type": "Module Navigation Page",
    "UID": "7a28fc0e7c3147669a14413ad8f44488",
    "allow_discussion": false,
    "blocks": {
      "3343eead-7d86-4eb7-9393-0ab4a7d1aaa6": {
        "@type": "listing",
        "block": "8d726b48-f73b-4fcf-b851-7590bdc3e35e",
        "headline": "Anleitungen",
        "headlineTag": "h2",
        "query": "[]",
        "querystring": "{query: Array(2), sort_order: \"ascending\"}",
        "variation": "default"
      },
      "8d726b48-f73b-4fcf-b851-7590bdc3e35e": {
        "@type": "listing",
        "block": "8d726b48-f73b-4fcf-b851-7590bdc3e35e",
        "headline": "Anleitungen",
        "headlineTag": "h2",
        "query": "[]",
        "querystring": "{query: Array(2), sort_order: \"ascending\"}",
        "variation": "default"
      },
      "eaef6ccb-2fa5-4d7c-aeae-555cae09a122": {
        "@type": "title"
      }
    },
    "blocks_layout": "{items: Array(3)}",
    "contributors": "[]",
    "created": "2023-01-09T07:49:42+00:00",
    "creators": "[\"admin\"]",
    "description": "",
    "effective": null,
    "exclude_from_nav": false,
    "expires": null,
    "id": "uebersicht-komponente-lena",
    "is_folderish": false,
    "kompasscomponent": null,
    "layout": "view",
    "lock": "{locked: false, stealable: true}",
    "modified": "2023-01-09T08:09:14+00:00",
    "next_item": "{}",
    "parent": "{@id: \"http://igib.example.com/api/dokumentation/ma…}",
    "previous_item": "{@id: \"http://igib.example.com/api/dokumentation/ma…}",
    "review_state": "published",
    "rights": "",
    "title": "Übersicht Komponente LENA",
    "version": "current",
    "working_copy": null,
    "working_copy_of": null
  },

[tiberiuichim]

@ksuess Maybe we need to define a cloneData() method in the listing block configuration. we have support for custom copy methods for each block:

volto/src/components/manage/Form/BlocksToolbar.jsx

Lines 96 to 99 in a7e3e08

	const blockConfig = config.blocks.blocksConfig[blockData['@type']];
	return mode === 'copy'
	? blockConfig.cloneData
	? blockConfig.cloneData(blockData)

[ksuess]

@tiberiuichim The responsibility to fix of data.block could be forwarded to blockConfig.cloneData. But I think it must be done in any case, so I think the fix can be done after blockConfig.cloneData / default cloning.

[tiberiuichim]

@ksuess I'm not sure I understand your fix.

The block id doesn't belong together with the block data, so my idea for a fix would be to make sure that the listing block always generates a new block id, in its cloneData method.

[ksuess]

@tiberiuichim What I don't see, why the blocks data has a block attribute.

    "blocks": {
      "8d726b48-f73b-4fcf-b851-7590bdc3e35e": {
        "@type": "listing",
        "block": "8d726b48-f73b-4fcf-b851-7590bdc3e35e",
        "headline": "Anleitungen",
        "headlineTag": "h2",
        "query": "[]",
        "querystring": "{query: Array(2), sort_order: \"ascending\"}",
        "variation": "default"
      },

[stevepiercy]

Just to be clear, every block always has a UUID.

A listing block has an additional attribute of "block": "some-UUID". I don't understand why it is present either.

      "8d726b48-f73b-4fcf-b851-7590bdc3e35e": { <-- Block UUID
        "@type": "listing",
        "block": "8d726b48-f73b-4fcf-b851-7590bdc3e35e", <-- WAT?

[ksuess]

Just to be clear, every block always has a UUID.

A listing block has an additional attribute of "block": "some-UUID". I don't understand why it is present either.

      "8d726b48-f73b-4fcf-b851-7590bdc3e35e": { <-- Block UUID
        "@type": "listing",
        "block": "8d726b48-f73b-4fcf-b851-7590bdc3e35e", <-- WAT?

Yes, exactly, that's what I stumbled upon. Would like to understand the need for the data.block. But anyway, eliminating would mean to carfully check, where this extra is taken into account. I would call eliminating this additional attribute a breaking change. So for me it's OK to correct the value of data.block on pasting a new listing block (the PR code does the fix for blocks with data.block in general, independent on the block type).

[tiberiuichim]

I think having the data.block uid stored in the block data is a bug. The block's UID should not be stored in the data. It probably got there by wrongly spreading some props into the data.

[tiberiuichim]

I can see that it's specifically inserted here:

volto/src/components/manage/Blocks/Listing/Edit.jsx

Line 38 in a7e3e08

block,

Why?!

[tiberiuichim]

@ksuess the useEffect from

volto/src/components/manage/Blocks/Listing/Edit.jsx

Lines 33 to 42 in a7e3e08

	React.useEffect(() => {
	if (!data.query) {
	onChangeBlock(block, {
	...data,
	query: [],
	block,
	});
	}
	/* eslint-disable react-hooks/exhaustive-deps */
	}, []);

should be completely removed, I've just tested and things seem to work. We have another framework for block default values, anyway.

[ksuess]

@ksuess the useEffect from

volto/src/components/manage/Blocks/Listing/Edit.jsx

Lines 33 to 42 in a7e3e08

	React.useEffect(() => {
	if (!data.query) {
	onChangeBlock(block, {
	...data,
	query: [],
	block,
	});
	}
	/* eslint-disable react-hooks/exhaustive-deps */
	}, []);

should be completely removed, I've just tested and things seem to work. We have another framework for block default values, anyway.

Well now data.block uid can be removed from listing block. I finally found a way to modify withQuerystringResults to work without having block uid in data available.

[tiberiuichim]

I saw. Awesome!

[sneridagh]

I don't recall now why the listing block is persisting the block prop... probably for historical reasons, maybe coming from the era where we didn't have a clear block interface.