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.

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

Yieldmo bid adapter #1415

Merged
merged 29 commits into from
Sep 8, 2017
Merged

Yieldmo bid adapter #1415

merged 29 commits into from
Sep 8, 2017

Conversation

xlozinguez
Copy link
Contributor

@xlozinguez xlozinguez commented Jul 25, 2017

Type of change

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

Description of change

Added Yieldmo adapter

  • test parameters for validating bids
{
  bidder: 'yieldmo',
  params: {}
}

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

  • contact email of the adapter’s maintainer: [email protected]
  • official adapter submission

Other information

@cdoher01 @harquail @yieldmo-rao @elberdev

@cdoher01
Copy link
Contributor

Please note it's a new adapter, not a feature, misclicked :/ (I can't change it for some reason it says "You can't perform that action at this time.")

var e = 4; // 0 (COP) or 4 (DFP) for now -- ad server should reject other environments (TODO: validate that it will always be the case)
var bust = new Date().getTime().toString(); // cache buster
var scrd = window.devicePixelRatio || 0; // screen pixel density
var ae = 0; // prebid adapter version

Choose a reason for hiding this comment

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

I don't think we need to send this (per Rahul Jain)

Choose a reason for hiding this comment

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

also misleading comment 😸

}

// @if NODE_ENV='debug'
// utils.logMessage('ymCall request built: ' + ymCall);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be commented out?

@dbemiller dbemiller self-assigned this Jul 31, 2017
@dbemiller
Copy link
Contributor

dbemiller commented Aug 1, 2017

Hey. This code looks good... but I'm not getting any bids during test. These are my ad units (plugged into integrationExamples/gpt/hello_world.html):

var adUnits = [{
        code: 'div-gpt-ad-1460505748561-0',
        sizes: [[300, 250], [300,600]],

        // Replace this object to test a new Adapter!
        bids: [{
          bidder: 'yieldmo',
          params: {}
        }]

      }];

This also turned up a bug. Your server is returning {} for me, which your YMCB handler processes by doing nothing. Prebid then waits for your bid until the auction's timeout, which is really bad for the publisher's page's performance.

You need to make sure your adapter calls addBidResponse with a NO_BID bid regardless of what your server returns. Please add some unit tests to make sure this happens.

screen shot 2017-08-01 at 3 02 07 pm

^^ This code flow never triggers a call to addBidResponse

@cdoher01
Copy link
Contributor

Hey @dbemiller I've updated the bid adapter to add NOBID responses if the server returns an invalid response. We've also got our endpoint up now so you should see a valid response if you try it again. Thanks!

@cdoher01
Copy link
Contributor

@dbemiller Hey just wanted to check if there is anything else we need to do to keep this moving forward. Thanks!

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

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

Sorry for the long lagtime. Been dealing with production stuff and neglecting JS.

This is almost mergeable... the only change which really needs to happen is the Object.assign. The rest is just FYI, because we're expecting to start pushing adapters to become 1.0-compliant in the next few weeks.

I did verify that the auction is ending properly when the server responds with a {} now.

I'm still not able to get a true bid back from the server, though. Maybe the test campaign has ended? I'm running this from the US, if it matters.


return {
callBids: _callBids
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be:

return Object.assign(this, {
  callBids: _callBids
});

See #1459

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

// expose the callback to the global object:
$$PREBID_GLOBAL$$.YMCB = function(ymResponses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for right now... but beware that Prebid will be dropping support for JSONP in version 1.0, which should be coming in the next few months.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll make a note for us to update this. Do you know if any existing adapters are a good example to look at for 1.0 compatible callback?

Copy link
Contributor

Choose a reason for hiding this comment

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

We won't be letting servers respond with code in 1.0--just data. So you'll be making a GET/POST request, and then unpacking bids from the JSON (or XML, plain text, etc) in the response body.

The exact API for adapters in 1.0 is still under discussion in #1494, if you want to take a look and make suggestions over there. I'm updating the appnexusAst adapter to help prototype it.

}

function _registerNoResponseBids() {
var yieldmoBidRequests = $$PREBID_GLOBAL$$._bidsRequested.find(bid => bid.bidderCode === 'yieldmo');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue here... this is fine for now, but won't work in Prebid 1.0. #1421 breaks it, since different auctions will have different bid requests, so the global _bidsRequested array won't be valid anymore.

bidmanager.addBidResponse(placementCode, bid);
} else {
// no response data
// @if NODE_ENV='debug'
Copy link
Contributor

Choose a reason for hiding this comment

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

Prebid isn't using preprocess... so it won't respect these annotations.

The utils.logMessage function is a no-op unless debugging is enabled, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@cdoher01
Copy link
Contributor

@dbemiller Thanks for taking another look. I've updated to use Object.assign. Looks like the ad response is still up, you could reference this test page to see an example of the response coming back: http://s3.amazonaws.com/static.yieldmo.com/icons/beta/hello_world.html

Let me know if there is anything else to update to get this first version in. Thanks! Connor

@cdoher01
Copy link
Contributor

Oh, one thing to note is that we only run our ads on mobile so you will want to put the browser into mobile emulation to get back a full bid response, that might be why you weren't seeing a full response.

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

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

Aha... that explains it. Bid came back when I emulated mobile.

Ok, this looks good. I left behind two more little cleanup comments... but they're not blockers by any means.

// to prevent Prebid waiting till timeout for response
_registerNoResponseBids();

// @if NODE_ENV='debug'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also be removed

ymCall = ymCall.substring(0, ymCall.length - 1);
}

// @if NODE_ENV='debug'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also be removed

Connor Doherty and others added 2 commits September 5, 2017 10:32
@cdoher01
Copy link
Contributor

cdoher01 commented Sep 5, 2017

Cool thanks, @dbemiller I've removed those other unneeded comments. @jaiminpanchal27 let me know if you spot anything else that needs updating before it ready to go. Thanks!

@jaiminpanchal27 jaiminpanchal27 merged commit 598817f into prebid:master Sep 8, 2017
@cdoher01
Copy link
Contributor

cdoher01 commented Sep 8, 2017

@jaiminpanchal27 @dbemiller Thanks for reviewing and getting this merged in! Is there a place that I need to make a PR to have Yieldmo show up on the list of Bid Adapters here: http://prebid.org/download.html ? Thanks

@dbemiller
Copy link
Contributor

@cdoher01 yes. Make a PR against the docs repo: https://github.com/prebid/prebid.github.io

@cdoher01
Copy link
Contributor

cdoher01 commented Sep 8, 2017

@dbemiller cool will do, thanks!

@mkendall07
Copy link
Member

@cdoher01
we'll actually take care of the http://prebid.org/download.html page automatically when we update.

@dbemiller
Copy link
Contributor

akh... I'm sorry. For some reason I read your question as "how do I make it show up in the docs?" On this page

Listen to @mkendall07 ^^

@cdoher01
Copy link
Contributor

cdoher01 commented Sep 8, 2017

Okay cool. @mkendall07 do you roughly when the update will happen?

outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Sep 18, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (46 commits)
  Serverbid alias (prebid#1560)
  Add user sync to process for approving adapter PRs (prebid#1457)
  fix travis build (prebid#1595)
  Rubicon project improvement/user sync (prebid#1549)
  Adding Orbitsoft adapter (prebid#1378)
  Fix renderer test for new validation rule (prebid#1592)
  Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577)
  Add support for video stream context (prebid#1483)
  Invalidate bid if matching bid request not found (prebid#1575)
  allow adapters to set default adserverTargeting for specific bid (prebid#1568)
  Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591)
  BaseAdapter for the Prebid 0.x -> 1.x transition  (prebid#1494)
  Add a version to the Criteo adapter (prebid#1573)
  Allow bundling from node.js or with new gulp task bundle-to-stdout  (prebid#1570)
  Add url.parse option to not decode the whole URL (prebid#1480)
  Tremor Video Bid Adapter (prebid#1552)
  Yieldmo bid adapter (prebid#1415)
  Switch `gulp docs` to build its output using documentation.js (prebid#1545)
  Increment pre version
  Prebid 0.28.0 Release
  ...
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
* Create yieldmoBidAdapter file

* Copy over TL adapter and change name spacing

* Update bid adapter naming to Yieldmo

* Init tests for yieldmoBidAdapter

* Add yeildmo config to pbjs_example_gpt

* feature/yieldmoBidAdapter: added request build

* Update ym response to have placement and impression level info

* Remove environment param

* Update ym tests for single bid request structure

* Update url to use _ instead of camelcase

* Update YMCB to handle multiple placements in one response

* Clean up and remove TODO from adapter

* Update response tests to work with single response structure

* Remove trailing space

* Cleanup

* Adjusted code according to CR comments

* Update bid adapter with fallback for incorrect bid response

* Update yeildmo prebid endpoint

* Add check to make sure response is an Array

* Remove environment tracking as we are no going to use this

* Update test to pass with new Yieldmo endpoint

* Update return obj and remove node debug comments

* Remove changes to hello_world and yarn.lock files

* Remove unneeded comments
@cdoher01
Copy link
Contributor

cdoher01 commented Oct 5, 2017

@mkendall07 just following up, do you know the timeline for updating the download page? Would be nice to have Yieldmo bid adapter available to download via option 1. Thanks

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Create yieldmoBidAdapter file

* Copy over TL adapter and change name spacing

* Update bid adapter naming to Yieldmo

* Init tests for yieldmoBidAdapter

* Add yeildmo config to pbjs_example_gpt

* feature/yieldmoBidAdapter: added request build

* Update ym response to have placement and impression level info

* Remove environment param

* Update ym tests for single bid request structure

* Update url to use _ instead of camelcase

* Update YMCB to handle multiple placements in one response

* Clean up and remove TODO from adapter

* Update response tests to work with single response structure

* Remove trailing space

* Cleanup

* Adjusted code according to CR comments

* Update bid adapter with fallback for incorrect bid response

* Update yeildmo prebid endpoint

* Add check to make sure response is an Array

* Remove environment tracking as we are no going to use this

* Update test to pass with new Yieldmo endpoint

* Update return obj and remove node debug comments

* Remove changes to hello_world and yarn.lock files

* Remove unneeded comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants