Use default query in SSR rendering and client fetch of the listing block when we don't have a search criteria

[wesleybl]

Now we use getQueryStringResults on withQuerystringResults even when we have no search criteria.

The query used will have the same result as the items key of the content json.

Fix #3839

This also makes #3474 obsolete.

[netlify]

✅ Deploy Preview for volto canceled.

Name	Link
🔨 Latest commit	0f459fe
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/63dc049edf951100071cb8d8

[cypress]

Passing run #3803 ↗︎

0

459

20

0

0

Details:

Merge branch 'master' into listing-query
Project: Volto	Commit: 0f459fed33
Status: Passed	Duration: 13:58 💡
Started: Feb 2, 2023 6:48 PM	Ended: Feb 2, 2023 7:02 PM

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[wesleybl]

Can someone please take a look here?

@tiberiuichim @sneridagh @ksuess

[sneridagh]

@wesleybl sorry for not answering before, since I find it valuable, I think we should discuss it in the Volto Team meeting, since it changes completely the default behavior and this could break feature expectations in existing sites.

Next meeting is next Tuesday.

/cc @plone/volto-team Feedback?

[sneridagh]

@wesleybl also, it's a breaking change (and a bug fix) since it modifies default behavior. Also it differs from the default collections in Plone classic. Could you please note it accordingly in the changelog? Thanks!

[wesleybl]

since it changes completely the default behavior and this could break feature expectations in existing sites.

@sneridagh from the end user's point of view, I tried to do it in a way that doesn't change. The default query that I put, returns the same items that are returned today, when it has no criteria.

it's a breaking change (and a bug fix) since it modifies default behavior. Also it differs from the default collections in Plone classic. Could you please note it accordingly in the changelog?

I put the entry in Breaking.

[wesleybl]

@sneridagh has this been discussed?

[tisto]

@sneridagh @wesleybl I checked out the PR locally and I could not spot any difference in the editor UI:

The fix is definitely beneficial. If there is no user-facing change, we might get away without a breaking change. Do I understand correctly that we send a default query in case the user did not fill out anything to avoid the REST API error on the backend?

Sorry, I did not look into the code yet. Just did a quick superficial review.

[wesleybl]

Do I understand correctly that we send a default query in case the user did not fill out anything to avoid the REST API error on the backend?

Yes. We sent a default query to getAsyncData.js, avoiding the backend error. This query is the same used in withQuerystringResults.jsx when the user does not select a search criterion. And this default query returns the same result that the code without this PR returns today. So I don't think this will change the result of blocks that don't have a search criteria.

[wesleybl]

This PR fix yet another problem that occurs today. Today, if you create a listing block without filling in the search criteria and save, it will return the contents of the container. If you edit it again by selecting the search criteria and save, it will show the criteria result. But from then on, if you remove the search criteria, it no longer returns the contents of the container. It will return an empty list.

[tiberiuichim]

So, let's summarize what this PR is about:

With current volto, the listing block behaves like:

	SSR	clientside
no query	incorrectly calls getQuerystringResults	needesly calls getContent()
with query	calls getQuerystringResults	flashes content.items before showing querystringsearch results

With this PR, will always call getQuerystringResults. We lose the cool feature we already get the content items for free (which is a source of problems if they're not serialized in the same way).

In principle I would be ok with this change. It's just a matter of being clear about expected behavior.

@sneridagh , @plone/volto-team let's move forward with this PR. What do you guys think?

[tiberiuichim]

We need to document this better, including in the upgrade guide. If we move forward with this, we'd need an entry in the upgrade guide. I don't expect that any code change would be required in any project (after all, it would fix at least 2-3 bugs), but there may be some different behavior: the folder content items now receive more brain data, there's an extra request on all folder contents.

[wesleybl]

the folder content items now receive more brain data, there's an extra request on all folder contents.

@tiberiuichim current templates work fine with basic metadata. Then we can change it so that the queries don't fetch all the metadata. I was thinking of doing this in another PR

An extra request already occurs today, but for /++api++/ instead of @querystring-search

[tiberiuichim]

@wesleybl I think the plone.restapi serializer should be enhanced for the items, so that the children are summarized with the same serializer as catalog batch results. This would make sure that the result of content.items is the same as the querystringsearch.

I know that in default Volto this doesn't make a difference, but in projects we have the case where we use "enhanced result items", for example to show the tags. It confusing for the editors when they add a new listing block, they first set the variation and are surprised that they have results, but the information is not there.

[wesleybl]

I saw a problem with this implementation. When we are going to add new content, the initialPath will be the current folder, since the content we are adding has not yet been created. So, at that moment, the listed contents will be those of the current folder. But I would expect no content to be listed since the content has not yet been created. After saving, the content is created and initialPath will be the path of the new content. At this time, no content will be listed, as there are no created contents within the created content yet.

I tried to do a test to check if current URL ends with /add and not call the actions that do the search in that situation. But lint complained that the hooks calls were conditional, and that's not good.

Another option I see is to search for a URL that does not exist, for example for /undefined