Prebid core: cached bids should contain the ID of the auction they actually won #10541
Conversation
src/targeting.js
Outdated
| @@ -464,6 +464,14 @@ export function newTargeting(auctionManager) { | |||
| if (typeof filterFunction === 'function') { | |||
| bidsReceived = bidsReceived.filter(bid => latestAuctionForAdUnit[bid.adUnitCode] === bid.auctionId || !!filterFunction(bid)) | |||
| } | |||
|
|
|||
| bidsReceived = bidsReceived | |||
| .map(bid => { | |||
There was a problem hiding this comment.
this looks more suited to forEach. Also I think it should be done after all filtering is done, around line 480, or do you see a reason why it's better to tag them here?
There was a problem hiding this comment.
You're right, forEach is mor suited for this.
Yes, I agree that the property should be set after all filtering is done, provided it should be set on all the bids in the pool (as you mentioned in your comment below). The reason I added my change where I did was to avoid increasing the size of bid objects in the cases where auctionId would have the same value as the new property. But I think it's a good idea to set the property on all bids in the pool.
src/targeting.js
Outdated
| bidsReceived = bidsReceived | ||
| .map(bid => { | ||
| if (latestAuctionForAdUnit[bid.adUnitCode] !== bid.auctionId) { | ||
| bid.latestAuctionForAdUnit = latestAuctionForAdUnit[bid.adUnitCode]; |
There was a problem hiding this comment.
- IMO this should be set on all bids in the pool, not just those from a previous auction.
- I don't like the name, "forAdUnit" is redundant since the object is a bid and already tied to a particular ad unit. Maybe
latestTargetedAuctionId? Asking also @patmmccann for opinions. I'd prefer to make targeting explicit since this only runs when the publisher uses prebid's targeting logic, which in theory they could do without.
There was a problem hiding this comment.
On naming , I agree with a shorter name yes
Type of change
Description of change
Cached bids should contain the ID of the auctions that they actually won so that this information is available through
BID_WONevents.Other information
More information about the motivation for this PR can be found here: #10540