feat: allow queries preview with same query but different filters or params #1392

lauramargar · 2024-01-15T08:13:47Z

Pull request template

In the current implementation, we are only saving queries. As a second step, we should also support filters and extraparams, but then we should save the hash of query+filters.

We have used js-md5 hash to make an id to differentiate the same queries but with different filters applied.

There was a problem when dealing with multiple instances of the QueryPreview component with the same query hash. In that case, the order of mounting and unmounting the components matter and it could lead to a scenario where a component is mounted but afterwards, the other one with the same query hash is unmounted and both of them will be gone from the DOM.

We have applied the following SOLUTION:

Refactor the events emitted from QueryPreview. Remove NonCacheableQueryPreviewUnmounted and add QueryPreviewMounted: string; QueryPreviewUnmounted: { query: string, cache: boolean };

Enhance the QueryPreviewItem from the state with a field

  /** The number of instances showing the query preview*/
  instances: number;

The fetchAndSaveQueryPreview action will set the instances field to 1 since it will be the first instance of a QueryPreview using this query hash.

Every QueryPreview instance when is created and picks the data from the cache will emit QueryPreviewMounted so the instances count is increased. Every instance of QueryPreview will emit QueryPreviewUnmounted so the instances count for that query hash will be decreased and if the cache is set to false, then the query will be removed safely from the state since there will be no component showing that data.

Motivation and context

Dependencies. If any, specify:
Open issue. If applicable, link: EMP-3079

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that causes existing functionality to not work as expected)
Change requires a documentation update

What is the destination branch of this PR?

Main
Other. Specify:

How has this been tested?

You can test 2 scenarios:

Same query + different filters
Same query in different instances

In scenario 1 --> you can add in Home.vue file this new query preview in queriesPreviewInfo :

{
  query: 'summer dress',
  filters: ['brand:marni']
}

You should see 2 carrousels with 'summer dress' as the title but with different results.

In scenario 2 --> add in the same place as the previous scenario the following query preview:

{
  query: 'charger'
}

Then search for battery in the search box. This query has the query charger as a semantic query.
As we had saved charger in the state from the brand recommendations, in semantic queries should be loaded from the state instead of being requested again.

Once we clear the query in the search box, we should see that charger keeps saved in the query preview state and is displayed as a brand recommendation.

Checklist:

My code follows the style guidelines of this project.
I have performed a self-review on my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have added tests that prove my fix is effective or that my feature works.
New and existing unit tests pass locally with my changes.

…w-queries-preview-with-same-query-but-different-filters-or-params

…same-query-but-different-filters-or-params

EMP-3079

annacv

Also tested with:{ query: 'wallet', filters: ['brand:valentino']}, as a queryPreview + searching for charger to check that Semantic Queries will show results for the query wallet without being filtered.

packages/x-components/src/x-modules/queries-preview/components/query-preview.vue

packages/x-components/src/x-modules/queries-preview/events.types.ts

packages/x-components/src/x-modules/queries-preview/utils/get-hash-from-query-preview.ts

…same-query-but-different-filters-or-params

CachedaCodes · 2024-01-24T11:43:45Z

packages/x-components/src/x-modules/queries-preview/components/query-preview.vue

+     *
+     * @internal
+     */
+    public queryOfQueryPreviewHash = getHashFromQueryPreviewInfo(this.queryPreviewInfo);


Rename to queryPreviewHash

CachedaCodes · 2024-01-24T11:44:30Z

packages/x-components/src/x-modules/queries-preview/components/query-preview.vue

+     *
+     * @internal
+     */
+    public queryOfQueryPreviewHash = getHashFromQueryPreviewInfo(this.queryPreviewInfo);


This uses a dynamic value to generate a non-dynamic value. Make it a computed property.

CachedaCodes · 2024-01-24T11:48:48Z

packages/x-components/src/x-modules/queries-preview/events.types.ts

+  /**
+   * The query preview has been mounted.
+   * Payload: The query preview query as a key converted into a unique id (query hash).
+   * When QueryPreviewMounted is emitted, the instances count is increased.


No need to explain this here. What the module does when the event fires is not relevant here.

CachedaCodes · 2024-01-24T11:48:56Z

packages/x-components/src/x-modules/queries-preview/events.types.ts

+   * When QueryPreviewUnmounted is emitted, the instance count for that query hash
+   * will be decreased. If the cache is set to false, the query will be removed
+   * safely from the state since there will be no component showing that data.


Same as above

CachedaCodes · 2024-01-24T11:53:06Z

packages/x-components/src/x-modules/queries-preview/store/module.ts

@@ -41,7 +42,21 @@ export const queriesPreviewXStoreModule: QueriesPreviewXStoreModule = {
      state.selectedQueryPreview = selectedQueryPreview;
    },
    setConfig,
-    mergeConfig
+    mergeConfig,
+    addQueryPreviewInstance(state, query: string) {


This is not the query, is the queryPreviewHash

CachedaCodes · 2024-01-24T12:26:47Z

packages/x-components/src/x-modules/queries-preview/utils/get-hash-from-query-preview.ts

+  let queryPreviewFilters = '';
+  if (queryPreviewInfo.filters) {
+    queryPreviewInfo.filters.forEach(filter => {
+      queryPreviewFilters = queryPreviewFilters.concat('-' + filter);
+    });
+  }


You can refactor to this:

Suggested change

let queryPreviewFilters = '';

if (queryPreviewInfo.filters) {

queryPreviewInfo.filters.forEach(filter => {

queryPreviewFilters = queryPreviewFilters.concat('-' + filter);

});

}

const queryPreviewFilters = queryPreviewInfo.filters ? queryPreviewInfo.filters.join('-') : '';

CachedaCodes · 2024-01-24T12:39:20Z

packages/x-components/src/x-modules/queries-preview/utils/get-hash-from-query-preview.ts

+  let queryPreviewFilters = '';
+  if (queryPreview.request.filters) {
+    const queryPreviewFiltersKeys = Object.keys(queryPreview.request.filters);
+    queryPreviewFiltersKeys.forEach(key => {
+      queryPreview.request.filters![key].forEach(filter => {
+        queryPreviewFilters = queryPreviewFilters.concat('-' + filter.id.toString());
+      });
+    });
+  }


You can refactor this to:

Suggested change

let queryPreviewFilters = '';

if (queryPreview.request.filters) {

const queryPreviewFiltersKeys = Object.keys(queryPreview.request.filters);

queryPreviewFiltersKeys.forEach(key => {

queryPreview.request.filters![key].forEach(filter => {

queryPreviewFilters = queryPreviewFilters.concat('-' + filter.id.toString());

});

});

}

const queryPreviewFilters = Object.values(queryPreview.request.filters)

.flat()

.map(filter => filter.id.toString())

.join('-');

CachedaCodes · 2024-01-24T12:42:21Z

packages/x-components/src/x-modules/queries-preview/utils/__tests__/utils.spec.ts

Make the QP in the tests have filters. Half of the implementation is not being tested right now

CachedaCodes · 2024-01-24T12:43:29Z

packages/x-components/src/x-modules/queries-preview/components/query-preview-list.vue

-          this.queriesStatus[query] === 'success' || this.queriesStatus[query] === 'loading'
-      );
+      return this.queriesPreviewInfo.filter(item => {
+        const query = getHashFromQueryPreviewInfo(item);


Rename to queryPreviewHash

CachedaCodes · 2024-01-24T12:44:55Z

...s/x-components/src/x-modules/queries-preview/components/__tests__/query-preview-list.spec.ts

@@ -86,7 +86,9 @@ describe('testing QueryPreviewList', () => {
    expect(queryPreviews.at(1).text()).toEqual('jeans - Sick jeans');
  });

-  it('hides queries with no results', async () => {
+  // TODO Uncomment when the 'error' event is fixed


If there's a task already, add the task ID here

Reference task EMP-3402

CachedaCodes

Fix the conflicts with main too

CachedaCodes · 2024-01-25T07:14:10Z

packages/x-components/src/x-modules/queries-preview/store/module.ts

-    addQueryPreviewInstance(state, query: string) {
-      if (state.queriesPreview[query]) {
-        state.queriesPreview[query].instances += 1;
+    addQueryPreviewInstance(state, queryPreviewHash: string) {


No need to specify the type in queryPreviewHash: string, it's already being done in the types file, remove the string.

CachedaCodes · 2024-01-25T07:14:51Z

packages/x-components/src/x-modules/queries-preview/store/module.ts

-        state.queriesPreview[query].instances -= 1;
+    removeQueryPreviewInstance(
+      state,
+      { queryPreviewHash, cache }: { queryPreviewHash: string; cache: boolean }


Same here, you don't need the type in { queryPreviewHash, cache }: { queryPreviewHash: string; cache: boolean }, leave it as { queryPreviewHash, cache }

CachedaCodes · 2024-01-25T07:15:06Z

packages/x-components/src/x-modules/queries-preview/store/types.ts

@@ -111,8 +111,14 @@ export interface QueriesPreviewMutations extends ConfigMutations<QueriesPreviewS
   * @param selectedQueryPreview - The selected query preview to save to the state.
   */
  setSelectedQueryPreview(selectedQueryPreview: QueryPreviewInfo | null): void;
-  addQueryPreviewInstance(query: string): void;
-  removeQueryPreviewInstance({ query, cache }: { query: string; cache: boolean }): void;
+  addQueryPreviewInstance(queryPreviewHash: string): void;


CachedaCodes · 2024-01-25T07:15:13Z

packages/x-components/src/x-modules/queries-preview/store/types.ts

-  addQueryPreviewInstance(query: string): void;
-  removeQueryPreviewInstance({ query, cache }: { query: string; cache: boolean }): void;
+  addQueryPreviewInstance(queryPreviewHash: string): void;
+  removeQueryPreviewInstance({


CachedaCodes · 2024-01-25T07:23:21Z

...s/x-components/src/x-modules/queries-preview/components/__tests__/query-preview-list.spec.ts

@@ -86,7 +86,9 @@ describe('testing QueryPreviewList', () => {
    expect(queryPreviews.at(1).text()).toEqual('jeans - Sick jeans');
  });

-  it('hides queries with no results', async () => {
+  // TODO Uncomment when the 'error' event is fixed


Reference task EMP-3402

…same-query-but-different-filters-or-params # Conflicts: # pnpm-lock.yaml

lauramargar and others added 9 commits December 19, 2023 10:08

feat(query-preview): convert queriesPreview keys into hash keys

f37961f

chore: remove rollup from renovate restrictions

17f6daf

feat(query-preview): implementation with 2 lists

9b56106

Merge remote-tracking branch 'origin/main' into feature/EMP-3079-allo…

a4a0802

…w-queries-preview-with-same-query-but-different-filters-or-params

feat(query-preview): new implementation with an internal cache

9ecec58

draft QueryPreviewMounted and QueryPreviewUnmounted events

7da122b

fix(query-preview): component, actions and getters tests

143211f

fix(query-preview): fix tests and add documentation

52f2206

fix(query-preview): fix e2e tests

b7a39a2

lauramargar requested a review from a team as a code owner January 15, 2024 08:13

annacv and others added 6 commits January 15, 2024 12:05

Merge branch 'main' into feature/EMP-3079-allow-queries-preview-with-…

a2da941

…same-query-but-different-filters-or-params

update lockfile

0359041

doc: fix doc test typo

218bf6e

EMP-3079

doc: add query preview events types documentation

1d86255

EMP-3079

chore: rm unused constant

8c0b15a

EMP-3079

doc: add get-hash-from-query-preview.ts doc

2f1f995

EMP-3079

annacv previously approved these changes Jan 15, 2024

View reviewed changes

diegopf requested changes Jan 16, 2024

View reviewed changes

fix(query-preview): pr comments

11e0e76

lauramargar dismissed annacv’s stale review via 11e0e76 January 17, 2024 11:20

lauramargar and others added 2 commits January 17, 2024 13:58

Merge branch 'main' into feature/EMP-3079-allow-queries-preview-with-…

4ecb9a6

…same-query-but-different-filters-or-params

fix(query-preview): add a missing coma

7c4f689

CachedaCodes suggested changes Jan 24, 2024

View reviewed changes

lauramargar added 2 commits January 24, 2024 15:52

fix: pr comments

2782b4b

fix: QueryPreviewUnmounted payload

46fd292

CachedaCodes suggested changes Jan 25, 2024

View reviewed changes

lauramargar added 2 commits January 25, 2024 08:43

Merge branch 'main' into feature/EMP-3079-allow-queries-preview-with-…

c28dd0c

…same-query-but-different-filters-or-params # Conflicts: # pnpm-lock.yaml

fix: leak of doc and types

1f32b13

CachedaCodes approved these changes Jan 25, 2024

View reviewed changes

CachedaCodes merged commit 4c1f63a into main Jan 25, 2024
4 checks passed

CachedaCodes deleted the feature/EMP-3079-allow-queries-preview-with-same-query-but-different-filters-or-params branch January 25, 2024 08:54

annacv mentioned this pull request Feb 7, 2024

Query Previews stop requesting when one fails #1405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow queries preview with same query but different filters or params #1392

feat: allow queries preview with same query but different filters or params #1392

lauramargar commented Jan 15, 2024 •

edited by jira bot

Loading

annacv left a comment

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 24, 2024

CachedaCodes Jan 25, 2024

CachedaCodes left a comment

CachedaCodes Jan 25, 2024

CachedaCodes Jan 25, 2024

CachedaCodes Jan 25, 2024

CachedaCodes Jan 25, 2024

CachedaCodes Jan 25, 2024

feat: allow queries preview with same query but different filters or params #1392

feat: allow queries preview with same query but different filters or params #1392

Conversation

lauramargar commented Jan 15, 2024 • edited by jira bot Loading

Pull request template

Motivation and context

Type of change

What is the destination branch of this PR?

How has this been tested?

Checklist:

annacv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CachedaCodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauramargar commented Jan 15, 2024 •

edited by jira bot

Loading