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

Enable adding analytics to template ad #13114

Merged
merged 6 commits into from
Jan 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions extensions/amp-a4a/0.1/amp-ad-templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import {getMode} from '../../../src/mode';
import {urls} from '../../../src/config';
import {parseUrl} from '../../../src/url';
import {LRUCache} from '../../../src/utils/lru-cache';
import {isArray} from '../../../src/types';
import {createElementWithAttributes} from '../../../src/dom';
import {dict} from '../../../src/utils/object';

/** @private {!Object<string, string|boolean>} */
const TEMPLATE_CORS_CONFIG = {
Expand Down Expand Up @@ -78,6 +81,35 @@ export class AmpAdTemplates {
.findAndRenderTemplate(element, templateValues);
}

/**
* @param {!Element} element
* @param {!Array|!JsonObject} analyticsValue
*/
insertAnalytics(element, analyticsValue) {
analyticsValue = /**@type {!Array}*/
(isArray(analyticsValue) ? analyticsValue : [analyticsValue]);
for (let i = 0; i < analyticsValue.length; i++) {
const config = analyticsValue[i];
const analyticsEle = element.ownerDocument.createElement('amp-analytics');
if (config['remote']) {
analyticsEle.setAttribute('config', config['remote']);
}
if (config['type']) {
analyticsEle.setAttribute('type', config['type']);
}
if (config['inline']) {
const scriptElem = createElementWithAttributes(
element.ownerDocument,
'script', dict({
'type': 'application/json',
}));
scriptElem.textContent = JSON.stringify(config['inline']);
analyticsEle.appendChild(scriptElem);
}
element.appendChild(analyticsEle);
}
}

/**
* Converts the canonical template URL to the CDN proxy URL.
* @param {string} url
Expand Down
25 changes: 25 additions & 0 deletions extensions/amp-a4a/0.1/test/test-amp-ad-templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ describes.fakeWin('amp-ad-templates', {amp: true}, env => {
expect(ampAdTemplates.getTemplateProxyUrl_(canonicalUrl)).to.equal(cdnUrl);
});

it('should render a template with correct values', () => {
win.AMP.registerTemplate('amp-mustache', AmpMustache);
});

it('should render a template with correct values', () => {
win.AMP.registerTemplate('amp-mustache', AmpMustache);
const parentDiv = doc.createElement('div');
Expand All @@ -76,5 +80,26 @@ describes.fakeWin('amp-ad-templates', {amp: true}, env => {
});
});

it('should insert analytics component', () => {
const parentDiv = doc.createElement('div');
parentDiv./*OK*/innerHTML =
'<p>123</p>';
doc.body.appendChild(parentDiv);
const analytics = [{
'remote': 'remoteUrl',
'inline': {
'requests': 'r',
},
}, {
'type': 'googleanalytics',
}];
ampAdTemplates.insertAnalytics(parentDiv, analytics);
expect(parentDiv.childNodes.length).to.equal(3);
expect(parentDiv.innerHTML).to.equal('<p>123</p>' +
'<amp-analytics config="remoteUrl">' +
'<script type="application/json">{"requests":"r"}</script>' +
'</amp-analytics>' +
'<amp-analytics type="googleanalytics"></amp-analytics>');
});
});

Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,18 @@ export class AmpAdNetworkAdzerkImpl extends AmpA4A {

/** @override */
getAmpAdMetadata(unusedCreative) {
if (this.ampCreativeJson_.analytics) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should you just load the analytics extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer not to load the extension here. AmpA4A will load extension based on metadata, I think it's better to load all extensions together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked offline. It's not that straightforward to load extension here. Will keep the current approach.

if (!this.creativeMetadata_) {
this.creativeMetadata_ = /**@type {?CreativeMetaDataDef}*/ ({});
}
if (!this.creativeMetadata_['customElementExtensions']) {
this.creativeMetadata_['customElementExtensions'] = [];
}
if (this.creativeMetadata_['customElementExtensions'].indexOf(
'amp-analytics') < 0) {
this.creativeMetadata_['customElementExtensions'].push('amp-analytics');
}
}
return /**@type {?CreativeMetaDataDef}*/(this.creativeMetadata_);
}

Expand All @@ -161,6 +173,10 @@ export class AmpAdNetworkAdzerkImpl extends AmpA4A {
this.ampCreativeJson_.data,
this.iframe.contentWindow.document.body)
.then(renderedElement => {
if (this.ampCreativeJson_.analytics) {
ampAdTemplates.insertAnalytics(
renderedElement, this.ampCreativeJson_.analytics);
}
this.iframe.contentWindow.document.body./*OK*/innerHTML =
renderedElement./*OK*/innerHTML;
});
Expand Down
17 changes: 16 additions & 1 deletion extensions/amp-ad-network-adzerk-impl/0.1/data/0
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,20 @@
"title": "Adzerk Demo",
"width": "272px",
"height": "92px"
}
},
"analytics": [{
"inline": {
"requests": {
"a": "https://www.fake.com"
},
"triggers": {
"visible": {
"on": "visible",
"request": "a"
}
}
}
}, {
"type": "googleanalytics"
}]
}
Original file line number Diff line number Diff line change
Expand Up @@ -137,4 +137,87 @@ describes.fakeWin('amp-ad-network-adzerk-impl', {amp: true}, env => {
});
});
});

describe('#getAmpAdMetadata', () => {
let template;

beforeEach(() => {
template = `<!doctype html><html ⚡><head>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-template="amp-mustache"
src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>
</head>
<body>
<template type="amp-mustache">
<p>{{foo}}</p>
</template>
</body></html>`;
fetchTextMock.withArgs(
'https://www-adzerk-com.cdn.ampproject.org/c/s/www.adzerk.com/456',
{
mode: 'cors',
method: 'GET',
ampCors: false,
credentials: 'omit',
}).returns(Promise.resolve(
{
headers: {},
text: () => template,
}));
});

it('should auto add amp-analytics if required', () => {
const adResponseBody = {
templateUrl: 'https://www.adzerk.com/456',
analytics: {'type': 'googleanalytics'},
};
return impl.maybeValidateAmpCreative(
utf8Encode(JSON.stringify(adResponseBody)).buffer,
{
get: name => {
expect(name).to.equal(AMP_TEMPLATED_CREATIVE_HEADER_NAME);
return 'amp-mustache';
},
},
() => {})
.then(buffer => utf8Decode(buffer))
.then(creative => {
expect(impl.getAmpAdMetadata()).to.jsonEqual({
minifiedCreative: creative,
customElementExtensions: ['amp-analytics'],
extensions: [],
});
// Won't insert duplicate
expect(impl.getAmpAdMetadata()).to.jsonEqual({
minifiedCreative: creative,
customElementExtensions: ['amp-analytics'],
extensions: [],
});
});
});

it('should not add amp-analytics if not', () => {
const adResponseBody = {
templateUrl: 'https://www.adzerk.com/456',
analytics: undefined,
};
return impl.maybeValidateAmpCreative(
utf8Encode(JSON.stringify(adResponseBody)).buffer,
{
get: name => {
expect(name).to.equal(AMP_TEMPLATED_CREATIVE_HEADER_NAME);
return 'amp-mustache';
},
},
() => {})
.then(buffer => utf8Decode(buffer))
.then(creative => {
expect(impl.getAmpAdMetadata()).to.jsonEqual({
minifiedCreative: creative,
customElementExtensions: [],
extensions: [],
});
});
});
});
});