-
Notifications
You must be signed in to change notification settings - Fork 15
[CDX-470] Add additionalTrackingKeys option
#472
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
bfb3d45
ad68a18
35750b3
b2b471b
2947a6e
c1f0bcc
ad7223a
5ba822e
bea7b45
70d7fa0
7dc32ee
3a6875f
0892bb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -318,6 +318,137 @@ describe('ConstructorIO - Utils - Request Queue', function utilsRequestQueue() { | |
| }); | ||
| }); | ||
|
|
||
| describe('additionalTrackingKeys', () => { | ||
| let defaultAgent; | ||
| let cleanup; | ||
|
|
||
| before(() => { | ||
| helpers.clearStorage(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| global.CLIENT_VERSION = 'cio-mocha'; | ||
| cleanup = jsdom(); | ||
| defaultAgent = window.navigator.userAgent; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| window.navigator.__defineGetter__('userAgent', () => defaultAgent); | ||
| delete global.CLIENT_VERSION; | ||
| cleanup(); | ||
| helpers.clearStorage(); | ||
| }); | ||
|
|
||
| it('Should add duplicate requests for each additional tracking key', () => { | ||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| additionalTrackingKeys: ['extra-key-1', 'extra-key-2'], | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123', 'POST', { action: 'session_start', key: 'primary-key' }); | ||
|
|
||
| const queue = RequestQueue.get(); | ||
| expect(queue).to.be.an('array').length(3); | ||
|
|
||
| // Primary request | ||
| expect(queue[0].url).to.contain('key=primary-key'); | ||
| expect(queue[0].body.key).to.equal('primary-key'); | ||
|
|
||
| // First additional key | ||
| expect(queue[1].url).to.contain('key=extra-key-1'); | ||
| expect(queue[1].url).to.not.contain('key=primary-key'); | ||
| expect(queue[1].body.key).to.equal('extra-key-1'); | ||
|
constructor-claude-bedrock[bot] marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
|
||
|
|
||
| // Second additional key | ||
| expect(queue[2].url).to.contain('key=extra-key-2'); | ||
| expect(queue[2].url).to.not.contain('key=primary-key'); | ||
| expect(queue[2].body.key).to.equal('extra-key-2'); | ||
| }); | ||
|
|
||
| it('Should not add duplicates when additionalTrackingKeys is an empty array', () => { | ||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| additionalTrackingKeys: [], | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123'); | ||
|
constructor-claude-bedrock[bot] marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
|
||
|
|
||
| expect(RequestQueue.get()).to.be.an('array').length(1); | ||
| }); | ||
|
|
||
| it('Should not add duplicates when additionalTrackingKeys is not provided', () => { | ||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123'); | ||
|
|
||
| expect(RequestQueue.get()).to.be.an('array').length(1); | ||
| }); | ||
|
|
||
| it('Should add duplicate requests for GET method', () => { | ||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| additionalTrackingKeys: ['extra-key-1'], | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123'); | ||
|
|
||
| const queue = RequestQueue.get(); | ||
| expect(queue).to.be.an('array').length(2); | ||
|
|
||
| expect(queue[0].url).to.contain('key=primary-key'); | ||
| expect(queue[1].url).to.contain('key=extra-key-1'); | ||
| expect(queue[1].url).to.contain('action=session_start'); | ||
| }); | ||
|
|
||
| it('Should skip invalid entries in additionalTrackingKeys', () => { | ||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| additionalTrackingKeys: ['valid-key', '', null, 123, 'another-valid-key'], | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123', 'POST', { action: 'session_start', key: 'primary-key' }); | ||
|
|
||
| const queue = RequestQueue.get(); | ||
| expect(queue).to.be.an('array').length(3); | ||
| expect(queue[1].url).to.contain('key=valid-key'); | ||
| expect(queue[2].url).to.contain('key=another-valid-key'); | ||
| }); | ||
|
|
||
| it('Should not duplicate events when additionalTrackingKeys contains the primary key or duplicates', () => { | ||
|
evanyan13 marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This test is a good addition. However, the test verifies that expect(queue[0].body.key).to.equal('primary-key');
expect(queue[1].body.key).to.equal('extra-key');This guards against the body not being swapped even when the URL deduplication works correctly. |
||
| store.session.set(humanityStorageKey, true); | ||
| const requests = new RequestQueue({ | ||
| sendTrackingEvents: true, | ||
| trackingSendDelay: 1, | ||
| apiKey: 'primary-key', | ||
| additionalTrackingKeys: ['extra-key', 'primary-key', 'extra-key'], | ||
| }); | ||
|
|
||
| requests.queue('https://ac.cnstrc.com/behavior?action=session_start&key=primary-key&_dt=123', 'POST', { action: 'session_start', key: 'primary-key' }); | ||
|
|
||
| const queue = RequestQueue.get(); | ||
| expect(queue).to.be.an('array').length(2); | ||
| expect(queue[0].url).to.contain('key=primary-key'); | ||
| expect(queue[1].url).to.contain('key=extra-key'); | ||
| }); | ||
| }); | ||
|
|
||
|
evanyan13 marked this conversation as resolved.
|
||
| describe('send', () => { | ||
| let fetchSpy = null; | ||
| let cleanup; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,7 @@ class ConstructorIO { | |
| * @param {number} [parameters.networkParameters.timeout] - Request timeout (in milliseconds) - may be overridden within individual method calls | ||
| * @param {string} [parameters.humanityCheckLocation='session'] - Storage location for the humanity check flag ('session' for sessionStorage, 'local' for localStorage) | ||
| * @param {boolean} [parameters.useWindowParameters=false] - Indicates if window globals (cnstrc/cnstrcUserId/cnstrcTestCells/cnstrcUserSegments) should be used as fallback for userId, testCells, and segments | ||
| * @param {string[]} [parameters.additionalTrackingKeys] - Additional API keys that each receive a duplicate of every tracking event. Events are always sent to the primary `apiKey` and, in addition, to every key in this array. | ||
| * @property {object} search - Interface to {@link module:search} | ||
| * @property {object} browse - Interface to {@link module:browse} | ||
| * @property {object} autocomplete - Interface to {@link module:autocomplete} | ||
|
|
@@ -97,6 +98,7 @@ class ConstructorIO { | |
| networkParameters, | ||
| humanityCheckLocation, | ||
| useWindowParameters, | ||
| additionalTrackingKeys, | ||
|
constructor-claude-bedrock[bot] marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
constructor-claude-bedrock[bot] marked this conversation as resolved.
|
||
| } = options; | ||
|
evanyan13 marked this conversation as resolved.
|
||
|
|
||
| if (!apiKey || typeof apiKey !== 'string') { | ||
|
|
@@ -147,6 +149,7 @@ class ConstructorIO { | |
| networkParameters: networkParameters || {}, | ||
| humanityCheckLocation: humanityCheckLocation || 'session', | ||
| useWindowParameters: useWindowParameters === true, | ||
| additionalTrackingKeys, | ||
| }; | ||
|
|
||
| if (useWindowParameters === true) { | ||
|
|
@@ -179,10 +182,11 @@ class ConstructorIO { | |
| * @param {string} [options.userId] - User ID | ||
| * @param {boolean} [options.sendTrackingEvents] - Indicates if tracking events should be dispatched | ||
| * @param {string} [options.serviceUrl] - API URL endpoint (normalized to include an HTTPS protocol and strip a trailing slash) | ||
| * @param {string[]} [options.additionalTrackingKeys] - Additional API keys that each receive a duplicate of every tracking event. Events are always sent to the primary `apiKey` and, in addition, to every key in this array. | ||
| */ | ||
|
evanyan13 marked this conversation as resolved.
|
||
| setClientOptions(options) { | ||
| if (Object.keys(options).length) { | ||
| const { apiKey, segments, testCells, sessionId, userId, sendTrackingEvents, serviceUrl } = options; | ||
| const { apiKey, segments, testCells, sessionId, userId, sendTrackingEvents, serviceUrl, additionalTrackingKeys } = options; | ||
|
evanyan13 marked this conversation as resolved.
|
||
|
|
||
| if (apiKey) { | ||
| this.options.apiKey = apiKey; | ||
|
|
@@ -214,6 +218,10 @@ class ConstructorIO { | |
| const formattedServiceUrl = helpers.addHTTPSToString(normalizedServiceUrl, this.options.allowHttpServiceUrl); | ||
| this.options.serviceUrl = formattedServiceUrl || this.options.serviceUrl; | ||
| } | ||
|
|
||
| if (Array.isArray(additionalTrackingKeys)) { | ||
|
constructor-claude-bedrock[bot] marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a big deal, but if we wanted to stop sending events to multiple keys, we would have to send |
||
| this.options.additionalTrackingKeys = additionalTrackingKeys; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,30 @@ class RequestQueue { | |
| body, | ||
| networkParameters, | ||
| }); | ||
|
|
||
| // Duplicate request for each additional tracking key | ||
| const additionalKeys = this.options?.additionalTrackingKeys; | ||
|
|
||
| if (Array.isArray(additionalKeys) && additionalKeys.length) { | ||
|
t3t3c marked this conversation as resolved.
|
||
| const encodedOriginalKey = helpers.encodeURIComponentRFC3986(this.options.apiKey); | ||
|
|
||
| const uniqueKeys = new Set( | ||
| additionalKeys.filter((key) => key && typeof key === 'string' && key !== this.options.apiKey), | ||
| ); | ||
|
|
||
| uniqueKeys.forEach((additionalKey) => { | ||
| const encodedAdditionalKey = helpers.encodeURIComponentRFC3986(additionalKey); | ||
| const swappedUrl = url.replace(`key=${encodedOriginalKey}`, `key=${encodedAdditionalKey}`); | ||
|
Comment on lines
+61
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason why we're doing this validation and uniqueness check every time we queue an event? It feels like an additional step that could be handled earlier (e.g. during client instantiation in the construtor or when setClientOptions is called)? |
||
|
|
||
| queue.push({ | ||
| url: obfuscatePiiRequest(swappedUrl), | ||
| method, | ||
| body: { ...body, key: additionalKey }, | ||
| networkParameters, | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| RequestQueue.set(queue); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The
setClientOptionstest foradditionalTrackingKeysonly covers the DOM-less path. Given that the existing tests for other options (e.g.apiKey,segments) include a parallel test that asserts the change propagates to sub-module options (instance.tracker.options,instance.search.options, etc.), consider adding the same assertion here — or, at minimum, assertinginstance.tracker.requests.options.additionalTrackingKeysif direct access is available. This ensures the shared-reference contract (noted above in theconstructorio.jscomment) is actually tested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude raised a good point. We should have equivalent tests for the DOM context.