OpenX official adapter release: Fixing the existing openx adapter and… by lntho · Pull Request #896 · prebid/Prebid.js

lntho · 2016-12-27T20:31:56Z

… adding test coverage on it

Type of change

[ x ] official adapter submission

test parameters for validating bids

{
  bidder: 'openx’,
  params: {
    // …
    params: {
              unit: ‘538562284’, // OpenX ad unit ID
              delDomain: 'clientname-d.openx.net',  // OpenX delivery domain
              customParams: {"gender": "female"}   // Publisher specified custom parameters, optional
            }
  }
}

bidder code: openx
Send All Bids Ad Server Keys: hb_pb_openx, hb_adid_openx, hb_size_openx
Default Deal ID Ad Server Key: hb_deal_openx

contact email of the adapter’s maintainer: team-openx@openx.com

Other information

Note:
The OpenX adapter requires setup and approval from your OpenX team. Please reach out to your OpenX representative or Support@openx.com for more information and to enable using this adapter.

mkendall07 · 2017-01-05T16:52:31Z

src/adapters/openx.js

prefer const here

mkendall07 · 2017-01-05T16:52:44Z

src/adapters/openx.js

mkendall07 · 2017-01-05T16:53:33Z

src/adapters/openx.js

lntho · 2017-01-06T19:49:40Z

@mkendall07 Thanks for the review! I've fixed the ones you have mentioned, please take a look again and let me know if there are additional things to fix :)

mkendall07 · 2017-01-06T20:02:08Z

@lntho
Hey sorry I got interrupted during code review so I just commented inline on those. I should be able to finish up the review today. Thanks for responding.

mkendall07 · 2017-01-05T17:00:51Z

src/adapters/openx.js

Consider using the "alias" pattern with baseAdapter to allow whitelabeling of your adapter.

See https://github.com/prebid/Prebid.js/blob/master/src/adapters/appnexusAst.js#L22 for example

mkendall07 · 2017-01-06T21:43:39Z

src/adapters/openx.js

window.top can throw. Please add try/catch.

Okay, will add a try catch here

mkendall07 · 2017-01-06T21:58:55Z

src/adapters/openx.js

mkendall07 · 2017-01-06T22:08:40Z

src/adapters/openx.js

It doesn't look like this img is used anywhere. Did you want to trigger a pixel/beacon here?

You're right, the return is not needed. This might be a relic from testing phases, I will remove the return.

mkendall07 · 2017-01-06T22:16:46Z

src/adapters/openx.js

These lines need try/catch

The window.top here is already checked below at line 195, I think it should be fine here without a try/catch?

Yeah that should be good.

mkendall07 · 2017-01-06T22:18:03Z

src/adapters/openx.js

What is this used for?

the bc parameter is for us to track what kind of service is making the ad request in our events

mkendall07 · 2017-01-06T22:19:29Z

src/adapters/openx.js

mkendall07 · 2017-01-06T22:20:02Z

src/adapters/openx.js

encodeURIComponent is safe to use directly. You don't need && here.

Here we're trying to check if window.location.href exists to begin with, and if it is undefined we want to leave it as such instead of encoding it to be a string 'undefined'

mkendall07 · 2017-01-06T22:21:45Z

test/spec/adapters/openx_spec.js

nit: replace all vars with const or let in this file.

mkendall07 · 2017-01-06T22:27:56Z

src/adapters/openx.js

Please pass the bidRequest bid object through the bidfactory.createBid(CONSTANTS.STATUS.GOOD, bid) so that we can match the bidId param. Example https://github.com/prebid/Prebid.js/blob/master/src/adapters/appnexusAst.js#L116

mkendall07 · 2017-01-09T15:54:00Z

@lntho
For validating purposes, we will need a real parameters. clientname-d.openx.net is not a valid endpoint. Please update with proper test params. Thanks

lntho · 2017-01-10T00:46:25Z

The PR has been updated, please let me know if there are additional issues that you see :)

