-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[synacormedia] Update adapter to support Consent Management Module #5506
Conversation
* commit '25c5ad3b60ffd6b4d3c645dd9bf16bbb4397a346': CAP-1614 - updated docs to show correct size for banner and some other small fixes
* commit 'b6dfbc9a938563c2964d30719a78cde1f15d3740': CAP-1636 support schain object in prebid
* commit 'c934f2d341bea0938cd5b1b75e62030ce5447117': CAP-1636 updated the review comments
* commit 'a26077f86598287d5c7d2f624cd3fec5b0b8d3e1': CAP-1849 - split up banner and video impressions to use format
* commit 'c70d8fc30e44779a8f3f78fa341ad19ad1cf5395': CAP-1879 - added adapter support for consent management module
This pull request introduces 1 alert when merging cd80003 into 307ef96 - view on LGTM.com new alerts:
|
It looks like the failing build is due to a different adapter's (YieldOne) unit test. |
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.
Can you please add unit tests?
modules/synacormediaBidAdapter.js
Outdated
if (!openRtbBidRequest.regs) { | ||
openRtbBidRequest.regs = {}; | ||
} | ||
if (!openRtbBidRequest.regs.ext) { | ||
openRtbBidRequest.regs.ext = {}; | ||
} | ||
openRtbBidRequest.regs.ext.us_privacy = bidderRequest.uspConsent; |
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.
I would suggest using utils.deepSetValue()
method instead of all the if statements here (you'll need to import utils
as well)
if (!openRtbBidRequest.regs) { | |
openRtbBidRequest.regs = {}; | |
} | |
if (!openRtbBidRequest.regs.ext) { | |
openRtbBidRequest.regs.ext = {}; | |
} | |
openRtbBidRequest.regs.ext.us_privacy = bidderRequest.uspConsent; | |
utils.deepSetValue(openRtbBidRequest, 'regs.ext.us_privacy', bidderRequest.uspConsent); |
This pull request introduces 1 alert when merging fa60070 into bb91d54 - view on LGTM.com new alerts:
|
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.
other than the unnecessary checks i commented on, this LGTM and I'll go ahead and merge this after you take a look and ideally remove them.
This pull request introduces 1 alert when merging 801603e into bb91d54 - view on LGTM.com new alerts:
|
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.
LGTM
Type of change
Description of change
Modified Synacormedia bid adapter to use the Consent Management Module.