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

⬆️ Upgrade to the latest API versions on android and iOS #934

Open
shankari opened this issue Jul 11, 2023 · 51 comments
Open

⬆️ Upgrade to the latest API versions on android and iOS #934

shankari opened this issue Jul 11, 2023 · 51 comments

Comments

@shankari
Copy link
Contributor

We are currently at API 32 (upgraded in #838)
We need to be at API 33 (Android 13) by Sept 2023
https://developer.android.com/google/play/requirements/target-sdk

So, like we did before, we need to identify possible changes to background operation of the app, including background location, background motion activity and background launches in general for syncing the data. And then we need to address them.

Behavior changes list: https://developer.android.com/about/versions/13/behavior-changes-13
https://developer.android.com/about/versions/13/behavior-changes-all

@shankari shankari moved this to Current two week sprint in OpenPATH Tasks Overview Jul 11, 2023
@niccolopaganini
Copy link

These were some API differences I noticed between API 32 and 33:

1. Android.location

              a. GnssMeasurementError 
                            i.	Setclock() – removed
                            ii.	setFullTracking() – enabled by default
                            iii.LocationReport() – class now includes a property for background priority/ tracking
                            iv.	GnssAutomaticGainControl() – increased accuracy of GNSS positioning

2. Permissions

              a. Background location is denied by default in API 33
              b. General location tracking is coase with “burst” modes for fine
              c. Always on tracking has to be enabled in settings

3. Android.gms.location

              a. fusedLocationProviderClient – isLocationSettingsIgnored() – ig ignored, app will RECEIVE location updates [not sure about the receiving part]

4. android.content.contrast

              a. getSystemService() – new safe method  that return NULL if a service is not available. 

@shankari
Copy link
Contributor Author

shankari commented Jul 16, 2023

@niccolopaganini

  1. GnssMeasurementError: I don't think we use this class directly
  2. Permissions:
    a. We already go to the settings to enable "Always on tracking" (I think that this was a requirement in API 31 as well)
    b. "burst" modes for fine might be a problem for our algorithms; will need to upgrade and test out what that means.
  3. Android.gms.location: we don't actually use android.gms.location directly - we use the FusedAPI from Google Play Services directly. Does this apply to it as well?
  4. android.content.contrast: I don't believe we use any classes from this package

You might want to check out our plugins: (from the project package.json)

    "cordova-plugin-em-datacollection": "git+https://github.com/e-mission/e-mission-data-collection.git#v1.7.8",
    "cordova-plugin-em-opcodeauth": "git+https://github.com/e-mission/cordova-jwt-auth.git#v1.7.2",
    "cordova-plugin-em-server-communication": "git+https://github.com/e-mission/cordova-server-communication.git#v1.2.6",
    "cordova-plugin-em-serversync": "git+https://github.com/e-mission/cordova-server-sync.git#v1.3.2",
    "cordova-plugin-em-settings": "git+https://github.com/e-mission/cordova-connection-settings.git#v1.2.3",
    "cordova-plugin-em-unifiedlogger": "git+https://github.com/e-mission/cordova-unified-logger.git#v1.3.6",
    "cordova-plugin-em-usercache": "git+https://github.com/e-mission/cordova-usercache.git#v1.1.6",

to see which APIs we use and whether they are in fact affected

Also, you should see:

  • whether there are newer versions of the cordova platforms (https://cordova.apache.org/blog/) and whether they highlight any changes that we should accommodate
  • whether there are new versions of any of the other plugins that we use. You can find the full list from the package.cordovabuild.json linked earlier as well.

@github-project-automation github-project-automation bot moved this from Current two week sprint to Sprint tasks completed in OpenPATH Tasks Overview Jul 16, 2023
@shankari shankari reopened this Jul 16, 2023
@shankari
Copy link
Contributor Author

shankari commented Jul 16, 2023

@niccolopaganini where did you get these API changes from? They are not listed in
https://developer.android.com/about/versions/13/behavior-changes-13

The only one that I see from this list (behavior-changes-13) that might be concerning is
https://developer.android.com/about/versions/13/behavior-changes-13#battery-resource-utilization

Ah but we already ask for unrestricted background operation, so this should be covered. Should test to verify

@shankari shankari moved this from Sprint tasks completed to Current two week sprint in OpenPATH Tasks Overview Jul 16, 2023
@niccolopaganini
Copy link

@shankari https://developer.android.com/sdk/api_diff/33/changes - this was the website I primarily referred to.

@niccolopaganini
Copy link

niccolopaganini commented Jul 25, 2023

I went through the plugins again. As @shankari mentioned previously, these were the following plugins @shankari and team had written:
1 . cordova-plugin-em-datacollection
2. cordova-plugin-em-opcodeauth
3. cordova-plugin-em-server-communication
4. cordova-plugin-em-serversync
5. cordova-plugin-em-settings
6. cordova-plugin-em-unifiedlogger
7. cordova-plugin-em-usercache2

I don't think the API migration will affect these packages

Edit: I cross-referenced some packages that caught my eye.

@shankari
Copy link
Contributor Author

@niccolopaganini that's good. can you outline the method that you used to verify this? a listing of the unique packages used in each plugin and a link to the related API doc page would be helpful.

@niccolopaganini
Copy link

niccolopaganini commented Jul 26, 2023

Update on the aforementioned packages:

1.cordova-plugin-em-datacollection

android->location->OPGeofence->ExitActivityIntentService &WalkExitWorker has java.util.locale; TripDiaryStateMachineForegroundService.java has NotificationManager; Verification->SensorControlChecks has NotificationManager; wrapper -> Battery.java has BatteryManager

3. cordova-plugin-em-server-communication

java.net.URL

4. cordova-plugin-em-serversync

Notification Manager

5. cordova-plugin-em-settings

java.net.URL

6. cordova-plugin-em-unifiedlogger

java.io.File; NotificationManager

2. cordova-plugin-em-opcodeauth & 6. cordova-plugin-em-unifiedlogger

None

@niccolopaganini
Copy link

niccolopaganini commented Jul 26, 2023

@niccolopaganini that's good. can you outline the method that you used to verify this? a listing of the unique packages used in each plugin and a link to the related API doc page would be helpful.

I "cross-verified" the API update and narrowed them down to these packages:

  1. android.app.NotificationManager
  2. android.view.View
  3. java.io.File
  4. java.util.locale
  5. java.long.System
  6. java.net.URL
    Cordova_plugin_
    1. splashScreen
    2. camera
    3. geolocation

I have determined these packages based on the potential use cases that can take advantage of.

Edit: Based on one of your comments regarding the "android.gms.location," I didn't find anything that might directly have a direct influence in this migration update

@shankari
Copy link
Contributor Author

shankari commented Jul 26, 2023

@niccolopaganini

but how did you cross-verify?
I have determined these packages based on the potential use cases that can take advantage of.

I am instead looking for something like: "I searched for all strings starting with "package". Then I went to URL https://... and searched for the string, etc."

I "cross-verified" the API update and narrowed them down to these packages:

Also what are the changes in these packages? Do they affect the functionality that our plugins use?

android->location->OPGeofence->ExitActivityIntentService &WalkExitWorker has java.util.locale; TripDiaryStateMachineForegroundService.java has NotificationManager; Verification->SensorControlChecks has NotificationManager; wrapper -> Battery.java has BatteryManager

I am not sure what you mean by this. Are you saying that the only packages that cordova-plugin-em-datacollection uses are Locale, NotificationManager and BatteryManager?

Cordova_plugin_

  1. splashScreen
  2. camera
  3. geolocation

I am not sure what you mean by this. We may use the cordova splashscreen, but we definitely do not use the camera or geolocation plugins. Where did you find references to them?

@niccolopaganini
Copy link

niccolopaganini commented Aug 2, 2023

Process:

I started out with the links that you suggested:

1. https://developer.android.com/about/versions/13/behavior-changes-13 2. https://developer.android.com/about/versions/13/behavior-changes-all

Other links that I used:

3. https://developer.android.com/sdk/api_diff/33/changes

- I used the "package.cordovabuild.json" file, and then navigated to the src directory for all plugins to locate the files with the packages.

(entered by @shankari on behalf of @niccolopaganini )

From link 1, I identified the areas of android that were affected by the migration to APi 33
From link 2, I matched the areas to the android package names affected by the migration
For each package affected by the migration, I searched for any matching imports by opening the java files one by one and checking manually.
This check has been at the package + class name - I have not yet identified whether the affected methods are being used by our code.

For instance, link 1 had indicated changes to foreground notification(s). The "data-collection" plugin had (the) "NotificationManager" (as part of the android.app package). This is why I added it to the list of packages that might be affected by the migration update. They have added a new method: https://developer.android.com/reference/android/app/NotificationManager.html#matchesCallFilter(android.net.Uri)

@shankari
Copy link
Contributor Author

shankari commented Aug 2, 2023

I have three high-level comments on that process:

  1. Good job finding the API diffs - I haven't seen or used that before. I have just used the high level description of the changes because I am familiar with the packages we use. However, the API is more comprehensive and a better resource to use going forward.
  2. Given that we have the link to the API diffs, it is not clear why we need to use the first link. I guess it doesn't hurt, but it seems like it should be validation that we have not missed any high-level changes, but not the starting point
  3. I don't like the manual checking - it is very easy to miss a file or an import. It seems like we should be able to write an automated script that will read through the API changes, and then iterate over all the files and check the imports.
  • Can we spend a time-bounded effort to at least see if there is an existing script to pull out the tree of API changes from a URL? OR
  • write one ourselves using something like Beautiful Soup (or whatever is the modern equivalent)

@niccolopaganini
Copy link

niccolopaganini commented Aug 3, 2023

I am currently working on the script. I was able to extract from the website. However, I am somewhat unsuccessful in extracting text from the Java files. I am getting an output but also an error which I trying to iron out. Hopefully, I will be able to figure it out by 8/3/2023 afternoon

Edit:

Approach:

  1. Extracting text using beautifulsoup:
    I put together a code to filter out packages from the website in such a way that they access the hyperlink and include the changed class. This is stored in a CSV file. This seems to work fine and I cross-checked with a couple of packages in random.

  2. Comparing contents of Java files:
    My goal was to basically put together a piece of code which in a given directory, traverses through all the Java files and compares them against the above-mentioned CSV. The output I'm expecting is just a bunch of print statements that basically looks like: class in file. Although, I am getting the desired result, the code is terminated as follows: "error: unterminated character set at position 127" and the error is at line: match_css("/Users/nseptank/Downloads/WebScrapper_Java_Code/links.csv","/Users/nseptank/e-mission-phone/plugins/cordova-plugin-em-serversync/src/android")

I'm currently troubleshooting this error.

@niccolopaganini
Copy link

niccolopaganini commented Aug 4, 2023

Update on scripts:

I have worked on putting together two scripts: one that extracts the packages with classes from the API_diffs website (https://developer.android.com/sdk/api_diff/33/changes/alldiffs_index_changes) and one to use the extracted information from the previously-mentioned website to scan and match against the contents of the java files in a given directory.

I had some issues with the web-scraping part where it would store as "hidden link." I was able to fix this issue but extracting the hyperlinks as texts and storing the information with the packages' respective classes.

For the second python script, I tried the RE approach but since the CSV had multiple instances of special characters, I had to take an alternative approach to avoid problems mentioned in #934 (comment). I do this by manually iterating over each file in a given directory, check if it is a java file, and then cross-check the contents against the CSV.

Although the code is reproducible, one thing to consider is that PATH defined for the script that matches the classes in JAVA files are absolute. To be more precise, you'll have to be very specific in defining the PATH that the script performs its operation on as it ignores directories.

Edit: This way of approach is reproducible and therefore, minimal time is consumed in actually finding the classes. As this DOES NOT scan for the methods used, that work still have to be done manually, but by taking this approach, you'll reduce time as opposed to completely checking it manually.

@niccolopaganini
Copy link

niccolopaganini commented Aug 8, 2023

Update on Native Plugins List:

Running the scripts through the directories, we were able to extract packages more easily and efficiently; and the whole process in reproducible. Amongst the several packages/ classes harvested, 27 were found to be unique and requires more attention. They are as follows:

  1. android.app.Activity
  2. java.util.Arrays
  3. android.content.Context
  4. android.os.BatteryManager
  5. java.util.stream.Collectors
  6. java.util.Collection
  7. android.content.Intent
  8. android.os.Build.VERSION
  9. android.os.Bundle
  10. android.content.IntentFilter
  11. android.os.Build.VERSION_CODES
  12. android.app.Dialog
  13. org.json.JSONObject
  14. android.location.Location
  15. android.content.pm.PackageManager
  16. java.util.concurrent.TimeUnit
  17. android.app.NotificationChannel
  18. android.app.NotificationManager
  19. android.os.PowerManager
  20. android.provider.Settings
  21. java.io.InputStream
  22. android.app.Service
  23. java.lang.String
  24. java.io.FileReader
  25. java.io.PrintStream
  26. java.io.PrintWriter
  27. android.database.sqlite.SQLiteDatabase

The script to extract the packages and classes works. With this, I was able to extract the methods and compile them in a CSV. To not work on a code from scratch, I ran the CSV with the same python script to extract the methods present in the java file and I am troubleshooting it.

This was the script that I used to find classes present in our plugins:

def match_csv_java(csv_file, directory):
    java_files = os.listdir(directory)
    with open(csv_file, "r") as f:
        reader = csv.reader(f)
        for row in reader:
            classes = row[1]         
    #java_files = os.listdir(directory)
            for java_file in java_files:
                if not os.path.isdir(java_file) and java_file.endswith(".java"):
                    with open(os.path.join(directory, java_file), "r", encoding="utf-8") as f:
                        text = f.read()
                        if classes in text:
                            print(f"Found {classes} in {java_file}")   
    
                            
if __name__ == "__main__":
    match_csv_java("/tmp","/tmp")

I tried modifying the code to find the methods and the data types:

def match_csv_java(csv_file, directory, keyword):
    java_files = os.listdir(directory)
    with open(csv_file, "r") as f:
        reader = csv.reader(f, skipinitialspace = True)
        for row in reader:
            methods = row[0].split(",")
            java_classes = set()
    #java_files = os.listdir(directory)
            for java_file in java_files:
                if not os.path.isdir(java_file) and java_file.endswith(".java"):
                    with open(os.path.join(directory, java_file), "r", encoding="utf-8") as f:
                        for line in f:
                            for match in line.split():
                                if match.startswith(keyword) and match.endswith(";"):
                                    java_classes.add(match.split(" ")[1])
            for method in methods:
                if method in java_classes:
                    print(f"Found {method} in {directory}")
    
                            
if __name__ == "__main__":
    match_csv_java("/tmp","/tmp", "public")

In this case, the code does not return any error but also does not output anything at the moment. I was hoping it would return matching methods in a given directory and use that as a foundation to build for the data types but it seems like this approach may not be the best.

@shankari
Copy link
Contributor Author

but it seems like this approach may not be the best.

I am not sure how you came to that conclusion. I don't see any debugging of the functions. What have you done to debug it? I don't even see print statements in here.

@niccolopaganini
Copy link

for method in methods:
if method in java_classes:
print(f"Found {method} in {directory}")

I thought of looping over the list of methods; where the loop checks for the java classes. If there was a match, then the loop would print the method and directory where it was found

@niccolopaganini
Copy link

I have modified the code to scan for the existing methods that might be deprecated, restored, etc.

void stopForeground(boolean) - Now deprecated.

I am doing the same for the present data types currently but I am not getting any matches. Fixing the code currently.

@shankari
Copy link
Contributor Author

I thought of looping over the list of methods; where the loop checks for the java classes. If there was a match, then the loop would print the method and directory where it was found

Yes, but that doesn't address how you are debugging the issue.

Fixing the code currently.

Fixing the script, or fixing the android code?

@niccolopaganini
Copy link

niccolopaganini commented Aug 14, 2023

Fixing the script, or fixing the android code?

I was working on the script (on 8/11/2023). Current status: working.

Process description

By suggestion of @shankari, I started working on scripts (currently in Python, will have to be updated to shell in the future) - First script for webscraping that finds packages and its respective classes. Everything is compiled in a CSV.

The CSV is then used in with the script that compare the CSV against the java files in a given directory. I created a common method:

def match_csv_java(csv_file, directory): 
    java_files = os.listdir(directory)
    with open(csv_file, "r") as f: 
        reader = csv.reader(f)
        for row in reader:
            classes = row[1]         
            for java_file in java_files:
                if not os.path.isdir(java_file) and java_file.endswith(".java"): 
                    with open(os.path.join(directory, java_file), "r", encoding="utf-8") as f:
                        text = f.read()
                        if classes in text:
                            print(f"Found {classes} in {java_file}") 

You can call the function by:

if __name__ == "__main__":
    match_csv_java("/tmp","/tmp")

Sample Output (that can be expected from the code):

Screen Shot 2023-08-14 at 4 41 31 PM

It takes absolute Paths, first one being CSV and the latter for the directory. One can use the classes and then narrow down the exact methods. I compiled the methods in a CSV stripping that can be counted as "common entity" (Screenshot below)
Screen Shot 2023-08-14 at 4 24 00 PM

This is also done for the previous CSV file. I do this for simplicity. For example:

Before:

Screen Shot 2023-08-14 at 4 29 03 PM

After:

Screen Shot 2023-08-14 at 4 28 45 PM

It is to be noted that both CSVs had multiple counts of the same packages/ classes/ methods. Now, it can be solved with code but Excel's UNIQUE function is much easier which is what I used to simplify the list.


Now continuing with the methods extraction, I modified the previous code as follows:

def find_matches(csv_file,java_directory): #custom method
    
    methods = []
    with open(csv_file,"r") as csvfile: #reading the CSV
        reader = csv.reader(csvfile, delimiter=",")
        for row in reader:
            method_name = row[0]
            methods.append(method_name)
            
    matching_methods = [] #reading the java file
    for file in os.listdir(java_directory):
        if file.endswith(".java"):
            with open(os.path.join(java_directory, file), "r") as f:
                for line in f:
                    for method in methods:
                        if method in line:
                            matching_methods.append((file, line))
                            
    return matching_methods

to return the methods In the CSV. The function is called with the following code:

if __name__ == "__main__":
    csv_file = "/tmp"
    java_directory = "/tmp"
    matching_methods = find_matches(csv_file, java_directory)
    for file, line in matching_methods:
        print(f"{file} has {line}")

Sample Output (An output that can be expected from the code):
Screen Shot 2023-08-14 at 4 39 45 PM

The classes that seem to be affected for this migration therefore are as follows:

  1. Data-Collection -> Location Directory
    • TripDiaryStateMachineForegroundService.java has srv.stopForeground(true);
  2. Data-Collection -> Location -> Actions
    • GeofenceActions.java has GeofenceActions.this.newLastLocation = intent.getParcelableExtra(GeofenceLocationIntentService.INTENT_RESULT_KEY);
    • GeofenceActions.java has GeofenceActions.this.newLastLocation = intent.getParcelableExtra(GeofenceLocationIntentService.INTENT_RESULT_KEY);
  3. Data-Collection -> Wrapper Directory
    • StatsEvent.java has this.client_app_version = ctxt.getPackageManager().getPackageInfo(ctxt.getPackageName(), 0).versionName;
  4. Unified Logger
    • UnifiedLogger.java has import java.io.PrintWriter;
    • UnifiedLogger.java has e.printStackTrace(new PrintWriter(sw));
    • UnifiedLogger.java has e.printStackTrace(new PrintWriter(sw));
    • UnifiedLogger.java has e.printStackTrace(new PrintWriter(sw));
    • UnifiedLogger.java has e.printStackTrace(new PrintWriter(sw));
    • Log.java has import java.io.FileReader;
    • Log.java has import java.io.PrintWriter;
    • Log.java has PrintWriter pw = new PrintWriter(sw);
    • DatabaseLogHandler.java has import java.io.PrintStream;

@shankari
Copy link
Contributor Author

shankari commented Aug 15, 2023

@niccolopaganini high level comments:

We do need to eventually make sure that all plugins work, so I see this as a plus

It is to be noted that both CSVs had multiple counts of the same packages/ classes/ methods. Now, it can be solved with code but Excel's UNIQUE function is much easier which is what I used to simplify the list.

Wait, so you are saying that you can't just run the scripts one after the other? The process involves opening the file in Excel in the middle? That is not even listed as a step in your process. We are not going to use a manual process using point and click in Excel as part of this flow.

@shankari
Copy link
Contributor Author

@niccolopaganini discussed this issue and I am stepping in to help out. It also looks like the there are not significant changes, based on the API diff lists that @niccolopaganini generated, so I am optimistic that we can still get this done by the end of the month.

@niccolopaganini will continue with developing the script, which will be useful in the future to fix deprecated methods over the course of the year, until the next API upgrade season is upon us 😄 @niccolopaganini please file another issue for the script and copy your comments in there. I have collapsed them here to avoid overwhelming this issue.

@shankari
Copy link
Contributor Author

Looking through the list of behavior changes:

  • Notification permission affects foreground service appearance: we make users give us the notification permission. If they subsequently turn it off, I guess there will be some difference in behavior, but not sure there is much we can do about that. We will warn them when they launch the app and make them turn it on again, which is likely the best that we can do
  • we already have ACCESS_FINE_LOCATION permission
  • granular media permissions: we need the external storage permission so that we can store our logger files and make them available to the mail mail client. We are not reading any media files. TO VERIFY
  • body sensor background permissions: is motion_activity a body sensor? Otherwise, we don't need it TO VERIFY
  • restricted state -> no BOOT_COMPLETED: we actually make users set us to non-optimized, so if they put us in restricted, we will notice and make them fix it
  • media controls (not applicable)
  • app color theme for webview content: we don't use dark mode
  • advertising ID: don't use it
  • non-SDK restrictions: don't use any non-SDK interfaces

@shankari
Copy link
Contributor Author

Verifications:

  • confirmed that we have WRITE_EXTERNAL_STORAGE not READ_EXTERNAL_STORAGE
    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
    <queries>
        <intent>
            <action android:name="android.intent.action.SENDTO" />
            <data android:scheme="mailto" />
        </intent>
    </queries>

@shankari
Copy link
Contributor Author

ok! I don't think we need any upgrades to our native code. Yay! Maybe we have finally gotten over the hump wrt privacy updates.

Per the cordova blog
https://cordova.apache.org/blog/

Cordova has been upgraded to:

  • iOS 7.0.0:
    • Removal of podspec from the framework tag (have to update our plugins)
    • min nodeJS is 16.x.x. We are at export NODE_VERSION=19.5.0, so we are fine.
      • however, current node version has been bumped to 20.5.1 so we could try upgrading as well
    • LimitsNavigationsToAppBoundDomains: we don't need cookie authentication or browser APIs
    • Support Apple Cloud Distribution Signing: cool, but we can just keep building manually until RN migration and/or iOS upgrade
  • android 12.0.0:
    • bump up targetSDK to 33 and minSDK to 24. Note that then we don't need to/can't support Android 6 any more
    • may need to upgrade SDK packages
    • themed icon support (we don't care)
  • the blog also highlights an update to cordova-plugin-file related to removing the WRITE_EXTERNAL_STORAGE permission. Apparently, this was removed in android 11 (https://developer.android.com/about/versions/11/privacy/storage). What did we do about storage in android 11?

@JGreenlee
Copy link

For disabling Android battery optimizations, why can't we show this popup?

image

It's really bad UX to make people go through their phone's battery optimizations, find OpenPATH in the list, and disable it themselves.
I bet this is a point in the onboarding flow at which some users just get frustrated, give up and uninstall

I actually think ALL of the permission requests should be popups instead of Settings shortcuts. But this one seems higher priority because it's the biggest inconvenience

@shankari
Copy link
Contributor Author

shankari commented Aug 17, 2023

I agree that this is bad UX, and we do show popups whenever we can. If you try to install OpenPATH on an Android 6 or Android 7 phone, for example, it will have a simple popup for the location permission.

However, Android has become progressively stricter about background permissions, particularly for location.

The battery optimization popup, in particular, is because of this change in android 12
https://developer.android.com/guide/components/foreground-services#background-start-restrictions

If we don't have a foreground service, we will only receive location updates "few times an hour" (https://developer.android.com/about/versions/oreo/background-location-limits)

If our foreground service is killed, we cannot restart it from the background unless we qualify for one of the exceptions.
https://developer.android.com/guide/components/foreground-services#background-start-restriction-exemptions

The exceptions include:

The user turns off battery optimizations for your app. You can help users find this option by sending them to your app's App info page in system settings. To do so, invoke an intent that contains the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent action.

We invoke the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent action here:
https://github.com/e-mission/e-mission-data-collection/blob/bff039e40252cd004956e76f3e4d401dd46819c8/src/android/verification/SensorControlForegroundDelegate.java#L431

I am not sure what API the other app is using. But it is not what the android documentation indicates that we should use. I don't want to use an undocumented API for obvious reasons, but if you can find an alternate documented method, we can easily make the change.

@Abby-Wheelis
Copy link
Member

We should add/update the text as part of this release

By this release do you mean the one on staging right now? If so, I can take care of updating the existing strings in en/es.json and reach out about lo.json if we have time, I'm not super familiar with Android yet, but I can probably research and/or test out instructions since I have test phones.

I can also roll wider updates of any text into the work I'm doing with permissions in Profile, though that isn't used for the onboarding flow (yet) that is still the older page.

@shankari
Copy link
Contributor Author

Video of location permission popup on android 7 😄

android-7-permissions.mov

@JGreenlee
Copy link

However, Android has become progressively stricter about background permissions, particularly for location.

Yeah I have observed that Android has gotten stricter. But that doesn't mean we can't show popups. Tons of other apps do.
I think we just have to find the appropriate ways of requesting perms in newer APIs.

For location perms we will need a series of popups instead of just 1, because they make you specifically ask for "Precise location" and "all the time".

We invoke the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent action here:

I think if we request for ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS, we'd get the popup:
https://developer.android.com/reference/android/provider/Settings#ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS

@JGreenlee
Copy link

I know this would probably require native changes and I'm not necessarily prepared to dig into the Java code myself.
But it is higher up on my wishlist because I think it is significant to the UX.
Maybe after the API version upgrade we can look at it

@shankari
Copy link
Contributor Author

shankari commented Aug 18, 2023

I think if we request for ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS, we'd get the popup:

That's a great find, @JGreenlee - I am happy to make that particular change along with these API updates.

I wish the Android documentation would tell us the user-friendly method to use.
I will admit to being focused on just getting the API upgrades done with minimal effort before going on to the many other things that are also competing for attention. But now that we have an intern focused on android updates, @niccolopaganini maybe we can find other such methods...

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

Something to investigate

Adds runtime permission. For your app to send non-exempt notifications, the user must grant this permission to your app.

https://developer.android.com/develop/ui/views/notifications/notification-permission

If a user installs your app on a device that runs Android 13 or higher, your app's notifications are off by default. Your app must wait to send notifications until after you request the new permission and the user grants that permission to your app.

We currently prompt the user to enable notifications from the app settings page. It seems like once they have done that, we would not need to request again. @niccolopaganini we should definitely test this.

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

Before I continue, let's quickly check whether any of the other plugins require updates.

  • @havesource/cordova-plugin-push: unchanged
  • cordova-plugin-ionic-keyboard: unchanged
  • cordova-plugin-ionic-webview: unchanged (although I saw some note about webview in a previous cordova release, need to double-check)
  • cordova-plugin-app-version: unchanged
  • ``cordova-plugin-file`: update to 8.0.0 (already known)
  • cordova-plugin-device: unchanged
  • cordova-plugin-customurlscheme: unchanged
  • cordova-plugin-email-composer: unchanged
  • cordova-plugin-x-socialsharing: unchanged
  • cordova-plugin-inappbrowser: unchanged
  • cordova-plugin-local-notification-12: using our own fork but the master has not changed
  • cordova-plugin-advanced-http: unchanged
  • cordova-plugin-androidx-adapter: unchanged
  • phonegap-plugin-barcodescanner: unchanged

@niccolopaganini as you can see, none of these other plugins have changed. But maybe they should have been, and are only unchanged because they are unmaintained. We need to test all their functionality to make sure that it still works.
If it is broken, we will need to find an existing fix or clone/fix which will take time.

So this is super urgent and critical to get right.

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

Do we need to update the SDK tools? It looks like we already have SDK 33 support

build-tools;33.0.2
...
platforms;android-33
...
system-images;android-33;google_apis;x86_64
system-images;android-33;google_apis_playstore;x86_64

However, the SDK manager now seems to have updates, including for the API 33 system image

Available Updates:
  ID                                          | Installed | Available
  -------                                     | -------   | -------
  emulator                                    | 32.1.11   | 32.1.14
  platform-tools                              | 34.0.1    | 34.0.4
  platforms;android-33                        | 2         | 3
  system-images;android-32;google_apis;x86_64 | 5         | 7
  system-images;android-33;google_apis;x86_64 | 9         | 13

We might as well update to test against the latest images, and now get some preliminary API 24 support as well.

  add-ons;addon-google_apis-google-24                                                      | 1            | Google APIs
  build-tools;34.0.0                                                                       | 34.0.0         platforms;android-34                                                                     | 2            | Android SDK Platform 34
  system-images;android-34;google_apis;arm64-v8a                                           | 8            | Google APIs ARM 64 v8a System Image
  system-images;android-34;google_apis;x86_64                                              | 8            | Google APIs Intel x86_64 Atom System Image
  system-images;android-34;google_apis_playstore;arm64-v8a                                 | 8            | Google Play ARM 64 v8a System Image
  system-images;android-34;google_apis_playstore;x86_64                                    | 8            | Google Play Intel x86_64 Atom System Image

Note that the cmdtools version itself has been upgraded to 10406996

@shankari
Copy link
Contributor Author

I think if we request for ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS, we'd get the popup:
https://developer.android.com/reference/android/provider/Settings#ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS

FYI, this requires an additional permission

Activity Action: Ask the user to allow an app to ignore battery optimizations (that is, put them on the whitelist of apps shown by ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS). For an app to use this, it also must hold the Manifest.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS permission.

@shankari
Copy link
Contributor Author

The local notification plugin seems to already have some of this?

$ grep -r REQUEST_IGNORE_BATTERY_OPTIMIZATIONS platforms/android/
Binary file platforms/android//app/build/intermediates/project_dex_archive/debug/out/de/appplant/cordova/plugin/localnotification/LocalNotification.dex matches
Binary file platforms/android//app/build/intermediates/javac/debug/classes/de/appplant/cordova/plugin/localnotification/LocalNotification.class matches
Binary file platforms/android//app/build/intermediates/dex/debug/mergeProjectDexDebug/14/classes.dex matches
platforms/android//app/src/main/java/de/appplant/cordova/plugin/localnotification/LocalNotification.java:import static android.Manifest.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS;
platforms/android//app/src/main/java/de/appplant/cordova/plugin/localnotification/LocalNotification.java:            // User can add "REQUEST_IGNORE_BATTERY_OPTIMIZATIONS" to the manifest, but
platforms/android//app/src/main/java/de/appplant/cordova/plugin/localnotification/LocalNotification.java:                    if (pi.requestedPermissions[i].equals(REQUEST_IGNORE_BATTERY_OPTIMIZATIONS)) {
platforms/android//app/src/main/java/de/appplant/cordova/plugin/localnotification/LocalNotification.java:                        action = Settings.ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS;
platforms/android//app/src/main/java/de/appplant/cordova/plugin/localnotification/LocalNotification.java:                // and did not have access to launch REQUEST_IGNORE_BATTERY_OPTIMIZATIONS

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

Looking, at that plugin, I see this note:

            // use the generic intent if we don't have access to request ignore permissions
            // directly
            // User can add "REQUEST_IGNORE_BATTERY_OPTIMIZATIONS" to the manifest, but
            // risks having the app banned.
            try {
                PackageManager packageManager = this.cordova.getContext().getPackageManager();
                PackageInfo pi = packageManager.getPackageInfo(packageName, PackageManager.GET_PERMISSIONS);

                for (int i = 0; i < pi.requestedPermissions.length; ++i) {
                    if (pi.requestedPermissions[i].equals(REQUEST_IGNORE_BATTERY_OPTIMIZATIONS)) {
                        action = Settings.ACTION_REQUEST_IGNORE_BATTERY_OPTIMIZATIONS;
                    }
                }

Looks like at least in olden times, requesting the permission and launching the prompt could get you banned.
https://stackoverflow.com/questions/44862176/request-ignore-battery-optimizations-how-to-do-it-right

And there are some reports that using the dialog does not put the app in the list.
https://stackoverflow.com/questions/54077966/how-to-change-battery-optimization-settings-manually-after-requesting-to-ignore

Although it looks like they may be less strict now as seen in react native permissions

Flagging @niccolopaganini for testing issues reported in stackoverflow.

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

@JGreenlee I am tempted to not add the permission and use the prompt given that I don't want to risk being banned so close to the API upgrade requirement.

I think that the chances of being banned are low since we ask for all kinds of other permissions. And when they launch and test the app, they must know that we ask for the permission from the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent.

But it may be that adding the permission flags the app in a way that the manual review doesn't. And I really want to get the API update out without hitches.

We can add the permission in the release after the API upgrade and see what happens. Thoughts?

@shankari
Copy link
Contributor Author

First level of upgrade: to e-mission-data-collection:
e-mission/e-mission-data-collection#207

@shankari
Copy link
Contributor Author

shankari commented Aug 19, 2023

Note that we should also upgrade the frameworks that we use - primarily the Google Play Services APIs for location and motion activity but again, not going to take that on now to minimize update footprint.

    <framework src="com.google.code.gson:gson:2.10.1"/>
    <framework src="com.google.android.gms:play-services-location:$LOCATION_VERSION"/>
    <preference name="LOCATION_VERSION" default="21.0.1"/>
    <framework src="androidx.core:core:$ANDROIDX_CORE_VERSION"/>
    <preference name="ANDROIDX_CORE_VERSION" default="1.8.0"/>
    <framework src="androidx.work:work-runtime:$ANDROIDX_WORK_VERSION"/>
    <preference name="ANDROIDX_WORK_VERSION" default="2.7.1"/>

@niccolopaganini for visibility into future changes.

@shankari
Copy link
Contributor Author

Pending tasks:

Once those are done, we just need to check that the instruction text in the permission screen works for Android 13, and create another conditional in case it does not.

@JGreenlee
Copy link

JGreenlee commented Aug 20, 2023

@JGreenlee I am tempted to not add the permission and use the prompt given that I don't want to risk being banned so close to the API upgrade requirement.

I think that the chances of being banned are low since we ask for all kinds of other permissions. And when they launch and test the app, they must know that we ask for the permission from the ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS intent.

But it may be that adding the permission flags the app in a way that the manual review doesn't. And I really want to get the API update out without hitches.

We can add the permission in the release after the API upgrade and see what happens. Thoughts?

Sure, if that's what you think is best. I am not familiar with the app release/ update process, or what constitutes a risk of being flagged, so I'd just defer to your expertise on that

@shankari
Copy link
Contributor Author

Sure, if that's what you think is best. I am not familiar with the app release/ update process, or what constitutes a risk of being flagged, so I'd just defer to your expertise on that

Given that we are also going to have a redesigned permissions screen in the next release. I am going to postpone this change also until then.

@shankari
Copy link
Contributor Author

shankari commented Aug 26, 2023

Following up on this to find the dependencies for the file plugin which we updated. The plugin itself is cordova.file but it also exports to window directly. So let's put our grep to work.

Find all the exports in the file plugin and then see where they exist in our code.

Method location
requestFileSystem www/js/services.js
DirectoryEntry no matches
FileSystem no matches
file.*Directory emailService, uploadService
FileEntry comment in services.js
DirectoryReader no matches
FileReader services, uploadService
FileWriter same comment in services.js
FileUploadResult does not exist
file.*Entry services.js, uploadService

services.js:

this.getMyData = function(startTs) {
var emailData = function(result) {

    this.writeFile = function(fileEntry, resultList) {
      // Create a FileWriter object for our FileEntry (log.txt).
    }

emailService.js

        this.sendEmail = function (database) {

uploadService.js

                const readDBFile = function(parentDir, database, callbackFn) {
            return new Promise(function(resolve, reject) {
                window.resolveLocalFileSystemURL(parentDir, function(fs) {
                    fs.filesystem.root.getFile(fs.fullPath+database, null, (fileEntry) => {

If getMyData, emailData and upload work, file works.

@shankari
Copy link
Contributor Author

Plugin testing versions (set: 10,11,12,13)
cordova-plugin-push "Trip labels requested" shows up all
cordova-plugin-ionic-keyboard type entries in various screens all
cordova-plugin-ionic-webview replacement for built-in webview used by default for display
cordova-plugin-app-version version shows up in settings all
cordova-plugin-file`: (updated) only used in emailServices, uploadService, getMyData . Emailed my data, and received it successfully. Log was too large, so on emailLog, saved to drive. Save was successful, which indicates that the file plugin works properly Android 12
cordova-plugin-device used in permission screen, which has been tested via the UI all
cordova-plugin-customurlscheme I don't think we need this any more. It was important when we used to to use the emission:// URL on the website. But now we don't. Instead, we support copy/paste and in-app scan. I checked scanning the QR code directly in the camera, and it doesn't appear to open the app - it just searches in google. we can probably remove in the next release all
cordova-plugin-email-composer See notes on "file" above, we email file attachments, so the same testing applies to both Android 12
cordova-plugin-x-socialsharing Share button works but link does not all except 12
cordova-plugin-inappbrowser used only in survey external launch (currently obsolete) and remote push notifications for URL
cordova-plugin-local-notification-12 dummy notification from profile screen (I would have liked to check on a study that uses a custom reminder scheme as well) all
cordova-plugin-advanced-http label and dashboard screens work all
cordova-plugin-androidx-adapter used for backwards compat, no explicit testing needed all except 13 (which doesn't need compat)
phonegap-plugin-barcodescanner scan QR code to join study all

shankari added a commit to shankari/e-mission-phone that referenced this issue Sep 9, 2023
This addresses updating the SDK tools to the latest versions:
e-mission/e-mission-docs#934 (comment)

And removing the obsolete HAXM package:
e-mission/e-mission-docs#958 (comment)

It also updates the README to indicate the required java version after the upgrade
e-mission#1016 (comment)
e-mission#1016 (comment)

Testing done:

After upgrading to the most recent version of OpenJDK (17)

```
$ java --version
openjdk 17.0.8.1 2023-08-24
OpenJDK Runtime Environment Temurin-17.0.8.1+1 (build 17.0.8.1+1)
OpenJDK 64-Bit Server VM Temurin-17.0.8.1+1 (build 17.0.8.1+1, mixed mode, sharing)
```

Android SDK install succeeds

```
$ bash setup/prereq_android_sdk_install.sh

---------------------------------------
Accept? (y/N): Y
[=======================================] 100% Unzipping... android-12/zipalign

END: Done with android SDK download, exiting script
```

And an android build in an existing checked-out repo succeeds

```
$ npm run build-dev-android
BUILD SUCCESSFUL in 33s
52 actionable tasks: 12 executed, 40 up-to-date
Built the following apk(s):
	.../platforms/android/app/build/outputs/apk/debug/app-debug.apk

```

Albeit with several deprecated APIs

```
w: /Users/kshankar/in-house/openpath-phone/platforms/android/app/src/main/java/com/adobe/phonegap/push/FCMService.kt: (169, 17): 'get(String!): Any?' is deprecated. Deprecated in Java
w: /Users/kshankar/in-house/openpath-phone/platforms/android/app/src/main/java/com/adobe/phonegap/push/FCMService.kt: (316, 20): 'get(String!): Any?' is deprecated. Deprecated in Java
w: /Users/kshankar/in-house/openpath-phone/platforms/android/app/src/main/java/com/adobe/phonegap/push/FCMService.kt: (627, 33): 'constructor Builder(Context)' is deprecated. Deprecated in Java
w: /Users/kshankar/in-house/openpath-phone/platforms/android/app/src/main/java/com/adobe/phonegap/push/FCMService.kt: (1190, 37): 'fromHtml(String!): Spanned!' is deprecated. Deprecated in Java
w: /Users/kshankar/in-house/openpath-phone/platforms/android/app/src/main/java/com/adobe/phonegap/push/PushPlugin.kt: (97, 25): 'get(String!): Any?' is deprecated. Deprecated in Java
```
@Abby-Wheelis
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

4 participants