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

Feature : Display counter at Adslot level #2940

Merged
merged 4 commits into from
Aug 14, 2018

Conversation

vedantseta
Copy link
Contributor

Type of change

  • Feature

Description of change

As discussed in #2295 thread, some bidders will be interested in getting the Display Counter which is added in this PR.

buildRequests of an adapter will be called with an key displayCount at adSlot level.

This PR also contains Media.net adapter integration with this feature

Other information

As discussed in #2802 with @mkendall07 , the ad unit counter variable is set private.

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.

Thank you for changing the implementation. Just a few small changes and this will be good

src/adUnits.js Outdated
return deepAccess(adUnits, `${adunit}.counter`) || 0;
}

exports.adunitCounter = {
Copy link
Member

Choose a reason for hiding this comment

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

new modules should prefer ES6 module interface (ie import / export).

src/adUnits.js Outdated
function incrementCounter(adunit) {
adUnits[adunit] = adUnits[adunit] || {};
adUnits[adunit].counter = (deepAccess(adUnits, `${adunit}.counter`) + 1) || 1;
return adUnits[adunit].counter;
Copy link
Member

Choose a reason for hiding this comment

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

why return the count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is increment and get rather than just increment. Will be useful for developer.

src/adUnits.js Outdated
return adUnits[adunit].counter;
}

function getCounter(adunit) {
Copy link
Member

Choose a reason for hiding this comment

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

please add jsdoc definitions to public functions.

@mkendall07 mkendall07 self-assigned this Aug 7, 2018
@vedantseta
Copy link
Contributor Author

@mkendall07 Have made required changes, please check the same.

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. It's also a nice starting point to start moving some of the adUnit logic into adUnits.js

@mkendall07 mkendall07 added LGTM needs docs needs 2nd review Core module updates require two approvals from the core team labels Aug 8, 2018
@@ -122,7 +122,8 @@ function slotParams(bidRequest) {
let params = {
id: bidRequest.bidId,
ext: {
dfp_id: bidRequest.adUnitCode
dfp_id: bidRequest.adUnitCode,
display_count: bidRequest.displayCount
Copy link
Member

@mkendall07 mkendall07 Aug 8, 2018

Choose a reason for hiding this comment

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

err small nit with this name. Isn't it more accurate as bidRequestsCount? We don't know if it was displayed. Or auctionParticipationCount

@mkendall07 mkendall07 removed the LGTM label Aug 8, 2018
@mkendall07
Copy link
Member

Please also submit a PR to update the documentation for the bidder. Somewhere in here would be good: http://prebid.org/dev-docs/bidder-adaptor.html#bidder-adaptor-Building-the-Request

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 but I agree it could use a better name.

@vedantseta
Copy link
Contributor Author

@mkendall07 @snapwich @jaiminpanchal27 have renamed it to bidRequestsCount. Also will be sending PR on docs soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs 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.

4 participants