From 644bbca7972d0bb4373fc1d524b1407259813adf Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 10 May 2016 19:34:00 -0700 Subject: [PATCH] Add feature to allow adding URL params to the front of the URL. (#3163) This is needed because some ad servers have (probably for legacy reasons) a behavior, where they treat URL params at the end specially. This is likely because these encode a URL and people can get that wrong. Affects #2934 --- ads/alp/handler.js | 2 +- src/url.js | 25 +++++++++++++++++-------- test/functional/test-alp-handler.js | 14 ++++++-------- test/functional/test-url.js | 10 ++++++++++ 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/ads/alp/handler.js b/ads/alp/handler.js index 8feea1b471e6..1ea013cbd0c4 100644 --- a/ads/alp/handler.js +++ b/ads/alp/handler.js @@ -65,7 +65,7 @@ export function handleClick(e) { // Tag the original href with &=1 and make it a fragment param with // name click. const fragment = 'click=' + encodeURIComponent( - addParamToUrl(link.a.href, 'amp', '1')); + addParamToUrl(link.a.href, 'amp', '1', /* opt_addToFront */ true)); let destination = link.eventualUrl; if (link.eventualUrl.indexOf('#') == -1) { destination += '#' + fragment; diff --git a/src/url.js b/src/url.js index ff7870d6ad87..d32158f550b8 100644 --- a/src/url.js +++ b/src/url.js @@ -62,19 +62,27 @@ export function parseUrl(url) { } /** - * Appends the string just before the fragment part of the URL. + * Appends the string just before the fragment part (or optionally + * to the front of the query string) of the URL. * @param {string} url * @param {string} paramString + * @param {boolean=} opt_addToFront * @return {string} */ -function appendParamStringToUrl(url, paramString) { +function appendParamStringToUrl(url, paramString, opt_addToFront) { if (!paramString) { return url; } - const parts = url.split('#', 2); - let newUrl = parts[0] + ( - parts[0].indexOf('?') >= 0 ? `&${paramString}` : `?${paramString}`); - newUrl += parts[1] ? `#${parts[1]}` : ''; + const mainAndFragment = url.split('#', 2); + const mainAndQuery = mainAndFragment[0].split('?', 2); + + let newUrl = mainAndQuery[0] + ( + mainAndQuery[1] + ? (opt_addToFront + ? `?${paramString}&${mainAndQuery[1]}` + : `?${mainAndQuery[1]}&${paramString}`) + : `?${paramString}`); + newUrl += mainAndFragment[1] ? `#${mainAndFragment[1]}` : ''; return newUrl; } /** @@ -83,11 +91,12 @@ function appendParamStringToUrl(url, paramString) { * @param {string} url * @param {string} key * @param {string} value + * @param {boolean=} opt_addToFront * @return {string} */ -export function addParamToUrl(url, key, value) { +export function addParamToUrl(url, key, value, opt_addToFront) { const field = `${encodeURIComponent(key)}=${encodeURIComponent(value)}`; - return appendParamStringToUrl(url, field); + return appendParamStringToUrl(url, field, opt_addToFront); } /** diff --git a/test/functional/test-alp-handler.js b/test/functional/test-alp-handler.js index b2995747ee76..b1c7a23e11fd 100644 --- a/test/functional/test-alp-handler.js +++ b/test/functional/test-alp-handler.js @@ -73,9 +73,8 @@ describe('alp-handler', () => { expect(open.callCount).to.equal(1); expect(open.lastCall.args).to.jsonEqual([ 'https://cdn.ampproject.org/c/www.example.com/amp.html#click=' + - 'https%3A%2F%2Ftest.com%3Fadurl%3Dhttps%253A%252F%252F' + - 'cdn.ampproject.org%252Fc%252Fwww.example.com%252Famp.html%26' + - 'amp%3D1', + 'https%3A%2F%2Ftest.com%3Famp%3D1%26adurl%3Dhttps%253A%252F%252F' + + 'cdn.ampproject.org%252Fc%252Fwww.example.com%252Famp.html', '_top', ]); expect(event.preventDefault.callCount).to.equal(1); @@ -104,9 +103,8 @@ describe('alp-handler', () => { handleClick(event); expect(open.lastCall.args).to.jsonEqual([ 'https://cdn.ampproject.org/c/www.example.com/amp.html#click=' + - 'https%3A%2F%2Ftest.com%3FTEST%3Dhttps%253A%252F%252F' + - 'cdn.ampproject.org%252Fc%252Fwww.example.com%252Famp.html%26' + - 'amp%3D1', + 'https%3A%2F%2Ftest.com%3Famp%3D1%26TEST%3Dhttps%253A%252F%252F' + + 'cdn.ampproject.org%252Fc%252Fwww.example.com%252Famp.html', '_top', ]); }); @@ -118,9 +116,9 @@ describe('alp-handler', () => { handleClick(event); expect(open.lastCall.args).to.jsonEqual([ 'https://cdn.ampproject.org/c/www.example.com/amp.html#test=1&click=' + - 'https%3A%2F%2Ftest.com%3Fadurl%3Dhttps%253A%252F%252F' + + 'https%3A%2F%2Ftest.com%3Famp%3D1%26adurl%3Dhttps%253A%252F%252F' + 'cdn.ampproject.org%252Fc%252Fwww.example.com%252Famp.html' + - '%2523test%253D1%26amp%3D1', + '%2523test%253D1', '_top', ]); }); diff --git a/test/functional/test-url.js b/test/functional/test-url.js index b5a2fbd9fc40..7ae328a9984c 100644 --- a/test/functional/test-url.js +++ b/test/functional/test-url.js @@ -305,6 +305,16 @@ describe('addParamToUrl', () => { expect(url).to.equal('https://www.ampproject.org/get/here?hello=world&foo=bar&elementId=n1&UserId=12345'); }); + it('should optionally add params to the front', () => { + let url = addParamToUrl('https://www.ampproject.org/get/here?hello=world&foo=bar', + 'elementId', 'n1', /* addToFront */ true); + expect(url).to.equal('https://www.ampproject.org/get/here?elementId=n1&hello=world&foo=bar'); + + url = addParamToUrl('https://www.ampproject.org/get/here', + 'elementId', 'n1', /* addToFront */ true); + expect(url).to.equal('https://www.ampproject.org/get/here?elementId=n1'); + }); + it('should encode uri values', () => { url = addParamToUrl(url, 'foo', 'b ar'); expect(url).to.equal('https://www.ampproject.org/get/here?foo=b%20ar#hash-value');