mkendall07 · 2017-01-10T15:44:43Z

@lntho thanks, will review again.

mkendall07 · 2017-01-10T16:18:52Z

src/adapters/openx.js

nit: there is an extra space after this brace.

mkendall07 · 2017-01-10T16:19:13Z

src/adapters/openx.js

we have a newline at the end of the file by convention.

mkendall07 · 2017-01-10T16:20:05Z

Verified bids received and ads renderered. LGTM. Please address those 2 style items and we'll be good to merge. Thanks for contributing!

… adding test coverage on it

lntho · 2017-01-10T19:26:38Z

The style changes have been made, thank you for the review!

mkendall07 · 2017-01-10T19:33:37Z

Thanks. BTW you don't have to squash your commits (it's actually easier to review if you don't). Github has automatic squashing during merge to master.

mkendall07 · 2017-01-10T19:40:32Z

@lntho
Also please open PR to update the bidder docs for OpenX: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/openx.md

lntho closed this Dec 27, 2016

lntho reopened this Dec 28, 2016

lntho force-pushed the openx-release branch from 9713c7f to b17f82f Compare December 28, 2016 20:57

protonate self-assigned this Jan 3, 2017

mkendall07 assigned mkendall07 and unassigned protonate Jan 4, 2017

mkendall07 reviewed Jan 5, 2017

View reviewed changes

src/adapters/openx.js Outdated

Copy link

Contributor

mkendall07 Jan 5, 2017

Choose a reason for hiding this comment

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

prefer const here

mkendall07 reviewed Jan 5, 2017

View reviewed changes

src/adapters/openx.js Outdated

Copy link

Contributor

mkendall07 Jan 5, 2017

Choose a reason for hiding this comment

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

const

mkendall07 reviewed Jan 5, 2017

View reviewed changes

src/adapters/openx.js Outdated

Copy link

Contributor

mkendall07 Jan 5, 2017

Choose a reason for hiding this comment

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

let

mkendall07 added this to the Prebid 0.18.0 milestone Jan 5, 2017

mkendall07 added the in progress label Jan 5, 2017

lntho force-pushed the openx-release branch from b17f82f to ccb7ef4 Compare January 6, 2017 19:44

mkendall07 suggested changes Jan 6, 2017

View reviewed changes

mkendall07 added the needs update label Jan 9, 2017

lntho force-pushed the openx-release branch from ccb7ef4 to 30541c8 Compare January 9, 2017 20:54

mkendall07 reviewed Jan 10, 2017

View reviewed changes

src/adapters/openx.js Outdated

Copy link

Contributor

mkendall07 Jan 10, 2017

Choose a reason for hiding this comment

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

nit: there is an extra space after this brace.

mkendall07 reviewed Jan 10, 2017

View reviewed changes

src/adapters/openx.js Outdated

Copy link

Contributor

mkendall07 Jan 10, 2017

Choose a reason for hiding this comment

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

we have a newline at the end of the file by convention.

mkendall07 approved these changes Jan 10, 2017

View reviewed changes

OpenX official adapter release: Fixing the existing openx adapter and…

e726624

… adding test coverage on it

lntho force-pushed the openx-release branch from 30541c8 to e726624 Compare January 10, 2017 19:25

mkendall07 merged commit 0853677 into prebid:master Jan 10, 2017

mkendall07 removed the in progress label Jan 10, 2017

dbemiller mentioned this pull request Jun 5, 2017

remove superfluous parameter from openx adapter #1237

Merged

Comments

Conversation

lntho commented Dec 27, 2016

Type of change

Other information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntho commented Jan 6, 2017

Uh oh!

mkendall07 commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkendall07 commented Jan 9, 2017

Uh oh!

lntho commented Jan 10, 2017

Uh oh!

mkendall07 commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkendall07 commented Jan 10, 2017

Uh oh!

lntho commented Jan 10, 2017

Uh oh!

mkendall07 commented Jan 10, 2017

Uh oh!

mkendall07 commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants