add user syncing for kargo #3099
Conversation
| const seed = spec._generateRandomUuid(); | ||
| const clientId = spec._getClientId(); | ||
| if (syncOptions.iframeEnabled && seed && clientId) { | ||
| for (let i = 0; i < SYNC_COUNT; i++) { |
There was a problem hiding this comment.
Just to confirm the intended logic here - this setup will always generate 5 sync requests that are largely the same minus the idx param which gets incremented by 1 for each subsequent sync request. Is that the expected behavior?
There was a problem hiding this comment.
Yes, we want to attempt 5 syncs (barring a cap set by the configuration) and we want our server to be able to choose what to sync, potentially randomly, using a stateless system that gets passed an instance ID/seed and an incrementer.
There was a problem hiding this comment.
Thanks for clarifying.
| uid = {}; | ||
| shouldSimulateOutdatedBrowser = false; | ||
|
|
||
| sandbox.stub(crypto, 'getRandomValues').callsFake(function(buf) { |
There was a problem hiding this comment.
This currently returns an undefined error on the crypto object when the unit tests run in IE11 (see the circleci failed job for details). Can you please take a look and fix the issue?
| const clientId = '74c81cbb-7d07-46d9-be9b-68ccb291c949'; | ||
| var shouldSimulateOutdatedBrowser, | ||
| uid, | ||
| isActuallyOutdatedBrowser; |
There was a problem hiding this comment.
There are some extra spaces before these variables that are causing some lint errors when I run gulp lint. See below for a copy of the error message:
/Users/jsnellbaker/git/Prebid.js/test/spec/modules/kargoBidAdapter_spec.js
394:1 error Expected indentation of 6 spaces but found 8 indent
395:1 error Expected indentation of 6 spaces but found 8 indent
Can you make this fix?
Normally linting errors would be caught by circleci. So going forward, you may want to look into updating/rebasing your fork with the latest version of master so we can catch these automatically when you submit a PR.
There was a problem hiding this comment.
Ah good catch, I'll fix and rebase as well.
|
@samuelhorwitz Thanks for making the additional changes; LGTM |
Type of change
Description of change
Add user syncing to Kargo