OpenX Adapter Update: #1438

lntho · 2017-07-28T22:13:32Z

Implementing AJAX calls for ad requests
Adding prebid trasaction id to ad request
Updated timeout to use the timeout passed as params
Implemented cache busting on ad request
Add tbd feature field to bidResponse

Type of change

Bugfix
Feature

Description of change

Implementing AJAX calls for ad requests
Adding prebid trasaction id to ad request
Updated timeout to use the timeout passed as params
Implemented cache busting on ad request
Add tbd feature field to bidResponse

snapwich · 2017-07-31T22:54:44Z

modules/openxBidAdapter.js

@@ -187,10 +203,25 @@ const OpenxAdapter = function OpenxAdapter() {
      }
    });

-    params.callback = 'window.$$PREBID_GLOBAL$$.oxARJResponse';
-    let queryString = buildQueryStringFromParams(params);
+    if (window.XMLHttpRequest && 'withCredentials' in new XMLHttpRequest()) {


withCredentials should be available in any browser version that Prebid.js supports. Are you seeing otherwise?

Unfortunately we require the credentials for our ajax support to function correctly, so we must do this check here

snapwich · 2017-07-31T22:56:59Z

modules/openxBidAdapter.js


-    adloader.loadScript(`//${delDomain}/w/1.0/arj?${queryString}`);
+    if (useJsonp) {


It would be ideal to just remove Jsonp altogether if the ajax is working correctly. Also, to switch to a scoped callback rather than using a pbjs global function.

As mentioned in the previous comment, there are times where we cannot support ajax so we can't really remove jsonp completely. What did you mean by the switching to a scoped callback?

When can you not support AJAX? What I'm trying to say, is in any browser that doesn't support AJAX won't support Prebid.js, so if you can run Prebid.js then you should be able to use AJAX, including withCredentials always.

Hello! We have some special cases to support IE9 which doesn't fully support Ajax yet, so it is best to check withCredentials and have a fallback to jsonp.

@snapwich Hello there! I just noticed that prebid supports only IE10+ now, just wanted to confirm with you on that. If that is the case then we're okay with removing jsonp completely and I'll get to working on that.

snapwich · 2017-07-31T23:01:46Z

modules/openxBidAdapter.js

  let startTime;
+  let timeout = $$PREBID_GLOBAL$$.bidderTimeout;


This is going to always be the default value since any custom timeouts will be set in a pbjs.queue block which will execute after adapter initialization. It might be best for you to always just look at $$PREBID_GLOBAL$$.bidderTimeout so that you know you're getting the latest.

We are trying capture the timeout at the time of request and store it until time of response. Do you know if this global stays constant throughout? We have observed that sometimes it seems the value changes so that's why I'm doing this to make sure.

I didn't notice that you're overwriting the timeout below from the bidderRequest params. That should be accurate. 👍

Regarding the timeout, there is at least one case which is not captured. If the old style PREBID_TIMEOUT variable is used in the style

var PREBID_TIMEOUT = 700;
// Load GPT when timeout is reached.
setTimeout(initAdserver, PREBID_TIMEOUT);

like it's done in the example the bidmanager will have no clue about it and default to 3000 instead.

Therefore, I'd recommend
bt: Math.min($$PREBID_GLOBAL$$.cbTimeout || $$PREBID_GLOBAL$$.bidderTimeout, window.PREBID_TIMEOUT || $$PREBID_GLOBAL$$.bidderTimeout)
instead

I agree with this to try to capture the PREBID_TIMEOUT on the window but we've observed the inside cbTimeout being changed at the time of response, hence the change to save the one coming from the paramter for later use. I'll make the changes accordingly to try to capture the one on the window.

snapwich

Left a few comments

lntho · 2017-08-16T22:43:39Z

@snapwich after talking with the team, I removed the JSONP requests completely. Please let me know if there are more changes needed. Thanks.

dbemiller · 2017-08-17T18:43:13Z

modules/openxBidAdapter.js

+    try {
+      let queryString = buildQueryStringFromParams(params);
+      let url = `//${delDomain}/w/1.0/arj?${queryString}`;
+      ajax.ajax(url, $$PREBID_GLOBAL$$.oxARJResponse, void (0), {


Now that JSONP is gone, is there any reason for this function to be on the global?

If not, let's remove it.

dbemiller · 2017-08-17T18:44:42Z

test/spec/modules/openxBidAdapter_spec.js

-    assert(!spyLoadScript.called);
-    spyLoadScript.restore();
+    assert(!spyAjax.called);
+    spyAjax.restore();


spies, stubs, etc need to be done in beforeEach or afterEach blocks.

Otherwise spyAjax.restore() won't execute when your test fails, and it causes tons of unrelated cascading failures (sinon won't let you wrap an already-wrapped function)

1) Implementing AJAX calls for ad requests 2) Adding prebid trasaction id to ad request 3) Updated timeout to use the timeout passed as params 4) Implemented cache busting on ad request 5) Add tbd feature field to bidResponse

