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

136 - Dialog with "prominent" disclosure to request location #142

Merged
merged 25 commits into from
Jan 12, 2021

Conversation

mrsarm
Copy link
Contributor

@mrsarm mrsarm commented Dec 17, 2020

Dialog with the "prominent" disclosure to request location to the user.

Issue: #136

The dialog is rendered by the app instead of the CHT (to avoid users to update the CHT). It works in Webview and XWalk. Here is how it looks now in Android 10:

CHT Location Permission Dialog

To discuss and complete:

  • In the preview above you can see that a native dialog is used, instead of a nice activity with a custom made layout. The dialog does not allow to add images or HTML markup as long I could see, only an icon, a title, a message, and the labels for the accept / deny buttons. So I can try to replace later the dialog builder with a native Activity / layout , following the design proposed in the issue. For now I just wanted to provide something native that works in both XWalk and Webview, and maybe in a second iteration with can improve the dialog. → Activity / layout now replace the dialog following the sketch from the mockups.
  • We need the exact message to display. Does the usage of the location data change between partners? if not, only one message should be provided and then translated to different languages.
  • Once the English message is provided, we need the translations. Because this is a "native" Android solution, we will use the i18n provided by Android, not the translations provided by the CHT. Also we need the right translations for the button labes: "No thanks" and "Turn on". → Main "generic" translations provided. Possible translations from partners can be included in next PRs.
  • For now the message says "This app..." , but we should replace that with the name of the app, right? I'll check whether I can get the name of the app dynamically from the Android API or from the app configurations.
  • When the user denied the access to the location data from the dialog, the decision is recorded in the app (not the CHT) to avoid request the same permission next time. When the user accepts the dialog, the native Android dialog is shown asking again the location permission, in this case the decision is recorded by Android, but the result is the same: the decision is recorded so next time the location is acceded by the app without asking first.
  • Added in the PR the x86 platform to be able to test the changes with virtual devices with Android 10, not sure if it can cause issues with the CI pipeline → Added but commented to not affect the CI process.

CC @craig-landry @garethbowen @michaelkohn

@mrsarm mrsarm marked this pull request as draft December 17, 2020 15:38
@abbyad
Copy link
Contributor

abbyad commented Dec 17, 2020

Nice to see the in progress version!

For localization and getting the name of the app, you can use this technique, and add the program name programmatically. You'll see that it is often used in Collect (search for %s or %1). You'll also see in Collect how the app_name is obtained programmatically.

@mrsarm
Copy link
Contributor Author

mrsarm commented Dec 17, 2020

Found a bug that makes the form to not work when the user has denied the access location in the dialog built by the app instead of the native Android dialog. Apparently is because the CHT app still try to get the location data, so I have to find the way to avoid that without tweaking the CHT, maybe calling some JS code within the android app...

@mrsarm
Copy link
Contributor Author

mrsarm commented Dec 17, 2020

Found a bug that makes the form to not work when the user has denied the access location in the dialog built by the app instead of the native Android dialog. Apparently is because the CHT app still try to get the location data, so I have to find the way to avoid that without tweaking the CHT, maybe calling some JS code within the android app...

Found the solution (calling a JS function as I suspected). Works in both XWalk and Webview, so working in the patch, but doing some refactor to avoid anti-DRY pattern between XWalk and Webview code.

if(!ed.commit()) throw new SettingsException(
"Failed to save 'denied-geolocation' to SharedPreferences.");
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should store this in the settings. I think this would be very difficult to clear if the user has changed their mind - we'd either have to add a UI to do it or wipe all app data.

Android already remembers this information and provides a UI for changing the permissions. Can we instead check that somehow and only show the dialog if it's not already set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is something discussed in the ticket, in the current master version if you regret to allow / deny the locations, you have the same problem, but it is true that it is more convenient just to remove the permission or the rejection from the app info in the Android settings than wipe all the data in order to clean this setting, but either case the user needs to go to the Android settings to do so.

Can we instead check that somehow and only show the dialog if it's not already set?

