HIRO Media: Remove batching mechanism and use AJAX instead of JSONP #1133

ronenst · 2017-04-18T15:38:19Z

Type of change

Refactoring (no functional changes, no api changes)

Description of change

Switched to AJAX instead of JSONP
The mechanism that batched multiple requests according to specific keys has been removed. Each placement will now invoke a request.

These changes are related since the generated batchKey was also used as a request id in the JSONP responses. No such id is necessary with AJAX.

contact email of the adapter’s maintainer
support@hiro-media.com
official adapter submission

dbemiller

Hey, can you post some valid bid params that I can use for testing? The code and tests looks solid, minus my few comments below.

dbemiller · 2017-05-01T18:59:33Z

src/adapters/hiromedia.js

-      handleResponse(response);
+    // Sample the bid responses according to `response.chance`,
+    // if `response.chance` is not provided, sample at 100%.
+    if (response.chance === undefined || checkChance(response.chance)) {


Should use typeof here, in case someone re-defines undefined on their page.

See (6) from the top answer here: https://www.quora.com/What-is-the-best-way-to-check-if-a-property-or-variable-is-undefined

dbemiller · 2017-05-01T19:07:32Z

src/adapters/hiromedia.js

-      // batchKey has not been sent.
-      _bidStorage.forEach(function (bidInfo) {
+          Object.keys(requestParams).forEach(function (key){
+            if (requestParams[key] === '' || requestParams[key] === undefined) {


ronenst · 2017-05-03T12:34:08Z

Thank you for the review, I've pushed a fix for the undefined issue.

You should be able test the adapter with the test account:

{
	bidder: 'hiromedia',
	params: {
		accountId: '1'
	}
}

dbemiller · 2017-05-03T13:18:30Z

Not seeing the ad when I test with those params...

<html>
<head>
    <!-- Prebid Config Section START -->
    <!-- Make sure this is inserted before your GPT tag -->
    <script>
      var PREBID_TIMEOUT = 700;

      var adUnits = [{
        code: 'div-gpt-ad-1460505748561-0',
        sizes: [[300, 250], [300,600]],
        bids: [{
          bidder: 'hiromedia',
          params: {
            accountId: '1'
          }
        }]
      }];

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

    </script>
    <!-- Prebid Config Section END -->

    <!-- Prebid Boilerplate Section START. No Need to Edit. -->
    <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>
    <!-- Prebid Boilerplate Section END -->

    <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>

ronenst · 2017-05-04T11:25:55Z

Please try again, it should work now, sorry about that.

dbemiller · 2017-05-04T13:11:48Z

Still no?

I see your server's response... but it's not making it back into the page.

dbemiller · 2017-05-04T13:39:34Z

I think because your bid doesn't have a cpm? Here's a console.log(pbjs._bidsReceived)

ronenst · 2017-05-04T13:59:49Z

Thanks again, should be resolved now. It was a Geo-location issue, I'm non-US so I was getting a different response. We'll make sure we have a stable test account for future testing.

dbemiller · 2017-05-04T14:05:59Z

gotcha... no worries. This LGTM now.

dbemiller · 2017-05-17T16:18:23Z

hey @ronenst... really sorry about the delay merging this, but it looks like some file conflicts popped up. Would you mind taking a look?

We're planning to release tomorrow... so I'll try to get another reviewer to take a look at it to get it in there.

Thanks

# Conflicts: # src/adapters/hiromedia.js # test/spec/adapters/hiromedia_spec.js

ronenst · 2017-05-17T19:19:00Z

Hi @dbemiller, thanks for the heads up, I've pushed a fix. Hopefully, we can manage to get it into the next release.

Please note the test account parameters have changed and the following parameters should be used:

{
  bidder: 'hiromedia',
  params: {
	accountId: '4'
  }
}

Since there are no code changes in the merge (just white-space), I've opted for a merge commit instead of a rebase so that the reviewed commit is comparable to the new commit, but the diff turned out strange on the GH page, I recommend checking it with white-space changes ignored.

dbemiller · 2017-05-17T19:22:38Z

No problem at all. Still looks fine to me. Thanks!

jaiminpanchal27

LGTM

…rebid#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks

* Added ad unit size to bid request * Fixed lint errors * Added ad unit size to bid request * Prebid 0.23.1 Release * Add trafficSourceCode + test (#1184) * pre release version bump * inclusion of popular Nordic ad sizes to default size list (#1168) inclusion of popularNordic ad sizes to default size list Removed redundant 104 size and added size 32 instead Fixed trailing comma * Add PubWise Analytics (#1151) * PubWise Analytics * Updates based on Feedback * add new rp_secure param to rubicon adapter (#1190) * Add type conversion into PrebidServer to handle inconsistent types. (#1195) * Add type conversion into prebxdserver to handle inconsistent types. * Only check for params that exist. * adding test * fix string conversion and add unit tests * fix for code review * add debug output * remove accidental commit * newline * Add dynamic bidfloor parameter to Smart Adserver Adapter (#1194) * Smart AdServer adapter Add Smart AdServer adapter with tests * fix not supported method Replace startsWith which is not supported in all browser version by lastIndexOf. * Issue with optional parameter Fix issue when no targeting is specified and remove "undefined" value passed in url * Add dynamic bidfloor option in the SmartAdServer prebid call. * Bug fix: bids served by secure creatives does not get pushed into _winningBids (#1192) * Upgrade linters to ESLint with stricter code style (#1111) * Upgraded to ESLint with stricter code style, for both sources and tests * Updated some dependencies and the yarn.lock file * Add Support for DigiTrust in Rubicon Adapter (#1201) * Add support for DigiTrust * Add rubicon tests covering digitrust failures * Remove whitespace in Rubicon adapter * HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks * Rename secureCreatives file and lint (#1203) * Rename secureCreatives file and lint * Updated package script for linting * Use 'gulp run-tests' in package script for testing * updated tag (#1212) * Common user-sync (#1144) * Changed “bidRequest” to “bid” for clarity

…built * 'master' of https://github.com/prebid/Prebid.js: (23 commits) Increment pre version Probed 0.24.0 Release Beachfront adapter - add ad unit size (prebid#1183) Thoughtleadr adapter - fix postMessage (prebid#1207) When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204) Improvement/timeout xhr (prebid#1172) Add native support (prebid#1072) Improvement/alias queue (prebid#1156) Updated documentaion (prebid#1160) Improvement/prebid iframes amp pages (prebid#1119) Fixes prebid#1114 possible xss issue (prebid#1186) Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158) updated tag (prebid#1212) Common user-sync (prebid#1144) Rename secureCreatives file and lint (prebid#1203) HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (prebid#1133) Add Support for DigiTrust in Rubicon Adapter (prebid#1201) Upgrade linters to ESLint with stricter code style (prebid#1111) Add dynamic bidfloor parameter to Smart Adserver Adapter (prebid#1194) Bug fix: bids served by secure creatives does not get pushed into _winningBids (prebid#1192) ...

Remove batching mechanism and use AJAX instead of JSONP

fc2d268

ronenst closed this Apr 18, 2017

ronenst reopened this Apr 18, 2017

protonate requested a review from dbemiller April 28, 2017 16:52

protonate assigned dbemiller Apr 28, 2017

dbemiller reviewed May 1, 2017

View reviewed changes

dbemiller added the needs update label May 2, 2017

Fix undefined value checks

0941f8d

dbemiller added LGTM and removed needs update labels May 4, 2017

dbemiller approved these changes May 4, 2017

View reviewed changes

dbemiller mentioned this pull request May 5, 2017

Integration test page for adapters #1182

Merged

1 task

Merge branch 'master' into hiromedia-adapter-v3

89b84d0

# Conflicts: # src/adapters/hiromedia.js # test/spec/adapters/hiromedia_spec.js

jaiminpanchal27 self-requested a review May 17, 2017 19:48

jaiminpanchal27 approved these changes May 17, 2017

View reviewed changes

jaiminpanchal27 merged commit ca4c66a into prebid:master May 17, 2017

bfmdeploy pushed a commit to bfmdeploy/Prebid.js that referenced this pull request May 19, 2017

HIRO Media: Remove batching mechanism and use AJAX instead of JSONP (p…

4f072e3

…rebid#1133) * Remove batching mechanism and use AJAX instead of JSONP * Fix `undefined` value checks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIRO Media: Remove batching mechanism and use AJAX instead of JSONP #1133

HIRO Media: Remove batching mechanism and use AJAX instead of JSONP #1133

ronenst commented Apr 18, 2017

dbemiller left a comment •

edited

Loading

dbemiller May 1, 2017

dbemiller May 1, 2017

ronenst commented May 3, 2017

dbemiller commented May 3, 2017

ronenst commented May 4, 2017

dbemiller commented May 4, 2017

dbemiller commented May 4, 2017

ronenst commented May 4, 2017

dbemiller commented May 4, 2017

dbemiller commented May 17, 2017

ronenst commented May 17, 2017

dbemiller commented May 17, 2017

jaiminpanchal27 left a comment

HIRO Media: Remove batching mechanism and use AJAX instead of JSONP #1133

HIRO Media: Remove batching mechanism and use AJAX instead of JSONP #1133

Conversation

ronenst commented Apr 18, 2017

Type of change

Description of change

dbemiller left a comment • edited Loading

Choose a reason for hiding this comment

dbemiller May 1, 2017

Choose a reason for hiding this comment

dbemiller May 1, 2017

Choose a reason for hiding this comment

ronenst commented May 3, 2017

dbemiller commented May 3, 2017

ronenst commented May 4, 2017

dbemiller commented May 4, 2017

dbemiller commented May 4, 2017

ronenst commented May 4, 2017

dbemiller commented May 4, 2017

dbemiller commented May 17, 2017

ronenst commented May 17, 2017

dbemiller commented May 17, 2017

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

dbemiller left a comment •

edited

Loading