-
Notifications
You must be signed in to change notification settings - Fork 34
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
Compliance with iOS Permissions Guidelines #1094
Comments
Given that latest versions of iOS will use a two-step strategy, and latest versions of Android are also using a two-step strategy (#990, implemented in e-mission/e-mission-data-collection#210), I think the best long-term solution may be to split the "Location Permissions" item into two separate items However, for now I will just try to meet the guidelines as quickly as possible so we are unblocked |
Yes. We used to use the standard popups, but then people would not give us the correct permissions and then complain that "the app does not work" |
@catarial is going to help verify
To do that, we need to turn off the notification permission and see if we still get the silent push notifications once an hour. So I would suggest two potential ways to check this: Option 1:
Option 2: The set of checks for validity are: So to hack, we can just change With either option, we should be able to see whether turning off notifications affects silent push notifications. So we know/can see that we get silent push when the notification permission is present; do we get them when the permission is off. |
I left the simulator running with the notification check disabled for about 2 hours and the only thing I saw in the debug logs was one instance of "Ignoring silent push notification". |
Im getting the same thing when I turn notifications back on. I need test this more. What makes this most challenging is that I have to disable push notifications to be able to install the test app on a real phone. |
I'll try option 1 with the app store version on a real phone and see what happens tomorrow. |
@catarial push notifications are not delivered to the simulator (you may see the notification about that when you start up the app). You have to try it on a physical phone. |
So I didn't see any silent push notifications, even when notifications were turned on. It seems like the app stopped logging when I turned location off. Do I have to keep the phone unlocked or the app open for it to log? |
This is when I turned notifications off
Either it doesn't log in the background or it doesn't log if it doesn't have all the required permissions |
I have the majority of these changes done now. I implemented the two-step approach for location permissions on iOS versions >=13.4. iOS 13.0 to 13.3 is still using the old flow – hopefully that is not a problem. I am still running into one issue where after granting location permission via popups, the UI does not update with a green checkmark until clicked again or "refresh" is clicked. Making the notifications optional was mainly just a UI change in e-mission-phone. Added an optional flag to the checks and allow I adjusted text in several places, a few examples:
I also found that the location permissions popups were displaying text related to iBeacons (e.g. "This app would like to scan for iBeacons while it is in use.") Those strings aren't present in any e-mission codebase. We actually have our own strings for those descriptions: https://github.com/e-mission/e-mission-data-collection/blob/dcc4853919089527427c444260f63a49dca66af3/plugin.xml#L214-L224 But they were being overridden by the descriptions from cordova-plugin-ibeacon: https://github.com/petermetz/cordova-plugin-ibeacon/blob/270ffbbc12159861a16e5e81481103c1e09139cb/plugin.xml#L61-L72 Hopefully adjusting the order that the plugins are listed in package.cordovabuild.json will prevent this override. |
This worked, but I can't find internationalized versions of these strings. Are we missing translations?? |
@catarial wrt #1094 (comment), do you see push notifications in the logs before you made the permission changes? @JGreenlee wrt #1094 (comment), it is possible. We were really scrambling to get the beta version out ASAP. |
no, I'm retesting with the phone on and the app open |
e-mission/e-mission-docs#1094 (comment) If on iOS 13.4 or higher, we will first ask for 'whenInUse' authorization. If the user accepts this, we will receive the authorization status change in didChangeAuthorizationStatus; at which point we will attempt to trigger another request, this time for 'always', which should show the user a second prompt. If that fails, we will navigate to the app settings. This approach involved re-registering the foreground delegate from within that foreground delegate's handler, so I adjusted the way the delegate list is cleared, so as to not clear out the new additions. Also adjusted the strings in plugin.xml to be more descriptive and transparent about what permissions are needed for tracking, as well as when and why.
Note (for your learning) that this again highlights why we need the baseline. If we had tested only with notification permission off, we may have concluded that no permission = no notifications. But since we never saw notifications, we know that there is something more fundamentally incorrect. |
I tried to test this out on my own physical phone, and I am not seeing silent push notifications since Sept 12, even for an app installed using TestFlight
|
Checking the logs, that is because the previous fix to migrate to the new version of the FCM HTTP API (e-mission/e-mission-server#980) works for visible notifications, but not for silent push. Will fix that and retry this weekend.
|
Testing done for e-mission/e-mission-phone#1182 and e-mission/e-mission-data-collection#237, screen recordings showing some of the scenarios tested iOS 13.3Granted location + fitness, denied notifications: Untitled.mp4iOS 15.6Granted location ("Allow While Using App" on first popup, then "Change to Always Allow" on second popup). Granted fitness. Denied notifications. Untitled2.mp4Initially chose "Just Once" for location, then fixed in app settings. Granted fitness. Denied notification. Untitled1.mp4Initially denied location, then fixed in app settings. Initially denied fitness, then fixed in app settings. Granted notifications. Untitled3.mp4After taking these I realized that the location permissions popups do not actually show up in the screen recordings. But I hope you can tell what's happening by how I have labeled the scenarios |
The one thing I would flag as problematic is in the last scenario, where fitness is denied and then fixed in app settings. When returning to the app, it appeared to restart so I had to go back through the privacy policy. I was ultimately able to get through onboarding, but it was a bit annoying and might signify an issue in the native code that caused a crash. I don't think it was a result of my changes. |
I fixed the silent push issue in e-mission/e-mission-server#987 (edit: validation in the staging environment is at e-mission/e-mission-server#987 (comment)) although it needs some additional investigation on why it only occurs on certain deployments. Now we can return to this verification:
|
[1] FSM gets stuck in `WAITING_FOR_TRIP_STATE if settings are wrong
[2] Receiving push notification when notification permissions are turned off
[3] Not sure where the 19:06 is from, but otherwise, we seem to get values fairly consistently on the hour
I hearby declare this investigation done. We do continue to get silent push notifications even when the notification permission is turned off; that seems to mainly affect the visibility of notifications@JGreenlee has already made the notification permission optional in the UI. The next steps are to:
@catarial can you get these changes done ASAP? We should then investigate and fix [1], but that is not a showstopper. |
Is this necessary to unblock the release? If the user denies the notification permission, they won't see any "fix" prompts, right? How are users being "annoyed" in this scenario? |
Good point. They won't see any "fix" prompts since notifications are turned off. Couple of things to check;
Given that they are not launching the app through the notification, I assume the redirection will not happen, but we should test.
The permission checks are also modular, so the change is super simple. If @catarial can't get to it in the next couple of hours, I can just make the change, and push a release out to staging for testing. |
The toast is from the UI (it's a SnackBar) |
…hecker As part of e-mission/e-mission-docs#1094, we made the notification permission optional. Since it is not required for silent push notifications, notifications are nice-to-have. They are still helpful to ensure that people know if there are issues, and hopefully help them label their trips, but the app will still work if the permission is turned off. Since this is no longer required for normal operation, we don't need to check it in the background every hour, and nag the user to fix it if it is turned off. This is a super-easy change (two lines), so rolling this in while fixing e-mission/e-mission-docs#1094 instead of coming in as a future fix.
…hecker As part of e-mission/e-mission-docs#1094, we made the notification permission optional. Since it is not required for silent push notifications, notifications are nice-to-have. They are still helpful to ensure that people know if there are issues, and hopefully help them label their trips, but the app will still work if the permission is turned off. Since this is no longer required for normal operation, we don't need to check it in the background every hour, and nag the user to fix it if it is turned off. This is a super-easy change (two lines), so rolling this in while fixing e-mission/e-mission-docs#1094 instead of coming in as a future fix. Testing done: - Replaced the data collection plugin with the most recent version - Code compiles - Wasn't able to test in the emulator since with the permission turned off, without e-mission/e-mission-phone#1182 in place, we got a prompt to fix the notification, so we couldn't simulate a remote push Pushing this and testing all the changes together
Pulled and verified that:
I do think this is something that needs to be fixed, but it is a corner case, so we can handle it in the point release. |
Here's the weird behavior with the popup. I think it only happens in the upgrade scenario - where the user had originally installed the app with the old onboarding flow, and then the app upgraded and they went and turned off the notification permission, and then launched the app. I have also not tested similar scenarios on android. |
Android seems to be similar to iOS; shows the notification if the app was upgraded but then the permission turned off, but not on subsequent launches Screen.Recording.2024-10-08.at.11.51.18.PM.mov |
Transferring discussion from Teams @JGreenlee Even in the new onboarding flow, we would have requested the notification permission from the user, right?
we already have a flag for "gone through onboarding" though, which is "isConsented"
the advantage of this is that when we migrate to @JGreenlee implemented this in e-mission/e-mission-data-collection#241 |
I think we will still need the change for setting HasRequestedNotificationPermission if isConsentedbecause otherwise the following case will break:
This is a two line change, so I am going to make that change. |
If it is not set and the user has consented This fixes e-mission/e-mission-docs#1094 (comment)
That seems to have fixed it. I tried a couple of upgrade scenarios and they all worked. |
Just tested on a short walk with notifications off. It was able to go from ONGOING_TRIP to WAITING_FOR_TRIP_START, but I'm not seeing the trip in the UI after force pushing. |
It shows up now, I think it's just a delay with the server processing the trip. |
This has been accepted by app store review now, so closing this issue. |
FCM is doubling down on the "I'm going to change my API and break everything" approach. We made one round of fixes in: e-mission/e-mission-docs#1094 (comment) at which time the mapping to convert APNS tokens to FCM was working However, in the ~ 2 months since, that has also regressed, and we are now getting a 401 error with the old code. The new requirements include: - using an OAuth2 token instead of the server API key - passing in `"access_token_auth": "true"` as a header We already use an OAuth2 token to log in and actually send the messages ``` DEBUG:google.auth.transport.requests:Making request: POST https://oauth2.googleapis.com/token DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): oauth2.googleapis.com:443 DEBUG:urllib3.connectionpool:https://oauth2.googleapis.com:443 "POST /token HTTP/1.1" 200 None DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): fcm.googleapis.com:443 ``` So it seems like it would be best to just reuse it for this call as well. However, that token is retrieved from within the pyfcm library and is not easily exposed outside the library. Instead of retrieving the token, this change retrieves the entire authorization header. This header includes the token, but is also formatted correctly with the `Bearer` prefix and is accessible through the `requests_session` property. With this change, the mapping is successful and both silent and visible push notification are sent to iOS phones. Before the change: ``` DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): iid.googleapis.com:443 DEBUG:urllib3.connectionpool:https://iid.googleapis.com:443 "POST /iid/v1:batchImport HTTP/1.1" 401 None DEBUG:root:Response = <Response [401]> Received invalid result for batch starting at = 0 after mapping iOS tokens, imported 0 -> processed 0 ``` After the change ``` DEBUG:root:Reading existing headers from current session {'User-Agent': 'python-requests/2.28.2', 'Accept-Encoding': 'gzip, deflate, br', 'Accept': '*/*', 'Connection': 'keep-alive', 'Content-Type': 'application/json', 'Authorization': 'Bearer ...'} DEBUG:root:About to send message {'application': 'gov.nrel.cims.openpath', 'sandbox': False, 'apns_tokens': [.... DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): iid.googleapis.com:443 DEBUG:urllib3.connectionpool:https://iid.googleapis.com:443 "POST /iid/v1:batchImport HTTP/1.1" 200 None DEBUG:root:Response = <Response [200]> DEBUG:root:Found firebase mapping from ... at index 0 DEBUG:root:Found firebase mapping from ... at index 1 DEBUG:root:Found firebase mapping from ... at index 2 ... ``` Visible push ``` ... s see if the fix actually worked" -e nrelop_open-access_default_1hITb1CUmGT4iNqUgnifhDreySbQUrtP WARNING:root:Push configured for app gov.nrel.cims.openpath using platform firebase with token AAAAsojuOg... of length 152 after mapping iOS tokens, imported 0 -> processed 0 combo token map has 1 ios entries and 0 android entries {'success': 0, 'failure': 0, 'results': {}} Successfully sent to cK0jHHKUjS... {'success': 1, 'failure': 0, 'results': {'cK0jHHKUjS': 'projects/nrel-openpath/messages/1734384976007500'}} ```
Our current permissions flow is deemed unacceptable. I think this is due to:
And perhaps a third point:
I am working on the changes to resolve these issues, and hopefully provide a better onboarding UX in the process.
The text was updated successfully, but these errors were encountered: