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.

Sign up for GitHub

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

Bugfix add bid parameters if not present #3808

Merged
merged 44 commits into from
Jun 4, 2019
Merged

Bugfix add bid parameters if not present #3808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
afdf755
initial orbidder version in personal github repo
hendrikiseke1979 Jan 9, 2019
ffc6f0b
use adUnits from orbidder_example.html
hendrikiseke1979 Jan 9, 2019
5c9a947
replace obsolete functions
hendrikiseke1979 Jan 10, 2019
06d127e
forgot to commit the test
hendrikiseke1979 Jan 10, 2019
8617515
check if bidderRequest object is available
hendrikiseke1979 Jan 10, 2019
8b5c884
try to fix weird safari/ie issue
hendrikiseke1979 Jan 10, 2019
4eace10
Merge remote-tracking branch 'upstream/master'
hendrikiseke1979 Jan 14, 2019
8d9dd6f
merge changes from upstream
hendrikiseke1979 Jan 16, 2019
699d05d
fetch changes from upstream
hendrikiseke1979 Jan 17, 2019
367b1a7
ebayK: add more params
hendrikiseke1979 Jan 18, 2019
cfe0911
update orbidderBidAdapter.md
hendrikiseke1979 Jan 18, 2019
4279c11
use spec.<function> instead of this.<function> for consistency reasons
hendrikiseke1979 Jan 21, 2019
5625d93
fetch changes from upstream
hendrikiseke1979 Jan 21, 2019
67dc263
Merge branch 'feature/RAAS-2676'
hendrikiseke1979 Jan 21, 2019
89fb80b
fetch changes from upstream
hendrikiseke1979 Jan 24, 2019
ab665e9
fetch changes from upstream
hendrikiseke1979 Jan 28, 2019
b91d68d
fetch changes from upstream
Feb 1, 2019
393b3fe
fetch changes from upstream
Feb 15, 2019
d0b0cbc
fetch changes from upstream
hendrikiseke1979 Mar 5, 2019
5967f6e
fetch changes from upstream
hendrikiseke1979 Mar 6, 2019
df95b81
fetch changes from upstream
hendrikiseke1979 Mar 13, 2019
6169ec7
fetch changes from upstream
Mar 15, 2019
b05496f
add bidfloor parameter to params object
Mar 15, 2019
5a3d816
fetch changes from upstream
Mar 15, 2019
4c95866
fetch changes from upstream
hendrikiseke1979 Apr 2, 2019
e27e3dd
fetch changes from upstream
hendrikiseke1979 Apr 3, 2019
1470e8e
fetch changes from upstream
hendrikiseke1979 Apr 16, 2019
476c431
fix gdpr object handling
hendrikiseke1979 Apr 16, 2019
ff62c25
fetch changes from upstream
hendrikiseke1979 Apr 17, 2019
e011950
fetch changes from upstream
hendrikiseke1979 Apr 24, 2019
ba295b6
fetch changes from upstream
hendrikiseke1979 Apr 29, 2019
605fb7f
fetch changes from upstream
hendrikiseke1979 May 2, 2019
91d90fc
fetch changes from upstream
May 6, 2019
415af96
fetch changes from upstream
hendrikiseke1979 May 7, 2019
8906bcd
Merge branch 'master' of github.com:hiseke/Prebid.js
hendrikiseke1979 May 7, 2019
da8dbde
default to consentRequired: false when not explicitly given
hendrikiseke1979 May 7, 2019
07c805c
wip - use onSetTargeting callback
mathiasmethner0815 May 7, 2019
86664b5
add tests for onSetTargeting callback
mathiasmethner0815 May 7, 2019
0664e97
fix params and respective tests
RainerVolk4014 May 8, 2019
d11fca0
Merge pull request #2 from hiseke/feature/fix-params-onSetTargeting
May 8, 2019
cbcb311
fetch changes from upstream
hendrikiseke1979 May 8, 2019
b17d19b
Merge branch 'master' of github.com:hiseke/Prebid.js
hendrikiseke1979 May 8, 2019
a102857
fetch changes from upstream
udojuettner6667 May 15, 2019
534cf58
fetch changes from upstream
hendrikiseke1979 May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions modules/orbidderBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,9 @@ export const spec = {
const getRefererInfo = detectReferer(window);

bid.pageUrl = getRefererInfo().referer;
if (spec.bidParams[bid.adId]) {
bid.params = spec.bidParams[bid.adId];
if (spec.bidParams[bid.requestId] && (typeof bid.params === 'undefined')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm - is this logic correct?

I checked through the code and the spec.bidParams appears to be populated during the buildRequests function by reading the bid.params field from the bidRequest object.

The bid object that's passed into the onSetTargeting gets a copy of the params from the bidRequest object when the bid object is processed through the prebid code (which is read from the setup in the adUnit object for your bidder).

Given the above - it seems unlikely that this current if statement would return true. Can you please review/confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, you're right - if we can rely on bid.params always being defined. I will discuss with @arneschulz1984, and we will update this PR accordingly. Thanks for finding!

Copy link
Contributor

Choose a reason for hiding this comment

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

@hiseke and I just tested it with the current prebid version (2.15.0). What we see (reproducible):

  • Whenever onHandler is called via onBidWon, bid.params is defined and contains the expected data.
  • Whenever onHandler is called via onSetTargeting, bid.params is undefined.

I don't know whether this is expected behavior or a prebid bug or some timing issue. But under these conditions, it seems like we have to check bid.params for undefined. Do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you be able to provide some test params for your adapter so I can review both scenarios? I tried to use the ones in the md file, but I'm getting a 403 error on the request to your server.

Below is a copy of the details for reference:
URL: https://orbidder.otto.de/bid
Data/Payload:

{"pageUrl":"http://test.localhost:9999/integrationExamples/gpt/hello_world.html?pbjs_debug=true","bidId":"2ee2206a1622b3","auctionId":"7d686fc2-8309-4e6b-8f81-b170ea2d6085","transactionId":"b62c98b3-1fec-4e06-88de-bd5c600735c3","adUnitCode":"div-gpt-ad-1460505748561-0","sizes":[[300,250],[300,600]],"params":{"accountId":"someAccount","placementId":"somePlace"}}

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some validations happening. You can use the dev endpoint https://dev.orbidder.otto.de/bid with a payload of {"pageUrl":"https://dev.orbidder.otto.de/zzz/","bidId":"aaaa0000bbbb1111","auctionId":"cccccc-dddd","transactionId":"eeeeee-ffff","adUnitCode":"/111111111/orbidder_test","sizes":[[728,90]],"params":{"accountId":"orbidder-test","placementId":"center-banner"}} and the cookie Cookie: aditionUserId=6666666666666666666. If you receive a 204 ("No Content"), just retry a couple of times ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @ujuettner
Do some of those validations look at the actual request headers? I've tried to setup the test with the values you suggested, but I've just been seeing 204s come back after repeated attempts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jsnellbaker , if you continue to see 204, it's most likely that the cookie is not set correctly (its name is aditionUserId and its value has to be known to the DSP, e.g. 6666666666666666666 is a valid value for testing). The following curl command just worked from my laptop and from a GCE VM:

curl -v https://dev.orbidder.otto.de/bid -H 'Cookie: aditionUserId=6666666666666666666' -d '{"pageUrl":"https://dev.orbidder.otto.de/zzz/","bidId":"aaaa0000bbbb1111","auctionId":"cccccc-dddd","transactionId":"eeeeee-ffff","adUnitCode":"/111111111/orbidder_test","sizes":[[728,90]],"params":{"accountId":"orbidder-test","placementId":"center-banner"}}'

Could you please just retry by copy&pasting the aforementioned curl command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujuettner
While the curl command did return the ad response, it wasn't loading through the adapter where the onBidWon and onSetTargeting functions occur which is what I hoping to look at. That said, I tried a different browser and some other temporary edits to get the test payload/cookie you provided to successfully return an ad through the adapter.

I observed both events triggered and through some console.log statements I added in the adapter to print the values - I saw both sets of params were present in both events.

For reference, below is a screenshot of the console log statements based on what I saw from the onBidWon event (the onSetTargeting had the same output):
Screen Shot 2019-05-21 at 10 47 35 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jsnellbaker , thanks for coming back on this. I've deployed a temporary prebid.js build with some console.log's onto dev.orbidder.otto.de, and I still find bid.params being undefined on the /targeting route:
orbidderBidAdapter_onHandler_2019-05-22_162127

As there seem to be circumstances where bid.params can be undefined, I'd happy to keep the check for undefined :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright - if you're 100% certain this logic should be like this, then we can leave it as is.

bid.params = [spec.bidParams[bid.requestId]];
}

spec.ajaxCall(`${spec.orbidderHost}${route}`, JSON.stringify(bid));
},

Expand Down
14 changes: 11 additions & 3 deletions test/spec/modules/orbidderBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {expect} from 'chai';
import {spec} from 'modules/orbidderBidAdapter';
import {newBidder} from 'src/adapters/bidderFactory';
import openxAdapter from '../../../modules/openxAnalyticsAdapter';
import {detectReferer} from 'src/refererDetection';

describe('orbidderBidAdapter', () => {
const adapter = newBidder(spec);
Expand Down Expand Up @@ -153,9 +154,16 @@ describe('orbidderBidAdapter', () => {
adId: 'testId',
test: 1,
pageUrl: 'www.someurl.de',
referrer: 'www.somereferrer.de'
referrer: 'www.somereferrer.de',
requestId: '123req456'
};

spec.bidParams['123req456'] = {'accountId': '123acc456'};

let bidObjClone = deepClone(bidObj);
bidObjClone.pageUrl = detectReferer(window)().referer;
bidObjClone.params = [{'accountId': '123acc456'}];

beforeEach(() => {
ajaxStub = sinon.stub(spec, 'ajaxCall');
});
Expand All @@ -169,13 +177,13 @@ describe('orbidderBidAdapter', () => {
expect(ajaxStub.calledOnce).to.equal(true);
expect(ajaxStub.firstCall.args[0].indexOf('https://')).to.equal(0);
expect(ajaxStub.firstCall.args[0]).to.equal(`${spec.orbidderHost}/win`);
expect(ajaxStub.firstCall.args[1]).to.equal(JSON.stringify(bidObj));
expect(ajaxStub.firstCall.args[1]).to.equal(JSON.stringify(bidObjClone));

spec.onSetTargeting(bidObj);
expect(ajaxStub.calledTwice).to.equal(true);
expect(ajaxStub.secondCall.args[0].indexOf('https://')).to.equal(0);
expect(ajaxStub.secondCall.args[0]).to.equal(`${spec.orbidderHost}/targeting`);
expect(ajaxStub.secondCall.args[1]).to.equal(JSON.stringify(bidObj));
expect(ajaxStub.secondCall.args[1]).to.equal(JSON.stringify(bidObjClone));
});
});

Expand Down