Android 13 update #2683

avazirna · 2023-07-25T12:52:30Z

Summary

This PR implements the changes required to support Android 13. The most relevant are:

Nearby Wi-Fi devices permission - from Android 13, access to devices via Wi-Fi are no longer through the ACCESS_FINE_LOCATION permission. Apps that call any of these Wi-Fi APIs need to request the NEARBY_WIFI_PERMISSION;
POST_NOTIFICATION permission - Google is invested in making sure users' wishes around notifications are respected, this means that users can now very easily revoke permissions around notifications. This motivated changes in places where notifications are triggered to skip them when the permission is not granted;
Stop Foreground services - this feature causes the system to remove the app from memory, which makes the app to stop, not just the Foreground service. CommCare seems to handle this elegantly, so no changes were done. The video below shows the behavior:

Screen.Recording.2023-09-13.at.01.11.48.mov

Safety Assurance

I have confidence that this PR will not introduce a regression for the reasons below

Safety story

I had the opportunity to run some smoke tests around the affected parts of the code and it ran well, however, some Instrumentation tests became flakier than usual, I think it's something to look into.
A few questions for reviewers:

Are there any concerns with the user stopping the Foreground Service, and subsequently CommCare, that we should account for?
Regarding the approach when the POST_NOTIFICATION permission is revoked, it seems to me that the approach should be around preventing notifications from being triggered but I wonder if there are other options we should explore.
Android 13 introduces Predictive back gesture and recommends its adoption as soon as possible, it seems fine to me to migrate later but happy to be persuaded otherwise. Any comments?

cross-request: dimagi/commcare-core#1329

QA Note: Testing for Android 13

avazirna · 2023-09-12T14:28:47Z

@damagatchi retest this please

avazirna · 2023-09-12T22:54:34Z

@damagatchi retest this please

avazirna · 2023-09-13T14:56:15Z

@damagatchi retest this please

avazirna · 2023-09-13T16:36:41Z

@damagatchi retest this please

avazirna · 2023-09-14T00:14:57Z

@damagatchi retest this please

avazirna · 2023-09-14T11:02:10Z

@shubham1g5 I was planning to PR the Support Library changes separately, but the decided to include here considering that there aren't many

avazirna · 2023-09-14T12:36:40Z

@damagatchi retest this please

shubham1g5

I took a quick look and Looks good overall. I plan to take another deeper look into changes but leaving some high level notes until then.

shubham1g5 · 2023-09-14T15:06:42Z

app/src/org/commcare/utils/Permissions.java

+                    Manifest.permission.READ_MEDIA_AUDIO,
+                    Manifest.permission.READ_MEDIA_VIDEO}));
+        }
+        else{


nit: formatting

shubham1g5 · 2023-09-14T16:46:12Z

app/src/org/commcare/utils/Permissions.java

                Manifest.permission.CALL_PHONE,
                Manifest.permission.ACCESS_FINE_LOCATION,
                Manifest.permission.ACCESS_COARSE_LOCATION,
-                Manifest.permission.WRITE_EXTERNAL_STORAGE,


do we not need to request WRITE permissions exolicitly anymore for api 33 ?

@shubham1g5 it has been deprecated, so it has no effect on devices running Android 13.

So does the READ_MEDIA_IMAGES gives permissions for both reading and writing images to the storage ?

@shubham1g5 not necessarily, my understanding is that from Android 10 with the introduction of scoped storage, apps don't actually need permission to write in their specific folders and we also don't need permissions to write to shared storage (considering that the app owns the files it creates). I just wonder if there are any use cases we should be concerned about? I looked into this and ran a few tests and it seems that we don't

shubham1g5 · 2023-09-14T16:50:04Z

app/src/org/commcare/utils/Permissions.java

@@ -104,7 +104,8 @@ public static String[] getAppPermissions() {
            appPermissions.addAll(Arrays.asList(new String[]{
                    Manifest.permission.READ_MEDIA_IMAGES,
                    Manifest.permission.READ_MEDIA_AUDIO,
-                    Manifest.permission.READ_MEDIA_VIDEO}));
+                    Manifest.permission.READ_MEDIA_VIDEO,
+                    Manifest.permission.NEARBY_WIFI_DEVICES}));