lntho · 2017-08-21T23:10:20Z

@dbemiller I have removed the callback from the global, as well as changed the unit test structure to incorporate beforeEach and afterEach. Please let me know if there are any other things you see.

…built * 'master' of https://github.com/prebid/Prebid.js: (83 commits) feat(strAdapt): check if tagJS is already present (prebid#1500) Updated karma-mocha, and simplified the test framework for runs which dont include --watch. (prebid#1520) Revert "drop specific code for index adapter (prebid#1487)" (prebid#1529) Override default asset params when set on ad unit (prebid#1524) Adding new kv to xhb Adapter (prebid#1513) removing for...of loops because IE cannot handle them properly (prebid#1523) Increment pre version Prebid 0.27.0 Release Move unit test file to appropriate location (prebid#1516) Support 'cta' native asset (prebid#1505) Add adapter parameter types (prebid#1504) Register bid adapter (prebid#1514) Match port when testing legacy browser (prebid#1511) modify 'featureforward' adapter to be an indepandant adapter (prebid#1288) Feature/s2s client side fallback (prebid#1485) Make bid.vastUrl use the cache URL if the bid didnt already have one. (prebid#1506) OpenX Adapter Update: (prebid#1438) Add session id feature for roxot analytics adapter (prebid#1498) Update platformioBidAdapter (prebid#1493) Adding VAST Payload support for video bids (prebid#1407) ...

1) Implementing AJAX calls for ad requests 2) Adding prebid trasaction id to ad request 3) Updated timeout to use the timeout passed as params 4) Implemented cache busting on ad request 5) Add tbd feature field to bidResponse

lntho force-pushed the openx-release branch from e6a52b2 to 24c8e6a Compare July 28, 2017 22:23

dbemiller assigned ndhimehta Jul 31, 2017

ndhimehta assigned snapwich Jul 31, 2017

snapwich reviewed Jul 31, 2017

View reviewed changes

snapwich requested changes Jul 31, 2017

View reviewed changes

dbemiller added the needs update label Aug 10, 2017

lntho force-pushed the openx-release branch 2 times, most recently from b4ee232 to efb9515 Compare August 16, 2017 22:38

snapwich previously approved these changes Aug 17, 2017

View reviewed changes

snapwich added LGTM and removed needs update labels Aug 17, 2017

dbemiller self-requested a review August 17, 2017 18:42

dbemiller assigned dbemiller and unassigned snapwich and ndhimehta Aug 17, 2017

dbemiller suggested changes Aug 17, 2017

View reviewed changes

dbemiller added the needs update label Aug 17, 2017

OpenX Adapter Update:

cc61389

1) Implementing AJAX calls for ad requests 2) Adding prebid trasaction id to ad request 3) Updated timeout to use the timeout passed as params 4) Implemented cache busting on ad request 5) Add tbd feature field to bidResponse

lntho dismissed snapwich’s stale review via cc61389 August 21, 2017 23:02

lntho force-pushed the openx-release branch from efb9515 to cc61389 Compare August 21, 2017 23:02

dbemiller approved these changes Aug 22, 2017

View reviewed changes

dbemiller merged commit afc36d1 into prebid:master Aug 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenX Adapter Update: #1438

OpenX Adapter Update: #1438

lntho commented Jul 28, 2017

snapwich Jul 31, 2017

lntho Aug 1, 2017

snapwich Jul 31, 2017 •

edited

Loading

lntho Aug 1, 2017

snapwich Aug 2, 2017

lntho Aug 3, 2017

lntho Aug 11, 2017

snapwich Jul 31, 2017

lntho Aug 1, 2017

snapwich Aug 2, 2017

DubKF Aug 11, 2017 •

edited

Loading

lntho Aug 11, 2017

snapwich left a comment

lntho commented Aug 16, 2017

dbemiller Aug 17, 2017

dbemiller Aug 17, 2017

lntho commented Aug 21, 2017


		adloader.loadScript(`//${delDomain}/w/1.0/arj?${queryString}`);
		if (useJsonp) {

		let startTime;
		let timeout = $$PREBID_GLOBAL$$.bidderTimeout;

OpenX Adapter Update: #1438

OpenX Adapter Update: #1438

Conversation

lntho commented Jul 28, 2017

Type of change

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich Jul 31, 2017 • edited Loading

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

DubKF Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich left a comment

Choose a reason for hiding this comment

lntho commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lntho commented Aug 21, 2017

snapwich Jul 31, 2017 •

edited

Loading

DubKF Aug 11, 2017 •

edited

Loading