Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GSuite Device Update Notifier] compareOSVersions always returns -1 if lengths don't match #2

Closed
paultag opened this issue Sep 10, 2019 · 4 comments

Comments

@paultag
Copy link

commented Sep 10, 2019

iOS currently has the stable latest version (12.4.1) and some users have the beta (13.1).

If compareOSVersions will return -1 when comparing both 12.4.1 to 13.1 as well as 13.1 to 12.4.1. This means beta users are being caught as 'out of date'.

Because comparing versions is hard, and we're v1-centric, I added a kludge for myself:

function compareOSVersions(v1, v2) {
  if (v2.length > v1.length) {
    v1 = v1.concat(Array.apply(null, Array(v2.length - v1.length)).map(function(){return 0}))
  }
    
  // if (v1.length != v2.length) {
  //   return -1;
  // }
  
  for (var i = 0; i < v1.length; i++) {
    if (v1[i] < v2[i]) {
      return -1;
    } else if (v1[i] > v2[i]) {
      return 1;
    }
  }
  return 0;
}

While this is most absolutely not correct, this will give the right output in most cases I can think of in my pre-caffeinated Tuesday at 9:00 AM brain. I originally used new Array(v2.length - v1.length).fill(0), but realized Google Script isn't ECMAScript 6 yet. That can be massively simplified in the future.

Filling with 0 may also not be right. Maybe -1? If you get "out of bounds" it ought to have the current behavior, I think.

@alex

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Thanks for reporting this! Does the beta show up in the version string at all, or does it simply present as 13.1?

@paultag

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

Thanks for the quick reply! Just 13.1. Here's a lightly scrubed version of the pre-patch output email:

Found 27 mobile devices.
Outdated mobile devices owned by:
- user@domain.tld (12.3.1)
- user@domain.tld (12.4)
- user@domain.tld (12.3)
- user@domain.tld (12.4)
- user@domain.tld (12.4)
- user@domain.tld (12.4)
- user@domain.tld (12.4)
- user@domain.tld (13.1)
- user@domain.tld (12.4)
- user@domain.tld (13.1)
- user@domain.tld (13.1)
- user@domain.tld (12.4)
- user@domain.tld (13.1)
- user@domain.tld (13.0)
- user@domain.tld (13.1)
- user@domain.tld (12.4)
- user@domain.tld (13.1)

Found 0 ChromeOS devices.
No outdated ChromeOS devices
@alex

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Ok. I think the simplest fix it to something like (untested):

function compareOSVersions(v1, v2) {
  for (var i = 0; i < Math.min(v1.length, v2.length); i++) {
    if (v1[i] < v2[i]) {
      return -1;
    } else if (v1[i] > v2[i]) {
      return 1;
    }
  }

  if (v1.length != v2.length) {
    return -1;
  }

  return 0;
}

Do you mind testing to see if that works for you (I don't have any users on iOS betas, which complicates testing this!)? If you can confirm it works, I'll land it!

@paultag

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

I can confirm that works!

@alex alex closed this in a535d00 Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.