Skip to content

Commit

Permalink
Add feature to allow adding URL params to the front of the URL. (#3163)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cramforce committed May 11, 2016
1 parent 2afa6d2 commit 644bbca
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 17 deletions.
2 changes: 1 addition & 1 deletion ads/alp/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function handleClick(e) {
// Tag the original href with &amp=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;
Expand Down
25 changes: 17 additions & 8 deletions src/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
/**
Expand All @@ -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);
}

/**
Expand Down
14 changes: 6 additions & 8 deletions test/functional/test-alp-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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',
]);
});
Expand All @@ -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',
]);
});
Expand Down
10 changes: 10 additions & 0 deletions test/functional/test-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,16 @@ describe('addParamToUrl', () => {
expect(url).to.equal('https://www.ampproject.org/get/here?hello=world&foo=bar&elementId=n1&ampUserId=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');
Expand Down

0 comments on commit 644bbca

Please sign in to comment.