Handle an odd case with the user agent

[aub]

Fixes #243

I'm getting a JS crash due to an odd useragent from the client. The agent I'm getting is:

Mozilla/5.0 (iPad; CPU OS 11_4_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15G77 KurogoVersion/2.7.7 (Kurogo iOS Tablet) KurogoOSVersion/11.4.1 KurogoAppVersion/2.0.1 (com.telerik.TridentUniversity)

This Kurogo browser is used by universities, and it includes the name of the university in the useragent string. In this case, the university is called Trident. In the isMsie code in utils.js, the first line tests for the presence of msie or trident, and the positive side of the unary operator assumes this means it will be checking a standard IE useragent string to get the revision. In my case, that is not the case, so navigator.userAgent.match(/(msie |rv:)(\d+(.\d+)?)/i) return null and trying to index into null as an array makes a crash.

What I've done is split out the unary to get the result of the match and make sure it exists before indexing into it. This fixes the crash.

This is an attempt at solving the crash described above. It fixes issue #243.

Unfortunately, I was unable to figure out how to spoof the useragent in a test after a bunch of searching. To validate this, I copied the useragent string i'm getting directly into the code, made a test that isMsie was working as expected, and then deleted that hack. It worked as expected both with my hack in place and with the user agent provided by phantomjs.

[Haroenv]

Thanks, this seems to be correct, I think a test could be added where navigator.userAgent is set before doing the test of isMsie, what do you think? There's currently no test of it yet, so a new file for utils can be made

[aub]

@Haroenv I've tried that but unfortunately the useragent doesn't carry through to the test. Here's the test I wrote in utils_spec.js:

  it('should handle a user agent that is not IE but includes the string "trident"', function() {
    navigator.userAgent = 'FOO!!!!!!!!!!!!!!!!!!!!!!!!!!!';
    var actual = _.isMsie();
    expect(actual).toEqual(false);
  });

Then I added a console.log to the isMsie function to print out the useragent, but it always prints out:

Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/538.1 (KHTML, like Gecko) PhantomJS/2.1.1 Safari/538.1

[coveralls]

Coverage increased (+0.05%) to 88.806% when pulling 0e6227e on aub:handle-odd-useragent into 55807b8 on algolia:master.

[Haroenv]

Could you try: https://stackoverflow.com/a/1307088/3185307

navigator.__defineGetter__('userAgent', function(){
    return 'foo' // customized user agent
});

navigator.userAgent; // 'foo'

preferably wrapped in a function :)

[aub]

@Haroenv unfortunately that doesn't work either 😭 I've added a test of isMsie now so that the code coverage doesn't go down, but it is also fragile... if the test were run under actual IE then it would fail.

[Haroenv]

Yes, I understand, thanks for trying to find a solution!

[Haroenv]

I'll still leave this open so someone else can take a look at it, see if they find a way to add a proper test for that. Maybe we should change _isMsie to accept the user agent instead, that way it becomes testable

[aub]

I'd be happy to make that change if you want... I thought it would be bad to change the function signature just for test support, but it would be super easy to add an optional argument that would default back to the navigator's useragent if not provided.

[Haroenv]

I think the advantage to make it testable would be worth it, feel free to do so in this PR and add some tests with various real user agents :)

[aub]

@Haroenv I added the tests.

[Haroenv]

ah, I didn't think about adding it like that, seems fine to me, I originally suggested to change the usage everywhere, but this is easier and makes sense too.

[bobylito]

I'm ok with this change. Since I wasn't able to get much info on this browser, so I hope that it is indeed like MS IE.

[Haroenv]

I will release this next Monday, together with a fix for #241

[Haroenv]

@aub, sorry for the delay, the other feature proved to be complicated to test, this is now released in 0.31.0