Skip to content

Add Fidelity adapter#862

Merged
matthewlane merged 5 commits intoprebid:masterfrom
onaydenov:fidelity_adapter
Dec 14, 2016
Merged

Add Fidelity adapter#862
matthewlane merged 5 commits intoprebid:masterfrom
onaydenov:fidelity_adapter

Conversation

@onaydenov
Copy link
Contributor

@onaydenov onaydenov commented Dec 7, 2016

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

  • test parameters for validating bids
{
  bidder: 'fidelity',
  params: {
        zoneid: '27248'
  }
}

Other information

@matthewlane matthewlane self-assigned this Dec 12, 2016
matthewlane
matthewlane reviewed Dec 13, 2016
View reviewed changes
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Tests are passing and was able to validate bids with the provided parameters. Just left a few questions and notes for your review below, thanks

src/adapters/fidelity.js Outdated
if (document.MAX_used !== ',') m3_u += '&exclude=' + document.MAX_used;
m3_u += document.charset ? '&charset='+document.charset : (document.characterSet ? '&charset='+document.characterSet : '');

var loc = window.top !== window ? document.referrer:window.location.href;
Copy link
Collaborator

@matthewlane matthewlane Dec 13, 2016

Choose a reason for hiding this comment

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

window.top will crash the browser when called from inside a cross-domain iframe. Please wrap window.top in a try/catch block at minimum

Copy link
Contributor Author

@onaydenov onaydenov Dec 14, 2016

Choose a reason for hiding this comment

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

Done, wrapped window.top in a try/catch block

src/adapters/fidelity.js Outdated
}

function addBlankBidResponses(placementsWithBidsBack) {
var allFidelityBidRequests = $$PREBID_GLOBAL$$._bidsRequested.find(bidSet => bidSet.bidderCode === 'fidelity');
Copy link
Collaborator

@matthewlane matthewlane Dec 13, 2016

Choose a reason for hiding this comment

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

You've defined 'fidelity' in FIDELITY_BIDDER_NAME on line 8, could use that here to avoid duplicating the string

Copy link
Contributor Author

@onaydenov onaydenov Dec 14, 2016

Choose a reason for hiding this comment

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

Definition 'fidelity' replaced with variable FIDELITY_BIDDER_NAME

src/adapters/fidelity.js Outdated
if (!utils.contains(placementsWithBidsBack, fidelityBid.placementCode)) {
// Add a no-bid response for this placement.
var bid = bidfactory.createBid(STATUS.NO_BID, fidelityBid);
bid.bidderCode = 'fidelity';
Copy link
Collaborator

@matthewlane matthewlane Dec 13, 2016

Choose a reason for hiding this comment

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

FIDELITY_BIDDER_NAME could go here also

Copy link
Contributor Author

@onaydenov onaydenov Dec 14, 2016

Choose a reason for hiding this comment

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

Definition 'fidelity' replaced with variable FIDELITY_BIDDER_NAME hete too.

src/adapters/fidelity.js Outdated
function _callBids(params) {
var bids = params.bids || [];
bids.forEach(function (currentBid) {
if (!document.MAX_used) document.MAX_used = ',';
Copy link
Collaborator

@matthewlane matthewlane Dec 13, 2016

Choose a reason for hiding this comment

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

I'm curious about why a variable is being attached to the document object like this? Could an internal function variable be used instead?

Copy link
Contributor Author

@onaydenov onaydenov Dec 14, 2016

Choose a reason for hiding this comment

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

This one has been removed

src/adapters/fidelity.js Outdated
if (document.referrer) m3_u += '&referer=' + encodeURIComponent(document.referrer);
if (document.context) m3_u += '&context=' + encodeURIComponent(document.context);
if (currentBid.params.click) {
document.MAX_ct0 = decodeURI(currentBid.params.click);
Copy link
Collaborator

@matthewlane matthewlane Dec 13, 2016

Choose a reason for hiding this comment

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

Curious about attaching a variable to the document object here too

Copy link
Contributor Author

@onaydenov onaydenov Dec 14, 2016

Choose a reason for hiding this comment

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

This one has been removed also

@onaydenov
Copy link
Contributor Author

onaydenov commented Dec 14, 2016

Hi Matt @matthewlane ,
We'll give an update on all the points mentioned above ASAP.

@onaydenov
Copy link
Contributor Author

onaydenov commented Dec 14, 2016

We'll perform additional checks to discover why we see this - "CI build failed"

@onaydenov
Copy link
Contributor Author

onaydenov commented Dec 14, 2016

Test #1276 passed successfully. It looks like everything is correct. Let me know if we should do anything else.

@matthewlane matthewlane merged commit a65dc98 into prebid:master Dec 14, 2016
@onaydenov
Copy link
Contributor Author

onaydenov commented Dec 15, 2016

Hello Matt @matthewlane
Thank you.
Please advise when prebid.js with our adapter could be available for download from the main website? Could you also let me know when adapter name may appear here http://prebid.org/download.html and at parameters description page?

@mkendall07
Copy link
Contributor

mkendall07 commented Dec 15, 2016

@onaydenov
Usually a few days after release. We have a release planned for Friday. Thanks

@mkendall07
Copy link
Contributor

mkendall07 commented Dec 15, 2016

@onaydenov
Also please add a PR here to add your bidder docs: https://github.com/prebid/prebid.github.io/tree/master/dev-docs/bidders

@onaydenov
Copy link
Contributor Author

onaydenov commented Dec 16, 2016 •
edited
Loading

@mkendall07
Thank you for info. PR #145 has been submitted into proper tree.
Could you please use name "Fidelity Media" on the site.

mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
@vzhukovsky
Merge pull request prebid#68 in AOLP_ADS_JS/prebid.js from feature/Pr…
0758d4d 
…ebid-official-0.17.0 to release/1.12.0

* commit 'bce975978c2c088d301d120b949d38080ca9d314':
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
@vzhukovsky
Merge pull request prebid#70 in AOLP_ADS_JS/prebid.js from release/1.…
a8df144 
…12.0 to master

* commit '728f1763ce2a282a757546e934199313a4771a21':
  Fix failing unit test on CI.
  Add changelog entry.
  Prebid Release 0.17.0
  remove hb_pb targeting key for deal bids with no cpm (prebid#881)
  Fix for bug prebid#866 (prebid#867)
  Allow changing of 'ga' global variable (prebid#832)
  emit auction end event before bidsBackHandler callback (prebid#884)
  Add Widespace adapter (prebid#846)
  prevent rubicon adapter from registering two bids on exceptions (prebid#854)
  Code Refactoring - Upgrading end point. (prebid#826)
  Ignore test html pages (prebid#878)
  Detect browser width instead of the screen width (prebid#837)
  New aardvark adapter with support for aliasing (prebid#875)
  Add Fidelity adapter (prebid#862)
  Adkernel adapter aliasing (prebid#857)
  0.16.1-pre
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewlane matthewlane matthewlane approved these changes

Assignees

@matthewlane matthewlane

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@onaydenov @mkendall07 @matthewlane

Comments