-
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
New Module: Bid response filter #12147
Conversation
let's make sure to help populate attr and categories with https://github.com/prebid/Prebid.js/blob/9e2c63ec92b3f88882d04ae2b9cbbdc80f504a32/libraries/ortbConverter/processors/default.js#L98C1-L99C1 |
lets set boolean for each of the field and add the unknown toggle in the config eg ` block-unknown-adv-cat -- |
Tread carefully! This PR adds 6 linter errors (possibly disabled through directives):
|
modules/bidResponseFilter/index.js
Outdated
import { getHook } from '../../src/hook.js'; | ||
|
||
export const MODULE_NAME = 'bidResponseFilter'; | ||
export const PUBLISHER_FILTER_REJECTION_REASON = 'PUBLISHER_FILTER_REJECTION_REASON'; |
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.
these should be human readable
modules/bidResponseFilter/index.js
Outdated
}; | ||
|
||
export function addBidResponseHook(next, adUnitCode, bid, reject) { | ||
const { bcat = [], badv = [], battr = [] } = config.getAnyConfig('ortb2') || {}; |
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.
ortb2 can be different for each bid response. You can get a specific response's with auctionManager.index.getBidderRequest(bid)?.ortb2
; however, responses can also not be associated with any request, in which case the best avialable ortb2
is the one associated with the auction (like you used in #11946)
It may be worth to add a getOrtb2
method to auctionManager.idex
- similar to getMediaTypes, which also has a fallback for when there is no request.
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 just noticed that battr
is not here - it's repeated for each media type in imp. It's not clear where the publisher should set it - candidates are ortb2Imp[mediaType].battr
and mediaTypes[mediaType].battr
Assuming the first one should have priority, we could:
- update ad unit validation for each media type to find
battr
fromortb2Imp[type].battr
, falling back tomediaTypes[type].battr
, and copy it to both places - add some unit test for ortbConverter banner, video and native to make sure the "right" battr wins
- this should then look for
battr
fromindex.getBidRequest(bid).ortb2Imp[bid.mediaType].battr
, falling back toindex.getAdUnit(bid).ortb2Imp[bid.mediaType].battr
when there is no request for the bid
(pending confirmation on the relative priority of ortb2Imp
vs mediaTypes
)
modules/bidResponseFilter/index.js
Outdated
const { bcat = [], badv = [], battr = [] } = config.getAnyConfig('ortb2') || {}; | ||
const moduleConfig = config.getConfig(MODULE_NAME); | ||
|
||
const catConfig = moduleConfig?.cat || { enforce: true, block_unknown_cat: true }; |
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.
if I
setConfig({bidResponseFilter: {cat: {block_unknown_cat: false}}})
my expectation would be that bcat
would still be enforced (e.g., enforce
defaults to true
), but I think this would disable it.
modules/bidResponseFilter/index.js
Outdated
const moduleConfig = config.getConfig(MODULE_NAME); | ||
|
||
const catConfig = moduleConfig?.cat || { enforce: true, block_unknown_cat: true }; | ||
const advConfig = moduleConfig?.adv || { enforce: true, block_unknown_adomain: true }; |
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.
It's probably out of scope, but I don't like these parameter names. attr: {block_uknown_attr}
is redundant; and we do not use snake_case anywhere else. It's also inconsistent within itself (why cat/block_unknown_cat
, but adv/block_unknown_adomain
?)
If we weren't following PBS this would be something like adv: {enforce, blockUnknown}
. Do we still prefer PBS's syntax @patmmccann ?
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.
those are good reasons to abandon the hope of matching the syntax. oh well
2962389
to
839fe58
Compare
839fe58
to
5697f4e
Compare
bidResponse.meta.secondaryCatIds = bid.cat; | ||
bidResponse.meta.primaryCatId = bid.cat[0]; |
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.
is a primary category also a secondary category?
src/auctionIndex.js
Outdated
@@ -65,6 +65,9 @@ export function AuctionIndex(getAuctions) { | |||
.flatMap(ber => ber.bids) | |||
.find(br => br && br.bidId === requestId); | |||
} | |||
}, | |||
getOrtb2(bid) { | |||
return this.getBidderRequest(bid)?.ortb2 || this.getAuction(bid)?.ortb2 |
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.
auctions do not have an ortb2
property. This should be getFPD().global
.
src/prebid.js
Outdated
if (adUnit.ortb2Imp?.[mediaType] && battr) { | ||
adUnit.ortb2Imp[mediaType].battr = battr; | ||
} | ||
if (adUnit.mediaTypes?.[mediaType] && battr) { | ||
adUnit.mediaTypes[mediaType].battr = battr; | ||
} |
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.
IMO this should always copy it over, since the goal is to make it easy to find for adapters. For example if I have {mediaTypes: {video: {battr}}, ortbImp: {}}
, it would remain relegated to mediaTypes
; turn it around and it would remain only in ortb2Imp
, and adapters would still need to check both places. (They're also both decent test cases). We should also probably log a warning if they're both specified but they differ.
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
src/prebid.js
Outdated
const mediaTypes = adUnit.mediaTypes || {}; | ||
|
||
if (ortb2Imp[mediaType]?.battr && mediaTypes[mediaType]?.battr && (ortb2Imp[mediaType]?.battr !== mediaTypes[mediaType]?.battr)) { | ||
logWarn('battr field differ between ortb2Imp and mediaTypes'); |
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.
this should give some clue as to where the problem is - e.g. adUnit '${adUnit.code}' has ...
or ...and mediaTypes:', adUnit
), or both
src/prebid.js
Outdated
adUnit.ortb2Imp = {...ortb2Imp, [mediaType]: {...(ortb2Imp[mediaType] || {}), battr}}; | ||
adUnit.mediaTypes = {...mediaTypes, [mediaType]: {...(mediaTypes[mediaType] || {}), battr}}; |
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 think this would add battr: undefined
in most cases. You can also simplify a bit by using deepSetValue
, e.g.
adUnit.ortb2Imp = {...ortb2Imp, [mediaType]: {...(ortb2Imp[mediaType] || {}), battr}}; | |
adUnit.mediaTypes = {...mediaTypes, [mediaType]: {...(mediaTypes[mediaType] || {}), battr}}; | |
if (battr != null) { | |
['mediaTypes', 'ortb2Imp'].forEach(target => deepSetValue(adUnit, `${target}.${mediaType}.battr`, battr)) | |
} |
could you please also add some tests?
@@ -121,7 +121,9 @@ describe('Geniee adapter tests', () => { | |||
currency: 'JPY', | |||
mediaType: 'banner', | |||
meta: { | |||
advertiserDomains: ['geniee.co.jp'] | |||
advertiserDomains: ['geniee.co.jp'], |
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.
was this failing tests? it shouldn't need to be changed - you may need to tear down the bidResponseFilter tests. (if this was failing, more will likely be affected in the future, so it's better to try to get rid of test side effects).
fb9458c
to
e72e53f
Compare
e72e53f
to
fb9458c
Compare
* initial commit * update * review changes * + auction index * unit tests to ortb converter * review changes * battr setting * improvements * fix log message --------- Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Demetrio Girardi <[email protected]>
* initial commit * update * review changes * + auction index * unit tests to ortb converter * review changes * battr setting * improvements * fix log message --------- Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Marcin Komorski <[email protected]> Co-authored-by: Demetrio Girardi <[email protected]>
Type of change
Bugfix
Feature
New bidder adapter
Updated bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Other information
#6345