From 366a535bf7a4c0c0e33125637c1052593a5bd199 Mon Sep 17 00:00:00 2001 From: zhouyx Date: Mon, 17 Oct 2016 10:48:48 -0700 Subject: [PATCH] another approach --- build-system/server.js | 20 ++---- src/impression.js | 69 +++++++++---------- src/service/url-replacements-impl.js | 31 +++++++++ test/functional/test-impression.js | 87 +++--------------------- test/functional/test-url-replacements.js | 34 ++++++++- 5 files changed, 113 insertions(+), 128 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index cf45e0e4acf51..1852de648ddb1 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -407,21 +407,13 @@ app.use('/examples/amp-fresh.amp.(min.|max.)?html', function(req, res, next) { app.use('/impression-proxy/', function(req, res) { assertCors(req, res, ['GET']); - const fakeHeaders = { - 'Host': 'adclick.g.doubleclick.net', - 'Origin': 'https://cdn.ampproject.org', - 'Cookie': 'Cookie:id=4903ef7b59596f28||t=1451370221|et=730|cs=002213fd48e4405e4579c95dfa', + // TODO(@zhouyx): How to request real ad server response with cookie set + const body = { + 'location': 'localhost:8000/examples/#gclid=1234', + 'gclid': '1234', + 'tracking': 'tracking_url', }; - const options = { - url: 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236', - headers: fakeHeaders, - }; - - request(options, (error, response, body) => { - if (!error && response.statusCode == 200) { - res.send(body); - } - }); + res.send(body); }); // Proxy with unminified JS. diff --git a/src/impression.js b/src/impression.js index bffc0894f7204..254ab6a82ecef 100644 --- a/src/impression.js +++ b/src/impression.js @@ -14,13 +14,16 @@ * limitations under the License. */ -import {user} from './log'; +import {dev, user} from './log'; import {isExperimentOn} from './experiments'; import {viewerForDoc} from './viewer'; import {xhrFor} from './xhr'; import {getMode} from './mode'; -import {openWindowDialog} from './dom'; +/** A promise that resolve after getting info from ad server + * {Promise=} + */ +export let trackImpressionPromise; /** * Emit a HTTP request to a destination defined on the incoming URL. @@ -28,23 +31,30 @@ import {openWindowDialog} from './dom'; * @param {!Window} win */ export function maybeTrackImpression(win) { + let resolveImpression; + + trackImpressionPromise = new Promise(resolve => { + resolveImpression = resolve; + }); + if (!isExperimentOn(win, 'alp')) { + resolveImpression(); return; } const viewer = viewerForDoc(win.document); /** @const {string|undefined} */ - let clickUrl = viewer.getParam('click'); + const clickUrl = viewer.getParam('click'); - // One test clickUrl - clickUrl = 'https://googleads.g.doubleclick.net/pcs/click?alp=1&xai=AKAOjstvASJmd6-NeoO3ner8FBNJW2w8n-sXMo0Nj5YC_LIY2NQjbNs0CoXQtM9tPi8by4H4bHRpMdB14qgRLctKkBKkh3vpR3m8fvPCzcFZ6HrxvxXUqzP17YJsihcINtRniOfmGFkzIolJ3ccPSPq6oYdJpg5lPeufOrLhtWsNspOsRgMSBFP7zH0l8tgtAb665jHEFmdAMH1vl69BxpqU2Q0ZoGDO_SVBMArlL--2nLOVgQt8om6IdzkcodppT9c&sig=Cg0ArKJSzEJGKs3-NmXNEAE&urlfix=1&adurl=https://cdn.ampproject.org/c/www.nbcnews.com/news/us-news/amp/milwaukee-cop-cars-smashed-torched-after-police-kill-suspect-n630236'; if (!clickUrl) { + resolveImpression(); return; } if (clickUrl.indexOf('https://') != 0) { user().warn('Impression', 'click fragment param should start with https://. Found ', clickUrl); + resolveImpression(); return; } if (win.location.hash) { @@ -57,11 +67,9 @@ export function maybeTrackImpression(win) { viewer.whenFirstVisible().then(() => { // TODO(@zhouyx) We need a timeout here? // TODO(@zhouyx) need test with a real response. - invoke(win, clickUrl).then(response => { - if (!response) { - return; - } + invoke(win, dev().assertString(clickUrl)).then(response => { applyResponse(win, viewer, response); + resolveImpression(); }); }); } @@ -70,7 +78,7 @@ export function maybeTrackImpression(win) { * Send the url to ad server and wait for its response * @param {!Window} win * @param {string} clickUrl - * @return {!Promise} + * @return {!Promise} */ function invoke(win, clickUrl) { if (getMode().localDev && !getMode().test) { @@ -80,7 +88,6 @@ function invoke(win, clickUrl) { credentials: 'include', requireAmpResponseSourceOrigin: true, }); - // TODO(@cramforce): Do something with the result. } /** @@ -91,34 +98,24 @@ function invoke(win, clickUrl) { */ function applyResponse(win, viewer, response) { const adLocation = response['location']; + const adTracking = response['tracking']; - // If there's a tracking_url, need to redirect to that url. - const adTrackingUrl = response['tracking_url']; - if (adTrackingUrl) { - // TODO(@zhouyx) Confirm we assume the tracking_url is not - // a cdn.ampproject page, and we don't do viewer redirect - openWindowDialog(win, adTrackingUrl, '_top'); - return; + // If there is a tracking_url, need to track it + // Otherwise track the location + const trackUrl = adTracking || adLocation; + if (trackUrl) { + new Image().src = trackUrl; } - // If adLocation is not an amp page, need to redirect to it. - if (adLocation && adLocation.indexOf('https://cdn.ampproject.org') != 0) { - // TODO(@zhouyx) Confirm we assume the tracking_url is not - // a cdn.ampproject page, and we don't do viewer redirect - openWindowDialog(win, adLocation, '_top'); - return; - } - - // If have gclid, or location contains gclid. add it to location hash - // Set gclid. - // If provided use it, if not try to find in adLocation from response - let gclid = response['gclid']; - if (!gclid) { - if (adLocation.indexOf('gclid=') > 0) { - gclid = adLocation.substr(adLocation.indexOf('gclid=') + 6); + // If have we have gclid replace the location href with new location we get. + // TODO(@zhouyx) should we replaceState directly w/o checking gclid? + const gclid = response['gclid']; + if (gclid && adLocation) { + if (getMode().localDev && !getMode().test) { + win.history.replaceState(null, '', win.location.href + + '#gclid=' + gclid); + return; } - } - if (gclid) { - win.location.hash = '#gclid=' + gclid; + win.history.replaceState(null, '', adLocation); } } diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js index 341bf295f2b4f..7318819a0b917 100644 --- a/src/service/url-replacements-impl.js +++ b/src/service/url-replacements-impl.js @@ -29,6 +29,7 @@ import {viewportForDoc} from '../viewport'; import {userNotificationManagerFor} from '../user-notification'; import {activityFor} from '../activity'; import {isExperimentOn} from '../experiments'; +import {trackImpressionPromise} from '../impression.js'; /** @private @const {string} */ @@ -163,6 +164,18 @@ export class UrlReplacements { return removeFragment(info.sourceUrl); })); + this.setAsync_('SOURCE_URL', () => { + let promise = trackImpressionPromise; + if (!promise) { + promise = Promise.resolve(); + } + return promise.then( + this.getDocInfoValue_.bind(this, info => { + return removeFragment(info.sourceUrl); + }) + ); + }); + // Returns the host of the Source URL for this AMP document. this.set_('SOURCE_HOST', this.getDocInfoValue_.bind(this, info => { return parseUrl(info.sourceUrl).host; @@ -197,6 +210,24 @@ export class UrlReplacements { defaultValue; }); + this.setAsync_('QUERY_PARAM', (param, defaultValue = '') => { + user().assert(param, + 'The first argument to QUERY_PARAM, the query string ' + + 'param is required'); + let promise = trackImpressionPromise; + if (!promise) { + promise = Promise.resolve(); + } + return promise.then(() => { + const url = parseUrl(this.ampdoc.win.location.href); + const params = parseQueryString(url.search); + + return (typeof params[param] !== 'undefined') ? + params[param] : + defaultValue; + }); + }); + /** * Stores client ids that were generated during this page view * indexed by scope. diff --git a/test/functional/test-impression.js b/test/functional/test-impression.js index 74d419d4b37b5..cda05f0cbae87 100644 --- a/test/functional/test-impression.js +++ b/test/functional/test-impression.js @@ -14,11 +14,13 @@ * limitations under the License. */ -import {maybeTrackImpression} from '../../src/impression'; +import { + trackImpressionPromise, + maybeTrackImpression, +} from '../../src/impression'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; import {xhrFor} from '../../src/xhr'; -import * as dom from '../../src/dom'; import * as sinon from 'sinon'; describe('impression', () => { @@ -49,6 +51,7 @@ describe('impression', () => { it('should do nothing if the experiment is off', () => { viewer.getParam.throws(new Error('Should not be called')); maybeTrackImpression(window); + return trackImpressionPromise.should.be.fulfilled; }); it('should do nothing if there is no click arg', () => { @@ -56,6 +59,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns(''); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); + return trackImpressionPromise.should.be.fulfilled; }); it('should do nothing if there is the click arg is http', () => { @@ -63,6 +67,7 @@ describe('impression', () => { viewer.getParam.withArgs('click').returns('http://www.example.com'); maybeTrackImpression(window); expect(xhr.fetchJson.callCount).to.equal(0); + return trackImpressionPromise.should.be.fulfilled; }); it('should invoke URL', () => { @@ -82,84 +87,12 @@ describe('impression', () => { }); }); - it('should redirect if xhr return tracking url', () => { + it('should replace location href', () => { toggleExperiment(window, 'alp', true); viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', - (win, url, target) => { - expect(url).to.equal('test_tracking_url'); - expect(target).to.equal('_top'); - }); - - xhr.fetchJson = () => { - return Promise.resolve({ - 'location': 'test_location', - 'tracking_url': 'test_tracking_url', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.be.calledOnce; - }); - }); - }); - - it('should redirect if location is some other url', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.stub(dom, 'openWindowDialog', - (win, url, target) => { - expect(url).to.equal('test_location'); - expect(target).to.equal('_top'); - }); - xhr.fetchJson = () => { return Promise.resolve({ 'location': 'test_location', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.be.calledOnce; - }); - }); - }); - - it('should not redirect if location is valid', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); - - xhr.fetchJson = () => { - return Promise.resolve({ - 'location': 'https://cdn.ampproject.org/c/test/?gclid=654321', - }); - }; - - maybeTrackImpression(window); - return Promise.resolve().then(() => { - return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.not.be.called; - expect(window.location.hash).to.equal('#gclid=654321'); - }); - }); - }); - - it('should set gclid to location hash', () => { - toggleExperiment(window, 'alp', true); - viewer.getParam.withArgs('click').returns('https://www.example.com'); - - const openWindowDialogSpy = sandbox.spy(dom, 'openWindowDialog'); - - xhr.fetchJson = () => { - return Promise.resolve({ 'gclid': '123456', }); }; @@ -167,8 +100,8 @@ describe('impression', () => { maybeTrackImpression(window); return Promise.resolve().then(() => { return Promise.resolve().then(() => { - expect(openWindowDialogSpy).to.not.be.called; - expect(window.location.hash).to.be.equal('#gclid=123456'); + expect(window.location.href).to.contain('test_location'); + return trackImpressionPromise.should.be.fulfilled; }); }); }); diff --git a/test/functional/test-url-replacements.js b/test/functional/test-url-replacements.js index c972b3ec71774..54fff06797fbf 100644 --- a/test/functional/test-url-replacements.js +++ b/test/functional/test-url-replacements.js @@ -33,6 +33,7 @@ import {setCookie} from '../../src/cookies'; import {parseUrl} from '../../src/url'; import {toggleExperiment} from '../../src/experiments'; import {viewerForDoc} from '../../src/viewer'; +import * as trackPromise from '../../src/impression'; import * as sinon from 'sinon'; @@ -198,6 +199,7 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOST', () => { + sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOST').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOST/); @@ -205,12 +207,32 @@ describe('UrlReplacements', () => { }); it('should replace SOURCE_URL and _HOSTNAME', () => { + sandbox.stub(trackPromise, 'trackImpressionPromise', Promise.resolve()); return expandAsync('?url=SOURCE_URL&host=SOURCE_HOSTNAME').then(res => { expect(res).to.not.match(/SOURCE_URL/); expect(res).to.not.match(/SOURCE_HOSTNAME/); }); }); + it('should update SOURCE_URL after track impression', () => { + const win = getFakeWindow(); + win.location = parseUrl('https://wrong.com'); + const clock = sandbox.useFakeTimers(); + sandbox.stub(trackPromise, 'trackImpressionPromise', + new Promise(resolve => { + setTimeout(() => { + win.location = parseUrl('https://example.com#gclid=123456'); + resolve(); + }, 1000); + })); + clock.tick(1001); + return installUrlReplacementsServiceForDoc(win.ampdoc) + .expandAsync('?url=SOURCE_URL') + .then(res => { + expect(res).to.contain('example.com'); + }); + }); + it('should replace SOURCE_PATH', () => { return expandAsync('?path=SOURCE_PATH').then(res => { expect(res).to.not.match(/SOURCE_PATH/); @@ -727,7 +749,17 @@ describe('UrlReplacements', () => { it('should replace QUERY_PARAM with foo', () => { const win = getFakeWindow(); - win.location = parseUrl('https://example.com?query_string_param1=foo'); + win.location = parseUrl('https://example.com?query_string_param1=wrong'); + const clock = sandbox.useFakeTimers(); + sandbox.stub(trackPromise, 'trackImpressionPromise', + new Promise(resolve => { + setTimeout(() => { + win.location = + parseUrl('https://example.com?query_string_param1=foo'); + resolve(); + }, 1000); + })); + clock.tick(1001); return installUrlReplacementsServiceForDoc(win.ampdoc) .expandAsync('?sh=QUERY_PARAM(query_string_param1)&s') .then(res => {