Prominent disclosure when "handling users' Files"
Closed this issue · 17 comments
Follow-up after #136
Apps are getting denied from the app store because we are requesting READ_EXTERNAL_STORAGE without a prominent alert asking for access. See screenshot for details form google.
Created the PR#150 where the READ_EXTERNAL_STORAGE permission is removed from selected flavors.
Tomorrow I'll check the necessary changes to implement at runtime the prominent disclosure requested by Google, but it's possible the move may require changes in the way the CHT requires access to the file storage (though thinking it well, it shouldn't, because we are not accessing any JS API to request storage access, it is required when the user clicks in an upload field 🤔 )
So, we have 2 alternatives:
- The changes in the Android App requires changes in the CHT: in this case, we need to release a new version of the CHT, and upgrade all the deployments currently have the app published in the Play Store. In this case, the PR mentioned above allows us to skip the CHT upgrade for those deployments that really don't use upload fields, therefore don't need the storage location in the app.
- The changes in the Android App DON'T require changes in the CHT: we can work in a new app release without the need to change anything in the CHT, but still we will need to publish a new version in the Play Store with the changes, and they aren't backward compatible with older versions of the App, specifically v0.5.0 where we change the way we request location access.
Regardless of the choice, we can still keep the changes from PR#150 and remove the storage permission at all for those flavors, making the Play Store approval process simpler, but we will need to keep in sync what flavors do access to the local storage to avoid the app to crash. In this case we can also add a change using the Android API to check whether the app has the permission in the manifest and avoid the app crashing and letting the user know about the error, despite it shouldn't happen.
Checking in with existing deployments, I asked @derickl:
are any deployments using file uploads?
To which he replied:
None that I know of
@derickl - I'm going to assume it's OK if we remove the feature as needed for this ticket. We can re-introduce the feature as needed at a later date which can then be play store compliant.
OK, here's the latest from @kennsippell when asked by @derickl if the upload feature was OK to remove:
i also cannot recall apps using file upload, but it doesn't seem particularly bulletproof to rely on the memory of a few select people to understand cht feature adoption.
this scraper is the only window i know of it analyse configurations and feature adoption across projects. it could be adapted to answer this question with higher confidence. ultimately this is a decision owned by the product team - removing file upload for a few versions doesn't seem unreasonable as long as the breaking change is signalled clearly through semver
@MaxDiz and @garethbowen - I think you two have been on point in the past about communicating medic-android changes like this
Adding to v3.11 to include as part of the next android release. Based on the roadmap planning discussion, we want to maintain the ability to access local storage to align with x-forms functionality and long-term direction for the product.
@mrsarm please follow-up with @craig-landry regarding best pathway forward
Hi @mrsarm . I see that for flavours that had the access storage permission, there is an additional attribute tools:node="remove" in that flavour's manifest. However, the main manifest still has the original permission enabled (https://github.com/medic/medic-android/blob/0f4385e1f1a76338b734577de06cf8f4e6d51ec4/src/main/AndroidManifest.xml#L24)
Shouldn't we "remove" this as well, just in case new app builder tries to branch their flavour off this?
Version of the app v0.7.3 (not released yet) is going to include the removal of the storage permission for most flavors that are not using the permission (pull/150).
The plan for the next major release (v.0.8.0?) is to add the perm again, adding the prominent disclosure requested by the Play Store, so I'll keep this ticket open for follow-up.
Thanks @mrsarm . Since this was not a functional change, QA will do a quick sanity check on the newly created unbranded-test flavour for basic functionality....and basic app loading (at least up to login screen) for the newly added flavours, ie safaridoctors_kenya, trippleeighty and icm_ph_chc.
-
Installing APKs
Side loading selected APKs, including newly created ones (safaridoctors_kenya, trippleeighty and icm_ph_chc)... from GH releases -
==> Installed with no errors...
2. Open the app (just the login page as flavoured apks point to production servers for most part)
==> All apps tested open without crashing - except Living Goods (Innovation Network) which points to a server that has been shut down
Going forward, I suggest we remove it or point it to the dev instance currently used for internal testing. New issue created to investigate inactive servers and flavours.
==> Also 'CHAMP', the app for ICM PH, does not have all languages required and no translation for Tagalog on the login page. Obviously this is a server-side issue and I assume @binokaryg is still working with the partner with regard to translations ( and upgrading their instance to latest medic version that has the additional languages and translations)?
3 . Other features tested
a) Private policy translations when the user logs in with a preferred language
b) Location access disclosure when the system language is Tagalog
c) Reports are saved with or without location data depending on whether the user accepted or not
d) MRDT app
e) Geopoint
f) basic functions and workflows
Move the ticket to the 4.0.0 board.
We disabled the access to files from the app for all the active flavors (#150), so the problem of not being delisted from Google Play is solved, and due this bug #127 we need first to re-implement the file uploader picker #159, that is scheduled for the 4.0.0 release.
More about prominent disclosure process from Android.
Beginning from 1 April 2022, all apps must also post a privacy policy in the Google Play Console and within the app itself.
@garethbowen should we add the privacy policy as part of the release process for Android apps? I couldn't find it
This is probably the closest thing we've got: https://docs.communityhealthtoolkit.org/core/guides/android/publishing/#new-app-in-the-play-store
There are heaps of requirements for listing on Google Play (see this internal guide for how Medic App Services do it) and I don't want to try and duplicate them all in the docs site because it'll quickly get out of date. Fortunately the Play Store walks you through the steps - if you create a new app it has a section on Privacy Policies which you must fill out before your app will show.
We could add a sentence or two to the publishing page about following the Play Store steps potentially with a link to an Google resource on how to do it?
This is ready for AT in this PR.
The PR includes the following:
-
Adds a prominent disclosure for when requesting storage permission.
- The disclosure is displayed when using a file picker field or the Android App Launcher widget in a form.
- To test the Android App Launcher, you can use the covid-19 config. Install the RDToolkit app. Fill out the provision form.
-
At some point we can't request the grant automatically, so the user is redirected to the app's settings to manually grant the permission. This happens on the following escenarios and not all the phones behave the same, for example Android 5 grants anything we ask.
- "never ask again" option. For example my Samsung Android 12 doesn't have this option anymore, but some older phones still have it.
- Permission is denied for second time.
- Permission is denied from the App's Settings page.
-
Updates location prominent disclosure to use intent's callback contracts. The prominent disclosure is displayed when opening a form that requires Location.
- Currently, when the user denies the permission, we save that decision in the SettingsStore to prevent asking again, it isn't a very good practice since we're not considering the case when the user rethinks the decision and wants to grant it later. I commented this code to test this escenario 😅.
- We will remove the SettingsStore flag in this ticket where we'll work further in Location.
See screenshots in the PR's description.
I have been getting some strange behaviour when I press "Launch App". The app goes to app info instead of opening the 3rd party app.
Testing with an android 10 phone. Did you want to have a second look @latin-panda ? or ping if you need more logs.
148.mov
2 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/MedicMobile: org.medicmobile.webapp.mobile.RequestStoragePermissionActivity :: RequestStoragePermissionActivity :: User agree with prominent disclosure message.
2022-04-04 20:20:32.128 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: MSG_WINDOW_FOCUS_CHANGED 0 1
2022-04-04 20:20:32.128 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: prepareNavigationBarInfo() DecorView@99289b6[RequestStoragePermissionActivity]
2022-04-04 20:20:32.128 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: getNavigationBarColor() -855310
2022-04-04 20:20:32.159 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/MedicMobile: org.medicmobile.webapp.mobile.RequestStoragePermissionActivity :: RequestStoragePermissionActivity :: User rejected storage permission twice or has selected "never ask again". Sending user to the app's setting to manually grant the permission.
2022-04-04 20:20:32.183 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: stopped(false) old=false
2022-04-04 20:20:32.191 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: Relayout returned: old=(0,0,1080,2220) new=(0,0,1080,2220) req=(1080,2220)0 dur=6 res=0x1 s={true 507534827520} ch=false
2022-04-04 20:20:32.201 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: MSG_WINDOW_FOCUS_CHANGED 1 1
2022-04-04 20:20:32.201 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: prepareNavigationBarInfo() DecorView@99289b6[RequestStoragePermissionActivity]
2022-04-04 20:20:32.201 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: getNavigationBarColor() -855310
2022-04-04 20:20:32.219 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: MSG_WINDOW_FOCUS_CHANGED 0 1
2022-04-04 20:20:32.219 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: prepareNavigationBarInfo() DecorView@99289b6[RequestStoragePermissionActivity]
2022-04-04 20:20:32.219 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputMethodManager: getNavigationBarColor() -855310
2022-04-04 20:20:33.040 10090-10090/org.medicmobile.webapp.mobile.unbranded_test D/InputTransport: Input channel destroyed: 'ClientS', fd=194
2022-04-04 20:20:33.370 10090-20261/org.medicmobile.webapp.mobile.unbranded_test I/mali_egl: eglDestroySurface() in
2022-04-04 20:20:33.373 10090-20261/org.medicmobile.webapp.mobile.unbranded_test I/mali_winsys: delete_surface() [1080x2220] return
2022-04-04 20:20:33.373 10090-20261/org.medicmobile.webapp.mobile.unbranded_test I/mali_egl: eglDestroySurface() out
2022-04-04 20:20:33.374 10090-20261/org.medicmobile.webapp.mobile.unbranded_test W/libEGL: EGLNativeWindowType 0x7635f86650 disconnect failed
2022-04-04 20:20:33.380 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: Relayout returned: old=(0,0,1080,2220) new=(0,0,1080,2220) req=(1080,2220)8 dur=5 res=0x5 s={false 0} ch=true
2022-04-04 20:20:33.381 10090-10090/org.medicmobile.webapp.mobile.unbranded_test I/ViewRootImpl@a791b16[RequestStoragePermissionActivity]: stopped(true) old=false
Redirecting to app's settings page is okay for the scenario #2.
There might be an issue in Android 10, I'm going to see what's the problem later today.
I found the issue @latin-panda . Last year the storage permission was removed and Mariano created a new flavour (unbranded-test) with the permission removed. That flavour has a separate manifest.
I think now that we are enabling storage permission again, I think we need to remove that to avoid confusion .
Other than that the feature works.
storage.mov
Good to merge @latin-panda '. New ticket created to remove the flavour.