Can we only request this permission later on in the wifi direct screen ? Most users will never access wifi direct feature and it doesn't seem right to ask for the permission upfront for such a rare use case.

shubham1g5 · 2023-09-14T16:52:50Z

app/src/org/commcare/preferences/AdvancedActionsPreferences.java

+        showingPermissionDialog.observe(this, value -> {
+            if (value == REJECT_FEATURE){
+                informUserAboutFeatureUnavailability();
+            }
+            else if (value == REQUEST_PERMISSION){
+                showNearbyWiFiPermissionRationale();
+            }
+        });


Should we move all wifi-direct permission handling in CommCareWiFiDirectActivity itself ? It doesn't make much sense to me for this code to be in preference class as it doesn't access any APIs behind this permission directly.

@shubham1g5 I battled with this a bit, the reasoning was to prevent the creation of CommCareWiFiDirectActivity ini case the permission was not granted, which happens in the AdvancedActionsPreference. It does feel misplaced having it there, but within CommCareWiFiDirectActivity we will need to put behind the Transfer and Receive options.

Can't we simply add the permission check in onCreate for CommCareWiFiDirectActivity and show user an error if the permission is not granted on the same screen ?

@shubham1g5 we can, but where should we trigger the permission request workflow, in case the user hasn't yet been asked?

you can trigger it in onCreate for CommCareWiFiDirectActivity itself if the permission is not given already.

shubham1g5 · 2023-09-14T16:55:59Z

app/src/org/commcare/CommCareNoficationManager.java

+    public boolean areNotificationsEnabled() {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+            return ((NotificationManager)context.getSystemService(NOTIFICATION_SERVICE))
+                    .areNotificationsEnabled();
+        }
+        else {
+            return true;
        }


nit: all the new code should be formatted as per our style files.

shubham1g5 · 2023-09-19T00:34:59Z

app/src/org/commcare/CommCareNoficationManager.java

-            } else {
-                updateMessageNotification();
+
+            if (areNotificationsEnabled()) {


Do mNM.cancel require permission as well ? If not, What do you think of putting this check at just one place at start of updateMessageNotification ?

shubham1g5 · 2023-09-19T00:48:44Z

app/src/org/commcare/mediadownload/MissingMediaDownloadHelper.kt

@@ -83,7 +83,9 @@ object MissingMediaDownloadHelper : TableStateListener {

            RequestStats.register(InstallRequestSource.BACKGROUND_LAZY_RESOURCE)
            global.setInstallCancellationChecker(cancellationChecker)
-            startPinnedNotification()
+            if (CommCareApplication.notificationManager().areNotificationsEnabled()) {


nit: should put this check in the startPinnedNotification() method itself

shubham1g5 · 2023-09-19T13:49:38Z

app/src/org/commcare/preferences/AdvancedActionsPreferences.java

+        showingPermissionDialog.observe(this, value -> {
+            if (value == REJECT_FEATURE){
+                informUserAboutFeatureUnavailability();
+            }
+            else if (value == REQUEST_PERMISSION){
+                showNearbyWiFiPermissionRationale();
+            }
+        });


you can trigger it in onCreate for CommCareWiFiDirectActivity itself if the permission is not given already.

shubham1g5 · 2023-09-19T14:17:53Z

app/src/org/commcare/tasks/LogSubmissionTask.java

@@ -268,7 +272,9 @@ private static LogSubmitOutcomes submitDeviceReportRecord(DeviceReportRecord slr
            return LogSubmitOutcomes.SUBMITTED;
        }

-        listener.startSubmission(index, f.length());
+        if (listener != null) {


nit: add @Nullable to the global variable declation for listener to signify it can be null

shubham1g5 · 2023-09-19T14:18:43Z

app/src/org/commcare/utils/CommCareUtil.java

+        if (CommCareApplication.notificationManager().areNotificationsEnabled()) {
+            reportSubmitter = new LogSubmissionTask(
+                    CommCareApplication.instance().getSession().getListenerForSubmissionNotification(R.string.submission_logs_title),
+                    url,
+                    forceLogs);
+        } else {
+            reportSubmitter = new LogSubmissionTask(url, forceLogs);
+        }


nit: formatting seems off

shubham1g5 · 2023-09-19T23:23:27Z

app/src/org/commcare/activities/CommCareWiFiDirectActivity.java

+        REQUEST_PERMISSION;
+    }
+
+    {


Nice pattern!

avazirna added 3 commits July 18, 2023 15:41

Bump compile and target SDK to version 33

f02b2c2

Location API changes

5cf6917

Add granular media permissions

e043658

shubham1g5 mentioned this pull request Aug 22, 2023

Changed compile SDK version to 33. #2693

Closed

2 tasks

avazirna added 7 commits September 7, 2023 12:13

Add nearby WI-FI devices permission

064efca

Request nearby Wi-Fi devices permission

f6857bc

Handle configuration changes

b6ce66d

Add POST_NOTIFICATION permission

671f530

Avoid posting notifications when not allowed

113c723

Set enableOnBackInvokedCallback to false

099faf1

Retrieve themed drawables

e247428

Refactor

99e25a5

Retrieve themed drawables

beff040

avazirna force-pushed the android_13_update branch 3 times, most recently from aeb6e4b to beff040 Compare September 14, 2023 00:05

avazirna marked this pull request as ready for review September 14, 2023 00:23

avazirna requested a review from shubham1g5 September 14, 2023 00:23

avazirna added 2 commits September 14, 2023 12:08

Support Library - AndroidX migration

fb29be7

Support Library - Set compile and target SDK to 33

332609b

avazirna added this to the 2.54 milestone Sep 14, 2023

avazirna self-assigned this Sep 14, 2023

avazirna added the Release Note label Sep 14, 2023

shubham1g5 added the QA Note label Sep 14, 2023

shubham1g5 requested changes Sep 14, 2023

View reviewed changes

shubham1g5 mentioned this pull request Sep 15, 2023

Initial development on Connect workflows #2695

Open

avazirna added 2 commits September 18, 2023 16:46

Format code

fd067e3

Remove Nearby Wi-Fi devices permission from standard permissions

ea741a3

shubham1g5 requested changes Sep 19, 2023

View reviewed changes

avazirna added 5 commits September 19, 2023 16:32

Set max SDK for deprecated permission

eda3bbc

Refactor Nearby Wi-Fi Devices permission logic

55b5545

Refactor

955ce8e

Make listener nullable

dbd799f

Lint

1e59c39

shubham1g5 approved these changes Sep 19, 2023

View reviewed changes

shubham1g5 merged commit faa5a5b into master Sep 20, 2023
4 of 10 checks passed

shubham1g5 deleted the android_13_update branch September 20, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android 13 update #2683

Android 13 update #2683

avazirna commented Jul 25, 2023 •

edited by shubham1g5

avazirna commented Sep 12, 2023

avazirna commented Sep 12, 2023

avazirna commented Sep 13, 2023

avazirna commented Sep 13, 2023

avazirna commented Sep 14, 2023

avazirna commented Sep 14, 2023

avazirna commented Sep 14, 2023

shubham1g5 left a comment

shubham1g5 Sep 14, 2023

shubham1g5 Sep 14, 2023

avazirna Sep 18, 2023

shubham1g5 Sep 18, 2023

avazirna Sep 19, 2023

shubham1g5 Sep 14, 2023

avazirna Sep 18, 2023

shubham1g5 Sep 14, 2023

avazirna Sep 18, 2023

shubham1g5 Sep 18, 2023

avazirna Sep 19, 2023

shubham1g5 Sep 19, 2023

avazirna Sep 19, 2023

shubham1g5 Sep 14, 2023

shubham1g5 Sep 19, 2023

avazirna Sep 19, 2023

shubham1g5 Sep 19, 2023

avazirna Sep 19, 2023

shubham1g5 Sep 19, 2023

shubham1g5 Sep 19, 2023

shubham1g5 Sep 19, 2023

shubham1g5 Sep 19, 2023

avazirna Sep 20, 2023

Android 13 update #2683

Android 13 update #2683

Conversation

avazirna commented Jul 25, 2023 • edited by shubham1g5

Summary

Safety Assurance

Safety story

avazirna commented Sep 12, 2023

avazirna commented Sep 12, 2023

avazirna commented Sep 13, 2023

avazirna commented Sep 13, 2023

avazirna commented Sep 14, 2023

avazirna commented Sep 14, 2023

avazirna commented Sep 14, 2023

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Jul 25, 2023 •

edited by shubham1g5