Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PubMatic adapter #1707

Merged
merged 8 commits into from
Nov 10, 2017
Merged

PubMatic adapter #1707

merged 8 commits into from
Nov 10, 2017

Conversation

pm-harshad-mane
Copy link
Contributor

@pm-harshad-mane pm-harshad-mane commented Oct 17, 2017 •
edited
Loading

Type of change

  • [ x] Bugfix
  • Feature
  • [ x] New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • [ x] Build related changes
  • [ x] CI related changes
  • [ x] Other

Description of change

  • test parameters for validating bids
For a slot of 300x250: 
{
  bidder: 'pubmatic',
  params: {
    publisherId: '301', // MANDATORY
    adSlot: '/15671365/DMDemo@728x90:0', // MANDATORY
    pmzoneid: 'customZone1, customZone2', // OPTIONAL
    dctr: 'key1=value1,value2'  // OPTIONAL
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

Other information

@dbemiller dbemiller mentioned this pull request Oct 17, 2017
Closed
5 tasks
@dbemiller
Copy link
Contributor

hey @pm-harshad-mane ... we're not accepting any pre-1.0 adapter PRs anymore. For more info, see how to add a Prebid 1.0 Bidder Adapter. In particular, you'll need to use AJAX rather than JSONP, and go through the bidderFactory. See the AppNexusAst adapter (if you can request all the bids using a single request) or Rubicon adapter (if you need multiple requests to request all the bids) for examples

@dbemiller dbemiller closed this Oct 18, 2017
@mkendall07 mkendall07 reopened this Nov 6, 2017
@mkendall07 mkendall07 assigned mkendall07 and unassigned dbemiller Nov 6, 2017
mkendall07
mkendall07 reviewed Nov 7, 2017
View reviewed changes
modules/pubmaticBidAdapter.js Outdated

/**
* Adapter for requesting bids from Pubmatic.
*
* @returns {{callBids: _callBids}}
* @constructor
*/
function PubmaticAdapter() {
var PubmaticAdapter = function PubmaticAdapter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer const

modules/pubmaticBidAdapter.js Outdated
var _pm_pub_gender;
var _pm_pub_kvs;
var _pm_optimize_adslots = [];
var usersync = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer let

modules/pubmaticBidAdapter.js

// istanbul ignore else
if (conf.pubId && slots.length > 0) {
_legacyExecution(conf, slots);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the legacy path is currently the only supported path (ie it still uses an iframe) is that correct? Is this optimized vs the last version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our older implementation used to include an external js library, now we do not include it.
_legacyExecution is the only flow available now, we are planning to add a new flow later.

mkendall07
mkendall07 reviewed Nov 7, 2017
View reviewed changes
modules/pubmaticBidAdapter.js

function _legacyExecution(conf, slots) {
var url = _generateLegacyCall(conf, slots);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear why this is still loaded into an iframe. Can you not use the ajax method here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our server response uses global namespace also our server is not supporting CORS.
By calling our server from a friendly-iframe we can get better priority than calling from a script tag thus we are calling our server using an iframe.

@pm-harshad-mane
Copy link
Contributor Author

Hello @mkendall07 ,
Thank you for your review comments.
I have implemented your suggestions, also I have answered your queries.
Let me know if you need more information.

@mkendall07
Copy link
Member

@pm-harshad-mane
I'm not receiving bids. Here is the generated ad call:
http://gads.pubmatic.com/AdServer/AdCallAggregator?SAVersion=1100&wp=PreBid&js=1&wv=prebid_prebid_0.33.0-pre&screenResolution=1440x900&ranreq=0.42388223356585986&inIframe=0&pageURL=http%3A%2F%2Flocalhost%3A9999%2FintegrationExamples%2F4_bidders.html&refurl=&kltstamp=2017-11-8%2015%3A37%3A40&timezone=-5&pubId=9999&kadpageurl=http%3A%2F%2Flocalhost%3A9999%2FintegrationExamples%2F4_bidders.html&dctr=key1%3Dvalue1%2Cvalue2&pmZoneId=customZone1%2C%20customZone2&adslots=%5B38519891%40300x250%5D

Noticed that there was an error: 2 in the response. Please advise.

@pm-harshad-mane
Copy link
Contributor Author

Hello @mkendall07 ,

I have updated test publishe ID (301) and adSlot (/15671365/DMDemo@728x90:0), now you can see the response, you might need to try few times to get bid-responses.

Sample URI: http://gads.pubmatic.com/AdServer/AdCallAggregator?SAVersion=1100&wp=PreBid&js=1&wv=prebid_prebid_0.33.0-pre&screenResolution=1440x900&ranreq=0.42388223356585986&inIframe=0&pageURL=http%3A%2F%2Flocalhost%3A9999%2FintegrationExamples%2F4_bidders.html&refurl=&kltstamp=2017-11-8%2015%3A37%3A40&timezone=-5&pubId=301&kadpageurl=http%3A%2F%2Flocalhost%3A9999%2FintegrationExamples%2F4_bidders.html&dctr=key1%3Dvalue1%2Cvalue2&pmZoneId=customZone1%2C%20customZone2&adslots=%5B%2F15671365%2FDMDemo%40728x90%3A0%2C%2F15671365%2FDMDemo1%40300x250%3A0%2C%2F15671365%2FDMDemo2%40728x90%3A0%5D

mkendall07
mkendall07 approved these changes Nov 10, 2017
View reviewed changes
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkendall07 mkendall07 merged commit ccbdf4a into prebid:master Nov 10, 2017
@PubMatic-OpenWrap
Copy link
Contributor

Thank you very much @mkendall07 👍

vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Dec 28, 2017
@vzhukovsky
Merge pull request prebid#133 in AOLP_ADS_JS/prebid.js from release/1…
e9bf890 
….33.0 to aolgithub-master

* commit '3e9756098bb20ecbe0314f16eed5298c5675b24c': (32 commits)
  Wrapped content type in options object.
  Added partners ids.
  Added changelog entry.
  Prebid 0.33.0 Release
  Update AOL adapter for v1.0  (prebid#1693)
  Sovrn 1.0 compliance (prebid#1796)
  Platform.io Bidder Adapter update (prebid#1817)
  Drop non-video bidders from video ad units (prebid#1815)
  Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795)
  Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823)
  Remove require.ensure entirely (prebid#1816)
  Add custom keyword support for pbs bid adapter (prebid#1763)
  OpenX Video Adapter update to Prebid v1.0 (prebid#1724)
  Fix test that hard-coded pbjs global. (prebid#1786)
  Update Pollux Adapter to v1.0 (prebid#1694)
  PubMatic adapter (prebid#1707)
  Added sizes to Rubicon Adapter (prebid#1818)
  jsonpFunction name should match the namespace (prebid#1785)
  Adding 33Across adapter (prebid#1805)
  Unit test fix (prebid#1812)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkendall07 mkendall07 mkendall07 approved these changes

Assignees

@mkendall07 mkendall07

Labels
not receiving bids
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pm-harshad-mane @dbemiller @mkendall07 @PubMatic-OpenWrap @jaiminpanchal27