medic / cht-android

A native Android container for Community Health Toolkit (CHT) applications
GNU Affero General Public License v3.0
25 stars 49 forks source link

chore(#324): upgrading to android 13 #325

Closed latin-panda closed 1 year ago

latin-panda commented 1 year ago

Description

This is to upgrade from Android 11 to Android 13.

324

latin-panda commented 1 year ago

@garethbowen, it turns out that our version of robolectric doesn't fully support target 33. I tried upgrading the library but it has deprecations that we need to fix and it requires upgrading androidx libraries, which also have more deprecations that we need to figure out. It's going to take longer to resolve them.

Knowing that we have limited time before PlayStore blocks our apps, I've dropped the work on the tests and patch them by running them in target 31. I created a separate ticket to fix the dependencies. Then @ngaruko and I can focus on testing.

My question is:

If the manual testing is good, can we release this to unblock the PlayStore and work on the dependencies later?

garethbowen commented 1 year ago

@latin-panda Yes let's go manual to unblock this - incomplete testing is better than blocking publishing altogether. But let's make sure we fix this soon - if you don't have time to take this on let me know and I'll shop it around.

Thanks for doing such a deep dive here!

latin-panda commented 1 year ago

Reading from Google developer, one of the major changes around permissions is replacing READ_EXTERNAL_STORAGE by granular media permission. Apparently we removed the permission and don't seem to have introduced any granular permission. Is that intentional @latin-panda ?

@ngaruko I saw that, but I'm not clear under what circumstances it's needed; for example, they don't mention anything about the other types of files that aren't images, video, or audio, such as pdf or spreadsheets, and that we were accessing using READ_EXTERNAL_STORAGE (sample form). Some blogs say that since it's in a shared place then it's not needed. When I tested it, it worked on my phone even when not requesting any of those permissions.

In another topic:

latin-panda commented 1 year ago

Continuing with @ngaruko's point # 3...

I did a lot of reading to understand why we can access files in Android 13 without requesting granular permission for media. This is what I found as possible reasons:

Reason # 1: The app was upgraded where it had previously granted access.

If your app was previously granted the READ_EXTERNAL_STORAGE permission, then any requested READMEDIA* permissions are granted automatically when upgrading.

Source (last paragraph of that section)

Test: I did a fresh installation, so the app has no permission assigned.

Result: I can still access the files using the picker from an Eketo form.

Reason # 2: We've selected files in our app's folder or the download folder

On devices that run Android 10 or higher, you don't need storage-related permissions to access and modify media files that your app owns, including files in the MediaStore.Downloads collection.

Source

Test: I tried picking files not in the download or cht folders.

Result: I can still access the files using the picker from an Eketo form.

Reason # 3: We're using the system's default picker

Source I remember following the Storage Access Framework when remaking the file picker. We use ACTION_GET_CONTENT to trigger the system default picker.

Test: I tried picking files made by other apps (WhatsApp, Google Docs, etc.)

Result: I can still access the files using the picker from an Eketo form.

If I'm interpreting this correctly, the main reason why we can still pick files without having granular permissions is that we are using the system's default picker. So, the user has control over file privacy.

Why just not ask for these permissions

Reading the Android docs, they don't recommend asking for permissions that the app isn't using. In our experience, asking for permission makes things more complex with Playstore. They are quite picky and it turns out like asking for access to review the app, asking for clear terms and conditions for app usage, etc. I suggest not adding these permissions if we don't need them.

@garethbowen @ngaruko I did a lot of testing on this storage permission topic and didn't spot any issue accessing images, audio, videos and other files like pdf.
Please let me know if you have any recommendations or would like to proceed differently. My experience with Android development is limited. Otherwise, by Tuesday at the latest, I'll assume this is good as it is today :)

garethbowen commented 1 year ago

@latin-panda I don't have a lot of android experience either but I think your "reason 3" is spot on. Thanks for being so diligent!

latin-panda commented 1 year ago

@ngaruko waiting for your approval or feedback after you've done verifying the app

ngaruko commented 1 year ago

@latin-panda . Thanks for all the research. It all makes sense and practically, I do not see any scenario where access is denied due to the permission not present. So, good to go.