-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support native click tracking #1691
Conversation
* Fire based on postMessage in adserver creative * Fix tests, add comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty reasonable to me... few minor comments.
src/native.js
Outdated
@@ -78,6 +78,11 @@ export function nativeBidIsValid(bid) { | |||
return false; | |||
} | |||
|
|||
// all native bid responses must defined a landing page url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: define
src/native.js
Outdated
|
||
(impressionTrackers || []).forEach(tracker => { | ||
triggerPixel(tracker); | ||
return trackers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'd build tests around this return value... what you really want is to make sure that triggerPixel
gets called with the expected args. Since this is a separate code path, there's no guarantee that it stays in sync with triggerPixel
calls. Code may get added/removed over time.
I know it sucks for testing, but... it seems more robust to set a stub for triggerPixel
and make sure that it gets called with the expected args.
src/native.js
Outdated
* @param {Object} bid | ||
* @return {Object} targeting | ||
*/ | ||
export function setNativeTargeting(bid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably rename to getNativeTargeting
, parseNativeTargeting
, or something like that.
set
pretty universally means "this function changes some state." In this case, you're building a transformed copy of some existing state without changing it.
}); | ||
|
||
return keyValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to return this anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I'm sorry... had a brain fart and thought this was the return trackers
line. Nevermind.
@@ -201,6 +201,7 @@ function newBid(serverBid, rtbBid) { | |||
image: nativeAd.main_img && nativeAd.main_img.url, | |||
icon: nativeAd.icon && nativeAd.icon.url, | |||
clickUrl: nativeAd.link.url, | |||
clickTrackers: nativeAd.link.click_trackers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be validating the different bid types and objects... food for later thought.
* tag '0.31.0' of https://github.com/prebid/Prebid.js: (54 commits) Fix for prebid#1628 (allowing standard bidCpmAdjustment) (prebid#1645) Prebid 0.31.0 Release Support native click tracking (prebid#1691) Initial commit for video support for pbs (prebid#1706) Fixes: Immediate adapter response may end auction (prebid#1690) Rubicon feature/s2s test module (prebid#1678) Renaming of "huddledmasses" adapter into colossusssp (prebid#1701) Don't set non-object configurations (prebid#1704) Update JSDoc for `pbjs.enableAnalytics` (prebid#1565) Add ad units event (prebid#1702) AppnexusAst adapter: logging error message from endpoint (prebid#1697) AppnexusAst bidadapter markdown file (prebid#1696) Change Default Content-Type for POST Requests to 'application/json' (prebid#1681) Code improvement for trustx adapter (prebid#1673) PulsePoint Lite adapter - Enabling Sync pixel (prebid#1686) Update spotx video adapter to set the spotx_ad_key used in DFP (prebid#1614) Fix broken AOL mobile endpoint secure bid requests (prebid#1684) Fix adapter tests that hardcoded pbjs. (prebid#1666) no longer attaching gpt slots to adUnits, which breaks utils.cloneJson(adUnit) (prebid#1676) remove bidmanager from rubicon tests (prebid#1671) ...
@matthewlane based on your comment, looks like we need to update our Ad Ops docs for Native to show how to use these click trackers. Are there any changes needed to the developer setup docs for Native? |
* 'master' of https://github.com/prebid/Prebid.js: (414 commits) Make response headers available to the specs (prebid#1748) add option to run tests in a specific file (prebid#1727) Update JCM Adapter to 1.0 (prebid#1715) Finished an unfinished comment. (prebid#1749) Platform.io Bidder Adapter update. Prebid v1.0. (prebid#1705) Fix window.top.host cross origin issue when in nested iframes. (prebid#1730) fix log message not displaying when referencing missing bidder (prebid#1737) Allow more than one placement from one page (prebid#1692) Justpremium Adapter bugfix (prebid#1716) Updating license (prebid#1717) realvuBidAdapter (prebid#1571) Update JSDoc to call the module `pbjs` (prebid#1572) Update Beachfront adapter for v1.0 (prebid#1675) Update AdButler adapter for Prebid v1.0 (prebid#1664) Increment pre version Fix for prebid#1628 (allowing standard bidCpmAdjustment) (prebid#1645) Prebid 0.31.0 Release Support native click tracking (prebid#1691) Initial commit for video support for pbs (prebid#1706) Fixes: Immediate adapter response may end auction (prebid#1690) ...
….31.0 to aolgithub-master * commit 'e7341c948014a789084849495171d08d4b353d07': (21 commits) Added changelog entry. Fix for prebid#1628 (allowing standard bidCpmAdjustment) (prebid#1645) Prebid 0.31.0 Release Support native click tracking (prebid#1691) Initial commit for video support for pbs (prebid#1706) Fixes: Immediate adapter response may end auction (prebid#1690) Rubicon feature/s2s test module (prebid#1678) Renaming of "huddledmasses" adapter into colossusssp (prebid#1701) Don't set non-object configurations (prebid#1704) Update JSDoc for `pbjs.enableAnalytics` (prebid#1565) Add ad units event (prebid#1702) AppnexusAst adapter: logging error message from endpoint (prebid#1697) AppnexusAst bidadapter markdown file (prebid#1696) Change Default Content-Type for POST Requests to 'application/json' (prebid#1681) Code improvement for trustx adapter (prebid#1673) PulsePoint Lite adapter - Enabling Sync pixel (prebid#1686) Update spotx video adapter to set the spotx_ad_key used in DFP (prebid#1614) Fix broken AOL mobile endpoint secure bid requests (prebid#1684) Fix adapter tests that hardcoded pbjs. (prebid#1666) no longer attaching gpt slots to adUnits, which breaks utils.cloneJson(adUnit) (prebid#1676) ...
* Implement native click tracking * Fire based on postMessage in adserver creative * Fix tests, add comments * Require landing page urls on native bid responses * Address code review comments
@rmloveland Yeah, updating the Ad Opcs docs to show how to use click trackers is a good idea. On the developer setup docs, the native object section may be updated to indicate that |
* Implement native click tracking * Fire based on postMessage in adserver creative * Fix tests, add comments * Require landing page urls on native bid responses * Address code review comments
Type of change
Description of change
As per OpenRTB Native 1.2 , click trackers should be supported as an array of URLs. This PR adds support to Prebid for click trackers associated with native bid responses.
If a native bid response contains click trackers, bidder adapters should attach the click tracker array to the
native.clickTrackers
property of the bid response object (see appnexusAstBidAdapter for an example of this).As native creatives that are served from adservers are likely to be in a cross-origin frame, it is necessary to use
postMessage
from the native creative template to communicate to Prebid that a link was clicked. Prebid is (even before this PR) configured to fire native impression trackers when it receives a message with the following structure:This PR introduces the ability to fire click trackers for the associated
adId
when a'click'
action
is added to the message:Ad ops teams may use these structures of
postMessage
in their native creative templates in any way that fits their use case. One example usage is given below:Other information
Click through/landing page URLs are a required field in the OpenRTB 1.2 spec. This PR also requires native bid responses to have a landing page url, defined by
native.clickUrl
on the native bid response object.