Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix search block in edit mode re-queries multiple blocks with an empty search text #4694

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

reebalazs
Copy link
Member

@reebalazs reebalazs commented Apr 13, 2023

Multiple issues fixed:

Fix search block search text re-query

SearchBlockEdit calls onTriggerSearch without parameters. But
onTriggerSearch does not ignore toSearchText in case it's not specified
in the call.

If there are multiple blocks in the page, this causes the search text
of all the other block's reset and re-queried with the wrong condition.

Performance optimize onTriggerSearch

Previous to this fix, onTriggerSearch triggered 28 times during loading
and edit page with two search blocks in it. After the fix, it's just 2
times.

The fix is that the deep comparison condition for the query also has to
be applied in withSearch.jsx. Without that, the deep comparison in
SearchBlockEdit.jsx has no effect. This is because the onTriggerSearch
function will be regenerated each time there is an identity change in
data.query anyway. So the hook guard in SearchBlockEdit will be
triggered anyway because onTriggerSearch has changed.

In addition, I personally find it better to use JSON.stringify for the
deep comparison and skip using the useDeepCompareEffect hook. This uses
dequal under the hood which is capable to compare a lot of other data
types, but with pure json data it has no advantage to a string
comparison with JSON.stringify. (That said it would also work with
useDeepCompareEffect but perhaps a bit slower.)

Remove SearchableText from the query only if there is a searchtext

Previously, the SearchableText was removed within normalizeState, even
if there was no searchText. This is wrong, the searchText should only
replace SearchableText condition if searchText is provided.

In the edit page this caused the following, in case a SearchableText
is specified as a condition to the block:

  • Upon loading the edit page
  • First the search blocks were loaded with correct results
  • Then a second set of query was issued, but with the SearchableText
    condition stripped out. This means that the query result was wrong.

The current fix fixes the double querying, and the faulty condition in
the second query that results in faulty results.

The current fix still does not address the fact that eventually the
searchText should be a distinct facet from SearchableText, and the two
conditions could be combined, in comparison to the current state, when
one overrides the other.

@reebalazs reebalazs requested a review from sneridagh April 13, 2023 17:43
@netlify
Copy link

netlify bot commented Apr 13, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 8d94359
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/643927b41ee78d00081cf63a

@cypress
Copy link

cypress bot commented Apr 13, 2023

Passing run #4935 ↗︎

0 489 20 0 Flakiness 0

Details:

Add changelog
Project: Volto Commit: 8d94359923
Status: Passed Duration: 12:41 💡
Started: Apr 14, 2023 10:18 AM Ended: Apr 14, 2023 10:31 AM

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

@reebalazs
Copy link
Member Author

Fixes #4697

@reebalazs reebalazs changed the title WIP fix search block search text requery Fix search block in edit mode re-queries multiple blocks with an empty search text Apr 14, 2023
SearchBlockEdit calls onTriggerSearch without parameters. But
onTriggerSearch does not ignore toSearchText in case it's not specified
in the call.

If there are multiple blocks in the page, this causes the search text
of all the other block's reset and re-queried with the wrong condition.
Previous to this fix, onTriggerSearch triggered 28 times during loading
and edit page with two search blocks in it. After the fix, it's just 2
times.

The fix is that the deep comparison condition for the query also has to
be applied in withSearch.jsx. Without that, the deep comparison in
SearchBlockEdit.jsx has no effect. This is because the onTriggerSearch
function will be regenerated each time there is an identity change in
data.query anyway. So the hook guard in SearchBlockEdit will be
triggered anyway because onTriggerSearch has changed.

In addition, I personally find it better to use JSON.stringify for the
deep comparison and skip using the useDeepCompareEffect hook. This uses
dequal under the hood which is capable to compare a lot of other data
types, but with pure json data it has no advantage to a string
comparison with JSON.stringify. (That said it would also work with
useDeepCompareEffect but perhaps a bit slower.)
@reebalazs reebalazs force-pushed the ree-fix-search-block-search-text-requery branch from 812d437 to c0aa433 Compare April 14, 2023 08:16
@reebalazs reebalazs changed the title Fix search block in edit mode re-queries multiple blocks with an empty search text WIP Fix search block in edit mode re-queries multiple blocks with an empty search text Apr 14, 2023
Previously, the SearchableText was removed within normalizeState, even
if there was no searchText. This is wrong, the searchText should only
replace SearchableText condition if searchText is provided.

