-
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
vi bid adapter #2020
vi bid adapter #2020
Conversation
Update viBidAdapter.md
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 PR, a few changes requested below
import { expect } from 'chai'; | ||
import { spec } from 'modules/viBidAdapter'; | ||
import { newBidder } from 'src/adapters/bidderFactory'; | ||
import { REPO_AND_VERSION } from 'src/constants'; |
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.
This import isn't used in the test, line can be dropped
modules/viBidAdapter.js
Outdated
requestId: bid.id, | ||
cpm: parseFloat(bid.price), | ||
width: parseInt(bid.width), | ||
height: parseInt(bid.height), |
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.
A radix (likely, 10
) should be specified as the second argument to the above two parseInt
s to guarantee predictable behavior
Docs PR for this adapter at prebid/prebid.github.io#537 |
Removed import and added radix |
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.
Updates look good, missed one thing in my initial review, noted below
modules/viBidAdapter.js
Outdated
utils._each(bidReqs, function (bid) { | ||
imps.push({ | ||
id: bid.bidId, | ||
sizes: bid.sizes, |
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 ([[320, 480]]
) or single array ([320, 480]
). In the case of single sizes, your endpoint is erroring out with 400 (Invalid JSON: Cannot deserialize instance of int[] out of VALUE_NUMBER_INT
, but utils.parseSizesInput
can help. Something like
utils.parseSizesInput(bid.sizes).map(size => size.split('x'))
though feel free handle it however you'd like
Assigning to @snapwich for a second round of review. |
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 dunno, I'm more of a emacsBidAdapter kind of guy. Just kidding, LGTM!
Thank you guys! |
* '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) ...
@SnoopInf This adapter will be in the next Prebid release, which should be happening this week. When that happens we'll merge the docs PR and vi will be listed on the site. Thanks for contributing to the project 👍 |
* VI prebid.js adapter (points dev platform) * fix typo * Changed maintainer email * Added test parameters * Changed bidder hostname to pb.vi-serve.com * updated doc * Update viBidAdapter.md * VI bid adapter - tests * Removed unused import from spec, specified radix in parseInt * handling of various sizes formats
Type of change
Description of change
vi bid adapter
contact email of the adapter’s maintainer
[email protected]
[ X ] official adapter submission
Other information
prebid/prebid.github.io#537