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.

Sign up for GitHub

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

Jump to bottom

Prebid core: cached bids should contain the ID of the auction they actually won #10541

Merged
merged 2 commits into from
Oct 10, 2023
+39 −5
Merged

Prebid core: cached bids should contain the ID of the auction they actually won #10541

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
623d15c
Cached bids should contain the ID of the auction they actually won
msmeza Sep 27, 2023
6e9bd47
Property latestTargetedAuctionId should be set on all bids in the pool
msmeza Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/targeting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (latestAuctionForAdUnit[bid.adUnitCode] !== bid.auctionId) {
bid.latestAuctionForAdUnit = latestAuctionForAdUnit[bid.adUnitCode];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On naming , I agree with a shorter name yes

}
return bid;
});
}

bidsReceived = bidsReceived
Expand Down
22 changes: 22 additions & 0 deletions test/spec/unit/core/targeting_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,13 +955,15 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(1);
expect(bids[0].adId).to.equal('adid-1');
expect(bids[0].latestAuctionForAdUnit).to.equal(2);

useBidCache = false;

bids = targetingInstance.getWinningBids(adUnitCodes);

expect(bids.length).to.equal(1);
expect(bids[0].adId).to.equal('adid-2');
expect(bids[0].latestAuctionForAdUnit).to.equal(undefined);
});

it('should use bidCacheFilterFunction', function() {
Expand Down Expand Up @@ -989,9 +991,13 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(4);
expect(bids[0].adId).to.equal('adid-1');
expect(bids[0].latestAuctionForAdUnit).to.equal(2);
expect(bids[1].adId).to.equal('adid-4');
expect(bids[1].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[2].adId).to.equal('adid-5');
expect(bids[2].latestAuctionForAdUnit).to.equal(2);
expect(bids[3].adId).to.equal('adid-8');
expect(bids[3].latestAuctionForAdUnit).to.equal(undefined);

// Bid Caching Off, No Filter Function
useBidCache = false;
Expand All @@ -1000,9 +1006,13 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(4);
expect(bids[0].adId).to.equal('adid-2');
expect(bids[0].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[1].adId).to.equal('adid-4');
expect(bids[1].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[2].adId).to.equal('adid-6');
expect(bids[2].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[3].adId).to.equal('adid-8');
expect(bids[3].latestAuctionForAdUnit).to.equal(undefined);

// Bid Caching On AGAIN, No Filter Function (should be same as first time)
useBidCache = true;
Expand All @@ -1011,9 +1021,13 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(4);
expect(bids[0].adId).to.equal('adid-1');
expect(bids[0].latestAuctionForAdUnit).to.equal(2);
expect(bids[1].adId).to.equal('adid-4');
expect(bids[1].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[2].adId).to.equal('adid-5');
expect(bids[2].latestAuctionForAdUnit).to.equal(2);
expect(bids[3].adId).to.equal('adid-8');
expect(bids[3].latestAuctionForAdUnit).to.equal(undefined);

// Bid Caching On, with Filter Function to Exclude video
useBidCache = true;
Expand All @@ -1026,9 +1040,13 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(4);
expect(bids[0].adId).to.equal('adid-1');
expect(bids[0].latestAuctionForAdUnit).to.equal(2);
expect(bids[1].adId).to.equal('adid-4');
expect(bids[1].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[2].adId).to.equal('adid-6');
expect(bids[2].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[3].adId).to.equal('adid-8');
expect(bids[3].latestAuctionForAdUnit).to.equal(undefined);
// filter function should have been called for each cached bid (4 times)
expect(bcffCalled).to.equal(4);

Expand All @@ -1044,9 +1062,13 @@ describe('targeting tests', function () {

expect(bids.length).to.equal(4);
expect(bids[0].adId).to.equal('adid-2');
expect(bids[0].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[1].adId).to.equal('adid-4');
expect(bids[1].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[2].adId).to.equal('adid-6');
expect(bids[2].latestAuctionForAdUnit).to.equal(undefined);
expect(bids[3].adId).to.equal('adid-8');
expect(bids[3].latestAuctionForAdUnit).to.equal(undefined);
// filter function should not have been called
expect(bcffCalled).to.equal(0);
});
Expand Down
16 changes: 11 additions & 5 deletions test/spec/unit/pbjs_api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {stubAuctionIndex} from '../../helpers/indexStub.js';
import {createBid} from '../../../src/bidfactory.js';
import {enrichFPD} from '../../../src/fpd/enrichment.js';
import {mockFpdEnrichments} from '../../helpers/fpd.js';
import {generateUUID} from '../../../src/utils.js';
var assert = require('chai').assert;
var expect = require('chai').expect;

Expand All @@ -42,11 +43,12 @@ var adUnits = getAdUnits();
var adUnitCodes = getAdUnits().map(unit => unit.code);
var bidsBackHandler = function() {};
const timeout = 2000;
const auctionId = generateUUID();
let auction;

function resetAuction() {
if (auction == null) {
auction = auctionManager.createAuction({adUnits, adUnitCodes, callback: bidsBackHandler, cbTimeout: timeout});
auction = auctionManager.createAuction({adUnits, adUnitCodes, callback: bidsBackHandler, cbTimeout: timeout, labels: undefined, auctionId: auctionId});
}
$$PREBID_GLOBAL$$.setConfig({ enableSendAllBids: false });
auction.getBidRequests = getBidRequests;
Expand Down Expand Up @@ -3302,16 +3304,20 @@ describe('Unit: Prebid Module', function () {
const highestBid = $$PREBID_GLOBAL$$.getHighestUnusedBidResponseForAdUnitCode('/19968336/header-bid-tag-0');
expect(highestBid).to.deep.equal(_bidsReceived[2])
})
})
});

describe('getHighestCpm', () => {
describe('getHighestCpmBids', () => {
after(() => {
resetAuction();
});
it('returns an array containing the highest bid object for the given adUnitCode', function () {
const highestCpmBids = $$PREBID_GLOBAL$$.getHighestCpmBids('/19968336/header-bid-tag-0');
const adUnitcode = '/19968336/header-bid-tag-0';
targeting.setLatestAuctionForAdUnit(adUnitcode, auctionId)
const highestCpmBids = $$PREBID_GLOBAL$$.getHighestCpmBids(adUnitcode);
expect(highestCpmBids.length).to.equal(1);
expect(highestCpmBids[0]).to.deep.equal(auctionManager.getBidsReceived()[1]);
const expectedBid = auctionManager.getBidsReceived()[1];
expectedBid.latestAuctionForAdUnit = auctionId;
expect(highestCpmBids[0]).to.deep.equal(expectedBid);
});

it('returns an empty array when the given adUnit is not found', function () {
Expand Down