implement conversion tracking service #29
Conversation
…pt and test with test-site
| async trackConversion(event: ConversionEvent): Promise<void> { | ||
| const url = new URL(`https://${DEFAULT_CONVERSION_TRACKING_DOMAIN}/${conversionEndpoint}`); | ||
| const params = new URLSearchParams(); | ||
| params.set('cid', event.cid); |
There was a problem hiding this comment.
A note, CID id a required parameter if we want so the server will 400 if this doesn't exist.
There was a problem hiding this comment.
yes, it's marked required on the ConversionEvent interface
| } | ||
|
|
||
| /** {@inheritDoc ConversionTrackingService.trackConversion} */ | ||
| async trackConversion(event: ConversionEvent): Promise<void> { |
There was a problem hiding this comment.
Note we also need to pass a location field for the conversion event as well(its should be the same as the listings location param(currently for ytag it comes from window.location.href)
There was a problem hiding this comment.
I didn't see that on the required param for the conversion event itself, just the listingsclickevent in the docs, it's not even listed as an optional parameter on the conversion event: https://www.yext.com/docmd/docs/src/com/yext/analytics/events/conversions.html
There was a problem hiding this comment.
Ah yea location is a new param we added in the last few months for conversions(its not required for the event to work though). Its mainly to support our new analytics events pipeline. I'll update those docs.
| } | ||
|
|
||
| /** {@inheritDoc ConversionTrackingService.trackListings} */ | ||
| async trackListings(event: ListingsClickEvent): Promise<void> { |
There was a problem hiding this comment.
Curious, how is this expected to be used? I'm reading through the ytag code and the listings stuff has some special logic where it only fires once upon load.
There was a problem hiding this comment.
So some of the borders of this library are a bit weird because it's meant to be useable server side. I need to think about the pageView method and what it should provide implementers to tell them to remove the listings parameter from the pageurl if necessary. Any thoughts? I can pull the listings crediting out of the pageView method so that it's entirely on the implementer to do and add that logic into yext/pages (seems like that might be best). I could also add something to utils for implementers to use to manage that state
There was a problem hiding this comment.
Oh i see the code in PagesAnalyticsReporter where it does the only fire once logic. I think we might actually be ok with what logic already exists there, it seems that on the ytag implementation refreshing the page would cause the listings event to fire again.
Up to you if you want to move it out right now, think it should be ok.
There was a problem hiding this comment.
oh cool I was just looking at ytag and hadn't seen your comment and yeah I agree it doesn't seem to handle page refreshes. I can add a TODO to handle that in the future
| async trackListings(event: ListingsClickEvent): Promise<void> { | ||
| const url = new URL(`https://${DEFAULT_CONVERSION_TRACKING_DOMAIN}/${listingsEndpoint}`); | ||
| const params = new URLSearchParams(); | ||
| params.set(LISTINGS_SOURCE_PARAM, event.source); |
There was a problem hiding this comment.
Also note, /listings will throw a server error if y_source is not set
There was a problem hiding this comment.
it's required by the ListingsClickEvent
| } | ||
| } | ||
|
|
||
| private static formatBaseEvent(event: CommonConversionData, params: URLSearchParams): void { |
There was a problem hiding this comment.
Are we only dealing with first party cookies for sites?
There was a problem hiding this comment.
after discussing with @maperalta the cookieId is either the first or the third party cookie. This library doesn't send anything in the headers as it doesn't have direct access to the Document, which is why it seems a bit weird.
If you look at CookieManager in utils that has the cookie retrieval logic from ytag and in yext/pages I'm using it like this:
enableConversionTracking(): void {
if (this.canTrack()) {
this._cookieManager = new CookieManager();
this._analyticsReporter?.setConversionTrackingEnabled(
true,
this._cookieManager.setAndGetYextCookie(),
);
}
}
There was a problem hiding this comment.
let me also tag a WIP PR for yext/pages so you can see the implementation on the front end
There was a problem hiding this comment.
Adds Conversion Tracking to the Analytics Library, including: