-
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
Prebid Core & Browsi RTD provider: Support Vendor Billing with Billable events #8119
Changes from all commits
6121b4e
1a80b14
3b85815
0cb7b69
0908134
cf4c5a9
7beeee3
60aaeaa
0e06e6f
e9312c7
c0901fe
398f922
b3d0bea
a4f2de6
65ed991
15337d2
76b3208
6a8c111
b9b05f0
89013d7
342484c
faa02bf
88430b1
b73b6d7
00c027c
51555b6
c959997
a277d65
ebf1fa8
6ec752e
6af3494
f0d9bb7
608eabf
6bdd935
ca43a02
8346da4
1f5a82e
1c0d024
6c7cac7
1543516
efe5ddf
b404b69
107f00c
cea0004
2dcd7af
86d4cf7
191056f
97c94b2
ede3d7f
e435be5
6a2293e
ebaba7c
f918fb8
504eec0
654515b
383ae2f
d74e1fa
9ef351c
5b3817c
60d8c5c
6f3684e
28d6d5b
a1f37fd
6dd4ea3
66e782e
2f16e37
43c19e8
ad3855b
1c577d8
a52ac36
68a705c
6663f54
70c9d81
57ebdbf
f2b843e
e45b607
57e616b
1f0ce9b
3505c14
8d4e683
a60bd1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,13 +15,15 @@ | |
* @property {?string} keyName | ||
*/ | ||
|
||
import {deepClone, deepSetValue, isFn, isGptPubadsDefined, isNumber, logError} from '../src/utils.js'; | ||
import {deepClone, deepSetValue, isFn, isGptPubadsDefined, isNumber, logError, logInfo, generateUUID} from '../src/utils.js'; | ||
import {submodule} from '../src/hook.js'; | ||
import {ajaxBuilder} from '../src/ajax.js'; | ||
import {loadExternalScript} from '../src/adloader.js'; | ||
import {getStorageManager} from '../src/storageManager.js'; | ||
import {find, includes} from '../src/polyfill.js'; | ||
import {getGlobal} from '../src/prebidGlobal.js'; | ||
import events from '../src/events.js'; | ||
import CONSTANTS from '../src/constants.json'; | ||
|
||
const storage = getStorageManager(); | ||
|
||
|
@@ -106,6 +108,7 @@ export function setData(data) { | |
} | ||
|
||
function getRTD(auc) { | ||
logInfo(`Browsi RTD provider is fetching data for ${auc}`); | ||
try { | ||
const _bp = (_browsiData && _browsiData.p) || {}; | ||
return auc.reduce((rp, uc) => { | ||
|
@@ -331,13 +334,23 @@ export const browsiSubmodule = { | |
getBidRequestData: setBidRequestsData | ||
}; | ||
|
||
function getTargetingData(uc) { | ||
function getTargetingData(uc, c, us, a) { | ||
const targetingData = getRTD(uc); | ||
const auctionId = a.auctionId | ||
uc.forEach(auc => { | ||
if (isNumber(_ic[auc])) { | ||
_ic[auc] = _ic[auc] + 1; | ||
} | ||
const transactionId = a.adUnits.find(adUnit => adUnit.code === auc).transactionId; | ||
events.emit(CONSTANTS.EVENTS.BILLABLE_EVENT, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't this emit for every adUnit code in the auction? is requesting an auction with N ad units supposed to generate N billable events regardless of the bids it gets (or does not get)? It'd be helpful to have a test case that shows what billable events you expect an auction to generate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, please add actual test cases surrounding these events. As for when the events fire, perhaps there is or needs to be some documentation on how Browsi determines it? Is it just whenever the viewability is passed into the adServer for a given adUnit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We emit event per adUnitCode, regardless of the bids There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added unit testing |
||
vendor: 'browsi', | ||
type: 'adRequest', | ||
billingId: generateUUID(), | ||
transactionId: transactionId, | ||
auctionId: auctionId | ||
}) | ||
}); | ||
logInfo('Browsi RTD provider returned targeting data', targetingData, 'for', uc) | ||
return targetingData; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -451,4 +451,4 @@ | |
} | ||
} | ||
} | ||
} | ||
} |
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 believe that in general there may be more than one adUnit, and more than one transactionId, for a given code. Does it makes sense here to pick the first?
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.
Shouldn't ad unit code be unique?
This is how it's documented -
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.
The documentation could be improved; it also says:
https://docs.prebid.org/dev-docs/adunit-reference.html#twin-adunit-codes
If your aim is to emit an event for each adUnit, I'd just loop through them directly instead of the codes.
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 believe the use case for them not being unique still winds up targeting the same slot on the page.
So, perhaps the browsi use case does in fact only want to emit for each adUnitCode?
I'll let the browsi team determine that @omerBrowsi
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.
Correct. adUnitCode is the right use for us