-
Notifications
You must be signed in to change notification settings - Fork 197
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
setLocationShared Not working #406
Comments
I cannot get Xcode device logs, I think that is only populated if there is a crash? But here is some more logging from the Xcode console with logLevel set to 6.
Curious, if I don't call I'm a bit confused if these functions are supposed to be called between |
I have found the problem! It's a bug in the https://github.com/OneSignal/OneSignal-Cordova-SDK/blob/master/www/OneSignal.js#L229-L235 The
I tested this fix on Android and Unfortunately on iOS the location permission dialog still pops up!!! Which is what i'm trying to avoid. There is no longer an error in the console logs. But the users location is still saved to the OneSignal Dashboard, so it appears Here is some logs from iOS after fixing the
|
@canadaka Good catch, the callback was indeed missing. Unfortunately this project has never had any tests so it's difficult for us to catch stuff like this. Otherwise, note that |
I have tried before and after, neither have any affect on iOS. The location permission still pops up and the users lat/lng location is still saved. |
@canadaka Can you try this: remove/uninstall our SDK from your project and let me know if your app still immediately prompts the user for location permission. There are some plugins that will automatically prompt for location permission, so we'll want to make sure. Otherwise: please make sure that Also, please note that calling |
There is only two ways our SDK will send location data:
Our docs do not mention this, so I've updated the |
Thanks for the responses.
I just installed the Cordova demo app and no location prompt appeared. So it seems it's a conflict when the app has other plugins requesting permission. Notice the difference in the dialog from my app to the demo one, no do not allow. |
I just thought of something. My app via the But maybe OneSignal is checking/requesting for the Always Allow location permission and that's why the prompt comes up. |
@canadaka Even if I install both our plugin and If you keep OneSignal in your project but remove the |
Slight tangent from the iOS issues. So we've had the OneSignal plugin installed on our Android app the past week and users locations (32k) are being saved to the Dashboard, even though I set
I think if the app already has location permission via another plugin, Onesignal just goes ahead and saves users locations with their Onesignal data? Not what I'd expect |
I can confirm that location data is definitly being shared as described above (although works as expected on Android). This would suggest that the iOS code is ignoring the value sent to setLocationShared. We are in urgent need of this fixing, are you able to advise on how long until a new release is available? Thanks, |
Hi @Dave-Osborne we recently fixed a bug in the @canadaka I’ll Be investigating this one android today |
Ok after some more debugging I think I found a way to fix the issue of the location permission prompt from appearing on an app upgrade. If our app's plist file contains We removed Doing some more testing, to confirm 100% but looks promising. |
We have the plugin running on a Testflight iOS version, and even with setLocationShared = false, our locations are being saved. What we would really want, is first for the |
@canadaka @Dave-Osborne I still can't reproduce this issue, even when I add Are you absolutely certain that your app never calls Even better - can you modify our Cordova plugin |
On both iOS and Android, the app already has location permission from the OS. Our app is pretty much useless without this permission, so users will have granted our app this permission before we add OneSignal. We fixed the issue with the app asking for location permission a 2nd time here #406 (comment) Now the outstanding issue is a users location is still saved even if |
@canadaka I see what you're saying. I was under the impression we were still talking about the SDK inappropriately requesting location permission. I have done some testing and I am definitely able to reproduce this issue. It looks like the SDK is inappropriately sending the location even when This is an important issue - I'll be updating our SDK and releasing an update within the next 24 hours. I'll also add some tests to ensure it doesn't happen going forward. Thanks for bringing this to our attention! |
As an update, we just fixed the issue (it was actually an issue with our native iOS SDK) and we'll be releasing a Cordova SDK update to fix this issue shortly, here is the PR |
This issue should now be resolved in the new update (2.4.3). We've made a minor fix in iOS to resolve this issue, however this week we intend to make more comprehensive changes to the SDK to ensure location data can never be sent if |
So the solution we did to stop our app for asking for location permission a 2nd time, looks like it might not work in future.
We fixed the permission ask by removing any |
Description:
As I also mentioned here #173 (comment) the
setLocationShared
function does not work.On iOS it just has no effect, location permission dialog still appears and users location still recorded. On Android I get an error, depending on how I use it.
If I call the function like this, I get an error.
Android Error:
If I instead call the function after separately, I don't get that error, but my location is still recorded, when I look at the OneSignal users dashboard. I deleted all users before building the app.
window.plugins.OneSignal.setLocationShared(false);
However calling the
setLocationShared
function separately like above on iOS I get the following error in the Xcode log.Environment
Plugin: 2.4.1
Cordova: 7.1.0
Ionic CLI Version: 2.2.3
Ionic App Lib Version: 2.2.1
The text was updated successfully, but these errors were encountered: