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

Issue with implementing support for floor price module #6754

Closed
AskRupert-DM opened this issue May 12, 2021 · 9 comments
Closed

Issue with implementing support for floor price module #6754

AskRupert-DM opened this issue May 12, 2021 · 9 comments
Assignees

Comments

@AskRupert-DM
Copy link
Contributor

Type of issue

Bug

Description

Hi

i've been looking at supporting the floor price module in my adapter (ozoneBidAdapter) per #6465.

I've followed the examples listed here https://docs.prebid.org/dev-docs/modules/floors.html and when I check my adapter payload it doesn't appear to be working as expect.

(1) Defining floor prices at the ad unit level doesn't appear to be working - I just don't get the floor price object. Is there something I am missing/have defined wrong on my page config ? I am fairly certain I've followed the config as specified above so either that's missing or I've overlooked something very simple :)

(2) Defining floor price at the config level appears to work - however, I've defined different floors for banner and video and it looks like video uses/gets the same floor price as banner ? Not sure if I am doing something stupid or missing something in the config but didn't want to submit this in a PR until I was confident its working in the intended fashion

Steps to reproduce

Please see test pages - on each of them in chrome developer tools filter for 'auction' and you'll see the payload sent for each - look for bid[n].floor object - where you should be able to reproduce/see the issue listed above.

If you have debug enabled you should be able to filter in the console for - getFloor which also shows what the bidAdapter is receiving.

Test page

This one will not get any floor information, config is only in adUnits :
https://www.ardm.io/ozone/2.5.1/sizeconfig-testpage-20210506-floor-adunits-only.html?pbjs_debug=true

This one will get floor information, config is in setConfig:
https://www.ardm.io/ozone/2.5.1/sizeconfig-testpage-20210506-floor-setconfig-only.html?pbjs_debug=true

Expected results

a Floor object should be passed inside each individual adUnit when defining the config at ad Unit level with each media Type having its own floor.

Actual results

AdUnit level - no floor information is available.

Config level - floor information is available but the mediaTypes appear to get ignored / banner mediaType settings get applied to video when they have different floor prices defined on the config (0.8 for banner and 1.2 for video - but yet both get 0.8).

Platform details

All

@bszekely1
Copy link

bszekely1 commented May 12, 2021

This is really a documentation issue. If only using adUnit floors, we need a minimum some floors definition in the setConfig, even if it’s an empty floors object:

pbjs.setConfig({ floors: {} });

@AskRupert-DM
Copy link
Contributor Author

AskRupert-DM commented May 12, 2021

Thanks @bszekely1 - will update my testpage tomorrow and see if that solves my problem with adUnit floors not working.

How about the second issue / testpage I shared where I am defining the floors inside pbjs.setConfig ?

I have a different floor for video vs. banner but it looks like the video floor is being ignored and the banner floor is being applied to both video and banner mediaTypes ?

@AskRupert-DM
Copy link
Contributor Author

Hi @bszekely1 - I think both issues I flagged appear to be documentation issues !

(1) AdUnit Config not working - per your suggestion adding the setConfig object sorted that issue out.

(2) The issue around video vs. banner floors not working was down too a typo in the docs around mediatype - it should be mediaType (capital T) - when I reverted to the correct floor prices got assigned.

I think this can therefore be closed - probably worth having the docs updated in case this trips publishers or another adapter vendor up

Also one other discrepancy I saw in the docs is in some placed you refer to the function 'getFloor' and in other places its referred too as 'getFloors'

@AskRupert-DM
Copy link
Contributor Author

FYI test pages:

Filter console for “getFloorObjectForAuction”:

-- this is how to set the config in adunits (it also needs empty object in setConfig)
https://www.ardm.io/ozone/2.5.1/sizeconfig-testpage-20210506-floor-adunits-and-setconfig-minimum.html

-- this is how to set the config in setConfig
https://www.ardm.io/ozone/2.5.1/sizeconfig-testpage-20210506-floor-setconfig-only.html?pbjs_debug=true
(see the network request to auction - bidrequest[n].floor object)

@patmmccann
Copy link
Collaborator

@AskRupert-DM eagerly anticipating your fix and successful transition to prebid 5. Anything we can do to help?

@AskRupert-DM
Copy link
Contributor Author

@patmmccann sorry - apologies for the delay its all implemented we are just wrapping up our internal QA - am hoping we'll make a submission on Monday next week !

@patmmccann
Copy link
Collaborator

patmmccann commented Jun 4, 2021 via email

@AskRupert-DM
Copy link
Contributor Author

noted and appreciated @patmmccann

@AskRupert-DM
Copy link
Contributor Author

AskRupert-DM commented Jun 4, 2021

See you've already seen the PR @patmmccann but its been submitted here -#6946

Thought we'd get it pushed today incase any of the automated tests failed (they've passed)! !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants