-
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
Add warning messages for publishers while native ads send assets containing url without sendId #5096
Conversation
Hi @jsnellbaker @robertrmartinez, I'd like to know is there any blocking point for this? Many thanks :) |
Hi @bmwcmw I have not had a chance to take a look at this yet but will do so soon. Usually we recommend adding a test for any PR to make sure we do not lag behind on the required 80% coverage for each adapter. So if you could please add an associated test for this change that would be great! |
Thank you @robertrmartinez , just added a unit test on it |
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.
Not sure the change does what is intended.
Can you please point out if I am missing something?
modules/criteoBidAdapter.js
Outdated
@@ -279,6 +291,7 @@ function buildCdbRequest(context, bidRequests, bidderRequest) { | |||
} | |||
if (bidRequest.params.nativeCallback || utils.deepAccess(bidRequest, `mediaTypes.${NATIVE}`)) { | |||
slot.native = true; | |||
hasNativeSendId = hasNativeSendId || checkNativeSendId(bidRequest); |
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.
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.
It's updated with improved tests, thanks for the pointing out.
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.
Cool stuff!
Thanks for the patience!
* commit '8db2720d60a6533dee52e96f847854ef1c219345': (249 commits) Release 3.17.0 LunaMedia Adapter (prebid#5110) UserId module to export user ids as eids with pbjs.getUserIdsAsEids() (prebid#5108) Triplelift: expose tl_souce in bid response (prebid#5139) Add warning messages for publishers while native ads send assets containing url without sendId (prebid#5096) use customSlotMatching func for reseting targeting (prebid#5132) Teads fix production GDPR error (prebid#5122) No bid version 1.2.5 (prebid#5137) SublimeBidAdapter src/url.js import fix (prebid#5150) Improve Digital adapter: add support for outstream video (prebid#5129) Disallowing remote requests from Safari and Firefox due to browsers no longer supporting DigiTrust ID. (prebid#5109) PS bid adapter update to copy site object from config (prebid#5083) SublimeBidAdapter: Update to version 0.5.1 (prebid#4977) LuponMedia Bid Adapter (prebid#5146) [BUGFIX] AdagioBidAdapter getDataFromLocalStorage (prebid#5081) Revert "New LuponMedia Bid Adapter (prebid#5120)" (prebid#5145) New LuponMedia Bid Adapter (prebid#5120) Feature/send publisher domain (prebid#5121) update test adunit params (prebid#5135) add parameter to the conversant adapter to override the url (prebid#5133) ...
…aining url without sendId (prebid#5096) * Check sendId parameter for native ads and warn if missing on required assets * Extract checking method and test Co-authored-by: mi.chen <[email protected]>
Type of change
Description of change
Following the updated documentation, we want to drop a warning to tell that sendid is mandatory for all fields receiving URLs, notably: product images, clickUrl, logoimage