In the edit page this caused the following, in case a SearchableText
is specified as a condition to the block:

- Upon loading the edit page
- First the search blocks were loaded with correct results
- Then a second set of query was issued, but with the SearchableText
  condition stripped out. This means that the query result was wrong.

The current fix fixes the double querying, and the faulty condition in
the second query that results in faulty results.

The current fix still does not address the fact that eventually the
searchText should be a distinct facet from SearchableText, and the two
conditions could be combined, in comparison to the current state, when
one overrides the other.
@reebalazs reebalazs force-pushed the ree-fix-search-block-search-text-requery branch from c0aa433 to 8d94359 Compare April 14, 2023 10:15
@reebalazs reebalazs changed the title WIP Fix search block in edit mode re-queries multiple blocks with an empty search text Fix search block in edit mode re-queries multiple blocks with an empty search text Apr 14, 2023
@sneridagh sneridagh merged commit fa2b6d4 into master Apr 14, 2023
@sneridagh sneridagh deleted the ree-fix-search-block-search-text-requery branch April 14, 2023 10:42
sneridagh pushed a commit that referenced this pull request Apr 14, 2023
sneridagh added a commit that referenced this pull request Apr 14, 2023
…y search text (#4694) (#4700)

Co-authored-by: Balázs Reé <ree@greenfinity.hu>
sneridagh added a commit that referenced this pull request Apr 17, 2023
* master: (22 commits)
  Release changelog notes for 16.20.1
  Release 17.0.0-alpha.5
  Generate a split sitemap (also fix robots.txt) (#4639)
  Fix search block in edit mode re-queries multiple blocks with an empty search text (#4694)
  Fix Move to top of folder ordering in folder content view (#4691)
  Changelog
  Revert "Add current page parameter to the route in the listing and search block pagination (#4159)" (#4695)
  Release generate-volto 7.0.0-alpha.4
  Force the resolution of the `react-error-overlay` package to `6.0.9` (#4687)
  Fix training links (#4635)
  Release 17.0.0-alpha.4
  Release changelog notes for 16.20.0 (#4684)
  Update to latest backend versions (#4682)
  Support RelationList field with StaticCatalogVocabulary and SelectWidget. (#4614)
  Load a theme via a `theme` key in `volto.config.js` or in `package.json` (#4625)
  docs: improve creating view documentation (#4636)
  fix sitemap.xml.gz is not compressed #4622 (v2) (#4663)
  Make URL a literal string to fix broken link (#4667)
  Move developer guidelines to contributing #4665 (#4666)
  Update Volto contributing to align with and refer to the new Plone co… (#4634)
  ...
sneridagh added a commit that referenced this pull request May 10, 2023
* master: (83 commits)
  Apply suggestion from browser for password field (#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (#4742)
  added documentation regarding the static middleware #4518 (#4736)
  Closes issue #4567 (#4570)
  Fix whitespace in locales created by the generator (#4737)
  Tidy up from synch with 16.x.x (#4728)
  Release notes from 16.20.2, 16.20.3, 16.20.4 (#4729)
  Use new URL 6.docs.plone.org (#4726)
  Security upgrade for momentjs (#4715)
  Don't decode querystring while adding apiExpanders (#4719)
  (Synchronize redundant block id in listing block on pasting) Fix duplicating listing block (#4239)
  Translate add-on control panel (cleaned up PR) (#4620)
  Fix Move to top of folder ordering in folder content view by searchin… (#4709)
  Fix robot.txt - the sitemap link should respect x-forwarded headers (#4704)
  Refactor faulty reorder elements in ObjectBrowserList widget (#4703)
  Release changelog notes for 16.20.1
  Release 17.0.0-alpha.5
  Generate a split sitemap (also fix robots.txt) (#4639)
  Fix search block in edit mode re-queries multiple blocks with an empty search text (#4694)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants