Skip to content

fix(insights): pass clickAnalytics automatically#1080

Merged
Haroenv merged 7 commits intonextfrom
wip/insights-auto-clickAnalytics
Feb 2, 2023
Merged

fix(insights): pass clickAnalytics automatically#1080
Haroenv merged 7 commits intonextfrom
wip/insights-auto-clickAnalytics

Conversation

@Haroenv
Copy link
Copy Markdown
Contributor

@Haroenv Haroenv commented Feb 1, 2023

Summary

adds a new mechanism in preResolve to check the context for any extra query parameters to set. The insights plugin will set clickAnalytics: true as one of those parameters to apply by default.

To debate: should clickAnalytics be true for facet searches? We don't send the same automatic events if the items don't have objectID (as click is different). I'm guessing we can ignore that, as searchForFacetValues doesn't appear in analytics.

To debate: should the key be queryParameters, searchParameters or query.params?

Results

We automatically pass clickAnalytics: true parameter if the insights plugin is used.

FX-2191

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Feb 1, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 68a9008:

Sandbox Source
@algolia/autocomplete-example-github-repositories-custom-plugin Configuration
@algolia/autocomplete-example-instantsearch Configuration
@algolia/autocomplete-example-playground Configuration
@algolia/autocomplete-example-preview-panel-in-modal Configuration
@algolia/autocomplete-example-react-renderer Configuration
@algolia/autocomplete-example-starter-algolia Configuration
@algolia/autocomplete-example-starter Configuration
@algolia/autocomplete-example-reshape Configuration
@algolia/autocomplete-example-vue Configuration

@Haroenv Haroenv marked this pull request as ready for review February 2, 2023 15:04
Copy link
Copy Markdown
Contributor Author

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

things that could be changed

params: expect.objectContaining({ clickAnalytics: true }),
}),
expect.objectContaining({
params: expect.objectContaining({ clickAnalytics: true }),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this behaviour expected (facet search with clickAnalytics: true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like no queryID is returned anyway, it's likely ignored by the engine

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to filter it out to avoid sending an unnecessary larger payload?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to prevent wrong clickAnalytics, you don't want to say the search is tracked, when there's no clicks happen

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed and this is unlikely, and we'll deal with it when there ever is a queryID in sffv

@Haroenv Haroenv requested a review from FabienMotte February 2, 2023 16:31
params: expect.objectContaining({ clickAnalytics: true }),
}),
expect.objectContaining({
params: expect.objectContaining({ clickAnalytics: true }),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to filter it out to avoid sending an unnecessary larger payload?

@Haroenv Haroenv enabled auto-merge (squash) February 2, 2023 17:44
@Haroenv Haroenv merged commit 8048442 into next Feb 2, 2023
@Haroenv Haroenv deleted the wip/insights-auto-clickAnalytics branch February 2, 2023 17:50
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.

3 participants