In Android 6.0 adds a location permission request dialog for test apps

[juhanak]

[msftclas]

Hi @juhanak, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

[vjrantal]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

test/www/js/thali_main.js, line 38 [r1] (raw file):
Is the status argument meaningful? Should it be at least logged so that in our tests (for example in CI) we could debug what has been returned?

---

Comments from the review on Reviewable.io

[juhanak]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

test/www/js/thali_main.js, line 38 [r1] (raw file):
When request is successful, it always returns a string PERMISSION_GRANTED. Should I add it to logs?

---

Comments from the review on Reviewable.io

[vjrantal]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

test/www/js/thali_main.js, line 38 [r1] (raw file):
Or, should we get rid of that argument entirely from the API if we don't expect we or someone else has any use for it?

---

Comments from the review on Reviewable.io

[juhanak]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

test/www/js/thali_main.js, line 38 [r1] (raw file):
Yes, it makes sense to get rid of that argument. I'll do that change.

---

Comments from the review on Reviewable.io

[vjrantal]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

test/www/js/thali_main.js, line 42 [r2] (raw file):
Sorry to bother you with one more comment, but I guess the error can be at least two different things? Should that be logged here?

---

Comments from the review on Reviewable.io

[juhanak]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

test/www/js/thali_main.js, line 42 [r2] (raw file):
Yes, I'll add that logging.

---

Comments from the review on Reviewable.io

[vjrantal]

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[yaronyg]

I realize this code review is finished but there are some pretty serious issues here that I'm surprised were missed. I would ask that both @vjrantal and @juhanak take a look at my comments below and tat @juhanak please fix.

Also, where is the test to make sure this all works?

---

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

www/android/thaliPermissions.js, line 1 [r3] (raw file):
This file does not pass JSCS/JSHINT validation. Please fix.

---

www/android/thaliPermissions.js, line 29 [r3] (raw file):
We are defining a class here (which I don't think we should do, btw, but see my comments below on that). To do that properly we could stick in a jsdoc /** at the start which would then include a @classdesc variable to describe what the class is doing followed by the params followed by @constructor with anything specific to the constructor. You can look at any of the modern NextGeneration files for examples.

---

www/android/thaliPermissions.js, line 33 [r3] (raw file):
Why would we define a function inside of a parenthesis anyway? That is only needed if we were going to define and immediately call the function with arguments but we aren't so please lose that parenthesis. E.g. it should be function() not (function()

---

www/android/thaliPermissions.js, line 42 [r3] (raw file):
This should be defined as an @enum. See the jsdoc site for details.

---

www/android/thaliPermissions.js, line 51 [r3] (raw file):
There is no such thing as 'function'. There is @callback though. Please create the @callback and then refer to the @callback definition in the param types.

---

www/android/thaliPermissions.js, line 54 [r3] (raw file):
Why do we need a class here at all? It seems to me that what we really want is to define two static values. There would be module.exports.responseCodes and module.exports.requestLocationPermission.

---

Comments from the review on Reviewable.io

[vjrantal]

All great and valid comments that I bet @juhanak fixes.

As a side note, "serious issues" might feel a bit extreme wording since these were all non-functional and more about coding convention and docs - still - they could have been caught during the review so I'll take the bullet on that.

As a code reviewer, I did not mandate an automated test since we didn't have a CI with Marshmallow devices and I did not want to block this commit until we do have that. I now created an issue (#547) so that we remember to do that once possible (I should have created this issue already back when I accepted the code review).

---

Comments from the review on Reviewable.io

[yaronyg]

My point was just that helping folks new to Node learn how to write proper node is a big deal and code reviews are the time to do it. So when we see code that is functionally correct but not idiomatic we need to point it out so the folks can learn how to 'node'.

In regards to tests, we should still require the tests and until CI is back we expect them to be run locally. Not ideal but better than not having tests. My bigger concern is if this functionality really can be tested in CI since I think it involves a user dialog.

---

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks broke.

---

Comments from the review on Reviewable.io