GDPR - add consent information to PBS cookie_sync request #2530
Conversation
There was a problem hiding this comment.
Looks good overall. I left a few comments.
Obligatory: _synced is global state, and global state will absolutely cause you bugs and headaches. reset() functions (if used by live/prod code) will make it worst. So this is a bit risky, but... it's not my code to maintain, and in this case it's not easy to fix... so this'll work for now.
modules/prebidServerBidAdapter.js
Outdated
| if (_synced) { | ||
| return; | ||
| } | ||
| _synced = true; | ||
| const payload = JSON.stringify({ | ||
|
|
||
| let payload = { |
There was a problem hiding this comment.
why const => let here? I see mutation, but no re-assignments... seems like const should still work?
There was a problem hiding this comment.
I'll change it to const to be consistent.
There was a problem hiding this comment.
Sounds good.
If you have a choice, always use const instead of let. There are a million little principles out there which make code slightly easier to read, and they add up when you put them all together... so it's a good habit to have.
modules/prebidServerBidAdapter.js
Outdated
| @@ -666,6 +683,10 @@ export function PrebidServer() { | |||
| .reduce(utils.flatten) | |||
| .filter(utils.uniques); | |||
|
|
|||
| if (_s2sConfig && _s2sConfig.syncEndpoint) { | |||
| queueSync(_s2sConfig.bidders, bidRequests[0].gdprConsent); | |||
There was a problem hiding this comment.
Is bidRequests guaranteed to have at least one element in it? If not, this could break.
There was a problem hiding this comment.
Per this check, it should always at least have 1 element:
https://github.com/prebid/Prebid.js/blob/master/src/adaptermanager.js#L289
But I can add another check in this code to be sure.
There was a problem hiding this comment.
Either way.
My only two concerns are "does it work now?" and "is it likely that the code changes to break it in the future without anyone realizing?" Looks like the answer to (1) is "yes." (2) usually depends on how thorough the docs and unit tests are.
Your call either way. From my experience, I wouldn't trust the adaptermanager implementation to stay the same unless a unit test existed to enforce it.
| expect(requestBid.gdpr_consent).is.equal('abc123def'); | ||
|
|
||
| // check scenario if gdprApplies is false | ||
| resetSyncedStatus(); |
There was a problem hiding this comment.
Tests shouldn't have side-effects on the global state.
It's also dangerous to put this mid-test because it won't execute if one of the earlier assertions fail.
This test should be split into smaller ones, and resetSyncedStatus() called in an afterEach() block
There was a problem hiding this comment.
True on the above; I wanted to limit the scenarios where I was using this function to not affect other tests by having that function run for every test. But it doesn't seem like it's breaking the other tests when I put it there now. I'll make the changes here.
There was a problem hiding this comment.
it's fine... if other tests fail because the global state gets reset, then they're the broken ones. Tests should be initializing their own state, rather than relying on other tests to do it for them.
| /** | ||
| * @param {Array} bidderCodes list of bidders to request user syncs for. | ||
| */ | ||
| function queueSync(bidderCodes) { | ||
| function queueSync(bidderCodes, gdprConsent) { |
There was a problem hiding this comment.
Would be awesome if this code could be reused for spec.getUserSyncs but it's ok for now.
* PreBid.js/master: (79 commits) increment prebid version Prebid 1.11.0 release replace find reference with imported version (prebid#2542) GDPR - add consent information to PBS cookie_sync request (prebid#2530) add support for safeframe workflow and new utils method to read adunit sizes (prebid#2523) Smart: GDPR support (prebid#2528) fix getPreparedBidForAuction to look for renderer on correct bid (prebid#2505) PubMatic Adapter: Bug fix to read all bids from seatBid array (prebid#2520) Create GXOne Bid Adapter and tests for it (prebid#2540) Lifestreet: gdpr and consent string parameters (prebid#2537) -GDPR support added in media net bidder (prebid#2538) change AppNexus endpoint to use ORTB (prebid#2532) fixed bug when latitute/longitue are not provided (prebid#2533) added gdpr support to userSync in rubicon adapter (prebid#2531) Show only summary and errors (prebid#2514) Aardvark v1.0 (prebid#2507) Add 1024x768 (size_id = 53) in sizeMap (prebid#2527) Add new Adapter brainyBidAdapter (prebid#2458) OpenX Adapter: GDPR Support (prebid#2504) add support for latLong in rubicon adapter (prebid#2508) ...
* add consent information to cookie_sync request * restructured unit tests and other minor changes
Type of change
Description of change
Addresses the issue raised in #2516
Adds support to pass the consent information into the prebidServer cookie sync request.
We also removed an additional sync attempt made in the response as this was deemed redundant.