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.

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 function markWinningBidAsUsed for marking video bids #2777

Merged
merged 8 commits into from
Jul 20, 2018

Conversation

Caspervw
Copy link
Contributor

Type of change

  • Feature

Description of change

In accordance with the discussion in #2282 I’ve created a new function that’s capable of marking a winning bid as used. This addresses the issue where a video-advertisement can be played multiple times on the same page. By proactively calling pbjs.markWinningBidAsUsed the winning video-bid can be marked as consumed. We currently use it in production and call it straight after the buildVideoUrl function. It resolved most of our discrepancy issues video header bidding partners.

Documentation update: prebid/prebid.github.io#861

@jaiminpanchal27 jaiminpanchal27 added LGTM needs 2nd review Core module updates require two approvals from the core team and removed needs review labels Jul 9, 2018
src/prebid.js Outdated
* @param {string} adUnitCode - required ad unit code
* @alias module:pbjs.markWinningBidAsUsed
*/
$$PREBID_GLOBAL$$.markWinningBidAsUsed = function (adUnitCode) {
Copy link
Member

Choose a reason for hiding this comment

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

@Caspervw
Can you make this accept an object instead of a string?
I'm thinking we would also want to match against a bidId as well as a adUnitCode.
example:

{
  adUnitCode : <code>,
  bidId : <id>
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

@Caspervw
Copy link
Contributor Author

@mkendall07 I made the change, however I wonder if it became better. If you look at the snippet we now explicitly need to know which bid we’re going to mark. This improves the robustness, however (as far as I know) there is no exposed version of targeting.getWinningBids(adUnitCode);, so which bid did prebid start and what do we need to mark?

Do you think pbjs.getHighestCpmBids() is the best approach for that? What do you recon needs to go on the ...?

pbjs.requestBids({
    bidsBackHandler: function(bids) {
        // The video advertisement you want to show
        const bid = ...

        var videoUrl = pbjs.adServers.dfp.buildVideoUrl({
            adUnit: videoAdUnit,
            bid: bid
            params: {
                iu: '/19968336/prebid_cache_video_adunit'
            }
        });

        // Mark the bid, used in buildVideoUrl, as used
        pbjs.markWinningBidAsUsed({
            adUnitCode: videoAdUnit.code
            adId: bid.adId
        });

        invokeVideoPlayer(videoUrl);
    }
});

@mkendall07
Copy link
Member

mkendall07 commented Jul 16, 2018

@Caspervw
Thanks. I actually mean those as to be only one required.
ie - find the highest bid on the AdUnit:

 pbjs.markWinningBidAsUsed({
            adUnitCode: videoAdUnit.code
        });

or find exact bid:

 pbjs.markWinningBidAsUsed({
            adId: bid.adId
        });

if both are specified, then both must match.

@Caspervw
Copy link
Contributor Author

@mkendall07 Pushed the requested changes.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for updating!

@mkendall07 mkendall07 requested review from harpere and snapwich July 18, 2018 19:40
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@jaiminpanchal27 jaiminpanchal27 merged commit b0cda1c into prebid:master Jul 20, 2018
florevallatmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Sep 6, 2018
* Quick function to mark a video-bid as used

* Added tests for the mark function

* comments

* Changed the function to accept a markBidRequest object to improve security

* Changed the markWinningBidAsUsed to take and/or adUnitCode/adId
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
* Quick function to mark a video-bid as used

* Added tests for the mark function

* comments

* Changed the function to accept a markBidRequest object to improve security

* Changed the markWinningBidAsUsed to take and/or adUnitCode/adId
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* Quick function to mark a video-bid as used

* Added tests for the mark function

* comments

* Changed the function to accept a markBidRequest object to improve security

* Changed the markWinningBidAsUsed to take and/or adUnitCode/adId
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
* Quick function to mark a video-bid as used

* Added tests for the mark function

* comments

* Changed the function to accept a markBidRequest object to improve security

* Changed the markWinningBidAsUsed to take and/or adUnitCode/adId
AlessandroDG pushed a commit to simplaex/Prebid.js that referenced this pull request Mar 26, 2019
* Quick function to mark a video-bid as used

* Added tests for the mark function

* comments

* Changed the function to accept a markBidRequest object to improve security

* Changed the markWinningBidAsUsed to take and/or adUnitCode/adId
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Impact LGTM needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants