Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

Feature/page view adding segment properties#6

Open
wethu wants to merge 3 commits intosegment-integrations:masterfrom
getvero:feature/page_view_adding_segment_properties
Open

Feature/page view adding segment properties#6
wethu wants to merge 3 commits intosegment-integrations:masterfrom
getvero:feature/page_view_adding_segment_properties

Conversation

@wethu
Copy link

@wethu wethu commented Mar 20, 2017

Hey People,

We're changing the properties tracked along with page views. I have some implementation here which works, however I'm having trouble stubbing the properties passed into page.

Thanks!

Copy link
Contributor

@hankim813 hankim813 left a comment

Choose a reason for hiding this comment

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

@wethu Cool! Thanks for the PR. I'm guessing this is all backward compatible, augmentative data for our mutual customers (so we can just roll out to everyone at once)?

it('should push "viewed_page"', function() {
analytics.page();
analytics.called(window._veroq.push, ['trackPageview']);
analytics.called(window._veroq.push, ['track', 'viewed_page', {}, { source: 'segment' }]);
Copy link
Contributor

Choose a reason for hiding this comment

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

page.properties() returns the current page call's properties object. By default our .page() call collects some context page metadata:

{
	      "path": "/context.html",
	      "referrer": "http://localhost:9876/?id=70213249",
	      "search": "",
	      "title": "",
	      "url": "http://localhost:9876/context.html"
	    },

☝️ is what gets set inside the test suite. So you should change the analytics.called() method to include these properties!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for that,

I was able to fix the problem by passing in the referrer to #page(), this means the example will always have the same id parameter.

Let me know if theres a better way to accomplish this!

@codecov-io
Copy link

codecov-io commented Apr 3, 2017

Codecov Report

Merging #6 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #6   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          28     28           
=====================================
  Hits           28     28
Impacted Files Coverage Δ
lib/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 103b482...8e0e857. Read the comment docs.

@wethu
Copy link
Author

wethu commented Apr 3, 2017

@hankim813 Thanks for the review!

I'm currently triple checking with our Project Manager about the backwards compatibility aspect. And will follow up soon!

Thanks again :)

@SegmentDestinationsBot
Copy link
Contributor

Hi @wethu, as part of the monorepo migration, this issue has been moved to new issue. Our engineers have been notified and will prioritize and work on it ASAP. Thank you!

For more information, see README.md.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

migrated The issue has been migrated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants