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

IndexExchange Display Bid Adapter #2422

Conversation

jas-singh
Copy link

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
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Adding IndexExchange bid adapter

@dmitriyshashkin
Copy link
Contributor

Lot's of publishers (us included) said that it's the only reason that prevents from migrating to 1.X #2256 I really hope that this will be reviewed as soon as possible :-)

@Ximich
Copy link

Ximich commented Apr 20, 2018

+1, can't wait, so exciting!

@mercuryyy
Copy link
Contributor

I tried emailing "prebid.support@indexexchange.com" no response. ran a test with the new build for the last 24-48 hours fill rate is less then a 1/3 in 1.x for index impressions then what is was on 0.x

seems to all be working correctly bids are coming in and rendering but the index side it not winning nearly as much bids as in 0.x

@ix-prebid-support
Copy link
Contributor

@mercuryyy

An endpoint update wasn't completely deployed network wide until 20th April so poor performance was expected during that time frame. Feel free to let us know if you are still seeing an issue.

const isSecureWeb = utils.getTopWindowLocation().protocol === 'https:';
const baseUrl = isSecureWeb ? BANNER_SECURE_BID_URL : BANNER_INSECURE_BID_URL;
const PRICE_TO_DOLLAR_FACTOR = {
JPY: 1
Copy link
Member

@mkendall07 mkendall07 Apr 24, 2018

Choose a reason for hiding this comment

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

are you intending to hardcode currency rates in your adapter?

Copy link
Contributor

Choose a reason for hiding this comment

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

We would not be hardcoding any currency rates. The bid's returned by our platform are always in subunit of the account's currency therefore the PRICE_TO_DOLLAR_FACTOR was added so that we can skip the conversion of Japanese yen when we are parsing the bid response.

@mercuryyy
Copy link
Contributor

@ix-prebid-support
Thanks for the update, issue seems be resolved.

@jsnellbaker
Copy link
Collaborator

@jas-singh While I was checking over the adapter code I checked-out, I encountered some linting errors while doing a gulp serve.

Below is a copy of the errors related to the ixBidAdapter.js file:

  121:5   error  '&&' should be placed at the end of the line  operator-linebreak
  153:11  error  '&&' should be placed at the end of the line  operator-linebreak
  175:11  error  '||' should be placed at the end of the line  operator-linebreak

I'm not sure why these didn't appear in the automated checks, but I think they need to be addressed. Could you try to rebase off of master again?

@jas-singh jas-singh force-pushed the feature/index_integration branch from d69f515 to e33d280 Compare April 24, 2018 16:24
@ix-prebid-support
Copy link
Contributor

@jsnellbaker

The lint errors have been resolved

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@jsnellbaker jsnellbaker merged commit 6bbe2fa into prebid:feature/index_integration Apr 24, 2018
@akira28
Copy link

akira28 commented Apr 26, 2018

So nice that you merged it. Do you plan to release it in 1.9.0? It's literally the only thing that prevents us to update to 1.x

@dmitriyshashkin
Copy link
Contributor

+1, a release would be much appreciated! We're updating next week anyway, as we need the integration with GDPR consent framework, but it would be better to update to a stable version rather than to the head. Also, thank you for reviewing the PR so fast!

@chefbenjamin
Copy link

+1 we are also waiting on this release. We have been waiting for like 4 weeks for it. We've tested the adapter "as is" and we are not seeing bids on our test pages. Looking forward to it being merged into 1.9 or earlier.

@Ximich
Copy link

Ximich commented Apr 27, 2018

May be release 1.9 Alpha?

const CENT_TO_DOLLAR_FACTOR = 100;
const TIME_TO_LIVE = 60;
const NET_REVENUE = true;
const isSecureWeb = utils.getTopWindowLocation().protocol === 'https:';
Copy link

Choose a reason for hiding this comment

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

This line doesn't work for me - protocol does not include the colon. Works without the colon.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been addressed in #2496

@dmitriyshashkin
Copy link
Contributor

dmitriyshashkin commented May 3, 2018

Maybe it would be a good idea to add the alias to the old name (indexExchange). This will make the transition easier for some publishers. It's also kind of inconvenient that you've decided to change the register of some parameters. "siteID" in older version was replaced with "siteId" in the newer version. Such small things will create unnecessary difficulties for not so technical savvy publishers. It's not easy to spot such small changes, not to mention that updating settings for every unit is quite tedious

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.

10 participants