Skip to content

[DEST-897] added check to unset params and ref on new session #194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 29, 2019
Merged

[DEST-897] added check to unset params and ref on new session #194

merged 5 commits into from
Aug 29, 2019

Conversation

@omegabytes
Copy link
Contributor

@omegabytes omegabytes commented Aug 21, 2019 •
edited
Loading

What does this PR do?
Adds check for unsetParamsReferrerOnNewSession

Are there breaking changes in this PR?
no

Any background context you want to provide?
Snippet from amplitude docs:

unsetParamsReferrerOnNewSession
If false, the existing referrer and utm_parameter values will be carried through each new session. If set to true, the referrerand utm_parameteruser properties, which include referrer, utm_source, utm_medium, utm_campaign, utm_term, and utm_content, will be set to null upon instantiating a new session. Note: This only works if includeReferrer or includeUtm includeUtm are set to true.

--

Is there parity with the server-side/android/iOS integration components (if applicable)?
no

Does this require a new integration setting? If so, please explain how the new setting works
This PR introduces a new setting unsetParamsReferrerOnNewSession which tells amplitude to nullify utm/referrer params at the start of each session.

Links to helpful docs and other external resources

@omegabytes omegabytes requested a review from a team August 21, 2019 19:34
mericsson
mericsson reviewed Aug 21, 2019
View reviewed changes
mericsson
mericsson reviewed Aug 21, 2019
View reviewed changes
@mericsson
Copy link
Contributor

Can you add DEST-?? ticket to PR subject to see what this is fixing?

@omegabytes omegabytes changed the title added check to unset params and ref on new session [DEST-897] added check to unset params and ref on new session Aug 21, 2019
mericsson
mericsson reviewed Aug 28, 2019
View reviewed changes
integrations/amplitude/lib/index.js
.option('traitsToSetOnce', [])
.option('traitsToIncrement', [])
.option('appendFieldsToEventProps', {})
.option('unsetParamsReferrerOnNewSession', false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This sets the default value for the option right?

Technically I think this means there is a change in behavior for existing customers since the value for unsetParamsReferrerOnNewSession was undefined before.

But most likely Amplitude treats all falsey values the same then there is no difference in behavior and this is clearer. So I think this is fine. Just something to watch out for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we have been burned by this before @ccnixon ? Probably not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding on to this.. looks like Amplitude just check for falsy/truthy: https://github.com/amplitude/Amplitude-JavaScript/blob/master/src/amplitude-client.js#L127

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find @gpsamson !

@mericsson
Copy link
Contributor

mericsson commented Aug 28, 2019 •
edited
Loading

PR description says:

Does this require a new integration setting? If so, please explain how the new setting works

no

but I think this does right?

@omegabytes
Copy link
Contributor Author

omegabytes commented Aug 28, 2019 •
edited
Loading

@mericsson this is mobile-only and doesn't apply server-side

ninja-edit: I misunderstood, updating.

gpsamson
gpsamson approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@gpsamson gpsamson left a comment

Choose a reason for hiding this comment

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

LGTM

@omegabytes omegabytes merged commit 056ab37 into master Aug 29, 2019
@omegabytes omegabytes deleted the feature/amplitude branch August 29, 2019 17:18
patrickotoole pushed a commit to rockerbox/analytics.js-integrations that referenced this pull request Feb 27, 2020
@omegabytes @patrickotoole
[DEST-897] added check to unset params and ref on new session (segmen…
e3e583d 
…tio#194)

* added check to unset params and ref on new session

* removed ready cb, added option

* typo

* add unsetParamsReferrerOnNewSession setting
marinhero pushed a commit to Wootric/analytics.js-integrations-1 that referenced this pull request Apr 24, 2020
@omegabytes @marinhero
[DEST-897] added check to unset params and ref on new session (segmen…
425a557 
…tio#194)

* added check to unset params and ref on new session

* removed ready cb, added option

* typo

* add unsetParamsReferrerOnNewSession setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mericsson mericsson mericsson approved these changes

+1 more reviewer

@gpsamson gpsamson gpsamson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@omegabytes @mericsson @gpsamson