It works like that, first it checks whether the user already allowed / denied the locations permissions, and only shows the dialog if the user never did. Users that already has the app installed and already allowed the app to access the location won't see the new dialog when upgrade to this version, and the app won't store any configuration.

Anyway, I'll check later if there is a way that in case the user rejects the permission request from our dialog, store the rejection in the same permissions settings that Android uses for that purpose, but because it is a rejection, not sure whether is possible.

Copy link
Contributor Author

@mrsarm mrsarm Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back to this, I pushed a fix because a bug in the PR that was causing the form to be blocked when loc permissions were denied, and now testing again, turns out the user does not need to wipe the app data to rollback its decision to allow or deny the access to the location, it only needs to do the same that it does with the current master version: go to the app settings in the android settings and just add again or revoke the location permission, but no need to wipe the app data because the variable stored here is read after check the permissions against the Android API.

The only difference is that in case the user rejected the location permissions from the custom dialog instead of from the Android dialog, in the app settings you won't see the location permission accepted or rejected, the permission will be displayed like it was never accepted or rejected, and from there the user will be able to add the permission to access location in which case this variable stored will be ignored by the app.

Still will check later whether I can ask Android to store the negative of location access using the API like it was rejected from the native location dialog, but may be not possible.

build.gradle Outdated Show resolved Hide resolved
@garethbowen
Copy link
Member

We need the exact message to display. Does the usage of the location data change between partners? if not, only one message should be provided and then translated to different languages.

There are some clues in this google support page, however it seems to be focused on background location permissions which doesn't apply here. I'd go with something like...

Title: "Use your location"
Body: "{this app} collects location data when you submit a form to analyze and improve health outcomes in your area."
Ok button: "Allow"
Deny button: "Deny"

It's terribly generic, but it's really up to each project to get more specific if they want. @n-orlowski Do you have any recommendation on the wording here?

Once the English message is provided, we need the translations.

We have this issue to provide an i18n solution. Don't worry about it for now - each project can translate the strings.xml for the primary language for their project.

For now the message says "This app..." , but we should replace that with the name of the app, right? I'll check whether I can get the name of the app dynamically from the Android API or from the app configurations.

Hopefully this can be automated like Marc suggests, otherwise just stick with "This app" and again, projects can override as needed.

@n-orlowski
Copy link

n-orlowski commented Dec 17, 2020

@garethbowen I think we should try to be as crystal clear as possible with copy and give users a heads up on the next screen:

Title: "Location Access"
Body: "{This app} collects location data when you submit a form to analyze and improve health outcomes in your area. Select "Allow while using the app" on the next screen to turn on your location access.
Ok button: "OK"
Deny button: "No thanks"

@mrsarm mrsarm force-pushed the 136-request-location-disclosure branch from 86338b2 to aca0207 Compare December 17, 2020 23:04
@mrsarm mrsarm force-pushed the 136-request-location-disclosure branch from 5bdabf6 to 4ee7d20 Compare December 22, 2020 01:32
@mrsarm mrsarm force-pushed the 136-request-location-disclosure branch from 4ee7d20 to e7044a1 Compare December 22, 2020 01:52
@mrsarm
Copy link
Contributor Author

mrsarm commented Dec 22, 2020

@garethbowen I have replaced the dialog by an activity / layout following the mockup provided in the issue (I updated the recording in the description). I couldn't find a way to avoid the rejection storage in the shared preferences, but as mentioned, the user can go to the app preferences in the Android settings and add the permission like it would been rejected from the Android dialog and the geolocation will work again.

@craig-landry don't worry about what I said in the meeting about calling methods of an activity from other activities, I was using the intents the wrong way, but I did a refactor and I think now is fine (without doing that).

@mrsarm mrsarm marked this pull request as ready for review December 22, 2020 02:06
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! I've left a few minor comments inline...

