pbjs.getAllWinningBids() not working correctly

[snapwich]

Type of issue

bug

Description

Calling pbjs.getAllWinningBids() will not work correctly if you have auctions that have multiple adUnits and therefore have multiple winners. I think the issue resides with the logic in auction.js here: https://github.com/prebid/Prebid.js/blob/master/src/auction.js#L200

An auction instance only allows for setting and getting a singular winning bid when it should allow for multiple winning bids.

Also while looking into this I noticed that auction.setWinningBid and auctionManger.getAllWinningBids all function around calls to pbjs.renderAd, which is somewhat confusing since it's a different code path than what is used in targeting for getting winning bids. I think either pbjs.getAllWinningBids() should use a similar code path that is used in targeting.getWinningBids to eliminate the possibility of discrepancies between the two OR pbjs.getAllWinningBids() should be renamed to eliminate confusion, maybe pbjs.getAllRenderedBids() or something?

Steps to reproduce

Create an auction with multiple adUnits and call pbjs.getAllWinningBids() after the auction has ran.

Expected results

It returns all of the bids rendered.

Actual results

It returns the last bid rendered.

Other information

This causes the issues highlighted in #2190

[snapwich]

After discussing, I think we've decided that, to help prevent discrepancies and confusion around naming, pbjs.getAllWinningBids() should be renamed to pbjs.getAllRenderedBids(). The old function can stay around with a deprecation warning.

[chefbenjamin]

Just curious where this is right now ?

[jaiminpanchal27]

@chefbenjamin We will ship this in next release

[chefbenjamin]

@jaiminpanchal27 1.6 ?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chefbenjamin]

@jaiminpanchal27 @mkendall07 @snapwich I see that #2328 is finished. Does the snippet here have to be changed ? http://prebid.org/dev-docs/toubleshooting-tips.html#see-all-bids-in-the-console

also would be good to add a column here which showed if the bid is client side or server side.

[jaiminpanchal27]

@chefbenjamin Snippet should work as it is. Also since it's a core change, it requires 2nd review so it need some more time to be merged.

Yes adding new column makes sense, will update snippet

[chefbenjamin]

@jaiminpanchal27 @mkendall07 @snapwich we just pushed 1.8 on a few sites and I can confirm the multiple render issue related to the snippet is fixed. thankyou.