-
Notifications
You must be signed in to change notification settings - Fork 19
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
🍎 iOS Permissions Changes to Comply with iOS Guidelines #237
Conversation
If the user denies notifications, we are required to respect their choice, so we should not direct them to the app settings page after they decline, unless they trigger it again later. We should also update the text to reflect that notifications are not strictly required
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.
We want to give the user the option to revise their choice after their initial choice. We can only show the prompt once, so if the user triggers prompt again, we should go to app settings instead of doing nothing. Keeps track of whether prompt already happened by storing a bool in standardUserDefaults
Updated 2 strings to be more descriptive and removed a third string which is not used anywhere
} else { | ||
NSLog(@"iOS <13 detected, simply requesting 'always'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we do not support iOS <13 anymore, so maybe this should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, we can remove that in a cleanup change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once we revert the "OpenPATH" changes.
plugin.xml
Outdated
@@ -212,27 +212,27 @@ | |||
</config-file> | |||
|
|||
<config-file target="*-Info.plist" parent="NSLocationAlwaysAndWhenInUseUsageDescription"> | |||
<string>We use your data to create an automatic trip diary</string> | |||
<string>OpenPATH requires "Precise" location access allowed "Always" to create an automatic trip diary</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had this as "We" so that community apps such as Ma Mobilitie and FourStep wouldn't have to edit the plugin to work. Let's revert these and figure out the modified text in yet another cleanup change..
<string name="location_permission_off">Insufficient location permissions, please fix</string> | ||
<string name="location_permission_off_app_open">Insufficient location permissions, please fix in app settings</string> | ||
<string name="location_permission_off_enable">Location permission off, click to enable</string> | ||
<string name="location_permission_off">Insufficient permissions, please allow 'Always' and 'Precise' location access</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had left this vague because it felt like iOS kept adding new permissions and I didn't want to keep changing the plugin. But they haven't changed it for a while, and there are others who now have confidence in changing the native code, so this is fine.
I edited the descriptions to make them not specific to OpenPATH. |
As described in e-mission/e-mission-docs#1094