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

IX adapter code refactoring #711

Merged
merged 1 commit into from
Oct 25, 2016
Merged

IX adapter code refactoring #711

merged 1 commit into from
Oct 25, 2016

Conversation

indexexchange
Copy link
Contributor

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

The change includes following:

  • adding more test cases to increase IX adapter code coverage
  • improving configuration validation
  • adding prebid version in IX demand request parameter
  • adding addBidResponse() test case to make sure the function does not override adid. IX returns multiple bids from Add IX Deal Support #638 change. Individual bid's adid should be different.

Other information

@protonate
Copy link
Collaborator

Thanks for the PR, assigned for review.

@protonate protonate self-assigned this Oct 18, 2016
protonate
protonate suggested changes Oct 24, 2016
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.

Thank you, code looks good. I am not getting ads to render however. Some debugging found that the ID is not passed back in the server response. I'll post two screenshots that illustrate this. Please resolve the ad render for this branch and we can merge.

src/adapters/indexExchange.js
@@ -114,8 +115,8 @@ var cygnus_index_start = function () {
}

OpenRTBRequest.prototype.serialize = function () {
var json = '{"id":' + this.requestID + ',"site":{"page":"' + quote(this.sitePage) + '"';
if (typeof document.referrer === 'string') {
var json = '{"id":"' + this.requestID + '","site":{"page":"' + quote(this.sitePage) + '"';
Copy link
Collaborator

Choose a reason for hiding this comment

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

ES6 Template Literals are also an option, e.g.:

var json = `{"id":"${this.requestID}", "site": { "page": "${quote(this.sitePage)}"`;

protonate
protonate reviewed Oct 24, 2016
View reviewed changes
test/spec/adapters/indexExchange_request_spec.js

assert.isUndefined( adLoader.loadScript.firstCall.args[0], "no request made to AS");
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many thanks for the additional test coverage.

@protonate
Copy link
Collaborator

No ID in response (topic branch)

This shows the topic branch response, you'll see the ID highlighted in the address bar and on the last line of the response (at the bottom) the ID is null.

screen shot 2016-10-24 at 3 31 29 pm

ID in response (master branch)

This ad renders and the ID is contained in the response.

screen shot 2016-10-24 at 3 31 53 pm

@indexexchange
Copy link
Contributor Author

indexexchange commented Oct 25, 2016 •
edited
Loading

Please resolve the ad render for this branch and we can merge.

@protonate The issue is resolved from the sandbox server; no adapter code change required.

@protonate protonate merged commit caa5370 into prebid:master Oct 25, 2016
@protonate
Copy link
Collaborator

Thanks @indexexchange, merged.

mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
Merge pull request prebid#51 in AOLP_ADS_JS/prebid.js from feature/Pr…
262f371 
…ebid-0.14.0 to release/1.7.0

* commit '3eefcf043466f5457c81bfec18b032b53490b78b': (52 commits)
  New adapters ids.
  Prebid 0.14.0 Release
  add workaround to prevent IX adapter from ending auction earlier due to one request becoming many responses (prebid#763)
  Add pbjs.getHighestCpmBids for getting winning bids (prebid#755)
  Fix build
  Drop Sekindo adapter.
  Bugfix/suppress prebid request if sizeMapping undefined for device width (prebid#758)
  amp integration (prebid#756)
  Add contribution guidelines (prebid#761)
  Add api method to shuffle the order bidders are called in (prebid#760)
  build adapter from custom source path (prebid#753)
  reduce duplication and ignore ga.js in coverage (prebid#754)
  Log /ut response errors in dev console (prebid#747)
  Fix indentation (code style error when building) (prebid#751)
  Add package keywords (prebid#746)
  added new rubiconLite adapter (prebid#740)
  Show warning if bidCpmAdjustment is set for AOL bidder (closes prebid#725) (prebid#728)
  IX adapter code refactoring (prebid#711)
  Update memeglobal.js (prebid#737)
  Pulsepoint Analytics adapter for Prebid (prebid#706)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
Merge pull request prebid#54 in AOLP_ADS_JS/prebid.js from release/1.…
a3ee191 
…7.0 to master

* commit '262f371a5c855419c3ef357fb1f0cf87a107749f': (52 commits)
  New adapters ids.
  Prebid 0.14.0 Release
  add workaround to prevent IX adapter from ending auction earlier due to one request becoming many responses (prebid#763)
  Add pbjs.getHighestCpmBids for getting winning bids (prebid#755)
  Fix build
  Drop Sekindo adapter.
  Bugfix/suppress prebid request if sizeMapping undefined for device width (prebid#758)
  amp integration (prebid#756)
  Add contribution guidelines (prebid#761)
  Add api method to shuffle the order bidders are called in (prebid#760)
  build adapter from custom source path (prebid#753)
  reduce duplication and ignore ga.js in coverage (prebid#754)
  Log /ut response errors in dev console (prebid#747)
  Fix indentation (code style error when building) (prebid#751)
  Add package keywords (prebid#746)
  added new rubiconLite adapter (prebid#740)
  Show warning if bidCpmAdjustment is set for AOL bidder (closes prebid#725) (prebid#728)
  IX adapter code refactoring (prebid#711)
  Update memeglobal.js (prebid#737)
  Pulsepoint Analytics adapter for Prebid (prebid#706)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@protonate protonate protonate approved these changes

Assignees

@protonate protonate

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@indexexchange @protonate @rdgordon-index