e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 32 forks source link

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

Open shankari opened 12 months ago

shankari commented 12 months ago

We are currently at API 32 (upgraded in https://github.com/e-mission/e-mission-docs/issues/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

niccolopaganini commented 11 months ago

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 commented 11 months ago

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

shankari commented 11 months ago

@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

niccolopaganini commented 11 months ago

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

niccolopaganini commented 11 months ago

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

  1. cordova-plugin-em-opcodeauth
  2. cordova-plugin-em-server-communication
  3. cordova-plugin-em-serversync
  4. cordova-plugin-em-settings
  5. cordova-plugin-em-unifiedlogger
  6. 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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

@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 Cordovaplugin
    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 commented 11 months ago

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

Cordovaplugin

  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 commented 11 months ago

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

(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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 https://github.com/e-mission/e-mission-docs/issues/934#issuecomment-1663340939. 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 commented 11 months ago
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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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](https://github.com/e-mission/e-mission-docs/assets/69108657/772ef4c5-ae77-4964-b2ab-09639dcd73d7) 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](https://github.com/e-mission/e-mission-docs/assets/69108657/aeea0242-cede-49cf-9f62-3cd234946237) 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](https://github.com/e-mission/e-mission-docs/assets/69108657/715e1df0-faaf-41b4-b054-5cd007bdb379) ##### After: ![Screen Shot 2023-08-14 at 4 28 45 PM](https://github.com/e-mission/e-mission-docs/assets/69108657/1b282928-7663-49cb-ac66-8e2cac1997d3) 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](https://github.com/e-mission/e-mission-docs/assets/69108657/85df19f4-20ad-46e1-b75b-4a2ab8b0b7ca) ### 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 commented 10 months ago

@niccolopaganini high level comments:

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 commented 10 months ago

@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 commented 10 months ago

Looking through the list of behavior changes:

shankari commented 10 months ago

Verifications:

    <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 commented 10 months ago

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:

shankari commented 10 months ago

Follow-ups:

commit 6de1f4cd26ce79f1326f8a3c0f861c1385c67466
Author: K. Shankari <shankari@eecs.berkeley.edu>
Date:   Sat Mar 11 17:03:16 2023 -0800

    Bump up versions of the dev chain and all other plugins

and we were already at minSDK = 23 before we set it in the config file.

Concretely:

we can just follow cordova's lead on the min API version

The WRITE_EXTERNAL_STORAGE is currently added from the following plugins

$ grep -rl WRITE_EXTERNAL_STORAGE plugins/
plugins//cordova-plugin-file/plugin.xml
plugins//cordova-plugin-file/README.md
plugins//cordova-plugin-file/src/android/FileUtils.java
plugins//cordova-plugin-x-socialsharing/plugin.xml
plugins//cordova-plugin-x-socialsharing/README.md

cordova has already updated the file plugin (that's where we saw this message). what about the x-socialsharing plugin?

shankari commented 10 months ago

The x-socialsharing plugin does also appear to use getExternalFilesDir. But we should see if there is a new version

  private String getDownloadDir() throws IOException {
    // better check, otherwise it may crash the app
    if (Environment.MEDIA_MOUNTED.equals(Environment.getExternalStorageState())) {
      // we need to use external storage since we need to share to another app
      final String dir = webView.getContext().getExternalFilesDir(null) + "/socialsharing-downloads";
      createOrCleanDir(dir);
      return dir;
    } else {
      return null;
    }
  }
shankari commented 10 months ago

The most recent version of the plugin still includes WRITE_EXTERNAL_STORAGE https://github.com/search?q=repo%3AEddyVerbruggen%2FSocialSharing-PhoneGap-Plugin%20WRITE_EXTERNAL_STORAGE&type=code

Looks like it is only required for image sharing, but it used to be required for it https://github.com/EddyVerbruggen/SocialSharing-PhoneGap-Plugin/issues/86 https://github.com/EddyVerbruggen/SocialSharing-PhoneGap-Plugin/issues/112

However, we do share images (notably the QR code for the opcode). But this is working now, although the permission is apparently a NOP on android 11+

So we might want to test with it and either create our own fork or use a new fork that does not have the permission

shankari commented 10 months ago

So a summary of the initial tasks is:

Making the changes now...

shankari commented 10 months ago

Another note before I forget: we need to make sure that the instructions for all the settings are correct for Android 13 (API 33). This is currently done in many locations in this file

https://github.com/e-mission/e-mission-phone/blob/e91c8d05a5e84cd1a538a5e76dbb5515fba748cc/www/js/appstatus/permissioncheck.js#L180

although @Abby-Wheelis is actively working on migrating it 😄

I have a report from a user that the instructions, particularly for the unused apps, was different on Android 13 (this is https://github.com/e-mission/e-mission-phone/blob/e91c8d05a5e84cd1a538a5e76dbb5515fba748cc/www/js/appstatus/permissioncheck.js#L352)

We have also heard feedback that the app optimizations text is confusing.

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

JGreenlee commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Video of location permission popup on android 7 😄

https://github.com/e-mission/e-mission-docs/assets/2423263/9a336f18-2b10-4dd7-a5a7-26ff5b4a796e

JGreenlee commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

First level of upgrade: to e-mission-data-collection: https://github.com/e-mission/e-mission-data-collection/pull/207

shankari commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago
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