-
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
Prebid adapter for windtalker #5040
Conversation
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.
Hi @degroat,
Thanks for the PR! I've reviewed your submission and here are few of my observations:
- Unit test cases missing. Please create a file,
windtalkerBidAdapter_spec.js
and achieve at least80% code coverage
. - The request sent to your endpoint for banner, native & video media types is returning HTTP status code
204
. I'm not getting any response. In order to approve, we need to check and validate certain things in the response. - The sample video ad unit in your
md
file,
{
code: 'dfp-video-div-container',
sizes: [640, 480],
mediaTypes: {
video: {
context: "instream"
}
},
bids: [{
bidder: 'windtalker',
params: {
pubId: '29521',
siteId: '26049',
size: '640X480',
placementId: '123',
video: {
skipppable: true,
}
}
}]
}
, is using the property sizes
, which we've removed in the Prebidv3.0.0. You have to instead use mediaTypes.video.playerSize
property to declare the playerSize.
- The same native ad unit in your
md
file,
{
code: 'dfp-native-div',
mediaType: 'native',
mediaTypes: {
native: {
title: {
required: true,
len: 75
},
image: {
required: true
},
body: {
len: 200
},
icon: {
required: false
}
}
},
bids: [{
bidder: 'windtalker',
params: {
pubId: '29521',
siteId: '26048',
placementId: '123',
bidFloor: '0.001', // optional
ifa: 'XXX-XXX', // optional
latitude: '40.712775', // optional
longitude: '-74.005973', // optional
}
}]
}
, is using a property mediaType
which is set to native
. This is not required. mediaTypes.native
property itself is sufficient.
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.
Hi @degroat,
Thanks for adding those tests. I tried to send banner request to your endpoint, am still getting 204 No Content
. I haven't tested with video and native yet.
Can you please confirm that your endpoint is successfully returning a bid response to all three media types, banner, video and native. We have to test the responses to all three mediaTypes. Thank you.
@Fawke Please try again. We had some geo restrictions in place that were likely causing the issue. |
I was still unable to get bid response from India, so I told my colleague in US to test it. He got Can you check? |
@Fawke Native should be good now too. |
Hi @degroat I was helping @Fawke earlier this morning with testing the adapter. I tried the native params again, but I'm still not seeing a bid response. I have copied the reqeust payload for reference:
Can you take a look? |
@jsnellbaker I'm getting a bid back now. Here is my payload.
|
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.
Thanks for the follow-up, I tried again and I am now getting a native bid back successfully.
LGTM
* prebid adatper for windtalker * file extension for imports * fix whitespace in adapter * removed inital value assignment per LGTM request * Minorchange to md file for windtalker * Updated windtalker adapter with tests Co-authored-by: Chris DeGroat <chris@degroat.net>
* 'master' of https://github.com/prebid/Prebid.js: (102 commits) Marsmedia - Add vastXml and fix id response (prebid#5067) PubMatic adapter to support image sync (prebid#5104) minor consentManagement fix (prebid#5050) fix circle ci failing tests (prebid#5113) Add Relaido Adapter (prebid#5101) Add new bid adapter for ConnectAd (prebid#4806) change payload (prebid#5105) Utils updates (prebid#5092) Read OpenRTB app objects if set in config + bug fix for when ad units are reloaded (prebid#5086) Criteo : added first party data mapping to bidder request (prebid#4954) updateAdGenerationManual (prebid#5032) New bid adapter: Wipes (prebid#5051) Prebid manager analytics utm tags (prebid#4998) CRITEO RTUS Integration with Yieldmo Prebid (prebid#5075) isSafariBrowser update (prebid#5077) Support min &max duration for onevideo (prebid#5079) increment pre version Prebid 3.15.0 release prebid#5011 Fix to set Secure attribute on cookie when SameSite=none (prebid#5064) Prebid adapter for windtalker (prebid#5040) ...
* prebid adatper for windtalker * file extension for imports * fix whitespace in adapter * removed inital value assignment per LGTM request * Minorchange to md file for windtalker * Updated windtalker adapter with tests Co-authored-by: Chris DeGroat <chris@degroat.net>
Type of change
Description of change
Prebid adapter for Windtalker.
PR Docs: prebid/prebid.github.io#1788