src/main/res/layout/request_permission.xml Outdated Show resolved Hide resolved
<string name="locRequest_message">%s collects location data when you submit a form to analyze and improve health outcomes in your area. Select \"Allow while using the app\" on the next screen to turn on your location access.</string>
<string name="locRequest_okButton">Turn on</string>
<string name="locRequest_denyButton">No thanks</string>
<string name="locRequest_IconDescription">My location icon</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: whitespace.

src/main/res/values/strings.xml Outdated Show resolved Hide resolved
src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@@ -51,6 +51,10 @@

private final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)Math.random();

private static final int DISCLOSURE_LOCATION_PERMISSION_REQUEST = 1122331;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be the same int as the one above. It shouldn't be a problem but if both classes exist then the wrong one may catch the response. I think it should be random, like the ACCESS_FINE_LOCATION_PERMISSION_REQUEST above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that the constant above that is assigned with a random number and looking in the internet why we would use a random number in a constant I couldn't find a good reason, but yes random numbers are used in requestCode variables when you need a new value each time, but this is not the case, the variable is only generated one time (final), so why we are doing that?

Constant are constants, and using a random value at startup time looks like a bad idea specially in Java, because you cannot use them in switch statements or annotations if the value set is assigned dynamically. Also there is the risk (unlikely) for collisions is two constants defined like that are caught in the onActivityResult(..) method that catch most events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking around the code again, I found that while in the Webview version we define the constant randomly like this:

final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000);

In the XWalk version is:

final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)Math.random();

Math.random() always return a double between 0 and 1 but never 1, so casting the result to int always get 0. I guess this is a bug (but not resulting in errors for now) and not intended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely a bug - I should have fixed that when I fixed the Webview version, sorry!

I think the reason it's random is so it's different every time the app starts so you don't get a random response from a previous request. I'm happy to go with whatever the best practice is, if you can find one!

It seems like a strange API for someone used to callbacks...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 didn't know that the app can restart and still receive a response from a request from a previous process, in that case makes sense. Next event variable caught by the same method can be defined using different random ranges to avoid collisions, eg:

jshell> (int)(Math.random() * 1000)
$9 ==> 180

jshell> (int)(Math.random() * 1000)
$10 ==> 739

jshell> (int)(Math.random() * 1000) + 1000    // different range
$11 ==> 1819

jshell> (int)(Math.random() * 1000) + 1000
$12 ==> 1679

jshell>

So going to make it random and fix the one with the random that is always 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed using random values, and tested in XWalk and Webview


private final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000);
private static final int DISCLOSURE_LOCATION_PERMISSION_REQUEST = 1122331;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be random as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes same than above, I'll change it but I would like to understand what we do that.

mrsarm and others added 5 commits December 22, 2020 10:38
Editing a form for second time after reject the location access from the disclosure view instead of the Android dialog caused the form to freeze trying to access the location from JS
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work and a really impressive turn around time.

Feel free to make further tweaks as needed. I've pre-approved this so you're not blocked.

@mrsarm
Copy link
Contributor Author

mrsarm commented Dec 22, 2020

@@ -45,4 +45,11 @@
<string name="promptChooseImage">Choose image</string>

<string name="spnCompressingImage">Compressing image…</string>

<string name="locRequestTitle">Location access</string>
<string name="locRequestMessage">%s collects location data when you submit a form to analyze and improve health outcomes in your area. Select \"Allow only while using the app\" on the next screen to turn on your location access.</string>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user is not necesarily aware of what consists and doesn't consist of a form, so it would be clearer to avoid mentioning forms here.

Suggested change
<string name="locRequestMessage">%s collects location data when you submit a form to analyze and improve health outcomes in your area. Select \"Allow only while using the app\" on the next screen to turn on your location access.</string>
<string name="locRequestMessage">%s collects location data to analyze and improve health outcomes in your area. Select \"Allow only while using the app\" on the next screen to turn on your location access.</string>

Copy link
Contributor

@abbyad abbyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Minor comments and suggestions for the release notes, but should not be blocking as the PR is pre-approved.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@abbyad abbyad self-requested a review January 6, 2021 21:06
Copy link
Contributor

