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

feat: use the inferQueryID feature of search-insights #180

Merged
merged 12 commits into from
Jan 24, 2025

Conversation

samyj-alg
Copy link
Collaborator

@samyj-alg samyj-alg commented Jul 29, 2024

Since v2.15.0, search-insights can keep track of the queryID automatically.
This PR moves the SFRA cartridge to this new built-in method.

Changes

  • Upgrade Algolia libraries
  • Clean up the former manual mechanism using URL parameters
  • Cleans up the existing mechanism to keep track of queryIDs
  • Use the new inferQueryID parameter
  • Set userHasOptedOut when users have refused the tracking. This is because search-insights.js now systematically uses the browser's local storage to keep track of queryIDs, and since it's not technically necessary data, we need users' consent

@htuzel htuzel force-pushed the feature/use-inferred-insights-data branch from e52e642 to b908ba9 Compare July 30, 2024 12:41
Refactor the code in `recommend-config.js` to use the `URL` constructor instead of the `generateProductUrl` function for generating URLs. This change ensures that the URLs are correctly generated with the queryID and indexName parameters for analytics. Additionally, it improves the performance by using the `href` property of the `URL` object instead of concatenating strings.
queryID: item.__queryID,
indexName: item.__indexName,
});
item.url = new URL(item.url, window.location.origin).href;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not needed, it's done automatically by the browser, I'm removing it.

sbellone
sbellone previously approved these changes Aug 6, 2024
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

Nice cleanup 🙌

@sbellone sbellone self-requested a review August 6, 2024 09:22
@htuzel
Copy link
Collaborator

htuzel commented Aug 6, 2024

Thank you for the improvement

@sbellone sbellone dismissed their stale review August 6, 2024 10:53

Need to double check about user tracking

@sbellone
Copy link
Collaborator

I'm still tracking the subject.
For visibility:

  • I had confirmation that search-insights v2.15.0 systematically uses browser's storage to store queryIDs, without any way to opt-out
  • v2.17.2 introduces a way to opt-out
  • The frontend libraries are still not using this version, I'm waiting for an update

@sbellone
Copy link
Collaborator

sbellone commented Oct 9, 2024

Actually, the frontend libraries don't need to be updated, as they use the version that we load manually, so we're good on the 3rd point.

The problem now comes from the 2nd point: opting out is done with the userHasOptedOut flag. But when set to true, everything is disabled: local storage is not used anymore but events are not sent anymore either.
We need and intermediate state: opting-out from using localStorage but still be able to send events.

@sbellone sbellone changed the title Update libraries to use the new queryID inference insights feature feat: use the new inferQueryID insights feature Jan 22, 2025
@sbellone sbellone changed the title feat: use the new inferQueryID insights feature feat: use the inferQueryID feature of search-insights Jan 23, 2025
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

I've confirmed with the Events team that it's the expected behaviour and that they won't make further changes for now. Let's merge then!

So userHasOptedOut is now set to true when the user opt-out of tracking, which permits to not use his browser's localStorage in that case (the consequence being that no insights events are sent for those users)

@sbellone sbellone merged commit 96f3f2d into develop Jan 24, 2025
5 checks passed
@sbellone sbellone deleted the feature/use-inferred-insights-data branch January 24, 2025 16:22
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