Skip to content

Commit

Permalink
Use JSON for saving method params
Browse files Browse the repository at this point in the history
* Splitting with a delimiter would break if the delimiter was included
in the params
* Joining the array of params also inadvertently cast params to strings
* Use JSON stringify/parse to avoid these problems
* Catch JSON parse errors and continue, so that the troublesome cookie
gets deleted
* govuk-template provides a JSON shim for older browsers
* Remove old spec helper regarding JSON.parse, all tests pass correctly
with the latest PhantomJS it seems.
  • Loading branch information
fofr committed Mar 19, 2015
1 parent a7c4c5b commit cc1292d
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 22 deletions.
12 changes: 8 additions & 4 deletions app/assets/javascripts/analytics/static-tracker.js
Expand Up @@ -91,16 +91,20 @@

if (GOVUK.cookie && typeof this[method] === "function") {
params.unshift(method);
GOVUK.cookie('analytics_next_page_call', params.join('|'));
GOVUK.cookie('analytics_next_page_call', JSON.stringify(params));
}
};

StaticTracker.prototype.callMethodRequestedByPreviousPage = function() {
if (GOVUK.cookie && GOVUK.cookie('analytics_next_page_call') !== null) {
var params = GOVUK.cookie('analytics_next_page_call').split('|'),
method = params.shift();
var params, method;

if (typeof this[method] === "function") {
try {
params = JSON.parse(GOVUK.cookie('analytics_next_page_call'));
method = params.shift();
} catch(e) {}

if (method && typeof this[method] === "function") {
this[method].apply(this, params);
}

Expand Down
31 changes: 25 additions & 6 deletions spec/javascripts/analytics/static-tracker-spec.js
Expand Up @@ -54,7 +54,13 @@ describe("GOVUK.StaticTracker", function() {
it('sets them as a dimension', function() {
window.ga.calls.reset();
window._gaq = [];
spyOn(GOVUK, 'cookie').and.returnValue("_setCustomVar,21,name,value,3");
spyOn(GOVUK, 'cookie').and.callFake(function(name) {
if (name === "ga_nextpage_params") {
return "_setCustomVar,21,name,value,3";
}

return null;
});
tracker = new GOVUK.StaticTracker({universalId: 'universal-id', classicId: 'classic-id'});
universalSetupArguments = window.ga.calls.allArgs();

Expand Down Expand Up @@ -188,17 +194,17 @@ describe("GOVUK.StaticTracker", function() {
describe('and the method exists', function() {
it('sets a cookie with the method name', function() {
tracker.callOnNextPage('trackPageview');
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', 'trackPageview');
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', '["trackPageview"]');
});

it('sets a cookie with the parameters to call', function() {
tracker.callOnNextPage('trackPageview', ['/path', 'Custom Title']);
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', 'trackPageview|/path|Custom Title');
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', '["trackPageview","/path","Custom Title"]');
});

it('sets a cookie with the single parameter to call', function() {
tracker.callOnNextPage('trackPageview', '/path');
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', 'trackPageview|/path');
expect(GOVUK.cookie).toHaveBeenCalledWith('analytics_next_page_call', '["trackPageview","/path"]');
});
});

Expand All @@ -216,13 +222,26 @@ describe("GOVUK.StaticTracker", function() {
});

it('calls the method', function() {
spyOn(GOVUK, 'cookie').and.returnValue("trackPageview");
spyOn(GOVUK, 'cookie').and.callFake(function(name) {
if (name === "analytics_next_page_call") {
return '["trackPageview"]';
}

return null;
});
tracker.callMethodRequestedByPreviousPage();
expect(tracker.trackPageview).toHaveBeenCalledWith();
});

it('calls the method with given parameters', function() {
spyOn(GOVUK, 'cookie').and.returnValue("trackPageview|/path|Title");
spyOn(GOVUK, 'cookie').and.callFake(function(name) {
if (name === "analytics_next_page_call") {
return '["trackPageview","/path","Title"]';
}

return null;
});

tracker.callMethodRequestedByPreviousPage();
expect(tracker.trackPageview).toHaveBeenCalledWith('/path', 'Title');
});
Expand Down
12 changes: 0 additions & 12 deletions spec/javascripts/helpers/SpecHelper.js
Expand Up @@ -21,15 +21,3 @@ beforeEach(function () {
}
});
});

// When using phantomJS JSON.parse seems bugged, returning a string instead of a built object.
// I'm unsure as to why, since when testing this behaviour directly in phantom it behaves
// as expected. This monkey patches the issue, but It'd be good to find out what's going on here.
var _JSON_parse = JSON.parse;
JSON.parse = function(json_string) {
var result = _JSON_parse(json_string);
if (typeof result == "string") {
result = eval(result);
}
return result;
}

0 comments on commit cc1292d

Please sign in to comment.