Update AdButler adapter for Prebid v1.0 #1664

dharton · 2017-10-06T20:58:25Z

Type of change

Description of change

Update AdButler adapter for Prebid v1.0

test parameters for validating bids

{
  bidder: 'adbutler',
  params: {
    accountID: 167283,
    zoneID: 210093
  }
}

contact email of the adapter’s maintainer: dan@sparklit.com
official adapter submission

Only attempt to build a bid response if we have the information of which bid to respond to.

Now stubbing adLoader instead of spying. Additional changes to ensure all tests still passed.

Prevent AdButler TypeErrors and pass bid request object into the bid response.

Add optional domain parameter to AdButler adapter.

# Conflicts: # src/adapters/adbutler.js # test/spec/adapters/adbutler_spec.js

dbemiller · 2017-10-09T13:09:31Z

test/spec/modules/adbutlerBidAdapter_spec.js

-            placementCode: '/123456/header-bid-tag-1'
+import {expect} from 'chai';
+import adapterManager from 'src/adaptermanager';
+import bidManager from 'src/bidmanager';


We probably should have been more explicit about this in our docs, but... the bidmanager is going away in 1.0. The good news is that your tests can be simplified.

New your tests can just create objects with dummy data (bid request, server response, etc), pass them into the spec methods directly, and then make assertions on the return values. No need to worry about mocks, fake servers, or anything like that. The tests on the bidderFactory will make sure that your adapter's bids make it into the auction.

Check out the appnexusAst adapter for some working examples..

dbemiller · 2017-10-09T13:13:28Z

modules/adbutlerBidAdapter.js

