Add flag and logic to delete cross domain identifiers #54
Conversation
Some customers may want us to delete cross domain analytics related identifiers when it is disabled. This PR adds a new flag `deleteCrossDomainId` that is checked before running this new logic. We delete seg_xid (and metadata) from cookies and crossDomainId from traits in localStorage. The deletion logic is run only if deletion is enabled for this project, and when either the seg_xid cookie or crossDomainId localStorage trait exists. To delete the localStorage traits, we use an internal API in analytics.js. This minimizes the risk of introducing a bug that could arise due to interacting with lower level localStorage APIs directly. Ref: https://segment.atlassian.net/browse/LIB-752
|
This is not dependent on https://github.com/segmentio/ajs-renderer/pull/75 to deploy. |
lib/index.js
Outdated
| @@ -179,6 +180,9 @@ Segment.prototype.initialize = function() { | |||
| this.cookie('segment_cross_domain_id_timestamp', null); | |||
| } | |||
|
|
|||
| // Delete cross domain identifiers. | |||
| this.deleteCrossDomainId(); | |||
There was a problem hiding this comment.
a bit nitpicking, but I find the function name confusing. Here it looks like we're going to always delete the cross domain id, which is not the case. Maybe checkCrossDomainIdForDeletion(), or something better?
There was a problem hiding this comment.
updating to deleteCrossDomainIdIfNeeded()
lib/index.js
Outdated
| } | ||
|
|
||
| // Only continue if we have any lingering identifiers. | ||
| if (this.cookie('seg_xid') == null && this.analytics.user().traits().crossDomainId == null) { |
There was a problem hiding this comment.
Is it necessary to account for a case where seg_xid is empty? I'm guessing there is a slim to none chance this occurs, but just checking.
What about if the other identifiers such as seg_xid_fd and seg_xid_ts are null/empty?
The only think I wonder about is in the case where this is reenabled, if having lingering other identifiers / empty values would impact the re-enablement of the XID Service.
There was a problem hiding this comment.
This checks if both the cookie and localStorage trait don't exist. If both don't exist, then we don't need to do anything can bail. Otherwise at least 1 exists, and we proceed to delete both.
One improvement we could make is make the checks independent (i.e. delete cookie if it exists and delete localStorage trait if it exists).
we are not checking here if the other metadata (e.g. seg_xid_fd or seg_xid_ts) are empty, so it doesn't really matter in this case.
There was a problem hiding this comment.
One improvement we could make is make the checks independent (i.e. delete cookie if it exists and delete localStorage trait if it exists).
Updated now!
|
@nhi-nguyen I pushed another update - could you take a 2nd look? |
| it('should delete cross domain identifiers if enabled', function() { | ||
| segment.options.deleteCrossDomainId = true; | ||
|
|
||
| segment.cookie('seg_xid', 'test_xid'); |
There was a problem hiding this comment.
here we could add seg_xid_fd and seg_xid_ts and then later verify that they are deleted.
| age: 26 | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
there could be another test for the case when the cookie doesn't exist but crossDomainId trait in localStorage exists.
There was a problem hiding this comment.
added 2 - one for that and one for reverse.
test/index.test.js
Outdated
| assert.equal(analytics.user().traits().crossDomainId, null); | ||
| }); | ||
|
|
||
| it('should delete xid cookie if only cookie exists', function() { |
There was a problem hiding this comment.
Looks like the description is wrong here. This is the case where the localStorage trait exists?
There was a problem hiding this comment.
oops - fixed now.
test/index.test.js
Outdated
| assert.equal(analytics.user().traits().crossDomainId, null); | ||
| }); | ||
|
|
||
| it('should delete localStorage trait if only traits exists', function() { |
There was a problem hiding this comment.
this test case doesn't test what it's supposed to test.
There was a problem hiding this comment.
sorry - fixed now!
f2prateek
force-pushed
the
deleteCrossDomainId
branch
from
2f097e4
to
5521824
Compare
Some customers may want us to delete cross domain analytics related identifiers when it is disabled. This PR adds a new flag
deleteCrossDomainIdthat is checked before running this new logic.We delete seg_xid (and metadata) from cookies and crossDomainId from traits in localStorage. The deletion logic is run only if deletion is enabled for this project, and when either the seg_xid cookie or crossDomainId localStorage trait exists.
To delete the localStorage traits, we use an internal API in analytics.js. This minimizes the risk of introducing a bug that could arise due to interacting with lower level localStorage APIs directly.
Ref: https://segment.atlassian.net/browse/LIB-752