maccman / abba

A/B testing framework
MIT License
1.35k stars 70 forks source link

Bug in extend() causes abbaVariant, abbaPersistComplete cookies to have incorrect expiration dates #8

Closed typpo closed 11 years ago

typpo commented 11 years ago

'extend', when compiled to js, was returning an array of arrays rather than an object due to implicit return.

This broke the options sent to setCookie, because the 'expires' attribute of the returned options object was always undefined, so the cookie expiration field was never set as options.expires.toGMTString().

As a result, the abbaVariant and abbaPersistComplete cookies incorrectly had no expiration date or a received a default expiration from the browser, rather than the 600 day expiration specified in the code.

Below is the old extend, compiled to js, which incorrectly returns a list:

  extend = function() {
    var args, key, source, target, value, _i, _len, _results;
    target = arguments[0], args = 2 <= arguments.length ? __slice.call(arguments, 1) : [];
    _results = [];
    for (_i = 0, _len = args.length; _i < _len; _i++) {
      source = args[_i];
      _results.push((function() {
    var _results1;
    _results1 = [];
    for (key in source) {
      value = source[key];
      if (value != null) {
        _results1.push(target[key] = value);
      }
    }
    return _results1;
      })());
    }
    return _results;
  };

An explicit return compiles correctly:

  extend = function() {
    var args, key, source, target, value, _i, _len;
    target = arguments[0], args = 2 <= arguments.length ? __slice.call(arguments, 1) : [];
    for (_i = 0, _len = args.length; _i < _len; _i++) {
      source = args[_i];
      for (key in source) {
    value = source[key];
    if (value != null) {
      target[key] = value;
    }
      }
    }
    return target;
  };
typpo commented 11 years ago

I found this problem when making changes to allow custom expirations. This would help avoid having the cookie inflate request size with a year's worth of A/B tests. Unfortunately, this bug is a blocker for a custom expiration option.

maccman commented 11 years ago

Any chance you could edit the coffeescript?

On Wed, Mar 20, 2013 at 3:26 PM, Ian Webster notifications@github.comwrote:

I found this problem when making changes to allow custom expirations. This would help avoid having the cookie inflate request size with a year's worth of A/B tests. Unfortunately, this bug is a blocker for a custom expiration option.

— Reply to this email directly or view it on GitHubhttps://github.com/maccman/abba/pull/8#issuecomment-15207757 .

Alex MacCaw

+12147175129 @maccman

http://alexmaccaw.com

typpo commented 11 years ago

My attached change (e73c4d7) edits the coffeescript and solves the problem.

maccman commented 11 years ago

My bad, I missed that, so I applied the fix straight to master. Thanks so much! Really appreciated.