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

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

merged 44 commits into from
Jun 4, 2019

Conversation

arneschulz1984
Copy link
Contributor

Type of change

  • Bugfix

Description of change

We had the Problem that the object send to the onSetTargeting function do not include the bidder parameters. By analyzing the issue we found a bug in our implementation.

  • official adapter submission

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @arneschulz1984

See below for question.

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

@jsnellbaker jsnellbaker merged commit 2a10388 into prebid:master Jun 4, 2019
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* initial orbidder version in personal github repo

* use adUnits from orbidder_example.html

* replace obsolete functions

* forgot to commit the test

* check if bidderRequest object is available

* try to fix weird safari/ie issue

* ebayK: add more params

* update orbidderBidAdapter.md

* use spec.<function> instead of this.<function> for consistency reasons

* add bidfloor parameter to params object

* fix gdpr object handling

* default to consentRequired: false when not explicitly given

* wip - use onSetTargeting callback

* add tests for onSetTargeting callback

* fix params and respective tests
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.

None yet

8 participants