+export const spec = {
+  code: BIDDER_CODE,
+  pageID: Math.floor(Math.random() * 10e6),
+  zoneCounters: {},


No mutable globals allowed... see the 1.0 conventions.

Can this be moved inside your buildRequests function?

dbemiller · 2017-10-09T13:17:17Z

modules/adbutlerBidAdapter.js

+        bidResponse.width = width;
+        bidResponse.height = height;
+        bidResponse.ad = serverResponse.ad_code;
+        bidResponse.ad += spec.addTrackingPixels(serverResponse.tracking_pixels);


Adapters shouldn't add tracking pixels during the auction anymore. Implement getUserSyncs instead. prebid-core will make sure those pixels get added to the page alongside everyone else's

Thanks for the info @dbemiller. We offer our customers the option to track impressions on the client-side. If they've chosen this, their impression pixel would have been added here. Just to verify, if the publisher has userSync disabled, this means there is a chance their impression wouldn't be counted in this way. Is that correct?

Correct. Since tracking pixels affect the publisher's page performance, the prebid leadership decided that they should have the ultimate decision in whether or not to allow them.

That said, most publishers leave the default options... so it's not likely to impact you too much.

I understand the desire to improve publisher page performance. My only concern is that when all syncs are blocked, the winning bidder will not have any way to know that they have won a particular auction. Is there any way for us to provide Prebid with a URL that can be pinged or placed if our bid has won the auction?

I think there might be some confusion here. It's acceptable to add a tracking pixel to a bid response. This will only fire if the bid wins the auction. I think that's what this is so should be fine.

Ah, thanks for the clarification. Yeah, this function just appends the tracking pixel(s) to the ad code in the bid response.

dbemiller · 2017-10-09T13:34:21Z

Also: don't forget to add your .md file: http://prebid.org/dev-docs/bidder-adapter-1.html#planning-your-adapter

dharton · 2017-10-12T00:06:41Z

Thanks for your help! I've committed the recommended changes.

dbemiller · 2017-10-12T12:59:51Z

@mkendall07 to the rescue!... sorry about that.

dbemiller · 2017-10-12T13:18:48Z

This code looks good to me... the only thing preventing merge from my POV is the failing tests.

I expected this to be one of those intermittent failures, since it's the c1x adapter tests... but it doesn't look like it is. The tests pass in master, and fail reliably in this branch. Looks like they're doing all sorts of things with window and $$PREBID_GLOBAL$$, so I wouldn't be surprised if there was some weird interdependence on other peoples' tests in there somewhere.

dharton · 2017-10-12T20:06:18Z

Hmm. It's strange that my changes are causing an issue since, with the new Prebid requirements, they don't use either window or $$PREBID_GLOBAL$$.

dharton · 2017-10-16T21:02:02Z

Hey @dbemiller , I just wanted to give you a bit of an update. I've figured out why the c1x test are failing. In their adapter they are referencing $$PREBID_GLOBAL$$.adUnits. Somewhere before the module tests, the adUnits get filled up with a lot of bidders. Then (in my test file) it gets filled in with just one bidder. This allows the c1x test to pass the final test case. Now that my new tests don't make use of that those ad units, the c1x test is failing on that larger amount of bidders. If you'd like, I could send along some screenshots to show you what I mean.

Do you think you'd be able to merge my changes in, or will we have to wait for them to update?

dbemiller · 2017-10-17T13:03:13Z

@dharton we definitely can't merge something with broken tests. It would break the CI, so then the tests would fail on everyone else's PRs.

If it's just their tests which are broken, I'd recommend fixing them here. We haven't heard back from them about prebid 1.0 updates yet (to my knowledge)... so they might not ever come back to update & fix them.

This is always a risk of unit tests with side effects. Your new code looks like it's much safer.

dbemiller · 2017-10-17T15:25:11Z

One other issue I just noticed... this PR introduces an empty file: test/spec/adapters/adbutler_spec.js. Please delete it before we merge.

dbemiller · 2017-10-18T12:51:32Z

looks great... nice code!

* 'master' of https://github.com/prebid/Prebid.js: (414 commits) Make response headers available to the specs (prebid#1748) add option to run tests in a specific file (prebid#1727) Update JCM Adapter to 1.0 (prebid#1715) Finished an unfinished comment. (prebid#1749) Platform.io Bidder Adapter update. Prebid v1.0. (prebid#1705) Fix window.top.host cross origin issue when in nested iframes. (prebid#1730) fix log message not displaying when referencing missing bidder (prebid#1737) Allow more than one placement from one page (prebid#1692) Justpremium Adapter bugfix (prebid#1716) Updating license (prebid#1717) realvuBidAdapter (prebid#1571) Update JSDoc to call the module `pbjs` (prebid#1572) Update Beachfront adapter for v1.0 (prebid#1675) Update AdButler adapter for Prebid v1.0 (prebid#1664) Increment pre version Fix for prebid#1628 (allowing standard bidCpmAdjustment) (prebid#1645) Prebid 0.31.0 Release Support native click tracking (prebid#1691) Initial commit for video support for pbs (prebid#1706) Fixes: Immediate adapter response may end auction (prebid#1690) ...

dkharton and others added 24 commits October 14, 2016 14:04

Adding AdButler Adapter

771b74c

Prevent AdButler TypeError

45919de

Only attempt to build a bid response if we have the information of which bid to respond to.

Refactor AdButler Testing

5e72aac

Now stubbing adLoader instead of spying. Additional changes to ensure all tests still passed.

Prevent AdButler TypeErrors

10ab721

Prevent AdButler TypeErrors and pass bid request object into the bid response.

Merge remote-tracking branch 'refs/remotes/prebid/master'

38ad684

Merge remote-tracking branch 'refs/remotes/prebid/master'

79e7045

Merge remote-tracking branch 'prebid/master'

7fe5aa1

Merge remote-tracking branch 'refs/remotes/prebid/master'

1cf22f4

Merge remote-tracking branch 'refs/remotes/prebid/master'

eca8d59

Merge remote-tracking branch 'refs/remotes/prebid/master'

252ffac

Merge remote-tracking branch 'refs/remotes/prebid/master'

add7a75

Merge remote-tracking branch 'refs/remotes/prebid/master'

35b8180

Merge branch 'master' of https://github.com/sparklit/Prebid.js

2cdc25b

Merge remote-tracking branch 'refs/remotes/prebid/master'

f4cac5d

Merge remote-tracking branch 'refs/remotes/prebid/master'

c0ef919

Merge remote-tracking branch 'refs/remotes/prebid/master'

8f85d45

Add optional domain parameter.

1d4480a

Add optional domain parameter to AdButler adapter.

Merge remote-tracking branch 'refs/remotes/prebid/master'

4efc579

Merge remote-tracking branch 'remotes/prebid/master'

42b3c41

# Conflicts: # src/adapters/adbutler.js # test/spec/adapters/adbutler_spec.js

Merge remote-tracking branch 'remotes/prebid/master'

28df88d

Merge remote-tracking branch 'remotes/prebid/master'

5cc58bf

Merge remote-tracking branch 'remotes/prebid/master'

cdea438

Update AdButler adapter to Prebid 1.0

09804e4

Code Style updates based on lint warnings.

0117275

matthewlane added the needs review label Oct 7, 2017

matthewlane requested a review from dbemiller October 7, 2017 00:18

matthewlane assigned dbemiller Oct 7, 2017

dbemiller suggested changes Oct 9, 2017

View reviewed changes

dbemiller added needs update and removed needs review labels Oct 9, 2017

Removed mutable global, simplified tests, and added markdown file.

5af952b

mjacobsonny added the Prebid 1.0 label Oct 14, 2017

Update c1x adapter tests & remove old adbutler_spec file.

7ef0341

dbemiller approved these changes Oct 18, 2017

View reviewed changes

dbemiller added LGTM and removed needs update labels Oct 18, 2017

jaiminpanchal27 approved these changes Oct 18, 2017

View reviewed changes

jaiminpanchal27 merged commit 89a8ed6 into prebid:master Oct 18, 2017

CathyHuangtw mentioned this pull request Nov 17, 2017

[Internal Code Review] C1X adapter for Prebid v1.0 jpteam/Prebid.js#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update AdButler adapter for Prebid v1.0 #1664

Update AdButler adapter for Prebid v1.0 #1664

dharton commented Oct 6, 2017 •

edited

Loading

dbemiller Oct 9, 2017

dbemiller Oct 9, 2017

dbemiller Oct 9, 2017

dharton Oct 10, 2017

dbemiller Oct 11, 2017

dharton Oct 11, 2017

mkendall07 Oct 11, 2017

dharton Oct 11, 2017

dbemiller commented Oct 9, 2017

dharton commented Oct 12, 2017

dbemiller commented Oct 12, 2017

dbemiller commented Oct 12, 2017 •

edited

Loading

dharton commented Oct 12, 2017

dharton commented Oct 16, 2017

dbemiller commented Oct 17, 2017

dbemiller commented Oct 17, 2017

dbemiller commented Oct 18, 2017

Update AdButler adapter for Prebid v1.0 #1664

Update AdButler adapter for Prebid v1.0 #1664

Conversation

dharton commented Oct 6, 2017 • edited Loading

Type of change

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller commented Oct 9, 2017

dharton commented Oct 12, 2017

dbemiller commented Oct 12, 2017

dbemiller commented Oct 12, 2017 • edited Loading

dharton commented Oct 12, 2017

dharton commented Oct 16, 2017

dbemiller commented Oct 17, 2017

dbemiller commented Oct 17, 2017

dbemiller commented Oct 18, 2017

dharton commented Oct 6, 2017 •

edited

Loading

dbemiller commented Oct 12, 2017 •

edited

Loading