-
Notifications
You must be signed in to change notification settings - Fork 116
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
Update search-insights and compatibility #1129
Conversation
…compatible with changes
@damcou When you have a moment, can you review the order conversion part of this item? |
|
||
var sources = analyticsHelper.sources; | ||
var source = sources.find(function(e) { | ||
return e.name == dataset; |
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'm not sure about this line.
dataset is an integer, so it should be something like sources[dataset -1]
my bad, it's correct, but only when .name is defined. We should throw and error when it's not I believe, it will save us time debugging :D
return e.name == dataset; | ||
}); | ||
|
||
trackClick(source.indexName, suggestion.objectID, suggestion.__position, suggestion.__queryID); |
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.
source is the result of find
, so it can be undefined. It's best to check before trying to access .indexName
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've switched it to use filter
instead which should return an array. We can check if filter returns a count before accessing the source.
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.
Looks good to me 👍
I don't really know how it should behave when the quote item is merged or updated, do we need to change the value of the tracking ? For example I don't remember if the add_to_cart event is triggered if you update the quantity or if the item is merged from an existing quote.
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 👍
* Apply ruleContexts in autocomplete (#1133) * Update search-insights and compatibility (#1129) * Admin indexing queue remove uppercase on renderer's column (#1132) * GA configuration update the depends node to the correct field name (#1143) * remove ordered_qty as a default option for product attr and ranking (#1142) * Update CMS Page Indexing (#1141)
Updated search-insights.js version and updated Click Analytics to make it compatible with new changes. This will allow for Personalization tracking with the updated version.