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

Added MobFox Adapter #1312

Merged
merged 11 commits into from
Jul 26, 2017
Merged

Added MobFox Adapter #1312

merged 11 commits into from
Jul 26, 2017

Conversation

francoroy
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • [ x] 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

{
  bidder: 'mobfox',
  params: {
    s: "267d72ac3f77a3f447b32cf7ebf20673" // required - The hash of your inventory to identify which app is making the request,
    imp_instl: 1 // optional - set to 1 if using interstitial otherwise delete or set to 0
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

@francoroy Modules are introduced in Prebid with this #1177

So you need to update your Adapter by doing some minor changes. you can check here https://github.com/prebid/Prebid.js/pull/1177/files how you can migrate your code.

Also i left some comments on code.

ajax.ajax(`${BID_REQUEST_BASE_URL}?${queryString}`, {
success(resp, xhr) {
if (xhr.getResponseHeader("Content-Type") == "application/json")
resp = JSON.parse(resp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrap all JSON.parse in try catch block

@francoroy
Copy link
Contributor Author

Hi, any progress?
Thanks

@jaiminpanchal27
Copy link
Collaborator

@francoroy Changes looks good. But not able to validate bids.
I am using hello-wold test page

@francoroy
Copy link
Contributor Author

@jaiminpanchal27 Thanks. I can share my hello world page:

var adUnits = [{
        code: 'div-gpt-ad-1460505748561-0',
        sizes: [[320, 480]],

        // Replace this object to test a new Adapter!
        bids: [{
          bidder: 'mobfox',
          params: {
            s: '267d72ac3f77a3f447b32cf7ebf20673',
            imp_instl: 1
          }
        }]

      }];

@jaiminpanchal27
Copy link
Collaborator

@francoroy Got it. Thanks.
Now able to see ad with size 320x480.

@dbemiller
Copy link
Contributor

dbemiller commented Jul 12, 2017

I did find a bit of an issue here...

If your server sends back a "no bid available" error, the adapter is still trying to add the bid to the bidmanager.

This was likely working when you wrote it, but changes in #1277 (in particular, removing this check) cause an undefined reference now. You can reproduce the issue with this:

      var adUnits = [{
        code: 'div-gpt-ad-1460505748561-0',
        sizes: [[300, 250], [300,600]],

        // Replace this object to test a new Adapter!
        bids: [{
          bidder: 'mobfox',
          params: {
            s: "267d72ac3f77a3f447b32cf7ebf20673", // required - The hash of your inventory to identify which app is making the request,
            imp_instl: 1 // optional - set to 1 if using interstitial otherwise delete or set to 0
          }
        }]
      }];

Since my changes were unintended, I'm replacing the old behavior in #1372... but I don't think this is ideal. If an adapter doesn't have a bid to add, then it shouldn't be calling the addBidResponse method.

@francoroy
Copy link
Contributor Author

francoroy commented Jul 20, 2017

Hi @dbemiller
"Since my changes were unintended, I'm replacing the old behavior in #1372... but I don't think this is ideal. If an adapter doesn't have a bid to add, then it shouldn't be calling the addBidResponse method"

So I don't understand, should I or should I not call addBidResponse when no bid is available?

@dbemiller
Copy link
Contributor

dbemiller commented Jul 20, 2017

@francoroy
Copy link
Contributor Author

@dbemiller this is what I already do in my onBidResponseError function:

let bidResponse = bidfactory.createBid(CONSTANTS.STATUS.NO_BID, bid);
bidResponse.bidderCode = BIDDER_CODE;
bidmanager.addBidResponse(bid.placementCode, bidResponse);

Is it ok then?

@dbemiller
Copy link
Contributor

dbemiller commented Jul 20, 2017

@francoroy You're calling addBidResponse twice, and the second call uses an undefined bid. See the attached screenshot & test page. I set a breakpoint right before the page breaks.

screen shot 2017-07-20 at 4 32 45 pm

<!--

  This page calls a single bidder for a single ad slot. It can be considered a "hello world" example for using
  Prebid with the Google Publisher Tag.

  It also makes a good test page for new adapter PR submissions. Simply set your server's Bid Params object in the
  bids array inside the adUnits, and it will use your adapter to load an ad.

  NOTE that many ad servers won't send back an ad if the URL is localhost... so you might need to
  set an alias in your /etc/hosts file so that you can load this page from a different domain.

-->

<html>
<head>
    <script>
      var PREBID_TIMEOUT = 700;

      var adUnits = [{
        code: 'div-gpt-ad-1460505748561-0',
        sizes: [[300, 250], [300,600]],

        // Replace this object to test a new Adapter!
        bids: [{
          bidder: 'mobfox',
          params: {
            s: "267d72ac3f77a3f447b32cf7ebf20673", // required - The hash of your inventory to identify which app is making the request,
            imp_instl: 1 // optional - set to 1 if using interstitial otherwise delete or set to 0
          }
        }]
      }];

      var pbjs = pbjs || {};
      pbjs.que = pbjs.que || [];

    </script>

    <script type="text/javascript" src="../../build/dev/prebid.js" async></script>
    <script>
      var googletag = googletag || {};
      googletag.cmd = googletag.cmd || [];
      googletag.cmd.push(function() {
        googletag.pubads().disableInitialLoad();
      });

      pbjs.que.push(function() {
        pbjs.addAdUnits(adUnits);
        pbjs.requestBids({
          bidsBackHandler: sendAdserverRequest
        });
      });

      function sendAdserverRequest() {
        if (pbjs.adserverRequestSent) return;
        pbjs.adserverRequestSent = true;
        googletag.cmd.push(function() {
          pbjs.que.push(function() {
            pbjs.setTargetingForGPTAsync();
            googletag.pubads().refresh();
          });
        });
      }

      setTimeout(function() {
        sendAdserverRequest();
      }, PREBID_TIMEOUT);

    </script>

    <script>
      (function () {
        var gads = document.createElement('script');
        gads.async = true;
        gads.type = 'text/javascript';
        var useSSL = 'https:' == document.location.protocol;
        gads.src = (useSSL ? 'https:' : 'http:') +
          '//www.googletagservices.com/tag/js/gpt.js';
        var node = document.getElementsByTagName('script')[0];
        node.parentNode.insertBefore(gads, node);
      })();
    </script>

    <script>
      googletag.cmd.push(function () {
        googletag.defineSlot('/19968336/header-bid-tag-0', [[300, 250], [300, 600]], 'div-gpt-ad-1460505748561-0').addService(googletag.pubads());

        googletag.pubads().enableSingleRequest();
        googletag.enableServices();
      });
    </script>
</head>

<body>
<h2>Prebid.js Test</h2>
<h5>Div-1</h5>
<div id='div-gpt-ad-1460505748561-0'>
    <script type='text/javascript'>
      googletag.cmd.push(function() { googletag.display('div-gpt-ad-1460505748561-0'); });
    </script>
</div>
</body>
</html>

@francoroy
Copy link
Contributor Author

@dbemiller
Hi, thanks for the review.
I didn't manage to reproduce the issue with your test page but I found the bug and fixed it (onBidResponseError was called twice on invalid json).

Thanks

@dbemiller
Copy link
Contributor

@francoroy I'm still getting the exact same error. I've attached a screenshot from the same spot, but looking at your code.

screen shot 2017-07-24 at 12 14 54 pm

Your server is responding with a 200, but the response is {"error": "no Ad Available"}. transformResponse returns undefined, and onBidResponse tries to add "undefined" as a bid response.

It reproduces in your unit tests, too:

 let bidResponse1 = stubAddBidResponse.getCall(0).args[1];
 expect(stubAddBidResponse.calledTwice).to.equal(true); // This passes, but addBidResponse should only be called once
expect(bidPlacementCode1).to.equal('test-gpt-div-1234');

@francoroy
Copy link
Contributor Author

@dbemiller Thanks, I've fixed it

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

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

Looks good now.

I would recommend expanding your unit tests to catch this case, to prevent future regressions... but I won't require it for merge.

@dbemiller dbemiller merged commit 03d86a0 into prebid:master Jul 26, 2017
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Jul 28, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (95 commits)
  Specify --browsers when using gulp test --watch (prebid#1420)
  Added aliases for aol adapter. (prebid#1371)
  Added MobFox Adapter (prebid#1312)
  Fixed style error. (prebid#1419)
  Add native support for Criteo adapter (prebid#999)
  Update admediaBidAdapter.js (prebid#1395)
  Increment pre version (prebid#1413)
  Prebid 0.26.1 Release (prebid#1412)
  fix prebid#1410 - issue with ie and xhr.timeout (prebid#1411)
  Lint modules directory (prebid#1404)
  Set outstream mediaType based on renderer in response (prebid#1391)
  Fixing the BidAdjustmentEvent fire time (prebid#1399)
  Fix banner showing up in prebid-core.js (prebid#1388)
  Mention NodeJS 4.0 dependency in the README (prebid#1386)
  Increment pre version (prebid#1385)
  Prebid 0.26.0 Release (prebid#1384)
  PulsePoint Lite adapter - adding createNew method for aliasing. (prebid#1383)
  Modernizing build dependencies (prebid#1355)
  StickyAdsTV bidder adapter update (prebid#1311)
  Added CPM value as parameter for Vertoz Adapter (prebid#1306)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants