-
Notifications
You must be signed in to change notification settings - Fork 66
Fix default zero facets on placeholdersearch and routing #830
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
Conversation
| const response2 = await searchClient.search([ | ||
| { | ||
| indexName: 'movies', | ||
| params: { | ||
| facets: ['genres'], | ||
| query: 'no results', | ||
| }, | ||
| }, | ||
| ]) | ||
|
|
||
| expect(response2.results[0].facets).toEqual({ | ||
| genres: { | ||
| Action: 0, | ||
| Adventure: 0, | ||
| Animation: 0, | ||
| Comedy: 0, | ||
| Crime: 0, | ||
| Drama: 0, | ||
| 'Science Fiction': 0, | ||
| Thriller: 0, | ||
| }, | ||
| }) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this, because you have the same setup/exercise in the test below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually a difference for me!
The first tests:
- Makes a search with no query
- Should cache the default distribution correctly
- Makes a search with the same
searchClientas before but with a query that would return 0 documents - Checks if every facet is correctly set to 0
Second test:
- Does a search with a query that would return 0 documents
- Should cache the default distribution when none has been set before
- Checks if every facet is correctly set to 0
There is a cache system between the multiple calls on the same search client. These two tests ensure they are helping each other.
The tests that ensure that when doing two call on my search client only triggers the default facetdistribution call only once is tested here: #830 (comment). In this test we are calling two time searchClient.search. During the first search we make two requests to meilisearch, one for the default facet distribution, one for the actual search. During the second search on the same searchClient the default facet distribution should already be cached and only the actual search request should be made, thus 3 calls.
| clientAgents: [`Meilisearch instant-meilisearch (v${PACKAGE_VERSION})`], | ||
| }) | ||
| expect(mockedSearch).toHaveBeenCalledTimes(2) | ||
| expect(mockedSearch).toHaveBeenCalledTimes(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have +1 calls now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the first call always sets up the default facet distribution by making a call with zero filters (except for the facet field) to ensure we have the facet distribution of an empty search not impacted by any filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #830 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the scenario tested in the tests:
All of them are possible scenarios that were causing bugs and that I want to ensure are working correctly now
// Without facets
// Without keepZeroFacets
// With placeholderSearch
// With empty query
// Without facets
// With keepZeroFacets
// With placeholderSearch
// With empty query
// With facets
// without keepZeroFacets
// without placeholderSearch
// With multiple search (empty and no results expected)
// With facets
// without keepZeroFacets
// with placeholderSearch
// With empty query
// With facets
// with keepZeroFacets
// with placeholderSearch
// With multiple search (empty and no results expected)
// With facets
// with keepZeroFacets
// without placeholderSearch
// With multiple search (empty and no results expected)
// With facets
// with keepZeroFacets
// with placeholderSearch
// Without multiple search
// Without query expecting no results
mdubus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
brunoocasali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
|
bors merge |
fixes: #797
Maybe related: #776
Maybe related: #825
keepZeroFacetswas not working whenplaceholderSearchwas set to false and when using the routing system.This PR fixes the issue.