-
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!
…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 <mi.chen@criteo.com>
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