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

Pubgears Header Bidding Adapter #953

Merged
merged 11 commits into from
May 2, 2017
Merged

Pubgears Header Bidding Adapter #953

merged 11 commits into from
May 2, 2017

Conversation

Walexander
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

Adapter and specs for Pubgears header bidder.

  • test parameters for validating bids
{
  bidder: 'pubgears',
  adUnitCode: 'id123/header-bid-tag-0',
  params: {
    publisherName: "integration",
    pubZone: "i.testpub.com/multibidder"
  }
}

Other information

@protonate protonate self-assigned this Feb 1, 2017
@protonate protonate self-requested a review February 1, 2017 19:09
@protonate
Copy link
Collaborator

Is it possible to implement without using additional scripts?

protonate
protonate suggested changes Mar 2, 2017
View reviewed changes
Copy link
Collaborator

@protonate protonate left a comment

Choose a reason for hiding this comment

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

Please see if you can remove additional scripts downloaded by the adapter.

@protonate
Copy link
Collaborator

Try rebasing onto current master branch, or if you prefer it may be easier to start with a new branch cut from current master branch and reintroduce your changes there.

@mkendall07
Copy link
Member

@Walexander looks like this needs to be rebased off master.

@Walexander
Copy link
Contributor Author

The rebase has been completed. Apologies for the delay.

@Walexander
Copy link
Contributor Author

It would be difficult to implement this without loading our external library. The adapter is built around receiving events from the script element itself. The code that publishes these events is implemented in the external library.

@mkendall07 mkendall07 self-requested a review March 30, 2017 19:51
@mkendall07 mkendall07 assigned mkendall07 and unassigned protonate Mar 30, 2017
mkendall07
mkendall07 requested changes Mar 30, 2017
View reviewed changes
src/adapters/pubgears.js Outdated
var SLOT_LIST_ATTRIBUTE = 'slot-list';
var PUBLISHER_ATTRIBUTE = 'pub';
var FLAG_ATTRIBUTE = 'flag';

Copy link
Member

Choose a reason for hiding this comment

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

please remove all these extra new lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/adapters/pubgears.js Outdated
initialized = true;
}

function loadScript(script) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the standard adLoader.loadScript instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the script element to obtain the bid response data from our API. The adLoader.loadScript does not provide the element either via its return value or callback.

Copy link
Member

Choose a reason for hiding this comment

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

@Walexander can you give an example? That seems like a strange pattern to me.

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 API uses DOM events instead of method calls. The adapter registers event listeners with the script element here

API Documentation

src/adapters/pubgears.js Outdated

function getCreative(resource) {

var bookends = '%%';
Copy link
Member

Choose a reason for hiding this comment

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

You can use utils.replaceTokenInString for this functionality or a string template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. This has been done.

@mkendall07
Copy link
Member

@Walexander thanks for the updates. It looks like your adapter is adding 2 bid responses using the test params. See screenshot here:
image

You should only add a no_bid in case there isn't any demand. Please update.

Also - it doesn't appear that adUnitCode: 'id123/header-bid-tag-0', does anything? What's it's purpose?

mkendall07
mkendall07 reviewed Apr 20, 2017
View reviewed changes
test/spec/adapters/pubgears_spec.js Outdated
@@ -215,7 +215,7 @@ describe('PubGearsAdapter', () => {
expect(bidmanager.addBidResponse.calledOnce).to.be.ok
})

it('should not completely fucking fail', () => {
Copy link
Member

Choose a reason for hiding this comment

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

lol

@protonate protonate added this to the Prebid 0.23.0 milestone May 2, 2017
@mkendall07
Copy link
Member

LGTM

@mkendall07 mkendall07 added the LGTM label May 2, 2017
@protonate protonate merged commit c17c483 into prebid:master May 2, 2017
@protonate
Copy link
Collaborator

A note here that the use of script.dispatchEvent in tests is failing in some version of IE with "Object doesn't support this action". This is limited to unit tests in IE.

outoftime pushed a commit to Genius/Prebid.js that referenced this pull request May 4, 2017
Merge branch 'master' of https://github.com/prebid/Prebid.js into pre…
482c0ed 
…built

* 'master' of https://github.com/prebid/Prebid.js: (21 commits)
  add lodash as dependency (prebid#1174)
  fix size mapping for s2s (prebid#1175)
  Improve footer styling (prebid#1171)
  Bugfix: internal bids requested overwritten (prebid#1173)
  pre-release version bump
  Prebid 0.23.0 Release
  Yieldbot adapter - multiple requestBids per pageview (prebid#1146)
  Widespace adapter validate size fix (prebid#1140)
  Audience Network: bid when at least one valid slot size (prebid#1148)
  Quantcast adaptor (prebid#1063)
  AOL Adapter - ONE Mobile endpoint implemented. (prebid#1115)
  Prebid Server to Server (prebid#1165)
  Pubgears Header Bidding Adapter (prebid#953)
  remove old adloader#trackPixel (prebid#1159)
  added audit beacon to detect misuse of this bidder.  Detects auctions… (prebid#1134)
  Bidfluence CDN endpoint URL update (prebid#1163)
  AdSupply adapter (prebid#1162)
  Sonobi Adapter - Enable size overrides (prebid#1141)
  Added an editorconfig file to match jshint and jssrc files. (prebid#1147)
  force cpm to be a number (prebid#1161)
  ...
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

@protonate protonate protonate approved these changes

Assignees

@mkendall07 mkendall07

Labels
LGTM
Projects
None yet
Milestone
Prebid 0.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Walexander @protonate @mkendall07 @matthewlane