PUC update to include Native Phase 2 spec: #106

mmoschovas · 2020-06-22T19:54:59Z

Added renderNativeAd function to nativeTrackerManager.js
renderNativeAd checks for existence of adId and performs the following:
- Checks for rendererUrl and preloads if exists
- Uses prexisting loadAssets function with new callback
- Fires Impression tracker and loads click tracker
LoadAssets update:
- Checks for existence of assetsToReplace and adds each asset to placeholder
- If no placeholders exist, checks for requestAllAssets flag to fire new requestAllAssets function
requestAllAssets will postmessage Prebid using the action allAssetRequest instead of assetRequest
Replace assets will now use added logic:
- If rendererUrl existence. If exists in adServer (preloaded) then check for renderAd availability on the window. If none exists, set load event on the script tag using the id 'pb-native-renderer'. If url sent as asset from Prebid, append script to window and attach load event. When script loaded and renderAd available, fire function to retrieve HTML. This inserted into innerHtml
- Else if adTemplate exists, replace assets and insert into innerHtml
- Else run existing replace method
replace function updated to include check for new native to replace keys which are in the format of ##KEY##

nativeAssetManager_spec and nativeTrackerManager_spec both updated to include additional tests

nativeTracker.js updated to include renderNativeAd function definition on window object

1) Added renderNativeAd function to nativeTrackerManager.js 2) renderNativeAd checks for existence of adId and performs the following: - Checks for rendererUrl and preloads if exists - Uses prexisting loadAssets function with new callback - Fires Impression tracker and loads click tracker 3) LoadAssets update: - Checks for existence of assetsToReplace and adds each asset to placeholder - If no placeholders exist, checks for requestAllAssets flag to fire new requestAllAssets function 4) requestAllAssets will postmessage Prebid using the action allAssetRequest instead of assetRequest 5) Replace assets will now use added logic: - If rendererUrl existence. If exists in adServer (preloaded) then check for renderAd availability on the window. If none exists, set load event on the script tag using the id 'pb-native-renderer'. If url sent as asset from Prebid, append script to window and attach load event. When script loaded and renderAd available, fire function to retrieve HTML. This inserted into innerHtml - Else if adTemplate exists, replace assets and insert into innerHtml - Else run existing replace method 6) replace function updated to include check for new native to replace keys which are in the format of ##KEY## nativeAssetManager_spec and nativeTrackerManager_spec both updated to include additional tests nativeTracker.js updated to include renderNativeAd function definition on window object

wqi1972 · 2020-06-24T01:16:57Z