@abbyad abbyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it back, "request changes" did in fact block despite the other approval.

Comment on lines 47 to 53
private static final int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000);

private final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000);
// Use different ranges of random values for other request codes caught by the same method,
// eg. (int)(Math.random() * 1000) + 1000
private static final int DISCLOSURE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000);

private static final String[] LOCATION_PERMISSIONS = { Manifest.permission.ACCESS_FINE_LOCATION };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Math.random() here worries me. These codes should be fixed. As it is now, this number will be random (at class init time) between 0 and 999 (inclusive). There is a chance that this will match down on line 176 and go into the photo branch and not end up in the new permissions branch (line 190) as desired.

From our testing I think we've seen some very rare cases where the secondary permissions modal did not show, which would make sense since that is triggered in that new conditional branch, which wouldn't get hit if we got unlucky with the randomization. Also I know it's been reported that when the app is restarted (versus staying running when switching "windows" in Android) that has led to differing behavior, which would also line up with this randomization changing at class loading time.

This new constant needs to be a fixed integer, but we also need to do the same for the other random constant here to avoid that overlapping with our newly selected constant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put together #146 to address the result code collisions.

@eaaliprantis
Copy link

eaaliprantis commented Jan 8, 2021 via email

@craig-landry
Copy link
Member

Hi. I have a suggestion to make. For dealing with permissions, the first thing that when the app opens is to request all of the permissions right away. I’ve published apps before to do that. And if you do not have that done right away, you don’t get approved in the App Store. Just food for thought

Sent from my iPhone
On Jan 8, 2021, at 10:56 AM, Craig Landry @.***> wrote:  @craig-landry requested changes on this pull request. In src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java: > + private static final int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000); - private final static int ACCESS_FINE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000); + // Use different ranges of random values for other request codes caught by the same method, + // eg. (int)(Math.random() * 1000) + 1000 + private static final int DISCLOSURE_LOCATION_PERMISSION_REQUEST = (int)(Math.random() * 1000); + + private static final String[] LOCATION_PERMISSIONS = { Manifest.permission.ACCESS_FINE_LOCATION }; The Math.random() here worries me. These codes should be fixed. As it is now, this number will be random (at class init time) between 0 and 999 (inclusive). There is a chance that this will match down on line 176 and go into the photo branch and not end up in the new permissions branch (line 190) as desired. From our testing I think we've seen some very rare cases where the secondary permissions modal did not show, which would make sense since that is triggered in that new conditional branch, which wouldn't get hit if we got unlucky with the randomization. Also I know it's been reported that when the app is restarted (versus staying running when switching "windows" in Android) that has led to differing behavior, which would also line up with this randomization changing at class loading time. This new constant needs to be a fixed integer, but we also need to do the same for the other random constant here to avoid that overlapping with our newly selected constant. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

Thanks for your thoughts on this @eaaliprantis but that won't work for us here. We are purposefully taking a different approach here as Google requires the prompt to happen in the context of the GPS action taking place. It would definitely be easier to ask for everything up front, but we are not permitted to handle it that way for this app.

@eaaliprantis
Copy link

eaaliprantis commented Jan 8, 2021 via email

@abbyad
Copy link
Contributor

abbyad commented Jan 8, 2021

Interesting that Google changes their permission based on GPS actions instead of being done initially at the beginning

Ya, it seems to be part of an effort to avoid abuses of privacy, and for users to be aware when and why permissions are needed. The current recommendation is to "ask for permissions in context, when the user starts to interact with the feature that requires it."

* Avoid result code collisions

The new code to handle the location permissions risked colliding with the scheme used for handling simprints result codes. This change sets the new result code to a constant in the reserved space.

The prior version also used random numbers, which is removed as a side effect of this specific code selection.
@garethbowen garethbowen merged commit a4c31ab into master Jan 12, 2021
@garethbowen garethbowen deleted the 136-request-location-disclosure branch January 12, 2021 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants