-
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
GumGum Adapter for Prebid.js 1.0 #1966
Conversation
Hi @matthewlane and prebid team members, just wondering if you have any info on the time window in which we could see these reviewed/merged? We know holidays are here, but just to manage expectations on our end. Thanks |
Hi @mxcoder, many of us are out until the new year, and we plan to ramp back up with full PR reviews and other work in January. Thank you for your pull request and for your patience |
Excellent, thanks a lot @matthewlane . Have a great holiday season. |
With provided test params, no ad displays. The issue is due to the price coming back as zero. Even though the zero bid wins the prebid auction, DFP is not selecting the ad. If I hack the price higher, the ad renders. Response: |
User syncs are broken and causing 404s. The user sync url is being set to a fixed string: Which causes the browser to load 'ADAPTER_SYNC_URL' relative to the current path which 404s |
Hi @mike-chowla . Thanks for the feedback. For the test params, what adjustments would you recommend? Do you think this would require non-zero price values to be returned by our ad server all the time? I guess what I'm asking is whether there can be anything done on the adapter side to "hack" the price value for the test unit, or if the change would have to be done on our server side. |
The clean way to do this is to use a non-zero price coming back from the ad server. |
…r test unit so DFP chooses it.
Thanks @mike-chowla . I opted to make the change in the adapter as doing so on the ad server was not as simple as I'd hoped. |
Your isTestUnit check is failing because it expects the si field to be an integer but it's a string (at least when using the test parameters) This fixes the check:
|
Nice catch! Updated. |
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.
With changes, LGTM
Test failure is shown but it's unrelated to this code and tests pass on my machine.
modules/gumgumBidAdapter.js
Outdated
// dealId: DEAL_ID, | ||
// referrer: REFERER, | ||
ad: wrapper ? getWrapperCode(wrapper, Object.assign({}, serverResponseBody, { bidRequest })) : markup, | ||
bidderCode: spec.code, |
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.
bidderCode
will be set automatically by bidderFactory
now, this line can be dropped
modules/gumgumBidAdapter.js
Outdated
cw: wrapper | ||
} = serverResponseBody | ||
let isTestUnit = (bidRequest.data && bidRequest.data.pi === 3 && bidRequest.data.si === 9) | ||
let [width, height] = bidRequest.sizes[0] |
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.
ad unit sizes may be defined as a double array ([[300, 250]]
) or single array ([300, 250]
). This will break if the size is defined as a single array, but utils.parseSizesInput
can help account for either case. Something like utils.parseSizesInput(bidRequest.sizes)[0].split('x')
if you only want the first size in the list
…t.sizes to account for 1-dimensional array.
thanks @matthewlane . Updated based on your suggestions. |
@mxcoder @bruscantini This is merged to master. Please also submit a PR to update your bidder file in the docs repo, to add the line |
* 'master' of https://github.com/prebid/Prebid.js: (23 commits) Update Atomx adapter for Prebid v1.0 (prebid#2026) Add vi bid adapter (prebid#2020) Add eplanningBidAdapter (prebid#2003) OpenX Adapter: Update to support mediaTypes field, instead of the deprecated mediaType field (prebid#1974) Separate bids & won calls (prebid#2015) 1.0 adapter support for mantis (prebid#1840) Media.net adapter added (prebid#2038) GumGum Adapter for Prebid.js 1.0 (prebid#1966) Serverbid Bid Adapter: updated docs and ad sizes (prebid#2023) Adding districtm as an alias (prebid#2018) Use sudo to workaround Travis regression (prebid#2041) Fix uncached video bids triggering callback early (prebid#2017) Re-implemented RhythmOne audit beacon in prebid 1.0 interface (prebid#1953) Add NasmediaAdmixer adapter for Perbid.js 1.0 (prebid#1937) Update Adform adapter to Prebid v1.0 (prebid#1947) Upgrade Admixer adapter for Prebid 1.0 (prebid#1755) multiformat size validation checks (prebid#1964) Gjirafa Bidder Adapter (prebid#1944) pin gulp-connect at non-broken version (prebid#2008) Added dynamic ttl property for One Display and One Mobile. (prebid#2004) ...
* GumGum Adapter for Prebid.js 1.0 * removed getUserSyncs. Give cpm a non-zero value when bidRequest is for test unit so DFP chooses it. * parsing slot ID as integer from params * ADSS-78 removed bidderCode from response. Correctly parsing bidRequest.sizes to account for 1-dimensional array.
Added the needs-docs label since it's not showing up on http://prebid.org/download.html in the 1.2.0 dropdown due to lack of the page having the "prebid 1.0 support" key in the page's YAML header. |
Thanks, we're working on this. We'll send another PR. |
Type of change
Description of change
GumGum Adapter according to 1.0 rules
contact email of the adapter’s maintainer
[email protected]
official adapter submission
Other information
We already had a GumGum Adapter integrated, this is just to upgrade to 1.0
CC @bruscantini