src/nativeTrackerManager.js

 return {
- startTrackers
+ startTrackers,
+ renderNativeAd


Suggest move renderNativeAd to a new class instead of add into newNativeTrackerManager. In long run, newNativeTrackerManager.startTrackers will get deprecated. If we separate code renderNativeAd from newNativeTrackerManager now, it will be much easier when that happens.

renderAd code separated into new nativeRenderManager

wqi1972 · 2020-06-24T01:20:44Z

src/nativeTrackerManager.js

+
+ function fireNativeCallback(){
+ let parsedUrl = parseUrl(window.pbNativeData.pubUrl);
+ publisherDomain = parsedUrl.protocol + '://' + parsedUrl.host;


We suppose set publisherDomain value inside renderNativeAd once, instead of set it before each tracker call.

Corrected the above to one definition in renderNativeAd as opposed to the tracker definitions

wqi1972 · 2020-06-24T01:29:12Z

I don't see code inside renderNativeAd take care of request for Mobile App as startTrackers does. I don't have experience on native ad for Mobile App, need someone has that experience double check it.

wqi1972 · 2020-06-24T01:37:28Z

We need logic to take care <img class="pb-icon"> and <img class="pb-image"> as https://github.com/prebid/prebid-universal-creative/pull/60/files does.
This way will avoid 400 error message for icon and hero image.

…nderManager. Removed repetitive publisherDomain definitions

wqi1972 · 2020-06-25T09:39:11Z

src/nativeAssetManager.js

+ }else if(flag && win.pbNativeData.hasOwnProperty('requestAllAssets') && win.pbNativeData.requestAllAssets) {
+ callback = cb;
+ requestAllAssets(adId);


I think we suppose check requestAllAssets first. If it is true, then requestAllAssets. Only when requestAllAssets is not true, then requestAssets for placeholders.

Can you describe the use case for this? I would think that if placeholders are detected/defined, then requesting all assets should not be a requirement as the needed assets would be known.

2 reasons:

for Use Case 1 - “Render Replacements”, and Use Case 2 - “Ad Template as String”, these 2 use cases both have placementholder token inside html. pbNativeTagData.requestAllAssets = true; will be useless if the code check placementholders exist first;

for Use Case 1 - “Render Replacements”, for html <img class="pb-icon" alt="icon" height="10" width="10">, there suppose have some code which get asset of ##hb_native_icon##, and set src of the image be the value of ##hb_native_icon##. But inside html, there is no placeholder for ##hb_native_icon##, so the asset won't get pulled in existing logic.

To make it easier, I will create a PR against your branch, so you will understand what I mean.

unfortunately my account can't push commit into git@github.com:mmoschovas/prebid-universal-creative.git repo, even inside the my own branch.
Here is the code change I made, I change function fireNativeCallback() to:

function fireNativeCallback(){ const adElements = findAdElements(AD_ANCHOR_CLASS_NAME); for (let i = 0; i < adElements.length; i++) { adElements[i].addEventListener('click', function(event) { loadClickTrackers(event, window.pbNativeData.adId); }, true); } const imgElements = win.document.getElementsByTagName("img"); for (let i = 0; i < imgElements.length; i++) { if (imgElements[i].dataset && imgElements[i].dataset.src) { imgElements[i].src = imgElements[i].dataset.src; } } }

with this change, then it will support tag with following format

<img data-src="##hb_native_image##" alt="" /> <img data-src="##hb_native_icon##" alt="logo" />

This will avoid 400 error if we directly set img.src inside html template.

Notice, this is a different solution than using <img class="pb-icon">, I think it is better and easier.
And with this solution, it doesn't need pbNativeTagData.requestAllAssets = true; for Use Case 1 - “Render Replacements” anymore, since all asset placementholder exists in GAM template html.

One more notice, you need to adjust renderingMocks inside nativeRenderManager_spec.js to

const renderingMocks = { getWindowObject: function() { return { document: { getElementsByTagName: function() { return []; } }, parent: { postMessage: sinon.spy() } } } };

robertrmartinez · 2020-07-14T17:08:16Z

Hey all, I am setting up some GAM line items today to help make testing local PUC changes easier.

I'll share example test pages once I am done today / tomorrow.

I would like to finalize the native stuff and merge both these for next weeks release!

Thanks again.

wqi1972 · 2020-07-14T19:54:16Z

I've tested on my local with following test cases:

New PBJS + PUC code with existing configure and templates, (using native key/value). Confirmed it is backward compatible;
New PBJS + PUC code + sendTargetingKeys: false + template with macro ##native_##. Confirmed Use Case 1 - “Render Replacements” is working. The only issue the tag temporarily raises 400 error. And with my suggestion provided above, that 400 error will get solved.

robertrmartinez

couple minor things.

Want to get the Xander team to do a more in depth review of the rendererUrl use case.

robertrmartinez · 2020-07-22T22:05:52Z

dist/load-cookie.html

@@ -0,0 +1,16 @@
+<!DOCTYPE html>


I think this should not be checked in correct?

Same with the .DS_Store binary.

robertrmartinez · 2020-07-22T22:09:02Z

src/nativeAssetManager.js

- callback && callback();
- win.removeEventListener('message', replaceAssets);
+ if((data.hasOwnProperty('rendererUrl') && data.rendererUrl) || (flag && win.pbNativeData.hasOwnProperty('rendererUrl'))) {
+ if(win.renderAd){


some lint issues (we do not have automatic linter on puc code yet)

Maybe run the entire code on your local with your eslint plugin and do the fixes?

robertrmartinez · 2020-07-22T22:10:26Z

src/nativeAssetManager.js

- callback && callback();
- win.removeEventListener('message', replaceAssets);
+ if((data.hasOwnProperty('rendererUrl') && data.rendererUrl) || (flag && win.pbNativeData.hasOwnProperty('rendererUrl'))) {
+ if(win.renderAd){


@jaiminpanchal27 When the rendererUrl is loaded, is the expected output that a renderAd function is placed on the win.

If you could review this use case a little more in depth I would appreciate it since it was spec'd by you.

robertrmartinez · 2020-07-22T22:11:23Z

src/nativeRenderManager.js

+ nativeAssetManager.loadAssets(nativeTag.adId,fireNativeCallback);
+ fireNativeCallback();
+ fireNativeImpTracker(nativeTag.adId);
+ }else{


some more lint issues

Should have a space after closing bracket befor else and after

} else {

maybe just run enable eslint in your editor and see what else there is?

robertrmartinez · 2020-07-22T22:11:39Z

src/nativeTrackerManager.js

@@ -10,6 +10,7 @@ const AD_DATA_ADID_ATTRIBUTE = 'pbAdId';
 export function newNativeTrackerManager(win) {
 let publisherDomain;

+


extra new line?

Removing this. I believe I originally added logic that I later removed.

robertrmartinez · 2020-07-22T22:12:36Z

test/spec/nativeTrackerManager_spec.js

@@ -89,6 +95,58 @@ describe('nativeTrackerManager', function () {
 expect(trimPort(postMessageTargetDomain)).to.equal(tagData.pubUrl);
 });

+ /*it('should verify the postMessage for impression trackers was executed', function() {


what is the reasoning behind this being commented out?

I forgot to remove this test. Originally the new rendering code lived within trackerManager code but it was requested to live in a new class. This was mirrored in the nativeRenderManager_spec test.

robertrmartinez · 2020-09-24T17:45:05Z

Ok changes look good to me.

Would like @jaiminpanchal27 to do a review before we merge this.

Specifically making sure the Xander proposal of rendererUrl looks good. since they spec'd that part out.

More than happy to set some time or keep an open dialogue if you want @jaiminpanchal27

Thanks all!

robertrmartinez · 2020-09-28T17:55:49Z

The associated Prebid.js change has been approved and is ready for merge once this has been approved and merged as well:

prebid/Prebid.js#5411

robertrmartinez

Looks good to me

jsnellbaker

@mmoschovas
I think the code here looks okay. Can you please remove the .DS_Store file and the package-lock.json updates from the PR though?

mmoschovas · 2020-10-09T20:54:02Z

Removed both updates

… file to avoid loading unused code lib

…/prebid-universal-creative into native-phase-2-update

…nderer exists then adId in message must match adId in creative window, before replacing assets

robertrmartinez

couple minor things

robertrmartinez · 2020-10-21T20:15:11Z

src/nativeAssetManager.js

@@ -227,12 +271,45 @@ export function newNativeAssetManager(win) {
 }

 if (data.message === 'assetResponse') {
- const body = win.document.body.innerHTML;
- const newHtml = replace(body, data);
+ const body = win.document.body.innerHTML,flag = (typeof win.pbNativeData !== 'undefined');


Can you split this flag out to a new declaration please.

robertrmartinez · 2020-10-21T20:16:09Z

src/nativeAssetManager.js

- win.document.body.innerHTML = newHtml;
- callback && callback();
- win.removeEventListener('message', replaceAssets);
+ if (flag && data.adId != win.pbNativeData.adId) return;


Any particular reason to use != vs !===?

Do we expect adId to be the same data type in both? Or is is possible one is string and one is number?

Nope good catch. Updated

robertrmartinez · 2020-10-21T20:17:39Z

src/nativeRenderManager.js

+ nativeAssetManager.loadAssets(nativeTag.adId,fireNativeCallback);
+ fireNativeCallback();
+ fireNativeImpTracker(nativeTag.adId);
+ }else{


Should have a space after closing bracket befor else and after

} else {

robertrmartinez · 2020-10-21T20:21:50Z

src/nativeAssetManager.js

+ }
+
+ return false;
+ }


Could just do

return win.document.body.innerHTML.indexOf(str) !== -1

Not sure this function is even being used?

I dont remember this function but cant find a use for it so removing

robertrmartinez · 2020-10-21T20:22:50Z

src/nativeAssetManager.js


 if (placeholders.length > 0) {
 callback = cb;
 requestAssets(adId, placeholders);
+ }else if (flag && win.pbNativeData.hasOwnProperty('requestAllAssets') && win.pbNativeData.requestAllAssets) {


space after closing brackets

Fixed these. Not sure how they didnt update last time around

robertrmartinez · 2020-10-21T20:24:44Z

src/nativeRenderManager.js

+ nativeAssetManager.loadAssets(nativeTag.adId,fireNativeCallback);
+ fireNativeCallback();
+ fireNativeImpTracker(nativeTag.adId);
+ }else{


maybe just run enable eslint in your editor and see what else there is?

… to see if placeholders are found

bretg · 2020-11-06T23:00:33Z

Docs PR prebid/prebid.github.io#2479

jsnellbaker · 2021-01-12T18:44:13Z

@mmoschovas Can you please take a look at the code conflict in the gulpfile.js? I'm not sure if it's a recent thing, but it needs to be addressed.

Thanks.

mmoschovas · 2021-01-12T20:26:51Z

@jsnellbaker committed merge

jsnellbaker · 2021-01-13T13:56:49Z

@mmoschovas Thanks for the updates and for the overall effort behind this PR. Based on side conversations, we are good to merge this PR (finally).

robertrmartinez requested a review from jaiminpanchal27 June 23, 2020 18:45

robertrmartinez assigned jaiminpanchal27 and robertrmartinez Jun 23, 2020

robertrmartinez self-requested a review June 23, 2020 18:45

wqi1972 reviewed Jun 24, 2020

View reviewed changes

Update to separate renderNative method into new class called nativeRe…

1e220d4

…nderManager. Removed repetitive publisherDomain definitions

wqi1972 reviewed Jun 25, 2020

View reviewed changes

robertrmartinez requested changes Jul 22, 2020

View reviewed changes

Updates to fix lint errors and remove commented code

4f11052

robertrmartinez mentioned this pull request Sep 8, 2020

Proposal - Improve Prebid Native when sendAllBids is set to true prebid/Prebid.js#5149

Closed

robertrmartinez mentioned this pull request Sep 28, 2020

Prebid JS Native Phase 2 update: prebid/Prebid.js#5411

Merged

10 tasks

bretg requested a review from robertrmartinez October 7, 2020 20:40

robertrmartinez approved these changes Oct 7, 2020

View reviewed changes

jsnellbaker approved these changes Oct 9, 2020

View reviewed changes

mmoschovas added 2 commits October 9, 2020 16:44

Revert package-lock

b8d8f22

Remove DS_Store

6cacfc5

wqi1972 approved these changes Oct 14, 2020

View reviewed changes

mmoschovas added 4 commits October 20, 2020 12:03

Update to separate new native functionality into new native-render.js…

a4d5f58

… file to avoid loading unused code lib

Merge branch 'native-phase-2-update' of https://github.com/mmoschovas…

5d953f3

…/prebid-universal-creative into native-phase-2-update

Added line to message event listener to validate that if new nativeRe…

f436d1e

…nderer exists then adId in message must match adId in creative window, before replacing assets

Added test for mismatch adId in replaceAsset response

319ac69

robertrmartinez requested changes Oct 21, 2020

View reviewed changes

Minor fixes

413e443

mmoschovas requested a review from robertrmartinez October 21, 2020 21:24

mmoschovas added 3 commits October 22, 2020 16:19

Addition to inlcude replace of macros in CSS in head of document

6f1ca64

Update to lookup macros in CSS not defined in BODY

1af6176

Updating asset logic to first see if request all is set then fallback…

d0184e2

… to see if placeholders are found

bretg added the Needs: Docs label Oct 23, 2020

bretg mentioned this pull request Nov 6, 2020

Native phase 2 prebid/prebid.github.io#2479

Merged

mmoschovas added 2 commits November 13, 2020 11:46

Update to include post message to resize safeframe

4ee7db1

Fix for render URL

9ddae29

allanjun approved these changes Nov 16, 2020

View reviewed changes

bretg added LGTM and removed Needs: Docs labels Dec 10, 2020

Merge branch 'master' into native-phase-2-update

73fa612

jsnellbaker merged commit 647c7d5 into prebid:master Jan 13, 2021

		@@ -10,6 +10,7 @@ const AD_DATA_ADID_ATTRIBUTE = 'pbAdId';
		export function newNativeTrackerManager(win) {
		let publisherDomain;

PUC update to include Native Phase 2 spec: #106

PUC update to include Native Phase 2 spec: #106

Conversation

mmoschovas commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wqi1972 commented Jun 24, 2020

wqi1972 commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez commented Jul 14, 2020

wqi1972 commented Jul 14, 2020

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez commented Sep 24, 2020

robertrmartinez commented Sep 28, 2020

robertrmartinez left a comment

Choose a reason for hiding this comment

jsnellbaker left a comment

Choose a reason for hiding this comment

mmoschovas commented Oct 9, 2020

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretg commented Nov 6, 2020

jsnellbaker commented Jan 12, 2021

mmoschovas commented Jan 12, 2021

jsnellbaker commented Jan 13, 2021