-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Missena Bid Adapter: floor implementation #10534
Conversation
@pdamoc can you please add units tests for these updates |
We will add the tests for the floor changes and remove the notification refactoring (which we will submit in a later PR). |
The update includes tests for the bid floor. I have removed the notification refactoring and simplified the floor implementation. The |
modules/missenaBidAdapter.js
Outdated
if (!isFn(bidRequest.getFloor)) { | ||
return {}; | ||
} | ||
const sizesArray = getSizeArray(bidRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary, the floors module will already pick size from the request when it makes sense. What this does is force using the first size when there are multiple instead of looking for a floor that applies to all sizes.
Unless that was your intention I recommend removing this. (I am learning now that a lot of other adapters do this, so I suspect you're just following the pattern, which is unfortunately not a good one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify - if you just call getFloor()
, it will attempt to find reasonable defaults:
currency
defaults to 'USD'mediaType
defaults to the request's if it contains only onesize
defaults to the request's if it contains only one (which implies it also has only one mediaType)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisHuie is there anything else that is blocking this PR from being merged into the master? |
Type of change
Description of change
This is an implementation of the passing of the relevant price floor data to our server.
Other information
The PR also includes a small refactoring of the event notification used for
onBidWon
that is also reused foronTimeout
.