segment-integrations / analytics.js-integration-kissmetrics

The Kissmetrics analytics.js integration.
https://segment.com/docs/integrations/kissmetrics/
MIT License
0 stars 2 forks source link

Always use the provided page URL and referrer #15

Closed chriso closed 8 years ago

chriso commented 8 years ago

We've had a customer wanting to track AJAX page loads. They pass details to page(), but our internal KM.pageView() function records a Page View event with the window URL as a property rather than the URL of the specified page.

The KM.pageView() function is defined as follows:

KM.pageView = function() {
  window._kmq.push(['record', 'Page View', {
    'Viewed URL': KM.u(),
    'Referrer': KM.rf()  || 'Direct'
  }]);
}

This PR inlines and then modifies the page view tracking code so that we send the correct details.

cc @salomon

chriso commented 8 years ago

Any update on this?

hankim813 commented 8 years ago

@chriso sorry for the late response. I left a comment. We're currently undergoing a big rollout for our newest version of analytics.js. The rollout should be happening throughout this week. But we can definitely prep this PR so that once the migration is fully complete, we can ship these right away.

I'd expect ETA of sometime next week!

codecov-io commented 8 years ago

Current coverage is 98.42%

Merging #15 into master will increase coverage by 0.01%

@@             master        #15   diff @@
==========================================
  Files             1          1          
  Lines           126        127     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            124        125     +1   
  Misses            2          2          
  Partials          0          0          

Powered by Codecov. Last updated by 9812c5f...d9a4126

chriso commented 8 years ago

@hankim813 I've rebased and added an additional test case. Let me know if you need anything else.

hankim813 commented 8 years ago

@chriso LGTM!

Just to reiterate, going to merge this now but we won't ship the new release including these changes until we finish the rollout of our ajs which hopefully should be done by end of this week or early next week.

I'll definitely add a note on my calendar to make sure I address KM once we complete the rollout!

Thanks again for the PR :) 💯

chriso commented 8 years ago

going to merge this now but we won't ship the new release including these changes until we finish the rollout of our ajs which hopefully should be done by end of this week or early next week.

No problem, and thanks!

chriso commented 8 years ago

@hankim813 can you let us know once analytics.js+this has been released?

hankim813 commented 8 years ago

@chriso absolutely! We're at the last leg of the migration so I'm hoping we can ship all these changes first thing Monday barring any setbacks! Definitely will ping on this thread 👍