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

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

Merged
merged 5 commits into from
Aug 28, 2023

Conversation

latin-panda
Copy link
Contributor

@latin-panda latin-panda commented Aug 23, 2023

Description

This is to upgrade from Android 11 to Android 13.

  • READ_EXTERNAL_STORAGE is ignored. Therefore, we stopped requesting this permission for Android +13 with the prominent disclosure prompt. At the same time, we will keep asking this permission for any device in previous versions of Android (12, 11, 10, etc).
  • We limited the SDK target on the following tests because our current test dependencies don't support target 33
    • SmsSenderTest.java
    • FilterableListAdapterTest.java
    • AsyncExecutorTest.java
    • VibratorTest.java

#324

@latin-panda latin-panda changed the title chore(#324): testing android 13 chore(#324): upgrading to android 13 Aug 23, 2023
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
trace(this, "RequestStoragePermissionActivity :: READ_EXTERNAL_STORAGE permission is ignored in Android 13+.");
setResult(RESULT_OK, createResponseIntent());
finish();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case if someone is wondering

You can call finish() from within this function, in which case onDestroy() will be immediately called after onCreate(Bundle)

source

@latin-panda latin-panda marked this pull request as ready for review August 24, 2023 13:39
@@ -46,6 +46,7 @@
import java.util.stream.IntStream;

@RunWith(RobolectricTestRunner.class)
@Config(sdk = 31) // ToDo: Remove when upgrading robolectric
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're getting this error because our current version of robolectric doesn't support target 33

java.lang.IllegalArgumentException at RobolectricTestRunner.java:260
        Caused by: java.lang.IllegalArgumentException at DefaultSdkPicker.java:118

In this link, they recommend to use Config(sdk = 31) to make the test pass as a temporary fix.

@latin-panda
Copy link
Contributor Author

@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
Copy link
Contributor

@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!

Copy link
Contributor

@ngaruko ngaruko left a comment

Choose a reason for hiding this comment

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

  1. Test most critical scenarios and they all passed
  2. Random testing of a few workflows
  3. Reading from Google developer, one of the major changes around permissions is replacing [READ_EXTERNAL_STORAGE](https://developer.android.com/reference/android/Manifest.permission#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 ?

@latin-panda
Copy link
Contributor Author

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:

  • Did you try the feature of sending SMS when submitting forms? I need help getting the form configured for that. There's a known issue, but we just don't want to make it worse

@latin-panda
Copy link
Contributor Author

latin-panda commented Aug 25, 2023

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 READ_MEDIA_* 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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@ngaruko
Copy link
Contributor

ngaruko commented Aug 28, 2023

@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.

Copy link
Contributor

@ngaruko ngaruko left a comment

Choose a reason for hiding this comment

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

Good research on those permissions. 👍

@latin-panda latin-panda merged commit 948a455 into master Aug 28, 2023
@latin-panda latin-panda deleted the 324_update_target_API_to_latest branch August 28, 2023 04:25
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.

3 participants